reverse-proxy/tasks/fix/rename-misleading-test.md at 8ac39d9cd8a142c914a41d632cd855fa6284844a

Files

glm-5.1 54f1725173 Decompose security review #003 findings into 17 fix tasks and 1 review task

Address 4 critical, 8 warning, and 5 suggestion findings from the
security and bug review by creating atomic, dependency-ordered tasks:

Critical fixes (C1-C4): rate limiter IP source (ADR-025), InFlightCounter
increment + drain interval, connector timeout ceiling (ADR-026), JSON format
without log file.

Validation tightening (W1, W2): upstream host validation, ACME contact email
validation.

Robustness (W3, W4, W5, W12): upstream URI error handling (502 not silent
drop), admin socket resource limits (ADR-027), TlsMode wildcard mismatch,
http_port u32→u16.

Code quality (W6, W10, W11, S1, S3, W8/W9): config type consolidation,
TokenBucket field visibility, reload_mutex #[cfg(test)], dead code removal,
root cert count logging, misleading test names.

Test coverage (S10): rate limiter ConnectInfo tests (depends on C1 fix).

Review: post-security-fix-review checkpoint covering all critical fixes
and sensitive config consolidation path.

2026-06-12 13:42:37 +00:00

1.9 KiB

Raw Blame History

id, name, status, depends_on, scope, risk, impact, level, review_findings

name

status

depends_on

scope

risk

impact

level

review_findings

fix/rename-misleading-test

Rename misleading health check test and fix dynamic config test (W8, W9)

pending

single

trivial

isolated

implementation

Description

Two test quality issues in tests/integration_test.rs:

W8: test_health_check_disabled_when_port_zero is misleading — port 0 means "OS picks a random port" not "disabled". The test verifies that start_health_check_listener(0) works (binds to a random port), which is correct but the name implies it tests the disable path. The actual disable logic is in main.rs:94 where health_check_port > 0 gates the call.

W9: test_dynamic_config_with_limit constructs DynamicConfig directly with routing_table: Default::default() (empty HashMap), bypassing DynamicConfig::from_sites(). The body limit tests only read body.limit_bytes so it doesn't matter, but it's a latent trap for anyone copying this pattern.

Changes Required

tests/integration_test.rs:

Rename test_health_check_disabled_when_port_zero to test_health_check_binds_random_port_when_zero
Update test_dynamic_config_with_limit to use DynamicConfig::from_sites() instead of directly constructing with an empty routing table, OR add a comment explaining why the routing table is intentionally empty for this test

Acceptance Criteria

Test renamed to test_health_check_binds_random_port_when_zero
test_dynamic_config_with_limit either uses from_sites() or has a comment explaining the intentional empty routing table
cargo test passes
cargo clippy passes with no warnings

References

docs/reviews/003-security-and-bug-review.md — W8, W9 findings
tests/integration_test.rs — test names and implementations

Notes

To be filled on completion

Summary