Skip to content

fix(composer): redirect confirmation, discovery, setup fidelity, revert safety - #195

Open
Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
mainfrom
research/composer-review
Open

fix(composer): redirect confirmation, discovery, setup fidelity, revert safety#195
Mikola Lysenko (mikolalysenko) wants to merge 6 commits into
mainfrom
research/composer-review

Conversation

@mikolalysenko

@mikolalysenko Mikola Lysenko (mikolalysenko) commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

LLM Description written by Claude Code:claude-fable-5

What

Fixes the 13 verified bugs from a full audit of composer (PHP) support — code review across every composer code path plus behavioral testing against real composer 2.10.2 / PHP 8.5.8, with each finding adversarially verified before being fixed. Five logical commits, one per area.

Hosted / registry-redirect (94e51ec)

  • A fully successful composer redirect reported redirected: 0 — the rewriter wrote \/-escaped URLs, the confirmation probe searched raw/percent-encoded only. No patch records reached the ledger; VEX had nothing to attest. The rewriter now emits composer-native raw slashes and shares one artifact_url_present predicate with the probe.
  • A source-only (dist-less) target made the rewriter walk past the package boundary and silently rewrite the next package's dist url+shasum (checksum-clean wrong-code install). Entry lookup is now bounded by a string-literal-aware brace matcher; dist-less targets fail closed.
  • Name-only, exact-case entry matching now also requires the locked version (v-normalized) and matches names case-insensitively, like the crawler and vendor backend.
  • A dist block without a shasum key was redirected with no integrity pin; the sha1 is now inserted.
  • Goldens: basic fixture converted to the raw-slash format real composer writes, plus five new cases. New in-process e2e proves redirected: 1 with ledger records end to end — the coverage gap that hid all of this.

Scan discovery (e55d6cf)

  • config.vendor-dir / COMPOSER_VENDOR_DIR were ignored: installed packages were misreported as lockfile-only and apply silently couldn't locate them. Vendor-dir resolution now mirrors composer's precedence.
  • installed.json install-path was dropped, making composer/installers packages (WordPress plugins, Drupal modules) invisible and unpatchable. It is now the authoritative package dir, lexically contained to the project root, with no conventional-path fallback for rejected values.
  • V1.2.3 tags double-counted (crawler stripped v|V, lock inventory only v); both now share normalize_version.

Setup (f13889c)

  • Composer-authored 4-space manifests were permanently reformatted to 2-space, so --remove never restored bytes; serialization now keeps the input's indent and newline shape (byte-identical round-trip, proven against PHP-generated fixtures).
  • composer.json file mode was reset on edit (0744 → 0644); switched to the mode-preserving atomic writer and un-ignored the in-tree RED test.
  • The setup-matrix composer case never staged a composer.json, so it could never pass under --features setup-e2e; it now stages a minimal manifest and passes 15/15 against real composer, and baseline_supported flips true so future failures read REGRESSION.

Vendored (7c57ac4)

  • Reverting a repair-reconstructed (wiring-less) entry deleted the artifacts while leaving composer.lock pointing at them; it now refuses with the escape hatch spelled out, keeping artifacts and ledger row.
  • The GC orphan sweep deleted un-ledgered uuid dirs that lockfiles still referenced; candidates are now cross-checked against scan_vendor_references and kept with vendor_orphan_still_wired.
  • The docker vendor capstone's networked composer update gains the documented 3-attempt retry.

CI (734a8e0)

  • The e2e_composer matrix job ran -- --ignored against a suite with zero ignored tests — a permanent 0-test green. It now runs the real tests via a per-entry test_filter (--include-ignored); all other entries render unchanged. (FYI: e2e_cargo and e2e_golang have the same vacuous shape; left out of scope.)

Testing

Combined-tree battery, all green: full socket-patch-core suite; e2e_composer 2/2; in_process_redirect 24/24 (incl. the new composer confirmation e2e, whose failure mode was falsified against the pre-fix code first); e2e_vendor_composer_build 7/7 plus the real-composer host capstone (vendor → fresh-checkout install → revert); ecosystem_dispatch_e2e 22/22; in_process_vendor 27/27; setup_matrix_composer 15/15 under --features setup-e2e in host mode; cargo fmt --check clean. Crawler fixes additionally validated live against real composer projects (custom vendor-dir, composer/installers WP-plugin layout, COMPOSER_VENDOR_DIR).

Not run here: docker_e2e_composer / docker_e2e_vendor_composer — the local Docker daemon is wedged (the feature compiles; suites passed pre-fix on 2026-08-13 and deserve one real run once a daemon is available).

Follow-up (depscan)

The TS twin workspaces/app/src/patches/registry-rewrite/composer-lock.ts has the same rewriter bugs and is already ported to mirror this behavior byte-for-byte (verified against these goldens). It lands in depscan together with the socket-patch submodule bump after this merges.

🤖 Generated with Claude Code


Note

Medium Risk
Changes touch lockfile rewriting, patch target resolution, and vendor revert/delete paths where mistakes could break installs or delete the wrong artifacts; scope is Composer-focused with substantial new tests and fail-closed guards.

Overview
Fixes a cluster of Composer ecosystem bugs across redirect, scan/apply discovery, setup, vendor revert, and CI.

Registry redirect now emits composer-native raw slashes in composer.lock, shares artifact_url_present with the hosted scan confirmation probe (so successful redirects are counted and ledger/VEX records land), bounds dist edits to the correct package entry (version + case-insensitive name), inserts missing shasum pins, and fails closed on source-only targets and version mismatches instead of rewriting a neighbor package’s dist.

Discovery respects config.vendor-dir, COMPOSER_VENDOR_DIR, and install-path (with project-root containment), and aligns lock inventory version normalization with the crawler so packages are not double-counted.

setup preserves manifest indent/newline shape and file mode on composer.json edits; the setup-matrix composer case now stages a manifest and is marked baseline-supported.

Vendor revert refuses repair-reconstructed entries with no restorable wiring while the lock still points at vendored paths; orphan sweeps keep dirs still referenced in lockfiles and warn with vendor_orphan_still_wired.

CI runs e2e_composer with --include-ignored so non-ignored tests actually execute; docker vendor composer gets a retry loop on composer update.

Reviewed by Cursor Bugbot for commit 734a8e0. Configure here.

The composer.lock rewriter wrote `\/`-escaped URLs while the
post-rewrite confirmation probe searched only raw and percent-encoded
spellings, so every successful composer redirect reported
`redirected: 0`, persisted no patch records into the ledger, and left
`vex` with nothing to attest. The rewriter now emits composer-native
raw slashes (PHP `JSON_UNESCAPED_SLASHES`) and shares one
`artifact_url_present` predicate with the probe — both spellings
accepted — so writer and prober can never drift again.

Three more rewriter defects fixed:
- Entry lookup is bounded by a string-literal-aware brace matcher, so
  a source-only (dist-less) target fails closed with
  `redirect_composer_no_dist` instead of silently rewriting the NEXT
  package's dist url+shasum — a checksum-clean wrong-code install.
- Names match case-insensitively and the locked version must match the
  patched one through composer's leading-`v` normalization; a drifted
  lock fails closed with `redirect_composer_version_mismatch`.
- A dist block without a `shasum` key now gets the sha1 pin inserted
  (composer's key order) instead of redirecting unverified.

New codes: `redirect_composer_no_dist_url`,
`redirect_composer_lock_malformed`. The basic golden is converted to
the raw-slash format real composer writes, with five new cases
(escaped-slash lock, no-shasum-key, source-only bystander,
version-mismatch, mixed-case + v-prefix). A new in-process e2e proves
the full loop: rewritten lock, `redirected: 1`, patch record and
revert edit in the ledger.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Discovery hardcoded `cwd/vendor`, so a project using composer.json
`config.vendor-dir` or `COMPOSER_VENDOR_DIR` scanned as "not yet
installed (lockfile-only)" and apply resolved its packages as
not-found — installed vulnerable code could never be patched, with no
error. The vendor dir now resolves the way composer does
(`COMPOSER_VENDOR_DIR` > `config.vendor-dir` relative to the manifest
> `vendor`), shared by crawl, find, and apply. Unsafe config values
(traversal, absolute) fail closed rather than falling back to a tree
composer never installed into.

installed.json's `install-path` was ignored, so packages installed
through composer/installers (WordPress plugins, Drupal modules — the
classic PHP CVE carriers) were invisible and unpatchable even though
their exact path sat unread in the record. It is now the
authoritative package dir, gated lexically to the project root; a
rejected path drops the entry instead of falling back to the
conventional dir a poisoned record could squat.

lock_inventory now shares the crawler's `normalize_version`, so a
composer-legal `V1.2.3` tag no longer double-counts as an installed
row plus a phantom lockfile-only row (both previously POSTed to the
API).

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
`setup` re-serialized composer-authored manifests from their native
4-space indent (PHP `JSON_PRETTY_PRINT`) to 2-space, so it produced a
whole-file diff and `--remove` could never restore the original
bytes. It now detects and keeps the file's own indentation and
trailing-newline shape via the shared detect_indent/serialize_json
helpers; a composer-written manifest round-trips byte-identically.

The write also went through the non-mode-preserving atomic writer —
the only manifest editor that did — resetting e.g. 0744 to 0644.
Switched to `atomic_write_bytes_preserving_mode` like every sibling
editor; the in-tree RED test documenting the bug is un-ignored and
passing.

The setup-matrix composer case never staged a composer.json before
running `setup`, so it reported `no_files` and the case was
deterministically red under `--features setup-e2e` (and a silent
0-test green without it), mislabeling working functionality as a
baseline gap. The driver now stages a minimal manifest like the npm
arms, and `baseline_supported` flips to true so a future failure
reads REGRESSION instead of BASELINE GAP.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Reverting a repair-reconstructed composer entry (wiring lost, ledger
rebuilt from the wired lock) reported success while deleting the
vendored artifacts and leaving composer.lock pointing at the deleted
paths — the next install failed "Source path ... is not found" with
no guidance. The pristine dist survives only in `wiring[].original`,
which such an entry does not have, so synthesizing a restore would
mean guessing; revert now refuses the entry instead, keeps the
artifacts and the ledger row, and names the escape hatch
(`composer update --no-install <pkg>`, then revert again). Dry and
wet runs agree.

The GC orphan sweep deleted un-ledgered uuid dirs under
`.socket/vendor` on the false premise that no ledger means no live
wiring — with state.json lost, dirs still referenced by a lockfile
were swept and fresh installs broke. Candidates are now cross-checked
against `scan_vendor_references` (the same all-ecosystem scan repair
reconstructs from); referenced dirs are kept and surfaced as
`vendor_orphan_still_wired`, and reclaimed counts no longer include
them.

The docker vendor capstone's networked `composer update` gains the
sibling suite's 3-attempt retry — that fetch is the documented
dominant flake.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The e2e_composer matrix job invoked its suite with `-- --ignored`,
but e2e_composer.rs has no #[ignore] tests, so the job permanently
passed while running zero tests. The shared run step now takes a
per-entry `test_filter` and composer sets `--include-ignored` — the
two real tests run, and a future #[ignore]-gated composer capstone
still runs on this leg. Every other matrix entry renders the same
command as before.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issue.

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 734a8e0. Configure here.

Some(configured) if path_safety::is_safe_multi_segment(&configured) => {
Some(cwd.join(configured))
}
Some(_) => None,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Vendor-dir rejects Composer ./ paths

Medium Severity

config.vendor-dir values like ./vendor or ./lib/deps are valid in Composer but fail is_safe_multi_segment because . is not a safe segment. The new resolver then returns None with no fallback, so scan and apply find no installed packages. Previously those projects still resolved at vendor/.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 734a8e0. Configure here.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[agent] Fixed in 65ad6e8. config.vendor-dir values are now reduced to plain segments before the safety gate — ./vendor, ./lib/deps, lib/./deps, and backslash separators all resolve (.. resolves lexically and fails closed at or above the project root; absolute values stay refused as write-target hardening). Covered by the new config_vendor_dir_dot_prefix_is_normalized e2e and test_normalize_config_vendor_dir unit cases.

Composer-legal `config.vendor-dir` values with `.` segments or
backslash separators (`./vendor`, `./lib/deps`, `lib\deps`) were
refused by the safety gate with no fallback, regressing those
projects to zero discovery — worse than the old hardcoded `vendor/`
(Bugbot review finding). The value is now reduced to plain `a/b`
segments first (`.` drops, `..` resolves lexically and fails closed
above or at the project root, absolute stays refused), then gated.

The crawler e2e tests injected absolute paths into fixture JSON via
`display()`, which on Windows embeds raw backslashes and makes the
document unparseable — installed.json discovery collapsed to zero and
install_path_escaping_project_root_is_rejected failed for the wrong
reason (and the absolute vendor-dir refusal test passed vacuously).
Both injections now JSON-escape backslashes.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

2 participants