Fix 500 when a non-expiring API token is used as a raw bearer token - #1330
Merged
Merged
Conversation
create_access_token() always set the "exp" claim, encoding it as null
whenever there's no expires_delta. PyJWT's decode() only skips its
expiration check when the "exp" key is absent from the payload; if the
key is present but null, _validate_exp() does int(payload["exp"]) and
raises a raw TypeError, which isn't a PyJWTError and so isn't caught by
any of the callers' `except jwt.PyJWTError` handlers, producing an
unhandled 500.
This is reachable in practice: create_api_key() (used by both the
"create-user" CLI command and the /new-api-key endpoint) generates a
non-expiring token by default, and get_current_user() -- the auth
dependency behind every /api/v2/* endpoint -- decodes any bearer token
with default options (no verify_exp override), so presenting such a
token directly hits this path. The /api-token exchange endpoint was
already unaffected: it explicitly passes options={"verify_exp": False}
and checks expiration itself via the persisted RegisteredApiKey.
Fix: omit the "exp" claim entirely when there's no expiration, instead
of encoding it as null -- the documented, correct way to express "no
expiration" in a JWT, which PyJWT already treats as "skip the check".
Verified with both python-jose (as used by the currently released
2.5.0/2.5.1) and PyJWT (current main): both raise the same uncaught
TypeError on a null exp claim, so this isn't specific to either library
or a regression from the jose->PyJWT migration.
Added a regression test that uses a create_api_key()-issued token
directly as a bearer token against a protected endpoint (bypassing the
/api-token exchange) -- verified it reproduces the exact TypeError
traceback through get_current_user() on the pre-fix code, and passes
with the fix.
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.
Summary
Follow-up from yeti-docker#29 — one of two bugs bundled in that report (the
other, the empty-Feeds-page issue, was mitigated in
yeti-platform/yeti-docker#30; this is the separate
exp: null500 alsomentioned there).
create_access_token()(core/schemas/user.py) always set the"exp"claim on the JWT payload, encoding it as
nullwhenever noexpires_deltawas given. PyJWT'sdecode()only skips its expirationcheck when the
"exp"key is absent from the payload(
if "exp" in payload and options["verify_exp"]:injwt/api_jwt.py) — if the key is present butnull,_validate_exp()doesint(payload["exp"])and raises a rawTypeError. That's not aPyJWTError, so none of the callers'except jwt.PyJWTError:handlers catch it, and it becomes an unhandled500.
This is reachable in practice:
create_api_key()— used by both thecreate-userCLI command andthe
/new-api-keyendpoint — generates a non-expiring token bydefault (no
expires_deltapassed through).get_current_user()(core/web/apiv2/auth.py:89) — the authdependency behind every
/api/v2/*endpoint — decodes any bearertoken with PyJWT's default options (no
verify_expoverride), sopresenting such a token directly as
Authorization: Bearer <token>hits this path directly.
The
/api-tokenexchange endpoint (login_api) was already unaffected— it explicitly passes
options={"verify_exp": False}and checksexpiration itself via the persisted
RegisteredApiKey.expired.Not a regression, not fixed by the jose→PyJWT migration: verified
both python-jose (as shipped in the currently released 2.5.0/2.5.1) and
PyJWT (current
main) raise the identical uncaughtTypeErroron anull
expclaim.Fix
Omit the
"exp"claim entirely when there's no expiration, instead ofencoding it as
null— the standard, correct way to express "noexpiration" in a JWT, which PyJWT already treats as "skip the
expiration check".
Test plan
test_api_key_used_directly_as_bearertotests/apiv2/auth.py: uses acreate_api_key()-issued tokendirectly as a bearer token against
/api/v2/auth/me, bypassingthe
/api-tokenexchange.TypeErrortraceback throughget_current_user()on the pre-fix code, and passes with the fix.tests/apiv2suite: 199/201 pass (same 2 known pre-existingfailures in
tests/apiv2/tasks.py, unrelated).tests/schemas(190/190) andtests/core_tests(29/29) passunchanged.
ty check(core+yetictl and plugins jobs): 0 errors.ruff check/ruff format --check: clean.