oc-1875 : running OwnCloud on an alias (or not) with Nginx [the real one] #385

illwieckz · 2012-11-13T02:46:51Z

This Pull Request is a similar to #22 (the #22 could be deleted).
This is another attempt to do things properly.
I hope this Pull Request is clean and done properly :

1 commit
dedicated branch
rebased and up to date with owncloud/core

For my first "Pull Request" attempt I wrote something like this :

Hi, this is the code that I described in my recent bug report:
http://bugs.owncloud.org/thebuggenie/owncloud/issues/oc-1875

Purpose is to use Owncloud as an alias with Nginx¹. The code calculate OC::$SUBURI and OC::$WEBROOT using $_SERVER

¹ Nginx or other HTTPd server

ghost · 2012-11-13T02:47:12Z

Can one of the admins verify this patch?

DeepDiver1975 · 2012-11-13T06:15:13Z

This is ok to test

DeepDiver1975 · 2012-11-13T07:12:24Z

Unit Tests are no longer executed.
Can you please have a look?
Thx

karlitschek · 2012-11-13T09:04:07Z

Thanks again for the patch. Can you document your code a bit more so that it is easier to understand what you want to do here? Also the -13 smells as you rely on having ownCloud installed in a specific folder or a specific host. You shouldn´t do that. The Url is completely flexible.

illwieckz · 2012-11-13T10:53:54Z

Hum sorry,
I did not write that line. This was the original code that I modified:

OC::$SERVERROOT=str_replace("\\", '/', substr(__FILE__, 0, -13));

is similar to

OC::$SERVERROOT=str_replace("\\", '/', substr(__FILE__, 0, strlen('/lib/base.php')));

But I did a mistake when applying my patch, I replace new code (based on DIR path, written by another than me) by old code (based on FILE path, written by another than me) :

OC::$SERVERROOT=str_replace("\\", '/', substr(__DIR__, 0, -4));

which is similar to

OC::$SERVERROOT=str_replace("\\", '/', substr(__DIR__, 0, strlen('/lib')));

Perhaps we need to revert this line. That was not my idea, the previous code does this, this is the only part I don't modified.

@karlitschek : __FILE__ path is not related to URL, in fact it's the only path which does'nt change and this is the only reliable information we have, because it is not apache or nginx which provides it, but php (and there is no alias inside the filesystem ^^), it's the file path of the script (and the last characters '/lib/base.php' is well known)

illwieckz · 2012-11-13T11:22:50Z

lib/base.php

@@ -119,25 +119,26 @@ public static function autoload($className) {

 	public static function initPaths() {
 		// calculate the root directories


imagine your httpd document root is '/var/www'
→ requesting /index.html serves /var/www/index.html
imagine you have an alias named 'alias'
→ requesting /alias/index.html serves /var/www/index.html
imagine you put owncloud on a subdir named 'dir'
→ requesting /alias/dir/index.html serves /var/www/dir/index.html

karlitschek · 2012-11-13T13:34:42Z

Sorry. I don´t want annoy you but I meant documentation of the code of course. Not only in the pull request.

illwieckz · 2012-11-13T14:09:32Z

I do not use my computer right now, I will comment the code later. Have you seen my comments I made on GitHub?
https://github.com/owncloud/core/pull/385/files
I can copy these comments in the php source if you think they are good and understandable.

karlitschek · 2012-11-13T14:21:11Z

Yes. I think this is understandable. Thanks a lot.

karlitschek · 2012-11-13T14:44:38Z

I tested it and it works in all the scenarios that I have here. This is fragile code and it is possible that this doesn´t work in other, more esoteric environments. But this is something that we see when the code is in master. So a +1 from me for now if you add the documentation. Thanks a lot :-)

+1

DeepDiver1975 · 2012-11-13T14:54:40Z

@illwieckz
You'll get my okay as soon as the test suite is executing again - I'll have a look at it tonight as well. THX

illwieckz · 2012-11-13T15:14:08Z

There is a use case I've not tested, it's reverse proxying like in #250, when user request 'GET /alias/index.php' to the reverse proxy, but owncloud receive 'GET /index.php'. I do not know what happens in this case.

@karlitschek :
yes it's fragile, what do you think about putting "WEBROOT" as optional value in config.php ?
for esoteric environments (like esoteric reverse proxy in front of esoteric httpd server ^^) it could be a good way to have OC functionnal if this code is'nt sufficient to handle all use cases ! no ?

@DeepDiver1975 :
It's a good idea to test with OwnCloud installed and not Installed, because before configuration, behavior may differ.

@ALL :
Where can I publish some Nginx configuration sample compatible with this code ? This one : http://owncloud.org/support/webserver-notes/ is not fully-functionnal and contains some errors.

Edit : I wrote this code to have it functionnal with apache without changing habits of apache users, other httpd servers should perhaps emulate the behavior of apache, I can say how to do it with Nginx and fastcgi.

karlitschek · 2012-11-13T15:25:14Z

@illwieckz Yes. I think it is useful to make it possible to define the webroot, host, port and protocol in config.php for the cases that the automatic detection fails. Please also see this pull request #208 from @herbrechtsmeier Perhaps we can merge this somehow.

JFOC · 2012-11-13T15:34:28Z

imho defining the webroot would be great for general web server and do not need to make some custom configuration only for url

illwieckz · 2012-11-13T15:43:07Z

I'll try to write "defining webroot in config.php" in another branch, and if it works I'll do another pull request.

DeepDiver1975 · 2012-11-13T20:21:04Z

The test are failing because $_SERVER["SCRIPT_NAME"] is set to '/usr/bin/phpunit' and as a result OC::$WEBROOT is '/usr/bin/phpunit' as well.

I think in the old code following lines implicitly caught this situation:
illwieckz@03fd34b#L0L125

@illwieckz Can you please take care of this? THX

DeepDiver1975 · 2012-11-17T22:21:33Z

@illwieckz ping ;-) THX

illwieckz · 2012-11-18T18:10:04Z

sorry, it's hard to keep up (because job IRL ^^)

$_SERVER["SCRIPT_NAME"] is set to '/usr/bin/phpunit'

$_SERVER["SCRIPT_NAME"] must contain a path to a .php script, not the name of a binary (interpreter ?).
It seems to be a bug in phpunit.

Because phpunit is a tool used to debug, I think it's a very bad idea to write code to workaround phpunit bugs, it's like bypassing a debugger, who do this ?

Example with a correct interpreter :

illwieckz@arwen ~ $ echo '<?php echo $_SERVER["SCRIPT_NAME"]; ?>' > ~/script.php 
illwieckz@arwen ~ $ php ~/script.php 
/home/illwieckz/script.php

I think in the old code following lines implicitly caught this situation

It can't, this code works only if $_SERVER["SCRIPT_NAME"] end with a '/' character, but '/usr/bin/phpunit' end with a 't' character.

Also, '/usr/bin/phpunit/index.php' can't be a good $_SERVER["SCRIPT_NAME"], it must be something like '/var/www/index.php' (it depends of your configuration).

The code you pointed ensures $_SERVER["SCRIPT_NAME"] contains the 'index.php' script name if we request '/' with a http server configured to serve "index.php" as Index, and if the http server is buggy or misconfigurated and fill $_SERVER["SCRIPT_NAME"] with dirname instead of scriptname.

DeepDiver1975 · 2012-11-19T09:04:07Z

I did a quick search on this topic - for me it looks like setting $_SERVER["SCRIPT_NAME"] to phpunit is done on purpose. We should set $_SERVER["SCRIPT_NAME"] within out bootstrap code to the correct value (tests/bootstrap.php).

@icewind1991 Any objections? THX
@illwieckz Can you please give that a try? THX

DeepDiver1975 · 2012-12-05T10:09:15Z

@illwieckz rebase please and I'll try to get phpunit up and running
@karlitschek we still need/want this - or are the config parameters the answer to all issues concerning webroot detection ..... THX

illwieckz · 2012-12-05T11:17:49Z

We should set $_SERVER["SCRIPT_NAME"] within out bootstrap code to the correct value

Yes I think phpunit must set $_SERVER variable before running the php script, and it must test several values.

we still need/want this - or are the config parameters the answer to all issues concerning webroot detection

The config parameters are a good answer, at least to be able to use owncloud without patching the code when using it on an exotic setup which breaks autodetection.

I'll rebase tonight

illwieckz · 2012-12-15T17:18:10Z

Sorry for not being able to do it sooner! I just rebase.

DeepDiver1975 · 2012-12-19T08:23:31Z

@owncloud-bot retest this please

…r not)

…or this line

… symlinks

illwieckz · 2013-01-28T18:34:06Z

Four things :

I rebased to have this pull request up to date with upstream code
I corrected a bug which appears when DOCUMENT_ROOT have a symlink in its path
I rewrote some little things to have this code following coding style guidelines
I wrote the previous comments inside the code, as requested here by @karlitschek

illwieckz · 2013-01-28T18:39:49Z

in fact, five things :

Another guy has modified one line upstream after I forked, this code was modified to uses his line (which give the same result)

karlitschek · 2013-02-09T16:26:33Z

Hi @illwieckz

Can you check if this can be fixed with the new config options, overwritehost, overwriteprotocol, overwritewebroot, overwritecondaddr ?

Thanks
Frank

illwieckz · 2013-02-11T21:55:11Z

I will look at it, yes!

karlitschek · 2013-03-08T10:32:24Z

@illwieckz Any news here? :-)

ewildgoose · 2013-03-14T16:36:21Z

I think the root problem here is that there are so many potential entry points into the code, ie it's acceptable to reference random .php scripts all around the filesystem?

I concede less experience with PHP, but were I writing this in perl/ruby I would have a single entry point into the code, which in turn talks to a "url router" and that would then have the job to map urls forward/backward to controller actions. So the only tiddle-able that I would need is the base-uri and everything else falls out from that

It seems that perhaps we can get there really easily though:

Map any bare paths to remote.php
map /public/ to public.php
make the base uri configurable

Now we get nice looking urls and most of the guesswork disappears from the code?

bartv2 · 2013-07-05T09:07:54Z

please reopen if this is not fixed with the new config options, overwritehost, overwriteprotocol, overwritewebroot, overwritecondaddr

illwieckz mentioned this pull request Nov 13, 2012

oc-1875 : running OwnCloud on an alias (or not) with Nginx #22

Closed

illwieckz reviewed Nov 13, 2012
View reviewed changes

Thomas DEBESSE (illwieckz) added 7 commits January 28, 2013 18:15

calculate WEBROOT and SUBURI from SERVER env to run OC on an alias (o…

5e637fb

…r not)

resolve conflict (manual merge) after rebasing

7b2c60d

upstream modify one line that does the same : use the upstream code f…

975ec14

…or this line

follow coding guidelines

c67b150

use realpath() function to have this code working when path containes…

b407d60

… symlinks

some comments added as requested here : owncloud#385

f962a8e

follow coding guidelines

1aa553f

bartv2 closed this Jul 5, 2013

lock bot locked as resolved and limited conversation to collaborators Aug 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oc-1875 : running OwnCloud on an alias (or not) with Nginx [the real one] #385

oc-1875 : running OwnCloud on an alias (or not) with Nginx [the real one] #385

illwieckz commented Nov 13, 2012

ghost commented Nov 13, 2012

DeepDiver1975 commented Nov 13, 2012

DeepDiver1975 commented Nov 13, 2012

karlitschek commented Nov 13, 2012

illwieckz commented Nov 13, 2012

illwieckz Nov 13, 2012

karlitschek commented Nov 13, 2012

illwieckz commented Nov 13, 2012

karlitschek commented Nov 13, 2012

karlitschek commented Nov 13, 2012

DeepDiver1975 commented Nov 13, 2012

illwieckz commented Nov 13, 2012

karlitschek commented Nov 13, 2012

JFOC commented Nov 13, 2012

illwieckz commented Nov 13, 2012

DeepDiver1975 commented Nov 13, 2012

DeepDiver1975 commented Nov 17, 2012

illwieckz commented Nov 18, 2012

DeepDiver1975 commented Nov 19, 2012

DeepDiver1975 commented Dec 5, 2012

illwieckz commented Dec 5, 2012

illwieckz commented Dec 15, 2012

DeepDiver1975 commented Dec 19, 2012

illwieckz commented Jan 28, 2013

illwieckz commented Jan 28, 2013

karlitschek commented Feb 9, 2013

illwieckz commented Feb 11, 2013

karlitschek commented Mar 8, 2013

ewildgoose commented Mar 14, 2013

bartv2 commented Jul 5, 2013

		@@ -119,25 +119,26 @@ public static function autoload($className) {

		public static function initPaths() {
		// calculate the root directories

oc-1875 : running OwnCloud on an alias (or not) with Nginx [the real one] #385

oc-1875 : running OwnCloud on an alias (or not) with Nginx [the real one] #385

Conversation

illwieckz commented Nov 13, 2012

ghost commented Nov 13, 2012

DeepDiver1975 commented Nov 13, 2012

DeepDiver1975 commented Nov 13, 2012

karlitschek commented Nov 13, 2012

illwieckz commented Nov 13, 2012

illwieckz Nov 13, 2012

Choose a reason for hiding this comment

karlitschek commented Nov 13, 2012

illwieckz commented Nov 13, 2012

karlitschek commented Nov 13, 2012

karlitschek commented Nov 13, 2012

DeepDiver1975 commented Nov 13, 2012

illwieckz commented Nov 13, 2012

karlitschek commented Nov 13, 2012

JFOC commented Nov 13, 2012

illwieckz commented Nov 13, 2012

DeepDiver1975 commented Nov 13, 2012

DeepDiver1975 commented Nov 17, 2012

illwieckz commented Nov 18, 2012

DeepDiver1975 commented Nov 19, 2012

DeepDiver1975 commented Dec 5, 2012

illwieckz commented Dec 5, 2012

illwieckz commented Dec 15, 2012

DeepDiver1975 commented Dec 19, 2012

illwieckz commented Jan 28, 2013

illwieckz commented Jan 28, 2013

karlitschek commented Feb 9, 2013

illwieckz commented Feb 11, 2013

karlitschek commented Mar 8, 2013

ewildgoose commented Mar 14, 2013

bartv2 commented Jul 5, 2013