Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Issue with potential logging of sensitive data #41

Open
jordan0day opened this issue Aug 6, 2024 · 3 comments · May be fixed by #44
Open

Issue with potential logging of sensitive data #41

jordan0day opened this issue Aug 6, 2024 · 3 comments · May be fixed by #44

Comments

@jordan0day
Copy link

Looking through application logs today, I noticed that I was seeing some log messages like:

[notice] Invalid option {token,#{'__struct__' => 'Elixir.OAuth2.AccessToken',
                        access_token =>
                            <<"...snipped...">>, ...}} ignored

and

[notice] Invalid option {client_secret,<<"...snipped...">} ignored.

It appears these log messages are coming from httpc, which is the default HTTP client used in the OAuth2.Request module (via Tesla.Adapter.Httpc).

These are cropping up as part of the handle_callback! flow, during fetch_user/2.

In fetch_user/2, it looks like we're loading up all the oauth config options (via add_oauth_options/1) and passing them through as-is ever since #23. Maybe these more-sensitive options should be getting stripped out before being passed-along in Ueberauth.Strategy.Okta.Oauth.get_user_info/2? (Or maybe opts doesn't need to be passed along at all to Client.get/4, as the opts were already used to initalize the client?)
mustela added a commit to mustela/ueberauth_okta that referenced this issue Aug 6, 2024
@mustela
Removing opts from the client
4278254 
appcues#41
@mustela
Copy link

mustela commented Aug 6, 2024

I can confirm that removing the opts here fixes the issue. I guess the goal is to be able to easily pass options to the client downstream to the Tesla adapter. However, as @jordan0day suggests, we should probably remove the unnecessary attributes. I'm happy to open a pull request if you have any thoughts.

@oliverswitzer
Copy link

Hi, wondering if we could get this fix merged in soon-ish? Can confirm this is happening for us as well and it is causing us to log our sensitive information (ie client_secret). The fix that @mustela added works when we hotfixed the ueberauth_okta dependency in our app, but would prefer not to fork this lib to address the security issue if there is already a fix here.

@coreyti
Copy link

coreyti commented Dec 16, 2024

A colleague, @slackersoft, intends to submit a PR to block a number of options currently sent to Client.get.

The issue manifests because OTP's Httpc includes a list of valid options to pass to request functions, and logs any that are provided when not in that list. A viable workaround is to configure the oauth2 library to another HTTP adapter (e.g., config :oauth2, adapter: Tesla.Adapter.Mint).

However, such a workaround leaves the door open to the possibility that things not logged in a current version of a library are logged at some later date.

@slackersoft slackersoft linked a pull request Dec 17, 2024 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@coreyti @mustela @jordan0day @oliverswitzer