Fix cross-project content leakage on multi-domain sites - #254
Conversation
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[bot]](https://avatars.githubusercontent.com/u/63748100?s=60&v=4){kind=small}
There was a problem hiding this comment.
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:
DomainGuardSubscriberTestcovers unknown-host stops, single-domain bypass, no-domain-module, zero domains, port mismatch, and redirects both ways.QuantSeedWorkerProjectGuardTestcovers 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::UNPUBLISHis subscribed but never asserted — noUNPUBLISHtest anywhere intests/, yet the docblock atDomainGuardSubscriber.php:66-70calls 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;TrafficRegistryDomainTesttests 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— theuse Symfony\...\Request;insertion breaks alphabetical grouping of use statements.CliDomainContext::$initializedis set toTRUEbefore the early returns, so a call made before the container hasdomain.negotiatorpermanently 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 (FileItemTesthas 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.
|
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 —
|
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.
|
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 entirelyScope 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
The preconditions, all required together: two or more domains configured; an administrator with Same class as the unpublish gap. Guarding three more call sites would have repeated the mistake — that is now four times this pattern has bitten. So the decision moved into Applied to the seven methods that change something: Verified against two domains on an unrecognised host: The guard introduced a hazard of its ownA refused write returns an empty array, and Worth flagging as the general risk with defence in depth: the second layer has to fail as gracefully as the first. Also from this passStatic analysis over the changed files flagged a test constructing Re-checked but deliberately unchanged: Where it stands94 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 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.
|
Closed the last three gaps. Pushed in b6b3d3c. CI was red on every push since the phpunit job was addedWorth 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 Now green — Purge fan-out, verified end to endPreviously tested a layer at a time. Driven through a real cache tag invalidation: Note the second item: invalidating from Client A's context still resolves Client B's project correctly, which is the part A pre-existing issue found on the way, deliberately not fixed hereThe traffic registry only records requests carrying a Quant token, and the crawl only sends one when 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
39 cases, asserting routing rather than counts. Release notesDrafted 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:
Also flagged for reviewers: the earlier comment about |
There was a problem hiding this comment.
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$requestparameter (:248) has no@paramtag — likely aDrupal.Commenting.FunctionCommentphpcs failure; re-run the lint job.tests/src/Kernel/QuantSeedWorkerProjectGuardTest.php:148,166,205,219—@covers ::targetsActiveProjectreferences the old name; the method is nowassertTargetsActiveProject()(line190is already correct). Stale@coversare silently ignored, understating guard coverage (and fail under--strict-coverage).src/Plugin/QueueItem/RouteItem.php:96—$config->get('proxy_override') ?? FALSEis equivalent to the priorget('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.
|
The per-domain project resolution in
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 On the two-domain setup this PR targets, that means invalidating a cache tag for
|
2 participants
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:
quant:run-queueinherited no--uri, so they booted on the default domain regardless of the parent's context.getLockFileLocation()and the seed preparation read config throughgetEditable(), which bypasses overrides. Every domain shared one lock file, and each seeded with the base site's settings.QuantClientcaptured its credentials in the constructor. The container is built before the domain is negotiated, so the project it captured was always the base one.The root cause is partly external and easy to miss: the Domain module populates its negotiation context from a
kernel.requestsubscriber, and Drush never dispatches that event. Sodomain_configoverrides are absent under CLI even when--urinames 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 inDrupal\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.
DomainGuardSubscriberrefuses 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 wrongHostsilently 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_searchis installed. The hooks defer work withdrupal_register_shutdown_function, and Drupal pops the request off the stack once the response is sent. Contribtoken— aquant_searchdependency — collects token info during replacement, andsite:base-pathdescribes itself via\Drupal::request()->getBasePath(), which fatals on NULL. The shutdown handler can only reacherror_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 withoutquant_searchwere unaffected.Utility::getPageInfo()returned NULL against astringreturn type.$outputwas 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 toMerge::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
/aboutcollapsed 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. Update9103adds 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.settingsandquant_search.entities.settingshad none, andquant_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_searchnameddrupal:token, which the packaging facade reads as core and drops — published metadata for 2.0.0 requires nodrupal/tokenat all. Prefixes corrected, and acomposer.jsonadded, since the facade does not carry submodule dependencies up into the project requirement.CI never ran phpunit. The job named
phpunitinstalled the module and stopped, which is how a comprehensively broken test file went unnoticed.Testing
QuantClientTestwas broken before this branch — 6 errors and 3 failures of 20. It never calledreveal(), readgetStatusCodeas a property, builtRequestExceptionwith one argument, and expected requests withoutQuant-Project, withexceptionforexceptions, and without the/v1suffix. Rewritten to drive requests through a GuzzleMockHandlerand 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:
drush updb. 35 duplicates merged, column added, and all five legacy queue items survived and published.Deliberately not in scope
quantandquant_apidepend on each other, and the code matches. Breaking it means relocating shared classes.$entity->original,FormElement,NodeViewController,NodeStorage::revisionIds().\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
QuantApi::getSubscribedEvents(). A fourth event would go unguarded and nothing would fail loudly.composer.jsonchanges what the packaging facade contributes; worth checking the generated metadata on the next dev release.