Issue 191550043: Enforce X-Frame-Options=DENY on all services. (Closed)
DescriptionEnforce X-Frame-Options=DENY on all services.
We currently have no use-case of using frames. We could change it if ever needed
but I prefer to default to more secure.
R=vadimsh@chromium.org
BUG=swarming:193
Committed: https://code.google.com/p/swarming/source/detail?repo=default&r=fda2d73eb3c1561504fa0c3b0feadf7a15117453
Patch Set 1 #
Total comments: 4
Patch Set 2 : Use AuthenticationHandler.frame_options #MessagesTotal messages: 11
https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... File appengine/components/components/auth/handler.py (right): https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... appengine/components/components/auth/handler.py:177: self.response.headers['X-Frame-Options'] = 'DENY' I prefer it to be put in some other (more highlevel) class (or classes). AuthenticatingHandler is used by API handlers too. X-Frame-Options header would look extremely odd for API responses.
Sign in to reply to this message.
https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... File appengine/components/components/auth/handler.py (right): https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... appengine/components/components/auth/handler.py:177: self.response.headers['X-Frame-Options'] = 'DENY' On 2015/01/08 19:21:35, vadimsh wrote: > I prefer it to be put in some other (more highlevel) class (or classes). > AuthenticatingHandler is used by API handlers too. X-Frame-Options header would > look extremely odd for API responses. Yes but API response could still be embedded in an iframe.
Sign in to reply to this message.
On 2015/01/08 20:41:26, M-A wrote: > https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... > File appengine/components/components/auth/handler.py (right): > > https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... > appengine/components/components/auth/handler.py:177: > self.response.headers['X-Frame-Options'] = 'DENY' > On 2015/01/08 19:21:35, vadimsh wrote: > > I prefer it to be put in some other (more highlevel) class (or classes). > > AuthenticatingHandler is used by API handlers too. X-Frame-Options header > would > > look extremely odd for API responses. > > Yes but API response could still be embedded in an iframe. Which is not a big deal. X-Frame-Options is a measure against Clickjacking attack: https://www.owasp.org/index.php/Clickjacking
Sign in to reply to this message.
On 2015/01/08 21:35:22, vadimsh wrote: > On 2015/01/08 20:41:26, M-A wrote: > > > https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... > > File appengine/components/components/auth/handler.py (right): > > > > > https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... > > appengine/components/components/auth/handler.py:177: > > self.response.headers['X-Frame-Options'] = 'DENY' > > On 2015/01/08 19:21:35, vadimsh wrote: > > > I prefer it to be put in some other (more highlevel) class (or classes). > > > AuthenticatingHandler is used by API handlers too. X-Frame-Options header > > would > > > look extremely odd for API responses. > > > > Yes but API response could still be embedded in an iframe. > > Which is not a big deal. So you prefer me to I add create a new class RequestHandler to each *_frontend.py inheriting from auth.AuthenticatingHandler?
Sign in to reply to this message.
On 2015/01/08 21:43:18, M-A wrote: > On 2015/01/08 21:35:22, vadimsh wrote: > > On 2015/01/08 20:41:26, M-A wrote: > > > > > > https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... > > > File appengine/components/components/auth/handler.py (right): > > > > > > > > > https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... > > > appengine/components/components/auth/handler.py:177: > > > self.response.headers['X-Frame-Options'] = 'DENY' > > > On 2015/01/08 19:21:35, vadimsh wrote: > > > > I prefer it to be put in some other (more highlevel) class (or classes). > > > > AuthenticatingHandler is used by API handlers too. X-Frame-Options header > > > would > > > > look extremely odd for API responses. > > > > > > Yes but API response could still be embedded in an iframe. > > > > Which is not a big deal. > > So you prefer me to I add create a new class RequestHandler to each > *_frontend.py inheriting from auth.AuthenticatingHandler? Yes. Or make AuthenticatingHandler.frame_options_header class property and set it to 'DENY' by default, but unset it to None in ApiHandler? Don't know.
Sign in to reply to this message.
https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... File appengine/components/components/auth/handler.py (right): https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... appengine/components/components/auth/handler.py:177: self.response.headers['X-Frame-Options'] = 'DENY' Also, maybe set it before .dispatch()? That way 'self.abort(...)' and other responses initiated by exceptions would also contain the header.
Sign in to reply to this message.
On 2015/01/08 21:52:52, vadimsh wrote: > Yes. Or make AuthenticatingHandler.frame_options_header class property and set > it to 'DENY' by default, but unset it to None in ApiHandler? Don't know. I liked this idea since it's simple and the default behavior makes sense so I implemented it. Sadly, I had mistakenly merged this CL into my local master branch and pushed it by accident when rolling client/. This causes commit https://code.google.com/p/swarming/source/detail?r=6eaea0d26e25ace0eb8c79026c.... I reverted in https://code.google.com/p/swarming/source/detail?r=66bb8d2b488dc18b13e760eeb8... since it was not intended. https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... File appengine/components/components/auth/handler.py (right): https://codereview.appspot.com/191550043/diff/1/appengine/components/componen... appengine/components/components/auth/handler.py:177: self.response.headers['X-Frame-Options'] = 'DENY' On 2015/01/08 21:54:47, vadimsh wrote: > Also, maybe set it before .dispatch()? That way 'self.abort(...)' and other > responses initiated by exceptions would also contain the header. Done.
Sign in to reply to this message.
ping :)
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
Message was sent while issue was closed.
Committed patchset #2 (id:20001) manually as fda2d73eb3c1561504fa0c3b0feadf7a15117453 (presubmit successful).
Sign in to reply to this message.
|
