13 KiB
Implementation review remediation
Reviewed 2f95753, including the implementation commits after a1dc705,
against the accepted audit target and completed
implementation plan. 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.
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, lines 9 and 32–39;
pushURLs and run in
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:
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, lines 233–240;
newCommand and environmentEntries in
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: Complete. This addressed an A04 integration gap in the existing preparation workflow, separate from R01's URL-span parsing defect.
Completion: Fetch-failure errors render the remote with DisplayURL, while
the fetch retains its original URL and wrapped Git cause. Public preparation
and CLI regressions failed before the fix and now pass with local HTTP servers;
the public test also verifies authentication with the configured credentials.
The remaining owned Git URL renderers were reviewed, and the CLI guide describes
the implemented diagnostic behavior. Combined verification below passed.
Relevant code: PrepareTaggedCheckout in
internal/git/prepare.go, lines 18–20; the public
preparation facade; and error presentation in
runCIDelivery in 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:
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.
Completed remediation verification
All three findings are complete. Each fix includes regressions that reproduced the defect before the production change, and each unit has its own commit with the corresponding completion marker.
On Linux/amd64 with Go 1.26.8, combined verification passed with
GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off, the prepared build cache, and
TMPDIR=/var/tmp for test fixtures:
go test ./...go test -race ./...go run ./internal/apicheck
Changed Go files passed formatting inspection. Local links and fragments in this document and the CLI guide resolved, and the diff passed whitespace checks.