Skip to content

fix: harden storage coordination and browser auth - #8

Open
pageman wants to merge 1 commit into
tobi:mainfrom
pageman:fix/audit-critical-correctness
Open

fix: harden storage coordination and browser auth#8
pageman wants to merge 1 commit into
tobi:mainfrom
pageman:fix/audit-critical-correctness

Conversation

@pageman

@pageman pageman commented Aug 25, 2026

Copy link
Copy Markdown

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 unconditional DELETE. A lease owner that was delayed between those operations could delete a replacement lease. The repaired path sends If-Match: <ETag> on DeleteObject and maps S3 precondition failures to the store’s PreconditionFailed error. The correctness decision is now made by S3 at the mutation point.

The compose path no longer performs a preflight HEAD for create-if-absent. Multipart completion carries If-None-Match: * for create and If-Match for 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 Version value could otherwise reach a path that omitted the GCS generation precondition. The write-option helper now returns a typed InvalidArgument error 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.html is absent instead of silently embedding a development placeholder. The server crate also declares tempfile as 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 NodelayListener now supplies a local Axum connect-info wrapper, preserving remote peer-address extraction while satisfying Axum 0.8’s Connected trait and orphan rules.

9. Required validation

A new GitHub Actions workflow runs on pushes to main and 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

Area Files Purpose
S3 storage crates/walgit-store/src/s3.rs Atomic conditional delete, compose, and multipart completion semantics
GCS storage crates/walgit-store/src/gcs.rs Fail-closed generation parsing and error propagation
LFS crates/walgit-server/src/lfs.rs Separate read and write authorization for batch operations
Git protocol crates/walgit-git/src/lib.rs Remove unsupported deepen capability advertisement and add regression test
Readiness crates/walgit-server/src/prewarm.rs, src/health.rs, tests/sim.rs Fail-closed startup readiness
Browser auth crates/walgit-server/src/web/v1.rs, web/sdk/repos.ts Validated target origin for popup messages
Build/runtime crates/walgit-server/build.rs, src/lib.rs, Cargo.toml Release asset guard, Axum connect info, production tempfile dependency
Documentation web/API.md, web/sdk/README.md Document repaired popup-origin contract
CI .github/workflows/ci.yml Required formatting, web build, tests, and release compilation

Correctness invariants

  1. A conditional delete cannot remove an object whose version differs from the caller’s observed version.
  2. A create-if-absent operation is decided atomically by the backend at the mutation point.
  3. A malformed conditional version never turns into an unconditional write.
  4. Read-only LFS authorization cannot expose upload actions.
  5. A server never advertises an unsupported Git capability.
  6. An instance is ready only after all required prewarm work succeeds.
  7. An authentication popup message is delivered only to a configured opener origin.
  8. Release builds never embed a development placeholder UI.
  9. Peer-address extraction remains available for the custom HTTP listener.

Validation performed

The following checks passed locally:

cargo fmt --all -- --check
cargo check --workspace --lib --bins --all-features
cargo test -p walgit-store --lib
cargo test -p walgit-git --lib
cargo test -p walgit-server --lib web::v1::tests::popup_origin_must_match_configured_cors_origin
cargo test -p walgit-server --test sim readiness
cd web && pnpm install --frozen-lockfile && pnpm run build

Observed successful test counts:

Check Result
walgit-store unit tests 44 passed
walgit-git unit tests 7 passed
Popup-origin regression 1 passed
Readiness simulation 1 passed
Workspace library/binary check passed
Web production + SDK build passed

The full walgit-server library 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 broader walgit-git code. 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-Match and 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

  • Confirm S3 SDK methods map to If-Match and If-None-Match headers for the locked AWS SDK version.
  • Run the S3 contract suite against AWS S3 and at least one S3-compatible provider.
  • Run the GCS contract suite with concurrent stale-owner lease release.
  • Verify an unauthenticated browser popup preserves the origin through the actual OIDC login round trip.
  • Verify LFS read-only and write principals receive distinct batch actions.
  • Verify stock Git clients no longer see unsupported deepen capabilities.
  • Verify a failed prewarm produces /readyz 503 and a useful diagnostic body.
  • Confirm CI has access to the repository’s required Git version and web build dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant