feat(NO-TASK): Require trust for repository hooks and contain path arguments - #69
Merged
Merged
Conversation
…guments Closes four ways a cloned repository could execute code or destroy data through the wt surface. This matters more than usual here because the CLI is designed to be pre-approved in an agent allowlist: a rule like Bash(linchpin wt:*) matches the verb by prefix and cannot see the argument or the repository contents. Hook trust. .linchpin/hooks/* are committed files, so they arrive with a clone -- unlike .git/hooks, which git deliberately refuses to transfer for exactly this reason. Sourced rather than executed, a hook needs no execute bit and no shebang, so nothing in a diff marks it as code that will run. Hooks now do nothing until the machine approves them, keyed to a sha256 of the contents, so editing a trusted hook withdraws its trust. Adds `linchpin wt trust` to list, grant, --all and --revoke. Follows direnv and mise in blocking with instructions rather than prompting. A blocked hook does not fail the operation, and a hook that runs prints its path. Path containment. findHookFile joined an argv value straight onto the hooks directory and the result was sourced as bash, so `wt invoke ../../../payload` ran any file on the machine; wt copy and wt link shared the pattern feeding cpSync and symlinkSync. All three now resolve through a containment helper. Destructive switch. The target of the recursive delete behind `wt switch --force` comes from the committed .linchpin.json, so a repository chose what got removed. The target must now sit inside a WordPress content directory, and --force asks before destroying anything, refusing rather than assuming consent when there is no TTY. --yes is the explicit scriptable answer. Behaviour change for automation: scripts calling `wt switch --force` need --yes, and CI relying on hooks needs them trusted first. See docs/hooks.md. resolveContained uses path.resolve rather than path.join, because join folds an absolute segment into the root instead of refusing it. isContainedAfterLinks resolves both sides, since git tracks symlinks and a candidate-only realpath would compare /private/tmp against /tmp and reject every legitimate path on macOS. Trust keys on the hook realpath for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nses LINCHPIN_REGISTRY and npm_config_registry were accepted with no scheme check, so a plaintext registry -- which anyone on the path can answer -- was honoured. They are environment values, exactly as trustworthy as whatever set them. http is now refused unless the host is loopback, so a local Verdaccio mirror still works, or LINCHPIN_REGISTRY_ALLOW_INSECURE is set. The latest dist-tag and the response status text both reach a terminal, and neither was stripped of control characters, so a hostile or intercepted registry could rewrite the output around an update notice with ANSI escapes. Both are sanitized at the boundary rather than at each print site, because latest is cached to disk and read back by later runs -- cleaning it on arrival is what stops a hostile answer outliving the request that fetched it. The install command was never at risk: it is a hardcoded name@latest with no argv injection. The child npm does inherit the environment, so a poisoned npm_config_registry still steers the real install; that is npm's own behaviour and is out of scope here. Also fixes an unparseable registry value, including the empty string, producing a relative URL and a confusing fetch failure. It now falls back to the default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| function trustHooks(basePath) { | ||
| const hooksDir = path.join(basePath, '.linchpin', 'hooks'); | ||
|
|
||
| let store = { hooks: {} }; |
| const { findHookFile, runHook } = require('../lib/hooks'); | ||
| const { findHookFile, runHook: runHookRaw } = require('../lib/hooks'); | ||
| const { | ||
| describeUntrustedHook, |
| const { findHookFile, runHook: runHookRaw } = require('../lib/hooks'); | ||
| const { | ||
| describeUntrustedHook, | ||
| hashHookFile, |
| describeUntrustedHook, | ||
| hashHookFile, | ||
| isHookTrusted, | ||
| readTrustStore, |
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
1 participant
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.
Security review of the
wtsurface, closing five findings. Each was reproduced with a working proof-of-concept before the fix and re-verified against the built CLI after it.Full review: https://claude.ai/code/artifact/4510690e-32f4-4560-a752-e14f1f7cf389
Why these matter more than usual
This CLI is designed to be pre-approved in an agent allowlist —
package.jsoncalls it "deterministic verbs agents can call without approval prompts." A rule likeBash(linchpin wt:*)matches the verb by prefix and cannot see the argument or what the repository has committed. So the security boundary is not the verb; it is what the verb does with untrusted input.src/core/exec.tswas already right — argv arrays, noshell: true, branch names inert as data. Every finding is a layer up: what a repository could direct the tool to do once someone cloned it.Findings
git cloneequivalent to running codewt invokeescaped the hooks directory and sourced any file--forcerecursively deletedwt copy/wt linkshared the same unbounded joinF1 —
.linchpin/hooks/*are committed, so they arrive with a clone, unlike.git/hookswhich git deliberately refuses to transfer for exactly this reason. Sourced rather than executed, a hook needs no execute bit and no shebang, so nothing in a diff marks it as code that will run. Verified: clone + one ordinarywt switchexecuted a committed hook. Now gated behind content-hashed trust,direnv-style.F3 — the recursive delete target comes from the committed
.linchpin.json. Verified: a cloned repo destroyed a directory of real data, no backup taken. Now requires a WordPress-slot target and confirmation.Not marked breaking, so this needs calling out explicitly:
wt switch --forcemust add--yesor they will now fail rather than delete.linchpin wt trust --all, or seedLINCHPIN_TRUST_FILE).LINCHPIN_REGISTRY_ALLOW_INSECURE=1.docs/hooks.mddocuments the trust workflow.README.mdstill needs its hooks section updated — deliberately left alone here to avoid colliding with #shell-update-notice.Verification
paths,trustandupdate-registry, dual-mode so the legacy CJS and ported TS twins cannot drift.--forcerefuses without a TTY, and the slot guard refuses even when--yesis supplied.wp-content-shaped path. Worth confirming in review that those are the right new expectations rather than tests bent to fit.Notes for reviewers
Three subtleties that were real traps:
resolveContainedusespath.resolve, notpath.join—join('/repo', '/etc/passwd')silently yields/repo/etc/passwd, rewriting the caller's path instead of refusing it.isContainedAfterLinksrealpaths both sides. Git tracks symlinks, so a repo can commit a hook that is one; and resolving only the candidate would compare/private/tmp/…against/tmp/…and reject every legitimate path on macOS.🤖 Generated with Claude Code