fix: harden storage coordination and browser auth - #8
Open
pageman wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit-driven correctness and production-hardening repairs
Summary
This pull request implements the highest-risk repairs identified by the walgit architecture and code audit. It focuses on preserving the repository’s advertised invariants across real storage backends and real deployment states, rather than adding more surface area.
The patch makes S3 conditional operations use native request preconditions, makes malformed GCS generations fail closed, requires write authorization for LFS upload discovery, stops advertising unsupported Git capabilities, prevents cold or failed-prewarm instances from becoming ready, binds browser authentication-popup messages to an allowlisted origin, makes release builds fail when the real UI is absent, fixes production dependency resolution for LFS temporary files, restores Axum peer-address connect information for the custom listener, and adds required CI coverage.
Problems addressed
1. S3 stale-owner delete race
The prior S3 delete path implemented conditional deletion as a
HEAD, local version comparison, and unconditionalDELETE. A lease owner that was delayed between those operations could delete a replacement lease. The repaired path sendsIf-Match: <ETag>onDeleteObjectand maps S3 precondition failures to the store’sPreconditionFailederror. The correctness decision is now made by S3 at the mutation point.The compose path no longer performs a preflight
HEADfor create-if-absent. Multipart completion carriesIf-None-Match: *for create andIf-Matchfor update, so the destination precondition is enforced at completion rather than through a check-then-act sequence. Failed conditional completion aborts the multipart upload before returning the mapped precondition error.2. GCS malformed-version fail-open behavior
A malformed
Versionvalue could otherwise reach a path that omitted the GCS generation precondition. The write-option helper now returns a typedInvalidArgumenterror when a conditional generation cannot be parsed. The helper’s callers propagate the error, eliminating the panic-based defensive fallback and ensuring future direct callers also fail closed.3. LFS authorization separation
The LFS batch endpoint now requires write authorization when the requested batch operation includes upload discovery. Read-only clients may still obtain download actions, but cannot use the upload branch to discover or initiate write operations.
4. Git protocol truthfulness
The server no longer advertises unsupported deepen modes. Capability advertisements must remain a promise of implemented behavior; removing unsupported capabilities prevents clients from selecting protocol features that the server cannot execute correctly.
5. Readiness correctness
Readiness now means that all configured prewarm work completed successfully. A timeout no longer promotes a cold instance to ready, and a failed prewarm leaves the instance unready with diagnostic failure information. The readiness API no longer accepts an obsolete timeout parameter. The simulation test now asserts the fail-closed behavior.
6. Browser authentication-popup origin binding
The authentication landing page no longer posts its success message to
"*". The SDK includes the caller’s page origin in the popup URL. The server accepts that origin only if it matches the configured credentialed CORS allowlist; otherwise it falls back to the walgit host origin. The public API and SDK documentation now describe this contract. Focused tests cover exact origins, configured wildcard subdomains, unconfigured origins, and path-bearing invalid values.7. Release and build correctness
Release builds now fail when
web/dist/index.htmlis absent instead of silently embedding a development placeholder. The server crate also declarestempfileas a normal dependency because production LFS code uses it. A missing dependency previously prevented a clean library/binary build.8. Server build compatibility
The custom
NodelayListenernow supplies a local Axum connect-info wrapper, preserving remote peer-address extraction while satisfying Axum 0.8’sConnectedtrait and orphan rules.9. Required validation
A new GitHub Actions workflow runs on pushes to
mainand pull requests. It installs the pinned Rust toolchain, builds the real web assets, checks formatting, runs workspace tests, and compiles release artifacts. Clippy is intentionally not a required gate in this PR because the current repository contains unrelated baseline Clippy failures in untouched modules; those should be addressed in a separate lint-cleanup change rather than hidden inside a correctness patch.Files changed
crates/walgit-store/src/s3.rscrates/walgit-store/src/gcs.rscrates/walgit-server/src/lfs.rscrates/walgit-git/src/lib.rscrates/walgit-server/src/prewarm.rs,src/health.rs,tests/sim.rscrates/walgit-server/src/web/v1.rs,web/sdk/repos.tscrates/walgit-server/build.rs,src/lib.rs,Cargo.tomlweb/API.md,web/sdk/README.md.github/workflows/ci.ymlCorrectness invariants
Validation performed
The following checks passed locally:
Observed successful test counts:
walgit-storeunit testswalgit-gitunit testsThe full
walgit-serverlibrary test run reaches the test phase but has three existing environment-dependent installer failures because the sandbox Git version is 2.43.0 while the installer contract requires Git >= 2.46 for credential-authtype and bundle-URI support. Those failures are unrelated to this patch. The combined workspace test command was also initially interrupted while compiling the large Google Cloud dependency graph; the targeted packages and affected tests were subsequently run successfully.Clippy was run with warnings denied. It reported pre-existing warnings in untouched modules, including
walgit-config,walgit-store/fault.rs,walgit-store/util.rs, and broaderwalgit-gitcode. The new S3 warnings were corrected. Clippy is therefore not included as a required CI gate in this PR.Backend rollout requirements
Before enabling S3 leases in production, the deployment must use an S3 endpoint that supports conditional object deletion with
If-Matchand must pass the real S3 contract suite. S3-compatible providers should be tested independently; compatibility with the S3 API does not automatically prove support for every newer conditional operation.Before enabling cross-origin browser authentication, operators must list every allowed embedding origin in
server.cors_origins. The SDK’s supplied origin is not trusted by itself; it is only accepted when it matches that allowlist.The repaired readiness behavior is intentionally fail-closed. Deployments must provide prewarm configuration that can complete successfully, or they will remain out of service until the operator resolves the reported failures.
Follow-up work intentionally not included
This PR does not attempt to redesign multi-pack compaction publication, add a full fault-injection object-store emulator, implement all 50 stress tests from the audit catalogue, or perform a broad Clippy cleanup. Those are substantial follow-up changes and should be landed as separate reviewable PRs after these foundational invariants are in place.
Reviewer checklist
If-MatchandIf-None-Matchheaders for the locked AWS SDK version./readyz503 and a useful diagnostic body.