Skip to content

Commit 70def67

Browse files
committed
Resolve PR 725 round-1 review findings
Route smoke tests — false positives via wildcard fallback: - Add registered_routes() helper using RouterService::routes() for introspection - Replace 9 individual assert_ne!(404) tests with all_explicit_routes_are_registered() which checks the route table directly; a removed explicit route now fails even when a wildcard fallback would have returned non-404 Cross-adapter parity tests — value equality not just presence: - admin_rotate_unauthenticated_parity: extract both WWW-Authenticate header values and assert they are equal; assert the shared value starts with Basic - admin_deactivate_unauthenticated_parity: same fix - geo_header_parity_on_all_responses: compare X-Geo-Info-Available values across adapters rather than only checking presence; catches true vs false divergence CI clippy: - Add --all-targets to the integration-tests clippy invocation so test targets (tests/parity.rs, tests/routes.rs) are actually linted
1 parent 161c41e commit 70def67

3 files changed

Lines changed: 81 additions & 165 deletions

File tree

.github/workflows/test.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ jobs:
127127
run: cargo test --manifest-path crates/integration-tests/Cargo.toml --test parity
128128

129129
- name: Clippy (parity test crate)
130-
run: cargo clippy --manifest-path crates/integration-tests/Cargo.toml -- -D warnings
130+
run: cargo clippy --manifest-path crates/integration-tests/Cargo.toml --all-targets -- -D warnings
131131

132132
test-typescript:
133133
name: vitest

crates/integration-tests/tests/parity.rs

Lines changed: 41 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -215,18 +215,23 @@ async fn admin_rotate_unauthenticated_parity() {
215215
"both adapters must return the same status for unauthenticated admin route"
216216
);
217217

218-
assert!(
219-
axum_headers.contains_key("www-authenticate"),
220-
"Axum 401 must include WWW-Authenticate header"
221-
);
218+
let axum_www_auth = axum_headers
219+
.get("www-authenticate")
220+
.expect("Axum 401 must include WWW-Authenticate")
221+
.to_str()
222+
.expect("should be valid UTF-8");
222223
let cf_www_auth = cf_headers
223224
.get("www-authenticate")
224-
.expect("should have www-authenticate header on 401")
225+
.expect("Cloudflare 401 must include WWW-Authenticate")
225226
.to_str()
226227
.expect("should be valid UTF-8");
228+
assert_eq!(
229+
axum_www_auth, cf_www_auth,
230+
"WWW-Authenticate header value must match across adapters for /admin/keys/rotate"
231+
);
227232
assert!(
228-
cf_www_auth.starts_with("Basic realm="),
229-
"Cloudflare 401 WWW-Authenticate must be Basic scheme: {cf_www_auth:?}"
233+
axum_www_auth.starts_with("Basic"),
234+
"WWW-Authenticate must use Basic scheme: {axum_www_auth:?}"
230235
);
231236
}
232237

@@ -249,13 +254,23 @@ async fn admin_deactivate_unauthenticated_parity() {
249254
"both adapters must return the same status for unauthenticated admin/keys/deactivate"
250255
);
251256

252-
assert!(
253-
axum_headers.contains_key("www-authenticate"),
254-
"Axum 401 on admin/keys/deactivate must include WWW-Authenticate header"
257+
let axum_www_auth = axum_headers
258+
.get("www-authenticate")
259+
.expect("Axum 401 on admin/keys/deactivate must include WWW-Authenticate")
260+
.to_str()
261+
.expect("should be valid UTF-8");
262+
let cf_www_auth = cf_headers
263+
.get("www-authenticate")
264+
.expect("Cloudflare 401 on admin/keys/deactivate must include WWW-Authenticate")
265+
.to_str()
266+
.expect("should be valid UTF-8");
267+
assert_eq!(
268+
axum_www_auth, cf_www_auth,
269+
"WWW-Authenticate header value must match across adapters for /admin/keys/deactivate"
255270
);
256271
assert!(
257-
cf_headers.contains_key("www-authenticate"),
258-
"Cloudflare 401 on admin/keys/deactivate must include WWW-Authenticate header"
272+
axum_www_auth.starts_with("Basic"),
273+
"WWW-Authenticate must use Basic scheme: {axum_www_auth:?}"
259274
);
260275
}
261276

@@ -279,13 +294,20 @@ async fn geo_header_parity_on_all_responses() {
279294
cf_post_headers(path, body).await
280295
};
281296

282-
assert!(
283-
axum_headers.contains_key("x-geo-info-available"),
284-
"Axum: {method} {path} (status={axum_status}) must have X-Geo-Info-Available"
285-
);
286-
assert!(
287-
cf_headers.contains_key("x-geo-info-available"),
288-
"Cloudflare: {method} {path} (status={cf_status}) must have X-Geo-Info-Available"
297+
let axum_geo = axum_headers
298+
.get("x-geo-info-available")
299+
.unwrap_or_else(|| panic!("Axum: {method} {path} (status={axum_status}) must have X-Geo-Info-Available"))
300+
.to_str()
301+
.expect("should be valid UTF-8");
302+
let cf_geo = cf_headers
303+
.get("x-geo-info-available")
304+
.unwrap_or_else(|| panic!("Cloudflare: {method} {path} (status={cf_status}) must have X-Geo-Info-Available"))
305+
.to_str()
306+
.expect("should be valid UTF-8");
307+
assert_eq!(
308+
axum_geo, cf_geo,
309+
"{method} {path}: X-Geo-Info-Available value must match across adapters \
310+
(axum={axum_geo:?} cf={cf_geo:?})"
289311
);
290312
}
291313
}

crates/trusted-server-adapter-cloudflare/tests/routes.rs

Lines changed: 39 additions & 145 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,23 @@ use edgezero_core::app::Hooks as _;
88
use edgezero_core::http::request_builder;
99
use trusted_server_adapter_cloudflare::app::TrustedServerApp;
1010

11+
/// Return the set of (METHOD, path) pairs explicitly registered on the router.
12+
fn registered_routes() -> Vec<(String, String)> {
13+
TrustedServerApp::routes()
14+
.routes()
15+
.into_iter()
16+
.map(|r| (r.method().to_string(), r.path().to_string()))
17+
.collect()
18+
}
19+
20+
fn assert_route_registered(method: &str, path: &str) {
21+
let routes = registered_routes();
22+
assert!(
23+
routes.iter().any(|(m, p)| m == method && p == path),
24+
"{method} {path} must be explicitly registered; registered routes: {routes:?}"
25+
);
26+
}
27+
1128
#[test]
1229
fn routes_build_without_panic() {
1330
// build_state() may fail (no real settings in CI) — startup_error_router
@@ -92,152 +109,29 @@ async fn tsjs_route_is_routed_not_5xx() {
92109
assert!(status < 500, "tsjs route must not 5xx: got {status}");
93110
}
94111

95-
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
96-
async fn verify_signature_is_routed() {
97-
let router = TrustedServerApp::routes();
98-
let req = request_builder()
99-
.method("POST")
100-
.uri("/verify-signature")
101-
.header("content-type", "application/json")
102-
.body(edgezero_core::body::Body::from("{}"))
103-
.expect("should build request");
104-
let resp = router.oneshot(req).await;
105-
assert_ne!(
106-
resp.status().as_u16(),
107-
404,
108-
"/verify-signature must be routed"
109-
);
110-
}
111-
112-
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
113-
async fn admin_rotate_key_is_routed() {
114-
let router = TrustedServerApp::routes();
115-
let req = request_builder()
116-
.method("POST")
117-
.uri("/admin/keys/rotate")
118-
.header("content-type", "application/json")
119-
.body(edgezero_core::body::Body::from("{}"))
120-
.expect("should build request");
121-
let resp = router.oneshot(req).await;
122-
assert_ne!(
123-
resp.status().as_u16(),
124-
404,
125-
"/admin/keys/rotate must be routed"
126-
);
127-
}
128-
129-
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
130-
async fn admin_deactivate_key_is_routed() {
131-
let router = TrustedServerApp::routes();
132-
let req = request_builder()
133-
.method("POST")
134-
.uri("/admin/keys/deactivate")
135-
.header("content-type", "application/json")
136-
.body(edgezero_core::body::Body::from("{}"))
137-
.expect("should build request");
138-
let resp = router.oneshot(req).await;
139-
assert_ne!(
140-
resp.status().as_u16(),
141-
404,
142-
"/admin/keys/deactivate must be routed"
143-
);
144-
}
145-
146-
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
147-
async fn auction_is_routed() {
148-
let router = TrustedServerApp::routes();
149-
let req = request_builder()
150-
.method("POST")
151-
.uri("/auction")
152-
.header("content-type", "application/json")
153-
.body(edgezero_core::body::Body::from(r#"{"adUnits":[]}"#))
154-
.expect("should build request");
155-
let resp = router.oneshot(req).await;
156-
assert_ne!(resp.status().as_u16(), 404, "/auction must be routed");
157-
}
158-
159-
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
160-
async fn first_party_proxy_is_routed() {
161-
let router = TrustedServerApp::routes();
162-
let req = request_builder()
163-
.method("GET")
164-
.uri("/first-party/proxy")
165-
.body(edgezero_core::body::Body::empty())
166-
.expect("should build request");
167-
let resp = router.oneshot(req).await;
168-
// Handlers require valid outbound proxy settings; they may return 4xx/5xx in CI.
169-
// The assertion is routing only: the path must not fall through to the 404 not-found handler.
170-
assert_ne!(
171-
resp.status().as_u16(),
172-
404,
173-
"/first-party/proxy must be routed"
174-
);
175-
}
176-
177-
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
178-
async fn first_party_click_is_routed() {
179-
let router = TrustedServerApp::routes();
180-
let req = request_builder()
181-
.method("GET")
182-
.uri("/first-party/click")
183-
.body(edgezero_core::body::Body::empty())
184-
.expect("should build request");
185-
let resp = router.oneshot(req).await;
186-
assert_ne!(
187-
resp.status().as_u16(),
188-
404,
189-
"/first-party/click must be routed"
190-
);
191-
}
192-
193-
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
194-
async fn first_party_sign_get_is_routed() {
195-
let router = TrustedServerApp::routes();
196-
let req = request_builder()
197-
.method("GET")
198-
.uri("/first-party/sign")
199-
.body(edgezero_core::body::Body::empty())
200-
.expect("should build request");
201-
let resp = router.oneshot(req).await;
202-
assert_ne!(
203-
resp.status().as_u16(),
204-
404,
205-
"GET /first-party/sign must be routed"
206-
);
207-
}
208-
209-
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
210-
async fn first_party_sign_post_is_routed() {
211-
let router = TrustedServerApp::routes();
212-
let req = request_builder()
213-
.method("POST")
214-
.uri("/first-party/sign")
215-
.header("content-type", "application/json")
216-
.body(edgezero_core::body::Body::from("{}"))
217-
.expect("should build request");
218-
let resp = router.oneshot(req).await;
219-
assert_ne!(
220-
resp.status().as_u16(),
221-
404,
222-
"POST /first-party/sign must be routed"
223-
);
224-
}
112+
/// Verify that every expected explicit route is registered in the route table.
113+
///
114+
/// Uses [`RouterService::routes()`] for introspection rather than checking
115+
/// response status codes — wildcards (`/{*rest}`) can return non-404 even when
116+
/// an explicit registration is missing, making status-based checks false positives.
117+
#[test]
118+
fn all_explicit_routes_are_registered() {
119+
let expected: &[(&str, &str)] = &[
120+
("GET", "/.well-known/trusted-server.json"),
121+
("POST", "/verify-signature"),
122+
("POST", "/admin/keys/rotate"),
123+
("POST", "/admin/keys/deactivate"),
124+
("POST", "/auction"),
125+
("GET", "/first-party/proxy"),
126+
("GET", "/first-party/click"),
127+
("GET", "/first-party/sign"),
128+
("POST", "/first-party/sign"),
129+
("POST", "/first-party/proxy-rebuild"),
130+
];
225131

226-
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
227-
async fn first_party_proxy_rebuild_is_routed() {
228-
let router = TrustedServerApp::routes();
229-
let req = request_builder()
230-
.method("POST")
231-
.uri("/first-party/proxy-rebuild")
232-
.header("content-type", "application/json")
233-
.body(edgezero_core::body::Body::from("{}"))
234-
.expect("should build request");
235-
let resp = router.oneshot(req).await;
236-
assert_ne!(
237-
resp.status().as_u16(),
238-
404,
239-
"/first-party/proxy-rebuild must be routed"
240-
);
132+
for (method, path) in expected {
133+
assert_route_registered(method, path);
134+
}
241135
}
242136

243137
// ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)