[security] fix(payloads): validate store collection names#59
Open
Hinotoi-agent wants to merge 1 commit into
Open
[security] fix(payloads): validate store collection names#59Hinotoi-agent wants to merge 1 commit into
Hinotoi-agent wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR hardens
PayloadStorecollection-name handling so all public collection lookup and removal APIs enforce the same trust boundary assave()andload().Before this change,
save()andload()rejected traversal-like collection names, butexists(),delete(), andmanifest()used the caller-provided collection name directly in filesystem paths. A malformed collection name such as..or../outsidecould therefore resolve outside the configured payload store root for these management APIs.Security issues covered
Before this PR
save()andload()validated collection names.exists(),delete(), andmanifest()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 amanifest.jsonoutside the payload store root if present.After this PR
exists(),delete(), andmanifest()validate collection names before filesystem access..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
...PayloadStore.delete()orPayloadStore.manifest()joins that value with the configured store root without validation.Affected code
rampart/payloads/_store.pyPayloadStore.exists()PayloadStore.delete()PayloadStore.manifest()_validate_collection_name()Root cause
CVSS assessment
CVSS:3.1/AV:L/AC:L/PR:L/UI:N/S:U/C:L/I:L/A:LSafe reproduction steps
On vulnerable code, a focused regression test demonstrates the unsafe boundary without touching real user files:
The new test expects this to raise
ValueErrorbefore any destructive filesystem operation is reached.A second regression covers metadata reads:
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("..")reachesshutil.rmtree()on a path outside the intended collection namespace.manifest("..")can read../manifest.jsonrelative to the store root if that file exists.Changes in this PR
[A-Za-z0-9._-]{1,128}..and..explicitly.exists(),delete(), andmanifest().Files changed
rampart/payloads/_store.pytests/unit/payloads/test_store.pyexists(),delete(), andmanifest().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
PayloadStoreAPIs.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
Test plan
Focused regression tests:
Result:
Full test suite:
Result:
Additional checks:
Result: all passed.
Disclosure notes
This PR is limited to collection-name validation in
PayloadStoremanagement APIs. It does not claim to address unrelated payload artifact path handling or external storage integrations.