Conversation
Signed-off-by: Tom Brow <tomb@block.xyz>
🔐 Codex Security Review
|
|
@codex review |
|
@builderbot review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@buzz-security-review 360a8e7 |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
brow
changed the title
brow
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 360a8e74b4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut v = verifier(&dev.app_id, dev.root_cert_pem.as_bytes()); | ||
| v.environment = AppAttestEnvironment::Development; |
There was a problem hiding this comment.
The development-mode test constructs the production-only test helper and then mutates its private environment field, so it never exercises AppAttestVerifier::with_environment or the Config → main → verifier wiring. For example, changing main.rs back to AppAttestVerifier::new would make every configured development gateway reject enrollment while this test remains green; construct the verifier through the production API, ideally from parsed configuration, so the regression is falsifiable.
AGENTS.md reference: AGENTS.md:L188-L192
Useful? React with 👍 / 👎.
| [features] | ||
| # Personal device development only. Production builds remain production-only. | ||
| personal-dev-app-attest = ["appattest/testing"] |
There was a problem hiding this comment.
All standard gateway test lanes invoke the crate without features (scripts/run-tests.sh:118-119 and Justfile:399-401), while the Docker workflow also builds only the default image. Consequently the new Development variant, its appattest/testing dependency graph, and every #[cfg(feature = "personal-dev-app-attest")] test are skipped in CI, allowing the documented personal-development image to stop compiling or validating attestations without failing a gate; add a feature-enabled check/test lane.
AGENTS.md reference: AGENTS.md:L188-L192
Useful? React with 👍 / 👎.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES on exact head 360a8e74b49e3c7969fddb9e41f033753f9a4557 (base/merge-base 8953cbfff58ed768d996677fed3af0e3bac64a20).
Author-actionable defect
P1 — the opt-in feature’s production build/runtime seam is not regression-gated.
The normal gateway test paths run without personal-dev-app-attest (Justfile:401, scripts/run-tests.sh:118-119), and the public-image workflow builds only the default image without BUZZ_PUSH_CARGO_FEATURES (.github/workflows/docker.yml:495-557). CI therefore never compiles or executes the new Development variant, its appattest/testing dependency closure, or its feature-gated tests.
The only development-mode behavior test also bypasses the production wiring: it constructs a private production helper and mutates environment directly (crates/buzz-push-gateway/src/app_attest.rs:369-430). Replacing the production call at crates/buzz-push-gateway/src/main.rs:46-50 with AppAttestVerifier::new left the complete feature-enabled package suite green (51/51 non-ignored tests). A future break in config → constructor wiring—the seam that makes this PR work—would therefore ship green. This violates the repository’s explicit falsifiability requirement in TESTING.md and AGENTS.md.
Author action:
- Add a CI lane that runs the full gateway package with
--features personal-dev-app-attest; preferably also build the feature-enabled Docker target so the documented image path is gated. - Add a production-shaped regression seam that exercises parsed configuration through the environment-aware constructor/wiring and fails when the
main.rscall is reverted to the production-only constructor. Testing the private helper again is insufficient.
Verification owner: the author should provide failing-before/passing-after or equivalent mutation proof; the next review round will rerun the new exact-head feature lane and repeat the call-site-bypass mutation.
What was verified
No additional trust-chain defect was found. The implementation selects exactly one signed AAGUID before the dependency verifier (app_attest.rs:137-169); ordinary builds cannot represent Development and reject that config (config.rs:21-29,158-166); the special build still defaults to production. APNs environment selection remains server-owned and maps sandbox/production to the corresponding Apple origin (config.rs:169-179, apns.rs:94-99). Documentation correctly scopes this to personal development and does not introduce a UI contract.
On clean exact head with the pinned Hermit toolchain:
cargo test -p buzz-push-gateway: 50 passed, 11 ignored.cargo test -p buzz-push-gateway --features personal-dev-app-attest: 51 passed, 11 ignored.- Clippy
--all-targets -D warnings, default and feature: PASS. - Feature-enabled release binary build: PASS.
cargo fmt --all --checkandgit diff --check: PASS.- Environment-fence mutation testing correctly failed when the signed-AAGUID check was removed; the separate
main.rsconstructor-bypass mutation incorrectly survived, establishing the defect above. - Live GitHub checks were green and the PR was mergeable when reviewed; green default CI does not exercise the missing feature lane.
Confidence gaps (not additional author defects)
- No local Docker daemon was available, so the Docker ARG/cache path was not independently executed.
- The claimed personal Railway deployment, APNs sandbox TLS/HTTP2 probe, and physical-iPhone notification/deep-link journey were not independently observed. The PR reports those results; the release operator owns preserving or repeating them if required for rollout.
- The personal-only feature intentionally adds synthetic-attestation helpers plus OpenSSL/P-256 to that binary. The exact signed-AAGUID fence is consequently load-bearing; current mutation-sensitive coverage of that fence materially reduces, but does not erase, this residual attack-surface risk.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 8953cbfff58ed768d996677fed3af0e3bac64a20..360a8e74b49e3c7969fddb9e41f033753f9a4557 (exact head 360a8e74b49e3c7969fddb9e41f033753f9a4557)
Risk: high — this introduces an opt-in build/runtime trust boundary for development App Attest enrollment and sandbox push delivery.
Blocking finding
The PR's sole new production path can break while every required gate remains green.
The development feature is absent from standard gateway and Docker CI, so all #[cfg(feature = "personal-dev-app-attest")] code and tests are skipped by required automation (crates/buzz-push-gateway/Cargo.toml:18-20, Justfile:401, scripts/run-tests.sh:118-119, .github/workflows/docker.yml:547-557). Separately, development_mode_preserves_verification_and_rejects_production constructs a private test helper and mutates its environment field (crates/buzz-push-gateway/src/app_attest.rs:369-430); it does not exercise config → executable wiring or AppAttestVerifier::with_environment.
This is not theoretical: replacing the production environment-aware construction at crates/buzz-push-gateway/src/main.rs:46-50 with production-only AppAttestVerifier::new still left the complete feature-enabled package suite green (51 non-ignored tests passed). A configured development gateway would then reject every development enrollment while the regression suite certified it.
Author action:
- Add a required CI lane that runs the complete gateway package with
--features personal-dev-app-attest; preferably also build the feature-enabled Docker target soBUZZ_PUSH_CARGO_FEATURESand its dependency/runtime closure are covered. - Add a production-shaped regression seam that binds parsed development configuration to construction of the environment-aware verifier and fails if the executable path falls back to
AppAttestVerifier::new. Mutation-prove that seam; testing the private helper again is insufficient.
Verification owner: author supplies failing-before/passing-after or equivalent mutation proof; reviewer reruns the new exact-head feature lane and call-site mutation.
Contracts traced
No additional blocking defect was found in the trust-chain implementation. Ordinary builds cannot represent Development and reject that setting; the special build still defaults to production (config.rs:21-29,158-166). The wrapper selects exactly one signed AAGUID before the dependency verifies certificate chain, nonce, app ID, zero attestation counter, public-key hash, AAGUID, and credential ID (app_attest.rs:68-90,137-169). APNs environment selection remains independently server-controlled (config.rs:169-179, apns.rs:94-99). Documentation accurately distinguishes a personal stack from distributed dogfood validation.
Exact-head validation
At clean 360a8e74b49e3c7969fddb9e41f033753f9a4557 with the pinned Hermit toolchain:
cargo test -p buzz-push-gateway— 50 passed, 11 ignored, 0 failed.cargo test -p buzz-push-gateway --features personal-dev-app-attest— 51 passed, 11 ignored, 0 failed.cargo clippy -p buzz-push-gateway --all-targets -- -D warnings— passed.cargo clippy -p buzz-push-gateway --all-targets --features personal-dev-app-attest -- -D warnings— passed.cargo fmt --all -- --checkandgit diff --check— passed.- AAGUID-fence mutation (development mode expecting production AAGUID) — targeted feature test failed with rc 101, then the tree was restored clean.
- Production-constructor bypass mutation — feature package suite incorrectly remained green, establishing the blocker.
- Exact-head repository CI and ordinary multi-arch push-gateway image builds are green.
Confidence gaps
- The feature-enabled Docker build was not independently run because the local Docker daemon was unavailable. Author action: none beyond the CI coverage requested above. Verification owner: CI/reviewer tooling.
- The reported Railway deployment and physical iPhone/APNs journey were not independently observed. Author action: none. Verification owner: rollout/release operator if independent workflow evidence is required.
- Enabling
appattest/testingadds synthetic-attestation/OpenSSL/P-256 code to the personal-development binary. The explicit personal-only build boundary makes this residual attack surface a documented tradeoff, not a separate defect.
🧹 workspace cleaned: disposable PR checkout and lane artifacts removed.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Changes requested
Reviewed head 360a8e74b49e3c7969fddb9e41f033753f9a4557 against base 8953cbfff58ed768d996677fed3af0e3bac64a20.
P1: Bind the environment fence to the authData actually verified
Anchor: crates/buzz-push-gateway/src/app_attest.rs:147–155.
With personal-dev-app-attest enabled, an otherwise valid development attestation can pass a production-configured verifier, and the reverse also works. The precheck and appattest 0.1.1 can select different authData slices from the same CBOR buffer. This defeats the new exact-environment guarantee; ordinary feature-disabled binaries still reject development attestations.
Source-traced reproduction: take a valid development attestation for the configured app ID and enrollment transcript. Keep its signed authData and attestation statement unchanged. Encode this ordered, definite five-entry root map, using an indefinite map only for the outer attStmt value:
map(5):
"fmt": "apple-appattest"
"authData": <53-byte decoy with production AAGUID at bytes 37..53>
"attStmt": indefinite-map:
"attStmt": <original definite statement containing x5c and receipt>
"authData": <original signed development authData>
break
"padding1": 0
"padding2": 0
The wrapper skips the whole indefinite attStmt, validates only the outer decoy, and reaches EOF. However, the locked dependency’s attestation.rs:202–206 converts that indefinite length to zero with unwrap_or(0) without consuming its contents. Its fourth and fifth root iterations therefore read the two inner entries as root entries, replacing authData with the signed development bytes and loading the original certificates/receipt. from_cbor returns without checking EOF (:175–283). Full verification validates that inner material, and testing accepts its development AAGUID (authenticator.rs:61–75). No signature, app-ID, nonce, or root forgery is required. Swapping the environments produces the symmetric bypass.
Please ensure the environment check and cryptographic verifier use the same parsed authData, or reject the parser-confusing container shapes before calling the dependency. Add both adversarial cross-environment cases to the feature-enabled verifier tests while preserving valid production/development and feature-disabled rejection coverage. Checking only duplicate root authData, indefinite root maps, and wrapper EOF does not catch this construction.
Other review comments: Mongo found no additional concrete deployment defect. The existing Codex comments identify useful coverage improvements: run the opt-in feature in CI and cover configuration/constructor wiring. I am not treating those as two additional demonstrated runtime defects. The required regression for this blocker must execute the feature-enabled production verifier; retain the default-feature package tests as well. Environment switching against shared development/production authority is outside the documented isolated-stack contract, not an additional requested migration feature.
Validation: source/metadata only on Wes’s laptop; no checkout, build, test, or PR/dependency-code execution. The dependency sources were compared byte-for-byte with cached archives whose SHA-256 values match this head’s Cargo.lock (appattest 0.1.1 and minicbor 0.25.1). Mordecai independently confirmed the parser trace. Existing green CI and the author-reported physical-device happy path do not exercise this malformed envelope.
3 participants
Development-signed iOS apps cannot enroll with the current gateway because their Apple App Attest AAGUID is rejected.
Add an explicit personal development build feature and environment setting so a development gateway can use a developer's own application identity and sandbox APNs credentials.
Ordinary gateway builds remain production-only. Special builds also default to production, enforce exactly the selected signed AAGUID, and retain certificate-chain, pinned-root, nonce, app-ID, key, credential-ID, and counter verification.
Document the matching mobile signing configuration and expose the feature through an optional Docker build argument.
Validation
The development image deployed successfully to a personal Railway stack; private readiness, enrollment challenges, authenticated relay access, and APNs sandbox TLS/HTTP2 connectivity passed.
End-to-end validation on a physical iPhone 12 mini exercised this gateway’s development App Attest configuration: device enrollment, relay delegation, and a fresh push lease succeeded; a desktop mention produced a sandbox APNs notification containing the actual message text, and tapping it opened the correct community/channel and highlighted the exact message. Production-default behavior was covered by automated tests.