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

Kim 163890622 update cookie attributes #1852

Merged
merged 11 commits into from Mar 14, 2019

Conversation

2 participants
@kimallen
Copy link
Contributor

kimallen commented Mar 11, 2019

Description

Follow on to #1774

Adds 3 new cookie attributes upon cookie creation:
SameSite (set to SameSiteStrictMode)
Secure to false if isDevOrTest envt, otherwise set to true
HttpOnly to false in MaskedCSRF token
(HttpOnly to true for session cookie will be added in this story)

Code Review Verification Steps

  • This works in IE.
  • 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

@kimallen kimallen changed the title Kim 163890622 update cookie attributes [WIP]Kim 163890622 update cookie attributes Mar 11, 2019

Path: "/",
Expires: expiry,
MaxAge: maxAge,
HttpOnly: true,

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 11, 2019

Contributor

For this one we want HttpOnly: false per our discussion. Let's add a note that the CSRF cookie needs to be able to be read by the JS so it can be added in the X-CSRF-Token header.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 12, 2019

Codecov Report

Merging #1852 into master will increase coverage by <.01%.
The diff coverage is 71.87%.

@@            Coverage Diff             @@
##           master    #1852      +/-   ##
==========================================
+ Coverage   49.56%   49.57%   +<.01%     
==========================================
  Files         429      429              
  Lines       18479    18487       +8     
  Branches     1632     1632              
==========================================
+ Hits         9160     9165       +5     
- Misses       8515     8518       +3     
  Partials      804      804

@kimallen kimallen requested a review from chrisgilmerproj Mar 12, 2019

@kimallen kimallen changed the title [WIP]Kim 163890622 update cookie attributes Kim 163890622 update cookie attributes Mar 12, 2019

@kimallen kimallen requested review from stangah and pjdufour-truss Mar 12, 2019

useSecureCookie := true
if isDevOrTest {
useSecureCookie = false
}

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 13, 2019

Contributor

Could you just write useSecureCookie := !isDevOrTest?

Path: "/",
Expires: expiry,
MaxAge: maxAge,
HttpOnly: false, // must be false to be read by POST/PUT/PATCH/DELETE requests

This comment has been minimized.

@chrisgilmerproj

chrisgilmerproj Mar 13, 2019

Contributor

The comment here I believe is must be false to be read by client for use in POST/PUT/PATCH/DELETE requests.

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

I have two relatively small changes here that you can read inline. Then this looks ready to go!

@chrisgilmerproj
Copy link
Contributor

chrisgilmerproj left a comment

🚀 - Nice job!

@kimallen kimallen merged commit 4cfe8e0 into master Mar 14, 2019

19 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_tools Your tests passed on CircleCI!
Details
ci/circleci: client_test Your tests passed on CircleCI!
Details
ci/circleci: client_test_coverage 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 71.87% of diff hit (target 49.56%)
Details
codecov/project 49.57% (+<.01%) compared to fbaec44
Details

@kimallen kimallen deleted the kim-163890622-update-cookie-attributes branch Mar 14, 2019

@chrisgilmerproj chrisgilmerproj referenced this pull request Mar 14, 2019

Merged

Cg disable samesite cookie #1870

2 of 2 tasks 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.