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

feat: do not send non-JWTs in Authorization header #1293

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hf
Copy link
Contributor

@hf hf commented Oct 27, 2024

In preparation for the introduction of new API keys, the client library should not pass non-JWT values in the Authorization header, as requests like this will definitely fail.

Two backward compatible changes are introduced:

  • Configuring a createClient() with a non-JWT Supabase Key will not forward it as the Authorization: Bearer <...> header.
  • Configuring a createClient() with global header options that contains a non-JWT in the Authorization header will throw an error.

@coveralls
Copy link

coveralls commented Oct 27, 2024

Pull Request Test Coverage Report for Build 11540641030

Details

  • 23 of 34 (67.65%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.4%) to 64.684%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/SupabaseClient.ts 4 5 80.0%
src/lib/fetch.ts 2 4 50.0%
src/lib/helpers.ts 17 25 68.0%
Totals Coverage Status
Change from base Build 11434186924: -1.4%
Covered Lines: 120
Relevant Lines: 159

💛 - Coveralls

@hf hf force-pushed the hf/dont-send-non-jwts-in-authorization branch from 7563e9a to a70f6cf Compare October 27, 2024 12:44
@hf hf force-pushed the hf/dont-send-non-jwts-in-authorization branch from a70f6cf to 6193a96 Compare October 27, 2024 12:52
Copy link
Member

@kangmingtay kangmingtay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hf just a few things to confirm:

  1. Users don't need to upgrade their supabase-js version to use the new API keys
  2. This change just helps to ease the transition since upgrading the client lib version will provide more information about any unforseen errors. Our api-gateway already ignores checking the Authorization header and only looks at the apikey header.

Comment on lines +104 to +107
if (headers.authorization && headers.Authorization) {
console.warn(
'@supabase-js: Both `authorization` and `Authorization` headers specified in createClient options. `Authorization` will be used.'
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we initialise the headers field using the Headers interface instead since it's case-insensitive? (https://developer.mozilla.org/en-US/docs/Web/API/Headers)

return
}

if (authorization.startsWith('Bearer ') && authorization.length > 'Bearer '.length) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

authorization.length > 'Bearer '.length

not immediately obvious what this check is for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's making sure that we only check for a valid JWT if the authorization header actually has a value after "Bearer ".

It's worth noting that this check will not cover a scenario where someone only passes in "Bearer" (without a trailing space), but I'm guessing this whole scenario would be extremely rare anyway. Still, you might consider covering both.

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

Successfully merging this pull request may close these issues.

4 participants