Merge pull request #5616 from psiinon/auth/sessfix

zaproxy · Jul 30, 2024 · 12103e4 · 12103e4
2 parents 89368ca + d8cb6b4
commit 12103e4
Show file tree

Hide file tree

Showing 5 changed files with 51 additions and 19 deletions.
diff --git a/addOns/authhelper/CHANGELOG.md b/addOns/authhelper/CHANGELOG.md
@@ -6,6 +6,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 ## Unreleased
 ### Fixed
 - Potential timing issue trying to use browser based auth to authenticate before the session management method has been identified.
+- Timing issue with session management detection.
 
 ## [0.13.0] - 2024-05-07
 ### Changed

diff --git a/addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/AuthUtils.java b/addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/AuthUtils.java
@@ -488,7 +488,8 @@ protected static void addToMap(Map<String, SessionToken> map, SessionToken token
         map.put(token.getToken(), token);
     }
 
-    protected static Map<String, SessionToken> getAllTokens(HttpMessage msg) {
+    protected static Map<String, SessionToken> getAllTokens(
+            HttpMessage msg, boolean incReqCookies) {
         Map<String, SessionToken> tokens = new HashMap<>();
         String responseData = msg.getResponseBody().toString();
         if (msg.getResponseHeader().isJson() && StringUtils.isNotBlank(responseData)) {
@@ -529,16 +530,18 @@ protected static Map<String, SessionToken> getAllTokens(HttpMessage msg) {
                                                 p.getName(),
                                                 p.getValue())));
         // Add Cookies
-        msg.getRequestHeader()
-                .getCookieParams()
-                .forEach(
-                        c ->
-                                addToMap(
-                                        tokens,
-                                        new SessionToken(
-                                                SessionToken.COOKIE_SOURCE,
-                                                c.getName(),
-                                                c.getValue())));
+        if (incReqCookies) {
+            msg.getRequestHeader()
+                    .getCookieParams()
+                    .forEach(
+                            c ->
+                                    addToMap(
+                                            tokens,
+                                            new SessionToken(
+                                                    SessionToken.COOKIE_SOURCE,
+                                                    c.getName(),
+                                                    c.getValue())));
+        }
         msg.getResponseHeader()
                 .getHttpCookies(null)
                 .forEach(
@@ -597,7 +600,7 @@ static SessionManagementRequestDetails findSessionTokenSource(String token, int
                 try {
                     HttpMessage msg = hr.getHttpMessage();
                     Optional<SessionToken> es =
-                            AuthUtils.getAllTokens(msg).values().stream()
+                            AuthUtils.getAllTokens(msg, false).values().stream()
                                     .filter(v -> v.getValue().equals(token))
                                     .findFirst();
                     if (es.isPresent()) {

diff --git a/...er/src/main/java/org/zaproxy/addon/authhelper/HeaderBasedSessionManagementMethodType.java b/...er/src/main/java/org/zaproxy/addon/authhelper/HeaderBasedSessionManagementMethodType.java
@@ -128,7 +128,7 @@ protected static void replaceEnvVarsForTesting(Map<String, String> vars) {
 
         @Override
         public HttpHeaderBasedSession extractWebSession(HttpMessage msg) {
-            Map<String, SessionToken> tokens = AuthUtils.getAllTokens(msg);
+            Map<String, SessionToken> tokens = AuthUtils.getAllTokens(msg, true);
             LOGGER.debug(
                     "extractWebSession {} # tokens {}",
                     msg.getRequestHeader().getURI(),

diff --git a/addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/SessionDetectionScanRule.java b/addOns/authhelper/src/main/java/org/zaproxy/addon/authhelper/SessionDetectionScanRule.java
@@ -105,6 +105,11 @@ public void scanHttpResponseReceive(HttpMessage msg, int id, Source source) {
                         // Yes, found the token in a 'non standard' place
                         this.getTaskHelper()
                                 .raiseAlert(smrd.getMsg().getHistoryRef(), getAlert(smrd).build());
+                        LOGGER.debug(
+                                "Found {} 'unknown' response session token(s) in {}",
+                                responseTokens.size(),
+                                msg.getRequestHeader().getURI());
+
                         Stats.incCounter("stats.auth.detect.session." + st.getKey());
                         foundTokens.addAll(smrd.getTokens());
                     }

diff --git a/addOns/authhelper/src/test/java/org/zaproxy/addon/authhelper/AuthUtilsUnitTest.java b/addOns/authhelper/src/test/java/org/zaproxy/addon/authhelper/AuthUtilsUnitTest.java
@@ -279,7 +279,7 @@ void shouldExtractHeaderTokens() throws Exception {
                                         + "Header2: Value2\r\n"),
                         new HttpResponseBody("Response Body"));
         // When
-        Map<String, SessionToken> tokens = AuthUtils.getAllTokens(msg);
+        Map<String, SessionToken> tokens = AuthUtils.getAllTokens(msg, false);
 
         // Then
         assertThat(tokens.size(), is(equalTo(2)));
@@ -298,7 +298,7 @@ void shouldExtractUrlParams() throws Exception {
                         new HttpResponseHeader("HTTP/1.1 200 OK\r\n"),
                         new HttpResponseBody("Response Body"));
         // When
-        Map<String, SessionToken> tokens = AuthUtils.getAllTokens(msg);
+        Map<String, SessionToken> tokens = AuthUtils.getAllTokens(msg, false);
 
         // Then
         assertThat(tokens.size(), is(equalTo(2)));
@@ -328,7 +328,7 @@ void shouldExtractJsonTokens() throws Exception {
                                         + "  }\n"
                                         + "}}"));
         // When
-        Map<String, SessionToken> tokens = AuthUtils.getAllTokens(msg);
+        Map<String, SessionToken> tokens = AuthUtils.getAllTokens(msg, false);
 
         // Then
         assertThat(tokens.size(), is(equalTo(8)));
@@ -347,7 +347,7 @@ void shouldExtractJsonTokens() throws Exception {
     }
 
     @Test
-    void shouldExtractCookies() throws Exception {
+    void shouldExtractAllCookies() throws Exception {
         // Given
         HttpMessage msg =
                 new HttpMessage(
@@ -360,10 +360,9 @@ void shouldExtractCookies() throws Exception {
                                 "HTTP/1.1 200 OK\r\n" + "Set-Cookie: ccc=ddd; HttpOnly; Secure"),
                         new HttpResponseBody("Response Body"));
         // When
-        Map<String, SessionToken> tokens = AuthUtils.getAllTokens(msg);
+        Map<String, SessionToken> tokens = AuthUtils.getAllTokens(msg, true);
 
         // Then
-        System.out.println(tokens);
         assertThat(tokens.size(), is(equalTo(3)));
         assertThat(tokens.get("cookie:aaa").getValue(), is(equalTo("bbb")));
         assertThat(tokens.get("cookie:ccc").getValue(), is(equalTo("ddd")));
@@ -372,6 +371,30 @@ void shouldExtractCookies() throws Exception {
                 is(equalTo("ccc=ddd; HttpOnly; Secure")));
     }
 
+    @Test
+    void shouldExtractResponseCookies() throws Exception {
+        // Given
+        HttpMessage msg =
+                new HttpMessage(
+                        new HttpRequestHeader(
+                                "GET https://example.com/ HTTP/1.1\r\n"
+                                        + "Host: example.com\r\n"
+                                        + "Cookie: aaa=bbb\r\n\r\n"),
+                        new HttpRequestBody("Request Body"),
+                        new HttpResponseHeader(
+                                "HTTP/1.1 200 OK\r\n" + "Set-Cookie: ccc=ddd; HttpOnly; Secure"),
+                        new HttpResponseBody("Response Body"));
+        // When
+        Map<String, SessionToken> tokens = AuthUtils.getAllTokens(msg, false);
+
+        // Then
+        assertThat(tokens.size(), is(equalTo(2)));
+        assertThat(tokens.get("cookie:ccc").getValue(), is(equalTo("ddd")));
+        assertThat(
+                tokens.get("header:Set-Cookie").getValue(),
+                is(equalTo("ccc=ddd; HttpOnly; Secure")));
+    }
+
     @Test
     void shouldGetEmptyHeaderTokens() throws Exception {
         // Given