[D7] Full path disclosure from errors on maintenance pages

Adding a tag that the D8+ issue had.

Thanks, new branch / MR which adds a helper function to strip DRUPAL_ROOT from errors that will be displayed.

I'm not sure if replacing the actual path with the text DRUPAL_ROOT is less confusing than just removing it completely, but it could be a bit misleading if e.g. instead of:

/var/www/html/sites/default/settings.php

..we output:

/sites/default/settings.php

I don't think we want to use something like /path/to/drupal which would probably also confuse some people as it looks real but is not.

Open to suggestions but as it stands this will show an error like:

No such file or directory in include_once() (line 848 of DRUPAL_ROOT/sites/default/settings.php).

It might not be easy to test the exact scenario from the IS, but I'd think that we should be able to add a basic test that verifies this is working as expected when an error is displayed..

Thank you for reporting and working on this!

I have tested the MR also manually on a test site - checked the install.php, the update.php and a regular page served via index.php. I can confirm that with the patch from the MR, the DRUPAL_ROOT path is stripped from the error messages on all these three places correctly. I have also tested it on windows, where it seems to work as well.

Tests-only job seems to fail as expected: https://git.drupalcode.org/project/drupal/-/jobs/2975882

All other tests seems to pass: https://git.drupalcode.org/project/drupal/-/pipelines/302957

There is still a "Needs tests" tag, but I am not sure if we need more tests - do you think so? I would say the added tests covers the check.

I have also updated the issue summary with the current proposed resolution. Do we need a change record for D7? D11 fix did not have it, but since we are stripping this globally, should we consider it?

In overall, I think this looks good, thanks!

@ram4nd I think you've pushed a couple of unrelated branches to the MR remote? No harm done, but just wanted to make sure you're aware :)

@poker10 thanks for the review, and yes I think the testing looks okay as it is (I've removed the tag).

I don't love the fallback:

    else {
      // As a fallback, make sure DRUPAL_ROOT's value is not in the path.
      $error['%file'] = str_replace(DRUPAL_ROOT, 'DRUPAL_ROOT', $error['%file']);
}

...but my thinking was that if the value of DRUPAL_ROOT appears somewhere other than at the very beginning of a file path for some reason, it'd be pretty confusing to just make that part disappear. I'm not sure when this would ever happen, but let's say for some reason the docroot path is reflected in a longer path to a network mount or something like that..

I think it's probably worth leaving it in, but I've made a little tweak to the conditional to avoid the fallback running at all when DRUPAL_ROOT is not present in the string in the first place.

A wise man once told me to avoid assignments inside if conditions, but I'd argue it's worth doing here to avoid doing unnecessary work for every error.

If you're happy with this little tweak @poker10 (and assuming tests all pass) I think it's ready to commit.

I think we can keep the fallback as is, just to be sure.

Test-only test is still failing as expected: https://git.drupalcode.org/project/drupal/-/jobs/3119379
And others are green: https://git.drupalcode.org/project/drupal/-/pipelines/316825

Adding a tag for Needs change record - we are stripping the root path globally and there is a drupal_set_message call, which is something, what some sites can eventually use in a specific ways (probably to monitor such errors and so). If for any reason someone expects the paths to be the same, it can cause an issues. So I think that at least short CR will be beneficial here.

Otherwise it looks good as I wrote in #9. Thanks!

Committed this to 7.x, thanks everyone!

Keeping the Needs change record tag.

poker10 → closed merge request !9766

Automatically closed - issue fixed for 2 weeks with no activity.

ram4nd → changed the visibility of the branch 3482184-d7-protect-pager to hidden.

ram4nd → changed the visibility of the branch jquery-371 to hidden.

ram4nd → changed the visibility of the branch drupal-3479425-3479425-d7-module-and to hidden.

ram4nd → changed the visibility of the branch 3479425-d7-module-and to hidden.