v0.4.0: close five untrusted-input paths, and bring the docs back in line - #8
Merged
Conversation
A security review of the tree found three places where the code did not do what its own comment claimed, plus two smaller ones. All five are fixed here with regression tests; the first two are confirmed exploitable and were verified against live net/url, net/http and git behaviour rather than reasoned about. A File `source` value reached the compiler's file_content endpoint as an unescaped path concatenation. It is the only one in the tree: every other URL this codebase builds escapes its interpolation or is a constant. A `source` is a parameter of a File resource in the candidate catalog, and the candidate catalog is compiled from the change under review, so `puppet:///../../pdb/query/v4/catalogs/<node>` was attacker-shaped input steering an mTLS-authenticated GET. net/url does not remove dot segments from a path it is handed and net/http sends the request line as written, so the traversal survived intact to the wire. Bounded by the transport, which pins scheme and authority and refuses a cross-authority redirect, and by this package never returning retrieved bytes, so it reached other paths on a host PIACE is already authorized against and yielded a digest rather than content. parsePuppetSourceURI now returns an error and refuses an empty, "." or ".." segment and a NUL byte. The inference client had no redirect policy. internal/transport, which carries no credential, has an explicit and tested one; internal/inference, which carries the only bearer token in PIACE, had none, and that asymmetry was the defect. net/http keeps an Authorization header across a redirect whenever the target host is the original host or a subdomain of it, judged on the host alone, so an endpoint answering `302 Location: http://<same host>` got the token in cleartext. checkRedirect now refuses any redirect that leaves https or changes authority. HTTPClient becomes unexported behind SetHTTPClient at the same time: an exported field that five tests and cmd/piace assigned to was a policy that could be dropped by assignment, which is the wrong property for the one client holding a credential. The text report passed control characters through. It is the format that goes to a terminal, and almost everything it interpolates is untrusted: a resource title and a parameter value come from the candidate catalog, a change assessment's summary is prose a model wrote. `ESC[2J ESC[1;1H` followed by a forged outcome line is enough to make a run that exits 30 read as clean to the person deciding whether to merge it. One textf helper now escapes C0, DEL and C1 in every line, preserving only the newlines the format string itself contributes. The JSON and HTML reports are deliberately unchanged: JSON escaping already makes a control character inert and that document's canonical checksum is what explain ties an assessment to, and html/template covers the HTML. Refs reaching git are validated. On a fork pull request the base and head refs are branch names whoever opened the change chose, and a ref of `--output=<path>` reaching `git diff` positionally is an arbitrary file write on the runner holding the catalog-reader identity. As it happens `merge-base` runs first and rejects an unknown option, so that value dead-ends before `diff`, but a guard that depends on the argument order of the first of four commands is not a guard. validateRef refuses a leading "-" before any git command runs, and --end-of-options covers what a future reordering might otherwise let through. Two smaller ones. The inference client read its response body through a LimitReader without the extra byte internal/transport uses to tell a body that reached the limit from one that exceeded it, so an oversized response was reported as "not a chat completion" and sent an operator looking at the wrong thing. And --debug-dump-dir wrote through os.WriteFile, which follows a symlink already sitting at the path and leaves that file's existing mode, applying 0600 only to a file it creates itself; those bodies are the unredacted ones and a dump directory is often /tmp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Anthropic's OpenAI-compatible endpoint documents itself as ignoring `response_format` rather than rejecting it, so a request that relied on structured output alone asked for nothing. TaskPrompt now describes the exact JSON shape unconditionally, and a test holds it to ResponseSchema so the two cannot drift. - Interpret unwraps a Markdown code fence that wraps the whole response, recording a warning, rather than discarding a complete assessment over its packaging. Only a fence that both opens and closes the trimmed reply is removed; backticks inside a rationale are left alone. - Label the impact-estimate section "Potential impact estimate" with a capital P across every output format and its golden files. - Color a successful differences_allowed run green: it shares exit 0 with clean, and yellow is reserved for the advisory medium-risk indication, not for a run that succeeded.
GitHub Copilot publishes no OpenAI-compatible chat-completions REST API, and GitHub Models was retired on 30 July 2026, so the block covers the endpoint that does work: the resource's v1 surface, the deployment name that goes in `model` rather than a model id, and the resource key or Entra ID token sent as a bearer.
A pass over every doc and package comment for claims that had drifted. Corrections, each checked against the code rather than reasoned about: - targets-snapshot-baseline.yaml claimed the TLS paths in services.yaml resolve against the working directory. They resolve against the services file, and have since 0.3.0 introduced the one path rule; the comment contradicted the README, CONTEXT.md and its own changelog entry. - development.md's package layout omitted internal/capture and internal/exitcode, tagged cmd/piace with a stale "(task 12)", and its CI table listed three jobs where ci.yml has four: `image` publishes the container to Docker Hub and GHCR after `release`. The claim that `release` is the only job granted contents: write does still hold. - development.md said every package's doc.go records its decisions. Eleven of seventeen packages have one; the rest carry the package comment on a source file. - ci.md's exit-code table dropped content-verification and enabled-impact-estimate failures from 30. - change-context.yaml still described a "generator script"; the change-context subcommand has taken the title and description by variable name or path since 0.3.0. - services.yaml pointed at a README section that lives in change-assessment.md. - requirements.md 1.8, design §8 and "10.5" were cited in targets-v3-legacy.yaml, the changelog and report/doc.go; neither document is in the repository. The twenty-one numbered spec markers in the report tests go the same way: where the number was the only thing saying why an assertion existed it is replaced with that reason, and where the t.Errorf below it already said so it is simply gone. Two properties the last two commits added but did not document: filecontent/doc.go described the source-to-path mapping without the traversal and NUL rejection parsePuppetSourceURI now performs, and report/doc.go covered HTML escaping without the text report's control-character escaping. Both are now in the package comment that owns them, along with the broken sentences and misdirected cross references found while reading those files. De-duplications, keeping the copy a reader needs to act on and linking the rest: the README's candidate-environment explanation against ci.md, its change-assessment opener against change-assessment.md, CONTEXT.md's path-rule mechanics against the README, the commit-body and untrusted-text arguments between ci.md and change-assessment.md, and release.md's docker run block against the README's. The one path rule stays stated in full in three places and the v3 data-loss warning in four, because each reader is about to act on it. The changelog gains the two commits since 0.3.0 and becomes 0.4.0; the version pins in ci.md and the three CI examples follow it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
alvagante
changed the title
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.
Four commits, fast-forward from
main. Cuts v0.4.0.Security (
6a8d16b)A review of the tree found three places where the code did not do what its own comment claimed, plus two smaller ones. All five are fixed with regression tests; the first two were verified against live
net/url,net/httpand git behaviour rather than reasoned about.sourcereached the compiler'sfile_contentendpoint as an unescaped path concatenation, sopuppet:///../../pdb/query/v4/catalogs/<node>was attacker-shaped input, compiled from the change under review, steering an mTLS-authenticated GET.net/httpkeepsAuthorizationacross a same-host redirect, so302 Location: http://<same host>leaked the token in cleartext.30read as clean in a CI log.gitwas unvalidated, and--debug-dump-dirwrote through a symlink already sitting at the path.Inference compatibility (
a7a86aa)Anthropic's OpenAI-compatible endpoint documents itself as ignoring
response_formatrather than rejecting it, so a request relying on structured output alone asked for nothing. The task prompt now states the response shape unconditionally, andInterprettolerates a code-fenced reply.Documentation (
e67eca8,4559e20)Azure OpenAI documented as the Microsoft inference path, plus a pass over every doc and package comment for claims that had drifted. The load-bearing correction:
targets-snapshot-baseline.yamltold readers the TLS paths inservices.yamlresolve against the working directory, which stopped being true when 0.3.0 introduced the one path rule. Also a missingimagejob in the CI table, two missing packages in the layout, an incomplete exit-30description, and the danglingrequirements.md/design §/ numbered-spec references throughout.De-duplication kept the copy a reader needs to act on and linked the rest. The one path rule is still stated in full in three places and the v3 data-loss warning in four, deliberately.
Verification
gofmt,go vet,go build,go test -race -count=1 ./...all clean on the tip.Tag
v0.4.0after merge to publish.🤖 Generated with Claude Code