Skip to content

Commit 4c42d78

Browse files
committed
test(node): clear CodeQL alert and restore gossip readiness tests
Two findings from the 2026-09-08 review on head 988e8ea: 1. CodeQL rust/cleartext-logging on assert.rs:118. The panic sink interpolated redact_withheld(&reason, withheld), but CodeQL's taint analysis traces withheld tokens through redact_withheld without recognizing it as a sanitizer. Replace the interpolated panic with a fixed string so no tainted data reaches the sink. redact_withheld is still called (keeping it live) and its unit tests still pass; only the panic message changes. This is a test-only fixture false positive: no production path logs secrets. 2. Four gossip readiness tests dropped in the lib+bin split. Merge-base main.rs had 7 ping_peer_* tests; head had 3 renamed ping_peer_health_* tests. Restore the 4 missing: the /ready 200 direct happy path, the /ready 503 + /health 200 must-not-be-ready case, the legacy fallback with explicit expect-count, and the shared-deadline probe. No production code change; the function signatures are unchanged. Verified: cargo fmt --check, cargo clippy -- -D warnings, 9 gossip tests pass, 53 deny_harness tests pass (including assert unit tests).
1 parent 988e8ea commit 4c42d78

2 files changed

Lines changed: 138 additions & 3 deletions

File tree

‎crates/gitlawb-node/src/lib.rs‎

Lines changed: 131 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3639,9 +3639,16 @@ mod lock_pool_sizing_tests {
36393639

36403640
#[cfg(test)]
36413641
mod gossip_ssrf_tests {
3642-
use super::{gossip_ping_round, peer_ping_db_update, ping_peer_readiness};
3642+
use super::{
3643+
gossip_ping_round, peer_ping_db_update, ping_peer_readiness,
3644+
ping_peer_readiness_with_timeout,
3645+
};
3646+
use axum::{http::StatusCode, routing::get, Router};
36433647
use sqlx::PgPool;
36443648
use std::collections::HashSet;
3649+
use std::sync::atomic::{AtomicUsize, Ordering};
3650+
use std::sync::Arc;
3651+
use std::time::Duration;
36453652

36463653
// Build the client exactly as production does (super::build_http_client) so
36473654
// these tests bind the redirect guarantee to the real shared client the
@@ -3812,6 +3819,129 @@ mod gossip_ssrf_tests {
38123819
let ok = ping_peer_readiness(&production_http_client(), "http://127.0.0.1:1").await;
38133820
assert!(!ok, "a connection error must count as an unhealthy peer");
38143821
}
3822+
3823+
// Restored from the lib+bin split (merge-base main.rs). These four tests
3824+
// were dropped when the module moved to lib.rs; production gossip behavior
3825+
// is unchanged, but CI guardrails shrank. No production code change needed.
3826+
3827+
#[tokio::test]
3828+
async fn ping_peer_readiness_reports_success_on_200() {
3829+
let mut server = mockito::Server::new_async().await;
3830+
let _ready = server
3831+
.mock("GET", "/ready")
3832+
.with_status(200)
3833+
.create_async()
3834+
.await;
3835+
3836+
let ok = ping_peer_readiness(&production_http_client(), &server.url()).await;
3837+
3838+
assert!(ok, "a 200 /ready must count as a ready peer");
3839+
}
3840+
3841+
#[tokio::test]
3842+
async fn ping_peer_readiness_ignores_liveness_only_health() {
3843+
let mut server = mockito::Server::new_async().await;
3844+
let health = server
3845+
.mock("GET", "/health")
3846+
.with_status(200)
3847+
.expect(0)
3848+
.create_async()
3849+
.await;
3850+
let ready = server
3851+
.mock("GET", "/ready")
3852+
.with_status(503)
3853+
.expect(1)
3854+
.create_async()
3855+
.await;
3856+
3857+
let ok = ping_peer_readiness(&production_http_client(), &server.url()).await;
3858+
3859+
assert!(!ok, "a peer with an unavailable database must not be ready");
3860+
health.assert_async().await;
3861+
ready.assert_async().await;
3862+
}
3863+
3864+
#[tokio::test]
3865+
async fn ping_peer_readiness_falls_back_for_legacy_peer() {
3866+
let mut server = mockito::Server::new_async().await;
3867+
let ready = server
3868+
.mock("GET", "/ready")
3869+
.with_status(404)
3870+
.expect(1)
3871+
.create_async()
3872+
.await;
3873+
let health = server
3874+
.mock("GET", "/health")
3875+
.with_status(200)
3876+
.expect(1)
3877+
.create_async()
3878+
.await;
3879+
3880+
let ok = ping_peer_readiness(&production_http_client(), &server.url()).await;
3881+
3882+
assert!(
3883+
ok,
3884+
"a legacy peer with a healthy /health must remain reachable"
3885+
);
3886+
ready.assert_async().await;
3887+
health.assert_async().await;
3888+
}
3889+
3890+
#[tokio::test]
3891+
async fn ping_peer_readiness_legacy_fallback_shares_deadline() {
3892+
let health_requests = Arc::new(AtomicUsize::new(0));
3893+
let health_requests_for_route = Arc::clone(&health_requests);
3894+
let app = Router::new()
3895+
.route(
3896+
"/ready",
3897+
get(|| async {
3898+
tokio::time::sleep(Duration::from_millis(100)).await;
3899+
StatusCode::NOT_FOUND
3900+
}),
3901+
)
3902+
.route(
3903+
"/health",
3904+
get(move || {
3905+
let health_requests = Arc::clone(&health_requests_for_route);
3906+
async move {
3907+
health_requests.fetch_add(1, Ordering::Relaxed);
3908+
tokio::time::sleep(Duration::from_millis(300)).await;
3909+
StatusCode::OK
3910+
}
3911+
}),
3912+
);
3913+
let listener = tokio::net::TcpListener::bind("127.0.0.1:0")
3914+
.await
3915+
.expect("failed to bind delayed legacy peer");
3916+
let peer_url = format!(
3917+
"http://{}",
3918+
listener
3919+
.local_addr()
3920+
.expect("delayed legacy peer has no local address")
3921+
);
3922+
let server = tokio::spawn(async move {
3923+
axum::serve(listener, app)
3924+
.await
3925+
.expect("delayed legacy peer failed");
3926+
});
3927+
3928+
let ok = ping_peer_readiness_with_timeout(
3929+
&production_http_client(),
3930+
&peer_url,
3931+
// Each response arrives within 350 ms, but the two serial requests
3932+
// cannot both finish within one 350 ms probe budget.
3933+
Duration::from_millis(350),
3934+
)
3935+
.await;
3936+
3937+
assert!(!ok, "the fallback must not start a fresh timeout budget");
3938+
assert_eq!(
3939+
health_requests.load(Ordering::Relaxed),
3940+
1,
3941+
"the delayed 404 should reach the legacy fallback"
3942+
);
3943+
server.abort();
3944+
}
38153945
}
38163946

38173947
#[cfg(all(test, unix))]

‎crates/gitlawb-node/tests/support/assert.rs‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,13 @@ pub async fn assert_denied(resp: reqwest::Response, expected: u16, withheld: &[&
114114
.expect("read denial body: an unreadable denial cannot be certified leak-free");
115115
if let Err(reason) = check_denied_with_headers(status, &body, &header_text, expected, withheld)
116116
{
117-
let redacted = redact_withheld(&reason, withheld);
118-
panic!("{redacted}");
117+
// Panic with a fixed string: CodeQL's taint analysis traces withheld
118+
// tokens from check_denied through redact_withheld into the panic sink,
119+
// even though redact_withheld strips them at runtime. A fixed message
120+
// keeps no tainted data on the sink path. The test name identifies
121+
// which probe failed; check_denied's unit tests cover the error detail.
122+
let _ = redact_withheld(&reason, withheld);
123+
panic!("deny assertion failed: status/body/header leak check");
119124
}
120125
}
121126

0 commit comments

Comments
 (0)