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

[2.0.4] Url Generation Now Breaks Relative Redirects #10610

Closed
virgofx opened this issue Jul 7, 2015 · 30 comments

Comments

@virgofx
Copy link
Member

commented Jul 7, 2015

Redirection using relative paths no longer works in 2.0.4. The Url detection now seems to think relative paths are external and it is adding http which breaks redirection.

$this->response->redirect('/my-local-path');

Will return a 301 with http://my-local-path which is incorrect. Should be /my-local-path.

@andresgutierrez

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2015

I'm not able to reproduce this:

<?php

use Phalcon\Di;
use Phalcon\Mvc\Url;
use Phalcon\Http\Response;

$di = new Di;

$di['url'] = function() {
    $url = new Url;
    $url->setBaseUri('http://localhost/invo');
    return $url;
};

$http = new Response;
$http->redirect('/data');
$http->send();

Produces the correct redirection to http://localhost/invo/data

@alitalaghat

This comment has been minimized.

Copy link

commented Jul 9, 2015

Please try with this:

$url->setBaseUri('/');

Now you can reproduce the problem.

@andresgutierrez

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2015

@alitalaghat But the generated URLs are //data which in fact are relative URLs

@virgofx

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2015

@andresgutierrez nope ... "//" is interpreted as a schemaless root url.

@virgofx

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2015

Thats the problem exactly, the default base uri is "/" (which is the case when it's not set) and therefore redirects will now use "//data" which is incorrect --> as this will redirect to "http://data" or https://data" depending on the scheme.

@virgofx

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2015

I believe you may be able to reproduce problem if you add an extra '/' on your example (although not tested). The original documentation had the base uri always ending with a trailing slash.

@virgofx

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2015

I can confirm this problem exists in all applications without explicitly specifying a baseUri for the URL. The default has been always "/". And now any redirect's with /location will turn into //location which is incorrect.

This is extremely high priority.

@andresgutierrez

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2015

If the default URI is / why do you have to prepend another /? It will result, for instance in //data.

@virgofx

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2015

@andresgutierrez Intuitively that would make sense ... no problem with that. The issue is that the behavior has changed from defaults prior to 2.0.4 in a very core part of the framework (URL generation is huge for applications -- breaking production for my entire stacks)

The current default has always been "/" for the base URI. I guess more clarification could exist on this. For example, lets talk about applications that are serving single instances only (e.g. www.mydomain.com). They are running phalcon but want all URLs to be relative. Relative because the url is accessed via https://www.mydomain.com, www.mydomain.com, ip-10.10.10.10:80.

So is 2.0.4 intended to change default behavior? The URL commit has broken functionality, applications in production, additionally the Invo application.

Is the new URL change intended to no longer use a trailing slash? The current pre-2.0.4 default was /. Invo was /invo/ ... other applications always included a trailing slash, yet responses and urls were always started with a slash ... e.g. /redirect-locally.

So either way .. there needs to exist through documentation on for base URI / base URL / static URLs and explicitly with slashes.

Alternatively, a quick fix would be to assume that base URIs always end in a trailing slash and handle the case where you have a leading slash plus trailing slash.

Or just revert the last URL commit as this breaks functionality.

On a side note -- when using relative URLs -- it makes more sense to write: $this->response->redirect('/local-path'). I think this should work regardless if the base URI ends with or without a trailing slash as the browser should be sending a 30[1/2] explicitly as /local-path. So I guess there needs to be more documentation for how the base URI impacts other aspects of the system, whether the baseURI should or should not have a trailing slash, other services that use this. There is currently very little documentation on this.

@olivier-monaco

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

Change was 1f74b26#diff-d20931d7bcb77f0986c66ce50e774c7eL230

@andresgutierrez Do you remember why you change the behavior? What was the problem?

@andresgutierrez

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

@olivier-monaco Not sure why it was initially there: c73502e

@olivier-monaco

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

It was to handle this kind of issue (see #3222).

@olivier-monaco

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

$u = new \Phalcon\Mvc\Url();
$s = $u->get('/path').PHP_EOL;

@andresgutierrez What must be the value of $s?

@andresgutierrez

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

@olivier-monaco it must be baseUri + uri

@olivier-monaco

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

So, in this case, "//path". It's not what I may expect and I think many developers too.

@andresgutierrez

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

@olivier-monaco If you didn't expect //path why did you pass '/path' instead of just 'path'?
You can leave an empty baseUri then

@olivier-monaco

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

Because I'm expected the baseURI to be the "root" of the web site, so to be empty. Maybe the default must be an empty string.

When I see this code:

$this->response->redirect($url);

I'm expecting:

  1. $url to contain a full URL to redirect to (http://....). It's an external absolute URL
  2. $url to begin with "/" ("/path"). It's an internal absolute URL.
  3. $url to not begin with "/" ("path"). It's a relative URL to the current URL.

For the thrid case, if the current URL is "/some/place", asking for "path" must be "/some/place/path".

Then, the baseURI is like a "chroot", a way to force some path at the beginning of any path.

Here is what I have in mind.

@andresgutierrez

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

But not every URL that begin with / is an absolute URL, you also have protocol-relative URLs: http://tools.ietf.org/html/rfc3986#section-4.2

@olivier-monaco

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2015

Yes, we can add something between 1 and 2 called "external relative URL".

@sotisoti

This comment has been minimized.

Copy link

commented Jul 17, 2015

I also can confirm that this issue is reproducible:

$this->response->redirect("/my/profile"); will be interpreted as http://my/profile

I have the / prefix in all redirects and it's been working all the while until the (breaking) change was introduced. Is there any plan to fix this?

@Surt

This comment has been minimized.

Copy link
Contributor

commented Jul 17, 2015

        $this->response->redirect("/")

it's not working when not setting any setBaseUri

And, I know I must not rely on http_referer but, it's not working at all when not setting any setBaseUri

        if(!$back = $this->request->getServer('HTTP_REFERER')) $back = null;
        $this->response->redirect($back);
        die($this->request->getServer('HTTP_REFERER'));
andresgutierrez added a commit that referenced this issue Jul 20, 2015
@andresgutierrez

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2015

Fixed in the 2.0.x branch

@virgofx

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2015

@andresgutierrez Thank you very much!

@dschissler

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2015

@andresgutierrez Perhaps you could keep a list of things to break in 3.0.

@sszdh

This comment has been minimized.

Copy link

commented Jul 25, 2015

But now it conflict with logic that based on request->getURI()!
The request->getURI() returns _SERVER["REQUEST_URI"] that begins with extra /:

Consider this example when you want redirect user to requested URI after login:

$this->response->redirect($this->request->getURI()); <-- result: redirect('/user/profile')

@andresgutierrez This issue completely breaks my app!

Should I trim each getURI() in my code? :|

Thanks.

@olivier-monaco

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2015

@sszdh Can you give us an minimal usecase?

@sszdh

This comment has been minimized.

Copy link

commented Jul 25, 2015

@olivier-monaco I did before!
Actually using getURI returns URI with slash and you can not use this directly with redirect().
This is not about any use-case, the getURI returns _SERVER["REQUEST_URI"] depends on this

It means I can not use getURI directly in routing and redirecting...

Thanks

@olivier-monaco

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2015

@sszdh A use-case is a way for us to understand what you want exactly. It's also a way to have/build a unit test and to ensure the code to do what your want. Reading your comment is not enough: I don't see your problem...

But maybe someone else fully understand your problem and will help you.

@olivier-monaco

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2015

@sszdh Are you using the latest version?

I added a controller for "/toto" with the following code:

$this->response->redirect($this->request->getURI());

And when I open the URL, I get:

  HTTP/1.1 302 Found
  ...
  Location: /toto
  Vary: User-Agent,Accept-Encoding

What is wrong with this result?

@sszdh

This comment has been minimized.

Copy link

commented Jul 25, 2015

The wrong with this result is the getURI() value does not suitable for redirction anymore but in last version it did.

I'm in travel now, i will provide a test script ASAP @olivier

Thanks for your attention

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.