Repository navigation
fix(loader): refuse a project .prompts directory that is itself a symlink - #295
Conversation
…link The per-file escape check added in #285 compares each symlinked template against realpathSync(dir), so when the .prompts entry is itself a link the attacker-chosen target became the trusted root and every *.md in it loaded as a project template. lstat the project dir first and refuse links with a diagnostic; ~/.prompts is the user's own and may remain a link. Fixes #294 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: sec-check <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Read src/loader.ts on this head against main and the #294 repro. The fix looks correct to me.
What I verified
src/loader.ts:30-41: thelstatSync(dir).isSymbolicLink()guard runs only forsource === 'project', beforereaddirSync/realpathSync, so a symlinked project.promptsnever becomesrealDirand the per-file prefix check at:80-82is never evaluated against an attacker-chosen root.~/.prompts(:99) is untouched, as the body states.- A dangling
.promptssymlink still hitsexistsSync(dir) === falseat:25and returns[]silently — same as before, no regression. src/loader.test.ts:173-192exercises exactly the #294 shape (real dir elsewhere,.prompts→ symlink) and pins that user templates survive and the diagnostic fires.console.errorcapture is valid becausediag()(src/log.ts:47-50) prints the legacy string when JSON logging is off.src/log.ts:19adds the event to theLogEventunion;dist/is rebuilt and the dist-sync gate passed. CI matrix (22/24/26 × linux/macos) is green.
One docs gap (low)
README.md:311 describes the per-file symlink rule from #285 ("Symlinked *.md entries are followed only when their real target is inside the same .prompts/ directory…") but does not mention that the project .prompts entry itself must now be a real directory and is refused with a diagnostic when it is a symlink. A user who committed a symlinked .prompts on purpose will see their templates vanish and have only the stderr line to go on. One sentence there would close it; not a blocker.
No other findings. Not approving — that is a maintainer's call.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Looks correct to me — read src/loader.ts at the PR head, built it, and ran the suite locally.
- The gap is real: before this change
loadFromDironly ever calledrealpathSync(dir)(src/loader.ts:41on main) and used the resolved path asrealDirPrefix, so a symlinked.promptsmade the attacker's target the trusted root and the per-file escape check from #285 passed every*.mdinside it. - The fix is in the right place:
lstatSync(dir).isSymbolicLink()runs beforereaddirSync/realpathSync(src/loader.ts:31-42), is scoped tosource === 'project', and returns[]so user templates are unaffected — the new test asserts both halves (src/loader.test.ts:173-192). dist/is in sync:npm run buildon the head leavesgit status -- dist/clean, and the newloader_dir_symlinkevent is reflected indist/log.d.ts.npm test: 418 pass, 0 fail.
One observation, not a defect: a dangling symlink at .prompts is still skipped silently because existsSync(dir) (which follows links) returns false before the new lstat runs. That's harmless — nothing is loaded — just no diagnostic in that case.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Reviewed (correctness) — approve; one advisory.
src/loader.ts:31-42—lstatSync(dir).isSymbolicLink()guard runs beforereaddirSync/realpathSync, so the attacker-chosen target never becomesrealDir(:57). Dangling.promptslinks are already filtered byexistsSync(:25);~/.promptsis untouched. Test atsrc/loader.test.ts:173-192pins refusal, user-template survival, and the diagnostic (legacy path ofdiag,src/log.ts:48-49, matches CI's default env).dist/loader.jsmirrors the source.- Advisory (low):
README.md:311still describes only the per-file symlink rule; it does not say the project.promptsentry itself must now be a real directory. Worth one sentence since this is a user-visible behavior change (theloader_dir_symlinkdiagnostic does explain it at runtime).
— hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
|
Hive filed review findings that do not block this PR as backlog items:
Cap: 10 backlog item(s) per PR per day. |
There was a problem hiding this comment.
Read src/loader.ts at d45e761 against the #285 code it extends. Looks correct to me.
lstatSync(dir).isSymbolicLink()runs beforeexistsSync(src/loader.ts:30-41), so the attacker-chosen target never becomesrealDirfor thestartsWith(realDirPrefix)check atloader.ts:76-79;source === 'user'is deliberately exempt, matching #294's recommendation.ENOENTfromlstatreturns[]silently, preserving the pre-existing "no.prompts" behaviour; a dangling link reaches the diag path (covered by the new test atloader.test.ts:191).- Locally:
npm ci && npm run buildleavesgit status --porcelain -- dist/empty, so the dist-sync gate should pass;npm testis green (99.38% lines).
One note, not a defect: a repo that legitimately lays out .prompts -> docs/prompts (in-repo symlink) is now refused as well. #294 asked for exactly this (refuse any symlinked project dir), and the README/changelog say so, so I'd land it as-is.
— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Read src/loader.ts at d45e761 end to end plus src/log.ts and the callers in loadTemplates/findTemplate; this looks correct to me.
Mechanism verified. Before this change the per-file escape check at src/loader.ts:52-56 compared symlink targets against realpathSync(dir) — so if the project .prompts entry was itself a symlink, the attacker's target became the trusted root and every *.md under it passed the prefix test. The new lstatSync(dir).isSymbolicLink() guard at src/loader.ts:31-40 runs only for source === 'project', before existsSync (which follows links), so ~/.prompts symlinks keep working and a dangling project link is explained rather than silently skipped. User templates still load when the project dir is refused (the two loadFromDir calls in loadTemplates are independent).
Checked locally at the PR head:
npm run build→git status --porcelain -- dist/is empty (dist-sync gate satisfied).npm test→ 419/419 pass, coverage thresholds met.- Manual repro:
.prompts -> ./xwithx/t.mdis refused withpromptargs: <path> is a symlink; …in default mode and{"event":"loader_dir_symlink","status":"refused"}underPROMPTARGS_LOG=json— no path leaks into the JSON line, consistent with the README promise. loader_dir_symlinkis added toLogEventin bothsrc/log.tsanddist/log.d.ts; README and changelog fragment (fixed-294-…, valid category) describe exactly what the code does.
No defects found. All check-runs on d45e761 are green; mergeable_state=unstable appears to be stale.
— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
…tidy test - README: state that the project .prompts entry must be a real directory (symlinked entry is refused with a diagnostic; ~/.prompts may be a link) and mention symlinked prompts directories in the PROMPTARGS_LOG=json list. - loader: check lstat before existsSync for the project source so a dangling project .prompts link is explained with loader_dir_symlink instead of being skipped silently; a plain missing dir stays silent. - loader.test: use t.mock.method(console, 'error') like sibling tests and add a dangling-link case. dist/ rebuilt. Hive-Fix-Attempt: 1/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: scanner <280983584+hivecommons-hive[bot]@users.noreply.github.com>
d45e761 to
ee33bff
Compare
|
hive/awaiting-ci: re-signed the unsigned follow-up commit (d45e761 → ee33bff, content unchanged) so the DCO check passes; CI pending — sweep will merge when green. 🐝 Hive Agent: — hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Security Fix
#285 made
loadFromDirrefuse.prompts/*.mdsymlinks whose real target escapes the owning.prompts/directory, but the root it compared against wasrealpathSync(dir)— so when the project.promptsentry is itself a symlink (which a cloned repo can commit), the attacker-chosen target became the trusted root and every*.mdinside it was listed, shown and expanded as a project template.This change
lstats the project.promptsentry before reading it and refuses it when it is a symlink, with aloader_dir_symlinkdiagnostic explaining why.~/.promptsis the user's own and may still be a symlink. User templates keep loading when the project dir is refused.src/loader.ts: lstat guard forsource === 'project'src/log.ts: newloader_dir_symlinklog eventsrc/loader.test.ts: pins the refused symlinked dir, that user templates survive, and that the diagnostic is emittedchangelog.d/fixed-294-prompts-dir-symlink.mddist/rebuilt (dist-sync gate)Validation:
npm run build && npm test— 418/418 pass, coverage thresholds met.Fixes #294
Filed by sec-check agent (ACMM L6 — full mode)
— hive: agent=sec-check backend=copilot model=claude-fable-5.1 copilot=1.0.88 requested_by=@clubanderson