[GitHub] [tomcat] akurth opened a new pull request #377: Set "Secure" session cookie attribute if called under SSL.

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] akurth opened a new pull request #377: Set "Secure" session cookie attribute if called under SSL.

GitBox

akurth opened a new pull request #377:
URL: https://github.com/apache/tomcat/pull/377


   Missing "Secure" flag may lead to redirect loops under certain
   conditions, e.g. when sameSiteCookies=none has been set at the
   CookieProcessor and browser is Chrome.
   
   Fixes <https://bz.apache.org/bugzilla/show_bug.cgi?id=64921>.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] ChristopherSchultz commented on a change in pull request #377: Set "Secure" session cookie attribute if called under SSL.

GitBox

ChristopherSchultz commented on a change in pull request #377:
URL: https://github.com/apache/tomcat/pull/377#discussion_r524294255



##########
File path: test/org/apache/catalina/valves/TestLoadBalancerDrainingValve.java
##########
@@ -238,6 +238,8 @@ private void runValve(String jkActivation,
                 expectedCookie.setPath(cookieConfig.getPath());
                 expectedCookie.setMaxAge(0);
 
+                EasyMock.expect(request.isSecure()).andReturn(true);

Review comment:
       Should this be `expect(request.isSecure() || cookieConfig.isSecure())`?




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] akurth commented on a change in pull request #377: Set "Secure" session cookie attribute if called under SSL.

GitBox
In reply to this post by GitBox

akurth commented on a change in pull request #377:
URL: https://github.com/apache/tomcat/pull/377#discussion_r524478033



##########
File path: test/org/apache/catalina/valves/TestLoadBalancerDrainingValve.java
##########
@@ -238,6 +238,8 @@ private void runValve(String jkActivation,
                 expectedCookie.setPath(cookieConfig.getPath());
                 expectedCookie.setMaxAge(0);
 
+                EasyMock.expect(request.isSecure()).andReturn(true);

Review comment:
       Feel free to add it. Makes no difference though, since || is short-circuit.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] martin-g commented on a change in pull request #377: Set "Secure" session cookie attribute if called under SSL.

GitBox
In reply to this post by GitBox

martin-g commented on a change in pull request #377:
URL: https://github.com/apache/tomcat/pull/377#discussion_r526015818



##########
File path: test/org/apache/catalina/valves/TestLoadBalancerDrainingValve.java
##########
@@ -238,6 +238,8 @@ private void runValve(String jkActivation,
                 expectedCookie.setPath(cookieConfig.getPath());
                 expectedCookie.setMaxAge(0);
 
+                EasyMock.expect(request.isSecure()).andReturn(true);

Review comment:
       Actually, I think we need a second test method to have coverage for the `cookieConfig.isSecure()` case. E.g. `runValve()` could have an additional boolean/enum parameter that decides whether to prepare `request.isSecure()` or `cookieConfig.isSecure()`.
   
   In addition I don't see any new code that verifies that the `expectedCookie` has `isSecure() == true`




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] markt-asf closed pull request #377: Set "Secure" session cookie attribute if called under SSL.

GitBox
In reply to this post by GitBox

markt-asf closed pull request #377:
URL: https://github.com/apache/tomcat/pull/377


   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]

Reply | Threaded
Open this post in threaded view
|

[GitHub] [tomcat] markt-asf commented on pull request #377: Set "Secure" session cookie attribute if called under SSL.

GitBox
In reply to this post by GitBox

markt-asf commented on pull request #377:
URL: https://github.com/apache/tomcat/pull/377#issuecomment-730424887


   Thanks for the PR. I have implemented a variation of this in 10.0.x and will back-port to 9.0.x and 8.5.x.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]



---------------------------------------------------------------------
To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]