Conversation
Findings from a security audit of the current tree. Each is independent
and small; the larger findings (unauthenticated netplay socket, image-baked
provider credentials, decompression bombs) are left for separate changes.
- csrf: an Authorization header exempted a request on presence alone, but
HybridAuthBackend resolves the session cookie first, so a cookie plus any
header value authenticated as the cookie's owner with the check skipped.
Exempt only when no session cookie is present.
- users: revoke every session of the user a credential change was applied
to, not just the caller's, so an admin resetting a compromised account
can lock the attacker out. Sessions are keyed by username, so the name is
captured before the update renames them.
- users: gate POST /{id}/ra/refresh on ownership. It took an arbitrary id
under me.write and wrote to that user's row.
- users: verify the invite token before the uniqueness checks, which let an
anonymous caller enumerate accounts with a token they never had. Split
verify from consume so a rejected username does not burn the invite.
- auth: fail password verification on a hash passlib cannot identify.
OIDC-provisioned accounts hold a placeholder, which raised and turned a
401 into a 500 that identified them.
- auth: log the invite token's jti rather than the link. The link goes back
to the caller in the response, and the log has a wider audience.
- archives: pass "--" before the 7-Zip member argument, matching the bsdtar
branch, so an entry named "-x" or "@file" stays a member name.
- init: trust X-Forwarded-For only from the bundled nginx. Trusting every
hop makes uvicorn read the leftmost entry, which the caller supplies, and
the per-IP rate limits read that address. FORWARDED_ALLOW_IPS overrides
it for deployments behind a further proxy.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC
Review-polish pass over the previous commit. Comments cut to the one non-obvious fact each: the cross-middleware ordering, passlib's behaviour on an unidentifiable hash, and what the leftmost X-Forwarded-For entry means. The rest was restating the code or defending the choice. verify_invite_link_token renamed to assert_invite_link_token_valid. The adjacent verify_password_reset_token verifies *and* consumes, so two verify_* methods on one class would have read as the same thing while differing on whether the token survives; assert_* is the idiom already used for raising checks (assert_rom_visible, assert_session_owner). It returned a role nothing read, so it returns None. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Critical issues remain in session revocation, invite-token atomicity, and forwarded-IP trust.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 3
Open (7)
What changed in this PR
Hardens authentication, session handling, invite validation, archive extraction, and proxy IP handling.
Changes:
- Tightens CSRF, password, invite-token, and ownership checks.
- Improves session revocation and trusted forwarded-IP configuration.
- Adds focused security regression tests.
| File | Description |
|---|---|
env.template |
Documents forwarded-IP configuration. |
docker/init_scripts/init |
Configures the trusted proxy allowlist. |
backend/utils/archives.py |
Prevents 7-Zip member names from being parsed as options. |
backend/tests/handler/auth/test_csrf_middleware.py |
Tests CSRF behavior with session cookies. |
backend/tests/handler/auth/test_auth.py |
Tests invalid password-hash handling. |
backend/tests/endpoints/test_identity.py |
Tests RA-refresh ownership enforcement. |
backend/handler/auth/middleware/csrf_middleware.py |
Enforces CSRF when auth headers accompany session cookies. |
backend/handler/auth/base_handler.py |
Hardens password verification and invite-token handling. |
backend/endpoints/user.py |
Adds session revocation, ownership checks, and invite validation. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…xy guidance Both from Greptile's review of this PR. A session loaded before `clear_user_sessions` ran was re-persisted when its request responded: the middleware writes any non-empty session back unconditionally, so an attacker holding one in flight restored it and the credential change locked nobody out. An existing record is now refreshed only while it is still in Redis, and a request whose session went away mid-flight has its cookie expired instead. A new session still writes unconditionally, so login is unaffected. The FORWARDED_ALLOW_IPS note told deployments behind a further reverse proxy to set the variable to that proxy's address. Gunicorn's immediate peer is always the bundled nginx on 127.0.0.1, so replacing it makes uvicorn distrust nginx and ignore the forwarded chain entirely, leaving every request on the loopback address and in one rate-limit bucket. The upstream proxy is appended, not substituted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC
From Copilot's review of this PR. `consume_invite_link_token` read the jti and deleted it as two operations, so two registrations racing on one invite could both see it valid and both create an account. It now reads and invalidates with a single GETDEL, matching how refresh tokens are already spent in this file. The non-consuming check keeps a plain read, and both raise the same response so neither distinguishes a spent invite from an unusable one. Tests for the boundaries the review noted were uncovered: - An admin resetting another user's password ends that user's sessions while leaving the admin's own usable. The existing test only changed the caller's own password, which the previous implementation already handled. - Registration rejects a bad token without disclosing whether an account exists, and a registration rejected on a duplicate username leaves the invite usable, then spent once it registers one. - The 7-Zip and bsdtar member commands put "--" before the member, for members shaped like "-x", "@ListFile" and "-so". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC
The new middleware test failed in CI, and the cause was in clear_user_sessions rather than the test: `smembers` answers with bytes on a client that does not decode, and the member went straight into an f-string key name, so the delete targeted `session:b'<id>'` and the real session stayed live. The write-back guard then found the key still there and refreshed it, which is exactly what the test asserts must not happen. Production is unaffected: `async_cache` is built with `decode_responses=True` there, so members arrive as strings. Only the pytest client, a bare `FakeAsyncRedis`, returns bytes, which is why this surfaced the moment a test exercised the helper end to end. Members are now decoded when they arrive as bytes, so the helper is correct under either client, and the keys go in one delete call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC
From Greptile's review. The write-back and key-decoding comments had grown past the two lines the repo allows and were narrating the failure they prevent; they now state the invariant only. The middleware test imported SESSION_COOKIE_NAME rather than repeating its value, which would have drifted if the cookie were ever renamed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
An unresolved critical session-revocation race and two moderate test-coverage issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
Resolved since last review (7)
| f"session:{sid.decode() if isinstance(sid, bytes) else sid}" | ||
| for sid in session_ids | ||
| ] | ||
| await async_cache.delete(*keys, f"user_sessions:{user_id}") |
Contributor
Author
There was a problem hiding this comment.
Verified, and you're right that the XX guard cannot cover this case. A /login that authenticated against the old password before the reset lands has existing_id is None, so it writes unconditionally — it has to, or no one could ever log in — and its SET plus SADD land after SMEMBERS has already been read. That session survives.
Two things about its shape, for whoever picks this up:
- The precondition is that the attacker already holds valid old credentials and overlaps a login with the reset window. It is a genuine hole, not a theoretical one, but it is narrower than the case fixed in 6942651, which needed only any in-flight request.
- Relative to
masterthis is strictly an improvement rather than a regression:masterhas both this race and the much wider resurrection one. Nothing here makes revocation weaker than it was.
I'm not fixing it in this PR. Closing it properly means the per-user revocation generation you and Greptile both suggested — stamp sessions with an issue time, record a valid_from per user on revoke, and reject anything older at load. That subsumes the XX guard rather than sitting beside it, and it changes the session validity model and payload schema, which is a design decision for a maintainer rather than something I should bolt onto a PR that started as a set of small hardening fixes. I've raised it with the repo owner to sequence as a follow-up.
Generated by Claude Code
The client kept the target's session cookie from its Basic login, and HybridAuthBackend resolves a cookie ahead of the Authorization header, so the reset authenticated as the target and took the self-update path. The test passed without ever exercising an admin resetting another account. The jar is cleared before the reset, leaving the bearer as the only credential. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC
gantoine
self-requested a review
September 19, 2026 17:41
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.
Description
Findings from a security audit of the current tree, plus the fixes that came out of review on this PR. Each change is small and independent.
csrf_middleware.pyAuthorizationheader exempted a request from CSRF on presence alone.HybridAuthBackendresolves the session cookie first, so a victim's cookie plus any header value at all (Authorization: Bearer x) authenticated as the cookie's owner with the check skipped. Now exempt only when no session cookie is present.endpoints/user.pyredis_session_middleware.pySET ... XX, so a request holding a copy of a session revoked mid-flight cannot write it back. Previously any in-flight request undid a revocation.redis_session_middleware.pyclear_user_sessionsdecoded itssmembersmembers before building the delete key. A bytes member interpolated into an f-string producedsession:b'<id>', so the real session survived the delete.endpoints/user.pyPOST /users/{id}/ra/refreshtook an arbitraryidunderme.writewith no ownership or admin check.endpoints/user.pybase_handler.pyGETDEL, so two registrations racing one invite cannot both create an account. The non-consuming precheck keeps a plainGET, and both raise the same error.base_handler.pyverify_passwordreturnsFalseon a hash passlib can't identify. OIDC accounts hold a placeholder, which raised and turned a 401 into a 500 that identified them.base_handler.pyjtirather than the link, which already goes back to the caller in the response.utils/archives.py--before the 7-Zip member argument, so an archive entry named-xor@filestays a member name. Member names come from the archive's own listing.docker/init_scripts/initX-Forwarded-Foronly from the bundled nginx. With--forwarded-allow-ips="*", uvicorn reads the leftmost entry, which the caller supplies.The CSRF change
The only cross-layer change. The
Session cookie present?branch is new.flowchart TD req[Request with a bearer or basic Authorization header] --> cookie{Session cookie present?} cookie -->|No| skip[Skip CSRF: the header is the credential] cookie -->|Yes| check[Enforce CSRF: HybridAuthBackend authenticates the cookie, not the header] skip --> app[Endpoint] check --> appKnown residual, for the merge decision
Session revocation still has one gap this PR does not close, raised by Copilot and confirmed: a
/loginthat authenticated against the old password before a reset lands has no existing session id, so it writes unconditionally — it must, or nobody could log in — and itsSET/SADDcan land afterclear_user_sessionshas read its member list. That session survives.It needs an attacker to already hold valid old credentials and to overlap a login with the reset window, so it is narrower than the resurrection case fixed here, which needed only any in-flight request. Relative to
masterthis is strictly an improvement:masterhas both races.Closing it properly means a per-user revocation generation — stamp sessions with an issue time, record a
valid_fromper user on revoke, reject anything older at load — which subsumes theSET XXguard rather than sitting beside it. That changes the session validity model and payload schema, so it is scheduled as a follow-up PR rather than bolted on here.Also deliberately not included
generate_password_reset_tokenreturns nothing and there is no email support, so that log line is the only delivery channel for a reset link. Removing it would break password reset.*withallow_credentials=True. Real, but changing the default is breaking for existing deployments and deserves its own PR. The CSRF fix above is the robust half of that pair./netplaySocket.IO namespace, provider credentials baked into the published image viaENV, and unbounded decompression on the ROM hashing path.Behaviour change worth a reviewer's eye
FORWARDED_ALLOW_IPS(new, documented inenv.template, defaults to127.0.0.1). Deployments behind a further reverse proxy append that proxy's address rather than replacing the bundled nginx's, since127.0.0.1is the peer uvicorn checks.Checklist
On testing — I could not run
pytestor Trunk in my environment (a pre-existing pydantic / Python 3.14.0rc2 collection failure, and a blocked Trunk plugin download), so CI has been the first execution throughout. That is not academic: CI caught theclear_user_sessionsdecoding bug when a test I added failed, and review caught that the admin-reset test was sending a stale session cookie alongside the bearer and so never exercised the admin path. Both are fixed. "Tested locally" stays unchecked because it would not be true.AI assistance disclosure
Per
CONTRIBUTING.md: written with AI assistance (Claude Code). The audit, the code changes, the tests and this description are AI-generated, each claim verified against the source.🤖 Generated with Claude Code
https://claude.ai/code/session_01AZF3cyuYh96LK2h3j25DPC