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

Deal with redirect loop protection and page caching #16

Open
Berdir opened this issue Apr 10, 2015 · 4 comments
Open

Deal with redirect loop protection and page caching #16

Berdir opened this issue Apr 10, 2015 · 4 comments

Comments

@Berdir
Copy link
Member

Berdir commented Apr 10, 2015

See #15

Page caching obviously breaks redirect loop protection. For now, we just disabled it for the test. I can see a few options:

a) Drop the feature, let browsers do it.
b) Replace the implementation, use a active check before redirecting, to at least still protect against redirect loops between redirect.module managed redirects.
c) Just keep it like it is, it will only work when page cache is disabled. Which means that for example authenticated users will notice it and can fix it.
d) Add an option to disallow caching of redirects.

@Berdir
Copy link
Member Author

Berdir commented Apr 10, 2015

Combination of multiple options might also be opssible ;)

@Berdir
Copy link
Member Author

Berdir commented Apr 10, 2015

Note that c) is essentially what we have now.

@wimleers
Copy link

a) is most appealing, but probably least practical, depending on how often redirect loops still are an actual problem.

Assuming we want redirects to be cached, b) then seems the next best option.

Of course, one could argue that redirects are cheap (they happen before routing), so why not just prevent them from being cached? (i.e. option d.) Well, redirects being cached is generally a good practice. Finally, this also means redirects cannot be cached in a CDN, so for sites with a global audience, every user will have to go through the CDN, all the way to the origin, and back again.

Therefore I think the answer depends on the answer to the question: "How real of a problem are redirect loops nowadays?"

@jhedstrom
Copy link
Contributor

If I'm reading this code correctly, the current loop detection is defined as any 5 redirects being triggered in a 15 second period.

Shouldn't that flood control use the $Request object to add the path to the flood control?

jhedstrom added a commit to jhedstrom/redirect that referenced this issue May 4, 2015
- Redirects are searched for recursively to avoid loops.
- Addresses issue md-systems#16.
jhedstrom added a commit to jhedstrom/redirect that referenced this issue May 5, 2015
- Redirects are searched for recursively to avoid loops.
- Addresses issue md-systems#16.
jhedstrom added a commit to jhedstrom/redirect that referenced this issue Jul 15, 2015
- Redirects are searched for recursively to avoid loops.
- Addresses issue md-systems#16.
jhedstrom added a commit to jhedstrom/redirect that referenced this issue Jul 31, 2015
- Redirects are searched for recursively to avoid loops.
- Addresses issue md-systems#16.
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

3 participants