Conversation
Signed-off-by: Jeremie Drouet <jeremie.drouet@gmail.com>
|
Contributor
|
📈 This is how your metrics evolved 📉 |
…wasm Signed-off-by: Jeremie Drouet <jeremie.drouet@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HttpIncludeLoaderchecks the origin of the URL in themj-include pathattribute, once, and then hands the raw URL to a fetcher that follows up to 10 redirects without checking any of them. The allow list only ever constrained the first hop.Reproduced with two local servers, allow list containing origin A only:
So any allow-listed host — or, under
allow_all()or a deny list, any host at all — can pivot the fetch to an address the caller never approved, and the response lands in the rendered email. That is an SSRF primitive in the one component whose job is to be a boundary.All three bundled fetchers now refuse redirects:
UreqFetcherviamax_redirects(0), the two reqwest fetchers viaredirect::Policy::none(). Neither library errors on a 3xx by itself — ureq documents that it returns the response, and reqwest'serror_for_statusonly covers 4xx/5xx — so there is an explicitis_redirection()check that turns it into anInvalidDataerror. A redirect must not become an empty template.No configuration knob for this.
HttpFetcherandAsyncHttpFetcherare public, so a caller who genuinely needs redirects can implement one and own that decision.Three regression tests, one per fetcher, each asserting the redirect target is never fetched. They were run against the unfixed code first and all three failed with
unwrap_err() on an Ok value: "<mj-text>Secret</mj-text>"— the bypass itself — before passing with the fix.Tests 621 -> 624. fmt, check and clippy clean under
-Dwarnings.Found by a correctness audit; more fixes from the same pass are coming, two of which need a major bump.