IsoHex/edge-router

Watch

Fork 0

HTTP→HTTPS redirect does not cover the admin/session routes, so the dashboard login accepts a plaintext password #69

New issue

Closed

opened 2026-10-27 09:24:14 +00:00 by james.oates · 1 comment

james.oates commented

2026-10-27 09:24:14 +00:00

Owner

Problem

redirect_http_to_https defaults to true and the code treats suppressing it as an attack (see the trust_forwarded comment in proxy.rs about sslstrip). But the redirect is implemented inside proxy::proxy_handler, which is mounted as the fallback in build_app (main.rs:116). Anything that matches a real route is handled before the fallback and therefore never redirected.

That includes every admin/session route, among them POST /api/session — the dashboard login, which carries the master key.

Observed against v2.0.0 (9ab9f83) on the devinfra deployment, HTTPS_PORT=443, redirect_http_to_https left at its default:

GET  /             Host: router.<admin-host>   over http  →  301 https://router.<admin-host>/
POST /api/session  Host: router.<admin-host>   over http  →  400   (served, not redirected)

The 400 is just the empty test body being rejected — the point is the request was handled on the plaintext listener instead of being bounced. A real credential posted there goes over the wire in the clear, and the session cookie's Secure flag won't be set on the response.

Why this needs fixing in the product

It is closeable per-deployment by dropping the plaintext listener from [[admin.access]] — admin_ip_middleware evaluates access_allowed against the listener (middleware.rs:151), so the route then 403s on http. The browser flow is unaffected, because GET / isn't a registered route and still falls through to the redirect.

But that requires every deployment to know about the gap and remember to close it in its own routing table, while the config it's reading tells it the redirect is on. The secure-by-default posture is doing the opposite of what it claims for exactly the routes where it matters most.

Suggested direction

Either:

Apply the redirect ahead of route matching (middleware layered over the whole app rather than logic inside the fallback), so it covers every route uniformly; or
Have the session route refuse a non-TLS request outright — narrower, and arguably correct regardless of the redirect, since a login is the one thing that should never be served plaintext even if an operator deliberately disables redirects.

(1) is the general fix; (2) is worth having either way as a backstop.

Mitigating context for the deployment that found it

On devinfra the plaintext port is loopback-only (docker.expose_ports_to_lan=false), and a browser never takes this path — it is redirected before the login form renders. So the practical exposure there is low and this is not urgent for that machine. The bug is general, though, and a deployment that does expose port 80 would be handing out the master key in cleartext to anything that skips the redirect.

## Problem `redirect_http_to_https` defaults to `true` and the code treats suppressing it as an attack (see the `trust_forwarded` comment in `proxy.rs` about sslstrip). But the redirect is implemented inside `proxy::proxy_handler`, which is mounted as the **fallback** in `build_app` (`main.rs:116`). Anything that matches a real route is handled before the fallback and therefore never redirected. That includes every admin/session route, among them `POST /api/session` — the dashboard login, which carries the master key. Observed against v2.0.0 (`9ab9f83`) on the devinfra deployment, `HTTPS_PORT=443`, `redirect_http_to_https` left at its default: ``` GET / Host: router.<admin-host> over http → 301 https://router.<admin-host>/ POST /api/session Host: router.<admin-host> over http → 400 (served, not redirected) ``` The 400 is just the empty test body being rejected — the point is the request was handled on the plaintext listener instead of being bounced. A real credential posted there goes over the wire in the clear, and the session cookie's `Secure` flag won't be set on the response. ## Why this needs fixing in the product It is closeable per-deployment by dropping the plaintext listener from `[[admin.access]]` — `admin_ip_middleware` evaluates `access_allowed` against the listener (`middleware.rs:151`), so the route then 403s on http. The browser flow is unaffected, because `GET /` isn't a registered route and still falls through to the redirect. But that requires every deployment to know about the gap and remember to close it in its own routing table, while the config it's reading tells it the redirect is on. The secure-by-default posture is doing the opposite of what it claims for exactly the routes where it matters most. ## Suggested direction Either: 1. Apply the redirect ahead of route matching (middleware layered over the whole app rather than logic inside the fallback), so it covers every route uniformly; or 2. Have the session route refuse a non-TLS request outright — narrower, and arguably correct regardless of the redirect, since a login is the one thing that should never be served plaintext even if an operator deliberately disables redirects. (1) is the general fix; (2) is worth having either way as a backstop. ## Mitigating context for the deployment that found it On devinfra the plaintext port is loopback-only (`docker.expose_ports_to_lan=false`), and a browser never takes this path — it is redirected before the login form renders. So the practical exposure there is low and this is not urgent for that machine. The bug is general, though, and a deployment that does expose port 80 would be handing out the master key in cleartext to anything that skips the redirect.

james.oates added this to the v2.1 milestone

2026-10-27 09:28:36 +00:00

james.oates referenced this issue

2026-10-27 09:43:38 +00:00

Apply the HTTP→HTTPS redirect ahead of route matching, exempting the liveness routes #70

james.oates referenced this issue

2026-10-27 09:43:50 +00:00

API-key routes are served on a plaintext admin-granted listener, so the key crosses in clear #71

james.oates commented

2026-10-27 09:44:06 +00:00

Author

Owner

Fixed in 78ae254 by option (2), not option (1). Both your observations were correct against the code, including that dropping the plaintext listener from [[admin.access]] closes it per-deployment — /api/session is behind admin_ip_middleware, unlike the registration routes in #55.

Why (2) rather than the general fix. Layering the redirect over the whole app would 301 /health and /ready, which are matched routes served on plaintext today. A probe that does not follow redirects, or follows one to a cert it does not trust, goes red — so (1) as written trades a credential leak for a monitoring outage. It is raised as #70 with the exemption to design in.

(2) is also strictly stronger for this route: it holds where an operator sets redirect_http_to_https = false, which is exactly the case a redirect cannot cover.

What landed:

POST /api/session and /api/session/logout return 403 unless the request reached the client over TLS. /health shares that router and is deliberately exempt, so the guard sits on a sub-router holding only the two session routes. health_is_still_served_on_plaintext fails if it moves up a level — verified by putting it there on purpose.
#59 folded in, because it is the same defect. There were two disagreeing definitions of "is this HTTPS": the redirect checked the TLS marker or a trusted listener's X-Forwarded-Proto, while the cookie's Secure flag checked only the marker. One trust-gated access::request_is_https now serves the redirect, the guard and the cookie.

The discriminating test is a forged X-Forwarded-Proto: https on an untrusted listener being refused — a bare header sniff passes the plaintext and trusted-proxy cases identically.

Narrow breaking case, in the v2.1 notes: TLS terminating upstream on a listener without trust_forwarded_for, plus redirect_http_to_https = false. Login returns 403 there until that listener is marked trusted.

One correction to your report. It says HTTPS_PORT=443, redirect_http_to_https left at its default for devinfra — that is right, and it corrects me: I had claimed in c8c6382's message that devinfra's redirect never fires, from reading its TOML without its compose env. Thank you for the precise reproduction; it is what caught my error.

Follow-ons: #70 (app-wide redirect) and #71 (API-key routes on plaintext, same class, wider blast radius). Neither started.

Fixed in `78ae254` by option (2), not option (1). Both your observations were correct against the code, including that dropping the plaintext listener from `[[admin.access]]` closes it per-deployment — `/api/session` is behind `admin_ip_middleware`, unlike the registration routes in #55. **Why (2) rather than the general fix.** Layering the redirect over the whole app would `301` `/health` and `/ready`, which are matched routes served on plaintext today. A probe that does not follow redirects, or follows one to a cert it does not trust, goes red — so (1) as written trades a credential leak for a monitoring outage. It is raised as #70 with the exemption to design in. (2) is also strictly stronger for this route: it holds where an operator sets `redirect_http_to_https = false`, which is exactly the case a redirect cannot cover. **What landed:** - `POST /api/session` and `/api/session/logout` return `403` unless the request reached the client over TLS. `/health` shares that router and is deliberately exempt, so the guard sits on a sub-router holding only the two session routes. `health_is_still_served_on_plaintext` fails if it moves up a level — verified by putting it there on purpose. - **#59 folded in**, because it is the same defect. There were two disagreeing definitions of "is this HTTPS": the redirect checked the TLS marker *or* a trusted listener's `X-Forwarded-Proto`, while the cookie's `Secure` flag checked only the marker. One trust-gated `access::request_is_https` now serves the redirect, the guard and the cookie. The discriminating test is a forged `X-Forwarded-Proto: https` on an **untrusted** listener being refused — a bare header sniff passes the plaintext and trusted-proxy cases identically. Narrow breaking case, in the v2.1 notes: TLS terminating upstream on a listener *without* `trust_forwarded_for`, plus `redirect_http_to_https = false`. Login returns `403` there until that listener is marked trusted. **One correction to your report.** It says `HTTPS_PORT=443, redirect_http_to_https left at its default` for devinfra — that is right, and it corrects me: I had claimed in `c8c6382`'s message that devinfra's redirect never fires, from reading its TOML without its compose env. Thank you for the precise reproduction; it is what caught my error. Follow-ons: #70 (app-wide redirect) and #71 (API-key routes on plaintext, same class, wider blast radius). Neither started.

james.oates closed this issue

2026-10-27 09:44:12 +00:00

james.oates referenced this issue

2026-10-27 09:44:23 +00:00

Decide whether Edge Router should emit HSTS, on the admin host and optionally for proxied hosts #59

james.oates referenced this issue from a commit

2026-10-27 09:45:55 +00:00

Refuse a session login on a cleartext hop, and unify is_https (#69, #59)

james.oates referenced this issue

2026-10-27 15:12:11 +00:00

Decide whether Edge Router should emit HSTS, on the admin host and optionally for proxied hosts #59

james.oates referenced this issue