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

Generate URLs via JavaScript #448

Merged
merged 1 commit into from
Jun 7, 2023
Merged

Conversation

kesselb
Copy link
Contributor

@kesselb kesselb commented Nov 13, 2022

Fix #265, Close #312

URL generation via PHP do not consider cases where the application URL at a reverse proxy is different from the local URL.

Example:

  • Public: home.acme/nextcloud
  • Private: 192.168.1.8

A reverse proxy forwards the request internally. As the updater is not installed in a subdirectory, the generated (via PHP) URLs are wrong. nextcloud/server has all the code to generate proper URLs for such cases. However, this code is not available here.

Generating the URLs via JavaScript is simpler as the browser is already visiting the right page.

To configure nginx-proxy to serve an application from a subdirectory for testing:

  - VIRTUAL_HOST=server2.internal
  - VIRTUAL_PATH=/cloud/
  - VIRTUAL_DEST=/

Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but didnt test

@szaimen
Copy link
Contributor

szaimen commented Nov 14, 2022

/rebase

@nextcloud-command nextcloud-command force-pushed the bug/265/generate-urls-via-js branch from 274bf9d to a60e1b4 Compare November 14, 2022 10:13
@szaimen
Copy link
Contributor

szaimen commented Nov 14, 2022

@come-nc @blizzz eho has the rights to merge this?

@blizzz
Copy link
Member

blizzz commented Nov 15, 2022

LGTM but didnt test

Would feel more confident if it was tested indeed ;)

@szaimen
Copy link
Contributor

szaimen commented Nov 17, 2022

/rebase

URL generation via PHP do not consider cases where the application URL at a reverse proxy is different from the local URL.

Example:
- Public: home.acme/nextcloud
- Private: 192.168.1.8

A reverse proxy forwards the request internally. As the updater is not installed in a subdirectory, the generated (via PHP) URLs are wrong. nextcloud/server has all the code to generate proper URLs for such cases. However, this code is not available here.

Generating the URLs via JavaScript is simpler as the browser is already visiting the right page.

Signed-off-by: Daniel Kesselberg <mail@danielkesselberg.de>
@fermulator
Copy link

I will test this weekend.

@fermulator
Copy link

fermulator commented Dec 17, 2022

TEST RESULTS:

  • Baseline Version: [Nextcloud Hub II] (23.0.7) (indeed, I am not always 100% keeping updated)

Caveats/Notes:

  • my starting version had /srv/nextcloud/updater/updater.php, but the changes here have updater.web.php

Pulling in Changes:

cd /srv/nextcloud/updater
mv index.php index.php.bak
wget https://raw.githubusercontent.com/nextcloud/updater/94058243a64fbac38685b69cd077fbccd7d4d935/index.web.php
mv index.web.php index.php
sudo chown www-data:www-data index.php
sudo chmod 644 index.php
sudo setfacl -b index.php

Then visit: https://NEXTCLOUD-URI/cloud/index.php/settings/admin/overview#
, click on "Open Updater"
which redirects to https://NEXTCLOUD-URI/cloud/updater/

❌ EMPTY / broken (the page is just a white/empty page)

If I restore the original version ... the updater page loads as expected.

Is there some sort of breaking changes on newer versions of the updater that would prevent it from working on an older core Nextcloud version? Potentially a compatibility issue here.

@fermulator
Copy link

Next Attempt: (try see if there is indeed a compatibility issue ...)

Conducted a few upgrades using the manual workaround steps to get to a supported version.
(and sometimes :/srv/nextcloud/updater$ sudo -u www-data php updater.phar)

Current version is 23.0.7.
Update to Nextcloud 23.0.12 available. (channel: "stable")

✔️

Current version is 23.0.12.
Update to Nextcloud 24.0.8 available. (channel: "stable")

✔️

^ so there, up to v24 (currently supported)

@fermulator
Copy link

fermulator commented Dec 17, 2022

Repeating test:

steps noted in #448 (comment)

[Nextcloud (Redbrick)](NEXTCLOUD-URI)24.0.8
A new version is available: Nextcloud 25.0.2

ends up with:

fermulator@nextcloud:/srv/nextcloud/updater$ ll
total 848
drwxrwx---+  2 www-data www-data   4096 Dec 17 16:13 ./
drwxrwx---+ 14 www-data www-data   4096 Dec 17 13:29 ../
-rw-r--r--   1 www-data www-data  32208 Dec 17 16:13 index.php
-rw-r--r--   1 www-data www-data  66320 Dec 17 13:29 index.php.bak
-rw-r--r--   1 www-data www-data  32208 Dec 17 12:50 index.php.new
-rw-r--r--   1 www-data www-data 723250 Dec 17 13:29 updater.phar

same problem ❌

(in my experience/past, this does usually mean there's a bug in the PHP/javascript/HTML somewhere ... hard to pinpoint)

@fermulator
Copy link

fermulator commented Dec 18, 2022

tried to enable error reporting but it isn't having any affect (tried index.html, index.php, and updater/index.php)
https://phoenixnap.com/kb/php-error-reporting

AH-HA ... wow ... so i was adding all this at the TOP of the file, but apparently we're randomly disabling it on line ~120

ini_set('display_errors', '0');
ini_set('log_errors', '1');

grrr

@fermulator
Copy link

fermulator commented Dec 18, 2022

https://docs.nextcloud.com/server/latest/admin_manual/configuration_server/config_sample_php_parameters.html#logging

OK here we go

tail -f /var/log/apache2/error.log

[Sat Dec 17 21:42:21.995174 2022] [php7:error] [pid 27624] [client 10.0.0.52:60376] PHP Fatal error:  Uncaught Error: Class 'Updater' not found in /srv/nextcloud/updater/index.php:121\nStack trace:\n#0 {main}\n  thrown in /srv/nextcloud/updater/index.php on line 121

@fermulator
Copy link

Last test for the day ... because of the Updater() class error there, I'm thinking indeed there is an incompatibility with this version of index.web.php with the version I'm running.

SO -- I manually patched my index.php (from version 24.0.8), with the changeset proposed by this PR; (the file is different enough that I couldn't do a git patch) -- took each diff, found it's respectively section, and applied

Current version is 24.0.8.
Update to Nextcloud 25.0.2 available. (channel: "stable")

Then visit: https://nextcloud-uri/cloud/index.php/settings/admin/overview#
, click on "Open Updater"
which redirects to https://nextcloud-uri/cloud/updater/

✔️ it worked!
nextcloud-updater-patched
nextcloud-updater-success

@fermulator
Copy link

/rebase

@fermulator
Copy link

@PVince81 - for your final review

@PVince81 PVince81 requested review from blizzz and skjnldsv December 20, 2022 07:40
@fermulator
Copy link

one other note. someone maybe should test to ensure this works in non-proxy scenario

@fermulator
Copy link

what's the hold up? now that we have a viable fix, we should really target this for delivery ASAP

@szaimen szaimen added this to the Nextcloud 26 milestone Jan 8, 2023
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@skjnldsv skjnldsv merged commit 2c5f141 into master Jun 7, 2023
@skjnldsv skjnldsv deleted the bug/265/generate-urls-via-js branch June 7, 2023 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Webbased Nextcloud updater fails in reverse proxy scenario with / vs. /something
5 participants