Avoid unauthorized redirect loops #2900
Conversation
|
|
||
|
|
||
| came_from_url = request.get('came_from') | ||
| if 'require_login_awoid_loop=' in came_from_url.split('?')[-1]: |
There was a problem hiding this comment.
awoid -> avoid (also in commit message, PR title and changelog 😜 )
|
|
||
| came_from_path = came_from_url[len(portal_url):].split('?')[0].split('#')[0] | ||
| try: | ||
| # Probe for an actualy Unauthorized exception while traversing. |
| else: | ||
| # Attach a marker so that we can detect redirect loops. | ||
| if '?' in came_from_url: | ||
| came_from_url += '&require_login_awoid_loop=true' |
There was a problem hiding this comment.
Hmm. I'm not too excited about this staying in user-visible URLs (apart from some URLs becoming a bit ugly). What if a user ends up on a document this way, and decides to copy & paste this link from the address bar into a word document? 😝 Not a big deal though.
There was a problem hiding this comment.
Yes, that is not nice, but I didn't find a better solution. Cookies wouldn't be a good idea because they are session-wide. I also thought about using an abbreviation, but it doesn't really make it better.
Do you have another idea how to detect redirect loops?
|
I would suggest to deploy this to (The CAS server URL ( |
jone
changed the title
The require_login script was customized in order to support MS Office probing, which happens without the session (cookies). The old implementation had issues: - When the request had a payload, such as a POST request, the payload did not survive the redirect. When a ++add++ could be rendered but not posted, this caused the form to reload empty without an error. - When the unauthorized exception did not happen during traversal but during publishing a redirect loop was created. In order to fix this issues I rewrote the require_login script to be more robust. - No longer redirect when the request method is not GET. - When redirecting, add a marker GET parameter for detecting loopbacks and breaking redirect loops. - No longer catch all exceptions; catch only the Unauthorized exception. - Remove GET-Params when converting to a path when probing. - Crash on purpose when came_from is not within our site root.
jone
force-pushed
the
jone-unauthorized-redirects
branch
from
efc380c
to
89eb287
Compare
|
@lukasgraf thanks for the review; I amended according to your comments. |
|
Be aware: the discussion about the GET parameter is now "outdated" because of a name change, but it is not really fixed. |
The require_login script was customized in order to support MS Office
probing, which happens without the session (cookies).
The old implementation had issues:
did not survive the redirect. When a ++add++ could be rendered but not
posted, this caused the form to reload empty without an error.
during publishing a redirect loop was created.
In order to fix this issues I rewrote the require_login script to be
more robust.
and breaking redirect loops.
Besides fixing a bunch of actual redirect loops in the application this change allows us to move on to a newer ftw.testbrowser later, without downgrading the HTTP error handling.