security: move session secrets to HttpOnly cookies#404
Open
Namraa310806 wants to merge 8 commits into
Open
Conversation
|
@Namraa310806 is attempting to deploy a commit to the firefistisdead's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR secures session credential handling by moving ChangesSession Secret Cookie-Based Authorization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Comment |
github-actions
Bot
added
backend
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/App.test.js (1)
48-70:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winTest helper
loadKnownSessionsis inconsistent with App.js implementation.The
loadKnownSessionshelper in this test file still filters out entries with missing or blanksession_secret(lines 60-61), but the App.js implementation at lines 119-132 no longer requiressession_secret. This causes downstream test failures:
- Test at lines 175-182 expects sessions with
nullsecrets to be retained, butloadKnownSessionsfilters them out.- Test at lines 199-213 expects 3 sessions, but the helper filters to only 1.
🐛 Proposed fix: Update the test helper to match App.js
function loadKnownSessions() { try { const raw = sessionStorage.getItem(SESSION_KEY); if (!raw) return []; const parsed = decodePayload(raw); if (!Array.isArray(parsed)) return []; return parsed .filter( (s) => s && typeof s.session_id === "string" && - s.session_id.trim() !== "" && - typeof s.session_secret === "string" && - s.session_secret.trim() !== "", + s.session_id.trim() !== "", ) .map((s) => ({ session_id: s.session_id.trim(), - session_secret: s.session_secret.trim(), + session_secret: + typeof s.session_secret === "string" && s.session_secret.trim() !== "" + ? s.session_secret.trim() + : null, })); } catch (_) { return []; } }🤖 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 `@frontend/src/App.test.js` around lines 48 - 70, The test helper loadKnownSessions is too strict compared to App.js: remove the checks that require a non-blank session_secret and instead only validate session_id (string and non-empty). Update the filter in loadKnownSessions to only ensure s && typeof s.session_id === "string" && s.session_id.trim() !== ""; in the map keep session_id as s.session_id.trim() and preserve session_secret as-is (e.g., s.session_secret or null) — if session_secret is a string you may trim it, otherwise allow null/undefined to match App.js behavior; keep SESSION_KEY and decodePayload usage and existing try/catch.
🤖 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.
Outside diff comments:
In `@frontend/src/App.test.js`:
- Around line 48-70: The test helper loadKnownSessions is too strict compared to
App.js: remove the checks that require a non-blank session_secret and instead
only validate session_id (string and non-empty). Update the filter in
loadKnownSessions to only ensure s && typeof s.session_id === "string" &&
s.session_id.trim() !== ""; in the map keep session_id as s.session_id.trim()
and preserve session_secret as-is (e.g., s.session_secret or null) — if
session_secret is a string you may trim it, otherwise allow null/undefined to
match App.js behavior; keep SESSION_KEY and decodePayload usage and existing
try/catch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd98ab7c-932b-4958-ac2d-fad12b6b2382
📒 Files selected for processing (5)
frontend/src/App.jsfrontend/src/App.test.jsfrontend/src/services/api.jsserver.jsserver.test.js
…kies (store token server-side)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@server.js`:
- Around line 504-505: SESSION_SECRET_MAP is an in-memory Map used to resolve
session cookie tokens (with SESSION_SECRET_TTL_MS controlling TTL), which causes
sessions to break across process restarts or different instances; replace the
Map with a shared backing store (e.g., Redis) and persist token -> { encrypted,
expiry } with an expiry set from SESSION_SECRET_TTL_MS, update all reads/writes
that reference SESSION_SECRET_MAP to async Redis GET/SET/EXPIRE operations (or
use a stateless signed cookie approach instead: embed and verify the
session_secret in the cookie), keep an in-memory fallback only for
single-process dev, and add robust error handling so lookups fall back
gracefully when the shared store is unavailable.
- Around line 566-568: The per-token eviction timer created in the
SESSION_SECRET_MAP cleanup uses setTimeout(...) which can keep the Node process
alive; update the timer logic around SESSION_SECRET_MAP.delete(token) to store
the return value from setTimeout, call .unref() on that timer when available,
and then let it fire normally after SESSION_SECRET_TTL_MS + 1000 so the timer no
longer holds the event loop open; reference the token variable, the
SESSION_SECRET_MAP deletion, and SESSION_SECRET_TTL_MS when applying this
change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 08a46a1d-4bc6-4910-a552-ded2aa11a54a
📒 Files selected for processing (2)
frontend/src/App.jsserver.js
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/App.js
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
frontend/src/App.js:154
upsertKnownSessionis still writing session data tolocalStorage, which contradicts the updated comment/migration and reintroduces the exact storage behavior the tests are guarding against. This should write tosessionStorage(and keep the same try/catch), otherwise the new migration will remove the legacy key but subsequent upserts will immediately recreate it.
{ session_id: normalizedSessionId, session_secret: normalizedSessionSecret },
...existing.filter((s) => s.session_id !== normalizedSessionId),
];
try {
localStorage.setItem(SESSION_STORAGE_KEY, encodePayload(next.slice(0, 50)));
} catch (_) {
Comment on lines
+795
to
+801
| res.cookie(getSessionSecretCookieName(sessionId), token, { | ||
| httpOnly: true, | ||
| sameSite: "lax", | ||
| secure: process.env.NODE_ENV === "production", | ||
| path: "/", | ||
| maxAge: SESSION_SECRET_TTL_MS, | ||
| }); |
Comment on lines
+702
to
+708
| SESSION_SECRET_MEMORY_MAP.set(token, { encrypted, expiry }); | ||
| const timeout = setTimeout(() => { | ||
| SESSION_SECRET_MEMORY_MAP.delete(token); | ||
| }, SESSION_SECRET_TTL_MS + 1000); | ||
| if (typeof timeout.unref === "function") { | ||
| timeout.unref(); | ||
| } |
Comment on lines
+593
to
+598
| // Encryption key must be provided via env var as base64-encoded 32 bytes. | ||
| // If not present, generate a runtime-only key (lost on restart) and log a warning. | ||
| let ENC_KEY = null; | ||
| const _initEncKey = () => { | ||
| if (ENC_KEY) return; | ||
| const fromEnv = (process.env.SESSION_SECRET_ENC_KEY || "").trim(); |
Comment on lines
+752
to
+772
| return header.split(";").reduce((cookies, pair) => { | ||
| const separatorIndex = pair.indexOf("="); | ||
| if (separatorIndex === -1) { | ||
| return cookies; | ||
| } | ||
|
|
||
| const rawName = pair.slice(0, separatorIndex).trim(); | ||
| const rawValue = pair.slice(separatorIndex + 1).trim(); | ||
|
|
||
| if (!rawName) { | ||
| return cookies; | ||
| } | ||
|
|
||
| try { | ||
| cookies[rawName] = decodeURIComponent(rawValue); | ||
| } catch (_) { | ||
| cookies[rawName] = rawValue; | ||
| } | ||
|
|
||
| return cookies; | ||
| }, {}); |
Comment on lines
+710
to
+725
| if (sessionSecretRedisClient) { | ||
| await _storeSessionSecretInRedis(token, encrypted, expiry); | ||
| } | ||
|
|
||
| SESSION_SECRET_MEMORY_MAP.set(token, { encrypted, expiry }); | ||
| // Only create a per-token timer for in-memory fallback. When Redis is | ||
| // configured we rely on Redis TTLs and lazy eviction to avoid creating | ||
| // large numbers of active timers in-process. | ||
| if (!sessionSecretRedisClient) { | ||
| const timeout = setTimeout(() => { | ||
| SESSION_SECRET_MEMORY_MAP.delete(token); | ||
| }, SESSION_SECRET_TTL_MS + 1000); | ||
| if (typeof timeout.unref === "function") { | ||
| timeout.unref(); | ||
| } | ||
| } |
Comment on lines
+815
to
+823
| // Respect operator-configured SameSite. If operators explicitly request | ||
| // 'none', ensure the Secure flag is set (browsers require this for None). | ||
| const sameSiteRaw = (SESSION_SECRET_COOKIE_SAMESITE || "lax").toString(); | ||
| const sameSite = ("" + sameSiteRaw).toLowerCase(); | ||
| const secureFlag = process.env.NODE_ENV === "production" || sameSite === "none"; | ||
|
|
||
| if (sameSite === "none" && !secureFlag) { | ||
| console.warn("SESSION_SECRET_COOKIE_SAMESITE=none was requested but secure cookies are not enabled; forcing Secure flag to true for compatibility."); | ||
| } |
Comment on lines
+149
to
153
| { session_id: normalizedSessionId, session_secret: normalizedSessionSecret }, | ||
| ...existing.filter((s) => s.session_id !== normalizedSessionId), | ||
| ]; | ||
| try { | ||
| localStorage.setItem(SESSION_STORAGE_KEY, encodePayload(next.slice(0, 50))); |
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR reduces exposure of session credentials by removing
session_secretfrom API responses and moving secret handling to secure HttpOnly cookies.Previously, session secrets were returned in upload/session bootstrap responses and could be accessed by browser JavaScript, client-side logs, analytics tooling, browser extensions, or other frontend integrations.
This change keeps session authentication functional while significantly reducing the amount of secret material exposed to the browser environment.
Changes Made
Added secure session secret cookie handling on the backend.
Introduced helper utilities for:
Stored session secrets in HttpOnly cookies instead of exposing them through API responses.
Updated upload and session-extension flows to use cookie-based credential handling.
Removed reliance on API responses returning
session_secret.Updated frontend API clients to send credentials using:
axios.defaults.withCredentials = truecredentials: "include"for streaming requestsMoved session tracking to
sessionStorageand reduced browser persistence of sensitive data.Added migration support for legacy session storage.
Added regression tests verifying:
session_secretis not returned in upload responsesRelated issue
Fixes: #359
Testing
Verified Scenarios
session_secret.Checklist:
Screenshots / recordings
N/A – Backend and security-focused change.
Notes
This change improves credential handling by reducing exposure of bearer-style session secrets to browser-accessible contexts.
The implementation preserves existing application behavior while moving sensitive session credentials into HttpOnly cookies that are inaccessible to frontend JavaScript.
No database migrations or infrastructure changes are required.
Security
Summary by CodeRabbit
New Features
Bug Fixes