Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.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.phpimmediately 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.phpis 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
| Comment | File | Size | Author |
|---|---|---|---|
| #36 | 2819197-36.patch | 13.44 KB | andypost |
| #36 | interdiff.txt | 649 bytes | andypost |
| #35 | interdiff.txt | 6.82 KB | andypost |
Comments
Comment #2
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedHere's a possible simple fix. NR for bot (needs tests)
Comment #4
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedHmm, fun - that adds another cache context to a lot of things.
Probably need some feedback here from Wim Leers, berdir, or core maintainers about the relative importance of fixing this.
I stumbled on this via #2818185: Views methods FieldPluginBase::renderAsLink and renderText() uses base_path() when it should use $context->getBaseUrl()
It seems like views link rendering and and other things may not work as expected if the rendered url can get cached with an unexpected index.php in the base url.
Comment #5
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedFrom discussion with catch, bumping to 8.3 for now since it requires a new cache context to fix correctly.
Comment #6
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedback to NR for bot - expect I will see some of the same unit test fails
Comment #8
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedWith some possible test fixes
Comment #10
Wim LeersI'm pretty sure we discussed this very problem before, but if so, we obviously failed, because we didn't document our reasoning for not implementing the expected behavior described in the IS
This is what I think I remember:
http://example.comorhttp://somedomain.com/example, not bothhttp://example.comorhttp://example.com/index.php, not bothIMO this is up to framework managers to decide whether this is a bug or intentional. In any case, I think this is a very minor edge case.
Comment #11
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedSo assuming we get #2818185: Views methods FieldPluginBase::renderAsLink and renderText() uses base_path() when it should use $context->getBaseUrl() fixed soon also, this bug may cause Views to incorrectly render links if the site is accessed with different base paths.
Comment #12
pwolanin CreditAttribution: pwolanin as a volunteer and at SciShield commentedOr, we could 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
Comment #14
cilefen CreditAttribution: cilefen commentedComment #15
anavarreJust registering I've seen this issue ~5 times on different sites in the past month and very confused people not knowing what to do and certainly not looking at cached paths but instead .htaccess redirects or anything that would alter paths instead.
Comment #16
anavarreComment #25
andypostComment #27
karishmaamin CreditAttribution: karishmaamin at Specbee commentedRe-rolled against 9.4.x
Comment #31
Ankit.Gupta CreditAttribution: Ankit.Gupta at Dotsquares Ltd. commentedRerolled the patch #27 with Drupal 10.1.x
Comment #32
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems last patch had a number of failures.
Adding a new service will require a change record.
Comment #33
andypostComment #34
andypostProper re-roll to get number of failed tests, looks a lot of tests needs to add new context
Comment #35
andypostfix some tests
Comment #36
andypostFix CS
Comment #39
BerdirThis 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.