-
-
Notifications
You must be signed in to change notification settings - Fork 271
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
base: master
Are you sure you want to change the base?
Conversation
Pull Request Test Coverage Report for Build 11540641030Details
💛 - Coveralls |
7563e9a
to
a70f6cf
Compare
a70f6cf
to
6193a96
Compare
There was a problem hiding this 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:
- Users don't need to upgrade their
supabase-js
version to use the new API keys - 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 theapikey
header.
if (headers.authorization && headers.Authorization) { | ||
console.warn( | ||
'@supabase-js: Both `authorization` and `Authorization` headers specified in createClient options. `Authorization` will be used.' | ||
) |
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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:
createClient()
with a non-JWT Supabase Key will not forward it as theAuthorization: Bearer <...>
header.createClient()
with global header options that contains a non-JWT in theAuthorization
header will throw an error.