fix(ui): guard sessionStorage access against strict-privacy crash (#1453)#1563
fix(ui): guard sessionStorage access against strict-privacy crash (#1453)#1563meetp06 wants to merge 2 commits into
Conversation
…cketride-org#1453) Bare `sessionStorage` access throws a `SecurityError` in strict privacy modes (Safari Private Browsing, Chrome Incognito with third-party storage blocked, or when the UI is embedded in a sandboxed iframe). Because these calls run at module-eval time (clientSingleton) and during the initial React render (App.tsx), the unhandled throw aborts the render cycle and the user gets a blank white screen — a denial of service. Add a `safeSessionStorage` helper (try/catch wrappers that fall back to null / no-op) in each affected app and route the previously-unguarded sessionStorage calls through it: - apps/chat-ui/src/App.tsx - apps/dropper-ui/src/App.tsx - apps/dropper-ui/src/hooks/clientSingleton.ts - apps/shell-ui/src/util/pkce.ts The shell-ui auth providers, WorkspaceContext, connection.ts and Shell.tsx already wrap their sessionStorage access, so they are left unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds guarded ChangesSafe session storage integration
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/chat-ui/src/utils/safeStorage.tsOops! Something went wrong! :( ESLint: 9.39.4 TypeError: expand is not a function apps/dropper-ui/src/utils/safeStorage.tsOops! Something went wrong! :( ESLint: 9.39.4 TypeError: expand is not a function apps/shell-ui/src/util/safeStorage.tsOops! Something went wrong! :( ESLint: 9.39.4 TypeError: expand is not a function 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@apps/chat-ui/src/utils/safeStorage.ts`:
- Around line 37-71: Extract the duplicated safeSessionStorage implementation
from apps/chat-ui/src/utils/safeStorage.ts#L37-L71 into a shared package or
workspace utility, then replace the copies in
apps/dropper-ui/src/utils/safeStorage.ts#L37-L71 and
apps/shell-ui/src/util/safeStorage.ts#L37-L71 with imports from that shared
location. Preserve the getItem, setItem, and removeItem behavior; if extraction
is not feasible, add synchronization comments to all three files.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 7f8dcdb5-571c-47da-8f72-cd8b8dd29104
📒 Files selected for processing (7)
apps/chat-ui/src/App.tsxapps/chat-ui/src/utils/safeStorage.tsapps/dropper-ui/src/App.tsxapps/dropper-ui/src/hooks/clientSingleton.tsapps/dropper-ui/src/utils/safeStorage.tsapps/shell-ui/src/util/pkce.tsapps/shell-ui/src/util/safeStorage.ts
| export const safeSessionStorage = { | ||
| /** | ||
| * Read a value from `sessionStorage`, returning `null` if storage is | ||
| * unavailable or the key is missing. | ||
| */ | ||
| getItem(key: string): string | null { | ||
| try { | ||
| return sessionStorage.getItem(key); | ||
| } catch { | ||
| return null; | ||
| } | ||
| }, | ||
|
|
||
| /** | ||
| * Write a value to `sessionStorage`. No-ops if storage is unavailable. | ||
| */ | ||
| setItem(key: string, value: string): void { | ||
| try { | ||
| sessionStorage.setItem(key, value); | ||
| } catch { | ||
| /* storage unavailable — degrade gracefully */ | ||
| } | ||
| }, | ||
|
|
||
| /** | ||
| * Remove a value from `sessionStorage`. No-ops if storage is unavailable. | ||
| */ | ||
| removeItem(key: string): void { | ||
| try { | ||
| sessionStorage.removeItem(key); | ||
| } catch { | ||
| /* storage unavailable — degrade gracefully */ | ||
| } | ||
| }, | ||
| }; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Three identical safeStorage.ts files — consider extracting to a shared package.
The same 71-line safeSessionStorage wrapper is duplicated verbatim across three apps. If a bug is found or a method (e.g., clear) needs to be added, all three copies must be updated in lockstep.
apps/chat-ui/src/utils/safeStorage.ts#L37-L71: extract to a shared package or workspace utility.apps/dropper-ui/src/utils/safeStorage.ts#L37-L71: re-import from the shared location instead of maintaining a copy.apps/shell-ui/src/util/safeStorage.ts#L37-L71: re-import from the shared location instead of maintaining a copy.
If a shared package isn't feasible in this monorepo right now, at minimum add a comment in each file pointing to the others so future changes stay synchronized.
📍 Affects 3 files
apps/chat-ui/src/utils/safeStorage.ts#L37-L71(this comment)apps/dropper-ui/src/utils/safeStorage.ts#L37-L71apps/shell-ui/src/util/safeStorage.ts#L37-L71
🤖 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 `@apps/chat-ui/src/utils/safeStorage.ts` around lines 37 - 71, Extract the
duplicated safeSessionStorage implementation from
apps/chat-ui/src/utils/safeStorage.ts#L37-L71 into a shared package or workspace
utility, then replace the copies in
apps/dropper-ui/src/utils/safeStorage.ts#L37-L71 and
apps/shell-ui/src/util/safeStorage.ts#L37-L71 with imports from that shared
location. Preserve the getItem, setItem, and removeItem behavior; if extraction
is not feasible, add synchronization comments to all three files.
Address review feedback (CodeRabbit) on rocketride-org#1563: the safeSessionStorage helper is duplicated across chat-ui, dropper-ui and shell-ui. A shared workspace package is not a clean fit — chat-ui/dropper-ui do not depend on `shared` (packages/shared-ui) and the only common dependency, `rocketride`, is the public SDK client (wrong home for a browser-only UI util, and would expand its public surface). Rather than add cross-package dependencies, add a sync note to each copy so future fixes are applied in lockstep. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the review. On the duplicated |
1 participant
Fixes #1453
Problem
Bare
sessionStorageaccess throws aSecurityErrorin strict privacy modes (Safari Private Browsing, Chrome Incognito with third-party storage blocked, or when the UI is embedded in a sandboxed iframe). Because these calls run at module-eval time (clientSingleton.ts) and during the initial React render (App.tsx), the unhandled throw aborts the render cycle and the user gets a blank white screen — a denial of service.Fix
Add a small
safeSessionStoragehelper (try/catch wrappers falling back tonull/ no-op) in each affected app and route the previously-unguarded calls through it:apps/chat-ui/src/App.tsxapps/dropper-ui/src/App.tsxapps/dropper-ui/src/hooks/clientSingleton.tsapps/shell-ui/src/util/pkce.tsThis mirrors the safe pattern already used in
apps/shell-ui/src/connection/connection.ts.Scope note
Audited every
sessionStorage/localStorageaccess acrosschat-ui,dropper-ui, andshell-ui. The shell-ui auth providers (ApiKeyAuthProvider,CloudAuthProvider),WorkspaceContext,connection.tsandShell.tsxalready wrap their access in try/catch, so they are intentionally left unchanged. Only the genuinely-unguarded sites are touched.Behavior
When storage is unavailable the app now degrades to in-memory/limited functionality instead of crashing — reads return
null, writes/removes no-op.Testing
Static change only (import + identifier swap, signatures identical to native
sessionStorage). Local type-check not run — monorepo toolchain (pnpm) is not installed in this environment.🤖 Generated with Claude Code
Summary by CodeRabbit