[jfrench9]

Summary

Hardening from a platform security review; companion to the robosystems bugfix/input-validation-hardening PR and robosystems-core #98. Public text is deliberately brief.

• The login home forwards the product app's ?nonce_hash= into the SSO exchange, so a handoff completes only in the browser that started the bridge
• Without one, the browser is sent to the product app's own login, which starts a bound bridge
• parseNonceHash accepts a SHA-256 hex digest and nothing else

Waits on

@robosystems/core minor from #98 (navigateToApp options). Typecheck is red until that publishes; the bump of every @robosystems/* package follows in this PR. Deploy order: API → core publish → this app → the product apps.

Test plan

• vitest — 726 passed (pre-commit; new nonce cases in SignInForm.test.tsx, return-to.test.ts)
• typecheck against the packed core branch

🤖 Generated with Claude Code

[github-actions]

Change classification: normal — platform or application code changed.

A normal change carries a documented automated review on this pull request before merge, in place of a second human approver. The change-classification job requests that review and passes once it has been posted. Recorded by the change-classification job.

[jfrench9]

@claude please review this PR

[claude]

Claude finished @jfrench9's task in 21s —— View job

---

Review of #518

The change is small and the nonce validation is tight (/^[0-9a-f]{64}$/, lowercase only, with tests). I found one likely functional problem and a few minor points. I couldn't run typecheck, which the PR says is red until core publishes.

1. The loop-breaker marker is set before the no-nonce redirect (likely bug)

• SignInForm.tsx:165 calls markBridgeAttempt(rawReturnTo) before navigateToApp(...). The same happens at line 324 for the interstitial.
• When nonce_hash is absent, navigateToApp doesn't bridge. It sends the browser to the product app's /login, which starts a bound bridge and returns here with ?return_to=…&nonce_hash=….
• That return trip lands within the 60s TTL, so shouldSuppressAutoBridge(rawReturnTo) (line ~219) is true. The user then gets the manual "Continue" interstitial instead of the silent bridge.
• This only costs an extra click, not a loop. The click goes through bridgeNonceHash() and now succeeds. It still defeats the point of the silent path for every flow that arrives without a nonce.
• Suggested fix: call markBridgeAttempt only when a nonce hash is present, i.e. when a real handoff is being attempted. Alternatively, have navigateToApp report which branch it took. A test should cover this: render without a nonce, assert the redirect, then assert that shouldSuppressAutoBridge is false.
• Please confirm against the core implementation, since I couldn't read navigateToApp in the installed package.

2. The nonce is read inside the click and bridge handlers, not from component state

• bridgeNonceHash() reads window.location.search at call time. That is fine here and matches the "read after mount" convention in return-to.ts. It could also live in return-to.ts next to parseNonceHash, since that file already owns the query parsing.
• Neither useCarriedAuthParams nor CARRIED_PARAMS carries nonce_hash. If a user toggles to register or forgot-password and back to login, the nonce is dropped. They will then be bounced to the product app's login and start over. That is safe but worth knowing. Carrying it is optional.

3. Test coverage

• The new cases cover:
  • the nonce being forwarded as browser_nonce_hash;
  • the no-nonce redirect;
  • parseNonceHash edge cases.
• The tests at SignInForm.test.tsx:167 and :207 only add the nonce so the existing flows keep passing.
• The test titled "bridges onward after a manual sign-in" doesn't assert that the nonce is forwarded. Adding the browser_nonce_hash assertion there would cover the post-login path (line ~163) too.
• The interstitial path (line ~324) also has no explicit assertion.

4. Merge ordering

• The PR depends on an unpublished @robosystems/core release, so CI stays red until the version bump lands in this PR. Don't merge before that bump is committed and typecheck is green.

Security notes

• Strict hex validation means there is no injection path through nonce_hash.
• Failing closed to the product app's own login when the nonce is missing or malformed is the right default.
  · branch bugfix/input-validation-hardening

[jfrench9]

Companion PRs for the one release: robosystems RoboFinSystems/robosystems#1945 · robosystems-core RoboFinSystems/robosystems-core#98 · robosystems-typescript-client RoboFinSystems/robosystems-typescript-client#302 · robosystems-python-client RoboFinSystems/robosystems-python-client#286 · robosystems-app #518 · roboledger-app RoboFinSystems/roboledger-app#527