208 lines
12 KiB
Markdown
208 lines
12 KiB
Markdown
# Implementation review remediation
|
||
|
||
Reviewed `2f95753`, including the implementation commits after `a1dc705`,
|
||
against the accepted [audit target](audit.md) and completed
|
||
[implementation plan](implementation.md). A follow-up integration audit reviewed
|
||
`c8fa165`, whose production code is identical, and added R02 and R03. Findings
|
||
and completion records below track the remediation. Production code and existing
|
||
tests were unchanged during both reviews.
|
||
|
||
## R01. Git diagnostic redaction misses apostrophes in credentials
|
||
|
||
**Priority:** P2. **Status:** Complete. This addressed a gap in A04 and
|
||
[Stage 6](implementation.md#stage-6-sanitize-git-endpoint-diagnostics).
|
||
|
||
**Completion:** Diagnostic matching recognizes complete userinfo before
|
||
surrounding quotation, preserving error context and wrapped causes. Native Git,
|
||
literal/encoded apostrophe, malformed userinfo, and wrapped-error regressions
|
||
pass; the new regressions failed before the fix. The complete `internal/git`
|
||
suite and public/CLI endpoint-mismatch redaction tests passed offline.
|
||
|
||
**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.
|
||
|
||
## R02. Original-object Git overlays retain the caller's PWD
|
||
|
||
**Priority:** P2. **Status:** Complete. This addressed the interaction of A02/A12
|
||
with the existing process environment helper and native Git hooks.
|
||
|
||
**Completion:** Child overlays now start from `Cmd.Environ()` after setting
|
||
`Dir`. Directory-derived `PWD`, explicit overrides/removals, and parent-state
|
||
preservation have process regressions; a native Go pre-push hook verifies public
|
||
publication from another directory. Both new regressions failed before the fix
|
||
and pass afterward. Process, Go-tool, and Git suites passed under `-race`;
|
||
public hook and original-object source/build eligibility regressions passed.
|
||
|
||
**Relevant code:** `run` in
|
||
[internal/git/remote.go](../../internal/git/remote.go), lines 233–240;
|
||
`newCommand` and `environmentEntries` in
|
||
[internal/process/process.go](../../internal/process/process.go), lines 146–184.
|
||
|
||
**Circumstances and actual behavior:** A caller invokes `Publish` for a checkout
|
||
other than its current directory, or invokes the CLI from a selected module
|
||
subdirectory. The new `GIT_NO_REPLACE_OBJECTS` overlay makes Git's command
|
||
environment explicit. The existing environment helper copies `os.Environ()`
|
||
before applying that overlay, retaining the parent process's `PWD`. The local
|
||
pre-push hook executes in the selected repository, while `PWD` still names the
|
||
caller's directory. Previously, this Git runner left `Cmd.Env` nil, allowing
|
||
Go to derive the child `PWD` from `Cmd.Dir` on POSIX systems.
|
||
|
||
An uncached public `Publish` probe used a real local bare remote and a Python
|
||
pre-push hook that compares `PWD` with its actual working directory. The hook
|
||
rejected the push, leaving publication `not_published` and the local tag retained.
|
||
An otherwise identical temporary comparison overlay, removing only the Git
|
||
runner's newly added environment assignment, published successfully. The
|
||
comparison isolates environment construction as the regression's cause.
|
||
|
||
**Expected behavior and impact:** Selecting another checkout preserves the
|
||
existing working-directory behavior of Git collaborators while adding the
|
||
original-object setting. Hooks that locate repository files through `PWD` can
|
||
now inspect the wrong checkout or reject a legitimate publication. The observed
|
||
failure is attributable to the new Git overlay exposing the older environment
|
||
helper's directory handling.
|
||
|
||
**Recommended remediation:** Build environment overlays from `command.Environ()`
|
||
after assigning `command.Dir`, then apply the requested removals and overrides.
|
||
This retains Go's directory-aware inherited environment and still honors an
|
||
explicit `PWD` override or removal. Keep `GIT_NO_REPLACE_OBJECTS=1` and the
|
||
existing local-object transport controls. Preserve process-global state. Add
|
||
focused process coverage for `Dir` combined with an unrelated overlay and a
|
||
public publication regression using a local hook from another caller directory.
|
||
|
||
## R03. Checkout preparation reintroduces credentials after Git redaction
|
||
|
||
**Priority:** P2. **Status:** Pending. This is an A04 integration gap in the
|
||
existing preparation workflow, separate from R01's URL-span parsing defect.
|
||
|
||
**Relevant code:** `PrepareTaggedCheckout` in
|
||
[internal/git/prepare.go](../../internal/git/prepare.go), lines 18–20; the public
|
||
[preparation facade](../../preparation.go); and error presentation in
|
||
`runCIDelivery` in [cmd/releasekit/main.go](../../cmd/releasekit/main.go).
|
||
|
||
**Circumstances and actual behavior:** `TaggedCheckoutRequest.Remote` supplies
|
||
an HTTP(S) repository URL with userinfo, and fetching its tag fails. The shared
|
||
Git runner sanitizes its captured diagnostic, but `PrepareTaggedCheckout`
|
||
formats the original `remote` directly into its outer error. The facade returns
|
||
that error unchanged; `ci-deliver` prints it directly.
|
||
|
||
An uncached public preparation probe used placeholder credentials and a local
|
||
HTTP server returning 503. The resulting error began:
|
||
|
||
```text
|
||
fetch tag v1.2.3 from remote http://review-user:review-password@127.0.0.1:PORT/repo.git: run "git": exit status 128
|
||
```
|
||
|
||
Git's own diagnostic in that reproduction omitted userinfo. The enclosing
|
||
ReleaseKit error exposed it. Ordinary username/password credentials reproduce
|
||
this issue.
|
||
|
||
**Expected behavior and impact:** The preparation workflow uses the same safe
|
||
URL rendering as publication. As implemented, failed CI preparation or direct
|
||
facade use can expose credentials to terminal output, callers, and logs despite
|
||
the shared runner's new redaction.
|
||
|
||
**Recommended remediation:** Render the remote through `DisplayURL` in the
|
||
preparation error and check the remaining owned URL-formatting boundaries for
|
||
the same bypass. Preserve the actual fetch URL and wrapped Git cause. Add a
|
||
public preparation failure regression with a local server and placeholder
|
||
credentials, and cover the CLI error path through its existing presentation
|
||
tests. Credential-bearing transport URLs keep their current supported behavior.
|
||
|
||
## 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.
|
||
|
||
R01–R03 record findings and remediation status from the initial review and
|
||
follow-up integration audit. Package ownership and public facade shape match
|
||
the target; the findings concern diagnostic and subprocess-environment behavior
|
||
across existing boundaries. The review identified 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.
|
||
|
||
During the initial review on Linux/amd64 with Go 1.26.8, these 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.
|
||
|
||
### Follow-up integration evidence
|
||
|
||
The follow-up audit traced original-object observations through candidate,
|
||
build, checkout preparation, and publication; subprocess cancellation through
|
||
batch readers and committed-tree validation; completed build facts through CLI
|
||
receipts and prebuilt delivery; cleanup errors through publication/delivery
|
||
outcomes and CLI statuses; and redirect/content limits through read-only asset
|
||
inspection and attempted-write reconciliation. It also checked status-only
|
||
output capture against full-output consumers and existing forwarding errors.
|
||
|
||
Focused committed tests ran uncached with the same offline Go settings. Public
|
||
and CLI cases covered exact publication, rejected/lost-acknowledgment recovery,
|
||
partial tag evidence, replacement-derived build eligibility, original-note
|
||
delivery, managed build cleanup/cancellation, receipt-to-prebuilt delivery,
|
||
CI preflight ordering, and completed-delivery cleanup reporting. Selected tests
|
||
in `internal/process`, `internal/git`, `internal/gotool`, `internal/delivery`,
|
||
and `internal/validation` passed under `-race`, including queued cancellation,
|
||
inherited-pipe cancellation, forwarding errors, joined cleanup outcomes,
|
||
bounded asset reads, upload reconciliation, and tree-reader cleanup precedence.
|
||
|
||
R02 and R03 were reproduced independently with temporary Go test overlays.
|
||
Their assertions failed against the reviewed implementation; R02's comparison
|
||
using the earlier Git environment behavior passed. Both audits left production
|
||
code and existing tests unchanged. The full-suite, static, and compiled
|
||
API/dependency results above record the audit baseline; completion notes record
|
||
verification of the subsequent repairs.
|