Scope form sessions to form_id to avoid conflicts

[danielnaab]

No description provided.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 3b35842

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[claude]

PR Review: Scope form sessions to form_id to avoid conflicts

Summary

This PR fixes a critical bug where form sessions were being shared across different forms, causing them to "clobber" each other. The fix scopes the session cookie to include the formId, changing from form_session_id to form_session_id_${formId}.

✅ Strengths

• Critical bug fix: Correctly identifies and fixes a session collision issue
• Minimal, focused change: Only 3 lines changed, reducing risk
• Consistent implementation: All three cookie operations (get, set, delete) are updated consistently

⚠️ Issues Found

1. Critical: Breaking change in localStorage (apps/spotlight)

The client-side code in apps/spotlight still uses the old non-scoped session key:

Location: apps/spotlight/src/features/form-page/store/actions/initialize.ts:12

const sessionId = window.localStorage.getItem('form_session_id') || undefined;

Location: apps/spotlight/src/features/form-page/store/actions/on-submit-form.ts:40

window.localStorage.setItem('form_session_id', submission.data.sessionId);

Impact: This creates a mismatch between server-side cookies (now scoped) and client-side localStorage (still global), which will cause session persistence to break across page reloads when users work with multiple forms.

Fix Required:

// initialize.ts
const sessionId = window.localStorage.getItem(`form_session_id_${opts.formId}`) || undefined;

// on-submit-form.ts  
window.localStorage.setItem(`form_session_id_${opts.formId}`, submission.data.sessionId);

2. Test Coverage Gap

The existing test file (packages/server/src/pages/forms/[id].test.ts:238) hardcodes the old cookie name:

headers: sessionId ? { Cookie: `form_session_id=${sessionId}` } : {},

Recommendation: Update this test to use the new scoped cookie name format to ensure tests accurately reflect production behavior:

headers: sessionId ? { Cookie: `form_session_id_${formId}=${sessionId}` } : {},

3. Missing E2E Test Coverage

There's no automated test that validates users can work with multiple forms simultaneously without session interference.

Recommendation: Add an E2E test that:

1. Opens form A and fills some fields
2. Opens form B in a new tab/context and fills fields
3. Returns to form A and verifies the session data is intact
4. Returns to form B and verifies its session data is intact

This would have caught this bug earlier and prevent regressions.

4. Potential Cookie Storage Concerns

Question: What happens to orphaned cookies when forms are deleted? Each form now creates its own cookie, which could accumulate over time.

Recommendation: Consider implementing:

• Cookie cleanup on form completion/deletion
• Max-age/expiration policies for form session cookies
• Documentation of cookie lifecycle

5. Security: Cookie Name Predictability

The cookie name includes the formId directly, which is somewhat predictable. While not necessarily a vulnerability, consider whether this exposes any information.

Low priority, but worth considering if formId values should be less predictable or if cookie attributes (httpOnly, secure, sameSite) need review.

📋 Action Items

Must fix before merge:

• Update apps/spotlight/src/features/form-page/store/actions/initialize.ts:12 to use scoped localStorage key
• Update apps/spotlight/src/features/form-page/store/actions/on-submit-form.ts:40 to use scoped localStorage key
• Update test at packages/server/src/pages/forms/[id].test.ts:238 to use new cookie format

Recommended for follow-up:

• Add E2E test for multi-form session isolation
• Document cookie lifecycle and cleanup strategy
• Review cookie security attributes (httpOnly, secure, sameSite)

Overall Assessment

Do not merge until the localStorage mismatch is fixed. The core fix is correct and important, but the incomplete migration will cause session persistence bugs in the spotlight app.

Once the localStorage references are updated and tests pass, this will be a solid bug fix. Great catch on the session collision issue!