fix: pin the login challenge shape and give the bearer header one home

[FSM1]

Four adjacent defects on one surface: the engine's Http seam and the auth flow that rides it.

Closes #1034
Closes #713
Closes #933
Closes #1086

#1034 — the login challenge is a signing oracle

Verified at 3d087d673: login_identity took body.challenge straight out of the /auth/challenge response and handed it to signer.sign_challenge. Per crates/engine/src/api/signer.rs, that is EcdsaSigner::sign_detcbor(challenge.as_bytes()) — RFC 6979 deterministic secp256k1 over sha256(utf8(challenge)), with the member's identity key, and the primitive is the same one that signs det-CBOR structures. Nothing checked the string. Anything answering at apiBaseUrl got an arbitrary UTF-8 preimage signed.

A prefix check is not sufficient — and that is the substance of this fix

The issue asks for "reject a challenge that does not carry the prefix, and cap its length". That would have been a weak fix:

• The signed preimage is the whole string. A prefix check pins the first 19 bytes and leaves everything after them to whatever answers at the API base URL. It buys domain separation, not content control — the oracle narrows, it does not close.
• A length cap bounds size, not alphabet. cipherbox-login:v2: followed by 64 bytes of attacker-chosen text still passes a prefix-plus-cap check.

So the whole shape is pinned instead. The API issues exactly IDENTITY_CHALLENGE_PREFIX + randomBytes(32).toString('hex') (apps/api/src/auth/services/challenge.service.ts:42), so the client can require exactly that: the domain tag plus 64 lowercase-hex characters. That leaves a hostile responder no steerable bytes outside [0-9a-f] — it cannot smuggle det-CBOR framing, an EIP-191/4361 message, or any other protocol's domain tag into the preimage, because none of those are expressible in a hex alphabet after a fixed 19-byte ASCII tag.

Two things this deliberately does not try to do, because they are not the client's to do:

• Challenge-to-publicKey binding is unverifiable client-side — the tail is opaque entropy. The API binds it (ChallengeService.consume compares the bound publicKey).
• expiresAt is not checked. The client has no injected Clock seam, and reading a wall clock in engine logic is against repo law. The server enforces the TTL and single-use consumption.

Over-tightening is guarded by the live contract suite, which runs the real client against a real API on every PR — if the API's entropy width ever drifts, that goes red immediately rather than silently. The v2: in the tag is the change-control mechanism for a deliberate format change.

Covered by a_challenge_the_api_could_not_have_issued_is_never_signed: nine hostile shapes (no tag, another protocol's tag, right tag with the responder's own text, wrong width both ways, uppercase hex, tag as a suffix, leading whitespace, empty). Each asserts Err and that only /auth/challenge was sent — no POST to /auth/login, and the key signed nothing.

#713 — a cancelled refresh leader wedged auth

Confirmed. The leader set refresh_waiters = Some(vec![]), awaited do_refresh(), and only took the vec back on completion. Drop the leader's future mid-await and the slot stays occupied forever: every later caller pushes a oneshot::Sender into a vec nothing will ever drain, and awaits a receiver whose sender is parked in the RefCell — so not even Canceled fires.

The RAII guard the issue suggests is the right shape, with one correction. RefreshLead releases leadership however the leader leaves; dropping the stored senders wakes every waiter with Canceled. But the existing Canceled mapping was ApiError::Unauthorized, which was dead code before this change and becomes reachable after it — and telling a waiter "your session is dead, re-login" because someone else's future was dropped is wrong. It is remapped to ApiError::Transport: availability, not a bypass, which is what a cancelled rotation actually is.

Two tests: the next caller leads a fresh rotation (request count goes 1 → 2), and a waiter already queued behind a cancelled leader is woken with a transport error rather than parking.

I did not make the waiter re-lead by recursing into refresh(). It works, but it adds an allocation and a re-entrancy path to buy one transparent retry; the caller's next request refreshes cleanly anyway.

#933 — one home for the Authorization header value

AUTHORIZATION was declared three times (api/client.rs:37, content/read.rs:25, content/provider.rs:38) and the Bearer {} splice written out four. crates/engine/src/seams/http.rs now owns both the name and the rule as bearer_header, beside the request type that carries the value.

Enforced at each splice, not just at a gate two frames up:

• api/client.rs — the access token is decoded out of an /auth/* JSON body, so it is the API's bytes. This was genuinely ungated: a hostile API returning an accessToken containing CRLF got it spliced into a header value. Now fails closed.
• content/read.rs — GatewaySource.bearer is host config, also ungated. A source whose token cannot be a header value is skipped rather than contacted bare; rotation drops to the next source.
• content/provider.rs — the BYO token is the one that was already gated, at config entry. validate_byo_config now delegates its rule to the shared check_bearer (the local is_bearer_byte is deleted) and the splice imports the shared header name. A test pins that the gate and the header the splice builds agree on every token, so the two cannot drift.

The rule is split in two: check_bearer asks the question, bearer_header builds the value. That is not cosmetic — see the review section below.

Corrections to the issue body

• The BYO validation claim is stale. The issue says content/provider.rs "holds the BYO token to visible ASCII" at the splice. It did not — is_bearer_byte was applied at config entry in validate_byo_config (:344-357), and both public entry points (place_block:118, test_connection:328) call it first. The splice itself was unguarded. That is now fixed in both places.
• "Nothing is live on web yet" is stale. packages/client/src/worker/engineHost.ts forwards the URLs.
• There is a fourth splice site the issue misses, at crates/load/src/runner.rs:91. It is now fixed too — see the scope note below.

#1086 — refuse redirects on the Http seam

ReqwestRecordTransport and FetchRecordTransport already refuse them; ReqwestHttp followed up to 10 hops (guarding only an https→http downgrade) and FetchHttp inherited the browser default follow. Both now match their siblings.

The issue says the substance is the verification, not the two-line change. Done — a full sweep of every configured API/accelerator/gateway URL, CI workflow, compose file, and proxy config:

• Every URL the seam builds is an absolute path onto a slash-normalized base (client.rs:102-109, read.rs:270, provider.rs probe paths), so no bare-host or trailing-slash 301 is reachable.
• The only redirect emitter in the repo is docker/Caddyfile:39-41 (:80 → https, 301). Every in-repo consumer addresses the API over https://api-staging.cipherbox.cc or loopback, so it is never traversed. No nginx/Traefik/ingress, no HSTS, no www/apex redirect.
• There are no hardcoded default public gateways — the fallback list is entirely vars.VITE_PUBLIC_GATEWAYS, and unset yields an empty list that fails closed.
• Web e2e configures no accelerator or gateway at all; the contract suite's gateway fetch uses crates/contract's own reqwest client, not this seam, so it is unaffected.
• No test anywhere mocked a 3xx on this seam or asserted redirect-following — including the old downgrade-stop branch, which had no coverage. There is one now, live, on the desktop host.

One thing to eyeball before merge: the values of vars.STAGING_API_URL, vars.VITE_READ_ACCELERATOR_URL, and vars.VITE_PUBLIC_GATEWAYS are not in the repo. Each should be https:// and path-free. A plaintext STAGING_API_URL is the one configuration that would turn this into a hard failure — it would hit the Caddy :80 301. Nothing in the build gate checks the scheme today.

The seam's own trait doc already stated this obligation; both transports were out of compliance with it. The doc is updated to say what it now means: no hop at all, because every target here is directly addressed and gated on the URL the engine supplied, so a hop the engine did not choose can only escape that gate.

Fixture changes

Pinning the challenge shape means test fixtures must mint what the API mints. crates/engine/src/facade.rs and packages/client/test/browser/mockAuth.ts used short suffixes (…:v2:abc, and a 16-char slice). Both now produce a 64-char lowercase-hex tail. The issue's note that "no harness change is needed" in mockAuth.ts held only for a bare prefix check.

What the three review gates changed

All three ran against this branch's own diff, with an independent reviewer pass on each. Findings folded into the second commit.

/security-review: no findings met the bar. It confirmed strip_prefix on an ASCII prefix is bytewise so no Unicode subtlety is reachable; that login_identity is the only caller of the only production ChallengeSigner; that no RefCell double-borrow is reachable through finish + Drop (it compiled and ran the exact shape); and that refusing redirects fails closed everywhere, since every consumer already treats 3xx as non-success.

/crypto-privacy-review: one real regression, which this branch introduced. Routing validate_byo_config through bearer_header turned a pure byte scan into one that built and dropped a non-zeroized "Bearer " + token — on every settings encode and decode, inside the module that declares itself the token's terminal zeroizing owner. Fixed by splitting check_bearer (the predicate) from bearer_header (the formatter). A caller that only asks the question never materializes a second copy of the credential.

It also confirmed domain separation is genuinely complete, which is the load-bearing claim behind #1034: all four sign_detcbor domains sign a det-CBOR container, so every preimage starts with a major-type-4/5 header (0xa2, 0xa3, 0x85) — never 0x63 ('c'). A challenge string can never be one of those preimages, nor they it. Worth recording that the pre-existing bug was worse than an internal forgery risk: sign_detcbor is single-SHA256 over caller bytes and the identity scalar is the raw Web3Auth secp256k1 export, so a hostile responder needed only to serve sha256(tx) — a Bitcoin sighash is sha256(sha256(tx)) — with the sole constraint that the 32 bytes be valid UTF-8. That was plausibly a transaction-signing oracle on a funded key. Pinning the shape closes it: no chosen digest can be placed in the preimage at all.

/simplify: the first commit was over-built in two places.

• The fallibility threaded through headers/kubo_block_put/pin_by_cid/probe_request was unreachable by construction — both public entries run validate_byo_config on the line above, and after this PR that gate is the same predicate. Four changed signatures and an uncoverable error arm that would read as a live path forever. Reverted; the shared rule stays at the gate, the shared name at the splice, and a new test pins that the two agree on every token. The genuinely ungated splices keep their check.
• Comment bloat: the refresh invariant was stated three times, the redirect rationale re-derived at each impl instead of cited from the trait, and is_identity_challenge's doc argued against a design not in the code while hardcoding a byte count that would rot. finish and Drop both released leadership; waiters now only takes.

Tests strengthened by the reviews. The hostile-challenge test proved the flow never sent; it now runs against a signer that panics if invoked, so it proves the key never signed. Cases added for the bare tag, mixed case, a wholly attacker-chosen tail, both bytes-vs-chars boundaries, and version confusion, plus an accept-side boundary test. The API side gains the matching /^cipherbox-login:v2:[0-9a-f]{64}$/ assertion, so narrowing randomBytes(32) or switching to base64url fails there rather than silently breaking every login. And the no-redirect obligation, previously documented but unchecked on the Rust host, now has a live 302 route in the desktop mock proving the seam surfaces the hop rather than taking it.

Scope note: crates/load

Three reviewers independently flagged that crates/load undercut the fix — runner.rs:91 still hand-spliced an unvalidated bearer, and seams.rs is the one ReqwestHttp::with_client caller, so it built a client with no redirect policy and followed up to ten hops. Its own module doc says the harness exists to exercise "the same byte mover the shipping client uses", which it had stopped doing. Both are fixed here. It is two additive lines in a workflow_dispatch-only crate that no sibling branch owns, and leaving it would have meant "one home" still had two.

Deliberately not done

• A zeroizing header-value type. engine: give the Authorization header value one home across every bearer splice site #933's tail notes the bearer leaves its Zeroizing owner the moment it becomes a HttpRequest.headers String. Changing that type ripples through net/, sync/, crates/wasm, desktop-seams, and crates/load. Both reviewers agreed it belongs in its own PR, and that this one is the right precondition — the eventual fix becomes a one-line signature change now that the splice has a single home. Worth a follow-up issue with a dependency edge to this PR.
• Challenge origin binding. Both reviewers noted the pinned challenge is still not bound to the API origin: an adversary controlling apiBaseUrl can relay a genuine challenge, harvest the signature, and complete the login itself. Shape pinning cannot close that — only channel binding can, and it needs an API-side change. Pre-existing, not widened here, and an attacker who controls apiBaseUrl already owns the session.
• crates/engine/src/api/signer.rs and session.rs fixtures still use …:v2:deadbeef. sign_challenge does not validate, so nothing fails; left alone to keep the diff off files this PR has no other reason to touch.

One residual worth knowing

A leader dropped after the server rotates but before store_tokens persists leaves the stored refresh token spent. With this fix the next caller leads a fresh rotation and replays it, and the API hard-deletes the whole token family on reuse — so every device is logged out, where previously the slot simply wedged. Not attacker-triggerable, and there is no production cancellation point today (no select!, race, or task abort anywhere in crates/engine/src, crates/wasm/src, or crates/desktop-seams/src); both outcomes end the session. Recorded rather than coded around.

Gates

cargo fmt --all --check, cargo clippy --workspace --all-targets, cargo test -p cipherbox-engine, cargo check -p cipherbox-wasm --target wasm32-unknown-unknown, pnpm typecheck, pnpm lint, pnpm lint:tracker-refs, pnpm test — all green, on a clean tree before and after.

Note

Pin login challenge format and centralize bearer header construction across all HTTP transports

• Adds is_identity_challenge in api/client.rs to validate that server-issued challenges match cipherbox-login:v2:[0-9a-f]{64} exactly; the client refuses to sign non-conforming challenges.
• Centralizes bearer token validation and Authorization header construction in seams/http.rs via check_bearer and bearer_header, replacing duplicated ad-hoc logic in api/client.rs, content/read.rs, content/provider.rs, and load/runner.rs.
• Unusable access tokens in request_authed are now dropped and surface a decode error rather than being sent; the next call proceeds unauthenticated and can re-authenticate via 401.
• Sets redirect: 'error' on FetchHttp and Policy::none() on all reqwest clients so 3xx responses are surfaced rather than followed, preventing silent Authorization header replay to redirect targets.
• Behavioral Change: all redirects are now refused across web client, desktop, and load-test transports; previously some redirects were followed or capped at 10 hops.

^{Macroscope summarized bdb8309.}

Summary by CodeRabbit

• Security

  • HTTP requests no longer follow redirects, helping prevent credentials from being sent to unintended destinations.
  • Bearer tokens are now validated consistently, rejecting empty, malformed, or unsafe values.
  • Invalid credentials fail safely before requests are sent.

• Reliability

  • Content retrieval can skip invalid authenticated sources and try configured fallback sources.
  • Authentication challenge validation now requires the expected version and secure 64-character hexadecimal format.
  • Token refresh cancellation is handled more reliably, reducing stalled authentication requests.

[coderabbitai]

Walkthrough

The PR centralizes bearer-token validation, enforces canonical identity challenge formats, handles cancelled refresh leaders, and disables redirect following across Rust and TypeScript HTTP transports.

Changes

Authentication and redirect hardening

Layer / File(s)	Summary
Shared bearer validation contract crates/engine/src/seams/http.rs, crates/engine/src/seams/mod.rs	The HTTP seam now validates visible-ASCII bearer tokens and builds authorization headers through shared helpers.
Bearer validation integration crates/engine/src/api/client.rs, crates/engine/src/content/*, crates/load/src/runner.rs	API, content, and gateway requests reject invalid credentials before transport and preserve credential-less behavior.
Identity challenge and refresh handling crates/engine/src/api/client.rs, crates/engine/src/facade.rs, apps/api/src/auth/services/challenge.service.test.ts, packages/client/test/browser/mockAuth.ts	Identity login requires the v2 prefix and a 64-character lowercase hexadecimal nonce. Refresh cancellation releases leadership and reports transport failures to waiters.
Redirect refusal across transports crates/desktop-seams/src/http.rs, crates/load/src/seams.rs, packages/client/src/seams/http.ts, crates/desktop-seams/tests/*, packages/client/src/seams/http.test.ts	HTTP clients reject or surface redirects without following them. Desktop conformance tests verify that bearer authorization is not replayed.
Estimated code review effort: 4 (Complex)	~45 minutes

Possibly related PRs

• FSM1/cipher-box#671: Extends the same API client and HTTP seam areas with authentication and transport handling.
• FSM1/cipher-box#943: Also updates identity challenge and nonce validation in the API challenge test and engine client.
• FSM1/cipher-box#1023: Also hardens HTTP seams by disabling redirects and tightening credential handling.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately identifies two major changes: strict login challenge validation and centralized bearer-header construction.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/1034-login-challenge-prefix-and-bearer-seam-hardening

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

crates/desktop-seams/src/http.rs (1)

37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

State the credential-scope rationale in the redirect comments.

The current comments describe configuration, expected output, or test mechanics. State why redirect refusal is required: a redirect can send a bearer-bearing request to a different target.

• crates/desktop-seams/src/http.rs#L37-L37: replace the contract reference with the redirect and credential-scope rationale.
• packages/client/src/seams/http.ts#L28-L29: replace the Fetch-mode description with the same rationale.
• crates/desktop-seams/tests/conformance.rs#L340-L342: remove the CI-mechanics statement and retain the rationale for the live-server test.

As per coding guidelines: comments should explain why rather than what and avoid duplicated happy-path narration.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/desktop-seams/src/http.rs` at line 37, Update the redirect-related
comments to state the credential-scope rationale: refusing redirects prevents
bearer-bearing requests from being sent to a different target. In
crates/desktop-seams/src/http.rs lines 37-37 and
packages/client/src/seams/http.ts lines 28-29, replace the existing
configuration or contract descriptions with this rationale; in
crates/desktop-seams/tests/conformance.rs lines 340-342, remove the CI-mechanics
explanation and retain the same rationale for the live-server test.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/engine/src/api/client.rs`:
- Around line 547-551: Update send_with_token to detect an unusable access
token, release the state.borrow() scope, then asynchronously call clear_session
before returning the decode error. Preserve valid-token header construction, and
ensure the token is removed so the next authenticated request can proceed
unauthenticated and trigger the existing 401 refresh flow.

In `@packages/client/src/seams/http.test.ts`:
- Around line 98-107: Update the FetchHttp redirect test and its recordingFetch
stub so requests with redirect: 'error' reject at runtime instead of always
resolving a 200 response. Assert that both send() and sendCapped() reject, while
retaining coverage that each request uses redirect: 'error'.

---

Nitpick comments:
In `@crates/desktop-seams/src/http.rs`:
- Line 37: Update the redirect-related comments to state the credential-scope
rationale: refusing redirects prevents bearer-bearing requests from being sent
to a different target. In crates/desktop-seams/src/http.rs lines 37-37 and
packages/client/src/seams/http.ts lines 28-29, replace the existing
configuration or contract descriptions with this rationale; in
crates/desktop-seams/tests/conformance.rs lines 340-342, remove the CI-mechanics
explanation and retain the same rationale for the live-server test.

🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: e158934f-707e-4e05-bf8d-a0155e04fe2a

📥 Commits

Reviewing files that changed from the base of the PR and between 3d087d6 and 792ec38.

📒 Files selected for processing (15)

• apps/api/src/auth/services/challenge.service.test.ts
• crates/desktop-seams/src/http.rs
• crates/desktop-seams/tests/conformance.rs
• crates/desktop-seams/tests/mock_http/mod.rs
• crates/engine/src/api/client.rs
• crates/engine/src/content/provider.rs
• crates/engine/src/content/read.rs
• crates/engine/src/facade.rs
• crates/engine/src/seams/http.rs
• crates/engine/src/seams/mod.rs
• crates/load/src/runner.rs
• crates/load/src/seams.rs
• packages/client/src/seams/http.test.ts
• packages/client/src/seams/http.ts
• packages/client/test/browser/mockAuth.ts