Skip to content

Commit

Permalink
Remove HSTS code from the app
Browse files Browse the repository at this point in the history
The per-user logic is much more twisty than it needs to be, and a 3
line nginx rule will do much the same thing.
  • Loading branch information
JordanMilne committed Sep 24, 2015
1 parent 42f96dc commit 5431f0a
Show file tree
Hide file tree
Showing 6 changed files with 7 additions and 116 deletions.
7 changes: 0 additions & 7 deletions r2/example.ini
Original file line number Diff line number Diff line change
Expand Up @@ -526,9 +526,6 @@ auth_trust_http_authorization = false


############################################ SSL
# max-age for Strict Transport Security, setting this to 0 disables
# HSTS and revokes any previous HSTS grants.
hsts_max_age = 10886400


############################################ CASSANDRA
Expand Down Expand Up @@ -865,10 +862,6 @@ feature_allow_force_https = {"employee": true}
feature_require_https = off
# Used to gradually redirect loggedin users to HTTPS (without HSTS or secure cookies)
feature_https_redirect = off
# HSTS grants are disabled by default since they make it a pain for devs
# to use local HTTP services. Beware that this will disable grant revocation
# as well.
feature_give_hsts_grants = off
feature_multireddit_customizations = off
# Test if `https_testing_img_test` is loaded with an acceptable HTTPS cert according to users' browsers
feature_test_https_certs = off
Expand Down
43 changes: 0 additions & 43 deletions r2/r2/controllers/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@

from r2.controllers.reddit_base import (
cross_domain,
hsts_modify_redirect,
is_trusted_origin,
MinimalController,
pagecache_policy,
Expand Down Expand Up @@ -610,12 +609,6 @@ def _login(self, responder, user, rem = None):
if request.params.get("hoist") != "cookie":
responder._send_data(modhash = user.modhash())
responder._send_data(cookie = user.make_cookie())
if user.https_forced:
# The client may decide to redirect somewhere after a successful
# login, send it our HSTS grant endpoint so it can redirect through
# there and pick up the user's grant.
hsts_redir = "https://" + g.domain + "/modify_hsts_grant?dest="
responder._send_data(hsts_redir=hsts_redir)
responder._send_data(need_https=user.https_forced)

@validatedForm(VLoggedOut(),
Expand Down Expand Up @@ -1262,40 +1255,6 @@ def POST_clear_sessions(self, form, jquery, password, dest):
# invalidated. drop a new cookie.
self.login(c.user)

@validatedForm(
VUser(),
VModhash(),
password=VVerifyPassword("curpass", fatal=False),
force_https=VBoolean("force_https"),
)
def POST_set_force_https(self, form, jquery, password, force_https):
"""Toggle HTTPS-only sessions, invalidating other sessions.
A valid password (`curpass`) must be supplied.
"""
if form.has_errors("curpass", errors.WRONG_PASSWORD):
return
if not force_https and feature.is_enabled("require_https"):
form.set_text(".status",
_("you may not disable HTTPS on this account"))
return
c.user.pref_force_https = force_https
c.user._commit()

# run the change password command to get a new salt.
# OAuth tokens are fine since that always happened over HTTPS.
change_password(c.user, password)
form.set_text(".status",
_("HTTPS preferences have been successfully changed"))
form.set_inputs(curpass="")

# the password salt has changed, so the user's cookie has been
# invalidated. drop a new cookie.
self.login(c.user)

# Modify their HSTS grant
form.redirect(hsts_modify_redirect("/prefs/security"))

@validatedForm(
VUser(),
VModhash(),
Expand Down Expand Up @@ -1408,8 +1367,6 @@ def POST_delete_user(self, form, jquery, delete_message, username, user, confirm
form.has_errors("delete_message", errors.TOO_LONG) or
form.has_errors("confirm", errors.CONFIRM)):
redirect_url = "/?deleted=true"
if c.user.https_forced:
redirect_url = hsts_modify_redirect(redirect_url)
c.user.delete(delete_message)
form.redirect(redirect_url)

Expand Down
10 changes: 2 additions & 8 deletions r2/r2/controllers/front.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@
from r2.controllers.reddit_base import (
base_listing,
disable_subreddit_css,
hsts_modify_redirect,
hsts_eligible,
pagecache_policy,
PAGECACHE_POLICY,
paginated_listing,
Expand Down Expand Up @@ -1472,6 +1470,7 @@ def GET_gilding(self):
@validate(dest=VDestination(default='/'))
def _modify_hsts_grant(self, dest):
"""Endpoint subdomains can redirect through to update HSTS grants."""
# TODO: remove this once it stops getting hit
from r2.lib.base import abort
require_https()
if request.host != g.domain:
Expand Down Expand Up @@ -1659,13 +1658,8 @@ def GET_logout(self, dest):
dest=VDestination())
def POST_logout(self, dest):
"""wipe login cookie and redirect to referer."""

# Check eligibility before calling logout(), as logout() changes
# cookies that hsts_eligible() looks at
is_hsts_eligible = hsts_eligible()
self.logout()
self.hsts_redirect(dest, is_hsts_eligible=is_hsts_eligible)

self.redirect(dest)

@validate(VUser(),
dest=VDestination())
Expand Down
6 changes: 2 additions & 4 deletions r2/r2/controllers/post.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@
###############################################################################
from r2.lib.pages import *
from reddit_base import (
hsts_eligible,
hsts_modify_redirect,
set_over18_cookie,
delete_over18_cookie,
)
Expand Down Expand Up @@ -185,7 +183,7 @@ def POST_login(self, dest, *a, **kw):
return LoginPage(user_login = request.POST.get('user'),
dest = dest).render()

return self.hsts_redirect(dest)
return self.redirect(dest)

@csrf_exempt
@validate(dest = VDestination(default = "/"))
Expand All @@ -198,7 +196,7 @@ def POST_reg(self, dest, *a, **kw):
return LoginPage(user_reg = request.POST.get('user'),
dest = dest).render()

return self.hsts_redirect(dest)
return self.redirect(dest)

def GET_login(self, *a, **kw):
return self.redirect('/login' + query_string(dict(dest="/")))
Expand Down
45 changes: 2 additions & 43 deletions r2/r2/controllers/reddit_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -766,22 +766,13 @@ def make_url_https(url):
return new_url.unparse()


def hsts_eligible():
def want_secure_session():
# When we're on HTTP, the secure_session cookie is the only way we can
# prove the user wants HSTS.
return (c.user.https_forced or
(not c.secure and have_secure_session_cookie()))


def hsts_modify_redirect(url):
hsts_url = UrlParser("https://" + g.domain + "/modify_hsts_grant")
# `dest` should be fully qualified so users get sent back to the right
# subdomain. `dest` must also be HTTPS because Safari will crash if
# you redirect to an http: URL after giving a grant.
hsts_url.query_dict['dest'] = make_url_https(url)
return hsts_url.unparse()


def enforce_https():
"""Enforce user preferences for HTTPS connections.
Expand Down Expand Up @@ -826,14 +817,11 @@ def enforce_https():
if have_secure_session_cookie() and not c.user_is_loggedin:
redirect_url = make_url_https(request.fullurl)

need_grant = False
grant = None
# Forcing the users through the HSTS gateway probably wouldn't help much for
# other render types since they're mostly made by clients that don't respect
# HSTS.
if c.render_style in {"html", "compact", "mobile"} and not is_api_request:
if hsts_eligible():
grant = g.hsts_max_age
if want_secure_session():
# They're forcing HTTPS but don't have a "secure_session" cookie?
# Somehow their HTTPS preferences changed without invalidating their
# old cookies, ensure that this session's cookies are secured
Expand All @@ -851,31 +839,18 @@ def enforce_https():
# The client might not support HSTS, or might have had their
# grant expire. redirect to the HTTPS version through the HSTS
# endpoint.
need_grant = True
redirect_url = make_url_https(request.fullurl)
else:
grant = 0
if c.secure:
# User disabled HTTPS forcing under another session or their
# session became invalid and they're left with a dangling cookie
if have_secure_session_cookie():
change_user_cookie_security(False)
need_grant = True

# Gradual rollout for HTTPS
if feature.is_enabled("https_redirect") and not c.secure:
redirect_url = make_url_https(request.fullurl)

if feature.is_enabled("give_hsts_grants") and grant is not None:
if request.host == g.domain and c.secure:
# Always set an HSTS header if we can and we're on the base domain
c.hsts_grant = grant
elif need_grant:
# Definitely need to change the grant, but we're not on an origin
# where we can modify it, redirect through one that can.
dest = redirect_url or request.fullurl
redirect_url = hsts_modify_redirect(dest)

if redirect_url:
headers = {"Cache-Control": "private, no-cache", "Pragma": "no-cache"}
abort(307, location=redirect_url, headers=headers)
Expand Down Expand Up @@ -1114,7 +1089,6 @@ def pre(self):
g.domain_prefix)
c.secure = request.environ["wsgi.url_scheme"] == "https"
c.request_origin = request.host_url
c.hsts_grant = None

#check if user-agent needs a dose of rate-limiting
if not c.error_page:
Expand Down Expand Up @@ -1292,10 +1266,6 @@ def post(self):
if c.ratelimit_headers:
response.headers.update(c.ratelimit_headers)

if c.hsts_grant is not None:
hsts_val = "max-age=%d; includeSubDomains" % c.hsts_grant
response.headers["Strict-Transport-Security"] = hsts_val

# send cookies
# HACK: make sure c.user always gets set to something
secure_cookies = c.user and c.user.https_forced
Expand Down Expand Up @@ -1404,17 +1374,6 @@ def should_update_last_visit(self):

return request.method.upper() != "POST"

@classmethod
def hsts_redirect(cls, dest, is_hsts_eligible=None):
"""Redirect to `dest` via the HSTS grant endpoint"""
if is_hsts_eligible is None:
is_hsts_eligible = hsts_eligible()
if is_hsts_eligible:
dest = hsts_modify_redirect(dest)
return cls.redirect(dest, preserve_extension=False)
else:
return cls.redirect(dest)


class OAuth2ResourceController(MinimalController):
defer_ratelimiting = True
Expand Down
12 changes: 1 addition & 11 deletions r2/r2/public/static/js/login.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,10 +177,6 @@ r.login.ui = {
}

this.popup.showLogin(actionDetails.description, dest && $.proxy(function(result) {
var hsts_redir = result.json.data.hsts_redir
if(hsts_redir) {
dest = hsts_redir + encodeURIComponent(dest)
}
window.location = dest
}, this))

Expand Down Expand Up @@ -234,14 +230,8 @@ r.ui.LoginForm.prototype = $.extend(new r.ui.Form(), {
this.$el.addClass('working')
var base = r.config.extension ? '/.'+r.config.extension : '/',
defaultDest = /\/login\/?$/.test($.url().attr('path')) ? base : window.location,
destParam = this.$el.find('input[name="dest"]').val(),
hsts_redir = result.json.data.hsts_redir
destParam = this.$el.find('input[name="dest"]').val()
var redir = destParam || defaultDest
// We might need to redirect through the base domain to grab
// our HSTS grant.
if (hsts_redir) {
redir = hsts_redir + encodeURIComponent(redir)
}
if (window.location === redir) {
window.location.reload();
} else {
Expand Down

0 comments on commit 5431f0a

Please sign in to comment.