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.

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

Error Exposes API Key in Log #104

Open
daniellionel01 opened this issue Oct 9, 2024 · 5 comments
Open

Error Exposes API Key in Log #104

daniellionel01 opened this issue Oct 9, 2024 · 5 comments

Comments

@daniellionel01
Copy link

daniellionel01 commented Oct 9, 2024

Describe the bug

Used this library in my app on Stream. There was an error in the HTTP request and it printed out the full request with headers and also the API Key.

Not a big deal in my case since I quickly generated a new key, but I'm sure there's a better way to do this.

Maybe providing an extra argument like "hide_key" could be implemented that modifies the request in such a way to hide sensitive data.

Willing to create a PR and do this on my own, but just wanted to get some input first.

Thank you :)

To Reproduce

/

Code snippets

No response

OS

macOS

Elixir version

Elixir 1.17.3

Library version

openai_ex 0.8.3

@daniellionel01 daniellionel01 added the bug Something isn't working label Oct 9, 2024
@daniellionel01
Copy link
Author

Sorry I think I should've labelled this as a Feature Request, can't seem to change it though

@daniellionel01 daniellionel01 changed the title Error exposes Headers & API Key Error Exposes Headers & API Key Oct 9, 2024
@restlessronin
Copy link
Member

restlessronin commented Oct 9, 2024

I'm not sure I understand the situation. Could you show the code snippet that is doing this. The library itself only returns values and throws / logs errors.

If it's a phoenix app, the full error (as well as the stack I believe) is visible on the error page in debug mode (but not in production mode). Is that what you're referring to?

@restlessronin restlessronin added enhancement New feature or request and removed bug Something isn't working enhancement New feature or request labels Oct 9, 2024
@daniellionel01
Copy link
Author

daniellionel01 commented Oct 9, 2024

Yeah I didn't think about Phoenix. It does indeed print out more information about the request when you use Phoenix.

However the openai_ex error, as seen below, still prints out the full api key. If I have a typo in there or you have a drop in your connection (2nd screenshot) and it doesn't work because of that, it just drops your API key right in the terminal. And since I live stream some work with Elixir, that's pretty bad. Or if I deployed this to production and was collecting logs, the raw key would sit there in the logs.

So I would definitely reduce the error message to just "invalid API key" and strip the Authorization header from the printed request.

But maybe it's too niche of an issue and I should just keep the terminal with the running phoenix server on the other monitor 😄 Since even if you'd remove those things, Phoenix would still most likely print out the full request (I assume)

WhatsApp Image 2024-10-09 at 19 47 06

WhatsApp Image 2024-10-09 at 19 47 06 (1)

@restlessronin
Copy link
Member

Hi @daniellionel01

Thanks for the detail.

Case 1 is an authentication error, coming from the OpenAI server, and in this case, we really do need to see the API key, so we can debug the issue.

Case 2 is a Finch error (networking error), and in this case the API key is getting dumped as a side effect of printing all the headers.

In both cases, you should be able to stop the automatic output of the error by catching and handling the exception.

Redacting the API key from the log, however, is something that we should do. I'm not an expert, but I think this is something that should be set up through logger configuration rather than by filtering the API outputs. Let me have a think and look into this. Be happy to hear your thoughts as well.

In any case, I think we need to handle the :nxdomain Finch Error, so there definitely needs to be a PR for that.

@daniellionel01
Copy link
Author

Yeah makes sense for case 1 & 2 👍 I'll handle the error and that'll be cool.

Regarding the logging I don't have experience with that yet.
But I'll take a crack at it tomorrow in my stream. I saw that there's a way you can hook into the logger in Phoenix, and maybe even the lower level Elixir Logger, and add custom logic. I'll keep you posted 💪

Thanks!

@restlessronin restlessronin changed the title Error Exposes Headers & API Key Error Exposes API Key in Log Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants