Skip to content

fix(security): bound credential guessing and close the username-enumeration oracle (PT-16/18/19) - #992

Closed
murdore wants to merge 1 commit into
fix/pt-password-policyfrom
fix/pt-auth-rate-limit
Closed

fix(security): bound credential guessing and close the username-enumeration oracle (PT-16/18/19)#992
murdore wants to merge 1 commit into
fix/pt-password-policyfrom
fix/pt-auth-rate-limit

Conversation

@murdore

@murdore murdore commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

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.

  • 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).


Split integrity

#930 was 57 files / +5040/-2776 in one commit. The seven slices reassemble byte-for-byte into that tree (verified with git diff over app/, .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:

slice commits vs base black / isort / autoflake pyrefly tests
1 ssrf-egress 1 OK 0 errors 942 passed
2 python-sandbox 1 OK 0 errors 949 passed
3 telephony-signatures 1 OK 0 errors 950 passed
4 rbac-tenancy 1 OK 0 errors 972 passed
5 password-policy 1 OK 0 errors 976 passed
6 auth-rate-limit 1 OK 0 errors 988 passed
7 token-revocation 1 OK 0 errors 992 passed

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.

…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).
@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bfdea356-c5c6-4c45-ad00-4c773e2d83c0

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@murdore

murdore commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Evidence — recorded against a live stack

Video: pr992-rate-limit.mp4
Local: /Users/sachinsharma/Developer/temp/clairvoyance/.proof-video/pr992/pr992-rate-limit.mp4

A real screencast, not a slideshow: Chrome DevTools Page.startScreencast capturing every repaint while the page is driven by real Input.dispatchKeyEvent/dispatchMouseEvent calls. Frames carry their arrival timestamps and are encoded with those durations, so the pacing in the file is the pacing that happened.

The stack under test: PostgreSQL 16 and Redis in containers, the clairvoyance API on :8100 at this branch, its own base branch on :8101, and the loom dashboard proxied to the API. Two tenants (ACME, GLOBEX) and four accounts seeded.

The finding

The credential endpoints could be guessed without limit.

Executed in the recording

Driven through the real login form, not curl. On the base branch (rate_limit.py ABSENT):

60 guesses at one account   ->  60 x 401   (no cap ever engaged)
then the real password      ->  200 ALLOWED, logged in

On this branch:

attempts 1-15   401
attempt    16   429  REFUSED   "Too many authentication attempts."
then the CORRECT password    429  REFUSED

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 work

ops@globex.local   -> 200 OK   signs in normally
admin@proof.local  -> 200 OK   signs in normally
shop@acme.local    -> 200 OK   signs in normally

and 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 matters

The 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.)

@murdore

murdore commented Aug 9, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by #996 — combined, and now independent

You 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 release directly.

The other four backend PRs — #987, #988, #989, #990 — were genuinely independent and have been re-based onto release. All five were trial-merged pairwise: 10 of 10 pairs merge with no conflict.

Nothing is lost: merging all five reproduces the original #930 tree, and the combined suite runs 992 passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant