Fix cross-project content leakage on multi-domain sites by stooit · Pull Request #254 · quantcdn/drupal

stooit · 2026-08-11T23:55:17Z

Why

A prospect is evaluating Quant for 50–100 client sites served from a single Drupal 11 backend, each publishing to its own Quant project. Testing that arrangement showed it did not work: every domain's content published to the base project instead. No error, no warning — the save succeeded and the wrong client's site changed.

Chasing that turned up several further defects on the publishing path, some of which affect single-domain sites too. They are separated into commits so they can be reviewed independently.

The original bug

Reproduced with two domains, two projects, and a recording API endpoint. Before this branch, ten of ten pushes across both domains arrived at the base project. After it, five arrive at each domain's own project.

Four things combined:

Workers forked by quant:run-queue inherited no --uri, so they booted on the default domain regardless of the parent's context.
getLockFileLocation() and the seed preparation read config through getEditable(), which bypasses overrides. Every domain shared one lock file, and each seeded with the base site's settings.
QuantClient captured its credentials in the constructor. The container is built before the domain is negotiated, so the project it captured was always the base one.
Queue items recorded no destination, leaving the worker to resolve the project from whatever context it happened to boot in.

The root cause is partly external and easy to miss: the Domain module populates its negotiation context from a kernel.request subscriber, and Drush never dispatches that event. So domain_config overrides are absent under CLI even when --uri names a valid domain. Forcing negotiation populates the context, but config objects built beforehand are already cached without the override, so the factory must also be reset. Both steps live in Drupal\quant\CliDomainContext.

Defence in depth

Queue items now carry the project they were queued for, and the worker refuses to send an item whose stamp does not match the project it is publishing to. Items queued before this change carry no stamp and are sent as before, so an upgrade with a full queue loses nothing.

DomainGuardSubscriber refuses to publish when the serving host matches no domain record. The Domain module falls back to the default domain in that case, and Quant follows it — so an unregistered alias, an apex/www slip, or a proxy forwarding the wrong Host silently republishes one client's content into another's project. It engages only where two or more domains are configured, so single-domain sites are untouched, including the many that run cron and seeds with no --uri.

The guard covers all three events the API subscriber listens to — content, redirects and unpublishes. Each was found by testing a different verb, and each was unguarded until then. Unpublish matters most: a delete on an unrecognised host takes a live page down on another client's site.

Domain negotiation happens inside that subscriber rather than at each entry point, because entry-point calls missed deletes made through drush php:eval, migrations, and anything that is not a Quant command.

Defects found on the way

These are independent of multi-domain and affect existing sites.

Live saves published nothing where quant_search is installed. The hooks defer work with drupal_register_shutdown_function, and Drupal pops the request off the stack once the response is sent. Contrib token — a quant_search dependency — collects token info during replacement, and site:base-path describes itself via \Drupal::request()->getBasePath(), which fatals on NULL. The shutdown handler can only reach error_log(), so the save succeeded, the page rendered, watchdog stayed silent, and nothing reached Quant. Seed callbacks now run with a request rebuilt from globals. Sites without quant_search were unaffected.

Utility::getPageInfo() returned NULL against a string return type. $output was only assigned in the branch handling a URL Quant already knew about, so any not-yet-synced page viewed by an admin with the page info block enabled was a 500. Its unmatched-URL list also closed itself once per URL.

Malformed multilingual redirects. getPathPrefix() already carries its leading slash and returns a bare slash for a language with no prefix; handleInternalPathRedirects() prepended another, so every translated page published a redirect at //fr/node/1. Utility::normalizePath() now collapses repeated slashes at the queue entry and at the API boundary, leaving query strings alone since an oEmbed route carries a whole URL in one.

TrafficRegistry::add() passed an array to Merge::key(), which takes a single field name and has asserted on arrays since Drupal 10.

Cache invalidation only refreshed one domain. The purger recorded traffic against a bare path, so every client's /about collapsed into one row. The registry now records the domain alongside the path and returns matches grouped by it, so the queuer raises one item per domain, each stamped with the project that owns it. Update 9103 adds the column, merging duplicate rows first — without that the unique key is refused with an integrity constraint and the update fails part-applied. Tags are combined rather than discarded.

Config schema was almost entirely absent — quant.settings, quant.token_settings, quant_api.settings, quant_cron.settings and quant_search.entities.settings had none, and quant_purger's still described keys renamed several updates ago. This is why kernel tests could not install the module.

Contrib dependencies were declared under the wrong project. quant_search named drupal:token, which the packaging facade reads as core and drops — published metadata for 2.0.0 requires no drupal/token at all. Prefixes corrected, and a composer.json added, since the facade does not carry submodule dependencies up into the project requirement.

CI never ran phpunit. The job named phpunit installed the module and stopped, which is how a comprehensively broken test file went unnoticed.

Testing

QuantClientTest was broken before this branch — 6 errors and 3 failures of 20. It never called reveal(), read getStatusCode as a property, built RequestException with one argument, and expected requests without Quant-Project, with exception for exceptions, and without the /v1 suffix. Rewritten to drive requests through a Guzzle MockHandler and assert what reaches the wire, so the header that decides which site content lands on is checked on every call.

76 unit and kernel tests, phpcs clean.

Beyond the suite, a harness drives every publishing path against a recording endpoint and asserts which project each request reached — 39 cases across single-site, multi-domain, multilingual, the two combined, and deletion in each. It asserts routing rather than counts: everything reached the expected project and nothing reached another.

Also verified manually:

Live API — a real seed to a live project; the API confirms four published records.
Upgrade from 2.0.0 — built a pre-upgrade site on 2.x code with 60 purger rows across 25 URLs and 5 queue items serialized by that release, then ran drush updb. 35 duplicates merged, column added, and all five legacy queue items survived and published.
Browser — editorial saves on both domains, including a French translation on the second, each reaching only its own project with its search record.

Deliberately not in scope

quant and quant_api depend on each other, and the code matches. Breaking it means relocating shared classes.
D12 deprecations: $entity->original, FormElement, NodeViewController, NodeStorage::revisionIds().
~118 \Drupal:: static calls that should be injected, which is much of why this module resists testing.

These belong in a follow-up hygiene release alongside D12 support.

Reviewer notes

One behaviour change to flag: if the project is changed while items are queued, those items are now skipped with a logged error rather than published to the new project. Defensible, but worth a release note.
The guard's subscribed events must stay in step with QuantApi::getSubscribedEvents(). A fourth event would go unguarded and nothing would fail loudly.
Adding a composer.json changes what the packaging facade contributes; worth checking the generated metadata on the next dev release.
Untested: a real cache-tag invalidation through purge's processors, and the purger update at scale — it batches, but was exercised at 120 rows.

Sites that serve many domains from one Drupal instance give each domain its own Quant project via config overrides. That routing was silently ignored: every domain's content published to the base project instead. Reproduced with two domains, two projects and a recording API endpoint. Before this change, ten of ten pushes across both domains arrived at the base project. After it, five arrive at each domain's own project. Four defects combined to cause it: - Workers forked by quant:run-queue inherited no --uri, so they booted on the default domain regardless of the parent's context. - getLockFileLocation() and the seed preparation read config through getEditable(), which bypasses overrides. Every domain shared one lock file, and each seeded with the base site's settings. - QuantClient captured its credentials in the constructor. The container is built before the domain is negotiated, so the project it captured was always the base one. - Queue items recorded no destination, leaving the worker to resolve the project from whatever context it happened to boot in. Domain negotiation is also forced explicitly in CLI. The Domain module populates its negotiation context from a kernel.request subscriber, and Drush never dispatches that event, so overrides are otherwise absent even when --uri names a valid domain. Queue items now carry the project they were queued for, and the worker refuses to send an item whose stamp does not match the project it is publishing to. Items queued before this change carry no stamp and are sent as before. This makes a leak impossible rather than unlikely: cron drains the queue in a single domain context, so without the check it would still misroute every other domain's items. Adds QuantClientProjectTest covering call-time project resolution, and repairs the config stub in QuantClientTest so the factory can answer more than one read.

quant_cron sends synchronously rather than queueing, so it published to whichever project the base configuration named. It now negotiates the domain first, making "drush --uri=... cron" target the right project. Verified against two domains: three nodes to each, none crossing over. quant_tome has the same shape. Its deploy command resolves the domain before checkConfig() reads the API settings, and the batch callback resolves it again because batch operations may run in a forked process. quant_search pushes index records straight to the API from its batch, so that resolves the domain too. Records ride the content push and were already correct once the queue was fixed; the end-to-end run confirms each domain's records reach only its own index. quant_sitemap needs no change: it contributes routes to the seed and inherits its context. quant_purger queues stamped items from HTTP, where the domain is already negotiated, so it is correct as well — but its traffic registry stores paths with no host, so a tag invalidation only purges one domain's copy. That is an under-purge rather than a leak and is left for a follow-up that needs a schema change. CliDomainContext now caches its result, so batch and loop callers can call it freely, and it only drops the config cache under CLI. A web request negotiates its domain from kernel.request before any Quant code runs, so resetting there would discard the config cache for nothing. QuantClientTest was broken long before this branch: 6 errors and 3 failures of 20, unnoticed because the CI job named phpunit only installs the module and never runs it. It never called reveal(), so the doubles were prophecies; getStatusCode was read as a property rather than called; RequestException was built with one argument; and the expected requests omitted Quant-Project, used 'exception' for 'exceptions' and expected the endpoint without its /v1 suffix. Rather than patch those expectations, requests now run through a Guzzle MockHandler with the history middleware, so the assertions describe the method, URI, headers and body that reach the wire. That makes the Quant-Project header — the one thing that decides which site content is published to — explicitly asserted on every call. Coverage extends to unpublish, getUrlMeta, search records, index clearing, facets, TLS verification and override reporting. The php built-in stubs are gone; the upload tests use real temporary files. quant_api unit tests: 22 of 22 pass, from 20 with 9 broken.

Kernel tests could not install this module's configuration, because most of it had no schema at all: quant.settings, quant.token_settings, quant_api.settings, quant_cron.settings and quant_search.entities.settings were all undeclared, and quant_purger's schema still described the tag_blacklist and path_blacklist keys that were renamed to blocklist and allowlist several updates ago. All are now declared and validate against the configuration the forms actually write. The CI job named phpunit installed the module and stopped there, which is how a comprehensively broken test file survived unnoticed. It now installs the test dependencies the image's phpunit bootstrap needs, and runs every unit and kernel test in the module and its submodules. Verified inside quantcdn/drupal-ci:11.1.x-dev. SitemapManagerTest extended KernelTestBase from a Unit namespace, so it failed for want of a database whenever it was run at all. Moved to Kernel, and the case that doubles a simple_sitemap class now skips where that optional module is absent. quant_purger recorded traffic against a bare path. Every client's /about collapsed into one row, so invalidating a cache tag refreshed whichever domain wrote that row last and left the others stale. The registry now records the domain alongside the path, and returns matches grouped by domain so the queuer can raise one item per domain, each stamped with the project that owns it. Queue items accept an explicit target project for exactly this case; everything else still stamps from the current context. Sites without the Domain module store an empty domain and behave as they did before. Update 9103 adds the column and the unique key. Fixes a latent bug found on the way: TrafficRegistry::add() passed an array to Merge::key(), which takes a single field name and has asserted on an array since Drupal 10. The call is now keys(), which is what it always meant. Test coverage across the module: 57 tests, from 20 of which 9 were broken. End to end, seeding and cron across two domains put 8 pushes in each client's project and none in the base project.

Driving a real node save through the browser showed that saving from the UI published nothing at all. The hooks queue their work with drupal_register_shutdown_function, and by the time those callbacks run Drupal has already popped the request off the stack. Info metadata calls $this->token->replace(), token info asks the request for its base path, and the resulting Error is caught by the shutdown handler, which can only reach error_log(). The save succeeded, the page rendered, watchdog stayed silent, and the content never reached Quant. Seed callbacks now run with a request rebuilt from the globals, which still describe the request that triggered the save, so the correct host and therefore the correct domain stay in scope. That also fixes the same crash reached through quant_search, whose subscriber renders the entity and runs ahead of the publisher, so its failure suppressed the push too. Utility::getPageInfo() declared a string return type but only assigned $output inside the branch handling a URL that Quant already knows about. Viewing any not-yet-synced page as an administrator, with the page info block enabled, returned NULL and produced a 500. It now starts from an empty string. Its unmatched-URL list also closed itself once per URL rather than once, so that markup is repaired. The browser test then caught something worse. A save on one domain was published to another client's project. When the request host matches no domain record the Domain module falls back to the default domain, and every Quant push follows it: the wrong customer's site changes and nothing reports it. The queue item stamp cannot catch this, because the stamp is taken from the same mistaken context. A guard subscriber now refuses to publish when the serving host has no domain record, ahead of both the search and publish subscribers, naming the host and the project it would otherwise have written to. It applies to the command line as well: a --uri that names no configured domain, or a cron run with no --uri, falls back identically and would republish an entire site into the default domain's project. Sites without the Domain module, or with no domains configured, are unaffected. Verified in the browser across both domains: each save reaches only its own project, with its search record. With a hostname deliberately broken, zero pushes leave Drupal and the refusal is logged. Command line seeding and cron behave the same way.

…n note. The guard blocked any publish from a host with no domain record, including on the command line. That is wrong for the many sites that run cron and seeds without a --uri: with a single domain there is only one project to publish to, so the fallback cannot misdirect anything, and refusing would stop publishing for no safety gain. It now engages only where more than one domain is configured, which is the only arrangement in which a page can reach a different site's project. Verified: a single-domain site running cron with no --uri publishes as before. A two-domain site still refuses an unrecognised host, on the web and on the command line alike. Also corrects the previous commit's account of the shutdown crash, which overstated its reach. Live saves were not broken everywhere. The failing call is the contrib token module collecting token info, where the site:base-path token describes itself by asking the request for its base path. Core's token service does not do this, so the crash only occurred where contrib token was installed — which in practice means sites running quant_search, since it depends on it. Confirmed by removing quant_search and token and watching a save publish correctly with the fix reverted. Keeping the fix regardless: running these callbacks without a request on the stack is fragile whatever happens to be listening, and it is what made the failure invisible.

Dependencies are declared as project:module. quant_search named all three of its dependencies under the drupal project, which claims quant, quant_api and token all ship with core. token in particular is contrib, and getting its project wrong means Drupal cannot point an administrator at what to install when it is missing. quant_api named quant the same way. The other submodules already do this correctly, with webform:webform, purge:purge and tome:tome_static. Note for later: quant depends on quant_api while quant_api depends on quant, and the code matches — the API subscriber uses QuantEvent, Utility and QuantQueueFactory from quant. Drupal tolerates the cycle today. Left alone here because breaking it means moving shared classes, which is not this branch's business.

The published metadata for 2.0.0 requires drupal/quantcdn, drupal/core and drupal/quant-quant_api, and nothing else. drupal/token is absent, because quant_search declared it under the drupal project and the facade read that as core and dropped it. drupal/purge, drupal/webform and drupal/tome are absent too, from which it is clear the facade does not carry submodule dependencies up into the project requirement at all. Correcting the prefixes was necessary but was never going to be enough on its own. These are listed as suggestions rather than requirements. Each belongs to one optional submodule, and a site using none of them should not be made to install four contrib projects. With the prefixes now correct, Drupal names the right project when it refuses to enable a submodule whose dependency is missing, so the path from error to fix is clear. No drupal/core requirement is declared, so the facade keeps deriving it from core_version_requirement. That leaves one source of truth and stops the 1.x and 2.x branches drifting apart. Worth checking the generated metadata on the next dev release: adding a composer.json changes what the facade contributes.

…dings. The update that adds the domain column also adds a unique key on (url, domain). The table never had one, so a site can hold several rows for the same URL, and the key was refused with an integrity constraint — the update failed part applied, mid deploy. Reproduced with three rows for one URL. Duplicates are now merged before the key is added, combining their tags rather than keeping one row's and discarding the rest: a tag dropped here is a page that stops being purged when its content changes. The work runs in phases over batches, because this table grows with traffic. Verified against six rows collapsing to three with every tag preserved. Config::get() takes one argument. Six calls passed a default as a second, which PHP discards silently, so the default never applied and the caller got NULL. Today the config/install defaults hide it, but any new setting whose default is not falsy would have been silently wrong. Rewritten with ??. QuantSearchPageForm built its facet rows in a foreach and then attached the "Add facet" button at $i, which is undefined when a page has no facets yet. Checked but deliberately left alone: $item->data in quant_process_queue is guarded by an earlier falsy check, and QuantSearchPageForm::save() declares no return type of its own so it cannot fatal on the missing return.

quant_tome queues its work in checkRequiredFiles(), which runs as a batch operation and may be handed to a separate process. That process had not negotiated a domain, so items were stamped with the base project while the sender resolved the real one, and every item was refused by the worker's project check. A deploy published nothing. Reproduced with tome installed: the run reported "Skipped [route_item] /rss.xml: queued for project SINGLE-SITE but this worker publishes to PROJECT-CLIENT-A" for each item. The domain is now resolved where the items are built, not only where they are sent. Verified with tome and webform installed: a single-site deploy publishes 7 pages, and a per-domain deploy sends every request to that domain's project and nothing to the other.

Utility::getPathPrefix() already carries its leading slash, and returns a bare slash for a language that has no prefix. handleInternalPathRedirects() prepended another, so every translated page published a redirect at //fr/node/1 instead of /fr/node/1. The truthiness check was wrong for the same reason: a bare slash is truthy, so a default-language page with an alias produced //node/1 too. Verified on a trilingual site: content reaches /fr/page-une and /de/seite-eins, the prefixed internal redirect is /fr/node/1, and no push carries a double slash. The domain guard covered content but not redirects, which are written to the same project by the same route. A run on an unrecognised host was therefore refused for pages and allowed for redirects, quietly rewriting another client's redirect map. It now guards both. That gap survived because the regression matrix counted any request carrying a url as a content push, and redirect payloads carry one. It now separates the two endpoints, and asserts routing rather than exact counts: everything reached the expected project and nothing reached another. Exact counts moved as soon as the fixture gained languages, which is precisely when the assertions should have kept working.

Deleting content was not guarded. unpublishUrl() dispatches QuantEvent::UNPUBLISH, a different event from the one the guard watched, so a delete on an unrecognised host withdrew the matching URL from whichever project the fallback landed on. That is the worst of the three: publishing to the wrong project adds a page, but unpublishing takes a live one down. Worse, the domain was only ever negotiated by Quant's own drush commands. Deleting a node through drush php:eval, a migration, or any other command resolved the base project even with --uri set. Verified: a delete as clienta withdrew /node/8 and /fr/node/8 from the base project rather than the domain's. Negotiation now happens in the guard subscriber, which every publish, redirect and unpublish passes through, so it no longer depends on which entry point started the work. The call is cached per process. Separately, nothing should ever be published at //fr/node/1. The handleInternalPathRedirects fix removed the cause found so far, but paths are assembled from prefixes, base paths and aliases all over the module, and any of them can be empty. Utility::normalizePath() collapses repeated slashes and is applied where routes enter the queue and again at the API boundary for content, redirects and unpublishes. Normalising before the self-redirect check also means a malformed source is recognised as equal to its destination instead of being published as a redirect to itself. quant_cron had one more producer, filtering out the empty default prefix. The query string is left alone: an oEmbed route carries a whole URL in one, and those slashes are not ours to collapse. Regression matrix now covers deletion in both shapes and multilingual: 25 cases, all passing. 76 unit and kernel tests.

quantcode-agent

Review: PR #254 — Fix cross-project content leakage on multi-domain sites

Verdict: REQUEST CHANGES — 1 blocker, 3 warnings.

The architecture here is sound and the layering is genuinely good: a stamp taken at enqueue time (TargetProjectTrait), re-checked at send time (QuantSeedWorker::targetsActiveProject), plus a host-level guard (DomainGuardSubscriber) and per-domain registry rows. The core leakage vector — items queued for domain A being sent by a worker booted on domain B — is genuinely closed. QuantClient::refreshCredentials() (modules/quant_api/src/Client/QuantClient.php:104-114) correctly fixes the singleton-pins-base-project bug, and applying it to all 12 call sites rather than just send() is the right call. The one blocker below is a data-corruption bug introduced by the new path normaliser, not a scoping gap.

Verified as covered

Path	Guard	Evidence
Publish (`QuantEvent::OUTPUT`)	Host guard @100, before search (1) and API (-999)	`DomainGuardSubscriber.php:61`, `QuantApi.php:68`
Redirect (`QuantRedirectEvent::UPDATE`)	Host guard @100	`DomainGuardSubscriber.php:65`
Unpublish/delete (`QuantEvent::UNPUBLISH`)	Host guard @100	`DomainGuardSubscriber.php:70`
Queue send	Stamp compared to active project	`QuantSeedWorker.php:33,54-76`
All 5 item types	All use `TargetProjectTrait`	`RouteItem:16`, `FileItem:14`, `NodeItem:14`, `RedirectItem:14`, `TaxonomyTermItem:14`
Purge fan-out	Per-domain project resolution	`QuantPurger.php:114-123,135-152`
Drush fork	`--uri`/`--root` propagated	`QuantDrushCommands.php:55-68`
Tome batch	`CliDomainContext::initialize()` at both create and send	`QuantTomeBatch.php:150,217`
Duplicate rows on update	Deduped before unique key added	`quant_purger.install:155-205`

Blockers

1. normalizePath() corrupts absolute URLs — breaks redirects to external destinations

src/Utility.php:323-334 collapses /{2,} across the whole path segment, including the // in a scheme:

public static function normalizePath(string $path) : string {
  $parts = explode('?', $path, 2);
  $parts[0] = preg_replace('#/{2,}#', '/', $parts[0]);

There is no scheme guard anywhere in the function. Verified by executing the logic:

'https://example.com/foo'  =>  https:/example.com/foo
'http://other.org/a//b'    =>  http:/other.org/a/b

This is reachable, not theoretical, and it is a regression introduced by this PR. Seed::getRedirectLocationsFromRedirect() at src/Seed.php:125 builds the destination via:

$destination = $redirect->getRedirectUrl()->toString();

The contrib redirect module supports external destinations, and toString() returns the absolute URL for them. That value flows unmodified into QuantApi::onRedirect(), which this PR changes to normalise it (modules/quant_api/src/EventSubscriber/QuantApi.php:85-86 — previously these were plain $event->getSourceUrl() / getDestinationUrl() reads). So every existing redirect pointing at an external host will now be published to Quant with a mangled single-slash scheme — a silently broken redirect for live traffic.

The module already has Utility::isExternalUrl() (src/Utility.php:154-162) using parse_url(). Suggested fix — bail out when a scheme is present, before the collapse. The test data provider at tests/src/Unit/UtilityNormalizePathTest.php:28-44 covers 10 cases but no absolute URL; the 'slashes in query untouched' case shows the query-string half was considered, so the scheme case looks like an oversight rather than a deliberate trade-off. Please add ['https://example.com/a//b', 'https://example.com/a//b'] and a protocol-relative case.

Warnings

2. Mismatched queue items are silently deleted, not requeued — content is dropped, not just withheld

QuantSeedWorker::processItem() returns NULL on mismatch (src/Plugin/QueueWorker/QuantSeedWorker.php:34). A normal return means the item is deleted — quant.module:408-409 calls $worker->processItem($item->data); then $queue->deleteItem($item); unconditionally, and the forked drush queue:run path inherits core's identical delete-on-return semantics. There is no RequeueException, SuspendQueueException, or releaseItem() anywhere in the tree. So on a multi-domain site, a worker booted on domain A will consume and discard every item queued for domains B and C. Since the queue is a single shared table with no per-domain partition, the practical outcome is that another domain's content is never published — it's eaten by whichever domain's worker claims it first.

This trades a leak for silent data loss, which is the safer direction, but it isn't complete. Throwing \Drupal\Core\Queue\RequeueException instead of returning NULL would leave the item for the correct worker; a per-domain queue name would be the more robust structural fix. The error log at :69-73 records it, but the work is lost.

3. Purger clear() and remove() are asymmetric on domain scoping

TrafficRegistry::remove() correctly scopes to the active domain (modules/quant_purger/src/TrafficRegistry.php:85-90, ->condition('domain', $this->getActiveDomainId())), but clear() at lines 95-97 deletes unconditionally:

public function clear() {
  $this->connection->delete('purge_queuer_quant')->execute();
}

Invoked from the admin UI at modules/quant_purger/src/Form/ConfigurationForm.php:171, so an administrator on one domain wipes the traffic registry for every client — every other domain silently stops purging until re-seeded. Given the PR's premise that domains belong to different clients, this deserves either domain scoping or an explicit confirmation that it is global.

4. Request::createFromGlobals() in the shutdown handler trusts the raw Host header

_quant_run_with_request() pushes Request::createFromGlobals() at quant.module:234, which bypasses Symfony's trusted-proxy/trusted-host handling, so $_SERVER['HTTP_HOST'] is taken verbatim. That request is what DomainGuardSubscriber::hostIsUnknown() reads via $request->getHttpHost() (line 126), so a spoofed Host feeds domain resolution.

Blast radius is limited — an unrecognised host is refused at line 99, so a spoofed header causes a denial of publish rather than a redirect of content to an attacker-chosen project, and reaching a valid other-client host requires knowing it. But the destination project is influenced by an untrusted header on a path that deliberately skips core's host validation. Consider capturing the validated host from the live request while it is still on the stack, rather than re-deriving it from globals after the fact. Relevant to ISM-1552 (untrusted input in a trust decision).

Test coverage assessment

Genuinely good on the guard logic — the failure modes are exercised, not just happy paths:

DomainGuardSubscriberTest covers unknown-host stops, single-domain bypass, no-domain-module, zero domains, port mismatch, and redirects both ways.
QuantSeedWorkerProjectGuardTest covers mismatch-withheld, legacy NULL stamp passes, no-active-project withheld, foreign object ignored, and stamp-at-enqueue-time.
Wiring CI to actually run PHPUnit (.github/workflows/ci.yml) is a real improvement.

Gaps worth closing:

No absolute-URL case in UtilityNormalizePathTest — this is precisely why blocker #1 slipped through.
QuantEvent::UNPUBLISH is subscribed but never asserted — no UNPUBLISH test anywhere in tests/, yet the docblock at DomainGuardSubscriber.php:66-70 calls this the irreversible path. The most destructive path is the least tested.
QuantPurger::getProjectForDomain() is untested — the whole per-domain purge fan-out (QuantPurger.php:135-152) has no coverage; TrafficRegistryDomainTest tests the registry beneath it but never the queuer.
quant_purger_update_9103() dedupe is untested — it's batched, multi-phase, mutates tags across merged rows, and runs against production tables. A kernel test seeding duplicates and asserting no tag is lost would be cheap insurance.

Nits (non-blocking)

quant.module — the use Symfony\...\Request; insertion breaks alphabetical grouping of use statements.
CliDomainContext::$initialized is set to TRUE before the early returns, so a call made before the container has domain.negotiator permanently caches a NULL. Correct for CLI's single-domain-per-process assumption, but worth a comment.
TargetProjectTrait::stampTargetProject() reads \Drupal::config() statically, making item types awkward to unit-test (FileItemTest has to stub the config factory just to construct one).

Nothing here undermines the core design — the stamp-and-verify approach is the right shape, and the scoping is enforced on every path traced. Blocker #1 is a self-contained bug in the new normaliser and should be quick to fix; warning #2 is the one worth a design conversation before this lands on a live multi-client site.

(Note: an earlier draft flagged a $host-read-before-assignment issue in DomainGuardSubscriber::onOutput(); on verification against PHP by-reference semantics and the callee body it does not hold and was dropped.)

normalizePath() collapsed the // in a scheme, so https://example.com became https:/example.com. Reachable: the redirect module supports external destinations, Seed builds them with Url::toString(), and this branch put that value through the normaliser. Every redirect pointing off site would have published broken. Reproduced with a real redirect entity. An absolute url is now returned untouched. A protocol-relative one is still collapsed, because it cannot be told apart from a malformed path by inspection and nothing here generates one; that is written down rather than left implicit. A mismatched queue item returned NULL, and a worker that returns normally has its item deleted. On a shared queue that meant the first domain's worker to claim consumed every other domain's work and it was never published. Traded a leak for silent loss. The worker now throws DelayedRequeueException, which drush and cron both honour, and the batch runner in quant_process_queue is taught to as well since it deleted unconditionally. Verified: 8 items queued for clientb, drained as clienta, all 8 requeued and none lost; drained again as clientb after the delay, all 8 published to its project and the queue empties. TrafficRegistry::clear() deleted every row while add() and remove() are domain scoped, so an administrator on one client's domain wiped the registry for all of them and each silently stopped purging. Now scoped to the active domain, which on a single-domain site is every row. The shutdown handler rebuilt its request with createFromGlobals(), taking $_SERVER['HTTP_HOST'] verbatim and bypassing trusted host checking — on the path where the host decides which project receives the work. The hooks now capture the live request while it is still on the stack and hand it over. Writing the test that asserts the guard covers every event QuantApi publishes on failed immediately: QuantFileEvent::OUTPUT was unguarded, so a misdirected run would have uploaded one client's files and media into another's project. That is the fourth event, found the way the review predicted a fourth would be. Now guarded, and the parity test will fail if a fifth appears. Also closes the coverage the review named: absolute urls in the normalizePath provider, UNPUBLISH asserted rather than merely subscribed, QuantPurger::getProjectForDomain(), and the update 9103 dedupe including that no tag is lost when rows merge. Import ordering and the CliDomainContext caching comment tidied. 91 unit and kernel tests, 39 regression cases, phpcs clean.

stooit · 2026-08-12T00:49:37Z

Thanks — this was a genuinely useful review. All four points verified before acting on them, and all four were real. Pushed in aaad0ef.

Blocker 1 — `normalizePath()` corrupts absolute urls

Confirmed, and reachable exactly as described. Reproduced with a real redirect entity rather than by reading:

destination as built by Seed: https://example.com/external-target
after normalizePath():        https:/example.com/external-target

My regression, introduced by this branch. normalizePath() now returns early when parse_url() finds a scheme.

One judgement call worth surfacing, since you asked for a protocol-relative case: //example.com/x and //fr/node/1 are the same shape, so nothing can tell them apart by inspection. Protocol-relative is therefore still collapsed. That is safe here because nothing in the module generates one — an external destination arrives from Url::toString() with its scheme intact — and a bare // in this codebase is always the malformed path this function exists to fix. Written into the docblock and pinned by a test, rather than left as an accident.

Provider now covers https://, http://, absolute-with-query, and the protocol-relative case asserting the collapse is deliberate.

Warning 2 — mismatched items deleted rather than requeued

Correct, and the sharpest catch here. Confirmed both drainers delete on normal return: core's Cron and Drush's queue:run both do, and quant_process_queue calls deleteItem() unconditionally. So a worker booted on domain A consumed and discarded every item queued for B and C. I had traded a leak for silent loss and only noticed the first half.

Fixed with DelayedRequeueException, which Drush and cron already honour — quant_process_queue needed teaching, since it was the one path that would still have deleted. Chose delayed over plain RequeueException because an immediate release lets the same run re-claim the item and spin; the delay is 60s, long enough that a single pass cannot loop, short enough that the correct worker gets it on its next run.

Verified end to end rather than by unit test alone:

8 items queued for clientb
drained as clienta  -> 8 requeue messages, 8 items still in queue
drained as clientb  -> 8 processed, 10 pushes all to PROJECT-CLIENT-B, queue empty

Per-domain queue names are the better structural answer and I agree with you; it needs a derivative for the worker plugin, so I have left it for the hygiene release rather than grow this branch.

Warning 3 — `clear()` not domain scoped

Correct and straightforward. Now scoped to the active domain, matching add() and remove(). On a single-domain site the active domain is the empty string, which is every row, so behaviour is unchanged there. The confirmation message now says "for this domain" so the admin UI does not overstate what it did.

Warning 4 — `createFromGlobals()` trusts the raw Host header

Correct. Rather than re-derive and re-validate, the hooks now capture the live request while it is still on the stack and hand it to the shutdown callback, which prefers it over rebuilding. That request already went through trusted host checking, so the untrusted path is gone rather than filtered. createFromGlobals() remains only as a fallback for a callback with no captured request.

Test gaps

All four closed, and writing one of them found a fifth defect.

The parity test you implied — asserting the guard covers every event QuantApi publishes on — failed on first run. QuantFileEvent::OUTPUT was unguarded, so a misdirected run would have uploaded one client's files and media into another's project. That is the fourth event, found precisely the way you predicted a fourth would be. Now guarded, and the test fails if a fifth appears.

Also added: absolute urls in the normalizePath provider, UNPUBLISH asserted rather than merely subscribed, QuantPurger::getProjectForDomain(), and quant_purger_update_9103() including an assertion that no tag is lost when rows merge.

Nits

Import ordering fixed. CliDomainContext::$initialized now carries the comment explaining why caching a NULL is correct for a CLI process. The static \Drupal::config() read in TargetProjectTrait I have left alone — it is one instance of the ~118 static calls that make this module hard to test, and it belongs with that work rather than here.

Where it stands

91 unit and kernel tests, phpcs clean. Separately, a harness drives every publishing path against a recording endpoint and asserts which project each request reached — 39 cases across single-site, multi-domain, multilingual, the two combined, and deletion in each: still green after these changes.

Deferred to the hygiene release, with your agreement: per-domain queue names, the quant/quant_api cycle, the D12 deprecations, and the dependency injection work.

@var

Reviewing the branch for anything the last round missed turned up the same gap a fourth time, in a place events never reach. Search records, facets and index clearing call QuantClient directly and dispatch nothing, so the subscriber never saw them. Clearing is the destructive one: on an unrecognised host it wipes another client's entire search index. Guarding those three call sites would have repeated the mistake, so the decision moved into Drupal\quant\PublishGuard and both the subscriber and the client consult it. The subscriber still runs first, and still stops early enough to skip the render and search work; the client is the backstop that covers a write method nobody has written yet. Applied to the seven methods that change something — send, sendRedirect, sendFile, unpublish, sendSearchRecords, clearSearchIndex, addFacets — and deliberately not to ping, project, search or getUrlMeta, which are reads and are how the settings form reports whether the connection works. Verified against two domains on an unrecognised host: the three search writes refused with zero requests reaching the API, while ping still answers. A refused write returns an empty array, which QuantApi::onOutput could not survive: it read $res['attachments']['js'] straight into array_merge(), which fatals on NULL. The guard makes that reachable, but the API was always entitled to answer without attachments, so the keys are no longer assumed. Static analysis on the changed files also flagged a test constructing QuantPurger with plugin arguments it has no constructor for, and an @var where a @PARAM belonged in the tome batch. 94 unit and kernel tests, 39 regression cases, phpcs clean.

stooit · 2026-08-12T01:03:43Z

Did another pass over the whole branch looking for anything the last round missed. Found the same gap a fourth time, in a place events never reach. Pushed in 6770485.

Three write paths bypassed the guard entirely

Scope first, because an earlier version of this comment overstated it: no current customer can be affected. All three need a multi-domain setup with per-domain projects, and that configuration did not work before this branch — it is the bug this PR fixes. Nobody is running it. These are latent hazards in a configuration this branch both enables and guards in the same change.

The guard only ever saw work that dispatched an event. These call QuantClient directly and dispatch nothing:

Call	Effect, given the preconditions below
`clearSearchIndex()`	Clears the default domain's search index instead of the intended one
`sendSearchRecords()`	Writes records into the default domain's index
`addFacets()`	Writes facet config into the default domain's project

The preconditions, all required together: two or more domains configured; an administrator with administer quant search reaching the admin form on a hostname matching no domain record; the Domain module then falling back to the default domain. The target is always the default domain's project, not an arbitrary client's.

Same class as the unpublish gap. clearSearchIndex() is the one that removes rather than adds, which is why it is worth closing before multi-domain ships rather than after.

Guarding three more call sites would have repeated the mistake — that is now four times this pattern has bitten. So the decision moved into Drupal\quant\PublishGuard, and both the subscriber and the client consult it. The subscriber still runs first and still stops early enough to skip the render and search work the later subscribers would do; the client is the backstop that covers a write method nobody has written yet.

Applied to the seven methods that change something: send, sendRedirect, sendFile, unpublish, sendSearchRecords, clearSearchIndex, addFacets. Deliberately not to ping, project, search or getUrlMeta — those are reads, and ping/project are how the settings form reports whether the connection works, so they must keep answering on any host.

Verified against two domains on an unrecognised host:

guard refuses: true (host: quant-domain-poc.ddev.site:33000)
clearSearchIndex:  []   refused, logged
sendSearchRecords: []   refused, logged
addFacets:         []   refused, logged
ping (a read):     true
requests that reached the API: 1   (GET /v1/ping)

The guard introduced a hazard of its own

A refused write returns an empty array, and QuantApi::onOutput() could not survive that — it read $res['attachments']['js'] straight into array_merge(), which fatals on NULL. The event guard stops that path first in practice, so it was not reachable, but it was one divergence away from being so. The API was always entitled to answer without attachments, so those keys are no longer assumed.

Worth flagging as the general risk with defence in depth: the second layer has to fail as gracefully as the first.

Also from this pass

Static analysis over the changed files flagged a test constructing QuantPurger with plugin arguments it has no constructor for, and an @var where a @param belonged in the tome batch. Both fixed.

Re-checked but deliberately unchanged: $item->data in quant_process_queue is still guarded by the earlier falsy check; $entity->original and the other D12 deprecations stay for the hygiene release; empty($domain) in CliDomainContext looks redundant to PHPStan only because the Domain module's return type says non-nullable while it demonstrably returns NULL.

Where it stands

94 unit and kernel tests, 39 regression cases, phpcs clean.

The structural answer to this recurring pattern is now in place twice over: the parity test fails if QuantApi gains an event the subscriber does not guard, and a new write method on the client is covered whether or not anyone remembers. I would still rather a fifth instance surfaced in review than in production, so more eyes welcome.

Edited to add the preconditions and scope. The original wording described the worst case without them, which read as a live production risk; it is not one.

Wiring the phpunit job was verified by running the unit tests inside the CI image locally. That missed the kernel tests, which install quant_purger and therefore need drupal/purge. CI only installed drupal/token, so all 14 purger kernel tests errored with "Unavailable module: 'purge'" on every push since the job was added. The suite passes locally because the harness has purge installed for the end-to-end work. Verifying a CI change by running part of it locally was not verifying it; the run on the pull request was.

The harness that found most of this branch's bugs existed only on one laptop. It drives every publishing path against a recording endpoint and asserts which project each request reached, which the unit and kernel suites cannot: they check the decision, this checks what leaves Drupal. Every bug it was written for was silent in production, and one reached only the PHP error log. Moved into tests/regression with a README, and the hard-coded path replaced. Also drove a real cache tag invalidation through the queuer, which had only been tested a layer at a time. /node/2 registered on both domains, invalidating node:2 from clienta's context queued two items stamped PROJECT-CLIENT-A and PROJECT-CLIENT-B, draining as clienta published A's and held B's, and draining as clientb published B's and emptied the queue. That is the fan-out and the per-domain project resolution confirmed against real data rather than doubles. Found while doing it, and left alone: the traffic registry only records requests carrying a Quant token, and the crawl only sends one when disable_content_drafts is off. That setting is on by default, so cache-tag purging does nothing on a default configuration — a full seed populated zero rows, and five once drafts were enabled. It predates this branch and is unrelated to multi-domain, and the fix has to reason about what that token is for, so it is written up as a known issue rather than rushed in here.

stooit · 2026-08-12T02:52:21Z

Closed the last three gaps. Pushed in b6b3d3c.

CI was red on every push since the phpunit job was added

Worth naming plainly: I added that job and verified it by running the unit tests inside the CI image locally. That missed the kernel tests, which install quant_purger and so need drupal/purge; CI installed only drupal/token, and all 14 purger kernel tests errored with Unavailable module: 'purge'.

Now green — Tests: 94, Assertions: 209, Skipped: 1, the skip being the simple_sitemap case that correctly skips when that optional module is absent.

Purge fan-out, verified end to end

Previously tested a layer at a time. Driven through a real cache tag invalidation:

/node/2 registered on:  clienta_ddev_site, clientb_ddev_site
invalidate node:2 (from clienta's context)
  -> 2 items queued: [route_item] /node/2 -> PROJECT-CLIENT-A
                     [route_item] /node/2 -> PROJECT-CLIENT-B
drain as clienta -> A published, B held for its own worker
drain as clientb -> B published, queue empty

Note the second item: invalidating from Client A's context still resolves Client B's project correctly, which is the part getProjectForDomain() exists for and the part only unit tests had covered.

A pre-existing issue found on the way, deliberately not fixed here

The traffic registry only records requests carrying a Quant token, and the crawl only sends one when disable_content_drafts is off. That setting is on by default, so cache-tag purging does nothing on a default configuration. Measured: a full seed populated zero registry rows with the default, and five once drafts were enabled.

This predates the branch and is unrelated to multi-domain. The fix has to reason about what that token is for — it grants draft access — so it is written up as a known issue in the release notes rather than rushed in alongside this work. Flagging it because it means the purger improvements in this PR are inert for most sites until it is addressed.

The regression harness is now in the repo

tests/regression/ — the harness and its recording endpoint, with a README covering what it asserts and why. It existed only on one laptop, and it is what found most of this branch's bugs: the unit and kernel suites check the decision, this checks what actually leaves Drupal. Every bug it was written for was silent in production, and one reached only the PHP error log.

39 cases, asserting routing rather than counts.

Release notes

Drafted for 2.1.0, matching the existing files. Two behaviour changes are called out prominently, since both will look like regressions to someone who has not read them:

A queued item whose project has since changed is held and retried rather than published to the new project.
Multi-domain sites refuse to publish from a hostname matching no domain record. Single-domain sites are unaffected and still publish with no --uri.

Also flagged for reviewers: the earlier comment about clearSearchIndex() has been edited to state its preconditions. The original wording described the worst case without them and read as a live production risk. It is not one — the configuration it needs did not work before this branch.

quantcode-agent

Re-review — verification of the four prior findings (B1, W2, W3, W4)

Baseline: prior review findings vs. current tree at HEAD, diff base 2.x. Delta triaged as aaad0ef ("Address review: absolute urls, requeue, clear() scope, host trust.") plus 6770485, 1d967a3, b6b3d3c. All findings re-verified against source (including a baseline check of the pre-PR code) before posting.

Item	Status
B1 — `normalizePath()` collapsing URL schemes	✅ RESOLVED
W2 — `QuantSeedWorker` silently deleting mismatched items	✅ RESOLVED
W3 — unscoped `TrafficRegistry::clear()`	✅ RESOLVED
W4 — shutdown handler trusting `Host` header	✅ RESOLVED

No blockers remain. All four were addressed at the root, and in two cases carried through to collaborators the finding didn't name (the batch runner for W2, the user-facing message for W3). Verdict below is COMMENT — approval-ready, with warnings worth addressing before merge.

B1 — `normalizePath()` mangling URL schemes — ✅ RESOLVED

A scheme guard now short-circuits before any collapsing, at src/Utility.php:337-339:

if (!empty(parse_url($path, PHP_URL_SCHEME))) {
  return $path;
}

Verified it executes before the // collapse at :344. Empirically: https://example.com/a → unchanged; /fr//node/1 → /fr/node/1; query strings untouched (/a//b?x=//y → /a/b?x=//y). The redirect vector that mattered is closed — Seed::getRedirectLocationsFromRedirect() → QuantApi::onRedirect() (modules/quant_api/src/EventSubscriber/QuantApi.php:85-86) now passes both source and dest through the guard.

isExternalUrl() was deliberately not used — that would consult host_domain config and return FALSE for an absolute URL on the site's own host, still collapsing https://own-host//a. parse_url(..., PHP_URL_SCHEME) is host-independent and the correct choice. Coverage for absolute URLs was added in tests/src/Unit/UtilityNormalizePathTest.php. See W1 for a call-site gap that does not affect this vector.

W2 — `processItem` silently deleting mismatched items — ✅ RESOLVED

The worker now throws instead of returning/deleting, at src/Plugin/QueueWorker/QuantSeedWorker.php:87:

throw new DelayedRequeueException(self::REQUEUE_DELAY);

REQUEUE_DELAY = 60 (:24); method renamed to assertTargetsActiveProject(); logging retained and reworded to "Requeued"; the legacy no-stamp passthrough is preserved so existing single-domain queues survive the update. The batch runner that previously deleted unconditionally was also fixed — quant.module:433-447 catches DelayedRequeueException, checks DelayableQueueInterface, and calls delayItem($item, $e->getDelay()) with a releaseItem() fallback. The bounded delay prevents a single run spinning on the same item. Covered by tests/src/Kernel/QuantSeedWorkerProjectGuardTest.php (testMismatchedItemIsRequeuedNotConsumed).

W3 — `TrafficRegistry::clear()` unscoped — ✅ RESOLVED

clear() is now domain-scoped consistently with remove(), at modules/quant_purger/src/TrafficRegistry.php:103:

->condition('domain', $this->getActiveDomainId());

Matches remove() (:88) and add()'s merge keys (:78), all via getActiveDomainId(), which returns '' when the Domain module is absent — so single-domain behaviour is unchanged. The user-facing string was updated to match ("...for this domain", ConfigurationForm.php:173).

Caveat (Warning, tied to W3): on a multi-domain site that upgrades, quant_purger_update_9103() intentionally leaves pre-existing rows with domain = '' (modules/quant_purger/quant_purger.install:147-149). Post-upgrade, clear()/remove() scoped to a real domain ID can never match those legacy rows, so they linger until re-seeded — and since getPathsByDomain() is the read path, stale rows can keep feeding purges for URLs an admin believes they cleared. Not data loss (self-healing), but surprising. Consider clearing domain IN ('', $active) or logging the count of domain = '' rows in the update message.

W4 — shutdown handler trusting the `Host` header — ✅ RESOLVED (web path)

All four hooks now capture the live, trusted-host-checked request while it is still on the stack — quant.module:63, :98, :127, :162:

'request' => \Drupal::requestStack()->getCurrentRequest(),

Threaded through quant_shutdown() (:219) into _quant_run_with_request(), which prefers it at :256:

$stack->push($request ?: Request::createFromGlobals());

Since getCurrentRequest() is non-NULL during a web request, the createFromGlobals() branch is unreachable from HTTP — closing the host-header injection path on the code path that selects the destination project. The fallback only triggers under CLI, where there is no attacker-supplied Host. Defended in depth by PublishGuard::refuses() (src/PublishGuard.php:41-82), consulted by both the subscriber and the client. See NEW-3 nit re: a stale docblock. Minor: ?: vs ?? — safe today since Request is never falsy.

Newly-observed issues (all Warnings/nits — none blocking)

W1 — `normalizePath()`'s absolute-URL guard is dead code at the `RouteItem` call site (`src/Plugin/QueueItem/RouteItem.php:50-53`)

The slash is prepended before normalising:

if (substr($route, 0, 1) != '/') {
  $route = "/{$route}";
}
$route = Utility::normalizePath(trim($route));

So an absolute route reaches normalizePath() as /https://example.com/page?page=2; parse_url() returns NULL for the scheme of that string, the B1 guard is skipped, and the collapse yields /https:/example.com/page?page=2.

Not a regression — the prepend is unchanged by this PR; pre-PR the line was $route = trim($route);, so the same input already produced the (equally broken) /https://… path. Nothing that worked breaks. It still matters because an absolute pager href reaches this path via QuantApi.php:263 ($node->getAttribute('href') passed straight through when not ?-relative). Suggested fix — normalise first, prepend only if no scheme:

$route = Utility::normalizePath(trim($route));
if (substr($route, 0, 1) !== '/' && empty(parse_url($route, PHP_URL_SCHEME))) {
  $route = "/{$route}";
}

Worth a RouteItem-level test for absolute routes — the isolated normalizePath tests are exactly why this gap survived.

W2 operational note

With REQUEUE_DELAY = 60 and the plugin's cron = {"time" = 60}, a queue holding many foreign-domain items can spend most of a cron window re-claiming/re-delaying them. Not a correctness issue (safe degradation) — but on a shared instance with a large cross-domain backlog, a larger delay or a per-run mismatch cap would cut churn.

Nits

quant.module:240-241 — _quant_run_with_request() docblock is stale and contradicts the W4 fix ("the globals still describe the request... so the rebuilt request also keeps the correct domain in scope"). The request is now normally passed in, not rebuilt from globals. Also, the new $request parameter (:248) has no @param tag — likely a Drupal.Commenting.FunctionComment phpcs failure; re-run the lint job.
tests/src/Kernel/QuantSeedWorkerProjectGuardTest.php:148,166,205,219 — @covers ::targetsActiveProject references the old name; the method is now assertTargetsActiveProject() (line 190 is already correct). Stale @covers are silently ignored, understating guard coverage (and fail under --strict-coverage).
src/Plugin/QueueItem/RouteItem.php:96 — $config->get('proxy_override') ?? FALSE is equivalent to the prior get('proxy_override', FALSE); harmless diff churn.

Verdict: COMMENT — approval-ready. B1, W2, W3, W4 are all fully resolved with no new blockers introduced. W1 (a pre-existing latent issue now sitting next to the new guard) is the one I'd most want fixed before merge, but it breaks nothing that currently works.

steveworley · 2026-08-14T00:10:08Z

The per-domain project resolution in QuantPurger::getProjectForDomain() doesn't run — the service and method it guards on don't exist in the domain project.

modules/quant_purger/src/Plugin/Purge/Queuer/QuantPurger.php:140:

if (!empty($domainId) && $container->has('domain.config_factory_override')) {
  $override = $container->get('domain.config_factory_override')
    ->getOverride($domainId, 'quant_api.settings');

In both 8.x-1.x and 2.0.x the config override service is domain_config.overrider (Drupal\domain_config\DomainConfigOverrider), and that class exposes loadOverrides(), getCacheSuffix(), createConfigObject() and getCacheableMetadata() — there is no getOverride(). So $container->has(...) is always FALSE and getProjectForDomain() unconditionally returns the base project for every domain.

On the two-domain setup this PR targets, that means invalidating a cache tag for clientb.example.com/about queues a RouteItem stamped target_project = base-project. When drush queue:run quant_seed_worker --uri=https://clientb.example.com claims it, QuantClient::getProject() returns clientb-project, the stamp mismatches, and QuantSeedWorker throws DelayedRequeueException — clientb's page never republishes. Run the worker under the base domain instead and the page publishes into the wrong project. Either way, cache-tag-driven purging on non-default domains doesn't work.

QuantPurgerProjectTest won't catch this as written: it only exercises the fallback branch, and testUnknownDomainFallsBackToBaseProject() asserts assertFalse(\Drupal::hasService('domain.config_factory_override')), so the override path has no coverage. Worth adding a test that stubs the real service and asserts a non-base project comes back.

stooit · 2026-09-01T16:36:28Z

Good catch, and sorry for the slow reply.

You're right. Our test bed pinned drupal/domain: ^3.0 so we got 3.0.1, where the service is domain.config_factory_override and does have getOverride(). On 2.0.x it's domain_config.overrider with no such method, so getProjectForDomain() just returns the base project for everything. Both lines are stable and both support D11, so plenty of sites will hit this.

Fix: drop the service lookup entirely and read the override storage, which covers both (domain.config.<id>.<name> on 2.0.x, collection domain.<id> on 3.0.x). No version sniffing, and no poking setDomain() on a shared service.

Not assuming the rest of it holds on 2.0.x either — the override mechanism differs, and CliDomainContext leans on forcing negotiation then resetting the config factory. Setting up a second test bed on 2.0.x and running the whole e2e matrix against both. Will report back.

@PARAM

getProjectForDomain() checked for domain.config_factory_override, which exists only in Domain 3.x. Both 2.0.x and 3.0.x are stable and support Drupal 11, and on 2.0.x the service is domain_config.overrider, which has no getOverride() at all. So the check was always false there and every domain resolved to whatever project the current context named. Confirmed on a live 2.0.1 install: invalidating a tag for a page served by two domains queued two items, both stamped PROJECT-CLIENT-A. Client B's page never republishes, and Client A gets the same push twice. The resolution no longer names a service. Both lines keep overrides in config, in different shapes — 2.x in an object called domain.config.DOMAIN_ID.NAME, 3.x in a collection called domain.DOMAIN_ID — so reading the storage covers both with no version detection. Asking the 2.x service instead would mean calling setDomain() on a shared singleton mid-request, which changes what every other config read resolves to. The old test asserted that the 3.x service was absent, so it recorded the bug as expected behaviour rather than catching it. Rewritten to write each layout and assert the right project comes back, which needs no domain module installed at all. Checked that it fails against the previous resolver before keeping it. The harness now writes both layouts too, and the matrix gained the purge fan-out case that was missing — the regression happened in the one path it did not cover. Verified on both versions: 97 unit and kernel tests, 41 regression cases, phpcs clean, on domain 2.0.1 and again on 3.0.1. Also from review: RouteItem prepended its slash before normalising, so an absolute route lost its scheme to the collapse; it normalises first now. Stale @Covers names and a missing @PARAM corrected, and the docblock that still described rebuilding the request from globals.

stooit · 2026-09-01T17:01:26Z

Fixed in a56a1cb. Reproduced it on a real 2.0.1 install first — a tag invalidation for a page on two domains queued two items, both stamped PROJECT-CLIENT-A, exactly as you said.

Dropped the service lookup. Both lines keep overrides in config, just differently shaped (domain.config.<id>.<name> on 2.x, collection domain.<id> on 3.x), so it reads the storage and needs no version detection. Didn't go near setDomain() — mutating a shared singleton to ask it about another domain would change what every other config read in that request resolves to.

Rewrote the test to write each layout and assert the right project comes back. Needs no domain module installed, and I checked it fails against the old resolver before keeping it — the previous one passed against broken code, which is how this got through.

Also added the purge fan-out to the e2e matrix. It wasn't covered, which is why a fully green run said nothing about the path that was broken.

Verified on both versions, switching the module underneath: 97 unit/kernel tests and 41 e2e cases green on 2.0.1 and again on 3.0.1. The harness writes both layouts now so it runs unchanged on either, and the README says to run it twice.

Rest of your review while I was in there: RouteItem was prepending its slash before normalising, so an absolute route lost its scheme to the collapse — normalises first now. Stale @covers names and the missing @param fixed.

You were right that everything else needed re-checking too. Worth noting for anyone reading later: CliDomainContext turns out to be needed only on 3.x. On 2.0.x the overrider resolves the active domain lazily through the negotiator, so per-domain config already resolves correctly under drush --uri with no forcing. Harmless there, load-bearing on 3.x.

Testing three domains against live projects turned up a failure state the routing work cannot see. Domain Access decides which domain serves which node through node grants, and enabling it leaves Drupal's grants stale until someone rebuilds. Until then every domain serves every page, so a seed collects all of it and publishes one client's content into another client's project. Every page reaches the correct project for the domain being seeded, so the routing is right and nothing reports a problem. Found the hard way: the first live seed of clienta pushed all 63 nodes into static-test-a. After node_access_rebuild() the same seed collected 23, which is clienta's 20 plus the three unrestricted originals. So the seed does respect domain access once the grants are current — content scoping works, it just cannot be assumed. Reported in two places rather than fixed silently. A warning on the status report, and an error on quant:seed-queue, which is where the damage happens and where someone is watching. Both name the consequence rather than the mechanism. Deliberately not rebuilding grants from an update hook. It rebuilds every node's grants, which on a large site holds a deploy open for a long time, and it would run for every site with Domain enabled including those whose grants are fine. Node grants belong to Domain Access, not here. Deliberately not refusing to publish either, unlike the unrecognised host guard. An unrecognised host has no legitimate reading; a pending rebuild does — the flag can be set while content is scoped correctly, and it stays set until an administrator acts, which on a large site can be a while. Only reported where it can do harm: domain_access enabled and two or more domains configured.

stooit · 2026-09-01T17:46:19Z

Ran the multi-domain matrix against three real projects (static-test-a/b/c), three domains, 63 nodes scoped per domain with Domain Access, French translations, aliases that carry the owning domain.

Isolation, asking each project what it actually has live:

static-test-a   own pages live 4/4 | foreign pages LIVE 0
static-test-b   own pages live 4/4 | foreign pages LIVE 0
static-test-c   own pages live 4/4 | foreign pages LIVE 0

Deleting B page 12 as clientb withdrew /b-page-12 from static-test-b only and left /b-page-13 published. A cache tag invalidation on a page all three domains serve stamped items for all three real project names. Base project was set to a deliberately invalid name for the whole run, so any fallback would have failed loudly instead of quietly writing somewhere real.

Two things worth reporting.

The guard caught a real mistake I made. Creating the 60 nodes via drush php:script with no --uri produced 60+ refusals — "the host quant-domain-poc.ddev.site:33000 matches no domain… this would have been written to project static-test-a". Without it every node would have landed in Client A's project.

Found a failure state the routing work can't see. Domain Access decides which domain serves which node via node grants, and enabling it leaves grants stale until someone rebuilds. Until then every domain serves every page, so a seed collects all of it and publishes one client's content into another's project — routed correctly, so nothing errors. My first live seed pushed all 63 nodes into static-test-a for exactly this reason. After node_access_rebuild() the same seed collected 23, being clienta's 20 plus the three unrestricted originals.

Good news in that: the seed does respect domain access once grants are current, so per-domain content scoping works.

Handling it as a warning in two places — the status report, and an error on quant:seed-queue where the damage happens and someone is watching. Not rebuilding grants from an update hook: it rebuilds every node's grants, which holds a deploy open on a large site, and it would run for every site with Domain enabled including ones that are fine. Not refusing to publish either, unlike the unrecognised-host guard — an unrecognised host has no legitimate reading, a pending rebuild does, and it stays set until an admin acts.

98 unit/kernel tests, 41 e2e cases, phpcs clean. Release notes updated with the setup step. Pushed in 1a73beb.

The previous commit only warned about stale Domain Access grants, on the grounds that the flag means grants may be stale rather than that content is mis-scoped. That reasoning holds for the flag. It does not hold for the content itself: Domain Access records which domains serve a node, so a node naming other domains and not this one is being published to the wrong project, whatever the cause. That is evidence, not suspicion, and it is now refused. Checked per entity, in the guard subscriber every publish passes through, so it covers seeds, cron, Tome, live saves and anything else. Silent on everything it cannot prove: no Domain Access, fewer than two domains, no active domain, no assignment on the entity, or content marked for all affiliates. A single-domain site is untouched. Verified both directions, which matters because a wrong refusal stops a customer publishing. With grants correct, all three domains seeded with zero refusals. With grants in the unrebuilt state Drupal actually leaves — one fallback row granting everything — clienta's seed collected all 62 nodes, 39 foreign pages were refused, none reached the project, and its own 47 published. Rebuilding grants returned it to zero refusals. The matrix gained that case, including the recovery. Writing it turned up one more behaviour worth knowing: content assigned to a domain that no longer exists is refused too, because nothing can say where it belongs. That does not arise from the UI, since deleting a domain strips the assignment from every node, which is itself worth knowing before anyone deletes one on a live multi-client site. 101 unit and kernel tests, 44 regression cases, phpcs clean.

stooit · 2026-09-01T19:59:29Z

One more in 86c1254, tightening the Domain Access handling from a warning to a refusal.

Previous commit only warned, on the basis that node_access_needs_rebuild() means grants might be stale rather than that content is mis-scoped. That's true of the flag, but not of the content — Domain Access records which domains serve a node, so a node naming other domains and not this one is provably going to the wrong project. That's evidence, so it's now refused.

Checked per entity in the guard subscriber, so it covers seeds, cron, Tome, live saves, everything. Stays silent on anything it can't prove: no Domain Access, fewer than two domains, no active domain, no assignment, or content marked for all affiliates. Single-domain sites unaffected.

Tested both ways, since a wrong refusal stops someone publishing:

grants correct   clienta 36 published / 0 refusals
                 clientb 24 published / 0 refusals
                 clientc 25 published / 0 refusals

grants stale     clienta sees all 62 nodes
                 39 foreign pages refused, 0 published
                 its own 47 published

after rebuild    0 refusals, back to normal

Stale state replicated the way Drupal actually leaves it — node_access truncated to a single fallback row granting everything. That case is in the e2e matrix now, recovery included: 44 cases.

Two things worth knowing before anyone builds on this:

Content assigned to a domain that no longer exists is also refused, since nothing can say where it belongs.
That doesn't arise through the UI, because deleting a domain strips field_domain_access from every node. Domain Access behaviour, not ours, but an unpleasant surprise on a live multi-client site.

101 unit/kernel tests, 44 e2e cases, phpcs clean, CI green.

quantcode-agent

Re-review — PR #254 "Fix cross-project content leakage on multi-domain sites"

Verdict: COMMENT — no blockers. Everything below is a Warning or a nit; none of it needs to gate the merge.

Reviewed 2.x (ea49ac8) → head (86c1254). The delta since the last review is b6b3d3c..86c1254, and it is not housekeeping — three commits add a whole new refusal mechanism (belongsToAnotherDomain(), +134 lines in PublishGuard.php) and rewrite the per-domain project resolver. That code is unreviewed, so it gets full rigor below; prior-reviewed code is only revisited for resolution status.

Status of prior feedback

Finding	Status	Evidence at HEAD
@steveworley: `getProjectForDomain()` guarded on the Domain 3.x-only `domain.config_factory_override` service + `getOverride()`	✅ Resolved	`b6b3d3c:140-147` had `$container->has('domain.config_factory_override')` and `->getOverride($domainId, 'quant_api.settings')`. Both gone. `QuantPurger.php:181-193` now reads both storage layouts directly — 3.x collection at 184-186, 2.x config object at 190-192
`QuantPurgerProjectTest` only exercised the fallback	✅ Resolved	`testResolvesDomain2xOverride:115`, `testResolvesDomain3xOverride:127`, `testEachDomainResolvesItsOwnProject:141` added
W1 — `RouteItem` prepend-before-normalise	✅ Resolved	`RouteItem.php:52` normalises, then `55-56` prepends only when `parse_url($route, PHP_URL_SCHEME)` is empty. Ordering is now correct and the docblock at 49-51 records why
Nit — stale `_quant_run_with_request()` docblock, missing `@param`	✅ Resolved	`quant.module:249-250` documents `$request`; body at 252
Nit — stale `@covers ::targetsActiveProject`	✅ Resolved	Now `@covers ::assertTargetsActiveProject` at `QuantSeedWorkerProjectGuardTest.php:148, 166, 190, 205, 219`, matching the real method at `QuantSeedWorker.php:64`
Nit — `assertFalse(hasService('domain.config_factory_override'))` asserted the old implementation	✅ Resolved	Present at `b6b3d3c:92`, absent at HEAD

Findings

W1 — the 2.x read may miss per-language overrides (conditional). QuantPurger.php:189-192 reads exactly one config name:

    yield $container->get('config.factory')
      ->get('domain.config.' . $domainId . '.quant_api.settings')
      ->get('api_project');

I read the whole generator body (181-193): there are two yields — one for the 3.x collection and this one — and no domain.config.<id>.<langcode>.<name> candidate. langcode does not appear anywhere under modules/quant_purger/. My understanding is that Domain 2.0.x loadOverrides() tries the language-qualified name first, which would mean a site with per-language project overrides resolves to the base project instead. I could not verify this — the domain module is not vendored in this checkout, so treat it as a question rather than a defect: if 2.0.x does check the langcode form, a third yield for domain.config.<id>.<langcode>.quant_api.settings closes it; if it doesn't, disregard.

W2 — $GLOBALS['config'] overrides are invisible to a storage read. QuantPurger.php:149 uses getOriginal('api_project', FALSE), which by design strips overrides, and the domain reads at 184-192 go to storage directly. $GLOBALS appears nowhere in the tree. For domain overrides that is the intended behaviour and the docblock at 166-171 argues it well; the part that isn't covered is a settings.php global override of api_project, which a storage read cannot see either. Probably fine in practice — worth a sentence in the docblock so the next reader doesn't have to work it out.

W3 — assigned domain IDs are never checked against existing domains (conditional reachability). PublishGuard.php:183 takes the raw field values:

    $assigned = array_column($entity->get('field_domain_access')->getValue(), 'target_id');

and 196-202 refuses whenever $active is not among them. There is no load-and-filter step: the file's only three getStorage('domain') call sites are 73 (refuses()), 135 (nodeGrantsAreStale()) and 222 (activeDomainId(), which only counts), and none validates $assigned. So a node whose sole assignment names a since-deleted domain can never match any active domain and is refused everywhere, permanently. Whether that state is reachable depends on whether domain_access purges node field values when a domain is deleted, which I could not verify from this clone — if it does, this is unreachable and you can ignore it. Two knock-ons that hold either way: the @owners placeholder at DomainGuardSubscriber.php:104-107 can name a domain that no longer exists, and the refusal is silent to the operator — it logs an error and calls stopPropagation() at 109, with nothing on Drush output, unlike the stale-grants warning printed at QuantDrushCommands.php:219. Intersecting $assigned against loaded domain entities would make the failure mode "publishes, unassigned" rather than "silently refuses forever".

W4 — the refusal path has no phpunit coverage. Every belongsToAnotherDomain() case in PublishGuardWriteTest.php asserts FALSE: testNothingRefusedWithoutDomainAccess:149, testEntityWithoutTheFieldIsPublished:162, testNonEntityIsIgnored:173. DomainGuardSubscriberTest.php has twelve tests, none touching it. Grepping the whole tests/ tree for belongsToAnotherDomain, field_domain_access and domain_all_affiliates turns up only those negative asserts plus regression.sh, and no test in the suite installs domain or domain_access — PublishGuardWriteTest.php:28-39 installs neither. The positive case does exist, at tests/regression/regression.sh:332-400, and it is a real end-to-end check (fabricates the unrebuilt node_access fallback row at 361-366, then asserts no /b- pages reach the project at 377) — but .github/workflows/ci.yml installs only token, purge and core-dev, runs vendor/bin/phpunit at line 75, and never invokes the harness, which additionally self-skips at 399 when domain_access is absent. So the mechanism this PR exists to add is currently only verified by hand. The docblock at PublishGuardWriteTest.php:120-129 states this honestly, which I'd rather have than a silent gap. Recommend a kernel test that installs domain/domain_access, creates two domains, and covers refuse / must-not-refuse-when-all-affiliates / must-not-refuse-when-assigned-here.

W5 — a breaking behaviour change with nothing in-tree telling operators. Going from warn (1a73beb) to refuse (86c1254) means content that published yesterday stops publishing after upgrade. git ls-tree -r refs/rev/prhead matches nothing for release, changelog, notes, upgrading or history; the tree contains exactly three .md files (README.md, modules/quant_purger/README.md, tests/regression/README.md), and the root README.md is untouched across the entire PR — it mentions neither refusal nor domains. The delta does add tests/regression/README.md:56-78, but that is harness instructions, not an operator-facing note. The PR body says "Release notes updated / Drafted for 2.1.0" — if those live outside the repo that's fine, but nothing in-tree carries the warning, and quant_requirements() at quant.install:16-37 only fires for stale grants, not for the refusal itself. Please make sure the warn→refuse change is called out where operators upgrading will see it.

What's good

Dropping version sniffing for a dual storage read is the durable fix, and the docblock at QuantPurger.php:166-171 explains why the service route was rejected — calling setDomain() on a 2.x shared singleton would change what every other config read in the request resolves to. That reasoning is the valuable part.
Deleting the assertFalse(hasService(...)) assertion rather than leaving a test pinned to the old implementation.
belongsToAnotherDomain() is genuinely proof-first: six early returns before it will refuse anything — non-fieldable at 170-172, no field_domain_access at 174-176, all-affiliates at 179-181, empty assignment at 186-188, no active domain at 190-194, and active-domain-is-assigned at 196-198 — with activeDomainId() returning NULL below two domains at 225-227. I traced the UNPUBLISH dispatch at Utility.php:372, which passes a NULL entity, and it lands on 170-172 unaffected.
getSubscribedEvents() carries an explicit parity note at DomainGuardSubscriber.php:78-81 and a test enforcing it (testGuardCoversEveryPublishingEvent). That's the right way to stop a future event being added unguarded.
The report-vs-enforce split is well judged: stale grants warn (PublishGuard.php:111-114 reasons it out — the flag means grants may be stale, and refusing for that whole window would be worse), while per-entity proof refuses.

Nits (non-blocking)

PublishGuard.php:104-105 — "through node / grants." wraps mid-phrase.
PublishGuard.php:130-133 — the comment "With one domain there is nowhere else for content to go" sits above the entity_type.manager service check; the count check it describes is at 137-139.
QuantPurger.php:181 — $container is untyped in the signature while @param at 173 declares ContainerInterface.

Summary: @steveworley's blocker is genuinely fixed (not papered over — the new resolver test would have failed against the old code), and W1-RouteItem plus all nits from the prior round are resolved. The new Domain Access refusal logic is well-structured and proof-first, but its one demonstrable gap is W4 — the mechanism the PR exists to add is only exercised by a harness that CI doesn't run. W3 and W5 are the next most worth addressing. W1/W3 reachability hinges on upstream drupal/domain behaviour I couldn't verify from this checkout; please confirm those two against the installed module before deciding whether to act.

_{🤖 Automated re-review. Findings verified against source at head ref 86c1254.}

stooit added 11 commits August 11, 2026 14:13

quantcode-agent Bot reviewed Aug 12, 2026

View reviewed changes

stooit added 2 commits August 11, 2026 19:09

quantcode-agent Bot reviewed Aug 12, 2026

View reviewed changes

quantcode-agent Bot reviewed Sep 1, 2026

View reviewed changes

Navigation Menu

Uh oh!

Fix cross-project content leakage on multi-domain sites - #254

Fix cross-project content leakage on multi-domain sites#254
stooit wants to merge 18 commits into
2.xfrom
fix/multi-domain-project-routing

stooit commented Aug 11, 2026

Uh oh!

quantcode-agent Bot left a comment

Uh oh!

stooit commented Aug 12, 2026

Uh oh!

stooit commented Aug 12, 2026 •

edited

Loading

Uh oh!

stooit commented Aug 12, 2026

Uh oh!

quantcode-agent Bot left a comment

Uh oh!

steveworley commented Aug 14, 2026

Uh oh!

stooit commented Sep 1, 2026 •

edited

Loading

Uh oh!

stooit commented Sep 1, 2026

Uh oh!

stooit commented Sep 1, 2026

Uh oh!

stooit commented Sep 1, 2026

Uh oh!

quantcode-agent Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

stooit commented Aug 11, 2026

Why

The original bug

Defence in depth

Defects found on the way

Testing

Deliberately not in scope

Reviewer notes

Uh oh!

quantcode-agent Bot left a comment

Choose a reason for hiding this comment

Review: PR #254 — Fix cross-project content leakage on multi-domain sites

Verified as covered

Blockers

Warnings

Test coverage assessment

Nits (non-blocking)

Uh oh!

stooit commented Aug 12, 2026

Blocker 1 — normalizePath() corrupts absolute urls

Warning 2 — mismatched items deleted rather than requeued

Warning 3 — clear() not domain scoped

Warning 4 — createFromGlobals() trusts the raw Host header

Test gaps

Nits

Where it stands

Uh oh!

stooit commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Three write paths bypassed the guard entirely

The guard introduced a hazard of its own

Also from this pass

Where it stands

Uh oh!

stooit commented Aug 12, 2026

CI was red on every push since the phpunit job was added

Purge fan-out, verified end to end

A pre-existing issue found on the way, deliberately not fixed here

The regression harness is now in the repo

Release notes

Uh oh!

quantcode-agent Bot left a comment

Choose a reason for hiding this comment

Re-review — verification of the four prior findings (B1, W2, W3, W4)

B1 — normalizePath() mangling URL schemes — ✅ RESOLVED

W2 — processItem silently deleting mismatched items — ✅ RESOLVED

W3 — TrafficRegistry::clear() unscoped — ✅ RESOLVED

W4 — shutdown handler trusting the Host header — ✅ RESOLVED (web path)

Newly-observed issues (all Warnings/nits — none blocking)

W1 — normalizePath()'s absolute-URL guard is dead code at the RouteItem call site (src/Plugin/QueueItem/RouteItem.php:50-53)

W2 operational note

Nits

Uh oh!

steveworley commented Aug 14, 2026

Uh oh!

stooit commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stooit commented Sep 1, 2026

Uh oh!

stooit commented Sep 1, 2026

Uh oh!

stooit commented Sep 1, 2026

Uh oh!

quantcode-agent Bot left a comment

Choose a reason for hiding this comment

Re-review — PR #254 "Fix cross-project content leakage on multi-domain sites"

Status of prior feedback

Findings

What's good

Nits (non-blocking)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Blocker 1 — `normalizePath()` corrupts absolute urls

Warning 3 — `clear()` not domain scoped

Warning 4 — `createFromGlobals()` trusts the raw Host header

stooit commented Aug 12, 2026 •

edited

Loading

B1 — `normalizePath()` mangling URL schemes — ✅ RESOLVED

W2 — `processItem` silently deleting mismatched items — ✅ RESOLVED

W3 — `TrafficRegistry::clear()` unscoped — ✅ RESOLVED

W4 — shutdown handler trusting the `Host` header — ✅ RESOLVED (web path)

W1 — `normalizePath()`'s absolute-URL guard is dead code at the `RouteItem` call site (`src/Plugin/QueueItem/RouteItem.php:50-53`)

stooit commented Sep 1, 2026 •

edited

Loading