sophie: A cartoon-like representation of a girl standing on a hill, with brown hair, blue eyes, a flowery top, and blue skirt. ☀ (Default)
Sophie ([personal profile] sophie) wrote in [site community profile] dw_dev2009-04-14 02:24 pm
Entry tags:

Lions and tigers and login redirects, oh my!

I'm doing some investigation into bug 125, which at first sight seems easy. And, to be fair, it is, if you just want to get it done.

However, when you look deeper, there are some oddities.



  • login.bml supports two POST parameters when using it to log in; one called ret and the other called returnto. They perform much the same function - redirecting you to another page on a successful login - except ret is checked more rigorously that it's allowed to redirect to the given URL, whereas returnto just blindly redirects you without checking anything about it other than appending $LJ::SITEROOT if the input begins with a slash.

  • I believe the above is the case because the ret POST parameter can be populated via a ?ret= GET argument when rendering, whereas the same isn't true of returnto. However, the login widget at the top *does* use a ?returnto= GET argument to populate a returnto POST parameter, while ignoring ret.

  • login.bml *also* supports a ref POST parameter, which, if neither of the above two parameters are used, goes through an entirely different series of checks (the most suitable ones for this bug, actually), and redirects to the given URL on a successful login.

  • The ret parameter mentioned above also has a dual purpose; if, when logging in, the ?ret= GET argument is set to the value "1", and the referring page has $LJ::DOMAIN in the URL, it simply redirects to the referring page. However, as this stage can only be reached via a POST request, the only way this can happen is through a form that starts off with something like <form method="post" action="/login.bml?ret=1">, which is kinda nonsensical.



This is far too many codepaths, and worryingly, returnto could be used as a phishing entrypoint - going through the actual login process, but then redirecting to a totally different site.

Research indicates that the initial purpose of the ?ret= GET argument was that it would populate the ref POST parameter with the referring page URL, if it was set to a true value. The code for this is so old that the change in which it was introduced isn't even in LJ's code versioning system - it was present in the initial import back in June 2001 (check lines 21-24, and see login_do.bml too, which was the other half of the equation back then).

The code evolved such that ?ret=1 kept mostly the same behaviour, while the ret parameter in general changed to allow redirects to external sources which authenticated from LJ. (I'm uncertain as to how external services would have reliably been able to tell, though.) I suspect that this functionality can be considered deprecated now that we have OpenID, although the commit message does indicate that it's a "small part of the bigger picture", so there may be more to this than I can see right now.

The returnto parameter was introduced when, instead of printing a login widget on the page itself when the <?needlogin?> BML tag was used, it was decided to make the tag redirect to the homepage instead. It's probable that the ret parameter couldn't be used because of its overly-restrictive checks, and of course ref was probably not used as it can't be populated via a GET call, which is how the redirect works.

All the above means we have a couple of things to ponder.

  • The bug's description is to change the redirect to the login page rather than the homepage, but perhaps what we really want to do is revert the changeset that redirected to the homepage in the first place?

    Thinking about it, the 'protected content' logic, which is what the bug is about, doesn't use <?needlogin?> anyway. Therefore, we'll need to go with the next point:

  • If we do want to change the redirect to go to the login page rather than taking the redirect out completely, which GET argument should be used for the original page?

    • I'm reluctant to allow returnto to be populated via GET because of its possible use as a phishing vector.

    • The overloaded use of ret seems to be deprecated via OpenID, and I'm not sure if ?ret=1 would work properly in a redirect.

    • The use of ref seems to be the sanest way, and fits its original purpose of acting as the referrer, but can't currently be populated via GET (and it would seem, well, weird to allow it to be).


Whichever we choose, I think we need to take a good, long look at all these options and hack at them with a machete.
afuna: Cat under a blanket. Text: "Cats are just little people with Fur and Fangs" (Default)

[personal profile] afuna 2009-04-18 11:42 am (UTC)(link)
Hmm, okay. Two thoughts


a.) can you handle it the way that logging in from the frontpage does right now?
b.) if that is not easy to do, I think just a login form on its own would be good.

My primary concern is to make it really obvious that you need to log in, by giving you a page where the login form is the main focus, and not just stuck in the corner