- Issue created by @ram4nd
- Merge request !9755Issue #3478716: [D7] Full path disclosure from errors on maintenance pages β (Open) created by ram4nd
- First commit to issue fork.
- Merge request !9766first pass at stripping DRUPAL_ROOT from errors that are displayed β (Open) created by mcdruid
- π¬π§United Kingdom mcdruid π¬π§πͺπΊ
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).
- π¬π§United Kingdom mcdruid π¬π§πͺπΊ
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..
- πΈπ°Slovakia poker10
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!
- π¬π§United Kingdom mcdruid π¬π§πͺπΊ
@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.