Skip to content

fix(mrml-core): stop the http include loader following redirects - #693

Open
jdrouet wants to merge 2 commits into
mainfrom
fix/http-loader-no-redirects
Open

jdrouet wants to merge 2 commits into
mainfrom
fix/http-loader-no-redirects

Conversation

@jdrouet

@jdrouet jdrouet commented Sep 13, 2026

Copy link
Copy Markdown
Owner

HttpIncludeLoader checks the origin of the URL in the mj-include path attribute, 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:

direct fetch of B/secret.mjml     -> blocked, not on the allow list
fetch of A/partial.mjml, 302 -> B -> returned B's body

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: UreqFetcher via max_redirects(0), the two reqwest fetchers via redirect::Policy::none(). Neither library errors on a 3xx by itself — ureq documents that it returns the response, and reqwest's error_for_status only covers 4xx/5xx — so there is an explicit is_redirection() check that turns it into an InvalidData error. A redirect must not become an empty template.

No configuration knob for this. HttpFetcher and AsyncHttpFetcher are 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.

Signed-off-by: Jeremie Drouet <jeremie.drouet@gmail.com>
@jdrouet-datadog-monitor

jdrouet-datadog-monitor Bot commented Sep 13, 2026

Copy link
Copy Markdown

Pipelines

⚠️ Warnings

⚠️ Your PR has warnings. Please review the issues below.

🚦 2 Pipeline jobs failed

mrml-wasm main | testing — 🔧 Needs a code fix, caused by this PR

View in Datadog · View in GitHub Actions

Compilation error in http_loader.rs:91: cannot find redirect in reqwest due to it being gated out for the wasm32 target.

test building demo | test — 🔧 Needs a code fix, caused by this PR

View in Datadog · View in GitHub Actions

Compilation error: cannot find redirect in reqwest at packages/mrml-core/src/prelude/parser/http_loader.rs:91

📋 Copy fix prompt
CI on my pull request is failing. Help me find and fix the root cause of each failing job below — they were flagged as caused by changes in this PR, so focus on the diff. For each job, explain the failure and propose a fix.

Before you start, set up the Datadog software-delivery tooling so you can
query the CI data yourself:

1. Check whether you already have the Datadog software-delivery MCP tools
   (e.g. a `search_datadog_ci_pipeline_events` tool) and the `unblock-pr` skill.
2. If either is missing, STOP and ask me for permission before installing
   anything. Do not install or run anything until I have said yes.
3. Only with my explicit approval, set up the Datadog software-delivery MCP
   server and skills by following:
     https://docs.datadoghq.com/getting_started/software_delivery_mcp_tools/
   then restart so the skill is picked up.
4. If I decline, skip all of the above and work from the context below alone.

Then run /unblock-pr — it will pull the CI data itself. The job context below is what we already know.

If /unblock-pr is not available — because I declined the setup above, or it did not install — work from the context below instead.

Datadog has already classified this failure as caused by changes in this PR.
Take that as given and work the fix:

1. Locate the change. Diff this branch against its base and find the change
   that produces this error. Explain the mechanism, don't just name a file:
     git fetch origin && git diff $(git merge-base origin/main HEAD)...HEAD
2. Reproduce it locally. Run the failing job's command or test before
   proposing anything.
3. Propose the smallest fix that addresses the root cause — not a workaround,
   not a broadened assertion, not a disabled or skipped test.
4. Re-run the same command to confirm, and say exactly what you ran.
5. If the failure turns out to be intermittent rather than deterministic, say
   so plainly instead of "fixing" it — that is a flaky test, and patching it
   hides the problem.

If the right move is to re-run the job rather than change code, use the job
link in the context below. For GitHub Actions: `gh run rerun <run-id> --failed`,
where the run ID is the number after `/runs/` in that URL (not the trailing
number, which is the job ID).

Branch: fix/http-loader-no-redirects

mrml-wasm main | testing
Commit: 152d3739f1f8cf8ef286791c2889ad215a5a777e
Error (code / build):
Compilation error in http_loader.rs:91: cannot find `redirect` in `reqwest` due to it being gated out for the wasm32 target.
CI job: https://github.com/jdrouet/mrml/actions/runs/34753292339/job/103713340376

test building demo | test
Commit: 152d3739f1f8cf8ef286791c2889ad215a5a777e
Error (code / build):
Compilation error: cannot find `redirect` in `reqwest` at packages/mrml-core/src/prelude/parser/http_loader.rs:91
CI job: https://github.com/jdrouet/mrml/actions/runs/34753292280/job/103713340362

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: 152d373 | Docs | View more details | Give us feedback!

@github-actions

Copy link
Copy Markdown
Contributor

📈 This is how your metrics evolved 📉


…wasm

Signed-off-by: Jeremie Drouet <jeremie.drouet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant