Fix verified findings in middlewares and helpers - #49
Conversation
Coverage Report for CI Build 32196839310Coverage increased (+0.4%) to 97.439%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
c484ec4 to
e8b27b1
Compare
Previously, Metrics() without arguments passed an empty list to matchSourceIP, which returns false for every request, so the middleware answered 403 to all callers instead of serving expvar as documented.
Previously, the sentinel was caught like any other panic and turned into a 500, so net/http never learned it had to abort the response and close the connection. Only the stack trace was suppressed for it.
Previously, the status was written before encoding started, so a value that fails to marshal left the caller with an error after a 2xx was already on the wire and no way to replace it with a 500. Encoding into a buffer first makes a returned error mean nothing was written.
Previously, customFS.Open discarded the handle used to probe for index.html and returned without closing the directory on the Stat and missing-index error paths. http.FileServer closes only the file it receives, so each directory request leaked a descriptor until the finalizer ran.
The middleware changes the representation based on Accept-Encoding but never told caches about it, so a shared cache could hand gzip bytes to a client that asked for none.
Previously, a failing io.ReadAll skipped the word check entirely and passed a partially consumed body to the handler, so a truncated or aborted request went through unchecked. Such a request now gets a 400 and never reaches the handler.
x/crypto stays at v0.48.0, the last release that still builds with the go 1.24.0 declared in go.mod.
Buffering the encode moved the write behind a discarded error, so a client disconnect or short write left the caller with a nil error where json.Encoder.Encode used to surface it. The write error is returned again, while an encoding failure still comes back before anything is committed.
5b0489e to
9ee60a7
Compare
|
the eight other fixes here are good, but the Metrics change can't go in as written.
pls keep empty args as deny-all and add an explicit option for the unrestricted case, something like |
…en case Serving expvar to everyone when no ips are given turns a dependency bump into an exposed endpoint for anyone who mounted Metrics() bare, and expvar publishes cmdline with the process flag values. Empty arguments deny every request again, as before, and callers who want the endpoint public now ask for it by name.
|
Inverted the Metrics commit rather than dropping it: Retargeting #51 and #52 to master now so neither is blocked by this branch. |
3 participants
A batch of fixes for issues verified against the current code, one commit each. Everything here is non-breaking: no exported signature changes, and behaviour changes only on paths that were already wrong.
The breaking CORS change that started out in this branch is now #52, stacked on top of this one, and the gzip negotiation rewrite is #51. This PR can go in on its own.
Fixes
Metrics()with no arguments passed an empty list tomatchSourceIP, which returns false for every request, so the middleware answered 403 to everyone instead of serving expvar as documented.Recoverercaughthttp.ErrAbortHandlerlike any other panic and turned it into a 500, sonet/httpnever learned it had to abort the response and close the connection. The sentinel is now re-panicked untouched.EncodeJSONwrote the status before encoding, so a value that fails to marshal left the caller holding an error after a 2xx was already committed, with no way to replace it. Encoding now happens into a buffer first, which makes a returned error mean nothing was written.customFS.Opendiscarded the handle used to probe forindex.html, and returned without closing the directory on theStatand missing-index paths. Sincehttp.FileServercloses only the file it receives, every directory request leaked a descriptor until the finaliser ran.Gzipnever sentVary: Accept-Encoding, so a shared cache could hand gzip bytes to a client that asked for none. The header is now set whether or not the response ends up compressed.BlackWordsskipped the word check entirely whenio.ReadAllfailed and passed a partially consumed body to the handler, so a truncated or aborted request went through unchecked. Such a request now gets a 400 and never reaches the handler. The middleware still has no cap of its own, and the README now points at composingSizeLimitin front of it to bound the allocation, rather than introducing a second hidden limit that would fight with the caller's own.Modules
x/cryptomoves to v0.48.0, the last release that still builds with the go 1.24.0 declared in go.mod, and testify to v1.12.0.go fixwas applied across the tree.Every fix ships with a test that fails without it. The suite passes on both legs of the CI matrix, which exercise the two CSRF implementations.