Cloud dot gov fixes #21
Conversation
pkarman
force-pushed
the
cloud-dot-gov-fixes
branch
2 times, most recently
from
d0ea5b4 to
9b44ccc
Compare
| @@ -11,7 +17,7 @@ | |||
| frame_src: %w('self'), # deprecated in CSP 2.0 | |||
| child_src: %w('self'), # CSP 2.0 only; replaces frame_src | |||
| # frame_ancestors: %w('self'), # CSP 2.0 only; overriden by x_frame_options in some browsers | |||
| form_action: %w('self'), # CSP 2.0 only | |||
| form_action: %w('self') + whitelisted_domains.uniq!, # CSP 2.0 only | |||
There was a problem hiding this comment.
The secure_headers gem already removes duplicates from this list, so I wouldn't duplicate the effort.
There was a problem hiding this comment.
Also, do the whitelisted domains apply to all controller actions in this app, or only when signing in and out?
There was a problem hiding this comment.
currently it's all controller actions, because it's the logout link in the nav which appears on every page.
However I am currently working on refactor to switch to SP-initiated SLO which may mean this isn't required at all.
pkarman
force-pushed
the
cloud-dot-gov-fixes
branch
from
7eee34a to
f2588d3
Compare
pkarman
force-pushed
the
cloud-dot-gov-fixes
branch
from
f2588d3 to
c9ff295
Compare
|
@monfresh I made |
| @@ -10,6 +10,7 @@ | |||
| post '/users/sessions' => 'users/sessions#create', as: :user_session | |||
| delete '/users/sessions' => 'users/sessions#destroy', as: :destroy_user_session | |||
There was a problem hiding this comment.
should this be removed if we're going to use get for logging out?
There was a problem hiding this comment.
good point. I've removed it.
|
This looks good. Are there any security concerns with using |
|
Session management is with Devise, so an un-authenticated GET request to logout would either be denied or a no-op. Typically I don't like to make a non-idempotent request into a GET; this is just a convenience to avoid the browser security. To be honest, I'm not sure exactly sure why DELETE or POST poses a problem, since the action is Moving to SP-initiated logout in #23 may change this yet again, so I will revisit once both these PRs are merged. |
|
👍 |
Several small changes to how app is deployed to cloud.gov, most importantly turning off
embed_signso that redirect URLs are smaller than 4k.