Repository navigation
chore(security): migrate JWT handling from unmaintained python-jose to PyJWT - #330
Merged
Merged
Conversation
…o PyJWT python-jose has had no release since 3.5.0 (May 2025) and its maintainer has not responded to the open advisory GHSA-3qf3-8w2g-rqmx (CVE-2026-85394: HS256 algorithm confusion when a DER-encoded public key is used as the HMAC secret). Nojoin is not exposed to it: tokens are HS256 only, every keyring entry is a random symmetric secret (or SECRET_KEY), decode pins algorithms=["HS256"] and an unknown kid is refused. The motivation is maintenance and dependency hygiene: a security-critical library that no longer ships fixes, and an unfixable CRITICAL that dependency scans keep reporting. Decision record (token handling is a trust boundary): - Context: the JWT surface is small (encode with a kid header, read the unverified header for the keyring lookup, decode with a pinned algorithm), and PyJWT is already installed in the api image as a dependency of mcp. - Alternatives: keep python-jose (no upstream fixes, permanent scan noise); joserfc/Authlib (a new dependency for the same three calls). - Trade-offs: PyJWT 2.10+ validates iat (a future iat is refused, no leeway: the api process both issues and verifies every token), requires sub and jti to be strings (both always are), parses the header strictly (a non-string kid used to raise TypeError in the keyring lookup and surface as a 500; it is now a 401), and warns, without refusing, on HS256 keys shorter than 32 bytes. Generated keyring keys are 32 random bytes; a short SECRET_KEY keeps working so no deployment is locked out, and docs/SECURITY.md now says how long it should be. Tokens issued by python-jose verify unchanged, so the upgrade forces no sign-out: the new tests carry session, API and MCP tokens minted by python-jose 3.5.0 through the replaced code. deps.py catches jwt.PyJWTError, PyJWT's base class, which like python-jose's JWTError covers every decode failure, so every rejection stays a 401. requirements/base.txt drops python-jose[cryptography] (and with it ecdsa and rsa) for PyJWT==2.15.1, and pins cryptography==50.0.2: encryption.py imports it directly, but it was only ever installed transitively, declared by the python-jose extra.
…ey case The token was signed with the DER encoding of a freshly generated RSA public key that the server never sees, so it only ever tested a wrong HMAC secret. It could not exercise the python-jose advisory it was named after, which needs the verifier's own key to be a public key; Nojoin has none. Naming the advisory in a test id suggested otherwise. The case is kept as "wrong-key", signed with random bytes, because no other forged token reaches the HMAC signature check: unknown-kid stops at the keyring lookup and rs256 at the algorithm pin.
The fixtures prove PyJWT accepts tokens python-jose issued. This proves more: for the same claims, key and kid, PyJWT produces the identical token, because both libraries sort the header keys and use compact JSON separators. The wire format is unchanged, and a future PyJWT serialization change would fail here instead of surfacing as a mismatch between old and new tokens. The claims are read straight from the fixture's payload segment, so the check does not depend on PyJWT's own decode.
The docstring promised jwt.InvalidTokenError, but jwt.decode also raises jwt.InvalidKeyError when the key stored for the token's kid is unusable as an HMAC secret (empty, or shaped like a PEM or SSH key). InvalidKeyError is a PyJWTError, not an InvalidTokenError, so a future caller following the docstring would turn that case into a 500. Both existing callers are already safe: the auth dependency catches jwt.PyJWTError and logout catches Exception. The new test pins the 401 for a token whose kid maps to an empty key. python-jose accepted such a token, signed with the empty key, as valid.
An empty legacy .secret_key (a zero-byte file from a failed write or a full disk) became the active keyring entry. python-jose signed with the empty HMAC key, so every token was forgeable. PyJWT refuses an empty key with InvalidKeyError, which create_access_token does not catch: sign-in became a bare 500 and every token check a 401, with nothing pointing at the cause. Loading the keyring now refuses an empty active key, both when migrating the legacy file and when reading .secret_keys.json, with an error that names the file and says to delete it and restart to generate a new key. The api loads the keyring at the start of its lifespan instead of at the first sign-in, so the error stops startup and lands in the startup log. Nothing is written for an empty legacy file, so the restart after deleting it generates a fresh key. Decision record: - Context: the empty key was already unusable after the PyJWT migration; the question was only how it fails. - Alternatives: silently generate a fresh key (hides a disk or write problem the operator should see, and signs everyone out without a word); a logged error at the point of use (every sign-in still fails, and the message is buried among request logs). - Trade-offs: an install with an empty key no longer starts until the operator deletes one file. It could not sign anyone in before this either, and regenerating signs everyone out, which an empty key warrants. A non-active empty key is left alone: a token naming it is refused with a 401. docs/SECURITY.md and docs/DEPLOYMENT.md describe the refusal and the fix.
"32 bytes of random data" read as if the HMAC key itself were 32 bytes. openssl rand -hex 32 and the generated keyring keys (secrets.token_hex(32)) are both 64-character hex strings: 64 bytes of key carrying 256 random bits. This also corrects two version attributions in the body of 163adbe, which cannot be amended on a pushed branch: PyJWT refuses a future iat since 2.6.0, not 2.10, and InsecureKeyLengthWarning arrived in 2.11.0. The sub and jti string checks are from 2.10.0, as stated. Generated keyring keys are 64-character hex strings carrying 256 random bits, not "32 random bytes".
PyJWT refuses an empty signing key, which surfaced as a bare 500 at sign-in. The previous commit loaded the keyring at startup and stopped the api when the key was unusable. The api now logs the problem and keeps serving instead, and sign-in and token verification keep failing closed: - Startup loads the keyring and, if it cannot be used, logs an ERROR that names the file and the fix (delete it and restart, which generates a new key and signs everyone out, or set SECRET_KEY). - Signing a token with an unusable keyring raises SigningKeyUnavailableError. An exception handler turns it into a 500 whose detail points to the api log, and logs the full message. It is a 500 rather than a 503 because the frontend reads 502-504 as "backend unreachable", and the sign-in page shows the detail. - Verifying a token turns the same error into InvalidTokenError, so every token is a 401 until the keyring is fixed. - A blank .secret_keys.json (a write cut short, for example by a full disk) gets the same message as an empty key. A malformed or unreadable keyring, and a data directory the api cannot write, get their own message naming the file or directory. - The empty-key check moves from reading the file to loading the keyring, so rotate_signing_key() can replace an empty active key. Decision record Context: the api is the only surface that can tell an administrator what is wrong, so upstream keeps it serving when a startup probe fails (the recordings storage probe in main.py, nojoin_chown_tree in entrypoint_common.sh). nginx and the frontend wait on a healthy api. Alternatives: refusing to start on an unusable key, as the previous commit did (rejected: the whole UI goes down and the only diagnostic is the container log, against the convention above); generating a fresh key silently (hides the failure from the operator); reporting only at sign-in (nothing is said until someone signs in). Trade-offs: the api reports healthy while nobody can sign in. The startup log, the sign-in error and the per-attempt log line make the cause visible.
…a refresh token The token endpoint committed the spent authorization code, or the rotated refresh token, before it signed the access token. When the JWT keyring cannot be loaded, signing fails after that commit: the client gets a 500 and never sees the new refresh token, and its retry is treated as reuse of a rotated token, which revokes the whole grant (or, for a code, as a replayed code). Every connected MCP client then has to be reconnected once the keyring is fixed. Both grants now sign the access token right after loading the user, before any state changes, so a failure leaves the code or refresh token usable and the client's retry succeeds once the key is back. The endpoint returns that failure in the RFC 6749 error shape (server_error, 500, no-store) like its other errors, and logs the message that names the key file.
The check caught only SigningKeyUnavailableError, so any other failure while loading the keyring aborted startup, unlike the recordings storage probe it follows. It now logs any other exception with its traceback and carries on. The keyring file's existence check also moves inside the error handling, so a data directory the api cannot traverse is reported as an unavailable keyring that names the file, instead of a raw PermissionError.
… fix A legacy .secret_key that is not UTF-8 was reported as a permissions problem in the data directory, without naming the file, when the fix is to delete it. A key file that is a directory (what Docker leaves when a bind mount names a host file that does not exist) got the same permissions advice, when it has to be removed. An unreadable key file was reported by its directory rather than by name. The legacy file now gets the malformed-file error, which names it and says to delete it; a directory gets its own error naming it and explaining the bind-mount cause; and the access error names the key file and the directory the api must be able to read and write. SECURITY.md lists the fix for each case.
A hand-edited .secret_keys.json whose key value is a number, boolean, list or null loaded without an error, then failed every sign-in and every authenticated request with a bare TypeError, and nothing was logged at startup. Such a keyring is now malformed: the startup check logs the error naming the file, sign-in gets the actionable 500 and tokens are 401s.
5 of 17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Description
Nojoin signs and verifies its session, API and MCP tokens with
python-jose. That library has had no release since 3.5.0 (May 2025), and it carries an open critical advisory, GHSA-3qf3-8w2g-rqmx (CVE-2026-85394), with no patched release; the proposed fix (mpdavis/python-jose#422) is unmerged. This PR moves the JWT handling to PyJWT.Nojoin is not affected by that advisory. It needs a verifier that accepts HS256 with a public key as the HMAC secret. Nojoin only issues HS256 tokens, every keyring entry is a random symmetric secret (or
SECRET_KEY),decodepinsalgorithms=["HS256"], and an unknownkidis refused. The motivation is maintenance: the token code depends on a security library that no longer ships fixes. GitHub's advisory database lists python-jose ≤ 3.5.0 as affected with no patched version, so a Dependabot alert for it cannot be closed by any upgrade. PyJWT is maintained and already installed in the api image as a dependency ofmcp.What changes:
backend/core/security.py:import jwt(PyJWT). Encoding is unchanged (HS256 with akidheader). The header is read withjwt.get_unverified_header, and the unknown-kidcase raisesjwt.InvalidTokenError. The broadexcept Exceptionaround the header read is gone: PyJWT raisesInvalidTokenErrorfor a malformed header itself. Loading the keyring reports an empty active key, a blank or malformed key file (including a key that is not a string), an unreadable one and one that is a directory with an error that names the file and the fix (see below). Token verification turns that error intoInvalidTokenError, so every token is a 401 until the keyring is fixed.backend/main.py: the api loads the keyring when it starts and, if it is unusable, logs that error and keeps serving; any other failure of that check is logged with its traceback, and the api still starts. An exception handler turns the same error at sign-in into a 500 whose detail points to the api log, instead of a bare 500.backend/api/services/oauth_service.py,backend/api/v1/endpoints/oauth.py: the MCP token endpoint signs the access token before it spends the authorization code or rotates the refresh token. If signing fails, nothing has changed, so the client's retry with the same code or refresh token succeeds once the key is fixed; before, the retry was treated as reuse of a rotated refresh token and revoked the grant. The endpoint returns that failure in the RFC 6749 shape (server_error, 500) like its other errors.backend/api/deps.pycatchesjwt.PyJWTError, PyJWT's base class. Like python-jose'sJWTError, it covers every failuredecodecan raise, includingInvalidKeyError, so every rejection stays a 401.requirements/base.txt:python-jose[cryptography]==3.5.0is replaced byPyJWT==2.15.1(the versionmcp==2.2.0already resolves to; it needs>=2.10.1). The base file is used because the worker importsbackend.core.securitytoo, and has nomcp.cryptography==50.0.2is now pinned explicitly:backend/core/encryption.pyimports it directly, but only the python-jose extra declared it.ecdsaandrsaleave both images;pyasn1stays (google-auth needs it).docs/SECURITY.mdsays which algorithm is accepted, what verification refuses, how longSECRET_KEYshould be and what happens with an empty or unloadable key;docs/DEPLOYMENT.mdpoints to it and has an upgrade note.Behaviour differences, all checked by tests:
kid, PyJWT produces byte for byte the token python-jose produced (both sort the header keys and use compact JSON). The new tests carry session, API and MCP tokens minted by python-jose 3.5.0 through the replaced code, verify them throughdecode_access_tokenand the API dependency, and re-mint them with PyJWT. The stakes are bounded anyway: every token expires within 12 hours (sessions 12 h, API and MCP tokens 60 min), so the worst case of an undetected incompatibility would be one forced sign-in, not a lockout.iatis refused (PyJWT 2.6+). No leeway is applied: the api process both issues and verifies every token, so they share one clock.subandjtimust be strings (PyJWT 2.10+). Both always are (str(subject),uuid4().hex).kidis a 401 instead of a 500. python-jose passed a list-valuedkidthrough to the keyring lookup, which raisedTypeErrorand escaped the dependency's exception handler. PyJWT rejects the header first.SECRET_KEYvalues warn (PyJWT 2.11+). PyJWT emitsInsecureKeyLengthWarningwhen an HS256 key is shorter than 32 bytes (RFC 7518 §3.2). Generated keyring keys are 64-character hex strings carrying 256 random bits, so only a hand-setSECRET_KEYcan trigger it. Refusing short keys would lock out existing deployments, so they still work, anddocs/SECURITY.mdnow gives the recommended size..secret_key, or a blank.secret_keys.jsonleft by a write cut short, for example by a full disk) would surface as a bare 500 at sign-in. The api now loads the keyring when it starts. If the key is empty, or a key file is malformed, unreadable or a directory, it logs an error that names the file and the fix (delete an empty or malformed file and restart, which generates a new key and signs everyone out; give the api access to the data directory; remove a directory left by a bind mount; or setSECRET_KEY) and keeps serving. Until the key is fixed, sign-in fails closed with a 500 whose detail points to the api log (not a 503, which the frontend reads as "backend unreachable"), every token is a 401, and the MCP token endpoint answersserver_errorwithout spending the client's grant.rotate_signing_key()can also replace an empty active entry.Alternatives considered: staying on python-jose (no release since May 2025 and no patched release for an open critical advisory); joserfc / Authlib (a new dependency for the same three calls, where PyJWT is already installed). For the empty key: generating a fresh key silently (hides the write failure from the operator); logging only at the point of use (nothing is said until someone signs in, and then only in request logs); refusing to start (rejected because the api is meant to keep serving so it can report what is wrong, and nginx and the frontend wait on a healthy api, so the whole UI would go down; sign-in fails closed instead).
New dependencies:
PyJWT(already installed transitively in the api image) and an explicitcryptographypin (already installed in both images). Removed:python-jose,ecdsa,rsa.Type of change
Dependency maintenance: none of the boxes fits exactly. Existing tokens keep verifying, so upgrading signs nobody out.
Upgrade note: if the api logs "The JWT signing key in … is empty, so no token can be signed or verified", delete the named file under
data/and restart; everyone signs in again. The same note is indocs/DEPLOYMENT.mdunder "Upgrading and Migration".Checks run
Tick the checks you ran for the areas you touched (see
CONTRIBUTING.md).source .venv/bin/activate && pytestpython scripts/check.py(Ruff lint, format check, mypy, doc and Alembic validators)cd frontend && npm run lint(no frontend change)cd frontend && npm run test(no frontend change)cd frontend && npm run build(no frontend change)python3 scripts/validate_docs.pypython3 scripts/validate_alembic.pyThe checks ran in a fresh Python 3.12 virtualenv built from this branch's
requirements/test.txtandlint.txt, so python-jose was not installed.pip checkwas clean, and pytest ran against Postgres (NOJOIN_TEST_POSTGRES_URL).scripts/check_whitespace.py,check_file_size.py,check_held_pins.py --offlineandcheck_migrations.pyagainst Postgres also passed.Migration impact
Documentation impact
Security impact
docs/SECURITY.mdboundaries preserved and updated where behaviour changed.Same boundaries: HS256 only,
kid-selected keyring keys, unknownkidrefused, rotation and pruning unchanged,SECRET_KEYoverride unchanged, MCP tokens still rejected by the general API routes.docs/SECURITY.mdnow also states the accepted algorithm, what verification refuses, theSECRET_KEYsize and how an empty or unloadable key is reported.Manual verification
backend/tests/test_jwt_verification.py(new) on this branch:get_authenticated_token_details, and PyJWT re-mints each of them byte for byte.alg: none, RS256, a list-valuedkid, an unknownkid, a futureiat, and a malformed string.upstream/maincode with python-jose 3.5.0 installed: 11 of its 29 tests fail.kid(aTypeError, at both the decode and the API level, where it is a 500), the futureiat(accepted at both levels), the short-key warning (none) andInvalidKeyErrorfor an unusable stored key (an exception class python-jose does not have).alg: none, RS256, unknownkid, malformed): they fail only because they expect PyJWT'sInvalidTokenErrorand get python-jose'sJWTError. The API-level tests for the same tokens pass there, as 401s.backend/tests/test_jwt_keyring.pyandtest_jwt_keyring_unavailable.py: an empty legacy file, an empty active keyring entry and a blank (zero-byte or whitespace) keyring file raise an error naming the file, with nothing written; so do a legacy file that is not UTF-8 (delete remedy), a key file that is a directory (remove remedy), an untraversable data directory, and a keyring key that is a number, boolean, list or null; rotation replaces an empty active entry; the api lifespan logs the error and carries on, also when the check fails unexpectedly; sign-in returns the 500 with the actionable detail and no cookie; a token is a 401.backend/tests/test_oauth_mcp_connector.py: a code exchange and a refresh during a keyring outage returnserver_error, and once the key is back the same code or refresh token succeeds, with no reuse warning and the grant intact..secret_key, before the last round of review fixes:/healthanswers 200, the startup log has the error naming the file, sign-in with valid credentials returns the 500 with the actionable detail (and the log names the file again), a wrong password is still a 400, and a malformed bearer token is a 401.docker/Dockerfile.api(before the review fixes, which change no dependencies);pip checkinside it is clean, PyJWT 2.15.1 and cryptography 50.0.2 are installed, andpython-jose,ecdsaandrsaare gone. I did not build the worker image. The fresh virtualenv above resolvesbackend.txtandworker.txttogether withpip checkclean, which covers the worker's requirement set.Screenshots (if relevant)
None.