Anatomy · 62a6fd05-1

Test does not assert Vary: Origin is preserved when an allowed origin is present

lowtest-gap

repo df3ede3f·PR #1·reviewed 1 week ago

The vulnerable code

src/core/jsonrpc_cors_tests.rs:72-80

Code snippet unavailable.

The reasoning

Opus

Comment misrepresents safety of mutating ALLOWED_ORIGINS_ENV during parallel test runs

mediumconcurrencyhigh

src/core/jsonrpc_cors_tests.rs:80-104
src/core/jsonrpc_cors_tests.rs:21-47

The comment claims parallel runs are safe because no other test reads ALLOWED_ORIGINS_ENV. However, the other tests in this file (`allows_tauri_webview_origins`, `allows_loopback_with_any_port`, `rejects_disallowed_origins`, `always_sets_methods_headers_and_max_age`) all call `is_origin_allowed(...)`, which by its very name and the existence of the ALLOWED_ORIGINS_ENV constant almost certainly consults that env var when computing the allowlist. Cargo's default test harness runs tests in parallel threads within the same process, and `std::env::set_var` mutates process-global state. While `env_override_allows_extra_origins` is running with the env var set to `https://debug.internal, http://harness:9000`, a concurrently executing test like `allows_tauri_webview_origins` could observe that allowlist (or its restoration race) and either fail or, worse, pass for the wrong reason. The comment's claim of safety is therefore deceptive and the test is genuinely flaky. Additionally, on Rust 2024 `std::env::set_var` is `unsafe` precisely because it is not thread-safe on Unix (races with getenv in other threads / libc), so the `SAFETY` comment is technically incorrect about the soundness invariant being upheld.

Recommendation

Either (a) serialize env-mutating tests using a `Mutex`/`serial_test` crate and document that all tests touching ALLOWED_ORIGINS_ENV must take the lock, (b) gate this test behind `#[ignore]` + a dedicated single-threaded test binary, or (c) refactor `is_origin_allowed` to take the allowlist as a parameter so tests don't need to mutate process-global state. Fix the misleading SAFETY comment so it doesn't assert thread-safety that doesn't hold.

GPT-5

Unnecessary unsafe blocks around std::env::set_var/remove_var in tests trigger unused_unsafe and hinder deny(warnings) builds

mediummaintainabilityhigh

src/core/jsonrpc_cors_tests.rs:81-90
src/core/jsonrpc_cors_tests.rs:98-103

std::env::set_var and std::env::remove_var are safe functions and do not require unsafe. Wrapping them in unsafe blocks produces the unused_unsafe lint. In projects that treat warnings as errors (e.g., RUSTFLAGS=-D warnings or CI pipelines), this will fail the build/test. It also miscommunicates that a memory-unsafe operation is occurring when it is not.

Recommendation

Remove both unsafe blocks. Keep the explanatory comment if desired. If you want to prevent future accidental introduction of unsafe code in tests, consider adding #![deny(unsafe_code)] at the top of the test module or enabling the corresponding lint in CI.

The agreement

Both frontier models flagged this within the same line range. AntFleet's unanimous gate fired — the finding posted on the PR.

Closure

Tweet thread template

tweet 1 of 8164 / 280

Two frontier models reviewed PR #1 on df3ede3f. Both found this bug: low test-gap: Test does not assert Vary: Origin is preserved when an allowed origin is present

tweet 2 of 8122 / 280

The vulnerable code (src/core/jsonrpc_cors_tests.rs:72-80): (full snippet at https://www.antfleet.dev/anatomy/62a6fd05-1)

tweet 3 of 8280 / 280

What Opus saw: "The comment claims parallel runs are safe because no other test reads ALLOWED_ORIGINS_ENV. However, the other tests in this file (`allows_tauri_webview_origins`, `allows_loopback_with_any_port`, `rejects_disallowed_origins`, `always_sets_methods_headers_and_max_…

tweet 4 of 8280 / 280

What GPT-5 saw: "std::env::set_var and std::env::remove_var are safe functions and do not require unsafe. Wrapping them in unsafe blocks produces the unused_unsafe lint. In projects that treat warnings as errors (e.g., RUSTFLAGS=-D warnings or CI pipelines), this will fail the …

tweet 5 of 897 / 280

Both flagged the same line range. AntFleet's unanimous gate fired — the finding posted on the PR.

tweet 6 of 893 / 280

The fix landed in commit pending: (view diff at https://www.antfleet.dev/anatomy/62a6fd05-1)

tweet 7 of 881 / 280

AntFleet reviews every PR with two frontier models. Only unanimous findings post.

tweet 8 of 877 / 280

Full anatomy + reasoning + diffs: https://www.antfleet.dev/anatomy/62a6fd05-1

Paste into X composer one tweet at a time. X has no multi-tweet intent API.

view receipt →all receipts →view disagreements →case studies →