WIP: Various updates for AI review - #646
Conversation
|
🚅 Deployed to the openchat-pr-646 environment in OpenChat
|
|
| Severity | Vulnerability Type | File | Line(s) |
|---|---|---|---|
| Medium | Session Token Storage Regression (XSS attack surface) | apps/web/src/lib/auth-client.tsx |
29-32 |
Medium: Session Token Storage Regression — apps/web/src/lib/auth-client.tsx
What changed: The custom inMemoryStorage adapter for crossDomainClient() was removed, reverting to the plugin's default storage (which uses localStorage). The previous implementation explicitly kept session tokens in memory only to reduce XSS token theft risk.
Previous (secure) code:
crossDomainClient({
storage: inMemoryStorage,
disableCache: true,
}),New code:
crossDomainClient(),Why this matters: With the default storage, crossDomainClient will persist session-related tokens in localStorage. If any XSS vulnerability is introduced anywhere in the application, an attacker can trivially read localStorage to exfiltrate these tokens. The previous in-memory-only approach eliminated this persistence, narrowing the attack surface. While the primary session is maintained via HttpOnly cookies (which remain safe from JS access), the cross-domain tokens stored in localStorage could be used to establish sessions on other domains or replay auth flows.
Recommended fix: If the inMemoryStorage was removed to fix the infinite loop issue, consider keeping a storage adapter that doesn't persist to localStorage while also incorporating the new server-side session hydration (initialUser). For example:
const inMemoryStorage = {
_store: new Map<string, string>(),
setItem: (key: string, value: string) => {
if (inMemoryStorage._store.get(key) === value) return;
inMemoryStorage._store.set(key, value);
},
getItem: (key: string) => inMemoryStorage._store.get(key) ?? null,
removeItem: (key: string) => { inMemoryStorage._store.delete(key); },
};
// Then use:
crossDomainClient({
storage: inMemoryStorage,
}),This preserves the XSS-hardening benefit while still allowing the new initialUser server-side hydration to work (since that flows through StableAuthProvider, not through crossDomainClient's storage).
Summary: 1 medium severity finding.
Scanned commit: c363ebc
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 issues found across 14 files
Confidence score: 3/5
- Potential auth state ambiguity in
apps/web/src/lib/auth-client.tsx:isAuthenticatedcan be true whilesessionis null withloadingfalse, which could mislead consumers into treating users as fully authenticated - Route blocking risk in
apps/web/src/routes/__root.tsxsince the sessionfetchhas no timeout inbeforeLoad, so a slow auth service could hang page rendering - Pay close attention to
apps/web/src/lib/auth-client.tsxandapps/web/src/routes/__root.tsx- partially initialized auth state and blocking session fetch behavior.
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/src/styles.css">
<violation number="1" location="apps/web/src/styles.css:184">
P2: Hardcoded `oklch` color values don't follow the project's theming pattern. The rest of the file uses CSS custom properties (e.g., `var(--muted-foreground)`) that adapt to dark/light mode. Consider using theme variables so the scrollbar appearance is consistent across themes.
For example, you could use `color-mix` or define dedicated scrollbar color variables in `:root` and `.dark`.</violation>
</file>
<file name="apps/web/src/components/model-selector.tsx">
<violation number="1" location="apps/web/src/components/model-selector.tsx:240">
P2: `TooltipProvider` is instantiated inside each `ModelItem`, creating a new context provider per list item. Since `ModelItem` is rendered for every model in the list, this creates N provider instances. Move the `TooltipProvider` out of `ModelItem` and wrap the entire list (or the `ModelSelector` component) with a single `TooltipProvider` instead.</violation>
</file>
<file name="apps/web/src/lib/benchmark-formatting.ts">
<violation number="1" location="apps/web/src/lib/benchmark-formatting.ts:41">
P2: JSDoc comment is stale: it still documents the old thresholds (≥70, 40-69, <40) and color name (red) but the code now uses ≥30, 15-29, <15 and rose. Update the comment to match the new logic.</violation>
</file>
<file name="apps/web/src/routes/__root.tsx">
<violation number="1" location="apps/web/src/routes/__root.tsx:34">
P2: The `fetch` to the auth session endpoint has no timeout. Since this runs in `beforeLoad` (which blocks route rendering), a slow or unresponsive auth service will cause the entire page load to hang. Consider adding an `AbortSignal.timeout()` to bound the wait time.</violation>
</file>
<file name="apps/web/src/components/model-info-panel.tsx">
<violation number="1" location="apps/web/src/components/model-info-panel.tsx:235">
P2: Hardcoded `"OpenRouter"` replaces dynamic `{model.provider}`. This field will always show the same value regardless of the actual model provider, making it redundant and uninformative. If the intent is to distinguish API provider from developer, consider using a constant or deriving this from configuration rather than a hardcoded string.</violation>
</file>
<file name="apps/web/src/lib/auth-client.tsx">
<violation number="1" location="apps/web/src/lib/auth-client.tsx:89">
P1: `isAuthenticated` is `true` while `session` is `null` when `initialUser` is provided. Since `loading` is also `false`, consumers have no way to distinguish this partially-initialized state from a fully authenticated one. Any code that accesses `session.id` or `session.token` after checking `isAuthenticated && !loading` will throw a null reference error. Consider keeping `loading: true` until `fetchSession` completes, or documenting that `session` may be null even when authenticated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| <span className="text-muted-foreground">Provider</span> | ||
| <p className="font-medium text-foreground">{model.provider}</p> | ||
| <span className="text-[11px] text-muted-foreground">Provider</span> | ||
| <p className="text-xs font-medium text-foreground">OpenRouter</p> |
There was a problem hiding this comment.
P2: Hardcoded "OpenRouter" replaces dynamic {model.provider}. This field will always show the same value regardless of the actual model provider, making it redundant and uninformative. If the intent is to distinguish API provider from developer, consider using a constant or deriving this from configuration rather than a hardcoded string.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/components/model-info-panel.tsx, line 235:
<comment>Hardcoded `"OpenRouter"` replaces dynamic `{model.provider}`. This field will always show the same value regardless of the actual model provider, making it redundant and uninformative. If the intent is to distinguish API provider from developer, consider using a constant or deriving this from configuration rather than a hardcoded string.</comment>
<file context>
@@ -176,89 +182,83 @@ export function ModelInfoPanel({ model, className }: ModelInfoPanelProps) {
- <span className="text-muted-foreground">Provider</span>
- <p className="font-medium text-foreground">{model.provider}</p>
+ <span className="text-[11px] text-muted-foreground">Provider</span>
+ <p className="text-xs font-medium text-foreground">OpenRouter</p>
</div>
<div>
</file context>
| <p className="text-xs font-medium text-foreground">OpenRouter</p> | |
| <p className="text-xs font-medium text-foreground">{model.provider}</p> |
- auth-client: keep loading=true until fetchSession completes - benchmark-formatting: update stale JSDoc comment - __root.tsx: add timeout to auth fetch (3s) - styles.css: use theme variables for scrollbar colors - model-selector: hoist TooltipProvider outside ModelItem loop
|
✅ No security issues found — scanned commits: |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/web/src/lib/auth-client.tsx">
<violation number="1" location="apps/web/src/lib/auth-client.tsx:89">
P1: Setting `loading` to `true` unconditionally defeats the server-side user hydration. When `initialUser` is provided, the user data is already available in `sessionData`, but consumers will still see `loading: true` and show loading indicators until the async `fetchSession` completes — reintroducing the exact loading flash this PR aims to eliminate. The previous logic `useState(!initialUser)` was correct.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| return { user: null, session: null }; | ||
| }); | ||
| const [loading, setLoading] = useState(true); |
There was a problem hiding this comment.
P1: Setting loading to true unconditionally defeats the server-side user hydration. When initialUser is provided, the user data is already available in sessionData, but consumers will still see loading: true and show loading indicators until the async fetchSession completes — reintroducing the exact loading flash this PR aims to eliminate. The previous logic useState(!initialUser) was correct.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/src/lib/auth-client.tsx, line 89:
<comment>Setting `loading` to `true` unconditionally defeats the server-side user hydration. When `initialUser` is provided, the user data is already available in `sessionData`, but consumers will still see `loading: true` and show loading indicators until the async `fetchSession` completes — reintroducing the exact loading flash this PR aims to eliminate. The previous logic `useState(!initialUser)` was correct.</comment>
<file context>
@@ -86,7 +86,7 @@ export function StableAuthProvider({
return { user: null, session: null };
});
- const [loading, setLoading] = useState(!initialUser);
+ const [loading, setLoading] = useState(true);
const fetchedRef = useRef(false);
const fetchingRef = useRef(false);
</file context>
| const [loading, setLoading] = useState(true); | |
| const [loading, setLoading] = useState(!initialUser); |
1 participant
Automated PR for AI review (Greptile/Cubic)
Summary by cubic
Aligns benchmark import with live OpenRouter models, improve model matching, and polish the model selector/info UI. Server-hydrated auth now avoids loading flashes with a timeout and stricter loading guard.
New Features
Bug Fixes
Written for commit 6d1f407. Summary will update on new commits.