Modify

Opened 16 years ago

Closed 11 years ago

Last modified 11 years ago

#2210 closed defect (fixed)

After redirecting to the login screen, trac always loads the default page.

Reported by: jspencer@… Owned by: ejucovy
Priority: normal Component: PermRedirectPlugin
Severity: normal Keywords: patch
Cc: Sascha Silbe, ejucovy, Ryan J Ollos, Steffen Hoffmann Trac Release: 0.10

Description

If you follow a link (from an email for example) the login page comes up as expected, but after logging in the specific link is forgotten and you are taken to the default page (main wiki page).

I'm not sure if this is specifically a PermRedirectPlugin problem or a Trac problem in general, but it does affect RSS feeds quite a bit.

Attachments (3)

permredirect.diff (2.5 KB) - added by Garth Roxburgh-Kidd 16 years ago.
Proposed fix
PermRedirect-urlfix.diff (968 bytes) - added by Nathan Bird 16 years ago.
Patch to the 0.11 branch to use the referer querystring parameter
PermRedirect-urlfix.2.diff (1.1 KB) - added by Nathan Bird 15 years ago.
Patch to the 0.11 branch to use the referer querystring parameter (now slightly improved to also capture the querystring of the original request instead of just the request path).

Download all attachments as: .zip

Change History (43)

comment:1 Changed 16 years ago by Noah Kantrowitz

This is due to how the HTTP referer header works with redirects. Unfortunately there isn't much I can do.

Changed 16 years ago by Garth Roxburgh-Kidd

Attachment: permredirect.diff added

Proposed fix

comment:2 Changed 16 years ago by Garth Roxburgh-Kidd

I've attached a patch for a fix. When you request a page and fail authentication, it sets a cookie to the URL you asked for. A wrapper around LoginModule._redirect_back adjusts the Referer header if it finds that cookie. _redirect_back then redirects back to the original page if authentication succeeds.

comment:3 Changed 16 years ago by anonymous

I couldn't get your patch file to apply to v0.11 of PermRedirectPlugin - I think the line numbering might be out?

comment:4 Changed 16 years ago by anonymous

Hmm... no, I think your patch must be for an earlier version.

comment:5 in reply to:  4 Changed 16 years ago by anonymous

Replying to anonymous:

Hmm... no, I think your patch must be for an earlier version.

It's a patch for 0.10 - you'll have to write your own patch for 0.11

comment:6 Changed 16 years ago by Remy Blank

This could now be fixed by using the referer= argument to /login, added to Trac 0.11-stable in [T7493].

comment:7 Changed 16 years ago by bzhu@…

Hi, Sir,

If you can provide a patch for 0.11 independently? We don't want to update the Trac, only want the the redirect problem fix. I searched but can't find the patch down. THanks in advance.

Bill

Changed 16 years ago by Nathan Bird

Attachment: PermRedirect-urlfix.diff added

Patch to the 0.11 branch to use the referer querystring parameter

comment:8 Changed 16 years ago by Nathan Bird

The patch I added above implements what rblank referred to in comment:6

comment:9 Changed 16 years ago by Bill

After implement the PermRedirect-urlfix.diff , I got this. I am not developer, could you check what it is please?

Oops...

Trac detected an internal error: invalid syntax (filter.py, line 27)

Traceback (most recent call last):
  File "/usr/lib/python2.4/site-packages/trac/admin/templates/deploy_trac.cgi", line 27, in ?
    cgi_frontend.run()
  File "/usr/lib/python2.4/site-packages/trac/web/cgi_frontend.py", line 71, in run
    gateway.run(dispatch_request)
  File "/usr/lib/python2.4/site-packages/trac/web/wsgi.py", line 87, in run
    response = application(self.environ, self._start_response)
  File "/usr/lib/python2.4/site-packages/trac/web/main.py", line 381, in dispatch_request
    env = open_environment(env_path, use_cache=not run_once)
  File "/usr/lib/python2.4/site-packages/trac/env.py", line 571, in open_environment
    env = Environment(env_path)
  File "/usr/lib/python2.4/site-packages/trac/env.py", line 185, in __init__
    load_components(self, plugins_dir and (plugins_dir,))
  File "/usr/lib/python2.4/site-packages/trac/loader.py", line 108, in load_components
    loadfunc(env, search_path, auto_enable=plugins_dir)
  File "/usr/lib/python2.4/site-packages/trac/loader.py", line 65, in _load_eggs
    entry.load(require=True)
  File "/usr/lib/python2.4/site-packages/pkg_resources.py", line 1912, in load
    entry = __import__(self.module_name, globals(),globals(), ['__name__'])
  File "/usr/lib/python2.4/site-packages/permredirect/filter.py", line 27
     login_url = req.href.login(referer=req.base_url + req.path_info)
     ^
 SyntaxError: invalid syntax

comment:10 Changed 16 years ago by anonymous

I am using AccountManager, so can't load trac.web.auth LoginModule in your first patch. If this will have problem for your new patch?

Bill

comment:11 Changed 16 years ago by anonymous

Replying to Bill :

After implement the PermRedirect-urlfix.diff , I got this. I am not developer, could you check what it is please?

   File "/usr/lib/python2.4/site-packages/permredirect/filter.py", line 27
      login_url = req.href.login(referer=req.base_url + req.path_info)
      ^
  SyntaxError: invalid syntax

Not too sure here; the best I can suggest is to check the syntax in the resulting file, maybe the patch applied incorrectly? I inserted that line 27 ('login_url...') in the patch, make sure it is indented the same as the line above('exctype...') and below it('if...').

Replying to anonymous:

I am using AccountManager, so can't load trac.web.auth LoginModule in your first patch. If this will have problem for your new patch?

Bill

The two patches proposed here are entirely independent. If you have already applied attachment:permredirect.diff you should get a clean version(I have this one) of PermRedirectPlugin before applying attachment:PermRedirect-urlfix.diff. I don't reference LoginModule anywhere in this code; other dependencies/incompatibilities I don't know anything about.

comment:12 Changed 16 years ago by Bill

I have rechecked the /usr/lib/python2.4/site-packages/permredirect/filter.py and sure it is same with your PermRedirect-urlfix.diff.

Before when I search for this problem, I did see one patch for AccountManager plugin. But that patch was old and didn't work ( I tested by myself).

PermRedirect plugin should fix this problem, that is why I installed it, but still have some problem. If anyone in your group and help to figure this out? Thanks anyway.

Bill

comment:13 Changed 15 years ago by Martijn Pieters

I can confirm that the PermRedirect-urlfix.diff patch works like a charm for 0.11. What is needed to have this applied to the repository? Can I assist in any way?

Changed 15 years ago by Nathan Bird

Attachment: PermRedirect-urlfix.2.diff added

Patch to the 0.11 branch to use the referer querystring parameter (now slightly improved to also capture the querystring of the original request instead of just the request path).

comment:14 Changed 15 years ago by jouvin@…

Is there any change for this patch to be included in Trac trunk ? If not, why ?

I am using Trac trunk (0.12dev) and have a similar problem : the redirect is not necessarily to the start page but if you were in an "edit" page (wiki edit, ticket modification) and when submitting your changes a perm redirect occurs, the modification is lost and you have to reapply it.

If you think this is not the same problem, I'll open a new ticket.

Cheers,

Michel

comment:15 Changed 15 years ago by Noah Kantrowitz

The situation you describe doesn't make any sense though, since there is no permission check that could fail at that time that wouldn't fail when opening the edit screen (unless you manually delete the auth cookie I suppose).

comment:16 in reply to:  14 Changed 15 years ago by Nathan Bird

Replying to jouvin@lal.in2p3.fr:

Is there any change for this patch to be included in Trac trunk ? If not, why ?

I made the patch against browser:permredirectplugin/0.11#4929 running against trac version 0.11stable-r7510 (from about). I was just able to update my permredirectplugin checkout to browser:permredirectplugin/0.11#5162 (doesn't appear to be any changes). We've been using this for the past couple of months and are very happy with it. coderanger: is there anything blocking acceptance of this patch?

I am using Trac trunk (0.12dev) and have a similar problem : the redirect is not necessarily to the start page but if you were in an "edit" page (wiki edit, ticket modification) and when submitting your changes a perm redirect occurs, the modification is lost and you have to reapply it.

If you think this is not the same problem, I'll open a new ticket.

The fact that your permission has expired is definitely a different problem. I'm somewhat in agreement with coderanger it doesn't make much sense; the only thing I could think of would be if your permission token expired due to time. Hitting refresh before editing might help? I've let tickets sit for a while without doing anything and then edited without a problem but this might be very dependent on what authentication scheme you are using.

Because ticket modification data is POSTed whereas this is just doing redirects (GET) that don't carry the post data along with it, this plugin (with or without the proposed patch here) will *not* solve the lost data problem.

Event Order

  1. Trac has a permission error
    • PermRedirectPlugin without the above patch, no referrer info is passed (in our setup at least). This is confirmed through firebug and apache logs.
    • With the patch, when we do the redirect we add a QueryString parameter named "referer" with the value of the page that was requested.
    • Trac w/o the plugin renders an error page, that when you click the link to go to login, the Http Header "referer" is added because you followed a standard link.
  2. Login url does what it is supposed to (http basic auth over ssl in my setup).
  3. When you get to the login page under recent Tracs(slightly after .11 was released there was a fixup patch so that it would read referer from all params not just the headers) it redirects you to whatever location is given in the "referer" request parameter.

In any of the above scenarios under point 1, POST data is lost. From what I know of HTTP redirects there isn't a good way to thread the POST data. A couple not good options: keeping it in the server's session memory, sticking it in a cookie, trying to create querystring parameters out of everything posted.

comment:17 Changed 15 years ago by Sascha Silbe

Cc: Sascha Silbe added; anonymous removed

comment:18 Changed 15 years ago by doug.patterson

I applied UnwashedMeme's PermRedirect-urlfix.2.diff patch, works great against Trac 0.11.2.

comment:19 Changed 15 years ago by Russ Tyndall

Keywords: patch added

Is there any chance of getting this patch applied?

We have been using it successfully for months, it sounds like others have had success as well, and it just worked again on an entirely fresh install.

Thanks,

Russ

comment:20 Changed 14 years ago by labs@…

How is this plugin and this patch supposed to work? (It seem it doesn't work with me)

With this plugin the url of the login page has become something like this:
https://my.trac.example.com/login?referer=http%253A%252F%252Fmy.trac.example.com%252Froadmap

But the inner workings of the /login page seems not to react on any of this ?referer= parameter.

Has anybody been getting this to work?
Thanks,
Dirk

comment:21 Changed 14 years ago by Nathan Bird

Dirk,

It's been working fine for us for quite a while-- really wish we could get this patch applied to the plugin.

You need to apply the patch PermRedirect-urlfix.2.diff to the plugin before installing it with python easy_install-- looks like you might already have that point. The login url you gave looks correct; that's mostly what this plugin+patch provides.

Beyond that we rely on handling introduced in trac in [T7493] (comment:6)-- that was sometime during early trac 0.11. The login page at this point is supposed to, on successful login, redirect you back to the page specified by the referer.

Are you using a recent-ish trac (0.11+) with this plugin and patch applied? What happens after you login do you land on default page, are you still on the login page? Do you have any other plugins that are trying to intercept/munge the login procedure?

comment:22 Changed 14 years ago by labs@…

I'm running Trac 11.4. And I have the Account Manager plugin enabled. Could this be interfering with the login in an unwanted way?

Thanks, Dirk

comment:23 in reply to:  22 Changed 14 years ago by Nathan Bird

Replying to labs@ixopusada.com:

I'm running Trac 11.4. And I have the Account Manager plugin enabled. Could this be interfering with the login in an unwanted way?

Yeah, I've heard reports of that plugin interfering especially if you have it configured for forms auth. I don't see a reason why the forms auth shouldn't be able to work with this, just reports that it doesn't. That will be a bug report over there, that it is ignoring the referer variable added in [T7493].

comment:24 Changed 14 years ago by labs@…

Getting back at this problem. I posted the presumed bug at the AccountManager ticket about this #3783, but nobody there responded. Now getting fed up with this and so I'm looking to solve it myself. :-)
After some digging, it seems that the ?referer= parameter is being received by the plugin. It's just tripping over the url quotes. Also the code in [T7493] doesn't seem to unquote the referer parameter.

Is that correct? Where is the unquoting suppose to happen? The referer is now http%3A%2F%2Fmy.trac.example.com%2Froadmap (as seen inside the _redirect_back method)

Thanks,
Dirk

comment:25 Changed 14 years ago by Nathan Bird

Confirming the upgrade to trac 0.12 went fine. Plugin with patch attachment:PermRedirect-urlfix.2.diff still works great for us.

any chance of getting it applied coderanger?

comment:26 Changed 14 years ago by labs@…

Ok. I finally fixed my issues (involving http <-> https and the AccountManagerPlugin).
The how to and the patch can be found here: #3783

comment:27 Changed 13 years ago by David

AccountManagerPlugin accepts the referer argument now, but PermRedirectPlugin still doesn't generate the argument.

Any chance this will get fixed?

comment:28 Changed 12 years ago by ejucovy

Cc: ejucovy added

comment:29 Changed 12 years ago by ejucovy

Resolution: fixed
Status: newclosed

(In [12073]) pass along referer (including query string) when redirecting to the login screen, so that after login trac will send the user back to the page he was trying to visit. fixes #2210. (thanks to nathan@acceleration.net and UnwashedMeme for the patch)

comment:30 Changed 12 years ago by ejucovy

I've just adopted this plugin. I've implemented UnwashedMeme's patch on the newly created trunk of this plugin's source, and I'll tag and release a new version of the plugin ASAP.

I've also created a new ticket #10393 to track the separate issue of POST data loss described by Michel and UnwashedMeme in comment:14 and comment:16 -- I think it's fair to call it a bug so I'll follow up on that new ticket.

comment:31 Changed 11 years ago by sdegrande

Cc: Ryan J Ollos Steffen Hoffmann added
Resolution: fixed
Status: closedreopened

It does still not work for me, and I had to add a call to urllib.unquote() in auth.LoginModule._referer().

Or am I missing something ?

I'm using Trac 1.0, with PermRedirect r12073 and AccountManager r12139.

comment:32 in reply to:  31 Changed 11 years ago by ejucovy

Owner: changed from Noah Kantrowitz to ejucovy
Status: reopenednew

Replying to sdegrande:

It does still not work for me, and I had to add a call to urllib.unquote() in auth.LoginModule._referer().

Or am I missing something ?

I'm using Trac 1.0, with PermRedirect r12073 and AccountManager r12139.

Hmm ... I haven't been able to reproduce the problem with an environment like you describe.

Can you describe a little bit more about your situation? When you added a call to urllib.unquote() (I'm assuming you mean in the trac core source code) did that fix the problem? Can you post a diff to show exactly where you added the unquote() call?

Could you also insert a pdb.set_trace() in acct_mgr/web_ui.py at L560 (just after the referer = self._referer(req) block) and see what the value of the referer variable is there when your unquote() patch is applied as well as when it is not? It would also be helpful to know the value of str(req.abs_href()) at that point as well.

comment:33 Changed 11 years ago by sdegrande

I will do ASAP, but here are some more informations.

For that project, there are only authenticated accesses (so there are no permissions for 'anonymous').

The recipe is:

  • the user is logged in
  • the user is displaying the timeline page, for example
  • the user clicks on the 'logout' button
  • the 'login page' of AccountManager is displayed (due to PermRedirect)

With old installation (Trac 0.11), the URL displayed by the browser is
https://the_host/the_project/login
and the HTML form contains
<input type="hidden" name="referer" value="http://the_host/the_project/timeline" />

With new installation (Trac 1.0), the URL displayed by the browser is
https://the_host/the_project/login?referer=http%253A%252F%252Fthe_host%252Fthe_project%252Ftimeline
and the HTML form contains [[BR] <input name="referer" value="http%3A%2F%2Fthe_host%2Fthe_project%2Ftimeline" type="hidden">

Since the 'referer' param seems to be double-encoded, I manually removed the double-encoding from the URL and pointed my browser to
https://the_host/the_project/login?referer=http%3A%2F%2Fthe_host%2Fthe_project%252Ftimeline
and now the HTML login form contains
<input type="hidden" name="referer" value="http://the_host/the_project/timeline" />
and the redirection works as expected.

So it seems that the issue is due to the double-encoding of the URL params. I can try to find where in the code that double-encoding is created, if you need.

comment:34 in reply to:  33 ; Changed 11 years ago by ejucovy

Replying to sdegrande:

Thanks, this is very helpful.

I see you're using HTTPS for login and HTTP for the rest of the site. I suspect that's relevant to the issue. http://trac-hacks.org/ticket/3783#comment:6 describes the same problem with the same setup, and although that ticket is closed, I can't find any existing analysis of where the double-quoting was occurring.

So it seems that the issue is due to the double-encoding of the URL params. I can try to find where in the code that double-encoding is created, if you need.

That would be very helpful, thanks.

If I have time over the next few days I'll try to configure HTTPS-on-login for my own Trac installs and see if I can reproduce the problem under that setup. How did you configure the "HTTPS on /login" behavior? Are you using an Apache rewrite rule on /login or some other method?

comment:35 in reply to:  34 Changed 11 years ago by sdegrande

Replying to ejucovy:

I see you're using HTTPS for login and HTTP for the rest of the site. I suspect that's relevant to the issue. http://trac-hacks.org/ticket/3783#comment:6 describes the same problem with the same setup, and although that ticket is closed, I can't find any existing analysis of where the double-quoting was occurring.

Interesting ! I will try without the https rewriting.

Are you using an Apache rewrite rule on /login or some other method?

That's Apache rewriting yes :

# Redirect all login pages to https
RewriteCond %{HTTPS} off
RewriteRule ^/([^/]+)/login$ https://%{HTTP_HOST}:443/$1/login [last]

(HTTPS being rewrote back to HTTP for the other pages)

comment:36 Changed 11 years ago by ejucovy

Status: newassigned

On my own trac environment I just tried setting up HTTPS for login using the Apache rewrite rule you provided, and I'm now seeing the error too! So it's definitely related to this somehow.

Aha: http://stackoverflow.com/questions/6520484/mod-rewrite-urlencoding-an-already-urlencoded-querystring-parameter-any-way-to

The solution there says to add the NE flag to the rewrite rule, so it should read:

RewriteRule ^/([^/]+)/login$ https://%{HTTP_HOST}:443/$1/login [L,NE]

I just tried this, and it fixed the double-encoding problem for me.

I'll leave this open until you confirm that it fixes the problem for you as well.

comment:37 Changed 11 years ago by sdegrande

It definitively fixes the issue.

I would never have thought that mod_rewrite was the source of the problem...

So, in my opinion, you can close the ticket.

Thanks a lot.

comment:38 Changed 11 years ago by ejucovy

Resolution: fixed
Status: assignedclosed

comment:39 in reply to:  37 ; Changed 11 years ago by Steffen Hoffmann

Replying to sdegrande:

It definitively fixes the issue.

I would never have thought that mod_rewrite was the source of the problem...

Congrats for finally resolving this issue.

Would you mind adding a hint on this finding to the wiki page then? So other Apache users attempting a similar setup wouldn't fall into the same trap, or at least have a second chance to resolve their problem, apart from this ancient ticket. Would have done this on my own, but I don't use Apache, so my wording would probably be not as clear as required. Thanks in advance for consideration.

comment:40 in reply to:  39 Changed 11 years ago by ejucovy

Replying to hasienda:

Would you mind adding a hint on this finding to the wiki page then? So other Apache users attempting a similar setup wouldn't fall into the same trap, or at least have a second chance to resolve their problem, apart from this ancient ticket. Would have done this on my own, but I don't use Apache, so my wording would probably be not as clear as required. Thanks in advance for consideration.

Thanks -- good idea. I just added a hint about this in wiki:PermRedirectPlugin#FrequentlyAskedQuestions (well, it's the only frequently asked question so far :-) and in wiki:PermRedirectPlugin#HTTPSOnly -- hopefully using enough keywords to make the solution appear in Google results.

I also posted about the problem and solution on the related t.e.o ticket: http://trac.edgewall.org/ticket/4733#comment:23

I also just implemented a Trac-only solution to the "HTTPS only" use-case. This is a new feature in PermRedirectPlugin. It's described at wiki:PermRedirectPlugin#HTTPSOnly and #10642, with additional details of the motivation in http://trac.edgewall.org/ticket/4733#comment:24.

For anyone who uses PermRedirectPlugin + AccountManager together, I think this is a better and easier approach. With this new feature installed, only reason to use Apache RewriteRules instead would be in situations where login is handled at the Apache level instead of AccountManager's login component.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain ejucovy.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.