Skip to content

fix(identity): mint bot passwords from 32 random bytes, not the clock (TASK-163) - #1941

Merged
lilyshen0722 merged 1 commit into
mainfrom
kai/task163-bot-password-entropy
Sep 27, 2026
Merged

lilyshen0722 merged 1 commit into
mainfrom
kai/task163-bot-password-entropy

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

Cut from 97f1aa66 (main, after #1938/#1939 merged). One line of source, one new suite. Backend only — no UI, no versioned package, no migration.

The defect

getOrCreateAgentUser (backend/services/agentIdentityService.ts, the single mint site) set:

password: `agent-password-${Date.now()}`,

A millisecond timestamp, against an identity whose username and botMetadata are both public. Measured on production (Vera, read-only): 577 of 577 bot users carry a non-empty password, all minted this way.

It is not exploitable today, and the reason matters: not because the credential is strong, but because five call sites remember to filter isBot — authController.ts:760 and the two recovery paths (:625, :668), plus three refusals by name in oauthController.ts (:272, :276, :365). That is an invariant held by repetition, which is the shape that decays: a sixth login path (SSO, magic link, device code) that forgets the filter converts 577 guessable credentials into 577 logins, and nothing at the mint says the password is load-bearing.

So the fix is at the mint, not at the guards:

password: crypto.randomBytes(32).toString('hex'),

No behaviour change (nothing reads the value), no migration (existing rows keep what they have; rotating them is a separate dry-run-first script on the operator's word, per the row).

The witnesses, and why the row's originals were replaced

The row's two proposed arms could not fail, so neither is here:

  • "Two bots minted in the same millisecond get different passwords" is already true of the old code — models/User.ts:421 hashes with bcrypt.hash(password, 10), and bcrypt salts per call, so identical plaintexts still store different hashes. It passes before and after the fix, and the revert mutation stays green.
  • "The minted value is not a function of Date.now()" is unobservable after the pre-save hook: there is no plaintext left on the row to inspect.

Three arms, each observing a place the property is actually visible:

arm what it observes
the creation-time guess does not authenticate the minted seat the stored hash, via the attack: freeze the clock, mint, then comparePassword('agent-password-<frozen ms>') must be false. Control: the row carries a real bcrypt hash (/^\$2[aby]\$/), so the false is the mint's property and not an empty field
the plaintext handed to the hash is random hex, not a low-entropy template the plaintext at bcrypt.hash, via a spy — and it is deliberately blind to Date.now(), so a future edit that swaps one guessable template for another is still caught. Control first: expect(hashSpy).toHaveBeenCalled()
two seats minted in the same millisecond do not share a plaintext the row's original first arm, repaired: the observation moved from the salted hash to the plaintext, where the collision is visible

Mutation ledger

Baseline and restore 3/3 green; each mutation reverted before the next.

mutation red
M1 the defect: agent-password-${Date.now()} back 3 — all three arms
M2 weak entropy: randomBytes(8) (right shape, too little of it) 1 — the shape arm, alone
M3 the clock returns indirectly: sha256(String(Date.now())) 1 — the same-millisecond arm, alone; the shape arm passes (64 hex) and the guess arm passes (the guess is not a digest)
M4 a static template: 'agent-password' 2 — shape and same-millisecond; the guess arm survives
M5 control: no password minted at all 3 — each arm fails on its own control, so none can pass vacuously

Two disclosures from that table, rather than tidy summaries:

  • M4's survivor is real and deliberate. The guess arm knows one literal template and says so; it is narrow by design. M3 is the arm that catches clock-dependence in a shape the guess cannot see, which is why both exist.
  • M5 is the control for the arms, not for the fix. It exists to demonstrate that a missing mechanism reddens every arm loudly instead of letting a vacuous not.toMatch pass.

Verification

  • New suite: 3/3 green; ledger as above.
  • backend/services/agentIdentityService.ts carries 6 eslint warnings at HEAD and 6 at HEAD~1 — the same six lines (11/19/27/467/655/698), none on a line this PR touches: 0 diagnostics added.
  • The new suite carries 4 import/no-unresolved + import/extensions diagnostics. That is the ambient class in this repo's un-gated .js test corpus: the sibling suite decisionCardReply.bridges.test.js carries 83 diagnostics including the same rules. (The first neighbour I measured, installableInstallationService.test.js, reported only 1 — a Parsing error: Identifier directly after number that stops eslint before the import rules run, so that comparison was void and is not being used.)
  • docs/* no change needed; no new ADR, no new row. Line-number note: the row cites agentIdentityService.ts:503 for the mint; the password line is at :511 on this head after the import and the comment. Same single site either way.

… (TASK-163)

`getOrCreateAgentUser` set `password: `agent-password-${Date.now()}`` — a
millisecond timestamp against an identity whose username and botMetadata are
both public. Nothing reads the value and no login path accepts a bot, so the
shape bought nothing; it would have become a reconstructable credential the
first time a login path was added without the `isBot` filter that five call
sites currently carry.

One line at the mint: `crypto.randomBytes(32).toString('hex')`. No behaviour
change and no migration — existing bot rows keep their timestamp passwords
until the operator runs the separate dry-run-first rotation script.

The witnesses observe the two places the property is visible: the stored hash,
via the attack (freeze the clock, reconstruct the guess, it must not
authenticate), and the plaintext as it reaches `bcrypt.hash` (shape, and that
two seats minted in the same millisecond differ). The row's two original
witnesses could not fail: bcrypt salts per call, so identical plaintexts still
store different hashes, and the plaintext is gone once the pre-save hook runs.
@lilyshen0722
lilyshen0722 added this pull request to the merge queue Sep 27, 2026
Merged via the queue into main with commit 6a52978 Sep 27, 2026
14 checks 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