Skip to content

[FIX]: contain deserialized payload artifacts#106

Open
Hinotoi-agent wants to merge 5 commits into
microsoft:mainfrom
Hinotoi-agent:fix/artifact-load-containment
Open

[FIX]: contain deserialized payload artifacts#106
Hinotoi-agent wants to merge 5 commits into
microsoft:mainfrom
Hinotoi-agent:fix/artifact-load-containment

Conversation

@Hinotoi-agent

Copy link
Copy Markdown

Summary

This is the focused follow-up requested in #57 (comment).

It keeps the load-time artifact containment hardening only: when deserializing a persisted payload collection, artifact references now have to remain under the collection's artifacts/ directory after normalization and symlink resolution.

What changed

  • Adds _resolve_artifact_path() in rampart/payloads/_store.py for deserialized artifact references.
  • Rejects persisted artifact paths that are absolute, contain .., or do not start with artifacts/.
  • Resolves the final path and rejects symlink escapes outside the collection's artifacts/ directory.
  • Adds regression tests for traversal, absolute/non-artifacts paths, and symlink escape handling.

Scope notes

  • This does not add Payload.id validation.
  • This does not add rampart/core/payload_ids.py.
  • This does not change _copy_file_artifact() or OneDriveSurface.upload_async().
  • This is framed as defense-in-depth for untrusted/shared collection deserialization, not as a high-severity payload-ID vulnerability.

Test plan

Host-local:

uv run pytest tests/unit/payloads/test_payload_store_security.py -q
uv run pytest tests/unit/payloads/test_store.py tests/unit/payloads/test_payload_store_security.py -q
uv run ruff check rampart/payloads/_store.py tests/unit/payloads/test_payload_store_security.py
uv run ruff format --check rampart/payloads/_store.py tests/unit/payloads/test_payload_store_security.py
uv run ty check rampart/payloads/_store.py tests/unit/payloads/test_payload_store_security.py
uv run pytest -q
python -m compileall -q rampart tests
git diff --check

Results:

  • Focused new tests: 5 passed
  • Existing payload store + new tests: 18 passed
  • Full suite: 617 passed, 5 skipped
  • Ruff check/format: passed
  • Ty check: passed
  • Compileall/diff whitespace check: passed

Container validation:

docker run --rm -e CI=true -v "$PWD":/work -w /work python:3.12-slim bash -lc 'apt-get update -qq && apt-get install -y -qq git cargo >/dev/null && python -m pip install --quiet uv && uv run pytest tests/unit/payloads/test_store.py tests/unit/payloads/test_payload_store_security.py -q && uv run ruff check rampart/payloads/_store.py tests/unit/payloads/test_payload_store_security.py && uv run ty check rampart/payloads/_store.py tests/unit/payloads/test_payload_store_security.py && git diff --check'

Result: passed (18 passed; ruff, ty, and diff whitespace checks passed).

Comment thread rampart/payloads/_store.py
@Hinotoi-agent

Copy link
Copy Markdown
Author

Thanks — updated in 261a966 to use Path.relative_to("artifacts") for the lexical artifacts-prefix check.

I kept the explicit absolute/.. rejection and the resolved containment check afterward so the security behavior remains unchanged for traversal and symlink escapes.

Validation:

uv run pytest tests/unit/payloads/test_payload_store_security.py -q
# 5 passed
uv run pytest tests/unit/payloads/test_store.py tests/unit/payloads/test_payload_store_security.py -q
# 18 passed
uv run ruff check rampart/payloads/_store.py tests/unit/payloads/test_payload_store_security.py
uv run ruff format --check rampart/payloads/_store.py tests/unit/payloads/test_payload_store_security.py
uv run ty check rampart/payloads/_store.py tests/unit/payloads/test_payload_store_security.py
python -m compileall -q rampart tests
git diff --check

Comment thread rampart/payloads/_store.py Outdated
Comment thread rampart/payloads/_store.py Outdated
Comment thread tests/unit/payloads/test_payload_store_security.py
@nina-msft nina-msft changed the title fix: contain deserialized payload artifacts [FIX]: contain deserialized payload artifacts Jul 22, 2026
Copilot AI review requested due to automatic review settings July 22, 2026 00:07

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens payload collection load-time artifact handling by validating and containing deserialized artifact references so they cannot escape the collection’s artifacts/ directory after normalization and symlink resolution.

Changes:

  • Added PayloadStore._resolve_artifact_path() (and helper containment logic) to validate serialized artifact references during deserialization.
  • Updated deserialization to use the resolver and fail fast on invalid artifact paths before attempting to read.
  • Added regression tests covering traversal, absolute/non-artifacts/ paths, missing artifacts, and symlink escape scenarios.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
rampart/payloads/_store.py Adds artifact-path resolution/containment checks and routes deserialization through them.
tests/unit/payloads/test_payload_store_security.py Adds unit regression coverage for invalid serialized artifact paths and symlink escape handling.

Comment thread rampart/payloads/_store.py
Comment thread rampart/payloads/_store.py Outdated
Copilot AI review requested due to automatic review settings July 22, 2026 00:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comment thread rampart/payloads/_store.py
@bashirpartovi

Copy link
Copy Markdown
Contributor

@Hinotoi-agent would you please fix the build? the checks are failing and Copilot also has recommendations

Signed-off-by: hinotoi-agent <paperlantern.agent@gmail.com>
@Hinotoi-agent

Copy link
Copy Markdown
Author

Fixed the failing build and remaining Copilot recommendation in 107e8ce:

  • added the required Raises/Returns docstring sections for the new helpers, resolving all three Ruff failures;
  • normalized non-string deserialized artifact values (null, number, list, object) to the existing ValueError("Invalid artifact path ...") contract with exception chaining;
  • added regression coverage for each non-string JSON type.

Validation:

  • focused security tests: 12 passed
  • combined payload-store tests: 25 passed
  • full suite: 624 passed, 5 skipped
  • full-repository Ruff check/format, focused ty, compileall, and git diff --check: passed
  • Docker Python 3.12: 12 focused tests passed; full Ruff check/format passed
  • strict MkDocs build: passed

The prior docs job failed while setup-uv was fetching its version manifest, before dependency installation; the strict local docs build passes and this push retriggers that job.

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.

5 participants