-
-
Notifications
You must be signed in to change notification settings - Fork 279
fix(gotrue): fix getClaims JWT token decoding #1288
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| services: | ||
| gotrue: | ||
| # Minimal override for asymmetric JWT signing + JWKS publishing. | ||
| # Used with: docker compose -f docker-compose.yml -f docker-compose.jwk.yml up | ||
| environment: | ||
| GOTRUE_JWT_KEYS: '[{"kty":"EC","kid":"23203d4b-184b-4915-bb30-e70047967f88","use":"sig","key_ops":["sign","verify"],"alg":"ES256","ext":true,"d":"sVoSxECYxh-gfZFCYU3U8vbjH2cHSwtc4_uDmhMRIUo","crv":"P-256","x":"uXsLvkycPMsWg8v-8CGqbwhqCG9YNrlQKFyZL96puXo","y":"xGyOad6_Dg0UpiTmpdOP1kn9W8LNM3afTpqAv2ZHM8M"}]' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| # GoTrue test environment configuration | ||
| # Copy this file to .env and configure for your test setup | ||
|
|
||
| # Default GoTrue URL and anon key | ||
| GOTRUE_URL=http://localhost:9998 | ||
| GOTRUE_TOKEN=anonKey | ||
|
|
||
| # Symmetric JWT signing (HS256) - used by default docker-compose.yml | ||
| GOTRUE_JWT_SECRET=37c304f8-51aa-419a-a1af-06154e63707a | ||
|
|
||
| # Asymmetric JWT signing (ES256) - used when docker-compose.jwk.yml is active | ||
| # Uncomment this line when running tests with JWK setup: | ||
| # docker compose -f infra/gotrue/docker-compose.yml -f infra/gotrue/docker-compose.jwk.yml up | ||
| # GOTRUE_JWT_KEYS=[{"kty":"EC","kid":"23203d4b-184b-4915-bb30-e70047967f88","use":"sig","key_ops":["sign","verify"],"alg":"ES256","ext":true,"d":"sVoSxECYxh-gfZFCYU3U8vbjH2cHSwtc4_uDmhMRIUo","crv":"P-256","x":"uXsLvkycPMsWg8v-8CGqbwhqCG9YNrlQKFyZL96puXo","y":"xGyOad6_Dg0UpiTmpdOP1kn9W8LNM3afTpqAv2ZHM8M"}] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -165,6 +165,38 @@ void main() { | |
| expect(claimsResponse.claims.claims['email'], newEmail); | ||
| }); | ||
|
|
||
| test('getClaims() verifies asymmetric JWT via JWKS (ES*/RS*)', () async { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't the test here be the same as before and testing RS/ES only depends on the sign key on the server? So this test can be removed here?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! Will remove |
||
| final response = await client.signUp( | ||
| email: newEmail, | ||
| password: password, | ||
| ); | ||
|
|
||
| expect(response.session, isNotNull); | ||
| final accessToken = response.session!.accessToken; | ||
|
|
||
| final decoded = decodeJwt(accessToken); | ||
|
|
||
| // The default local test stack uses HS* signing (via GOTRUE_JWT_SECRET). | ||
| // This test is meant to exercise the JWKS verification path, so we skip | ||
| // unless the server is issuing asymmetric JWTs with a kid. | ||
| if (decoded.header.alg.startsWith('HS')) { | ||
| markTestSkipped( | ||
| 'Server is issuing HS* JWTs; Skipping Asymmetric JWKS verification test.', | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| expect(decoded.header.kid, isNotNull); | ||
|
|
||
| // First call should fetch /.well-known/jwks.json and verify. | ||
| final claimsResponse1 = await client.getClaims(accessToken); | ||
| expect(claimsResponse1.claims.claims['email'], newEmail); | ||
|
|
||
| // Second call should succeed too (exercise cached JWKS path). | ||
| final claimsResponse2 = await client.getClaims(accessToken); | ||
| expect(claimsResponse2.claims.claims['email'], newEmail); | ||
| }); | ||
|
|
||
| test('getClaims() contains all standard JWT claims', () async { | ||
| final response = await client.signUp( | ||
| email: newEmail, | ||
|
|
||
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.
I would prefer we keep using the previous library and add the ES* alg case as well. That was the reason to change this here, right?
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.
No, with json_webtoken the solution would be more involved. Though it does understand the algorithm it doesn't understand the JWK format(the response from the public key well known endpoint), that is,
this part:
https://github.com/supabase/supabase-js/blob/5c06206cc10c759382d716a6624dd6301bb3ed44/packages/core/auth-js/src/GoTrueClient.ts#L3819
in supabase-js.
To do it manually we'd need this:
I strongly suggest moving the package to jose/jose-plus because they have the features built-in and will reduce the maintenance burden especially with Supabase supporting asymmetric keys. Jose has caching builtin(I didn't use because we already have our own implementation)