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
Draft
Conversation
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.
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.
Problem
Any UCP platform calling a correctly configured shop gets a 422 on every runtime request:
Setting the sales channel's
remoteProfileAllowlist(admin orucp: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:
DefaultHttpRequestContextFactory::assertSafeProfileUri()— uses the per-channel runtime configuration (remoteProfileAllowlist). This one works.UrlSafetyValidatorinsideHttpAgentProfileFetcher(also injected intoDefaultOrderWebhookDispatcher) — compiled from the static bundle configucp_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
ConfiguredUrlSafetyValidatorFactorybuilds 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).ReplaceSdkUrlSafetyValidatorPassswaps the SDK bundle's compiled definition for the factory-built one (compiler pass so the override wins regardless of bundle load order, same pattern asReplaceSdkSigningKeyCommandsPass).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_modeis passed through unchanged.Verification
Reproduced and verified against a live shop (Shopware PaaS) with a channel-scoped
remoteProfileAllowlist:POST /ucp/v1/catalog/searchwithUCP-Agent: profile="https://<allowed-host>/.well-known/ucp"→ 422Profile host … is not allowed.searchProductsround-trip returns products.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:
DefaultHttpRequestContextFactoryhas already resolved a per-requestRuntimeConfiguration(withallowedProfileHosts) when it callsAgentProfileFetcherInterface::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 exposesallowed_profile_hostsas 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 aRequestContextcreated through that same gate (or fromwebhookUrlOverride, whichUcpConfigvalidates 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).