diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 32ba4af..c430674 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -2,8 +2,10 @@ 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. +[implementation plan](implementation.md). A follow-up integration audit reviewed +`c8fa165`, whose production code is identical, and added R02 and R03. Findings +below identify remaining work; production code and existing tests were unchanged +during both reviews. ## R01. Git diagnostic redaction misses apostrophes in credentials @@ -51,6 +53,86 @@ 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:** Pending. This concerns the interaction of A02/A12 +with the existing process environment helper and native Git hooks. + +**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, @@ -61,13 +143,15 @@ 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. +R01–R03 are the remaining findings 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. -On Linux/amd64 with Go 1.26.8, the following checks passed with +During the initial review on Linux/amd64 with Go 1.26.8, these checks passed with `GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off`: - `go test ./...` @@ -81,3 +165,29 @@ 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 current implementation; R02's comparison +using the earlier Git environment behavior passed. The checkout contains no +probe code or production changes. The previously passing full-suite, static, +and compiled API/dependency checks apply to the unchanged production sources.