Primary finding
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.