Skip to content

fix(deps): revert joserfc JWT error migration — fastmcp still uses authlib#40688

Merged
aminghadersohi merged 3 commits into
apache:masterfrom
aminghadersohi:revert-joserfc-jwt-errors
Jun 2, 2026
Merged

fix(deps): revert joserfc JWT error migration — fastmcp still uses authlib#40688
aminghadersohi merged 3 commits into
apache:masterfrom
aminghadersohi:revert-joserfc-jwt-errors

Conversation

@aminghadersohi
Copy link
Copy Markdown
Contributor

Summary

Reverts #40582 (chore(deps): migrate MCP service JWT errors from authlib.jose to joserfc).

The migration was premature. fastmcp through at least version 3.3.1 still uses authlib internally — JWTVerifier.__init__ sets self.jwt = JsonWebToken([self.algorithm]) from authlib.jose. The Superset DetailedJWTVerifier inherits this and calls self.jwt.decode(), which raises authlib.jose.errors.* exceptions.

After #40582, the catch blocks in DetailedJWTVerifier.load_access_token() were updated to catch joserfc.errors.* classes, which have a completely separate class hierarchy (authlib.jose.errors.JoseError → AuthlibBaseError → Exception vs joserfc.errors.JoseError → Exception). The authlib exceptions are not caught, propagate through Starlette's AuthenticationMiddleware (which only catches AuthenticationError), and produce 500 Internal Server Error instead of 401 for every invalid or expired token.

The unit tests in #40582 did not catch this because they mock self.jwt.decode with side_effect=joserfc.errors.BadSignatureError() — the mock raises a joserfc exception that IS caught. The real authlib object raises authlib exceptions.

Changes

  • Reverts superset/mcp_service/jwt_verifier.py: restores from authlib.jose.errors import; restores original import order
  • Reverts tests/unit_tests/mcp_service/test_jwt_verifier.py: restores authlib-compatible constructor calls (BadSignatureError(result=None), ExpiredTokenError())
  • Reverts pyproject.toml: removes joserfc>=1.0.0,<2.0 from fastmcp extras
  • Reverts requirements/development.txt: removes joserfc==1.6.8 pin

When to re-do the migration

When a version of fastmcp within the >=3.2.4,<4.0 constraint replaces authlib.jose.JsonWebToken with a joserfc-based JWT object internally, the migration can be re-applied. At that point self.jwt.decode() will raise joserfc exceptions and the catch blocks will match.

🤖 Generated with Claude Code

@aminghadersohi aminghadersohi marked this pull request as ready for review June 2, 2026 16:58
@aminghadersohi aminghadersohi changed the title revert(deps): revert joserfc JWT error migration — fastmcp still uses authlib fix(deps): revert joserfc JWT error migration — fastmcp still uses authlib Jun 2, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 2, 2026

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 63.97%. Comparing base (242c27a) to head (61cffa6).
⚠️ Report is 4 commits behind head on master.

Files with missing lines Patch % Lines
superset/mcp_service/jwt_verifier.py 0.00% 3 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #40688   +/-   ##
=======================================
  Coverage   63.97%   63.97%           
=======================================
  Files        2661     2661           
  Lines      143136   143136           
  Branches    32909    32909           
=======================================
  Hits        91569    91569           
  Misses      49999    49999           
  Partials     1568     1568           
Flag Coverage Δ
hive 39.75% <0.00%> (ø)
mysql 58.38% <0.00%> (ø)
postgres 58.45% <0.00%> (ø)
presto 41.35% <0.00%> (ø)
python 59.94% <0.00%> (ø)
sqlite 58.11% <0.00%> (ø)
unit 100.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@aminghadersohi aminghadersohi merged commit 616c243 into apache:master Jun 2, 2026
60 of 66 checks passed
@bito-code-review
Copy link
Copy Markdown
Contributor

Bito Automatic Review Skipped – PR Already Merged

Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.
No action is needed if you didn't intend to review it. To get a review, you can type /review in a comment and save it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants