fix(security): route all plugin egress through safeFetch#1441
Open
joelorzet wants to merge 6 commits into
Open
fix(security): route all plugin egress through safeFetch#1441joelorzet wants to merge 6 commits into
joelorzet wants to merge 6 commits into
Conversation
Raw fetch() in plugin step and connection-test files bypassed the SSRF
guard in lib/safe-fetch.ts, so a user- or attacker-controlled destination
could reach link-local (169.254.169.254) and RFC1918 hosts even with
SAFE_FETCH_ENFORCE on. Replace every bare fetch() under plugins/ with
safeFetch(url, { plugin }) so the guard validates each request and blocks
attribute to the calling plugin.
Prioritized the two user-controlled-host paths: the discord webhookUrl and
the blockscout custom-instance URL. Also harden the discord webhook check:
the old substring match on "discord.com/api/webhooks/" passed a URL that
carried that string in its path while pointing the host at an internal
address (e.g. http://169.254.169.254/discord.com/api/webhooks/x). Validate
by parsed hostname over https with an /api/webhooks/ path instead.
Update blockscout-steps unit tests to mock safeFetch, and add a discord
send-message test covering the bypass URLs and the safeFetch wiring.
Add a lint-job step that git-greps for bare fetch()/axios/http.request under plugins/ (excluding *.test.ts, *.md, *.txt) and fails the build, so plugin egress cannot regress off safeFetch. Pattern avoids \b so it runs the same on the BSD and GNU regex engines.
Update the plugin generator template, authoring guides, and agent/command
definitions to use safeFetch(url, { plugin }) instead of raw fetch(), so
newly created plugins route egress through the SSRF guard and pass the
new CI egress check by default.
joelorzet
requested review from
a team,
OleksandrUA,
eskp and
suisuss
and removed request for
a team
The build failed because lib/safe-fetch.ts is "server-only" and the connection-test files (plugins/*/test.ts) are reachable from the client-bundled plugin registry (plugins/index.ts), so importing safeFetch into them pulled server-only into the client graph. Connection tests run server-side but cannot import the server-only guard from the client-reachable registry. Revert the nine test.ts files to the raw fetch global, exclude plugins/*/test.ts from the egress CI guard, and note the step-file vs connection-test distinction in the scaffolding and authoring guides. Workflow step files (the SSRF surface this ticket targets) remain routed through safeFetch and are unaffected.
Connection-test files (plugins/*/test.ts) are reachable from the client-bundled plugin registry, so they cannot import the server-only SSRF guard and fetch user-supplied instance URLs (e.g. blockscout's BLOCKSCOUT_API_URL) with raw fetch. A user could point that at 169.254.169.254 or an RFC1918 host and use the Test Connection result as a reachability oracle. Validate every user-supplied url-type field on the server in handlePluginTest, before the test runs, via assertUrlIsPublic -- mirroring the assertConnectionUrlIsPublic pre-flight already used for database connection tests. Generic across all plugins; no change to the client-reachable test.ts files.
Document, in the handlePluginTest guard comment and the plugin authoring guide, that assertUrlIsPublic is always-on (it does not honor SAFE_FETCH_ENFORCE), so Test Connection blocks a localhost/private instance URL in local dev even though workflow execution against it works under safeFetch shadow mode. Also normalize prose to avoid double hyphens.
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
1 participant
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.
Summary
Closes the raw-fetch SSRF bypass: plugin step files called bare
fetch(), so their egress never reached the SSRF guard inlib/safe-fetch.ts. WithSAFE_FETCH_ENFORCEon in staging/prod, a user- or attacker-controlled destination could reach link-local (169.254.169.254IMDS) and RFC1918 hosts through these paths.Changes
safeFetch. Route every barefetch()in plugin step/core files throughsafeFetch(url, { plugin })so the guard validates each request (and every redirect hop) and attributes blocks to the calling plugin. Covers the two user-controlled-host paths (discordwebhookUrl, blockscout custom-instance URL) plus the fixed-host defense-in-depth sites.fetch, guarded on the server. Theplugins/*/test.tsconnection-test files are reachable from the client-bundled plugin registry, so they cannot import theserver-onlyguard (doing so breaks the client build). Instead,handlePluginTest(lib/db/test-connection.ts) validates every user-suppliedtype: "url"field withassertUrlIsPublicbefore the test runs, mirroring theassertConnectionUrlIsPublicpre-flight already used for database connection tests. This closes the blockscout custom-instance SSRF on the "Test Connection" path generically for all plugins. The guard is always-on (it does not honorSAFE_FETCH_ENFORCE), consistent with the DB/RPC pre-flights, so in local dev alocalhostinstance URL is blocked at "Test Connection" even though execution against it works under shadow mode.discord.com/api/webhooks/, which a URL likehttp://169.254.169.254/discord.com/api/webhooks/xpasses by carrying that string in its path while the host is internal. It now parses the URL and requires https plus adiscord.com/discordapp.com(or subdomain) host plus an/api/webhooks/path, rejecting off-host URLs before any request.safeFetchremains the network-layer backstop.Forbid raw network egress in plugins(in thelintjob): git-greps for barefetch()/axios/http.requestin plugin step files so this cannot regress. Vitest specs and the client-reachabletest.tsconnection-test files are excluded.safeFetchin step files (and document the connection-test exception), so new plugins are compliant by default.Tests
tests/unit/blockscout-steps.test.tsupdated to mocksafeFetch(step code no longer touches thefetchglobal).tests/unit/discord-send-message.test.ts: the bypass URLs, non-https, and wrong-path are rejected with no egress; a valid webhook routes throughsafeFetchwith{ plugin: "discord" }.tests/unit/plugin-test-url-guard.test.ts: a connection-test url field pointing at an internal address is blocked before the test runs; a public URL is allowed; an empty field is skipped.safe-fetch.test.tsalready provessafeFetch/assertUrlIsPublicblock169.254.169.254/RFC1918 in enforce mode, which these paths now inherit.Full unit suite,
pnpm check, andpnpm type-checkpass; production build compiles cleanly.