Skip to content

[security] fix(payloads): validate store collection names#59

Open
Hinotoi-agent wants to merge 1 commit into
microsoft:mainfrom
Hinotoi-agent:fix/payload-store-name-validation
Open

[security] fix(payloads): validate store collection names#59
Hinotoi-agent wants to merge 1 commit into
microsoft:mainfrom
Hinotoi-agent:fix/payload-store-name-validation

Conversation

@Hinotoi-agent
Copy link
Copy Markdown

Summary

This PR hardens PayloadStore collection-name handling so all public collection lookup and removal APIs enforce the same trust boundary as save() and load().

Before this change, save() and load() rejected traversal-like collection names, but exists(), delete(), and manifest() used the caller-provided collection name directly in filesystem paths. A malformed collection name such as .. or ../outside could therefore resolve outside the configured payload store root for these management APIs.

Security issues covered

  • Payload store collection-name path traversal in collection management APIs.

Before this PR

  • save() and load() validated collection names.
  • exists(), delete(), and manifest() did not validate collection names before joining them with the store root.
  • delete("..") could attempt to remove a directory outside the payload store root.
  • manifest("..") could read a manifest.json outside the payload store root if present.

After this PR

  • exists(), delete(), and manifest() validate collection names before filesystem access.
  • Collection names are restricted to filename-safe identifiers using letters, numbers, dots, underscores, or hyphens.
  • Special dot entries (. and ..), path separators, control characters, and other unsafe filesystem characters are rejected consistently.

Why this matters

Payload collections are file-backed and may be managed from test helpers, fixtures, or higher-level tooling. Collection names should remain identifiers, not filesystem paths. Keeping every public collection API behind the same validation boundary prevents accidental or malicious traversal outside the configured payload cache root.

The highest-impact path was delete(): if a caller passed an unsafe collection name, the method could target a parent or sibling directory instead of a payload collection. Even when this raises on some platforms, reaching a destructive filesystem operation with an unvalidated traversal name is unsafe behavior.

Attack flow

  1. A caller obtains influence over a payload collection name used by store management code.
  2. The caller supplies a traversal-like name such as ...
  3. PayloadStore.delete() or PayloadStore.manifest() joins that value with the configured store root without validation.
  4. The resulting path resolves outside the intended payload store boundary.
  5. The operation can attempt deletion or manifest reads outside the payload collection directory.

Affected code

  • rampart/payloads/_store.py
    • PayloadStore.exists()
    • PayloadStore.delete()
    • PayloadStore.manifest()
    • _validate_collection_name()

Root cause

  • Collection-name validation was applied only to some public methods.
  • Management methods reused raw caller-provided names directly in filesystem paths.
  • The previous validator rejected path separators and dot entries but did not define a single positive allowlist for safe collection identifiers.

CVSS assessment

  • Issue: Payload store collection-name path traversal in management APIs
  • CVSS v3.1: 5.5 Medium
  • Vector: CVSS:3.1/AV:L/AC:L/PR:L/UI:N/S:U/C:L/I:L/A:L
  • Rationale: exploitation requires local/API-level ability to provide a collection name in the test process, but the affected operations can read or attempt to delete filesystem paths outside the configured payload store root.

Safe reproduction steps

On vulnerable code, a focused regression test demonstrates the unsafe boundary without touching real user files:

root = tmp_path / "store"
root.mkdir()
sentinel = tmp_path / "sentinel.txt"
sentinel.write_text("do not delete")
store = PayloadStore(root=root)

store.delete("..")

The new test expects this to raise ValueError before any destructive filesystem operation is reached.

A second regression covers metadata reads:

root = tmp_path / "store"
root.mkdir()
(tmp_path / "manifest.json").write_text('{"collection": "outside"}')
store = PayloadStore(root=root)

store.manifest("..")

The fixed behavior rejects the unsafe collection name instead of resolving a manifest outside the store root.

Expected vulnerable behavior

  • exists("../outside") does not reject the unsafe name.
  • delete("..") reaches shutil.rmtree() on a path outside the intended collection namespace.
  • manifest("..") can read ../manifest.json relative to the store root if that file exists.

Changes in this PR

  • Adds a positive collection-name allowlist: [A-Za-z0-9._-]{1,128}.
  • Rejects . and .. explicitly.
  • Applies collection-name validation to exists(), delete(), and manifest().
  • Adds regression tests for traversal rejection across read, existence, and delete management paths.

Files changed

  • rampart/payloads/_store.py
    • Adds the collection-name allowlist and validation calls in management APIs.
  • tests/unit/payloads/test_store.py
    • Adds regression coverage for traversal attempts against exists(), delete(), and manifest().

Maintainer impact

This is intentionally narrow. Existing collection names made from letters, numbers, dots, underscores, and hyphens continue to work. Names containing path separators or other filesystem-special characters now fail early with ValueError.

The patch does not change payload serialization format, artifact layout, or normal save/load behavior for valid collection names.

Fix rationale

A positive identifier allowlist is safer than trying to enumerate every platform-specific path edge case. It also keeps payload collection names portable across operating systems and consistent across all public PayloadStore APIs.

Applying validation at method entry preserves the intended abstraction: callers pass a collection identifier, and the store decides how that identifier maps to paths under its root.

Type of change

  • Security hardening
  • Bug fix
  • Tests
  • Documentation
  • Breaking change

Test plan

Focused regression tests:

uv run pytest tests/unit/payloads/test_store.py -q

Result:

16 passed

Full test suite:

uv run pytest -q

Result:

451 passed

Additional checks:

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

Result: all passed.

Disclosure notes

This PR is limited to collection-name validation in PayloadStore management APIs. It does not claim to address unrelated payload artifact path handling or external storage integrations.

@Hinotoi-agent Hinotoi-agent requested a review from a team May 21, 2026 02:39
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