fix(security): server-side JWT revocation, liveness recheck and a single S2S lifetime cap (PT-21/22) - #993
fix(security): server-side JWT revocation, liveness recheck and a single S2S lifetime cap (PT-21/22)#993murdore wants to merge 1 commit into
Conversation
…gle S2S lifetime cap (PT-21/22) 7 of 7 splitting the BB-DEEPDIVE-2026-001 remediation (was #930). PT-22: logout was a no-op. A stolen token stayed valid for its full lifetime because nothing server-side could refuse it, and a deactivated user kept working until their token expired. Adds a revocation denylist keyed by a hash of the token with a TTL equal to its remaining lifetime, plus a per-request is_active liveness recheck. Logout now actually revokes, and reports revoked:false rather than claiming a revocation that did not happen. The liveness check is Redis-cached for 10s and fails open on a Redis outage — both deliberate, and both bounded: the cache is a 10-second window on deactivation, not an indefinite one, and failing closed would turn a Redis blip into a total outage. verify_rbac_token and get_user_from_websocket become coroutines because the revocation check is a Redis round-trip. The four `await` additions in stt/handlers.py, the two webhook services and feature_flags/rbac.py are that change and nothing else. PT-21: the 365-day S2S cap. There are two mint paths, not one — POST /auth/s2s/token (admin-only) and POST /merchant with issue_token=true, which is reachable by resellers as well as admins and hands its value to the same create_access_token_with_rbac. The merchant one defaulted to 3650 days and allowed 365000, so capping only the first left a reseller able to mint a ten-year credential by default. Both now read MAX_S2S_TOKEN_LIFETIME_DAYS rather than their own literal, which is how they drifted apart in the first place, and a test fails if either schema is reverted. Tests: tests/test_token_lifetime_revocation.py.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Evidence — recorded against a live stackVideo: pr993-token-revocation.mp4 A real screencast, not a slideshow: Chrome DevTools The stack under test: PostgreSQL 16 and Redis in containers, the clairvoyance API on The findingLogout was a no-op. A stolen token stayed valid for its full lifetime. Executed in the recordingAnd what must still workOne wrinkle worth recording: tokens carry no 992 tests pass. The denylist is keyed by a hash of the token with a TTL equal to its remaining lifetime; the Why the middle column mattersThe base branch is run in the same take, with the same payloads. If an attack were already refused there, this PR would be guarding something that was never broken — and the recording would show that rather than hide it. The third act exists for the opposite failure: a control that bounds an attack by breaking legitimate use is not a fix. (GitHub has no API for attaching video to a comment — inline playback requires a browser upload. The recordings are hosted as release assets so the links are stable; the local paths above are the originals.) |
Superseded by #996 — combined, and now independentYou asked for PRs that can merge without depending on each other. This one could not: its dependencies on the others are real, not stylistic.
Split apart, none of the three builds alone. So the three are now one PR with one commit: #996, targeting The other four backend PRs — #987, #988, #989, #990 — were genuinely independent and have been re-based onto Nothing is lost: merging all five reproduces the original #930 tree, and the combined suite runs 992 passed. |
1 participant
Part 7 of 7 splitting #930 (BB-DEEPDIVE-2026-001 backend remediation) into reviewable pieces.
Base:
fix/pt-auth-rate-limit— this is a stacked chain, merge in order 1→7.7 of 7 splitting the BB-DEEPDIVE-2026-001 remediation (was #930).
PT-22: logout was a no-op. A stolen token stayed valid for its full lifetime
because nothing server-side could refuse it, and a deactivated user kept
working until their token expired. Adds a revocation denylist keyed by a hash
of the token with a TTL equal to its remaining lifetime, plus a per-request
is_active liveness recheck. Logout now actually revokes, and reports
revoked:false rather than claiming a revocation that did not happen.
The liveness check is Redis-cached for 10s and fails open on a Redis outage —
both deliberate, and both bounded: the cache is a 10-second window on
deactivation, not an indefinite one, and failing closed would turn a Redis
blip into a total outage.
verify_rbac_token and get_user_from_websocket become coroutines because the
revocation check is a Redis round-trip. The four
awaitadditions instt/handlers.py, the two webhook services and feature_flags/rbac.py are that
change and nothing else.
PT-21: the 365-day S2S cap. There are two mint paths, not one —
POST /auth/s2s/token (admin-only) and POST /merchant with issue_token=true,
which is reachable by resellers as well as admins and hands its value to the
same create_access_token_with_rbac. The merchant one defaulted to 3650 days
and allowed 365000, so capping only the first left a reseller able to mint a
ten-year credential by default. Both now read MAX_S2S_TOKEN_LIFETIME_DAYS
rather than their own literal, which is how they drifted apart in the first
place, and a test fails if either schema is reverted.
Tests: tests/test_token_lifetime_revocation.py.
Split integrity
#930 was 57 files / +5040/-2776 in one commit. The seven slices reassemble byte-for-byte into that tree (verified with
git diffoverapp/,.env.example,pyproject.toml,uv.lock), and every one of the original 36 pentest tests is routed into exactly one slice — asserted by the splitter, so a dropped test fails the build rather than quietly reducing coverage.Each slice was verified independently on its own branch:
The count rises monotonically because each slice adds its own tests and breaks none of the earlier ones.
Attack-side proof for this slice is posted as a comment below.