fix(security): bound credential guessing and close the username-enumeration oracle (PT-16/18/19) - #992
fix(security): bound credential guessing and close the username-enumeration oracle (PT-16/18/19)#992murdore wants to merge 1 commit into
Conversation
…ration oracle (PT-16/18/19) 6 of 7 splitting the BB-DEEPDIVE-2026-001 remediation (was #930). The credential endpoints could be guessed without limit, and they told an attacker which accounts existed in two different ways. - PT-16 enumeration: /auth/accounts required no proof of ownership on the email branch. It now requires the password. - PT-16 timing: /login and /auth/s2s/token returned faster for an unknown username than for a wrong password, because the no-such-user branch skipped bcrypt. Both now spend one verification against a shared dummy hash. The account-listing path spends a constant budget of verifications regardless of how many accounts share an email — verifying once per candidate leaks the count through response time. - PT-16 guessing: fixed-window per-IP and per-username caps on /login, /auth/s2s/token, /signup and /auth/accounts. - PT-18: a cross-IP aggregate cap per public_widget_key, so distributing an attack across source addresses does not buy unlimited attempts. - PT-19: the chat-demo client IP is derived from the trusted last XFF hop rather than the first, which the client controls. Rate limiting fails OPEN on a Redis outage. That is deliberate: a Redis blip locking every operator out of login is a worse outcome than a temporary loss of the cap, and bcrypt still bounds throughput underneath it. The dummy verification uses the async wrapper for the same reason the real check does — this is the branch an attacker drives, so running bcrypt inline would freeze the event loop for ~240ms per guess. Tests: tests/test_auth_rate_limit.py and tests/test_auth_enumeration.py (constant bcrypt budget, one bcrypt on the unknown-user branch of both routes).
|
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: pr992-rate-limit.mp4 A real screencast, not a slideshow: Chrome DevTools The stack under test: PostgreSQL 16 and Redis in containers, the clairvoyance API on The findingThe credential endpoints could be guessed without limit. Executed in the recordingDriven through the real login form, not curl. On the base branch ( On this branch: The last line is the one that matters — after the cap trips the correct password is refused too. That is a lockout, not a credential check. And what must still workand in the recording a different operator signs in through the same locked-out browser and the console loads. The cap is per-username, so one account under attack does not lock out the tenant. 988 tests pass. Enforcement is in Redis so it holds across pods, and fails open on a Redis outage — a blip losing the cap is a better outcome than a blip locking every operator out, and bcrypt still bounds throughput underneath. 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. |
Part 6 of 7 splitting #930 (BB-DEEPDIVE-2026-001 backend remediation) into reviewable pieces.
Base:
fix/pt-password-policy— this is a stacked chain, merge in order 1→7.6 of 7 splitting the BB-DEEPDIVE-2026-001 remediation (was #930).
The credential endpoints could be guessed without limit, and they told an
attacker which accounts existed in two different ways.
email branch. It now requires the password.
username than for a wrong password, because the no-such-user branch skipped
bcrypt. Both now spend one verification against a shared dummy hash. The
account-listing path spends a constant budget of verifications regardless of
how many accounts share an email — verifying once per candidate leaks the
count through response time.
/auth/s2s/token, /signup and /auth/accounts.
attack across source addresses does not buy unlimited attempts.
rather than the first, which the client controls.
Rate limiting fails OPEN on a Redis outage. That is deliberate: a Redis blip
locking every operator out of login is a worse outcome than a temporary loss
of the cap, and bcrypt still bounds throughput underneath it.
The dummy verification uses the async wrapper for the same reason the real
check does — this is the branch an attacker drives, so running bcrypt inline
would freeze the event loop for ~240ms per guess.
Tests: tests/test_auth_rate_limit.py and tests/test_auth_enumeration.py
(constant bcrypt budget, one bcrypt on the unknown-user branch of both routes).
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.