Skip to content

fix(desktop): reject unsafe imported session IDs - #743

Open
imMamdouhaboammar wants to merge 1 commit into
lsdefine:mainfrom
imMamdouhaboammar:fix/desktop-session-id-path-traversal-upstream
Open

fix(desktop): reject unsafe imported session IDs#743
imMamdouhaboammar wants to merge 1 commit into
lsdefine:mainfrom
imMamdouhaboammar:fix/desktop-session-id-path-traversal-upstream

Conversation

@imMamdouhaboammar

Copy link
Copy Markdown

Problem

Desktop session persistence uses the session ID as the filename under temp/desktop_sessions/. Generated IDs are safe, but imported session JSON can supply a different ID.

Without validation, an imported ID can either resolve outside the session store or normalize to the same file as a different session ID. Examples include ../../escape, absolute paths, and aliases such as nested/../sess-safe123.

The alias case is an integrity bug even when the normalized target stays inside the store: duplicate detection compares the raw IDs, so the alias can be accepted as a second session and overwrite the first session's JSON file.

Fix

  • Reject empty/non-string IDs and IDs containing / or \
  • Resolve the canonical session-store root and target in _session_file() as a second containment check
  • Require the resolved target to remain a direct child of the session-store root
  • Route both temporary and final persistence paths through the validated target
  • Validate IDs when loading persisted items
  • Treat unsafe imported IDs as skipped records rather than inserting them into manager state

Generated session IDs and normal imported IDs keep the existing format and behavior.

Verification

TDD was run on a separate validation branch before the clean contribution commit:

  1. Initial regression-only run 31161662002 failed exactly on the ../../escape case while the valid-session case passed
  2. First containment fix passed run 31162056903, then persisted-state run 31162136988 passed
  3. A collision regression exposed nested/../sess-safe123: run 31162487893 failed only on that alias case
  4. The stricter separator guard passed run 31162585528
  5. Persisted-state run 31162679798 passed all 5 final security/compatibility cases
  6. After review feedback, the test harness was isolated from global sys.path and cross-test filesystem state; run 31164041346 passed compile, all regressions, and git diff --check

Final regression coverage includes traversal IDs, absolute IDs, normalized aliases, Windows-style path separators, and a normal safe session ID.

Scope

  • 1 production file modified
  • 1 regression-test file added
  • 1 clean commit on top of current main
  • no dependencies or configuration changes

A separate review observation about deterministic session temp-file writes racing under concurrency was verified as pre-existing in base main, not introduced by this patch. It is intentionally kept out of this imported-ID boundary fix and should be addressed independently.

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