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

Geozak redirection handling #749

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

geozak
Copy link
Contributor

@geozak geozak commented Nov 13, 2015

Created a template view to use for friendly redirection.
Made Redirect class methods render the redirect view then exit to prevent other views from rendering.
Replaced uses of header('location: ...') with Redirect::to().

A view template for a page to be displayed when a user is redirected between pages.
Added lines to make calls to Redirect display the redirect template view and exit.

this should be satisfactory to fix this comment from Auth.php
// to prevent fetching views via cURL (which "ignores" the header-redirect above) we leave the application
// the hard way, via exit(). @see panique#453
// this is not optimal and will be fixed in future releases

this should make redirecting users friendly in that they won't see empty/broken pages.
Replaced header location calls with Redirect::to to use the friendly redirection.
Replaced header location calls with Redirect::to to use the friendly redirection.
Removed unreachable statement.
@geozak
Copy link
Contributor Author

geozak commented Nov 29, 2015

The comment about preventing empty/broken pages is response to issue #453.

The project already contains a Redirect class. These commits make shift the exit to into the redirect methods that way when a developer wants to redirect the user they only have to worry about adding one line and security is still intact.

The redirect template view for that if it takes the users browsers while to actually redirect or in the case of the curl example from #453 then a meaningful page will be displayed/returned.

A slightly better implementation to the template would be to make the address displayed a hyperlink.

@panique
Copy link
Owner

panique commented Dec 12, 2015

Thanks for the commits, but I think this has a problem:

It takes out extremely important logic (the exit()) out of the Auth class! This is super-important and might break the application, as the application is not stopped from running when authentication fails and the user is not using redirects! As lots of people might use the application without being aware that they absolutly have to use redirects to keep the app save... hmm, sorry but in this case I would prefer to keep it like it is instead of merging this pull request.

Is this okay for you ?

@geozak
Copy link
Contributor Author

geozak commented Dec 12, 2015

The exit isn't removed. It's actually more reinforced because its inside the redirect methods.

The having a view for the momentary delay in redirecting might not be necessary for most uses.

But I think it would be good to include the exit call in the redirect methods even if we leave the exit calls in auth and such.

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

Successfully merging this pull request may close these issues.

None yet

2 participants