Repository navigation
fix: stamp updatedAt, refuse dead selectors, and harden the local server - #40
Conversation
Seven audit findings, all cases where the tool reported success or published a value that was not true. Nothing wrote `manifest.updatedAt`, so every mutation left the date the manifest was first authored while the gallery rendered it as its only freshness signal. Each non-dry-run manifest write now stamps it. `convert --only hoem` converted nothing and exited 0, which in a gated workflow is a migration step that silently does not run. An id matching no entry is now refused, mirroring the existing empty-selector rule. `convert` encoded every selected asset before checking whether the run was even legal, so a collision that was going to be refused paid for the full encode first; the documented dry-run-then-run flow encoded everything twice. Planning and encoding are now separate, targets are validated first, and the dedupe that claimed to share an encode actually does. A failed `unlink` of an original after the manifest was durably committed turned a complete conversion into exit 1, and a retry then found nothing to do. Those paths are collected into `retainedSources` and the command succeeds. The gallery interpolated asset paths into href and src with HTML escaping only, while serve decodes the request path — so a file name containing a percent sequence rendered broken and 404ed while validate passed. Its reference count was also baked at build time while filtering is pure CSS, so it kept claiming the full count under an active filter. `serve` set one header and checked nothing else: no Host validation, no nosniff, no method gate.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
🟡 Changes recommended
src/serve.ts still has response paths that omit the intended security headers and HEAD requests currently stream bodies, so the hardening is not consistently applied.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR tightens CLI correctness and local-server safety by ensuring user-visible/stated outcomes match reality (manifest freshness, selector validation, conversion planning, and hardened HTTP responses) while updating tests and docs accordingly.
Changes:
- Stamp
manifest.updatedAton non-dry-run manifest rewrites, keeping dry runs byte-identical. - Make
convert --onlyfail fast on unknown ids, and split conversion into “plan → validate targets → encode once per target → write” phases (plus non-fatal cleanup reporting viaretainedSources). - URL-encode rendered asset paths, make the reference count reflect active CSS filters, and harden
servewith Host validation, method gating, and security headers.
File summaries
| File | Description |
|---|---|
| test/vref.test.ts | Extends test coverage for serve hardening, updatedAt stamping, unknown selectors, and URL-encoded asset paths; adjusts gallery count assertion. |
| src/types.ts | Adds retainedSources to conversion result typing. |
| src/serve.ts | Adds Host validation, method gating, and security headers; updates response headers for successful requests. |
| src/render.ts | URL-encodes asset paths, adds a dynamic results count element, and updates count on filter changes. |
| src/manifest.ts | Introduces touchUpdatedAt() helper for stamping manifest freshness. |
| src/manifest-edit.ts | Applies touchUpdatedAt() on non-dry-run manifest writes during add flows. |
| src/describe.ts | Documents --only unknown-id behavior and retainedSources; adds field to command outputs. |
| src/convert.ts | Refuses unknown selectors, separates planning from encoding, dedupes encodes by target, reports retained sources, and stamps updatedAt on write. |
| src/cli.ts | Exposes retainedSources in CLI --fields. |
| skills/vref/references/manifest.md | Updates agent guidance to reflect new --only, retainedSources, and updatedAt behavior. |
| README.md | Notes that manifest rewrites stamp updatedAt. |
| docs/VREF.md | Documents updatedAt stamping behavior and convert semantics updates. |
Review details
Suppressed comments (1)
src/serve.ts:102
- Some error responses still omit the new security headers. In the catch block, the VREF_BAD_SERVE_PATH 400 and the generic 404 fallback call writeHead without
securityHeaders(), so these paths won’t includex-content-type-options/CSP even though the server is intended to harden every response.
response.writeHead(200, { ...securityHeaders(), "content-type": contentType(filePath) });
await pipeline(createReadStream(filePath), response);
} catch (error) {
if (error instanceof VrefError && error.code === "VREF_BAD_SERVE_PATH") {
response.writeHead(400);
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9431053c10
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Allowing HEAD without short-circuiting streamed the file body with it. HEAD now carries the headers, a content-length, and nothing else. The catch branch answers the common cases — a missing file throws out of realpath rather than reaching the isFile check — and its 404 and 400 were the two responses still missing the new headers, so the every-response guarantee was not one. Host comparison used the spelling the user typed, but a client canonicalizes an IPv6 literal before sending it. `--host 0:0:0:0:0:0:0:1` therefore 403ed requests to the url serve itself printed. Both sides are folded now. A retained original is the one thing the exit code deliberately no longer reports, so the human output names it instead of claiming a clean migration. The unlink-failure test is skipped as root, where directory permissions do not prevent unlink and the failure it induces cannot occur.
## [1.3.3](v1.3.2...v1.3.3) (2026-09-18) ### Bug Fixes * stamp updatedAt, refuse dead selectors, and harden the local server ([#40](#40)) ([ee69529](ee69529))
|
🎉 This PR is included in version 1.3.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c41a309049
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Round 2 of the CLI audit — seven findings where the tool reported success, or published a value, that was not true. Findings 15 (CI macOS matrix) and 18 (remote brand fonts) were dropped by maintainer decision.
Manifest and convert
updatedAtis stamped on every real write. Nothing wrote it. Every mutation left whatever the manifest was first authored with, while the gallery renders it as a headline "Updated <date>" — the one freshness signal a reviewer looks at, drifting further from the truth with each screenshot added. Dry runs still leave the file byte-identical.--onlyrefuses an id that matches nothing.vref convert --only hoemconverted nothing, printed "converted 0 references", and exited 0. In the gated agent workflow the skill documents, that is a migration step that silently does not run.parseListalready argued this exact case for empty selectors.convertplans before it encodes. Every selected asset was encoded inside the collection loop, before the collision and claimed-target checks ran — so a run destined to be refused paid for the whole encode pass first, and the documented dry-run-then-run flow encoded every image twice. Planning and encoding are now separate phases. This also makes an existing comment true: it claimed two entries naming the same asset "share a single encode", but the dedupe ran after each had already been encoded separately.A failed cleanup no longer fails a finished conversion. The source
unlinkran unguarded after the manifest was durably committed, so an EACCES there turned a complete, correct conversion into exit 1 — and a retry found every entry already.webpand exited 0, making the transient look like a flake. Those paths come back asretainedSourcesand the command succeeds.savedBytesno longer credits bytes that were not actually reclaimed.Gallery and serve
Asset paths are URL-encoded.
href/srcgot HTML entity escaping only, butservecallsdecodeURIComponenton the request path. Afilecontaining a percent sequence resolved to a different name: broken image in the gallery, 404 underserve, whilevalidateandbuildboth passed.The reference count follows the filters. Filtering is pure CSS and the count was baked at build time, so "42 references" sat above 6 visible cards whenever a filter was active.
servehardens its responses. It setcontent-typeand nothing else — noHostvalidation, nonosniff, no method gate. A long-running local server was reachable from any page the developer had open, and the gallery is UI evidence thatSECURITY.mdwarns may carry account state. Requests whoseHostis not the bound host get 403, non-GET/HEAD get 405, and every response carriesnosniffplus a CSP. The CSP permits the inline style and scriptvref buildemits, and the put.io font host.Tests
99 → 104. Two harness problems worth noting, both mine rather than the code's:
fetchrefuses to setHost, so the rebinding case drives a rawhttp.request; andunlinkpermission comes from the directory, which also blocks creating a file, so the retained-source test pre-creates the target and locks the directory — the write then needs only the file.One existing test changed meaning: it asserted
--only missing-idyieldsconvertedCount: 0, pinning the behavior finding 7 calls wrong. It now asserts the refusal.Still open
Findings 10, 11, 14, 16 — docs contradicting
describeon.jpeg, Effect RC types in the published.d.mts, the duplicated rollback protocol, and no coverage measurement. Those are a separate PR.