264 lines
13 KiB
Markdown
264 lines
13 KiB
Markdown
# Remediation: forge boundary and credential preflight review
|
|
|
|
Status: open; the findings below are unaddressed.
|
|
|
|
## Review baseline and scope
|
|
|
|
Reviewed `cbdba90`, including the implementation commits after `9d6952a`,
|
|
against the [feature roadmap](forge-boundary.md),
|
|
[implementation plan](implementation.md), and the user's selected scope.
|
|
This document records work identified by the review; it does not implement it.
|
|
|
|
A follow-up integration audit reviewed `72de187`. Its production source is
|
|
unchanged from `cbdba90`. That audit traced the new boundary and preflight
|
|
through existing CI preparation, module/tag selection, build receipts, token
|
|
loading, source publication, and retry/error handling. It added finding 5 and
|
|
expanded finding 3 with a later authorization-failure reproduction.
|
|
|
|
The intended production boundary is present. The root facade owns neutral
|
|
destination selection and public preflight errors; `internal/forge` supplies
|
|
the backend contract; `internal/delivery` owns page/asset policy and recovery;
|
|
`internal/gitea` owns protocol operations; and `internal/artifact` owns local
|
|
declarations and content verification. Gitea remains the implemented backend.
|
|
Automatic and standalone preflight use read-only repository requests, and binary
|
|
CI checks access before dependency preparation. Source publication retains its
|
|
separate Git authentication and completion semantics.
|
|
|
|
## 1. Complete local destination preflight error handling — P2
|
|
|
|
Status: open.
|
|
|
|
**Evidence:** [Gitea preflight](../../internal/gitea/backend.go),
|
|
[public error conversion](../../delivery.go), and
|
|
[CLI error handling](../../cmd/releasekit/main.go).
|
|
|
|
`Backend.Preflight` returns `destinationAPI` validation errors directly. Those
|
|
errors are neutral `forge.Failure` values with `FailureInvalid`, rather than
|
|
classified preflight failures. `publicPreflightError` consequently passes them
|
|
through. Both CLI delivery paths treat the resulting unclassified error as
|
|
status 1.
|
|
|
|
A temporary consumer with an otherwise valid tagged source and a nonblank
|
|
synthetic token reproduced this with `base_url: "ssh://forge.example"`:
|
|
`PreflightDelivery` returned `*forge.Failure`, `errors.As` could not obtain a
|
|
`*DeliveryPreflightError`, and `deliver` exited 1. A malformed owner/repository
|
|
segment follows the same local validation path. No server request is needed to
|
|
identify these configuration problems.
|
|
|
|
This leaves the public preflight error description incomplete and conflicts
|
|
with the documented status 2 for profile/configuration failures. Operators
|
|
cannot reliably distinguish a locally invalid destination from an operational
|
|
verification failure through the new boundary.
|
|
|
|
**Required work:**
|
|
|
|
- Make deterministic destination-setting failures recognizable through the
|
|
facade and classify them as CLI configuration failures with status 2 in both
|
|
ordinary and CI delivery. Keep backend-specific validation in its adapter;
|
|
share its classification instead of duplicating Gitea rules in the CLI.
|
|
- Make GoDoc and the selected public error representation agree about local
|
|
configuration failures. Preserve useful causes and identify the invalid
|
|
setting; avoid labeling a malformed URL as a rejected token or an unavailable
|
|
service. Keep real service/transport failures at status 1.
|
|
- Add focused coverage for an invalid URL and invalid repository addressing,
|
|
including standalone preflight and the CLI mapping. Verify early failure,
|
|
`not_attempted` delivery state, and absence of hosted requests/build work.
|
|
|
|
This correction concerns existing destination rules and failure classification;
|
|
it adds no token-format restriction, remote permission requirement, or probe.
|
|
|
|
## 2. Protect the runtime boundary from the new forge contract — P2
|
|
|
|
Status: open.
|
|
|
|
**Evidence:** [dependency checker](../../internal/apicheck/main.go), especially
|
|
`forbiddenRuntimeDependency`, and the runtime boundary in
|
|
[architecture policy](../policy/architecture.md#runtime-version-boundary).
|
|
Implementation Stage 9 also calls for keeping the new delivery packages out of
|
|
the runtime dependency graph through the existing checking mechanism.
|
|
|
|
The checker excludes named delivery implementations such as `gitea` and
|
|
`delivery`, but its runtime exclusion list omits the newly added `forge`
|
|
package. A temporary test overlay supplied this package graph:
|
|
|
|
```text
|
|
buildinfo -> internal/forge
|
|
```
|
|
|
|
`checkImportDirections` returned nil. The graph would make the runtime facade
|
|
depend on the release backend contract while the advertised boundary check
|
|
continued to pass. The actual current `buildinfo` graph remains appropriately
|
|
small; the gap is in the planned regression protection.
|
|
|
|
**Required work:**
|
|
|
|
- Extend the existing runtime check to exclude the new forge delivery contract.
|
|
Retain the existing policy for legitimate runtime dependencies.
|
|
- Add a focused fixture showing rejection of a runtime dependency on
|
|
`internal/forge`, including traversal through a small intermediary where
|
|
needed to protect the existing transitive rule. Retain the passing actual
|
|
repository check.
|
|
|
|
This enforces the already selected separation from orchestration. It does not
|
|
require a new public package restriction or a broader dependency policy.
|
|
|
|
## 3. Qualify phase-specific CLI exit-status documentation — P3
|
|
|
|
Status: open.
|
|
|
|
**Evidence:** [CLI exit statuses](../cli.md),
|
|
[v0.6.0 release note](../releases/v0.6.0.md),
|
|
[write evidence](../../internal/gitea/backend.go),
|
|
[asset reconciliation](../../internal/delivery/remote_assets.go), and
|
|
[CLI delivery mapping](../../cmd/releasekit/main.go).
|
|
|
|
The new CLI prose and release note broadly state that cancellation returns
|
|
status 1. That is correct for preflight and preparation cancellation. After a
|
|
possibly successful write, cancellation can prevent reconciliation and leave
|
|
delivery unknown; the CLI correctly returns status 3 in that case.
|
|
|
|
A temporary shared-delivery test supplied an attempted uncertain upload with
|
|
`context.Canceled` and an unavailable canceled reconciliation. It returned
|
|
`State == Unknown`, retained the cancellation cause, and placed the asset in
|
|
`Uncertain`. The CLI's unknown-state branch maps that result to 3.
|
|
|
|
The broad documentation introduces a stronger cancellation guarantee than the
|
|
requested preflight behavior and conflicts with preserved write recovery.
|
|
|
|
The same integration issue affects the unqualified statement that credential
|
|
failures return status 2. With valid tagged source, a synthetic 401 on the first
|
|
repository preflight read returned status 2. When both repository preflight
|
|
reads succeeded and the hosted-tag read then returned 401, ordinary delivery
|
|
returned status 1. Later provider failures retain the existing delivery error
|
|
path and are not `DeliveryPreflightError` values. Successful preflight cannot
|
|
guarantee that authorization will remain valid during later operations.
|
|
|
|
**Required work:**
|
|
|
|
- Scope status 1 wording to preflight/preparation cancellation and cancellation
|
|
that leaves no unresolved write. State that an unresolved write retains
|
|
status 3, including when cancellation prevented verification.
|
|
- Align the CLI reference and unreleased v0.6.0 note. Keep implementation
|
|
behavior and unknown-write recovery intact.
|
|
- Qualify credential/access status 2 as applying to local credential loading
|
|
and classified preflight failures. Describe later authorization failures
|
|
through the existing delivery outcomes: known failure can return 1, while an
|
|
unresolved write retains 3. Do not reclassify later failures as preflight
|
|
failures or erase partial progress to satisfy overbroad prose.
|
|
- Reuse existing cancellation/recovery tests. If CLI coverage does not already
|
|
distinguish these cases, add a narrow case protecting unknown-write status
|
|
after cancellation; avoid an expanded cancellation matrix.
|
|
|
|
## 4. Align roadmap completion records — P3
|
|
|
|
Status: open.
|
|
|
|
**Evidence:** [feature roadmap](forge-boundary.md) and
|
|
[implementation plan](implementation.md).
|
|
|
|
The implementation plan marks all ten stages complete, while the feature
|
|
roadmap still says implementation has not started. The plan's introductory
|
|
baseline text also uses present-tense descriptions of the old implementation
|
|
and says the neutral destination and preflight are planned.
|
|
|
|
**Required work:** Mark the feature roadmap's implementation status accurately
|
|
and clearly label the implementation plan's original baseline as historical.
|
|
Keep these records consistent with the completed stages and remaining
|
|
remediation. Roadmap retirement can follow the existing release-preparation
|
|
cleanup; this finding does not require removing plans now.
|
|
|
|
## 5. Bind CI release identity before online preflight and preparation — P2
|
|
|
|
Status: open.
|
|
|
|
**Evidence:** [CI adapter](../../cmd/releasekit/main.go), especially
|
|
`runCIDelivery` and `checkExpectedModule`, and the existing identity/tag checks
|
|
in [shared delivery](../../internal/delivery/delivery.go).
|
|
|
|
Binary `ci-deliver` invokes standalone credential preflight and application
|
|
dependency preparation before `Deliver` derives the selected module's tag and
|
|
checks it against the CI event tag. `PrepareTaggedCheckout` proves that the
|
|
supplied Git tag and commit agree; it does not prove that this is the release
|
|
tag derived from the profile's module. The optional expected-module guard
|
|
compares selected directories and also does not establish that relationship.
|
|
|
|
A temporary CLI fixture had a clean tagged checkout at `v1.2.3`, a profile
|
|
selecting committed module `child`, and valid repository credentials. The
|
|
selected module's derived tag was `child/v1.2.3`. With `--prepared` and
|
|
`--expected-module child`, both profile variants eventually returned the same
|
|
tag-mismatch error, but their earlier effects differed:
|
|
|
|
| Profile | Forge requests | Dependency preparation |
|
|
| --- | --- | --- |
|
|
| Page-only | None | Skipped |
|
|
| Binary | Two credential-preflight reads | Invoked before the mismatch was reported |
|
|
|
|
Neither variant made a hosted release write; the existing late guard still
|
|
prevents delivery under the wrong module tag.
|
|
|
|
The dependency command was intercepted by a temporary local Go wrapper;
|
|
no application dependencies were fetched for the reproduction. A real binary
|
|
job can perform avoidable downloads or let a preparation failure obscure the
|
|
already determinable identity mismatch. A preflight credential failure can
|
|
likewise hide the source-selection error.
|
|
|
|
Dependency preparation already preceded this guard before the feature work.
|
|
The new standalone preflight adds hosted credential requests to that existing
|
|
ordering gap. Page-only delivery continues to reach the shared identity check
|
|
before any hosted requests, so the integration now has inconsistent early
|
|
validation behavior across the two supported CI paths.
|
|
|
|
**Required work:**
|
|
|
|
- After verifying the event checkout and loading its profile, derive the
|
|
committed selected-module release identity and apply the existing full-tag
|
|
comparison before the first forge probe or application dependency download.
|
|
Reuse the existing identity derivation and keep the optional launcher-module
|
|
comparison's responsibility separate.
|
|
- Preserve credential preflight before expensive dependency/build work, with
|
|
normal delivery verification after preparation. This moves an existing
|
|
acceptance check earlier; it adds no module map, source-publication gate,
|
|
candidate test suite, or extra requirement to standalone `PreflightDelivery`.
|
|
- Add a focused CI regression for a valid Git tag/commit and matching
|
|
`--expected-module` whose derived module tag differs. Cover page-only and
|
|
binary profiles, asserting the identity diagnostic and absence of forge
|
|
requests or dependency preparation. Retain the valid binary path's early
|
|
credential check and subsequent delivery behavior.
|
|
|
|
## Verification performed
|
|
|
|
Using Go 1.26.10 on linux/amd64 with module/toolchain downloads disabled:
|
|
|
|
- `go test ./...` passed.
|
|
- `go run ./internal/apicheck` passed for the current repository.
|
|
- `go test -race ./internal/delivery ./internal/gitea` passed.
|
|
- Temporary review fixtures reproduced findings 1 and 2, and confirmed the
|
|
cancellation outcome underlying finding 3. The runtime-boundary reproduction
|
|
intentionally failed its assertion because the prohibited graph was accepted.
|
|
- An additional temporary HTTP/1.1 and HTTP/2 check confirmed that a large upload
|
|
rejected before its body was consumed returns promptly as a known rejection;
|
|
that investigated path produced no finding.
|
|
|
|
Temporary fixtures and test overlays were kept outside the checkout. No
|
|
implementation files or permanent tests were changed during this review.
|
|
|
|
### Follow-up integration audit
|
|
|
|
The same Go 1.26.10 toolchain and offline environment were used. Existing full
|
|
suite, API-boundary, and focused race results above remain applicable because
|
|
the production source was unchanged. Additional checks comprised:
|
|
|
|
- A fresh CLI integration run covering tagged CI preparation, managed/prebuilt
|
|
delivery and stale receipts, module-relative output, multi-module dispatch,
|
|
Woodpecker workflows, and source-tool bootstrap; all selected tests passed.
|
|
- Focused token-file precedence, repository-root token-file loading,
|
|
forbidden-file validation, and check/publication integration tests passed.
|
|
- Temporary CLI fixtures demonstrating finding 5 and the initial-versus-later
|
|
401 exit-status difference in finding 3, using a local HTTP server and an
|
|
intercepted dependency command.
|
|
|
|
The build, receipt, publication, candidate-validation, dependency-preparation,
|
|
and Go-tool implementation owners were also compared with the pre-feature
|
|
baseline. Their production code is unchanged; the confirmed new interaction
|
|
occurs in the CI adapter's composition and the phase-specific error contract.
|