ueberauth / guardian

Elixir Authentication
MIT License
3.43k stars 379 forks source link

header_name should always be downcase #727

Closed gabeio closed 6 months ago

gabeio commented 6 months ago

get_req_header will never match for uppercase headers.

If a header_name is provided and is uppercase it will never match the header. Instead of simply documenting it, we should just assert that the header we are looking for is always downcase to prevent footguns.

yordis commented 6 months ago

Hey there, thank you very much for your contribution!

Have you try to test what would happen if you pass a header with uppercase letter without your changes?

gabeio commented 6 months ago

https://github.com/elixir-plug/plug/blob/v1.15.3/lib/plug/conn.ex#L10-L11

Plug already normalizes the header name to lowercase so by not forcing the lowercase when passing it in for the get_req_header we have potential to fail even though the header should have matched but since we are passing a potentially uppercase header it will not find the auth header.

yordis commented 6 months ago

Hey @gabeio,

Hope you're doing great! It's an interesting point because, within our ecosystem, Plug hasn't set a precedent for handling header casing. In my view, maybe Guardian should follow suit and not take on this responsibility either. This part of the setup is more or less set, and forget it, right? So, with thorough testing, we should be covered.

I see where you're coming from and acknowledge it's a potential hiccup. However, it hasn't popped up as a big issue so far. I'm hesitant to dive into defensive programming for something that isn’t a widespread problem, especially if Plug isn't going down that road either.

I'm not saying you're off base—I'm just offering another perspective. It's always great to discuss these things together. What do you think?

gabeio commented 6 months ago

That's fair then, I'm new to the elixir scene. Then it might be best if we change this to documentation instead and just hint that the system which handles headers might automatically lowercase the headers and that the user will need to determine that on their own.