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

Cleaner way for redirecting back to the Referrer #2692

Closed
wants to merge 1 commit into from

Conversation

ricordisamoa
Copy link

The 'back' special case has been moved from res.location() to a new req.back getter. In the future, this will make it possible to redirect to a regular 'back' page.

@ricordisamoa ricordisamoa force-pushed the req-back branch 2 times, most recently from ee5ef6c to ddec28b Compare June 28, 2015 00:15
@ricordisamoa
Copy link
Author

As this is a breaking change, though small, I wouldn't mind issuing a deprecation warning and waiting for 5.x instead.

@dougwilson dougwilson added the pr label Jun 28, 2015
@dougwilson dougwilson self-assigned this Jun 28, 2015
@dougwilson
Copy link
Contributor

Seems reasonable to me to remove the magic string value in 5.0

The 'back' special case has been moved from res.location() to a
new req.back getter. In the future, this will make it possible
to redirect to a regular 'back' page.
@ricordisamoa
Copy link
Author

Changed to emit deprecate() instead of breaking the current behavior.

@ricordisamoa
Copy link
Author

Any update?

@gabeio
Copy link
Member

gabeio commented Jul 28, 2015

How is this any different than just res.redirect(req.get("referer")||"/")?
The 'back' should just be removed as it is a feature that only aims to save keystrokes, vs being explicit.


Sidenote: If we are to keep this I suggest adding a test or two for res.location separate from res.redirect.

@dougwilson dougwilson force-pushed the master branch 2 times, most recently from 5f268a4 to 9848645 Compare July 31, 2015 20:58
@ricordisamoa
Copy link
Author

@dougwilson what should I do?

@dougwilson
Copy link
Contributor

I'm all for removing the magic string from Express 5.0, but yes, adding req.back is not a good replacement either, so what to replace it with needs to be thought on more.

@blakeembrey
Copy link
Member

Hi @ricordisamoa, taking a look over this I agree with @gabeio's comment and believe we should remove this altogether. The main reasoning is that people should be more aware of blindly using referrer and we should try and direct them toward writing more explicit code, mostly because req.back might not work or do what they expect. This is more true now that Referrer-Policy exists, which can mean the referrer isn't included or even the location they originally come from.

I realize this is a decade old PR though, so if you're not around I'll take this up and make a new PR in the next week.

@wesleytodd
Copy link
Member

I fully agree @blakeembrey. Maybe in conjunction with removing this we could also add specific docs on how to properly handle this situation?

@wesleytodd
Copy link
Member

I was unable to push to this branch, so I got the tests working and opened a new pr. #5932

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.

6 participants