[GitHub] [couchdb] rnewson opened a new pull request #3866: Add SameSite setting when clearing session cookie (2)

[GitBox <gi...@apache.org>]

rnewson opened a new pull request #3866:
URL: https://github.com/apache/couchdb/pull/3866


   ## Overview
   
   Deleting the session cookie on auth failure does not work if the SameSite attribute is enabled when using browsers that enforce it because we forgot to add the property. This PR adds the SameSite attribute if enabled.
   
   ## Testing recommendations
   
   Enable SameSite, login via the dashboard, then attempt authentication again with the wrong password. Observe that the AuthSession cookie has a value before that attempt but not after.
   
   ## Related Issues or Pull Requests
   
   <!-- If your changes affects multiple components in different
        repositories please put links to those issues or pull requests here.  -->
   
   ## Checklist
   
   - [x] Code is written and works correctly
   - [ ] Changes are covered by tests
   - [ ] Any new configurable parameters are documented in `rel/overlay/etc/default.ini`
   - [ ] A PR for documentation changes has been made in https://github.com/apache/couchdb-documentation
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] rnewson closed pull request #3866: Add SameSite setting when clearing session cookie (2)

[GitBox <gi...@apache.org>]

rnewson closed pull request #3866:
URL: https://github.com/apache/couchdb/pull/3866


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [couchdb] nickva commented on a change in pull request #3866: Add SameSite setting when clearing session cookie (2)

[GitBox <gi...@apache.org>]

nickva commented on a change in pull request #3866:
URL: https://github.com/apache/couchdb/pull/3866#discussion_r766210992



##########
File path: src/couch/src/couch_httpd_auth.erl
##########
@@ -447,7 +447,7 @@ handle_session_req(#httpd{method = 'POST', mochi_req = MochiReq} = Req, AuthModu
             authentication_warning(Req, UserName),
             % clear the session
             Cookie = mochiweb_cookies:cookie(
-                "AuthSession", "", [{path, "/"}] ++ cookie_scheme(Req)
+                "AuthSession", "", [{path, "/"}] ++ cookie_scheme(Req) ++ same_site()

Review comment:
       Should we also add the domain in there? Like we do in: https://github.com/apache/couchdb/blob/c12f0a723cc213679bb90cd263b1187b153627d9/src/couch/src/couch_httpd_auth.erl#L506-L513
   
   I think that might fix issue https://github.com/apache/couchdb/issues/3840
   
   Might even make sense to have a `clear_session(Req)` function to reuse the code and prevent future discrepancies.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@couchdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org