fix(middlewares): route preset fetches through safe HTTP transport (#615)#624
Conversation
`middlewares/preset.go::loadFromURL` previously called `http.DefaultClient.Do(req)` for remote preset fetches when `webhook-allow-remote-presets = true`. The TLS posture was implicit (Go stdlib defaults — safe today, but untested) and inconsistent with the webhook delivery stack, which routes through `TransportFactory()` in `middlewares/webhook_security.go`. Route preset fetches through the same `TransportFactory()` so the TLS / proxy posture is centrally configured, and add three regression tests: - Self-signed `httptest.NewTLSServer` is rejected at handshake. - `TransportFactory()` does not enable `InsecureSkipVerify`. - `loadFromURL` actually obtains its transport via `TransportFactory()` (would catch a future revert to `http.DefaultClient`). The request-level deadline is already set by `context.WithTimeout`, so no `Client.Timeout` is added. Fixes #615 Signed-off-by: Sebastian Mendel <[email protected]>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
github-actions
Bot
added
documentation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #624 +/- ##
==========================================
- Coverage 87.38% 87.34% -0.04%
==========================================
Files 88 88
Lines 10722 10723 +1
==========================================
- Hits 9369 9366 -3
- Misses 1112 1115 +3
- Partials 241 242 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the remote preset fetching logic to use a centralized TransportFactory() instead of http.DefaultClient, ensuring consistent TLS verification and security posture. New regression tests have been added to verify certificate rejection and transport configuration. A review comment suggests improving performance by reusing the http.Client instance within the PresetLoader to allow for proper connection pooling.
There was a problem hiding this comment.
Pull request overview
This PR hardens remote preset fetching in the middlewares package by routing HTTP(S) downloads through the same centrally configured HTTP transport used for webhook delivery, and adds regression tests to pin the intended TLS posture.
Changes:
- Switch remote preset downloads in
PresetLoader.loadFromURLfromhttp.DefaultClientto anhttp.ClientusingTransportFactory(). - Add security-focused regression tests ensuring TLS verification remains enforced and that
loadFromURLactually usesTransportFactory(). - Document the change in
CHANGELOG.mdunder Security.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
middlewares/preset.go |
Uses TransportFactory() for remote preset HTTP requests instead of http.DefaultClient. |
middlewares/preset_security_test.go |
Adds regression tests for TLS verification posture and transport-factory usage. |
CHANGELOG.md |
Adds an Unreleased security note describing the transport change and new regression guards. |
- Rename TestPresetLoader_LoadFromURL_RejectsUntrustedTLS to _PinsTLSVerificationPosture. The original name implied it was the #615 regression guard, but http.DefaultClient already rejects self-signed certs - the test passes both before and after the fix (test reviewer correctly flagged the framing as misleading). The actual regression guard is UsesTransportFactory; reflect that in the docstring. - Strengthen UsesTransportFactory with an observingRoundTripper that proves the factory-returned transport actually serves a request, not just that the factory function is called. Closes the future- refactor gap test reviewer identified (factory called but result ignored). - Migrate defer SetXxxForTest -> t.Cleanup so cleanup runs on t.FailNow / panic, not just on a clean return. Signed-off-by: Sebastian Mendel <[email protected]>
|
There was a problem hiding this comment.
Automated approval for maintainer PR
All automated quality gates passed. See SECURITY_CONTROLS.md for compensating controls.
Summary
middlewares/preset.go::loadFromURLpreviously fetched remote presets viahttp.DefaultClient.Do(req). The TLS posture was implicit (Go stdlib defaults — safe today, but untested) and inconsistent with the webhook delivery stack, which routes throughmiddlewares.TransportFactory()inmiddlewares/webhook_security.go.This PR routes preset fetches through the same
TransportFactory()so the TLS / proxy posture is centrally configured and pinned by regression tests.Changes
middlewares/preset.go— replacehttp.DefaultClient.Do(req)with&http.Client{Transport: TransportFactory()}. The request-level deadline is already set bycontext.WithTimeout, so noClient.Timeoutis added.middlewares/preset_security_test.go(new) — three regression tests:TestPresetLoader_LoadFromURL_RejectsUntrustedTLS— self-signedhttptest.NewTLSServeris rejected at handshake (would fail if a permissive transport ever slips in).TestTransportFactory_SafePosture—TransportFactory()does not enableInsecureSkipVerify.TestPresetLoader_LoadFromURL_UsesTransportFactory—loadFromURLactually obtains its transport viaTransportFactory(). Would catch a future revert tohttp.DefaultClient.CHANGELOG.md— entry under[Unreleased] → Security.Behavior change
None for operators on default config. The pre-fix code already used safe TLS defaults; this change makes the contract explicit and centralizes future tuning.
Test plan
go test -race ./middlewares/— passes (all existing + 3 new tests).golangci-lint run --timeout=3m ./middlewares/— 0 issues.go build ./...— clean.UsesTransportFactorytest FAILS on pre-fix code (proves the regression guard is meaningful).Fixes #615