CurrentPathStack::getPath should always return a path with leading slashes

+++ b/core/includes/theme.inc
@@ -1317,7 +1317,7 @@ function template_preprocess_html(&$variables) {
   else {
     $system_path = \Drupal::service('path.current')->getPath();
-    $variables['root_path'] = explode('/', $system_path)[1];
+    $variables['root_path'] = explode('/', $system_path)[1] ?? FALSE;

Both cases use path.current, could we enforce within that that it always returns a leading /?

This is touching an existing point about the incorrect documentation on \Drupal\Core\Path\CurrentPathStack::getPath(), I thought we had an issue but can't find it right now, which says it is without leading / but that's clearly wrong and code expects it to be otherwise.

\Symfony\Component\HttpFoundation\Request::getPathInfo() documents that there is a case when it returns an empty string, although I'm a bit confused about that.

+++ b/core/modules/block/tests/src/Functional/BlockTest.php
@@ -66,6 +66,34 @@ public function testBlockVisibility() {
+    $edit = [
+      'id' => strtolower($this->randomMachineName(8)),
+      'region' => 'sidebar_first',
+      'settings[label]' => $title,
+      'settings[label_display]' => TRUE,
+    ];
+    $this->drupalGet('admin/structure/block/add/' . $block_name . '/' . $default_theme);
+
+    $edit['visibility[request_path][pages]'] = '/user/*';
+    $this->drupalPostForm(NULL, $edit, t('Save block'));

why the split of the extra $edit addition and manually requesting the path first, can't we just put it all in the initial $edit definition and add the path to drupalPostForm() so it requests it first internally?

+++ b/core/lib/Drupal/Core/Routing/RequestContext.php
@@ -64,4 +64,24 @@ public function setCompleteBaseUrl($complete_base_url) {
+    return $path[0] !== '/' && $path !== '/' ? '/' . $path : $path;

could we use ltrim here and then just add back?

+++ b/core/lib/Drupal/Core/Path/CurrentPathStack.php
@@ -73,9 +74,11 @@ public function setPath($path, Request $request = NULL) {
   }
 
+
+

nitpick: extra spaces here.

Wondering if we want to merge the old issue about the documentation into this, it got stuck in 2018 and kind of makes sense to fix the documentation as we are fixing bugs the implementation?

Looks good to me. We can decide what to do with the other issue when this is committed? Possibly fine to close as duplicate although there's another change in those patches that I'm not sure still makes sense or not.

+++ b/core/lib/Drupal/Core/Routing/RequestContext.php
@@ -64,4 +64,24 @@ public function setCompleteBaseUrl($complete_base_url) {
+
+  /**
+   * Prepends a slash to the given path.
+   *
+   * @param string $path
+   *   The original path.
+   *
+   * @return string
+   *   The path with a leading slash.
+   */
+  public static function prependSlash($path) {
+    return '/' . ltrim($path, '/');
+  }

I think we should have a change record for this, and a small release note - seems like there's a small chance that contrib or custom code could be relying on the buggy behaviour (which I think the 500 error in core overrides).

Done change record and release note snippet.

Looks like this was set back to need work due to a random fail?

Rebased on 9.2.x, also noticed that we were using a bunch of deprecated methods in the test and updated that.

Looks like new checks for spelling were added since #17

Fail was a rando

Followed step 2 and was able to successfully reproduce the error. After applying the patch, could verify that accessing index.php.php don't returns error anymore.

RTBC +1

Looks good to me thanks for the new patches

Please do not ask the testbot to try again until #3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed.

#3207086: [HEAD BROKEN] Consistent failure in MonthDatePluginTest is fixed. I have not reviewed the code.

Committed 3b40d8c and pushed to 9.2.x. Thanks!

Before publishing the CR I'm going to ping catch about this going in 9.1.x and maybe even 8.9.x since this bug is causing pages to be 200 when they should be a 404 - which makes this a major.

Thanks @alexpott, it looks like that commit link is 404ing, maybe a forgotten push?

Woke up thinking that I'd committed this prematurely so not going to push.

1. +++ b/core/lib/Drupal/Core/Path/CurrentPathStack.php
   @@ -46,14 +47,14 @@ public function __construct(RequestStack $request_stack) {
   -      $this->paths[$request] = $request->getPathInfo();
   +      $this->paths[$request] = RequestContext::prependSlash($request->getPathInfo());
   
   +++ b/core/lib/Drupal/Core/Routing/RequestContext.php
   @@ -64,4 +64,24 @@ public function setCompleteBaseUrl($complete_base_url) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function setPathInfo($path_info) {
   +    return parent::setPathInfo(static::prependSlash($path_info));
   +  }
   

   This changes deserve there own tests - unit or kernel somewhere.

2. +++ b/core/modules/block/tests/src/Functional/BlockTest.php
   @@ -68,6 +68,33 @@ public function testBlockVisibility() {
   +    // Test the page by requesting invalid paths.
   +    $this->drupalGet('index.php.');
   +    $this->assertSession()->statusCodeEquals(404);
   +    $this->drupalGet('index.php.php');
   +    $this->assertSession()->statusCodeEquals(404);
   

   I think this test only needs one of these requests. The other should be in \Drupal\Tests\system\Functional\System\IndexPhpTest because the 404 is not really part of the block visibility stuff and if we refactored this test in the future because of changes to how block visibility work then we might remove this important coverage.

I also woke up wondering if this is a Symfony bug. I think if we reported this to Symfony they would point us to https://github.com/symfony/recipes-contrib/blob/master/symfony/apache-pa... and say that Drupal shouldn't serve duplicated content on / and /index.php but I think regardless \Symfony\Component\HttpFoundation\Request::preparePathInfo() should always return a leading slash. I've opened https://github.com/symfony/symfony/pull/40750 to fix this upstream.

- Refactoring the tests as suggested in #34
- Added new test cases to validate prependSlash method.

- Uploading interdiff for the above patch.

Rerolled #37 for 9.5

The requested comments have been made so back to RTBC

As per #35 I think the correct place to fix this is upstream - see https://github.com/symfony/symfony/pull/40750 - I'll push on that again and see if I can get it moving.

Going to postpone this one depending on the outcome.

Re #44: fair, I do think we could get a workaround in core while we push for upstream, but up to you. Just a reroll for now to update this for D10. Patch still applied but is not compatible with Symfony 6.

We ran into this bug on a site when someone loaded an oddly formatted URL. In fairness it was a bot trying to find an attack vector on the site, but it still shouldn't have caused an error when it should have been displaying a 404 error page.

Patch in #46 totally puts a dent in this for us, we got around 600+ logs directly related to this. Thanks @Berdir.

Thanks @alexpott for taking this upstream as well though it looks like it might have stalled?

Rerolled the patch from #46 for 10.3. Applies successfully. Successfully addresses the test criteria still.

This finally got fixed in Symfony - in a security issue no less! https://github.com/symfony/symfony/security/advisories/GHSA-3rg7-wf37-54rm and 10.6 has the fixed version of Symfony.