Prepare v0.7.0 notes and retire completed roadmaps
This commit is contained in:
@@ -1,231 +0,0 @@
|
||||
# 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:** 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](../../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.
|
||||
|
||||
## 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.
|
||||
Reference in New Issue
Block a user