Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

#8598 closed enhancement (wontfix)

HEAD request to `/login` should not redirect

Reported by: osimons Owned by: osimons
Priority: normal Milestone:
Component: general Version: none
Severity: normal Keywords:
Cc: Branch:
Release Notes:
API Changes:

Description

Bitten now supports 'real' Trac authentication py first making a HEAD request to /login and collecting cookies.

No big deal, but there would be no need for login module to redirect to a GET for project '/' for such requests. If req.method.lower() == 'head' it should just return a 200 ok with cookies and empty body.

Attachments (1)

t8598-no_redirect_for_head_login-r8551-011.diff (754 bytes ) - added by osimons 10 years ago.
Patch that don't redirect on HEAD requests to login or logout.

Download all attachments as: .zip

Change History (8)

by osimons, 10 years ago

Patch that don't redirect on HEAD requests to login or logout.

comment:1 by osimons, 10 years ago

Milestone: 0.11.6

Anyone mind me committing this to 0.11 and merging to trunk? I think the patch is mostly harmless, and just makes for much more correct behaviour.

comment:2 by Remy Blank, 10 years ago

Is this an optimization only, or does Bitten depend on the change? In the former case, I would be -1 for the change. As I understand HEAD and GET, the former should return the same headers as the latter, with an empty body (in particular, with a correct "Content-Length"). This would not be true anymore with the patch.

Also, I don't think the optimization is worth the trouble, as Bitten will do at most one request to /login per build, so the effect is negligible.

comment:3 by osimons, 10 years ago

It is a minor optimization only. I just spotted redirect to '/' with loading of a full page that the HEAD response does not really want or need. It just makes the response more inline with my expectations as I just need the auth cookies from headers. However, I can see that it may break with the specification.

There would be some difference in that the status code would change (303 ⇒ 200), and the location header used for redirect would not be included. Neither response have any body though, so that would not make much difference even if we added some missing headers.

I suppose I could somehow trap this in the client and ignore the redirect, but that makes it much more cumbersome as I'd need to filter out the valid redirects from the ones not to follow… For instance if /login is redirected from old domain name to new domain name by Apache. It isn't that important :-)

in reply to:  3 comment:4 by Remy Blank, 10 years ago

Replying to osimons:

It is a minor optimization only. I just spotted redirect to '/' with loading of a full page that the HEAD response does not really want or need.

If you do a HEAD on /login, and you get a redirect to /, doesn't the client also do a HEAD on / afterward (instead of a GET)? That would mean that you shouldn't get the full page after the redirect.

comment:5 by osimons, 10 years ago

Nope, I get Dispatching <Request "GET u'/'"> in Trac log.

comment:6 by Remy Blank, 10 years ago

According to 2616, in 10.3.3, for 302 redirect (which we do on /login):

Note: RFC 1945 and RFC 2068 specify that the client is not allowed to change the method on the redirected request. However, most existing user agent implementations treat 302 as if it were a 303 response, performing a GET on the Location field-value regardless of the original request method. The status codes 303 and 307 have been added for servers that wish to make unambiguously clear which kind of reaction is expected of the client.

So the client should not change the method, but most do. And indeed, looking at urllib2.HTTPRedirectHandler, the Python implementation does change the method to GET. It would be easy, though, to derive HTTPRedirectHandler in Bitten and override redirect_request() so that it returns a SaneHTTPRequest with a HEAD method instead.

I suggest "wontfix".

in reply to:  6 comment:7 by osimons, 10 years ago

Milestone: 0.11.6
Resolution: wontfix
Status: newclosed

Replying to rblank:

I suggest "wontfix".

Oki.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain osimons.
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from osimons to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.