Security: unify package artifact-id normalization and reject path traversal - #486
Draft
lezama wants to merge 1 commit into
Draft
Security: unify package artifact-id normalization and reject path traversal#486lezama wants to merge 1 commit into
lezama wants to merge 1 commit into
Conversation
…versal
Package artifact-id / source-path normalization was duplicated across four
sites with three incompatible rule-sets, and the adoption orchestrator's copy
validated nothing at all -- an unauthenticated path-traversal seam (CWE-22 /
CWE-20) into the artifact_key it builds.
Divergent copies unified behind one shared normalizer
(WP_Agent_Package_Artifact_Identity):
- src/Packages/class-wp-agent-package-adoption-orchestrator.php artifact_id():
only trimmed and normalized backslashes -- NO leading-slash check, NO ".."
check. This built artifact_key, so it was the loose end (the no-op copy).
- src/Packages/class-wp-agent-package-update-planner.php normalize_artifact_id():
rejected leading "/" and str_contains("..") -- too strict, also rejected the
legitimate name "a..b".
- src/Packages/class-wp-agent-package-installed-artifact.php prepare_id():
rejected str_contains("..") -- same over-broad rule.
- src/Packages/class-wp-agent-package-artifact.php prepare_source():
rejected ".." only as a whole path segment (correct) plus a drive-letter
guard, so "a..b" passed here but was rejected by the planner.
The shared normalizer applies the strictest CORRECT rule uniformly: reject a
leading "/", reject any ".." as a whole traversal segment (so "a..b" stays a
valid name), normalize backslashes before the segment check, and keep the
drive-letter guard on sources. All four call sites now route through it.
Adds tests/package-artifact-id-normalization-smoke.php (registered in the
composer "smoke" array), which fails without this fix: it asserts traversal /
absolute ids (../x, /abs, a/../b, plus backslash variants) are rejected at
every seam, the a..b segment edge is accepted consistently, and legitimate ids
(foo, foo/bar, foo-bar_baz, memory/agent/SOUL.md) pass unchanged.
composer smoke green; phpstan analyse reports no errors.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Package artifact-id / source-path normalization was duplicated across four sites with three incompatible rule-sets, and the adoption orchestrator's copy validated nothing — a path-traversal seam (CWE-22 Path Traversal / CWE-20 Improper Input Validation) into the
artifact_keyit builds and into the installed-artifact snapshots it records.The four divergent copies:
src/Packages/class-wp-agent-package-adoption-orchestrator.php:204—artifact_id()only didtrim( str_replace( '\\', '/', … ) ): no leading-/check, no..check. Because this value flows intoartifact_key(the canonical index key) and intosnapshot_from_target(), it was the loose end — the no-op copy (CWE-22 / CWE-20).src/Packages/class-wp-agent-package-update-planner.php:176—normalize_artifact_id()rejected a leading/andstr_contains( '..' ). Correct on traversal, but over-broad: it also rejected the legitimate namea..b.src/Packages/class-wp-agent-package-installed-artifact.php:184—prepare_id()rejectedstr_contains( '..' ): same over-broad rule, and inconsistent with the source check below.src/Packages/class-wp-agent-package-artifact.php:241—prepare_source()rejected..only as a whole path segment (the correct rule) plus a drive-letter guard, soa..bpassed here but was rejected by the planner — the three rule-sets disagreed on the same input.Fix
Introduces one shared normalizer,
WP_Agent_Package_Artifact_Identity(src/Packages/class-wp-agent-package-artifact-identity.php), following the existingclass-wp-agent-package-*static-utility convention (private constructor,string_valuehelper), and routes all four call sites through it:normalize_id()— rejects empty, rejects a leading/(absolute), and rejects any..as a whole traversal segment; normalizes backslashes to/before the segment check so a Windows-style separator can't smuggle a..past it. ThrowsInvalidArgumentException— the same rejection mechanism the existing callers already used.normalize_source()— same traversal/absolute rule, additionally allows an empty value (no payload location), keeps the drive-letter guard (A:/…), and collapses empty segments.Net effect: the traversal/leading-slash rule is now uniform across the orchestrator, planner, installed-artifact, and source paths; the orchestrator's unvalidated
artifact_keypath is closed first; and..is correctly treated as dangerous only as a whole segment, so the legitimate namea..bis accepted everywhere instead of being rejected in two of four places.Testing
Adds
tests/package-artifact-id-normalization-smoke.php(registered in thecomposersmokearray). It fails without this fix (the orchestrator seam accepts../x,/abs,a/../b) and passes with it, asserting:../x,/abs,a/../b, plus backslash variants..\x,a\..\b, across the orchestrator (via reflection, since its copy is private), planner, installed-artifact, and both source normalizers.a..bsegment edge accepted consistently — previously rejected by planner + installed-artifact, now uniformly accepted.foo,foo/bar,foo-bar_baz, and the slashedmemory/agent/SOUL.md; underscores and case preserved.Gates:
composer smoke— all green (exit 0), including the existing package smokes.vendor/bin/phpstan analyse --no-progress --memory-limit=2G— No errors.This change came from an automated tech-debt / security audit of
Automattic/agents-api. Opening as a draft for review — please sanity-check the unified traversal rule (segment-based.., backslash-normalized) against any downstream consumers ofartifact_id/sourcebefore merge.