Skip to content

fix(security): make personal path confinement CodeQL-visible#5713

Closed
RaresKeY wants to merge 1 commit into
odysseus-dev:devfrom
RaresKeY:fix/codeql-path-confinement
Closed

fix(security): make personal path confinement CodeQL-visible#5713
RaresKeY wants to merge 1 commit into
odysseus-dev:devfrom
RaresKeY:fix/codeql-path-confinement

Conversation

@RaresKeY

@RaresKeY RaresKeY commented Jul 23, 2026

Copy link
Copy Markdown
Member

Summary

Makes the existing personal-directory confinement visible to CodeQL without weakening its runtime boundary. The add and remove routes now canonicalize the requested path with realpath(), apply a route-local analyzer-recognized prefix guard, and then require either the exact personal-documents root or a separator-delimited descendant.

Keeping the guard in each request-to-sink scope is intentional: a local CodeQL reproduction showed that returning a checked path from the previous helper did not carry the safe-access fact to the filesystem and RAG sinks. Endpoint regressions cover both routes, accepted root/descendant forms, parent and sibling-prefix escapes, symlink escape, sink reachability, and unchanged resolved-path identity. No CodeQL query, severity, workflow, or SARIF filter is changed.

Target branch

  • This PR targets dev, not main. All PRs land in dev; main is curated by the maintainer at each release. If your PR is on main by accident, click "Edit" on this PR and change the base.

Linked Issue

Fixes #5712

Type of Change

  • Bug fix (non-breaking — fixes a confirmed issue)
  • New feature (non-breaking — adds new behaviour)
  • Breaking change (changes or removes existing behaviour)
  • Refactor / cleanup (behaviour unchanged)
  • Documentation only
  • CI / tooling / configuration

Checklist

  • I searched open issues and open PRs — this is not a duplicate.
  • This PR targets dev
  • My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
  • I actually ran the app (docker compose up or uvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.

Not run: a live app. This is a backend/static-analysis fix; the affected admin-only endpoints were exercised directly through the secretless test runner, including both accepted and rejected paths.

How to Test

  1. Run the focused personal-directory and adjacent RAG regressions:

    python -m pytest -q \
      tests/test_personal_dir_symlink_escape.py \
      tests/test_personal_remove_dir_confinement.py \
      tests/test_personal_delete_file_confinement.py \
      tests/test_personal_upload_isolation.py \
      tests/test_personal_upload_privilege.py \
      tests/test_rag_remove_directory_scope.py \
      tests/test_personal_index_hidden_dirs.py \
      tests/test_rag_index_hidden_dirs.py

    Expected: 34 passed.

  2. Compile the changed Python files and check the diff:

    python -m compileall -q \
      routes/personal_routes.py \
      tests/test_personal_dir_symlink_escape.py \
      tests/test_personal_remove_dir_confinement.py
    git diff --check origin/dev...HEAD
  3. Run the Python CodeQL py/path-injection query. The flow from DirectoryRequest.directory / the remove-directory query parameter to the personal route filesystem checks and src/rag_vector.py traversal should produce no alert. A local CodeQL 2.26.0 / python-queries 1.8.5 run against this exact head produced no result in either add/remove route and no result in src/rag_vector.py. The one remaining personal_routes.py result is an unrelated, pre-existing delete-file flow.

  4. Optional broader check:

    python -m pytest -q

    On the rebased head, the secretless review runner produced 4702 passed, 10 skipped, and 12 failures. Clean dev produced 4693 passed, 10 skipped, and 11 failures. The additional full-run failure is an upload-backup test that fails on both trees when its test module is run directly, confirming an order-sensitive baseline rather than a changed-file regression. The remaining failures cover the runner-excluded .env.example, an integration URL assumption, upload atomicity fixtures, and network-blocked web-fetch tests; none touches this change.

GitHub Actions CodeQL 2.26.1 passed on draft PR #5713, including the Python analysis and aggregate CodeQL check.

Visual / UI changes — REQUIRED if you touched anything that renders

N/A — backend route validation and tests only; no UI rendering changed.

  • Screenshot or short clip of the change in the running app, attached below. Mobile screenshot too if the change affects mobile.
  • Style match: the change uses Odysseus's existing visual language. Specifically:
    • Reuse existing CSS variables (--red, --fg, --bg, --card, --border, etc.) — do not introduce new color values, font sizes, or spacing units.
    • Reuse existing button/input/card/border classes. Don't invent parallel styling.
    • No Unicode emoji in UI or code. Use inline SVG (matching the monochrome icon style already in static/index.html) or plain text.
    • Monospaced font (Fira Code) for primary UI text. Don't override.
    • Dark theme is the default; any light-mode work must be wired through the existing theme system, not hard-coded.
  • No new component patterns. If a similar widget already exists in the app, extend it instead of writing a parallel one.
  • I am not an LLM agent submitting a bulk PR. If you are, please open an issue describing the problem first — bulk auto-generated PRs that don't match the project's visual style are closed on sight, even when the underlying fix is correct.

Screenshots / clips

N/A — no visual changes.

@github-actions github-actions Bot added the ready for review Description complete — ready for maintainer review label Jul 23, 2026
@RaresKeY RaresKeY closed this Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Description complete — ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CodeQL reopens personal-directory path-injection false positives after adjacent edits

1 participant