Break down findings from review #002 into dependency-ordered fix tasks: Critical/High: - fix/acme-contact-and-challenge (C1+C2): Add acme_contact field, wire to ACME, remove unused challenge_config, add validation rule 19 - fix/remove-health-and-hardcode-https (W5+W14+ADR-022): Remove /health from main listener, hardcode X-Forwarded-Proto to https - fix/config-reload-static-drift (C4): Use ArcSwap<StaticConfig> so reload diffs against last config, not startup config - fix/access-logging (W13): Wire up log_request! macro for every proxied request with client_ip, host, method, path, status, upstream, duration_ms Medium: - fix/graceful-shutdown (W1+W7): Join HTTPS tasks with timeout instead of abort, add shutdown signal to admin socket and eviction task - fix/connect-timeout (W4): Wire upstream_connect_timeout_secs to enforce separate connect timeout Low/Independent: - fix/token-bucket-nanosecond (W6): Use as_nanos() instead of as_millis() - fix/normalize-host-ipv6 (S3): Handle IPv6 bracket notation in normalize_host - fix/http-port-validation (S1): Validate http_port in range 0 or 1-65535 - fix/integration-test-toml (S10): Fix double-nested listeners.listeners.sites - fix/logging-test-global-subscriber (W9): Use try_init() to avoid test conflicts - fix/fragile-error-detection (W3): Add typed error matching or documented string match - fix/add-code-comments (C3,W8,W10,W11,S9): Document correct-but-non-obvious behaviors - fix/request-timeout-scope (S8): Document full-request timeout scope - fix/clean-dead-code (S4+S2): Remove dead_code annotations, add #[non_exhaustive] Review gate: - review/post-fix-review: Verify all fixes against architecture spec
3.7 KiB
id, name, status, depends_on, scope, risk, impact, level, review_findings, adr
| id | name | status | depends_on | scope | risk | impact | level | review_findings | adr | |||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| fix/remove-health-and-hardcode-https | Remove /health from main listener and hardcode X-Forwarded-Proto to https | pending | narrow | low | component | implementation |
|
|
Description
Two related changes that simplify the proxy handler:
-
W5 / ADR-022: Remove the
/healthroute from the main HTTPS listener. Health checking is an operational concern served exclusively by the dedicated local port (9900) and the admin socket'sstatuscommand. Serving/healthon the main listener creates collision with upstream applications and requires special-case routing before host matching. The route must be removed entirely — no configurable path replacement. -
W14: Remove the
is_httpsfield fromProxyStateand hardcodeX-Forwarded-Proto: httpsininject_proxy_headers. The proxy only proxies requests on the HTTPS listener (which always uses TLS), and the HTTP redirect listener sends 301 redirects rather than proxying. Theis_httpsfield was alwaystrueand was a latent bug for non-TLS contexts. Since there is no non-TLS proxying path, hardcoding"https"with a clear comment is correct.
Changes Required
src/proxy/handler.rs:
- Remove
health_handlerfunction - Remove the
/healthearly return inproxy_handler(lines 37-39) - Remove
is_httpsfield fromProxyStatestruct - Remove
is_httpsparameter fromproxy_router()— it's no longer needed - Remove the
/healthroute fromproxy_router()—Router::new().fallback(proxy_handler).with_state(state)instead ofRouter::new().route("/health", get(health_handler)).fallback(proxy_handler).with_state(state)
src/proxy/headers.rs:
- Change
inject_proxy_headerssignature to removeis_https: boolparameter - Hardcode
X-Forwarded-Prototo"https"with a comment explaining why:// X-Forwarded-Proto is always "https" because this proxy only forwards requests // received on the TLS listener. The HTTP listener redirects to HTTPS and does not // proxy requests, so X-Forwarded-Proto is never set for HTTP connections.
src/main.rs:
- Remove
is_https: truefromProxyStateinitialization - Update any calls to
proxy_router()orbuild_router()that passis_https
src/proxy/mod.rs:
- Update
build_routersignature if it referencesis_https
Tests:
- Remove
health_path_returns_200_regardless_of_hosttest - Remove
health_with_unknown_host_returns_200test - Update
make_proxy_statehelper — removeis_httpsfield - Update
inject_proxy_headerstests — removeis_httpsparameter, verifyX-Forwarded-Protois always"https" - The health check endpoint on port 9900 remains independently tested in
src/health.rs
Acceptance Criteria
- No
/healthroute on the main HTTPS listener ProxyStatestruct no longer hasis_httpsfieldinject_proxy_headersalways setsX-Forwarded-Proto: https(no parameter)- Code comment explains why
X-Forwarded-Protois always"https" proxy_handlerhas no special-case path matching before Host lookup- Port 9900 health check (
src/health.rs) is unchanged and working - All existing tests pass (minus removed health-on-main-listener tests)
cargo clippypasses with no warnings
References
- docs/architecture/decisions/022-health-check-scope.md — ADR-022
- docs/architecture/proxy.md — updated request flow (no /health route)
- docs/architecture/operations.md — health check is port 9900 only
- docs/reviews/002-implementation-review.md — W5, W14 findings
- docs/architecture/open-questions.md — OQ-08, OQ-11
Notes
To be filled by implementation agent
Summary
To be filled on completion