Skip to content

fix: feed shop allowlists into the SDK URL-safety validator (remote profile fetches always rejected) - #150

Draft
Robin Schulte (relativvv) wants to merge 1 commit into
mainfrom
fix/url-safety-validator-shop-allowlists
Draft

fix: feed shop allowlists into the SDK URL-safety validator (remote profile fetches always rejected)#150
Robin Schulte (relativvv) wants to merge 1 commit into
mainfrom
fix/url-safety-validator-shop-allowlists

Conversation

@relativvv

@relativvv Robin Schulte (relativvv) commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Problem

Any UCP platform calling a correctly configured shop gets a 422 on every runtime request:

Profile host "platform.example" is not allowed.

Setting the sales channel's remoteProfileAllowlist (admin or ucp:config:set) does not help — the rejection persists no matter what is configured.

Root cause

There are two profile-host checks with different config sources:

  1. DefaultHttpRequestContextFactory::assertSafeProfileUri() — uses the per-channel runtime configuration (remoteProfileAllowlist). This one works.
  2. UrlSafetyValidator inside HttpAgentProfileFetcher (also injected into DefaultOrderWebhookDispatcher) — compiled from the static bundle config ucp_sdk.allowed_profile_hosts, which this plugin never populates. It therefore always runs with an empty allowlist and rejects every remote profile fetch, right after check 1 passes.

The two checks are easy to conflate because their error messages are similar; the observed message (Profile host "…" is not allowed., without the "Allowed hosts: …" suffix) is the fetch-time validator's empty-list rejection.

Order webhooks are affected by the same empty allowlist through the shared service.

Fix

  • ConfiguredUrlSafetyValidatorFactory builds the SDK validator from what the shop already manages: the union of every sales channel's (and the global scope's) remoteProfileAllowlist + platformAllowlist, plus all sales-channel domain hosts (parity with the runtime fallback so own-host profile URIs stay fetchable).
  • ReplaceSdkUrlSafetyValidatorPass swaps the SDK bundle's compiled definition for the factory-built one (compiler pass so the override wins regardless of bundle load order, same pattern as ReplaceSdkSigningKeyCommandsPass).

The union is safe: per-channel policy is still enforced by assertSafeProfileUri() before any fetch; the validator remains the SSRF defense-in-depth layer (blocked hosts/IP checks unchanged). profile_fetching_development_mode is passed through unchanged.

Verification

Reproduced and verified against a live shop (Shopware PaaS) with a channel-scoped remoteProfileAllowlist:

  • before: POST /ucp/v1/catalog/search with UCP-Agent: profile="https://<allowed-host>/.well-known/ucp" → 422 Profile host … is not allowed.
  • after: same request → 200 success; full platform→shop searchProducts round-trip returns products.
  • a non-allowlisted profile host is still rejected (401) by the per-channel check.

6 new unit tests (factory union/dedup/lowercase, dev-mode passthrough, DB-failure fallback; pass replacement + no-op guards). php-cs-fixer/phpstan clean on touched files.

Design note (why plugin-level, and the SDK alternative)

The root architectural gap arguably sits in the SDK: DefaultHttpRequestContextFactory has already resolved a per-request RuntimeConfiguration (with allowedProfileHosts) when it calls AgentProfileFetcherInterface::fetch(string $uri) — but the fetch API has no config slot, so the fetcher falls back to a global static list. The deepest fix would be SDK-level: validate the fetch against the per-request configuration. This PR is the correct fix at the plugin layer for SDK ^0.0.2 as-is, since the SDK deliberately exposes allowed_profile_hosts as an integration-owned knob and the plugin is the layer that owns the (dynamic, DB-backed) merchant config.

Why the cross-channel union is safe: both consumers of the validator run after a per-channel policy gate — the profile fetch only happens once assertSafeProfileUri() passed for the resolved channel, and webhook targets come from a RequestContext created through that same gate (or from webhookUrlOverride, which UcpConfig validates against the same channel's allowlists at write time). The validator's SSRF checks (blocked hosts, private/reserved IP ranges, ports, DNS) are unchanged.

Trade-offs to be aware of: the validator is built lazily per container lifecycle (a few small queries on first UCP/webhook use per request cycle; long-running workers pick up allowlist changes on the next worker cycle).

The SDK bundle compiles its UrlSafetyValidator from the static
ucp_sdk.allowed_profile_hosts semantic config, which this plugin never
populates. The validator therefore always ran with an empty allowlist
and rejected every remote platform-profile fetch (and order-webhook
target) with 'Profile host ... is not allowed' - no matter what the
merchant configured. The per-channel remoteProfileAllowlist only guards
the earlier assertSafeProfileUri() check in the request-context factory
and never reaches the fetcher, so any real platform calling a correctly
configured shop got a 422 on every UCP runtime request.

Fix: a compiler pass replaces the SDK's compiled definition with one
built at runtime by ConfiguredUrlSafetyValidatorFactory, which unions
every sales channel's (and the global scope's) remoteProfileAllowlist
and platformAllowlist plus all sales-channel domain hosts. Per-channel
policy stays enforced upstream by assertSafeProfileUri(); the validator
keeps acting as the SSRF defense-in-depth layer behind it, and the
webhook dispatcher picks up the same allowlist.
@BrocksiNet

Copy link
Copy Markdown
Contributor

Read this properly while adjudicating the overlap between our UCP PRs. No conflict with #159/#160services.php is the only shared file and ours doesn't touch it (UcpMcpToolContext is autowired). Three things worth putting on the record.

1. Your design note asks a question we can now answer differently

You wrote:

The deepest fix would be SDK-level: validate the fetch against the per-request configuration. This PR is the correct fix at the plugin layer for SDK ^0.0.2 as-is.

That reasoning was right when you wrote it on 28 July. It has since changed: the SDK has moved three times this week — #111 and #112 are merged, #113 prepares 0.0.4, and the plugin will require >=0.0.4 <0.1.0 anyway. So "we cannot change the SDK" is no longer the constraint it was.

The SDK-level shape you describe — AgentProfileFetcherInterface::fetch() taking the RuntimeConfiguration that DefaultHttpRequestContextFactory has already resolved — would delete the whole problem rather than route around it: no compiler pass, no cross-channel union, no factory querying every channel's config on first use, and no divergence between the two checks that made this so hard to diagnose. It would also fix a second defect in the same validator that this PR cannot reach (see 3).

I'm not asking you to redo it. I'm asking which we want, because the two are genuinely different bets:

  • Merge this now — fixes a live 422 on real shops today, at the cost of a plugin-side override that becomes redundant when the SDK gains a config slot.
  • Do it in the SDK — one release cycle slower, but removes the second allowlist source entirely and takes O7 with it.

Given it's a live breakage on PaaS, I lean toward merging this now and filing the SDK change, with this PR's compiler pass documented as a stopgap to remove when the SDK lands it. That way nobody waits and the architecture still converges.

2. The union is the reviewable claim, and it deserves a named reviewer

Your argument is that the cross-channel union is safe because a per-channel gate always runs first. That holds cleanly for profile fetches — assertSafeProfileUri() runs before any fetch. It's less direct for webhooks: those targets come from a RequestContext created through the same gate or from webhookUrlOverride, which UcpConfig validates at write time. So for the webhook path the union is effectively the SSRF boundary at dispatch time, and the guarantee is "it was validated when it was written".

That's probably fine — webhookUrlOverride is admin-only and validated against the owning channel's allowlists — but it is the one claim in this PR where being wrong is a security bug rather than a 422, and it shouldn't be reviewed by whoever is in a hurry. Worth an explicit sign-off from someone with the SSRF context.

3. It does not fix O7, and I want to correct myself publicly

I had this PR filed in my notes as "probably the .localhost fix". That was wrong, and I'd rather say so than let it sit.

There are two independent defects in UrlSafetyValidator:

  • Where the allowlist comes from — static bundle config the plugin never populates, so it's always empty. That's this PR.

  • isLocalHost() accepting only bare names:

    return in_array($host, ['localhost', '127.0.0.1', '::1'], true);

    No .localhost suffix. So a dev lane on trunk.localhost:8088 is refused with "Plain http is only allowed for local development hosts" no matter how profile_fetching_development_mode is set. RFC 6761 reserves the whole .localhost TLD for loopback, so the check is stricter than the spec it implements. Every Shopware dev setup serves <shop>.localhost, which is why we work around it by pointing UCP_PROFILE_URI at the container-internal http://localhost:8000.

That one is SDK code, so this PR cannot reach it — and it's the natural companion to the fetch-config change in 1. If we go the SDK route, both land together.

Happy to write the SDK PR for the fetch-time configuration + .localhost widening if you'd rather stay on the plugin side. Just tell me which bet you want.

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.

2 participants