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

Router striping real dirpath from the urls. #148

Closed
sv3tli0 opened this issue Jul 6, 2016 · 16 comments
Closed

Router striping real dirpath from the urls. #148

sv3tli0 opened this issue Jul 6, 2016 · 16 comments

Comments

@sv3tli0
Copy link
Contributor

sv3tli0 commented Jul 6, 2016

When I clone the project into new localhost subdir http://localhost/citest
When I open the home page at http://localhost/citest/public/ it works fine.
How ever when I add it outputs 'http://localhost/' only.

What I found is that in this condition https://github.com/bcit-ci/CodeIgniter4/blob/develop/system/HTTP/IncomingRequest.php#L489
the query path is substr from the uri.
I think that there is some logical mistake.

I believe that the problem is routing + namespaces. At least if I understand it this clearing the real dir names from the path is to ensure that there are only namespaces/controllers/methods words into the router segments.

https://github.com/bcit-ci/CodeIgniter4/blob/develop/system/HTTP/IncomingRequest.php#L367
Here the URI class sets path but after this dirname path is cleared and I think that this is generating the problem.
$this->detectPath($protocol) is generating string '/' instead of '/test/public/'

Perhaps one good solution is to have another param $basepath into the URI class which will contain this stripped part and it will be involved into building of urls.

@sv3tli0 sv3tli0 changed the title Router striping real path from the urls. Router striping real dirpath from the urls. Jul 6, 2016
@arma7x
Copy link
Contributor

arma7x commented Jul 6, 2016

Have you set public $baseURL in /application/Config/App.php ?

@lonnieezell
Copy link
Member

I'll take a look at the code in question, however there are two things here:

  1. That code is pretty much a direct rip from CI3, so I think it's likely the logic has been all worked out there already.
  2. you're visiting your page from outside the public directory, which is not the intended way, and is a security risk. The URI should be pointing to the public directory. If it's not, things can't be guaranteed to work all of the time. And that would need .htaccess RewriteBase changed, etc.
  3. As @arma7x pointed out, you should set $baseURL. Currently, it's attempting to do you favor and guess what the base url is when you don't supply that. It cannot always guess correctly.

My guess is that if you'll either set $baseURL, or serve the site from the public folder, things will work just fine for you.

@sv3tli0
Copy link
Contributor Author

sv3tli0 commented Jul 6, 2016

@lonnieezell The problem is that the $_SERVER is returning that path /test/public/ and after that the router has removing it at the moment when it pass it to the URI.

By the way with set $baseURL the problem still exists.. Not quite sure why..

@arma7x
Copy link
Contributor

arma7x commented Jul 6, 2016

I'm pretty sure $_SERVER['SCRIPT_NAME'] is remove from $_SERVER['REQUEST_URI'] and will return URI after it only.
$_SERVER['SCRIPT_NAME'] = "/CodeIgniter4/public/index.php"
$_SERVER['REQUEST_URI'] = "/CodeIgniter4/public/index.php/home"
RESULT = /home

@sv3tli0
Copy link
Contributor Author

sv3tli0 commented Jul 8, 2016

In fact even when I set baseURL to
'http://localhost/test/public/' or 'http://localhost/test/'

Again I got my subfolders stripped from the url when trying to build it with
current_url(), base_url(), site_url()

From what I detecting this base url is only changing the host, but not and the path entered?

And in https://github.com/bcit-ci/CodeIgniter4/blob/develop/system/HTTP/IncomingRequest.php#L489
at least what I am understanding its always stripping the dirname of the called script file from the path no matter what.

@lonnieezell
Copy link
Member

I haven't had a chance to look into this yet. Dealing with other bugs. Will be out tonight so might not be until Sunday night that I have a chance to look into it, but 2 questions:

  1. Have you tried serving the site from the public folder, where it's intended to run from, to see if things work correctly?
  2. Have you stepped through the code to see what it's doing exactly to verify that's a problem? Like I said, that's exactly what's in CI2 and CI3, so I don't think that particular piece is part of the problem.

@sv3tli0
Copy link
Contributor Author

sv3tli0 commented Jul 8, 2016

In CI 3 base url is auto detected correctly
https://github.com/bcit-ci/CodeIgniter/blob/develop/system/core/Config.php#L88

But here I didn't find where is the autodetection and at the same time I didn't find when this base url is used at all :/.

I will try to find time at the weekend to search for the possible reasons.

(1) My idea is that there should be no difference if the site is rooted to the public folder or its some sub-sub-sub-folder

@arma7x
Copy link
Contributor

arma7x commented Jul 10, 2016

@lonnieezell
Copy link
Member

Having looked through the code, it is ensuring that the folder that the Front Controller is in is stripped from the URI if it's present. This is typically a good thing.

To serve the site from /public with that in the path is a security risk, that our move to having just the index.php file in the public folder is trying to mitigate. It keeps the other files in your application from being discovered, because they are not at all accessible from the web. .htaccess files, like CI3 and older relied on, are not foolproof. As other servers, like NGINX, gain popularity, their usefulness is even less. Server configuration can be accidentally screwed up allowing anyone to view your files and discover things about your app that no one should know. Heck, it's happened to Facebook in the past.

In light of all of this, my recommendation is that it's left as is. The site should be served from the public folder, period. We should not help introduce security issues in our app in the name of convenience. If you need it to be a flatter structure, you can easily move the index.php file with the rest and modify system and application paths to give you the same structure as previous versions of CI.

@sv3tli0
Copy link
Contributor Author

sv3tli0 commented Jul 11, 2016

Fast and simple case : Shared hosting with access only to www folder. In many cases with no way to configure your domain to go directly to the Public folder. Even to day this is common case for some clients.

Any auto filtering which is not configurable is a Limit. In this case this is a limit for any developer who don't fits the standard thinking. There are many reasons why some script may not be into the document root on 1 server and auto hidding part of the URL is not helpful for.
I don't have any idea if there is such thing at any other framewokr routing..

@lonnieezell
Copy link
Member

Most shared hosting that I've used you've been able to define what folder the domain is served from. Some older versions of cPanel used to restrict that for the primary domain, but that could be solved with an .htaccess trick, IIRC.

Worst case scenario - change the paths in the index and put it into a way that works for that server. Literally takes changing like 2 variables and moving the index file.

When we were discussing whether to make this move or not, I went looking at the top 8 or so most used PHP frameworks that I knew and every single one of them were served out of a public folder (though the name might have been different). So, it's obviously possible to server web pages with it still in this format.

And it's not that much of a limit because even if you're in the example you provided, the website only works from the main index.php file. If they try the domain one folder up - either a directory listing happens (which is horrible for security) or an error (which is horrible for usability).

@sv3tli0
Copy link
Contributor Author

sv3tli0 commented Jul 11, 2016

My point.. It is Developers work to put their project in the correct place (directory and etc and to setup doc root). The framework should not filter strings from the real url path just for some kind of idea, security or not.

I don't like the idea that the framework will decide what must be hidden and what not. This should be decided by the developer who is coding / or the one who is deploying the project.

@lonnieezell
Copy link
Member

lonnieezell commented Jul 11, 2016

It is decided by the developer - modify the paths in the index.php file. I get what you're saying, I do, but I as a framework developer, it's my job to make the framework as secure as possible.

@jim-parry @mwhitneysdsu What's your take on this subject? How would you prefer this to work?

@mwhitneysdsu
Copy link
Contributor

I haven't had a chance to test this, yet, but I would say that it should be possible to configure the $baseUrl with a path, as there are reasons other than the location of index.php to do so. For instance, I might serve part of my site from CI, while another part uses a CMS, and still another uses some forum software. Similarly, each directory in my domain might be handled by a different server (in fact, many of the directories in my current site used to be separate sites). The $baseURL is not necessarily the same thing as the host, and the directories listed in the URL do not necessarily reflect the underlying directories of the system.

So, if I set my $baseURL to be 'https://example.com/management' and the underlying site configuration serves up /var/www/management/public/index.php when you point the browser to 'https://example.com/management', then there's no security issue with outputting 'https://example.com/management' as the site's base URL, because the site's administrator controls what is output when the browser goes to https://example.com/, and it may be a completely different site (it may even be handled by a separate instance of CI4, on a different server).

If the user needs to serve the site from the project root, they're going to have to move index.php and modify the paths, then do their best to ensure the security of the site by preventing access to the directories. Alternatively, they could include /public in their URL, but it would be up to them to secure their site by preventing improper access to the non-public locations. We can't guess whether /public is part of their base URL because of a poor configuration or because it just happens to be the base URL for their site. Just as in the previous example, https://example.com/ could be served by a completely different server from https://example.com/public, and there would be no security issue in outputting https://example.com/public as the base URL.

@lonnieezell
Copy link
Member

lonnieezell commented Jul 11, 2016

I do need to test that base_url is acting properly because it should be overriding what's in the discovered URL. Totally agree with you there.

And it's not that it's arbitrarily removing the text '/public', but it's cleaning up the path to the front controller so the folders above it within the site are not shown. This is the same code that's been in CI2 and CI3, it's just now becoming visible since the default structure isn't flat. Not arguing the case, here just trying to explain the situation.

EDIT: This wouldn't prevent the site from existing in subfolders, but a RewriteBase (or equivalent) and possible site restructure would be needed at that point, anyway. And as long as the base_url is functioning correctly it should be good.

@jim-parry
Copy link
Contributor

Index.php ... whichever folder you put that in (project root or public) will be "wrong" for some developers, and we need to explain how to configure it for the other usecase. In other words, we cannot "win", i.e. keep everyone happy. My preference is to keep index.php in the public folder, as a "best practice". An experienced developer will have no problem moving it up one level if they wish, while an inexperienced developer will be somewhat sheltered from themselves :-/

Baseurl ... should work regardless of where a developer has put their index.php. We can strongly suggest that the web server point to the folder containing index.php, but a developer should be free to play with that, at their own risk of course.

I think that this thread has gotten off topic - it was originally intended to report a Router bug, which may or not have been addressed (TL;DR!!), yet most of the discussion is about best practices and the correctness of the url_helper. The "current_url" mention in the URI library page of the user guide is contradictory, which needs to be corrected. Actually, the URL helper writeup needs to be incorporated into the user guide, and that might go a long way to resolving the apparent confusion. I see you have opened an issue for the url_helper behavior, which is a correct approach :) I will start on the helper writeup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants