cyberchitta / openai_ex

Community maintained Elixir library for OpenAI API
https://hexdocs.pm/openai_ex
Apache License 2.0
132 stars 18 forks source link

Error Exposes API Key in Log #104

Open daniellionel01 opened 2 days ago

daniellionel01 commented 2 days ago

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 commented 2 days ago

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

restlessronin commented 2 days ago

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?

daniellionel01 commented 2 days ago

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 commented 1 day ago

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 commented 1 day ago

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!