652020 - Checking cookie for authentication should only be done over HTTPS

Status: VERIFIED FIXED

(Websites Graveyard :: getpersonas.com, defect)

Description

[Matthew N. [:MattN]]

Users are ending up at /signin if they try to visit a page requiring login over HTTP.  We should redirect to the requested page over HTTPS if the visit is over HTTP instead of redirecting to /signin since the user may be logged in over HTTPS.

Also, if a user ends up on /signin (over HTTPS) and they are already logged-in, they should be redirected to their destination (or at least provided with a button to take them there).

Comment 1

[Matthew N. [:MattN]]

STR:
1) Login to getpersonas.com (cookie is set in secure mode to only be sent over HTTPS)
2) Visit http://www.getpersonas.com/en-US/upload?check=false (which is a page that requires login) over HTTP.  This could happen in the wild from a bookmark or an internal link.

Actual Result:
Since the session cookie is not sent with this request, the user is redirected to https://www.getpersonas.com/en-US/signin?return=%2Fen-US%2Fupload%3Fcheck%3Dfalse (over HTTPS always).  Note that the upper-right corner of the page shows that you are logged in already (because it is HTTPS now again)

Expected Result:
Instead of redirecting to /signin, redirect to the HTTPS version of the page (if it is just HTTP). This way the session cookie is sent to do a proper auth. check.
The HTTPS version of the page should continue to redirect to /signin if the users is not logged-in.

Comment 2

[Brandon Savage [:brandon]]

Attachment: Enforce https on certain pages

This patch enforces HTTPS on signin, upload, admin and profile edits.

Comment 3

[Matthew N. [:MattN]]

Comment on attachment 528934 [details] [diff] [review]
Enforce https on certain pages

Review of attachment 528934 [details] [diff] [review]:

* Add /delete (persona deletion) since it only works when logged-in
* Please combine the "RewriteCond %{REQUEST_URI}" lines for the non-admin pages to reduce duplication.  The admin ones are slightly different so OK to keep separate if you want:
    RewriteCond %{REQUEST_URI} ^/([a-zA-Z-]+/)?(forgot_password|signin|upload|profile|delete.*)$
** Note that I also added the ^ & $ to the regex to make it more efficient and prevent false positives

Can you think of any cases where he other part of the solution I proposed would be useful?  I can't at the moment but it's late here.
(From Comment 0)
> Also, if a user ends up on /signin (over HTTPS) and they are already logged-in,
> they should be redirected to their destination (or at least provided with a
> button to take them there).

r+ with these changes

Comment 4

[Brandon Savage [:brandon]]

Fixed in r88185. The feedback for the patch was incorporated.

Comment 5

[Matthew N. [:MattN]]

I just tested this fix on stage and there is an issue of double URL encoding caused by the redirect:

STR:
1) Login at https://personas.stage.mozilla.com
2) Visit http://personas.stage.mozilla.com/en-US/gallery/ (note: HTTP)
3) Click 'Sign In' on that page.

Actual Result:
The following req./resp. happens because of the rewrite added in this bug:
=> GET /en-US/signin?return=%2Fen-US%2Fgallery%2F HTTP/1.1

<= HTTP/1.1 302 Found
Location: https://personas.stage.mozilla.com/en-US/signin?return=%252Fen-US%252Fgallery%252F
Note that the % has been encoded to %25

This eventually redirects to /%2Fen-US%2Fgallery%2F which gives a 404.

Expected result:
Redirect should not be escaped again:
<= HTTP/1.1 302 Found
Location: https://personas.stage.mozilla.com/en-US/signin?return=%2Fen-US%2Fgallery%2F

This can be achieved by adding the 'NE' flag to the two RewriteRule's committed.

(see http://httpd.apache.org/docs/2.0/mod/mod_rewrite.html#rewriteflags )

Comment 6

[Brandon Savage [:brandon]]

Good point. Fixed in r88217.

Comment 7

[Vishal Kamdar [:vish_moz]]

Verified, FIXED.