Access-control test coverage gaps (non-blocking, from Network Access Control review) #16
Loading…
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Background
Minor coverage gaps noted during per-task and whole-branch review of the Network Access Control feature. All the relevant logic was traced correct by reviewers and is exercised indirectly; these are "would-be-nice" explicit tests, not known bugs. Bundled into one issue.
Gaps
Malformed
CF-Connecting-IPheader —access::effective_client_ipuses.and_then(|s| s.parse().ok())then.unwrap_or(peer_ip), so an unparseable header falls back to the real peer (safe). No dedicated test asserts this fallback branch.Empty inner allow-list (
allow_ips: Some(vec![])) —access::access_allowedruns.any()over the empty CIDR list →false→ deny (safe direction). No dedicated test for this corner.API
access_rulesend-to-end round-trip —access_rulesis proven to round-trip at the DB layer (db.rstests) and the DTO/serde layer (api.rstests), but there's no single handler-level test POSTing a body withaccess_rulesthroughregister_service/register_l4_routeand reading it back from the DB. Low risk (construction sites are compile-enforced; handler bodies are one-line field threads).OR-of-rules — first alternate —
test_service_access_rules_or_togetherproves reachable-via-cloudflare(the second rule) and denied-via-lan, but never asserts reachable-via-tailscale(the first rule). A mutation that matched only the last rule in the list would still pass. Add a third request viatailscaleasserting!= NOT_FOUNDto fully cover OR semantics.Severity
Minor — coverage breadth only. No known incorrect behavior; the pure matching logic is unit-tested and the trust boundary is proven end-to-end over real sockets (
test_trust_forwarded_for_honored_only_on_structurally_trusted_binding).