Issue with redirect after login

[simon-code@…]

Regarding /trunk/trac/web/auth.py@3174#L190 - auth.py / LoginModule / def _redirect_back(self.req)

The test for "same site" is not quite sufficient, as it will get the result wrong when directly linking to /login when one projectname is a substring of another. A simple example is "sandbox" and "sandbox2". It actually gives errors both ways:

1. Link from sandbox to sandbox2/login will get you to the project, but not actually logged in properly. Why this happens, I have not been able to figure out. It just sees me as 'anonymous', even after doing the "login" link manually afterwards. Any ideas? Something else comparing projectnames giving false positives? I have a "home" module that I've made for front page replacement (default component), but it does not receive the authentication information. It is build to "Trac standards", and doesn't do anything complicated that should suggest a problem here.
2. Linking from /sandbox2 to /sandbox/login will actually not take you anywhere, as it does not get caught by the if-test (substring match). The redirect back does not make sense. I made a small change to the if-test, and this issue seems to work correctly:

           if referer and not (referer == req.base_url \
                       or referer.startswith(req.base_url + '/')):
   

This fix for issue 2 will catch both where the referrer is just /sandbox2 (no specific module/page) and where it is like /sandbox2/wiki/WikiStart (by closing the projectname with a '/' testing for /sandbox/).

I have made a cross-project personalised navigation, and have no issues of any kind with projects that are not substrings of each other.

[docwhat]

I think to be correct, you would have to look like this:

if referer and \
   not ( referer == req.base_url or\
         referer.startswith(req.base_url.rstrip('/')+'/') ):

Though, this doesn't fix the problem that if login or logout are in https://, while the main site is in http:// and you don't write stupid RewriteRules? to bounce the user back (why do that, anyway?)...

if referer:
   referer_without_protocol = referer.split('://',1)[-1]
   base_without_protocol = req.base_url.split('://',1)[-1]
   if referer_without_protocol != base_without_protocol and \
      not referer_without_protocol.startswith(base_without_protocol.rstrip('/')+'/'):
       referer = None

I'll happily do a patch for this, if someone wants (I actually already have one).

You can reach me at http://docwhat.gerf.org/ The email is the same as the hostname but the first dot is an at-sign.

Ciao!

[cboos]

Verify how the fix for #2553 would affect this one.

[osimons]

Replying to cboos:

Verify how the fix for #2553 would affect this one.

Problem 1) is now 'worksforme' with current trunk. I cannot make this problem appear anymore, but have no idea when it disappeared or why.

Problem 2) is still very much alive. I'll add it to my todo list.

[osimons]

OK, the original patch still solves this, but using the rstrip() from first comment to make the matching correct. I'd go with the harmless approach here though, and not add more than a correct matching of projectname, leaving the redirect working as before:

trac/web/auth.py

@@ -198 +198 @@
     def _redirect_back(self, req):
         """Redirect the user back to the URL she came from."""
         referer = req.get_header('Referer')
-        if referer and not referer.startswith(req.base_url):
+        if referer and not (referer == req.base_url or \
+                referer.startswith(req.base_url.rstrip('/')+'/')):
             # only redirect to referer if it is from the same site
             referer = None
         req.redirect(referer or req.abs_href())

If no one objects or has a better approach, I'll commit it.

[osimons]

Patch for issue 2) applied in [6279]. Closing.