core, tests, plan: keep warp's status on a bad request

apps/simplex-badge-service/src/BadgeService/Web/Server.hs

@@ -36,7 +36,7 @@ import Data.Text (Text)
 import qualified Data.Text as T
 import Data.Text.Encoding (decodeUtf8', encodeUtf8)
 import Network.HTTP.Types (Header, ResponseHeaders, Status, hCacheControl, hContentType, internalServerError500, methodNotAllowed405, notFound404, ok200)
-import Network.Wai (Application, Response, pathInfo, requestMethod, responseLBS)
+import Network.Wai (Application, Response, mapResponseHeaders, pathInfo, requestMethod, responseLBS)
 import qualified Network.Wai.Handler.Warp as Warp
 import Simplex.Messaging.Util (tshow)
 
@@ -97,12 +97,19 @@ runWebServer ws@WebServer {wsConfig = WebConfig {webPort, webHost, webDir}} = do
     settings =
       Warp.setPort webPort
         . Warp.setHost (fromString $ T.unpack webHost)
-        -- Warp's default 500 is its own bare page, with none of the security headers on it, and
-        -- an uncaught exception is exactly the response an attacker can most easily provoke.
-        -- Every exception surface later steps add -- D6's request decoding, E3's and F2's
-        -- webhooks -- lands on this same listener, and 'withServiceTransaction' already lets a
-        -- database exception through ('serveCatalog'), so this is not hypothetical.
-        . Warp.setOnExceptionResponse (const internalErrorResponse)
+        -- Warp's default exception response is its own bare page, with none of the security
+        -- headers on it, and an uncaught exception is exactly the response an attacker can most
+        -- easily provoke. Every exception surface later steps add -- D6's request decoding, E3's
+        -- and F2's webhooks -- lands on this same listener, and 'withServiceTransaction' already
+        -- lets a database exception through ('serveCatalog'), so this is not hypothetical.
+        --
+        -- The headers are added TO Warp's own response rather than replacing it: this handler
+        -- also answers requests that never reached the application, and
+        -- 'defaultOnExceptionResponse' is what decides that an unparsable request is 400 and an
+        -- oversized header block is 431, not 500. Collapsing those to 500 would answer a
+        -- malformed request with "try again" -- and a provider retries a 5xx webhook, so a
+        -- malformed E3/F2 callback would be retried for ever instead of rejected once.
+        . Warp.setOnExceptionResponse (mapResponseHeaders (securityHeaders <>) . Warp.defaultOnExceptionResponse)
         -- ... and replacing the response would otherwise also silence Warp's own default
         -- logging of it. 'defaultShouldDisplayException' keeps client disconnects quiet, which
         -- is the reason Warp's default is not simply "log everything".

plans/badges-codes/2026-08-21-badges-web-checkout.md

@@ -845,7 +845,7 @@ Phase D ends with a browsable, priced site wizard whose Pay button reaches a rea
 
 - Security headers on every response: `Content-Security-Policy: default-src 'self'`, `X-Content-Type-Options: nosniff`, `Referrer-Policy: no-referrer`, `X-Frame-Options: DENY`. The site loads no cross-origin resource, so `default-src 'self'` blocks nothing it needs.
 
-**Verify:** In `tests/Bots/BadgeServiceTests.hs`, over HTTP against the running service: `GET /` carries no `@@…@@`, resolves `@@support_contact@@` to the value in the ini specifically, and every asset URL it names is fetchable under ONE prefix; every relative specifier in a served module resolves under that module's own prefix (the reason the hash is per build); `GET /dev.html` and `/assets/<hash>/dev.html` are both 404 and carry none of `dev.html`'s bytes; an asset under a stale hash is 404 while the same name under the current one is 200; the four security headers are on every response, including a 404, a rejected method and both kinds of 500 (one the application decides on, one Warp catches), and no response names the server; the method is rejected per route, so an unknown path is 404 whatever the method while a known one is 405 with its own `Allow`; `/api/catalog` decodes as the RPC `BadgeCatalog` and reflects a price the fixture disabled and one it deprecated, with each offer's computed `total`; `web_dir` serves from disk with `no-store`, picks up an edited stylesheet with no restart and refuses a path outside its directory, percent-encoded traversal included; a token naming no served file fails `resolveWebPage`, which is what startup calls, naming the token; `base_url` rejects a non-loopback `http` URL and accepts `https` and loopback ones in every spelling; `support_contact` rejects `javascript:`, `data:` and relative values and accepts `https`, `http` and `mailto:`. Not automated: that a browser renders and executes the served page (§9 browser pass). The site wizard itself is reviewable after D3.
+**Verify:** In `tests/Bots/BadgeServiceTests.hs`, over HTTP against the running service: `GET /` carries no `@@…@@`, resolves `@@support_contact@@` to the value in the ini specifically, and every asset URL it names is fetchable under ONE prefix; every relative specifier in a served module resolves under that module's own prefix (the reason the hash is per build); `GET /dev.html` and `/assets/<hash>/dev.html` are both 404 and carry none of `dev.html`'s bytes; an asset under a stale hash is 404 while the same name under the current one is 200; the four security headers are on every response, including a 404, a rejected method and both kinds of 500 (one the application decides on, one Warp catches), and no response names the server, while a request Warp rejects before the application sees it keeps the status Warp chose for it (400, not 500 -- providers retry a 5xx); the method is rejected per route, so an unknown path is 404 whatever the method while a known one is 405 with its own `Allow`; `/api/catalog` decodes as the RPC `BadgeCatalog` and reflects a price the fixture disabled and one it deprecated, with each offer's computed `total`; `web_dir` serves from disk with `no-store`, picks up an edited stylesheet with no restart and refuses a path outside its directory, percent-encoded traversal included; a token naming no served file fails `resolveWebPage`, which is what startup calls, naming the token; `base_url` rejects a non-loopback `http` URL and accepts `https` and loopback ones in every spelling; `support_contact` rejects `javascript:`, `data:` and relative values and accepts `https`, `http` and `mailto:`. Not automated: that a browser renders and executes the served page (§9 browser pass). The site wizard itself is reviewable after D3.
 
 #### D5 — URL prefill
 
@@ -1520,13 +1520,15 @@ Append here when a step contradicts this plan: the step id, what was wrong, and
 - **D4 — `index.html` is itself in the served set, and is reachable under the asset prefix.** The step lists it as embedded but does not say whether a token may name it. `build.mjs` resolves `@@index.html@@` (it is in `ROOT_FILES`), so the service must too, or a token would resolve in the development build and fail at startup in production. It is served under the prefix with the same substituted bytes and the same `no-cache` as `/`, so a resolved `@@index.html@@` is a URL that works rather than a 404.
 - **D4 — `@@support_contact@@` is HTML-escaped on the way into the page; `build.mjs` does not escape its placeholder.** The value is operator-controlled and lands inside an `href` attribute, so a quote in it would end the attribute. The asset URLs are not escaped and need no escaping: they are built from a hex digest and a name in the token charset. This is a deliberate divergence from `build.mjs`, which substitutes a fixed development placeholder that contains nothing to escape; the token *resolution rule* — which names resolve, and to what — is identical in both.
 
-- **D4 fix round 1 — Warp's default exception response carried none of the security headers, and that is production-reachable.** `runWebServer` set no `setOnExceptionResponse`, so any exception escaping the application produced Warp's own bare page: `500` with no CSP, no `nosniff`, nothing. Not a `web_dir` curiosity — `withServiceTransaction` catches only `ServiceRollback`, so a database exception in `/api/catalog` leaves the same way, and D6's request decoding plus E3's and F2's webhook verification all add exception surface to this same listener. Fixed with `setOnExceptionResponse` returning the same internal-error response every route uses, and `setOnException` logging through `defaultShouldDisplayException` so replacing the response does not also silence Warp's own report of it. A test drives both halves of the 500 path — a dangling symlink (which the application turns into its own `Left`) and a permission-less directory inside `dist/` (which nothing catches) — and the mutation removing `setOnExceptionResponse` fails only the second, which is the proof that the second was ever reaching Warp.
+- **D4 fix round 1 — Warp's default exception response carried none of the security headers, and that is production-reachable.** `runWebServer` set no `setOnExceptionResponse`, so any exception escaping the application produced Warp's own bare page: `500` with no CSP, no `nosniff`, nothing. Not a `web_dir` curiosity — `withServiceTransaction` catches only `ServiceRollback`, so a database exception in `/api/catalog` leaves the same way, and D6's request decoding plus E3's and F2's webhook verification all add exception surface to this same listener. Fixed by adding the headers TO Warp's own response — `mapResponseHeaders (securityHeaders <>) . defaultOnExceptionResponse` — and by logging through `setOnException`/`defaultShouldDisplayException`, so hardening the response does not also silence Warp's own report of it. **Round 1 first shipped `const internalErrorResponse` here, which was a defect the fix introduced:** that handler also answers requests that never reached the application, and `defaultOnExceptionResponse` is what decides an unparsable request is 400 and an oversized header block 431. Collapsing them to 500 made the listener answer "try again" to a malformed request any client can send — and a provider retries a 5xx webhook, so a malformed E3/F2 callback would have been retried for ever rather than rejected once. Measured on the pinned Warp: 80 KB of header value gets `400 Bad Request` by default and got `500 Internal Server Error` under `const`. The test asserts the headers and the status on that same response, headers first, because the two regress independently. A test drives both halves of the 500 path — a dangling symlink (which the application turns into its own `Left`) and a permission-less directory inside `dist/` (which nothing catches) — and the mutation removing `setOnExceptionResponse` fails only the second, which is the proof that the second was ever reaching Warp.
 - **D4 fix round 1 — the method guard is per route, not server-wide.** Every non-`GET`/`HEAD` request was answered `405` with a server-wide `Allow: GET, HEAD` before `pathInfo` was looked at, which D6 (`POST /api/checkout`), E3 and F2 (`POST /webhooks/*`) would each have had to undo. The routing table now checks the method per route, so a route that exists answers `405` with its own `Allow` and a path that does not exist answers `404` whatever the method — which also stops an unauthenticated caller from mapping which paths are real by method alone.
 - **D4 fix round 1 — `logUnpricedOffers` lives in `Catalog.hs`, beside the function that produces the `Nothing`.** D4's report claimed a module cycle prevented sharing it with `Web/Server.hs`. That was wrong: the function needs only `BadgeCatalog`/`BadgeOffer`/`BadgePrice` and `logWarn`, and `Catalog.hs` is already imported by both `Service.hs` and `Web/Server.hs`. As D4 shipped it, a malformed offer was logged over RPC and read as "unavailable" on the site with nothing said anywhere — on the path most buyers take. Both readers of a database catalog now call it.
 - **D4 fix round 1 — `[web] support_contact` is validated as an absolute `https`/`http` URL with a host, or a `mailto:` address.** D4 escaped the value into the page's `href`, which stops a quote from ending the attribute but says nothing about the scheme: `javascript:…` survives escaping intact and becomes a live link. The operator's own ini is inside the trust boundary, so this is a typo guard rather than a defence — but `base_url` in the same section was validated and its neighbour was not, and that asymmetry was arbitrary. `mailto:` is allowed because it is the one non-web way an operator plausibly publishes support. The loopback list `base_url` accepts is now also case-insensitive and accepts `[::1]`.
 - **D4 — the symlink-inside-`web_dir` vector is measured, not assumed.** A symlink under the `web_dir` directory pointing outside it IS followed and its contents ARE served (200), because the served set is enumerated by walking the directory and a symlink is an ordinary entry of it. Development-only mode, operator's own directory, and the alternative (resolving every path and comparing prefixes on every request, in the mode whose point is that the directory changes underneath) buys nothing against an operator who can equally well copy the file in. Recorded so the next reader does not have to rediscover it: the traversal refusal D4 tests is about REQUEST paths, which cannot escape at all, not about what the directory itself points at.
 - **D4 — a `dist/index.html` is a startup error in the service and a silent override in `build.mjs`.** The service refuses a served set with two files of one name (`index.html` from `web/` and one copied into `dist/`), naming the collision; `build.mjs`'s `writeDevHtml` would simply let the `dist/` one win its `ROOT_FILES` entry. A divergence in the safe direction — the service fails loudly where the build script would quietly serve the wrong file — and unreachable unless someone adds `assets/index.html`, which `copyAssets` already refuses when it collides with a compiled module.
 
+- **D4 — `logUnpricedOffers` is asserted at neither call site, and was not before the move either.** No test produces an offer that is pinned to a returned price and still has no total, so the only evidence that either the RPC handler or `/api/catalog` logs it is the call site and a clean compile. The gap is pre-existing and symmetric — the RPC side was never asserted either, so fix round 1's move weakened nothing — but it is a gap, and the fixture is not free: it needs a `badge_offers` row with `free_months >= months` (or a discount over 100) written straight to the database, past `seedCatalog`'s own `requireTotal` guard, and an assertion over the log rather than over a response. B10 or H4 is the natural home; whoever takes it should assert both call sites, since the point of moving the function was that the site path logs too.
+
 ## 10. End-to-end verification
 
 After F5:

tests/Bots/BadgeServiceTests.hs

@@ -3794,12 +3794,14 @@ newWebManager :: IO HTTP.Manager
 newWebManager = HTTP.newManager HTTP.defaultManagerSettings
 
 -- 'HTTP.parseRequest' (unlike 'parseUrlThrow') installs no status check, so a 404 comes back as
--- a response to assert on rather than as an exception. The path is sent as written, which is what
--- lets the traversal cases below reach the server percent-encoded.
-webGet :: HTTP.Manager -> String -> IO WebResponse
-webGet mgr url = do
+-- a response to assert on rather than as an exception. The request is sent as written -- the path
+-- is not re-encoded, which is what lets the traversal cases below reach the server
+-- percent-encoded, and the headers are not policed, which is what lets the malformed-request case
+-- reach it at all.
+webRequestWith :: (HTTP.Request -> HTTP.Request) -> HTTP.Manager -> String -> IO WebResponse
+webRequestWith f mgr url = do
   req <- HTTP.parseRequest url
-  r <- HTTP.httpLbs req mgr
+  r <- HTTP.httpLbs (f req) mgr
   pure
     WebResponse
       { wrStatus = statusCode (HTTP.responseStatus r),
@@ -3807,17 +3809,12 @@ webGet mgr url = do
         wrBody = LBC.unpack (HTTP.responseBody r)
       }
 
--- The same with a method of the caller's choosing, for the per-route method assertions.
+webGet :: HTTP.Manager -> String -> IO WebResponse
+webGet = webRequestWith id
+
+-- With a method of the caller's choosing, for the per-route method assertions.
 webRequest :: BC.ByteString -> HTTP.Manager -> String -> IO WebResponse
-webRequest method mgr url = do
-  req <- HTTP.parseRequest url
-  r <- HTTP.httpLbs req {HTTP.method = method} mgr
-  pure
-    WebResponse
-      { wrStatus = statusCode (HTTP.responseStatus r),
-        wrHeaders = HTTP.responseHeaders r,
-        wrBody = LBC.unpack (HTTP.responseBody r)
-      }
+webRequest method = webRequestWith $ \req -> req {HTTP.method = method}
 
 webHeader :: WebResponse -> BC.ByteString -> String
 webHeader r name = maybe "" BC.unpack $ lookup (CI.mk name) (wrHeaders r)
@@ -3955,6 +3952,14 @@ testBadgeServiceWebSecurityHeadersEverywhere ps =
     -- HEAD is allowed on every route GET is
     headIndex <- webRequest "HEAD" mgr webUrl
     wrStatus headIndex `shouldBe` 200
+    -- A request Warp rejects before the application ever sees it: an oversized header block. Both
+    -- halves are asserted, and the headers first, because they can regress independently --
+    -- hardening this response by REPLACING Warp's own throws away the status it chose, and a
+    -- listener that answers 500 to a malformed request tells every client, and every provider
+    -- retrying a 5xx webhook (E3, F2), to send it again.
+    malformed <- webRequestWith (\req -> req {HTTP.requestHeaders = [("X-Overlong", BC.replicate 80000 'a')]}) mgr webUrl
+    assertSecurityHeaders "malformed request" malformed
+    wrStatus malformed `shouldBe` 400
 
 -- /api/catalog answers from the DATABASE through catalogTotals, never from Catalog.hs's
 -- defaults: the fixture disables one seeded price and deprecates the other, so the default