Generated url cache context is not correct for relative urls from the UrlGenerator

Open on Drupal.org →

Created on 16 October 2016, over 8 years ago

Updated 27 July 2023, over 1 year ago

Problem/Motivation

By accessing a site an an unexpected base path (e.h. with index.php) I may be able to force links to be cached that way and that could be considered a minor site defacement and possibly lead to a duplicate content SEO penalty too.

Expected behavior:

If I am on the homepage / using clean URLs (index.php is not in the URL)
The link to "My Account" is using a clean URL also

If manually navigate to /index.php
The link to "My Account" is rendered with /index.php in the path

If I clear cache and manually navigate to /index.php
And then navigate to the clean URL /
The link to "My Account" is using a clean URL also

Actual behavior:

If I am on the homepage / using clean URLs (index.php is not in the URL)
The link to "My Account" is using a clean URL also

If manually navigate to /index.php
The link to "My Account" is NOT CORRECTLY rendered with /index.php in the path

If I clear cache and manually navigate to /index.php
And then navigate to the clean URL /
The link to "My Account" is NOT CORRECTLY using a clean URL also

Only rendered absolute URLs reflect the index.php in the base URL because that's the only time the UrlGenerator sets the cache context.

Steps to reproduce

Rebuild caches
Add index.php immediately after the domain name. E.g. https://site.tld/index.php
Now click on any link on the frontend (node, menu items, etc.) and observe that index.php is persisting in the URL
Manually remove it from the URL and click on a few more links
Observe that it's coming back almost always with the default theme (Bartik)
Observe that on backend links it does not seem to be interfering, except on the first link you click on when you're still within Bartik (which makes sense)

Proposed resolution

Add the cache context 'url.site' to all GeneratedUrl objects in \Drupal\Core\Routing\UrlGenerator::generateFromRoute except in the _no_path case

OR

define a new cache context that depends on the base url of the request, but not the scheme and host.

OR

Close this issue in favor of a new feature allowing Drupal to 403 on an unexpected base URL via settings.php like the trusted hosts setting

Remaining tasks

Write a test showing the bug

User interface changes

none

API changes

none

Data model changes

none

🐛 Bug report

Status

Needs work

Version

11.0 🔥

Component

Last updated about 14 hours ago

Maintained by
🇬🇧United Kingdom @catch
🇧🇪Belgium @kristiaanvandeneynde

Created by

🇺🇸United States pwolanin

Live updates comments and jobs are added and updated live.

D8 cacheability

Needs tests
The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.

Needs framework manager review
It is used to alert the framework manager core committer(s) that an issue significantly impacts (or has the potential to impact) multiple subsystems or represents a significant change or addition in architecture or public APIs, and their signoff is needed (see the governance policy draft for more information). If an issue significantly impacts only one subsystem, use Needs subsystem maintainer review instead, and make sure the issue component is set to the correct subsystem.

Needs Review Queue Initiative
Used to track the progress of issues reviewed by the Drupal Needs Review Queue Initiative.

Needs change record
A change record needs to be drafted before an issue is committed. Note: Change records used to be called change notifications.

Incomplete comments

Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

Comment almost 2 years ago →
🇺🇸United States smustgrave
Seems last patch had a number of failures.

Adding a new service will require a change record.
Comment almost 2 years ago →
🇫🇷France andypost
Comment almost 2 years ago →
🇫🇷France andypost
Proper re-roll to get number of failed tests, looks a lot of tests needs to add new context
Status changed to Needs review almost 2 years ago12:20pm 27 February 2023
Comment almost 2 years ago →
🇫🇷France andypost
fix some tests
Comment almost 2 years ago →
🇫🇷France andypost
Fix CS
Comment almost 2 years ago →
System Message
The last submitted patch, 36: 2819197-36.patch, failed testing. View results →
Status changed to Needs work over 1 year ago5:42am 27 July 2023

Comment over 1 year ago →

🇨🇭Switzerland berdir Switzerland

+++ b/core/modules/block/tests/src/Functional/Views/DisplayBlockTest.php
@@ -318,7 +318,7 @@ public function testBlockEmptyRendering() {
     // empty block.
     $this->assertCacheTags(array_merge($block->getCacheTags(), ['block_view', 'config:block_list', 'config:views.view.test_view_block', 'http_response', 'rendered']));
-    $this->assertCacheContexts(['url.query_args:_wrapper_format']);
+    $this->assertCacheContexts(['url.base', 'url.query_args:_wrapper_format']);

This is an expensive fix if we end up with this cache context pretty much everywhere, leads to a ton of cache redirects. Would pretty much need to consider making this a required cache context then.

contrib.social Blog FAQ Discussions

Production build 0.71.5 2024