Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add httponly attribute session #2127

Merged
merged 9 commits into from May 16, 2019

Conversation

4 participants
@sarboc
Copy link
Contributor

commented May 13, 2019

Description

Disallow client-side decoding of our cookies

Reviewer Notes

Check out the history here: #1926

Code Review Verification Steps

  • Tested in the Experimental environment (for changes to containers, app startup, or connection to data stores)
  • Request review from a member of a different team.
  • Have the Pivotal acceptance criteria been met for this change?

References

@sarboc sarboc referenced this pull request May 13, 2019

Closed

[WIP] Make session cookie HttpOnly #1926

0 of 3 tasks complete

@sarboc sarboc force-pushed the sb_164585573-add-httponly-attribute-session branch from f65c466 to f08fbc7 May 13, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

I'm pretty excited about this PR. Let's figure out what's going on with the cookie thing and possibly break it out to another PR if we need to fix it (happy to help you merge that PR before this PR). After that and if the rest of the tests pass then let's get this approved!

@sarboc sarboc force-pushed the sb_164585573-add-httponly-attribute-session branch from 1db094a to 23c8f08 May 13, 2019

@codecov

This comment has been minimized.

Copy link

commented May 13, 2019

Codecov Report

Merging #2127 into master will decrease coverage by 0.16%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2127      +/-   ##
==========================================
- Coverage   59.03%   58.86%   -0.16%     
==========================================
  Files         237      237              
  Lines       13795    13781      -14     
==========================================
- Hits         8143     8112      -31     
- Misses       4662     4685      +23     
+ Partials      990      984       -6

@sarboc sarboc force-pushed the sb_164585573-add-httponly-attribute-session branch 2 times, most recently from d922dba to 30305bc May 14, 2019

@sarboc sarboc force-pushed the sb_164585573-add-httponly-attribute-session branch from 30305bc to b163aab May 14, 2019

Make sure the page is loaded before cookies clear
...otherwise we clear cookies before they are set, and we set them after
they're claered, and tests break, and it's bad. :-(

@sarboc sarboc force-pushed the sb_164585573-add-httponly-attribute-session branch from 293091c to 558c77e May 15, 2019

@sarboc sarboc marked this pull request as ready for review May 15, 2019

@sarboc sarboc requested a review from chrisgilmerproj May 15, 2019

@chrisgilmerproj
Copy link
Contributor

left a comment

Looks good!

@jim
Copy link
Contributor

left a comment

🚢

@jim jim self-requested a review May 16, 2019

@jim

jim approved these changes May 16, 2019

Copy link
Contributor

left a comment

D'oh! 🚢

@reggieriser
Copy link
Contributor

left a comment

I left a note about some unused code now that we're not doing the 401 check anymore, but not going to block on that since Sara's not here right now. We can make a separate PR to get rid of it if we want.

return {
...state,
isLoading: false,
hasErrored: true,
hasSucceeded: false,
error: action.error,
userInfo: userInfoDefault(),
};
case SET_LOGGED_OUT:

This comment has been minimized.

Copy link
@reggieriser

reggieriser May 16, 2019

Contributor

I think we can get rid of this case and the SET_LOGGED_OUT constant since we're not dispatching this action anymore.

@chrisgilmerproj chrisgilmerproj merged commit 51425a8 into master May 16, 2019

20 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
ci/circleci: acceptance_tests_experimental Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_local Your tests passed on CircleCI!
Details
ci/circleci: acceptance_tests_staging Your tests passed on CircleCI!
Details
ci/circleci: build_app Your tests passed on CircleCI!
Details
ci/circleci: build_migrations Your tests passed on CircleCI!
Details
ci/circleci: build_tasks Your tests passed on CircleCI!
Details
ci/circleci: build_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_api Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_mymove Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_office Your tests passed on CircleCI!
Details
ci/circleci: integration_tests_tsp Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_golang Your tests passed on CircleCI!
Details
ci/circleci: pre_deps_yarn Your tests passed on CircleCI!
Details
ci/circleci: pre_test Your tests passed on CircleCI!
Details
ci/circleci: server_test Your tests passed on CircleCI!
Details
ci/circleci: server_test_coverage Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 59.03%)
Details
codecov/project/go Absolute coverage decreased by -0.17% but relative coverage increased by +41.16% compared to 8d0092a
Details
@chrisgilmerproj

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

With @sarboc 's permission I've merged this in.

@chrisgilmerproj chrisgilmerproj deleted the sb_164585573-add-httponly-attribute-session branch May 16, 2019

@chrisgilmerproj chrisgilmerproj referenced this pull request May 16, 2019

Merged

Remove unused SET_LOGGED_OUT #2142

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.