Correct proxyabl SSRF marketing to match what actually runs (#43)#48
Open
davidcrowe wants to merge 1 commit into
Open
Correct proxyabl SSRF marketing to match what actually runs (#43)#48davidcrowe wants to merge 1 commit into
davidcrowe wants to merge 1 commit into
Conversation
proxyabl-core's assertUrlSafe/executeProxyRequest (DNS-resolving private-IP blocking, host allowlist, redirect:"manual", size/timeout caps) are exported and unit-tested but have zero consumers outside their own tests. The README and docs implied the gateway proxy runs this engine; it does not. Decision: correct the marketing (the issue's "Or" option), not wire the engine into the router — because the router's model is genuinely different, not a bug to paper over: - The three user-facing router fetches (mcpHandler, restToolHandler, proxyHandler) all forward to an operator-CONFIGURED backend (functionsBase / proxy target). The caller controls the tool name / path, not the host, which is pinned to the configured backend via a host+protocol equality check. - executeProxyRequest is built for the opposite case — proxying arbitrary, caller-influenced URLs against an allowlist. Dropping it into the router would (a) reject configured backends that are legitimately internal (private-IP blocking), (b) change response semantics (it buffers+caps and throws on non-2xx instead of passing upstream status/body through), and (c) block upstream redirects the router may rely on. That is a breaking change to live gateway behavior to satisfy a marketing line — backwards. - OSS has no arbitrary-URL / custom-connector path today (executeProxyRequest's ancestor served custom HTTP tools that live only in prod), so the engine has no natural OSS consumer yet. Changes (docs only): - README module table: proxyabl row now describes the router accurately (configured-backend, path-allowlisted, identity-aware) and points to the proxyabl-core SSRF primitive. - README: added an "SSRF note" spelling out the two threat models. - docs/packages.md: split the proxyabl entry into the core SSRF-safe primitive vs the pass-through Express router, and noted the router does not route through the engine + the convergence direction. - proxyabl-core/README: added a scope note so a reader (or a reviewer grepping assertUrlSafe) sees the primitive is intentional and standalone, not dead. The proxyabl-core README's own SSRF description was already accurate for the primitive and is unchanged except for the note. The real fix — one canonical proxyabl-core engine consumed by both the OSS router and prod's fork — is the convergence epic; flagged, not attempted here.
This was referenced Jul 23, 2026
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.
Fixes #43.
Recommendation: correct the marketing (the issue's "Or" option), don't force-wire
proxyabl-core'sassertUrlSafe/executeProxyRequest(DNS-resolving private-IP blocking, host allowlist,redirect:"manual", size/timeout caps) are exported and unit-tested but have zero consumers outside their own tests. The README/docs implied the gateway proxy runs this engine — it does not.I investigated the "preferred" wiring option and concluded it's the wrong call here, because the router's model is genuinely different (not a bug to paper over):
The three user-facing router fetches —
mcpHandler,restToolHandler,proxyHandler— all forward to an operator-configured backend (functionsBase/ proxy target). The caller controls the tool name / path, not the host, which is pinned to the configured backend via a host+protocol equality check.executeProxyRequestis built for the opposite case — proxying arbitrary, caller-influenced URLs against an allowlist. Dropping it into the router would:That's a breaking change to live gateway behavior in order to satisfy a marketing line — backwards.
OSS has no arbitrary-URL / custom-connector path today (
executeProxyRequest's ancestor served custom HTTP tools that live only in prod), so the engine has no natural OSS consumer yet.So this closes the credibility gap the honest way: make the docs match reality.
Changes (docs only)
proxyablrow now describes the router accurately (configured-backend, path-allowlisted, identity-aware) and points to theproxyabl-coreSSRF primitive; added an SSRF note spelling out the two threat models.proxyablentry into the core SSRF-safe primitive vs the pass-through Express router, and stated the router does not route through the engine (+ the convergence direction).assertUrlSafeand finding no consumers — sees the primitive is intentional and standalone, not abandoned.proxyabl-core's own SSRF description was already accurate for the primitive and is otherwise unchanged.The real fix (flagged, not attempted here)
One canonical
proxyabl-coreengine consumed by both the OSS router and prod's fork (integrations/custom/security.ts) — the proxyabl convergence epic. When an arbitrary-URL connector path lands in OSS,executeProxyRequestis the thing to wire there. Separately, addingredirect:"manual"to the router's fetches is a defensible standalone hardening, but it's a behavior change that deserves its own tested PR, not a rider on a docs correction.If you'd rather take the deeper wiring now, happy to — but I'd want it as its own change with real upstream testing. Not merged.