diff --git a/spec/functional/FR-004-source-resolution.md b/spec/functional/FR-004-source-resolution.md index cd4c6ef..aaa90f8 100644 --- a/spec/functional/FR-004-source-resolution.md +++ b/spec/functional/FR-004-source-resolution.md @@ -97,14 +97,37 @@ offline (no registry), which is how the default fetcher is integration-tested (FR-004-AC-10). An injected `NpmFetcher` replaces the default so tests run with no real `npm`/network (FR-004-AC-8, -AC-9). +**Argument-injection guard.** Source-descriptor fields flow into the `git` argv: +`repo`/`url` reach `git clone`/`git fetch` (the clone URL), `git-subdir.path` +reaches `git sparse-checkout set `, and `sha`/`ref` reach +`git checkout --detach `. `execFileSync` avoids a _shell_, but `git` +itself treats a leading-`-` argument as an **option** — e.g. a `ref` of +`--upload-pack=`, a `repo` of `--output=…`, or a `path` of `--stdin` becomes +a flag (a second-order command-line injection, +`js/second-order-command-line-injection`). Because `resolveSource` calls +`normalizeSource` **before** any `git` invocation, the guard that +`normalizeSource` ([FR-001](./FR-001-typed-source-union.md)) applies to these +fields (reject a value whose **trimmed** form begins with `-`) is sufficient: +`github.repo`, `git.url`, `git-subdir.url`, `git-subdir.path`, and `url.url`, plus +the optional `ref`/`sha` on every git variant, are rejected with `SourceError` +before they can reach the argv (FR-004-CON-5). The guard is on the **trimmed** +value because `toGitUrl` does `raw.trim()` before the URL reaches the argv: a raw +`startsWith("-")` check would let a leading-whitespace-then-dash payload (e.g. +`" --upload-pack=… ext://x"`, which `toGitUrl` also passes through unwrapped on +its `://`) slip past the guard yet trim to an option at the argv. Guarding the +trimmed value makes the value that is validated the value that actually reaches +the argv. Rejection is preferred over an argv `--` separator because +`git checkout` does not accept `--` cleanly before a ref. + ## Constraints -| ID | Constraint | Type | Validation | -| ------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | ------------- | -| FR-004-CON-1 | The only side effect is the per-source package-manager subprocess (`git` for git sources; `npm pack` + `tar` for npm sources); resolution performs no other network or I/O beyond filesystem reads/dir creation. | architectural | Test (TC-011) | -| FR-004-CON-2 | The clone is blobless and no-checkout (`--filter=blob:none --no-checkout`); subdir sources sparse-checkout only the requested path. | performance | Test (TC-008) | -| FR-004-CON-3 | `normalizeSource` rejects an `npm` `package` beginning with `-`, so it cannot reach `npm pack` as a CLI flag (second-order command-line-injection guard). | security | Test (TC-026) | -| FR-004-CON-4 | When `npm pack --json` output contains no metadata array (no array at all, an empty array, or an array whose first element is not an object with a string `filename`), `parseNpmPackJson` throws a descriptive `SourceError` rather than returning garbage or a confusing downstream failure. | robustness | Test (TC-027) | +| ID | Constraint | Type | Validation | +| ------------ | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------- | ----------------------------- | +| FR-004-CON-1 | The only side effect is the per-source package-manager subprocess (`git` for git sources; `npm pack` + `tar` for npm sources); resolution performs no other network or I/O beyond filesystem reads/dir creation. | architectural | Test (TC-011) | +| FR-004-CON-2 | The clone is blobless and no-checkout (`--filter=blob:none --no-checkout`); subdir sources sparse-checkout only the requested path. | performance | Test (TC-008) | +| FR-004-CON-3 | `normalizeSource` rejects an `npm` `package` beginning with `-`, so it cannot reach `npm pack` as a CLI flag (second-order command-line-injection guard). | security | Test (TC-026) | +| FR-004-CON-4 | When `npm pack --json` output contains no metadata array (no array at all, an empty array, or an array whose first element is not an object with a string `filename`), `parseNpmPackJson` throws a descriptive `SourceError` rather than returning garbage or a confusing downstream failure. | robustness | Test (TC-027) | +| FR-004-CON-5 | `normalizeSource` rejects a value whose **trimmed** form is option-like (begins with `-`) for `github.repo`, `git.url`, `git-subdir.url`, `git-subdir.path`, `url.url`, or any git-variant `ref`/`sha`, so it cannot reach the `git` argv as a CLI flag — including a leading-whitespace-then-dash payload that `toGitUrl` would otherwise trim to an option (second-order command-line-injection guard). | security | Test (TC-029, TC-030, TC-031) | ## Acceptance Criteria @@ -123,6 +146,9 @@ real `npm`/network (FR-004-AC-8, -AC-9). | FR-004-AC-11 | `npmPackArgs` builds the correct argv for a pinned spec (`@`), an unpinned spec (``), and includes `--registry ` when a registry is supplied. | Test (TC-025) | | FR-004-AC-12 | `parseNpmPackJson` robustly parses the `npm pack --json` metadata: it scans past lifecycle-script stdout noise (including stray `[` brackets, empty arrays, and non-metadata noise arrays) and returns the first array whose first element is an npm-pack metadata object (an object with a string `filename`); on no-array/empty/invalid output it throws a descriptive `SourceError`. | Test (TC-027) | | FR-004-AC-13 | An **unpinned** (or range) `npm` re-fetch clears any prior cached tarball + extraction before re-extracting, so the npm cache dir does not accumulate stale tarball artifacts across "latest" re-fetches. | Test (TC-028) | +| FR-004-AC-14 | An option-like (leading `-`) `repo`, `url`, `ref`, or `sha` on a git source throws `SourceError` ("must not begin with `-`") from `normalizeSource`, before any `git` invocation. | Test (TC-029) | +| FR-004-AC-15 | A leading-whitespace-then-dash `repo`/`url` (e.g. `" --upload-pack=… ext://x"`), which `toGitUrl` would trim to an option at the argv, throws `SourceError` from `normalizeSource` — the guard validates the trimmed value. | Test (TC-030) | +| FR-004-AC-16 | An option-like (leading `-`) `git-subdir.path` (e.g. `"--stdin"`, `"-X"`), which would reach `git sparse-checkout set ` as a flag, throws `SourceError` from `normalizeSource`. | Test (TC-031) | ## Dependencies diff --git a/spec/tests.md b/spec/tests.md index 31d43e5..41d1ec2 100644 --- a/spec/tests.md +++ b/spec/tests.md @@ -79,52 +79,55 @@ partial clones work offline. ## Functional Requirement Coverage -| Functional Req | Acceptance Criteria | Test Case · Case String | Coverage Status | -| -------------- | ----------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------ | -| FR-001 | AC-1: all six valid source shapes accepted | TC-001 — `normalizeSource › "accepts every valid shape"` | ✅ Unit | -| FR-001 | AC-2: null / no-`type` → SourceError | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | -| FR-001 | AC-3: missing required field → SourceError naming the field | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | -| FR-001 | AC-4: unknown type → SourceError "unknown source type" | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | -| FR-002 | AC-1: `owner/repo` → GitHub https URL | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | -| FR-002 | AC-2: `owner/repo.git` strips trailing `.git` | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | -| FR-002 | AC-3: full `https://` URL passes through | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | -| FR-002 | AC-4: `git@…` scp-style URL passes through | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | -| FR-002 | AC-5: surrounding whitespace is trimmed | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` (padded-input assertion) | ✅ Unit | -| FR-003 | AC-1: valid manifest with name round-trips | TC-004 — `validateMarketplaceManifest › "accepts a valid manifest (with and without a name)"` | ✅ Unit | -| FR-003 | AC-2: missing name → `name === undefined` | TC-004 — `validateMarketplaceManifest › "accepts a valid manifest (with and without a name)"` | ✅ Unit | -| FR-003 | AC-3: null / non-object manifest → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | -| FR-003 | AC-4: bad schemaVersion / non-array entries → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | -| FR-003 | AC-5: null entry / missing entry name → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | -| FR-003 | AC-6: invalid entry source → SourceError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | -| FR-004 | AC-1: path source returns the dir | TC-006 — `resolveSource › "path source returns the dir; missing path throws"` | ✅ Unit | -| FR-004 | AC-2: missing path → SourceError | TC-006 — `resolveSource › "path source returns the dir; missing path throws"` | ✅ Unit | -| FR-004 | AC-3: url → UnsupportedSourceError | TC-007 — `resolveSource › "url sources are not yet supported"` | ✅ Unit | -| FR-004 | AC-4: git-subdir sparse-checkout at tag → dir/sha/ref | TC-008 — `resolveSource › "git-subdir sparse-checks out only the subdir at a tag"` | ✅ Unit (git) | -| FR-004 | AC-5: whole-repo HEAD when unpinned; re-fetch existing cache | TC-009 — `resolveSource › "whole-repo git resolves to HEAD when unpinned, and re-fetches an existing cache"` | ✅ Unit (git) | -| FR-004 | AC-6: sha pin checks out the exact commit | TC-010 — `resolveSource › "sha pin checks out the exact commit"` | ✅ Unit (git) | -| FR-004 | AC-7: github + injected runner needs no real git | TC-011 — `resolveSource › "github source + injected runner needs no real git"` | ✅ Unit (fake) | -| FR-004 | CON-1: git is the sole side effect | TC-011 — `resolveSource › "github source + injected runner needs no real git"` | ✅ Unit (fake) | -| FR-004 | CON-2: blobless + sparse (subdir only) | TC-008 — `resolveSource › "git-subdir sparse-checks out only the subdir at a tag"` | ✅ Unit (git) | -| FR-004 | AC-8: npm resolves+extracts+pins resolved version (fake fetcher) | TC-022 — `resolveSource › "npm source downloads, extracts, and pins the resolved version"` + `"… resolves via the default fetcher when none is injected"` | ✅ Unit (fake/npm) | -| FR-004 | AC-9: exact-version pin cached; unpinned re-fetches | TC-023 — `resolveSource › "exact-version npm pins are cached; unpinned specs re-fetch"` | ✅ Unit (fake) | -| FR-004 | AC-10: defaultNpmFetcher local-pack offline (npm pack + tar) | TC-024 — `resolveSource › "defaultNpmFetcher packs and extracts a local package offline"` | ✅ Unit (npm) | -| FR-004 | AC-11: npmPackArgs builds pinned/unpinned + registry argv | TC-025 — `resolveSource › "npmPackArgs builds pinned, unpinned, and registry argv"` | ✅ Unit | -| FR-004 | CON-3: rejects option-like npm package (injection guard) | TC-026 — `normalizeSource › "rejects malformed input"` (option-like `-x` package assertion) | ✅ Unit | -| FR-004 | AC-12: robust npm-pack-json parse (skip noise; metadata object) | TC-027 — `resolveSource › "parseNpmPackJson skips prepack noise and parses the trailing array"` (+ scans-past-empty / scans-past-numeric tests) | ✅ Unit | -| FR-004 | CON-4: descriptive SourceError on no-array/empty/invalid output | TC-027 — `resolveSource › "parseNpmPackJson throws SourceError on no-bracket output"` (+ empty-array / numeric / no-string-filename throw tests) | ✅ Unit | -| FR-004 | AC-13: unpinned re-fetch clears stale tarballs (cache hygiene) | TC-028 — `resolveSource › "unpinned re-fetch does not accumulate stale tarballs"` | ✅ Unit (fake) | -| FR-005 | AC-1: missing / shape-invalid (`{}`) registry read as empty | TC-012 — `registry › "missing and malformed files read as empty"` | ✅ Unit | -| FR-005 | AC-2: atomic write + nested-dir creation round-trips | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | -| FR-005 | AC-3: upsert replaces by name (count stays 1) | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | -| FR-006 | AC-1: named git-subdir entry materializes + records | TC-014 — `installEntry › "materializes a named git-subdir entry and records it"` | ✅ Unit (git) | -| FR-006 | AC-2: name derived via readName when absent | TC-015 — `installEntry › "derives the name via readName when the entry has none"` | ✅ Unit (git) | -| FR-006 | AC-3: entry.path against a whole-repo source | TC-016 — `installEntry › "honors entry.path against a whole-repo source"` | ✅ Unit (git) | -| FR-006 | AC-4: symlink mode; re-install replaces | TC-017 — `installEntry › "symlink mode links instead of copying, and re-install replaces"` | ✅ Unit (git) | -| FR-007 | AC-1: lazy installs enabled, skips disabled | TC-018 — `reconcile › "lazy installs the enabled set, skips disabled, and is idempotent with zero git on the 2nd run"` | ✅ Unit (git) | -| FR-007 | AC-2: 2nd lazy reconcile → unchanged, zero git | TC-018 — `reconcile › "lazy installs the enabled set, skips disabled, and is idempotent with zero git on the 2nd run"` | ✅ Unit (git) | -| FR-007 | AC-3: sync unchanged on stable ref; updated on moved pin | TC-019 — `reconcile › "sync re-resolves: unchanged on a stable ref, updated on a moved pin"` | ✅ Unit (git) | -| FR-007 | AC-4: lazy re-materializes when target dir is gone | TC-020 — `reconcile › "lazy re-materializes when the target dir is gone"` | ✅ Unit (git) | -| FR-007 | AC-5: lazy sha pin → unchanged when matches, updated when differs | TC-021 — `reconcile › "lazy honors a sha pin: unchanged when it matches, updated when it differs"` | ✅ Unit (git) | +| Functional Req | Acceptance Criteria | Test Case · Case String | Coverage Status | +| -------------- | -------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------ | +| FR-001 | AC-1: all six valid source shapes accepted | TC-001 — `normalizeSource › "accepts every valid shape"` | ✅ Unit | +| FR-001 | AC-2: null / no-`type` → SourceError | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | +| FR-001 | AC-3: missing required field → SourceError naming the field | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | +| FR-001 | AC-4: unknown type → SourceError "unknown source type" | TC-002 — `normalizeSource › "rejects malformed input"` | ✅ Unit | +| FR-002 | AC-1: `owner/repo` → GitHub https URL | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | +| FR-002 | AC-2: `owner/repo.git` strips trailing `.git` | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | +| FR-002 | AC-3: full `https://` URL passes through | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | +| FR-002 | AC-4: `git@…` scp-style URL passes through | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` | ✅ Unit | +| FR-002 | AC-5: surrounding whitespace is trimmed | TC-003 — `"toGitUrl expands shorthand and passes through URLs"` (padded-input assertion) | ✅ Unit | +| FR-003 | AC-1: valid manifest with name round-trips | TC-004 — `validateMarketplaceManifest › "accepts a valid manifest (with and without a name)"` | ✅ Unit | +| FR-003 | AC-2: missing name → `name === undefined` | TC-004 — `validateMarketplaceManifest › "accepts a valid manifest (with and without a name)"` | ✅ Unit | +| FR-003 | AC-3: null / non-object manifest → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | +| FR-003 | AC-4: bad schemaVersion / non-array entries → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | +| FR-003 | AC-5: null entry / missing entry name → ManifestError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | +| FR-003 | AC-6: invalid entry source → SourceError | TC-005 — `validateMarketplaceManifest › "rejects malformed manifests and entries"` | ✅ Unit | +| FR-004 | AC-1: path source returns the dir | TC-006 — `resolveSource › "path source returns the dir; missing path throws"` | ✅ Unit | +| FR-004 | AC-2: missing path → SourceError | TC-006 — `resolveSource › "path source returns the dir; missing path throws"` | ✅ Unit | +| FR-004 | AC-3: url → UnsupportedSourceError | TC-007 — `resolveSource › "url sources are not yet supported"` | ✅ Unit | +| FR-004 | AC-4: git-subdir sparse-checkout at tag → dir/sha/ref | TC-008 — `resolveSource › "git-subdir sparse-checks out only the subdir at a tag"` | ✅ Unit (git) | +| FR-004 | AC-5: whole-repo HEAD when unpinned; re-fetch existing cache | TC-009 — `resolveSource › "whole-repo git resolves to HEAD when unpinned, and re-fetches an existing cache"` | ✅ Unit (git) | +| FR-004 | AC-6: sha pin checks out the exact commit | TC-010 — `resolveSource › "sha pin checks out the exact commit"` | ✅ Unit (git) | +| FR-004 | AC-7: github + injected runner needs no real git | TC-011 — `resolveSource › "github source + injected runner needs no real git"` | ✅ Unit (fake) | +| FR-004 | CON-1: git is the sole side effect | TC-011 — `resolveSource › "github source + injected runner needs no real git"` | ✅ Unit (fake) | +| FR-004 | CON-2: blobless + sparse (subdir only) | TC-008 — `resolveSource › "git-subdir sparse-checks out only the subdir at a tag"` | ✅ Unit (git) | +| FR-004 | AC-8: npm resolves+extracts+pins resolved version (fake fetcher) | TC-022 — `resolveSource › "npm source downloads, extracts, and pins the resolved version"` + `"… resolves via the default fetcher when none is injected"` | ✅ Unit (fake/npm) | +| FR-004 | AC-9: exact-version pin cached; unpinned re-fetches | TC-023 — `resolveSource › "exact-version npm pins are cached; unpinned specs re-fetch"` | ✅ Unit (fake) | +| FR-004 | AC-10: defaultNpmFetcher local-pack offline (npm pack + tar) | TC-024 — `resolveSource › "defaultNpmFetcher packs and extracts a local package offline"` | ✅ Unit (npm) | +| FR-004 | AC-11: npmPackArgs builds pinned/unpinned + registry argv | TC-025 — `resolveSource › "npmPackArgs builds pinned, unpinned, and registry argv"` | ✅ Unit | +| FR-004 | CON-3: rejects option-like npm package (injection guard) | TC-026 — `normalizeSource › "rejects malformed input"` (option-like `-x` package assertion) | ✅ Unit | +| FR-004 | AC-12: robust npm-pack-json parse (skip noise; metadata object) | TC-027 — `resolveSource › "parseNpmPackJson skips prepack noise and parses the trailing array"` (+ scans-past-empty / scans-past-numeric tests) | ✅ Unit | +| FR-004 | CON-4: descriptive SourceError on no-array/empty/invalid output | TC-027 — `resolveSource › "parseNpmPackJson throws SourceError on no-bracket output"` (+ empty-array / numeric / no-string-filename throw tests) | ✅ Unit | +| FR-004 | AC-13: unpinned re-fetch clears stale tarballs (cache hygiene) | TC-028 — `resolveSource › "unpinned re-fetch does not accumulate stale tarballs"` | ✅ Unit (fake) | +| FR-004 | AC-14 / CON-5: option-like git argv field rejected (injection guard) | TC-029 — `normalizeSource › "rejects option-like git argv fields (injection guard)"` | ✅ Unit | +| FR-004 | AC-15 / CON-5: leading-whitespace trim-bypass repo/url rejected | TC-030 — `normalizeSource › "rejects leading-whitespace option-like repo/url (trim bypass)"` | ✅ Unit | +| FR-004 | AC-16 / CON-5: option-like `git-subdir.path` rejected | TC-031 — `normalizeSource › "rejects option-like git-subdir path"` | ✅ Unit | +| FR-005 | AC-1: missing / shape-invalid (`{}`) registry read as empty | TC-012 — `registry › "missing and malformed files read as empty"` | ✅ Unit | +| FR-005 | AC-2: atomic write + nested-dir creation round-trips | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | +| FR-005 | AC-3: upsert replaces by name (count stays 1) | TC-013 — `registry › "write is atomic and round-trips; upsert replaces by name"` | ✅ Unit | +| FR-006 | AC-1: named git-subdir entry materializes + records | TC-014 — `installEntry › "materializes a named git-subdir entry and records it"` | ✅ Unit (git) | +| FR-006 | AC-2: name derived via readName when absent | TC-015 — `installEntry › "derives the name via readName when the entry has none"` | ✅ Unit (git) | +| FR-006 | AC-3: entry.path against a whole-repo source | TC-016 — `installEntry › "honors entry.path against a whole-repo source"` | ✅ Unit (git) | +| FR-006 | AC-4: symlink mode; re-install replaces | TC-017 — `installEntry › "symlink mode links instead of copying, and re-install replaces"` | ✅ Unit (git) | +| FR-007 | AC-1: lazy installs enabled, skips disabled | TC-018 — `reconcile › "lazy installs the enabled set, skips disabled, and is idempotent with zero git on the 2nd run"` | ✅ Unit (git) | +| FR-007 | AC-2: 2nd lazy reconcile → unchanged, zero git | TC-018 — `reconcile › "lazy installs the enabled set, skips disabled, and is idempotent with zero git on the 2nd run"` | ✅ Unit (git) | +| FR-007 | AC-3: sync unchanged on stable ref; updated on moved pin | TC-019 — `reconcile › "sync re-resolves: unchanged on a stable ref, updated on a moved pin"` | ✅ Unit (git) | +| FR-007 | AC-4: lazy re-materializes when target dir is gone | TC-020 — `reconcile › "lazy re-materializes when the target dir is gone"` | ✅ Unit (git) | +| FR-007 | AC-5: lazy sha pin → unchanged when matches, updated when differs | TC-021 — `reconcile › "lazy honors a sha pin: unchanged when it matches, updated when it differs"` | ✅ Unit (git) | --- @@ -171,34 +174,44 @@ partial clones work offline. | TC-026 | normalizeSource rejects option-like npm package (`-x`) | Unit | P0 | FR-004-CON-3 | ✅ | | TC-027 | parseNpmPackJson robust parse + descriptive parse-failure errors | Unit | P0 | FR-004-AC-12, -CON-4 | ✅ | | TC-028 | unpinned npm re-fetch clears stale tarballs (cache hygiene) | Unit (fake) | P1 | FR-004-AC-13 | ✅ | +| TC-029 | normalizeSource rejects option-like git argv fields (`-x`) | Unit | P0 | FR-004-AC-14, -CON-5 | ✅ | +| TC-030 | normalizeSource rejects leading-ws trim-bypass repo/url | Unit | P0 | FR-004-AC-15, -CON-5 | ✅ | +| TC-031 | normalizeSource rejects option-like `git-subdir.path` | Unit | P0 | FR-004-AC-16, -CON-5 | ✅ | > TC-022…TC-028 belong to the shared TC-022…TC-054 block also used by the > concurrent `feat/plugin-discovery` PR; IDs are reconciled at the second merge -> (this PR lands first). +> (this PR lands first). TC-029…TC-031 are the git argv-injection guard tests +> added by this branch, allocated after the npm block. --- ## Constraint Boundary Tests -| Constraint | Boundary / Case | Test Value | Test Case | Expected | -| ------------ | -------------------------------------------------- | --------------------------------------------- | -------------- | ------------------------------------------------------ | -| FR-004-CON-1 | package-manager subprocess is the sole side effect | injected fake `GitRunner` / `NpmFetcher` | TC-011, TC-022 | resolves with no real git/npm; argv[0]=`clone` | -| FR-004-CON-2 | blobless + sparse | `git-subdir` at `v0.2.0` | TC-008 | only the subdir present; tag sha resolved | -| FR-004-CON-3 | option-like npm package rejected | `{type:"npm", package:"-x"}` | TC-026 | `SourceError` "must not begin with -"; no `npm pack` | -| FR-004-CON-4 | no metadata array in `npm pack --json` output | `""`, `"[]"`, `"[1,2,3]"`, `'[{"name":"p"}]'` | TC-027 | `SourceError` "could not parse npm pack --json output" | +| Constraint | Boundary / Case | Test Value | Test Case | Expected | +| ------------ | -------------------------------------------------- | --------------------------------------------------------------- | -------------- | -------------------------------------------------------------- | +| FR-004-CON-1 | package-manager subprocess is the sole side effect | injected fake `GitRunner` / `NpmFetcher` | TC-011, TC-022 | resolves with no real git/npm; argv[0]=`clone` | +| FR-004-CON-2 | blobless + sparse | `git-subdir` at `v0.2.0` | TC-008 | only the subdir present; tag sha resolved | +| FR-004-CON-3 | option-like npm package rejected | `{type:"npm", package:"-x"}` | TC-026 | `SourceError` "must not begin with -"; no `npm pack` | +| FR-004-CON-4 | no metadata array in `npm pack --json` output | `""`, `"[]"`, `"[1,2,3]"`, `'[{"name":"p"}]'` | TC-027 | `SourceError` "could not parse npm pack --json output" | +| FR-004-CON-5 | option-like git argv field | `repo`/`url`/`ref`/`sha` = `-x` (e.g. `ref: "--upload-pack=…"`) | TC-029 | `SourceError` "must not begin with `-`"; no `git` invocation | +| FR-004-CON-5 | trim-bypass (leading ws) | `repo`/`url` = `" --upload-pack=… ext://x"` | TC-030 | `SourceError` "must not begin with `-`"; trimmed value guarded | +| FR-004-CON-5 | option-like subdir path | `git-subdir.path` = `"--stdin"` / `"-X"` | TC-031 | `SourceError` "must not begin with `-`"; no `git` invocation | --- ## Error-Path Coverage -| Error | Trigger | Test Case | Status | -| ---------------------------- | -------------------------------------------------- | --------- | ------ | -| `SourceError` | null / no-`type` / missing field / unknown type | TC-002 | ✅ | -| `SourceError` | `path` source dir does not exist | TC-006 | ✅ | -| `UnsupportedSourceError` | `url` passed to `resolveSource` (npm now resolves) | TC-007 | ✅ | -| `ManifestError` | non-object / bad schemaVersion / non-array entries | TC-005 | ✅ | -| `ManifestError` | null entry / entry missing a non-empty `name` | TC-005 | ✅ | -| `SourceError` (via manifest) | entry with an invalid `source.type` | TC-005 | ✅ | +| Error | Trigger | Test Case | Status | +| ---------------------------- | ------------------------------------------------------------------ | --------- | ------ | +| `SourceError` | null / no-`type` / missing field / unknown type | TC-002 | ✅ | +| `SourceError` | `path` source dir does not exist | TC-006 | ✅ | +| `SourceError` | option-like (`-`) `repo`/`url`/`ref`/`sha` (argv injection guard) | TC-029 | ✅ | +| `SourceError` | leading-whitespace trim-bypass `repo`/`url` (argv injection guard) | TC-030 | ✅ | +| `SourceError` | option-like (`-`) `git-subdir.path` (argv injection guard) | TC-031 | ✅ | +| `UnsupportedSourceError` | `url` passed to `resolveSource` (npm now resolves) | TC-007 | ✅ | +| `ManifestError` | non-object / bad schemaVersion / non-array entries | TC-005 | ✅ | +| `ManifestError` | null entry / entry missing a non-empty `name` | TC-005 | ✅ | +| `SourceError` (via manifest) | entry with an invalid `source.type` | TC-005 | ✅ | --- @@ -216,20 +229,23 @@ partial clones work offline. ## Coverage Summary -- **Acceptance Criteria → Test Case coverage: 40 of 40 functional ACs (100%) map to +- **Acceptance Criteria → Test Case coverage: 43 of 43 functional ACs (100%) map to an executed Test Case.** All ACs of FR-001…FR-007 (incl. FR-002-AC-5 whitespace trimming via the padded-input assertion in TC-003, the four npm-resolution ACs - AC-8…AC-11, and the two npm-robustness ACs AC-12 robust-parse and AC-13 - cache-hygiene), all four FR-004 constraints (CON-1…CON-4), and all 6 user-story ACs - map to a real test in `tests/index.test.ts`. NFR-001…NFR-004 are covered by the - coverage gate, the zero-git assertion, inspection, and analysis. -- The 28 TCs map to the tests in `tests/index.test.ts` (TC-022 spans both the + AC-8…AC-11, the two npm-robustness ACs AC-12 robust-parse and AC-13 + cache-hygiene, and the three git argv-injection guard ACs AC-14 via TC-029, the + trim-bypass AC-15 via TC-030, and the `git-subdir.path` AC-16 via TC-031), all + five FR-004 constraints (CON-1…CON-5), and all 6 user-story ACs map to a real test + in `tests/index.test.ts`. NFR-001…NFR-004 are covered by the coverage gate, the + zero-git assertion, inspection, and analysis. +- The 31 TCs map to the tests in `tests/index.test.ts` (TC-022 spans both the fake-fetcher and default-fetcher npm tests; TC-027 spans the five `parseNpmPackJson` - noise/no-bracket/empty/numeric/no-filename tests). All pass under `make test` at the + noise/no-bracket/empty/numeric/no-filename tests; TC-029…TC-031 are 1:1 with the + three argv-injection guard tests). All pass under `make test` at the 100% coverage gate. - All six test-matrix rules are satisfied: every AC has a TC (Rule 1); the copy/symlink materialize options are both exercised (Rule 2, TC-014/TC-017); the - two FR-004 constraints are boundary-tested (Rule 3); every documented error is + five FR-004 constraints are boundary-tested (Rule 3); every documented error is triggered (Rule 4); reconcile's installed/unchanged/updated/skipped outcomes are all reached (Rule 5); and the edge cases above are explicit (Rule 6). diff --git a/src/sources.ts b/src/sources.ts index 5ed3cc2..ed48c41 100644 --- a/src/sources.ts +++ b/src/sources.ts @@ -50,17 +50,36 @@ function req(value: unknown, field: string): string { /** * Like {@link req}, but also rejects an option-like value (leading `-`). Source * fields that flow into a `git`/`npm` argv must not be interpretable as a CLI - * flag — e.g. a package named `-x` would become an `npm pack` option (a - * second-order command-line injection), so it is rejected up front. + * flag — e.g. a package named `-x` would become an `npm pack` option, or a `ref` + * of `--upload-pack=` / a `repo` of `--output=…` would become a `git` + * option (a second-order command-line injection), so it is rejected up front, + * before any subprocess invocation. + * + * The check is on the **trimmed** value: `toGitUrl` does `raw.trim()` before the + * value reaches `git clone`/`git fetch`, so a leading-whitespace-then-dash + * payload (e.g. `" --upload-pack=…"`) would otherwise pass the raw guard yet + * trim to an option at the argv. Guarding the trimmed value makes the value that + * is validated the value that actually reaches the argv. */ function reqArg(value: unknown, field: string): string { const v = req(value, field); - if (v.startsWith("-")) { + if (v.trim().startsWith("-")) { throw new SourceError(`source field "${field}" must not begin with "-"`); } return v; } +/** + * Like {@link reqArg}, but for an optional field (`ref`/`sha`): `undefined` + * passes through, but any present value must be a non-empty, non-option-like + * string. `git checkout` does not accept a `--` separator cleanly before a ref, + * so an option-like ref/sha is rejected rather than escaped. + */ +function optArg(value: unknown, field: string): void { + if (value === undefined) return; + reqArg(value, field); +} + /** Validate a source descriptor, throwing {@link SourceError} on malformed input. */ export function normalizeSource(source: Source): Source { if (!source || typeof (source as { type?: unknown }).type !== "string") { @@ -68,17 +87,25 @@ export function normalizeSource(source: Source): Source { } switch (source.type) { case "github": - req(source.repo, "repo"); + reqArg(source.repo, "repo"); + optArg(source.ref, "ref"); + optArg(source.sha, "sha"); return source; case "git": - req(source.url, "url"); + reqArg(source.url, "url"); + optArg(source.ref, "ref"); + optArg(source.sha, "sha"); return source; case "git-subdir": - req(source.url, "url"); - req(source.path, "path"); + reqArg(source.url, "url"); + reqArg(source.path, "path"); + optArg(source.ref, "ref"); + optArg(source.sha, "sha"); return source; case "url": - req(source.url, "url"); + reqArg(source.url, "url"); + optArg(source.ref, "ref"); + optArg(source.sha, "sha"); return source; case "path": req(source.path, "path"); diff --git a/tests/index.test.ts b/tests/index.test.ts index 6e2d989..2d730d3 100644 --- a/tests/index.test.ts +++ b/tests/index.test.ts @@ -160,6 +160,98 @@ describe("normalizeSource", () => { normalizeSource({ type: "bogus" } as unknown as Source), ).toThrow(/unknown source type/); }); + + // TC-029 / FR-004-AC-14, -CON-5: source fields that flow into the `git` argv + // must not be interpretable as a CLI flag (leading `-`), or git treats them as + // an option (e.g. `ref` of `--upload-pack=`) — a second-order + // command-line injection. `normalizeSource` rejects them before any git + // invocation. + test("rejects option-like git argv fields (injection guard)", () => { + // repo / url reach `git clone` / `git fetch` + expect(() => + normalizeSource({ type: "github", repo: "-x" } as Source), + ).toThrow(/must not begin with "-"/); + expect(() => normalizeSource({ type: "git", url: "-x" } as Source)).toThrow( + /must not begin with "-"/, + ); + expect(() => + normalizeSource({ type: "git-subdir", url: "-x", path: "p" } as Source), + ).toThrow(/must not begin with "-"/); + expect(() => normalizeSource({ type: "url", url: "-x" } as Source)).toThrow( + /must not begin with "-"/, + ); + // ref / sha reach `git checkout --detach ` + expect(() => + normalizeSource({ + type: "github", + repo: "a/b", + ref: "--upload-pack=touch /tmp/pwned", + } as Source), + ).toThrow(SourceError); + expect(() => + normalizeSource({ type: "git", url: "u", sha: "-x" } as Source), + ).toThrow(SourceError); + expect(() => + normalizeSource({ + type: "git-subdir", + url: "u", + path: "p", + ref: "-r", + } as Source), + ).toThrow(SourceError); + expect(() => + normalizeSource({ type: "url", url: "u", sha: "-s" } as Source), + ).toThrow(SourceError); + }); + + // TC-030 / FR-004-AC-15, -CON-5: the argv guard must validate the value that + // actually reaches the + // `git` argv. `toGitUrl` trims its input (and passes a `://` value through + // unwrapped), so a leading-whitespace-then-dash payload (e.g. + // `" --upload-pack=touch /tmp/pwned ext://x"`) would slip past a raw + // `startsWith("-")` check yet trim to an *option* at the argv — a second-order + // command-line injection. The guard must reject the TRIMMED value. + test("rejects leading-whitespace option-like repo/url (trim bypass)", () => { + const payload = " --upload-pack=touch /tmp/pwned ext://x"; + expect(() => + normalizeSource({ type: "github", repo: payload } as Source), + ).toThrow(/must not begin with "-"/); + expect(() => + normalizeSource({ type: "git", url: payload } as Source), + ).toThrow(/must not begin with "-"/); + expect(() => + normalizeSource({ + type: "git-subdir", + url: payload, + path: "p", + } as Source), + ).toThrow(/must not begin with "-"/); + expect(() => + normalizeSource({ type: "url", url: payload } as Source), + ).toThrow(/must not begin with "-"/); + }); + + // TC-031 / FR-004-AC-16, -CON-5: `git-subdir.path` flows into + // `git sparse-checkout set ` + // and was only checked for non-emptiness (`req`), so an option-like value + // (`--stdin`, `-X`) reached the argv as a flag. Guard it like the other argv + // fields. + test("rejects option-like git-subdir path", () => { + expect(() => + normalizeSource({ + type: "git-subdir", + url: "u", + path: "--stdin", + } as Source), + ).toThrow(/must not begin with "-"/); + expect(() => + normalizeSource({ + type: "git-subdir", + url: "u", + path: "-X", + } as Source), + ).toThrow(/must not begin with "-"/); + }); }); test("toGitUrl expands shorthand and passes through URLs", () => {