Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed: Symlink to document root causing several update and install issues #3008

Closed
jenlampton opened this issue Mar 27, 2018 · 90 comments · Fixed by backdrop/backdrop#3708
Closed

Comments

@jenlampton
Copy link
Member

@jenlampton jenlampton commented Mar 27, 2018

Describe your issue or idea

When attempting to update on_the_web module using the updater at admin/modules/update, my update succeeded on my local site, but failed on my live site, with the following error:
File Transfer failed, reason: path/to/www/modules/contrib/on_the_web is outside of the path/to/docroot.

That error is incorrect, as on this server www is a symbolic link to docroot.

I suspect there's something about this symlink that's causing the problem. Noting the issue here incase anyone else is experiencing something similar.


Besides module updates, this problem also affects:

  • core updates
  • theme updates
  • layout updates
  • module/theme/layout installs via URL (via dialog "Manual installation")

PR by @bradbulger: backdrop/backdrop#3690 (this wider change has its own issue now)
PR by @indigoxela: in same function, but simpler backdrop/backdrop#3706 (outdated)
PR by @indigoxela: fixes it in the updater, not in file transfer: backdrop/backdrop#3708

@bradbulger
Copy link

@bradbulger bradbulger commented Dec 4, 2018

I have a similar problem in, I think. I'm running the site at https://localhost/mydirectory/. Apache looks in /var/www/html for localhost directories. /var/www/html/mydirectory is a symlink to /home/www/mydirectory for reasons.

I changed FileTransfer's checkPath function to run everything through backdrop_realpath() before comparisons and that stopped the problem. I'm hoping that means I'll be able to move modules/contrib and modules/custom back up above the docroot and use symlinks for them, which was my original problem (I thought).

@padprint
Copy link

@padprint padprint commented Aug 26, 2019

just a note to say this is still a problem:
File Transfer failed, reason: /web/reiff7/modules/job_scheduler is outside of the /usr/local/www/apache24/data/reiff7

My development server has a symbolic link to the docroot that throws this every time I try (after forgetting) to update modules.

@klonos
Copy link
Member

@klonos klonos commented Aug 26, 2019

Thanks for taking the time to confirm this @padprint 👍

@oadaeh
Copy link

@oadaeh oadaeh commented Aug 27, 2019

I have not looked into this issue, but in case it helps I will say that PHP de-references symbolic links when it encounters them. I have been able to work around some of them in the past, but it usually involved painful contortions.

@bradbulger
Copy link

@bradbulger bradbulger commented Oct 17, 2019

I believe I have a fix for this but I want to test it under more circumstances. I can't figure out what kind of configuration would result in the FileTransfer object having a value for its "chroot" property. If anyone has thoughts about how to create an installation so that it would be set to something, that would be helpful.

@olafgrabienski
Copy link

@olafgrabienski olafgrabienski commented Mar 25, 2020

Referencing a related forum post: https://forum.backdropcms.org/forum/error-update-modules-1and1

@olafgrabienski
Copy link

@olafgrabienski olafgrabienski commented Nov 23, 2020

I've tested the PR on a site hosted by Strato, and it fixes the problem for me. Here's what I did:

  1. Made sure I had a site with an outdated module.
  2. Went to admin/modules/update, hit the checkbox of the module, let Backdrop download the update, and tried to continue.

Result: Backdrop stopped with a file transfer error.

After applying the PR to my site and repeated the steps mentioned above, the update went just fine.

@bradbulger Do you need more details about the directory structure of the site to evaluate the fix further?

@bradbulger
Copy link

@bradbulger bradbulger commented Nov 23, 2020

Is the website document root a symlink to another location? That's how our sites are set up.

@olafgrabienski
Copy link

@olafgrabienski olafgrabienski commented Nov 24, 2020

Is the website document root a symlink to another location?

I'm not sure. It's a shared hosting, and I don't know how they organize the directories. Do you see if it's a symlink thing reading the following message?

/home/strato/http/premium/rid/81/23/SOME-LONG-ID/htdocs/backdrop-dev/modules/backup_migrate is outside of the /mnt/web214/c3/23/SOME-LONG-ID/htdocs/backdrop-dev

As I said, your PR fixes the issue for me. Not sure if and how to test further.

@bradbulger
Copy link

@bradbulger bradbulger commented Nov 24, 2020

That looks like it's the case, yes - two paths to the same location.

@olafgrabienski
Copy link

@olafgrabienski olafgrabienski commented Dec 18, 2020

@jenlampton Are you maybe up to have a look at the code of the PR? And/or label the issue with a milestone for the next bugfix release? (I've added the milestone candidate label.)

@klonos klonos added this to the 1.17.5 milestone Dec 18, 2020
@indigoxela
Copy link
Member

@indigoxela indigoxela commented Dec 19, 2020

I finally found the time to test this locally:

  1. Moved the web directory and set a symlink to the DocumentRoot (made sure the site still works)
  2. Installed an outdated version of a module via UI - that worked fine BTW
  3. Went to admin/reports/updates - the available update shows up
  4. Tried to run the updater - failure on core/authorize.php (see screenshot below)
  5. Applied the patch
  6. Back to admin/reports/updates - the available update shows up
  7. Tried to run the updater again, no failure this time, got a success message
  8. Back to admin/reports/updates - the available update still shows up 😕
  9. Repeated step 7 and 8, same result...
  10. Flushed "Update data"
  11. Repeated step 7 and 8, same result, the update still hasn't been done, the module version was still old

Odd.
I exported database table "cache_update" to figure out what's going on there... I suspect, the update cache is the culprit here, but that looked OK.

OK, another testing approach:

  1. Uninstalled that module
  2. Flushed all caches
  3. Enabled that module again
  4. Ran cron, available update shows up
  5. Ran update again, got success message
  6. Back to admin/reports/updates - the available update still shows up
  7. Verified a final time that the module has not been updated (checked the module's info file)
  8. Nothing in dblog, no errors

Can anyone confirm this behavior? Note: it might be unrelated, but I'd like to verify that the PR not only makes the error go away, but also gets the update working.
Funny enough, that installing modules works without any problem without this patch.

This error got fixed, though:
update-failed

@indigoxela
Copy link
Member

@indigoxela indigoxela commented Dec 19, 2020

Yet another approach

  1. I manually downloaded an outdated version of a theme
  2. Then I enabled the "Also check for updates of disabled modules, themes, and layouts" setting and ran cron
  3. Ran the update
  4. Verified that the disabled theme has been updated 👍

I need to dig a little deeper, but I'm already (almost) convinced, that the odd behavior described in my previous comment is totally unrelated to this PR.

UPDATE: Success, I installed yet another module manually (downloaded an old zip file), enabled it and ran cron.

Update showed up, updater ran without problems, update really happened - module got replaced.

So: works for me, too. 👍

@olafgrabienski
Copy link

@olafgrabienski olafgrabienski commented Dec 20, 2020

@indigoxela Thanks for testing the PR!

After having tested with the other approach, do you think the initial behavior of not updating is indeed unrelated?

@indigoxela
Copy link
Member

@indigoxela indigoxela commented Dec 20, 2020

After having tested with the other approach, do you think the initial behavior of not updating is indeed unrelated?

Yes, unrelated. It has been caused by some sort of update caching, although I don't fully understand, why.

Regarding the PR code:

@bradbulger that's a lot of code to solve a single problem (de-referencing a symlink, if I get it right?).

What baffled me when trying to test the PR was, that installing new projects works without any problems, only the updater fails. Could that mean that the base problem is somewhere else (authorize.inc?) and the solution is already there - somewhere in the installer code?

I admit, I'm not familiar with that part of Backdrop and probably can't do code review here.

@indigoxela
Copy link
Member

@indigoxela indigoxela commented Dec 20, 2020

Maybe interesting in this context is an older issue, which caused BACKDROP_ROOT in authorize.php to be switched from getcwd() - which resolves symlinks, to dirname(dirname($_SERVER['SCRIPT_FILENAME']))) - which doesn't.

@bradbulger
Copy link

@bradbulger bradbulger commented Dec 21, 2020

That's mostly debugging code - as I said, I don't feel I was able to test all possible circumstances. This was my local patch that we are using on my site. It could be trimmed down considerably. If there is some deeper or more fundamental problem, whoever finds it would be welcome to fix it.

@jenlampton
Copy link
Member Author

@jenlampton jenlampton commented Jan 14, 2021

I think the new error handling looks great, the PR is RTBC from me. Let's get some core committer eyes on it to see what they think.

@quicksketch
Copy link
Member

@quicksketch quicksketch commented Jan 16, 2021

I'm going to have spend a little time reviewing this PR. I don't think I should hurry it into today's release.

@bradbulger
Copy link

@bradbulger bradbulger commented Aug 28, 2021

What about @quicksketch's changes to core/misc/zen-ci/init_test.sh, btw? in PR 3690

@indigoxela
Copy link
Member

@indigoxela indigoxela commented Aug 28, 2021

What about @quicksketch's changes to core/misc/zen-ci/init_test.sh, btw?

Hm, good question. We're currently trying to switch from Zen.CI to Github Actions, so any effort in that direction may be futile. 🤔

@indigoxela
Copy link
Member

@indigoxela indigoxela commented Aug 28, 2021

Maybe it's necessary to better explain my current concerns with the changes in FileTransfer class:

Abstract class FileTransfer is used for all sorts of - ehm - file transfers, SSH, FTP...
And there's the API (hook_filetransfer_info, hook_filetransfer_info_alter), which makes it hard for me to understand, where these changes strike and might break something.

That checkPath is a protected final doesn't make it easier for us, because the possible places to fix something are limited.

The way I currently understand things - checkPath isn't actually meant to get fed with paths containing unresolved symlinks (correct me if I'm wrong). At least at the time this class was written, all paths already were real file system paths - note: that's an assumption. It's unclear to me, where or if stream wrappers play a role here.

The FileTransfer classes so far don't care about BACKDROP_ROOT in any way, it's the updater module that feeds in paths still containing symlinks. And that's the root cause for the problem this issue tries to fix - failing updates.

Call me coward 😨, but currently I'm leaning towards the workaround-ish change in the Updater classes (PR 3708), as this gives me confidence, that we don't accidentally break something else - something we might not be aware of yet.

But if there are good arguments, why the change has to be in checkPath, I'm all ears.

@bradbulger
Copy link

@bradbulger bradbulger commented Aug 28, 2021

@indigoxela
Copy link
Member

@indigoxela indigoxela commented Aug 30, 2021

Why keep the symlink in the definition of BACKDROP_ROOT in the first place?

In Issue #1297 it was discovered that it's impossible to install Backdrop when the "core" directory is outside the docroot. I'm not sure, why this was important, but since then (2016) it works that way and changing back from dirname(dirname($_SERVER['SCRIPT_FILENAME'])) to getcwd() would break that.

I think it would be OK to separate the problems, and open a separate ticket about checkPath().

Yeah, that problem seems to need a little more (and thorough) discussion, while the module update problem should (and could) get fixed quickly - even if that means we settle on the workaround for now. @bradbulger what do you think, shall we switch to backdrop/backdrop#3708 here and open a new issue for your PR and checkPath()?

@bradbulger
Copy link

@bradbulger bradbulger commented Aug 30, 2021

I understand about #1297. I'm suggesting that you can use realpath() around the dirname(...) result.

But in any case, yes, I'll open a new issue about checkPath() and close the PR on this ticket.

@olafgrabienski
Copy link

@olafgrabienski olafgrabienski commented Aug 30, 2021

I'm not sure about the current status. Is there consensus that PR 3708 is the way to go? And would it then make sense to test it manually in my shared hosting, or is it already sufficiently tested according to #3008 (comment)?

@indigoxela
Copy link
Member

@indigoxela indigoxela commented Aug 31, 2021

@olafgrabienski sorry about the confusion. Yes, backdrop/backdrop#3708 is the one to test here.

Testing in real (and different) environments is totally welcome - there can never be too much testing. 👍

@olafgrabienski
Copy link

@olafgrabienski olafgrabienski commented Sep 1, 2021

Let's see: After applying backdrop/backdrop#3708 in my shared hosting environment, I was able to update modules using the UI 👍 , but not Backdrop core.

After changing also line 17 of system.updater.inc, I was able to update core.

- return BACKDROP_ROOT;
+ return realpath(BACKDROP_ROOT);

Not sure if this is reasonable, though.

However interesting that updating core fails in my environment. Brad mentioned in #3008 (comment) it worked with the same PR. Maybe my environment is different? In case it helps, I get an error message like the following when trying to update core with the PR:

File Transfer failed, reason:
/kunden/homepages/some-id/another-id/htdocs/backdrop is outside of the
/homepages/some-id/another-id/htdocs/backdrop

@olafgrabienski
Copy link

@olafgrabienski olafgrabienski commented Sep 1, 2021

Side note: In my environment backdrop/backdrop#3708 fixes also the manual module installation using a URL.

@indigoxela
Copy link
Member

@indigoxela indigoxela commented Sep 1, 2021

I was able to update modules using the UI , but not Backdrop core.

Many thanks for testing! 👍 Yes, the change in line 17 seems reasonable. Will do some more checks locally, to make sure it also works when "core" itself is a symlink - to prevent possible side effects (#1297 again).

...3708 fixes also the manual module installation using a URL.

Another nice catch, many thanks for the hint. Will also check installation using URL.

@indigoxela
Copy link
Member

@indigoxela indigoxela commented Sep 1, 2021

Confirmed, realpath(BACKDROP_ROOT) in class CoreUpdater fixes broken core updates, when the docroot is a symlink.

I verified that there are no side-effects, when "core" itself is a symlink with following setup:

  • The docroot is a real directory
  • Core is outdated (1.19.2), patch applied
  • Move "core" outside and create the symlink: ln -s ../core/ core
  • Flush all caches
  • Update core via UI - it works
  • Also check the actual files in file system - everything as expected

And, yes, installing from URL does indeed work now. I wasn't aware that this is also broken since 2016 with docroot symlinks.

Tests are "passing" - besides the usual internal server error and one of our "random buddies". 🙄

@indigoxela indigoxela changed the title symlink to document root causing module update issues Symlink to document root causing several update and install issues Sep 1, 2021
@bradbulger
Copy link

@bradbulger bradbulger commented Sep 1, 2021

I never tried to update core because I assumed that the update would erase my patch halfway through. IF you got it working, that's good.

@indigoxela
Copy link
Member

@indigoxela indigoxela commented Sep 2, 2021

I never tried to update core because I assumed that the update would erase my patch halfway through.

After the update all patches are gone, of course, but the update itself is working fine. (All these tests should obviously never be done on a production site.)

@indigoxela
Copy link
Member

@indigoxela indigoxela commented Sep 7, 2021

Rebased. Some random test failures, but still ready for review.

@quicksketch
Copy link
Member

@quicksketch quicksketch commented Sep 15, 2021

Yay, looks like we're all agreeing that backdrop/backdrop#3708 is a simple and safe fix. Although I think more comprehensive symlink handling would be good, this fixes the problem that everyone is experiencing specifically with Installer and Update modules.

I've merged backdrop/backdrop#3708 into 1.x and 1.19.x. Huge thanks especially to @bradbulger and @indigoxela for their patience on this and working together to arrive at a solution. Kudos to @olafgrabienski, @padprint, @BWPanda, @jenlampton, and @klonos for their help validating and testing solutions!

@jenlampton jenlampton changed the title Symlink to document root causing several update and install issues Fixed: Symlink to document root causing several update and install issues Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment