Record the implementation review finding
This commit is contained in:
83
docs/roadmap/remediation.md
Normal file
83
docs/roadmap/remediation.md
Normal file
@@ -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.
|
||||||
Reference in New Issue
Block a user