Skip to content

fix(web): guard secure-context-only crypto in browser client - #5471

Merged
Jinwoo-H merged 2 commits into
mainfrom
Jinwoo-H/web-interface-bug
Jun 16, 2026
Merged

Jinwoo-H merged 2 commits into
mainfrom
Jinwoo-H/web-interface-bug

Conversation

@Jinwoo-H

@Jinwoo-H Jinwoo-H commented Jun 16, 2026 •

Copy link
Copy Markdown
Contributor

Problem

A user reported the web client (Orca in the browser) throwing crypto.randomUUID is not a function and failing to work reliably.

The web client is frequently served over plain HTTP on a LAN, which is not a secure context. In that case the browser leaves crypto.randomUUID() and crypto.subtle undefined (only crypto.getRandomValues is available). Electron always runs in a secure context, so unguarded crypto.* calls pass locally and in CI but crash in the browser — which is why this slipped through (the default vitest env is node, where crypto.randomUUID exists globally).

Reproduction

Added non-secure-context-crypto.repro.test.ts, which recreates the exact non-secure-context global shape (getRandomValues present, randomUUID/subtle undefined) and drives the real call sites. Against the pre-fix code it fails with:

TypeError: Cannot read properties of undefined (reading 'digest')
  at hashOrcaHookScript src/renderer/src/lib/orca-hook-trust.ts:6

With the fix, it passes.

Fix

Route the three unguarded renderer call sites through non-secure-context-safe code:

Site Before After
worktree-creation-flow.ts crypto.randomUUID() createBrowserUuid()
launch-agent-background-session.ts globalThis.crypto.randomUUID() createBrowserUuid()
orca-hook-trust.ts crypto.subtle.digest(...) guard crypto.subtle; fall back to a JS hash (tweetnacl, already bundled)

The hook-trust change keeps crypto.subtle as the primary path so existing stored trust hashes stay valid on Electron/HTTPS, and only falls back off a secure context.

createBrowserUuid() / mintStablePaneId() already existed and were the intended pattern — these sites just weren't using them.

Scope / what this does NOT cover

The reproduced crashes are on action paths (creating a worktree, launching an agent, hook-trust during launch). The reporter also mentioned "sessions are empty and one project is missing" on load. I traced the passive session/repo hydration path (web RPC ids use a counter, not crypto; tab hydration already uses createBrowserUuid) and did not find a crypto cause there. That symptom may be a cascade from a thrown exception during launch, or a separate bug. Recommend asking the reporter to retest loading after this lands; if sessions are still empty, I'll open a separate investigation into the web hydration path.

Testing

  • New repro test (non-secure-context-crypto.repro.test.ts) — 3 passing, demonstrably fails on pre-fix code
  • New orca-hook-trust.test.ts — covers both the secure path and the non-secure fallback
  • Existing ensure-hooks-confirmed.test.ts — 16 passing
  • typecheck:web and oxlint clean

Made with Orca 🐋


Open in Stage

The web client is often served over plain HTTP on a LAN, which is not a
secure context. crypto.randomUUID() and crypto.subtle are undefined there
(crypto.getRandomValues is not), so unguarded calls throw
"crypto.randomUUID is not a function" — passing in Electron but crashing
in the browser.

Route the three unguarded renderer call sites through the existing
non-secure-context-safe helpers:
- worktree-creation-flow.ts, launch-agent-background-session.ts ->
  createBrowserUuid()
- orca-hook-trust.ts -> keep crypto.subtle where present (so existing
  trust hashes stay valid on Electron/HTTPS) and fall back to a JS hash
  (already-bundled tweetnacl) off a secure context.

Adds a reproduction test that recreates the non-secure-context global
shape and drives the real call sites; it fails on the pre-fix code with
"Cannot read properties of undefined (reading 'digest')".

Co-authored-by: Orca <help@stably.ai>
@ghost

ghost commented Jun 16, 2026 •

Copy link
Copy Markdown

Ready to review this PR? Stage has broken it down into 4 individual chapters for you:

Title
1 Implement fallback SHA-256 for non-secure contexts
2 Guard hook trust hashing with fallback
3 Switch UUID generation to safe wrapper
4 Verify fix with non-secure context reproduction
Open in Stage

Chapters generated by Stage for commit 5358d9b on Jun 16, 2026 5:30am UTC.

@coderabbitai

coderabbitai Bot commented Jun 16, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 26987855-c221-4714-997c-8e7c950cbacc

📥 Commits

Reviewing files that changed from the base of the PR and between 3c4ae81 and 5358d9b.

📒 Files selected for processing (4)
  • src/renderer/src/lib/non-secure-context-crypto.repro.test.ts
  • src/renderer/src/lib/orca-hook-trust.ts
  • src/renderer/src/lib/sha256.test.ts
  • src/renderer/src/lib/sha256.ts
✅ Files skipped from review due to trivial changes (1)
  • src/renderer/src/lib/sha256.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/renderer/src/lib/non-secure-context-crypto.repro.test.ts

📝 Walkthrough

Walkthrough

This PR adds non-secure browser context (plain HTTP) support for cryptographic operations by introducing a self-contained SHA-256 fallback implementation and migrating crypto-dependent code to use fallback-aware alternatives. The sha256.ts module provides a standalone SHA-256 hash function for environments lacking crypto.subtle. hashOrcaHookScript is updated to detect globalThis.crypto?.subtle at runtime and conditionally use the native API or the fallback. Two call sites—launchAgentBackgroundSession and runBackgroundWorktreeCreation—replace direct crypto.randomUUID() calls with createBrowserUuid(). A reproduction test file validates all fallback paths by simulating a non-secure context with a minimal crypto object containing only getRandomValues.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: guarding secure-context-only crypto functions in the browser client to fix crashes on non-HTTPS contexts.
Description check ✅ Passed The description is comprehensive and covers all required template sections: problem statement, reproduction steps, solution details, testing approach, and security/platform considerations are addressed.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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 and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/renderer/src/lib/orca-hook-trust.test.ts (1)

21-30: ⚡ Quick win

Test doesn't verify hash compatibility between secure and non-secure contexts.

The test confirms the fallback path works in isolation, but doesn't verify that the SHA-512 fallback hash differs from the SHA-256 crypto.subtle hash for identical input. Given that trust persistence relies on hash stability (see ensure-hooks-confirmed.ts context snippet), consider adding an assertion that documents this mismatch:

it('fallback produces different hashes than crypto.subtle (SHA-512 vs SHA-256)', async () => {
  const contentSHA256 = await hashOrcaHookScript('echo hi')
  stubCrypto(undefined)
  const contentSHA512 = await hashOrcaHookScript('echo hi')
  expect(contentSHA256).not.toBe(contentSHA512)
  expect(contentSHA512.length).toBeGreaterThan(contentSHA256.length) // SHA-512 is 64 bytes
})

This would surface the cross-context trust re-prompting behavior during code review.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 3e590e52-d139-4d97-9869-68f5bf91577b

📥 Commits

Reviewing files that changed from the base of the PR and between 319402a and 3c4ae81.

📒 Files selected for processing (5)
  • src/renderer/src/lib/launch-agent-background-session.ts
  • src/renderer/src/lib/non-secure-context-crypto.repro.test.ts
  • src/renderer/src/lib/orca-hook-trust.test.ts
  • src/renderer/src/lib/orca-hook-trust.ts
  • src/renderer/src/lib/worktree-creation-flow.ts

Comment thread src/renderer/src/lib/orca-hook-trust.ts Resolved
CodeRabbit flagged that the non-secure-context fallback used nacl.hash
(SHA-512) while the crypto.subtle path uses SHA-256. trustedOrcaHooks is
a shared per-repo store, so a hook approved on Electron/HTTPS (SHA-256)
would mismatch when re-hashed in the HTTP web client (SHA-512), forcing a
spurious re-approval.

Add a self-contained JS SHA-256 (sha256.ts) and use it for the fallback
so both paths yield byte-identical digests. Tests cover NIST known-answer
vectors, a cross-check against crypto.subtle across block boundaries, and
cross-context hash equality.

Co-authored-by: Orca <help@stably.ai>
@Jinwoo-H
Jinwoo-H merged commit 88b8cdb into main Jun 16, 2026
1 check passed
@Jinwoo-H
Jinwoo-H deleted the Jinwoo-H/web-interface-bug branch June 16, 2026 06:29
AmethystLiang added a commit that referenced this pull request Jul 15, 2026
…tests

PR #8804 replaced two unguarded crypto.randomUUID() calls with
createBrowserUuid(), matching the convention established in #5471, but
shipped without extending that PR's regression-test pattern to the new
call sites. Add hook-level tests for useMarkupPointerHandlers.onPointerDown
and useMarkupEditor.commitPendingText under a simulated non-secure-context
crypto global.

Co-authored-by: Orca <help@stably.ai>
AmethystLiang added a commit that referenced this pull request Jul 15, 2026
* fix: address pr-bug-scan validated finding from #6335

Replaced unguarded crypto.randomUUID() with the established createBrowserUuid() fallback in both markup shape-id call sites.

* fix(review): cover markup crypto fallback call sites with regression tests

PR #8804 replaced two unguarded crypto.randomUUID() calls with
createBrowserUuid(), matching the convention established in #5471, but
shipped without extending that PR's regression-test pattern to the new
call sites. Add hook-level tests for useMarkupPointerHandlers.onPointerDown
and useMarkupEditor.commitPendingText under a simulated non-secure-context
crypto global.

Co-authored-by: Orca <help@stably.ai>

* fix(review): consolidate annotation bridge token onto createBrowserUuid

BrowserPane had its own inline crypto.randomUUID guard with a
Date.now()+Math.random() fallback, duplicating the getRandomValues-based
fallback already centralized in createBrowserUuid() and used at every
other call site in the renderer.

Co-authored-by: Orca <help@stably.ai>

---------

Co-authored-by: orca-bug-scan-bot <orca-bug-scan-bot@stably.ai>
Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
Co-authored-by: Orca <help@stably.ai>
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
* fix: address pr-bug-scan validated finding from stablyai#6335

Replaced unguarded crypto.randomUUID() with the established createBrowserUuid() fallback in both markup shape-id call sites.

* fix(review): cover markup crypto fallback call sites with regression tests

PR stablyai#8804 replaced two unguarded crypto.randomUUID() calls with
createBrowserUuid(), matching the convention established in stablyai#5471, but
shipped without extending that PR's regression-test pattern to the new
call sites. Add hook-level tests for useMarkupPointerHandlers.onPointerDown
and useMarkupEditor.commitPendingText under a simulated non-secure-context
crypto global.

Co-authored-by: Orca <help@stably.ai>

* fix(review): consolidate annotation bridge token onto createBrowserUuid

BrowserPane had its own inline crypto.randomUUID guard with a
Date.now()+Math.random() fallback, duplicating the getRandomValues-based
fallback already centralized in createBrowserUuid() and used at every
other call site in the renderer.

Co-authored-by: Orca <help@stably.ai>

---------

Co-authored-by: orca-bug-scan-bot <orca-bug-scan-bot@stably.ai>
Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
Co-authored-by: Orca <help@stably.ai>
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