Skip to content

fix: reject path separators in artifact identifier segments - #7068

Open
artemkulyk wants to merge 2 commits into
google:mainfrom
artemkulyk:fix/artifact-user-id-path-separators
Open

fix: reject path separators in artifact identifier segments#7068
artemkulyk wants to merge 2 commits into
google:mainfrom
artemkulyk:fix/artifact-user-id-path-separators

Conversation

@artemkulyk

Copy link
Copy Markdown

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

1. Link to an existing issue (if applicable):

Problem:
FileArtifactService joins user_id into root/apps/{app}/users/{user_id}/.... validate_path_segment allowed / and \, so user_id="u1/sessions/s2" composed onto victim user u1's session s2 artifacts (read/overwrite/delete). Nested filenames remain valid; this check is only for app_name / user_id / session_id.

Solution:
Reject embedded path separators in validate_path_segment (after the existing traversal/absolute/drive checks), matching evaluation/_path_validation.py. Slash-containing identifiers such as group/user123 now raise InputValidationError.

Testing Plan

Unit Tests:

  • I have added or updated unit tests for my change.
  • All unit tests pass locally.
.venv/bin/pytest tests/unittests/artifacts/test_artifact_util.py tests/unittests/artifacts/test_artifact_service.py -q
861 passed

Inverted the former namespaced-id success test. Added a FileArtifactService case: victim u1/s2/notes stays intact when attacker user_id="u1/sessions/s2" is rejected.

Manual End-to-End (E2E) Tests:

hunt_tests/phase4_f3_poc.py now raises user_id 'u1/sessions/s2' must not contain path separators. Cross-user without / still misses; ../u2 still reports traversal.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.
  • I have manually tested my changes end-to-end.
  • Any dependent changes have been merged and published in downstream modules.

Additional context

Google VRP 557708759 was closed as Infeasible with a request to file this publicly. I have signed the Google CLA. Commit author is artemkulyk artem.kulyk@gmail.com only (no co-authors).

Made with Cursor

FileArtifactService joins user_id into the on-disk tree. A value such as
u1/sessions/s2 composed onto another user's session artifacts. Reject / and
\ in validate_path_segment so identifiers stay a single path segment.
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.

FileArtifactService user_id with '/' composes onto another user's session artifacts

2 participants