diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..32ba4af --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,83 @@ +# Implementation review remediation + +Reviewed `2f95753`, including the implementation commits after `a1dc705`, +against the accepted [audit target](audit.md) and completed +[implementation plan](implementation.md). Findings below identify remaining +work; production code and existing tests were unchanged during this review. + +## R01. Git diagnostic redaction misses apostrophes in credentials + +**Priority:** P2. **Status:** Pending. This is a remaining gap in A04 and +[Stage 6](implementation.md#stage-6-sanitize-git-endpoint-diagnostics). + +**Relevant code:** `httpURLSpan` and `displayDiagnostic` in +[internal/git/display.go](../../internal/git/display.go), lines 9 and 32–39; +`pushURLs` and `run` in +[internal/git/remote.go](../../internal/git/remote.go), lines 209–252. + +**Circumstances and actual behavior:** The diagnostic URL matcher treats every +apostrophe as the end of a URL. An apostrophe is also accepted within URL +userinfo. For `https://user:secret'part@example.test/repo`, the matcher passes +only `https://user:secret` to `DisplayURL`. That truncated span has no `@`, so +it is returned unchanged, leaving the entire credential in the diagnostic. +Direct rendering of the complete URL correctly masks it; sanitizing surrounding +Git text takes the defective path. + +A local reproduction used a real temporary Git repository and passed the URL +above as `Resolve`'s remote name, representing a URL mistakenly configured in +the remote-name field. Native `git remote get-url --push --all` rejects that +name and includes it in its error. The returned ReleaseKit error was: + +```text +resolve push URL for remote "https://redacted@example.test/repo": run "git": exit status 2: error: No such remote 'https://user:secret'part@example.test/repo' +``` + +The values above are placeholders. This reproduction makes no network request. +A separate controlled Git failure reproduced the same leak through `PushTag`. +The existing tests cover simple and percent-encoded credentials but omit this +literal punctuation case. + +**Expected behavior and impact:** Owned diagnostics mask HTTP(S) userinfo while +preserving the underlying Git error identity. The observed failure can expose +a password to callers, terminal output, or logs even though the outer endpoint +field is redacted. The accepted credential-confidentiality repair is incomplete. + +**Recommended remediation:** Recognize the authority and userinfo boundaries +before treating surrounding quotation as a URL delimiter. Sanitize the complete +credential span while retaining useful host/path and surrounding error context. +Keep malformed userinfo handling and `displayError.Unwrap`'s cause preservation. +Add a regression using native Git's local remote-name rejection, plus coverage +for the wrapped-error rendering path and literal versus encoded apostrophes. +Preserve raw transport URLs, exact comparisons, and the raw public endpoint +result. This repair requires no new credential restrictions or public API. + +## Review coverage and verification + +The review covered all A01–A20 outcomes and the changed production code, tests, +and current documentation. The implementation retains the root orchestration +facade and lightweight `buildinfo` facade. Internal Git/process operations own +source observations and subprocess lifetime; validation owns its Go and tree +sequences; build settings have one internal representation; artifact declaration +matching is shared; neutral delivery policy remains separate from Gitea protocol +operations; and private CLI handlers own presentation. + +Apart from R01, the review identified no additional code-quality defects or +target-architecture gaps requiring remediation, and no inadvertently strengthened +public contract. The selected redirect and diagnostic limits match the accepted +scope. JSON response caps and production exclusion indexing remain explicitly +deferred with assessment evidence, as planned. + +On Linux/amd64 with Go 1.26.8, the following checks passed with +`GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off`: + +- `go test ./...` +- `go test -race ./...` +- `go vet ./...` +- `go run ./internal/apicheck` + +Some test results were reused from Go's cache. The suite used `/var/tmp` for +temporary fixtures because an environment-provided `/tmp/.git` marker affects +standalone repository discovery. The R01 probes ran uncached through a temporary +Go test overlay and failed their credential-absence assertions on the reviewed +implementation; no probe test was installed in the checkout. Those failures +substantiate R01 and are separate from the passing committed suite.