- last update
over 1 year ago 5 pass, 2 fail - Status changed to Needs review
over 1 year ago 3:16am 2 June 2023
The Fast404::pathCheck() method doesn't work properly with path-based views.
When you setup a new view in Drupal, an entry like this gets added to the router table:
name: view.some_view.page_1
path: /some/path
pattern_outline: /some/path
fit: 3
route: Serialized Object
number_parts: 2
On the contrary, if you setup a view that supports arbitrary arguments it would look something like this:
name: view.some_wildcard_view.page_1
path: /some/path/{param}
pattern_outline: /some/path/%
fit: 6
route: Serialized Object
number_parts: 3
Yet regardless of which type of route is setup here, fast404 won't block the page from getting fully bootstrapped.
I get where Fast 404 may fail in the latter case; or at least require additional lookups to validate arguments, but that could get tricky with the generic nature of the patterns. But in the former case, we should be able to definitively say that a view doesn't take additional arguments and therefore should get handled by fast404.
This is the code in question:
// If we have a database connection we can use it, otherwise we might be
// initialising it. We remove '/' from the list of possible patterns as it
// exists in the router by default. This means that the query would match
// any path (/%) which is undesirable.
$sql = "SELECT pattern_outline FROM {router} WHERE :path LIKE CONCAT(pattern_outline, '%') AND pattern_outline != '/'";
$result = Database::getConnection()->query($sql, [':path' => $path])->fetchField();
if ($result) {
return;
}
/some/page/some/page/does-not-exist and get the fully-bootstrapped 404 page instead of the fast 404 pageThis is obviously an issue for views, but I'm actually wondering if it's a larger issue with how the router table works in general, as other routes can take wildcards as well, but I haven't spent a ton of time studying how this table works within Drupal. If it's determined we could safely block all non-wildcard paths, then you could do something like this:
Change from:
WHERE :path LIKE CONCAT(pattern_outline, '%')
To:
WHERE :path LIKE CONCAT(pattern_outline, '%', '\%', '%')
This will only match patterns that have % in them, instead of the current greedy match.
But.... I don't know the full implications of this. Are there any cases where the generic pattern outline is getting matched intentionally without the wildcard? I'd assume it shouldn't be, but I'd say there could be some risk with that assumption. Further research should probably be done there.
Alternatively, if we wanted to only target views, we could modify the query to fetch multiple columns instead of the pattern outline, and then further inspect the response for views-specific behavior.
Needs review
3.0
Code
The change is currently missing an automated test that fails when run with the original code, and succeeds when the bug has been fixed.
Not all content is available!
It's likely this issue predates Contrib.social: some issue and comment data are missing.