fix(gotrue): fix getClaims JWT token decoding #1288
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in GoTrueClient.getClaims() where JWT token decoding fails with a null pointer exception when verifying JWTs signed with asymmetric algorithms (RS256, ES256). The fix properly handles the case where _jwks is not yet initialized by providing an empty fallback, and replaces the JWT verification implementation with jose_plus library for proper asymmetric key support.
Key Changes:
- Fixed null safety issue in
getClaims()by providing an emptyJWKSetfallback when_jwksis null - Replaced
dart_jsonwebtokenwithjose_plusfor JWT verification to properly support asymmetric algorithms - Added comprehensive test coverage for JWKS-based verification path with support for both symmetric (HS256) and asymmetric (ES256/RS256) JWT signing
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/gotrue/lib/src/gotrue_client.dart | Fixed null pointer exception by providing fallback for _jwks and implemented JWT verification using jose_plus library |
| packages/gotrue/lib/src/types/jwt.dart | Removed dart_jsonwebtoken dependency and the unused rsaPublicKey getter from JWK class |
| packages/gotrue/pubspec.yaml | Added jose_plus as a production dependency and moved dart_jsonwebtoken to dev dependencies |
| packages/gotrue/test/utils.dart | Added support for generating service role tokens with both symmetric and asymmetric signing algorithms |
| packages/gotrue/test/get_claims_test.dart | Added test case to verify JWKS-based JWT verification for asymmetric algorithms |
| packages/gotrue/test/src/gotrue_admin_oauth_api_test.dart | Refactored to use centralized getServiceRoleToken() utility function |
| packages/gotrue/test/src/gotrue_admin_mfa_api_test.dart | Refactored to use centralized getServiceRoleToken() utility function |
| packages/gotrue/.env.example | Added example environment configuration for both symmetric and asymmetric JWT signing |
| infra/gotrue/docker-compose.jwk.yml | Added docker-compose override file for testing with JWKS/asymmetric JWT signing |
| .github/workflows/gotrue.yml | Added new test-jwks job to run tests with JWKS-enabled configuration |
| .github/workflows/dart-package-test.yml | Enhanced to support multiple docker-compose files for more flexible test configurations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@khensunny thanks for opening this PR, I'll review and provide any feedback if needed. |
khensunny
force-pushed
the
fix-getclaims-token-decoding
branch
from
7e22b98 to
4db3241
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
khensunny
force-pushed
the
fix-getclaims-token-decoding
branch
from
4db3241 to
c20ec04
Compare
- update tests - add jose_plus package - update CI and docker infra
khensunny
force-pushed
the
fix-getclaims-token-decoding
branch
from
c20ec04 to
e3898c7
Compare
Pull Request Test Coverage Report for Build 20285703210Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
|
||
| try { | ||
| JWT.verify(token, signingKey.rsaPublicKey); | ||
| final keyStore = jose.JsonWebKeyStore(); |
There was a problem hiding this comment.
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.
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:
in supabase-js.
To do it manually we'd need this:
RSAPublicKey _jwkToRSAPublicKey(Map<String, dynamic> jwk) {
final n = _base64UrlToBigInt(jwk['n']);
final e = _base64UrlToBigInt(jwk['e']);
return RSAPublicKey(n, e);
}
BigInt _base64UrlToBigInt(String value) {
final bytes = base64Url.decode(base64Url.normalize(value));
return BigInt.parse(
bytes.map((b) => b.toRadixString(16).padLeft(2, '0')).join(),
radix: 16,
);
}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)
| expect(claimsResponse.claims.claims['email'], newEmail); | ||
| }); | ||
|
|
||
| test('getClaims() verifies asymmetric JWT via JWKS (ES*/RS*)', () async { |
There was a problem hiding this comment.
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?
4 participants
What kind of change does this PR introduce?
Bug fix (fixes
GoTrueClient.getClaims()JWT decoding for asymmetric algorithms), plus tests, CI/workflow updates, package/pubspec adjustments, and infra (JWKS docker-compose) additions.What is the current behavior?
getClaims()can throwDartError: Unexpected null value.when verifying JWTs signed with asymmetric algorithms (e.g.,RS256,ES256) because_jwksis force-unwrapped before it is initialized — see #1286. CI/tests may not surface this because the local test auth uses HS* signing.What is the new behavior?
getClaims()no longer force-unwraps_jwks; the JWKS path is handled safely by fetching and caching/.well-known/jwks.jsonon first use (or falling back appropriately), so asymmetric (RS*/ES*) JWTs withkidare verified without crashing. Tests updated to cover the JWKS path; CI/workflows, gotrue pubspec, and docker-compose.jwk.yml were updated to support and validate the fix.Feel free to include screenshots if it includes visual changes.
Additional context
Add any other context or screenshots.