Merge pull request #5548 from thc202/grpc/variant/skip-no-grpc-resp

zaproxy · Jul 1, 2024 · 3350e8c · 3350e8c
2 parents a9a8fe0 + 2f200b1
commit 3350e8c
Show file tree

Hide file tree

Showing 4 changed files with 111 additions and 6 deletions.
diff --git a/addOns/grpc/CHANGELOG.md b/addOns/grpc/CHANGELOG.md
@@ -4,7 +4,8 @@ All notable changes to this add-on will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/).
 
 ## Unreleased
-
+### Fixed
+- Do not try to decode non-gRPC responses when active scanning, which would lead to unnecessary warnings.
 
 ## [0.1.0] - 2024-06-11
 

diff --git a/addOns/grpc/grpc.gradle.kts b/addOns/grpc/grpc.gradle.kts
@@ -20,4 +20,6 @@ crowdin {
 dependencies {
     testImplementation(project(":testutils"))
     implementation("io.grpc:grpc-protobuf:1.61.1")
+
+    testImplementation(libs.log4j.core)
 }
diff --git a/addOns/grpc/src/main/java/org/zaproxy/addon/grpc/internal/VariantGrpc.java b/addOns/grpc/src/main/java/org/zaproxy/addon/grpc/internal/VariantGrpc.java
@@ -30,6 +30,8 @@
 import org.apache.logging.log4j.Logger;
 import org.parosproxy.paros.core.scanner.NameValuePair;
 import org.parosproxy.paros.core.scanner.Variant;
+import org.parosproxy.paros.network.HttpBody;
+import org.parosproxy.paros.network.HttpHeader;
 import org.parosproxy.paros.network.HttpMessage;
 
 public class VariantGrpc implements Variant {
@@ -44,7 +46,7 @@ public class VariantGrpc implements Variant {
 
     @Override
     public void setMessage(HttpMessage msg) {
-        if (isValidGrpcMessage(msg)) {
+        if (isValidGrpcMessage(msg.getRequestHeader(), msg.getRequestBody())) {
             try {
                 byte[] body = Base64.getDecoder().decode(msg.getRequestBody().getBytes());
                 byte[] payload = DecoderUtils.extractPayload(body);
@@ -92,9 +94,8 @@ private void parseContent(List<String> decodedList, String commonPrefixForNested
         }
     }
 
-    private boolean isValidGrpcMessage(HttpMessage msg) {
-        return msg.getRequestHeader().hasContentType("application/grpc")
-                && !msg.getRequestBody().toString().isEmpty();
+    private static boolean isValidGrpcMessage(HttpHeader header, HttpBody body) {
+        return header.hasContentType("application/grpc") && !body.toString().isEmpty();
     }
 
     @Override
@@ -179,6 +180,10 @@ public String setEscapedParameter(
 
     @Override
     public void decodeResponseBody(HttpMessage msg) {
+        if (!isValidGrpcMessage(msg.getResponseHeader(), msg.getResponseBody())) {
+            return;
+        }
+
         try {
             byte[] body =
                     DecoderUtils.splitMessageBodyAndStatusCode(msg.getResponseBody().getBytes());

diff --git a/addOns/grpc/src/test/java/org/zaproxy/addon/grpc/internal/VariantGrpcUnitTest.java b/addOns/grpc/src/test/java/org/zaproxy/addon/grpc/internal/VariantGrpcUnitTest.java
@@ -19,10 +19,28 @@
  */
 package org.zaproxy.addon.grpc.internal;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.not;
+import static org.hamcrest.Matchers.startsWith;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
+import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.function.Consumer;
+import org.apache.logging.log4j.Level;
+import org.apache.logging.log4j.core.LogEvent;
+import org.apache.logging.log4j.core.LoggerContext;
+import org.apache.logging.log4j.core.StringLayout;
+import org.apache.logging.log4j.core.appender.AbstractAppender;
+import org.apache.logging.log4j.core.config.Configurator;
+import org.apache.logging.log4j.core.config.LoggerConfig;
+import org.apache.logging.log4j.core.config.Property;
+import org.apache.logging.log4j.core.filter.BurstFilter;
+import org.apache.logging.log4j.core.layout.PatternLayout;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.parosproxy.paros.core.scanner.NameValuePair;
@@ -32,11 +50,33 @@
 import org.parosproxy.paros.network.HttpRequestHeader;
 
 class VariantGrpcUnitTest {
+
+    private static final String LOG_MSG_ERROR_DECODING_RESPONSE =
+            "Error decoding the Response Body";
+    private static final String APPENDER_NAME = "ZAP-TestAppender";
+    private static Level originalLevel;
+    private static List<String> logMessages;
+
     private VariantGrpc variantGrpc;
 
     @BeforeEach
     void setUp() {
         variantGrpc = new VariantGrpc();
+        logMessages = new ArrayList<>();
+    }
+
+    @BeforeAll
+    static void initAppender() {
+        LoggerConfig rootLogger = LoggerContext.getContext().getConfiguration().getRootLogger();
+        rootLogger.addAppender(new TestAppender(VariantGrpcUnitTest::handleLog), null, null);
+        originalLevel = rootLogger.getLevel();
+        Configurator.setRootLevel(Level.ALL);
+    }
+
+    @AfterAll
+    static void removeAppender() {
+        LoggerContext.getContext().getConfiguration().getRootLogger().removeAppender(APPENDER_NAME);
+        Configurator.setRootLevel(originalLevel);
     }
 
     @Test
@@ -152,13 +192,70 @@ void shouldExtractParametersFromCorruptedGrpcMessage() throws HttpMalformedHeade
         assertEquals(expectedParamList, variantGrpc.getParamList());
     }
 
+    @Test
+    void shouldNotWarnWhenDecodingResponseOfNonGrpcMessage() {
+        // Given
+        HttpMessage httpMessage = new HttpMessage();
+        // When
+        variantGrpc.decodeResponseBody(httpMessage);
+        // Then
+        assertThat(logMessages, not(hasItem(startsWith(LOG_MSG_ERROR_DECODING_RESPONSE))));
+    }
+
+    @Test
+    void shouldWarnWhenFailedToDecodeMalformedResponseGrpcMessage() {
+        // Given
+        HttpMessage httpMessage = new HttpMessage();
+        setGrpcHeader(httpMessage.getResponseHeader());
+        httpMessage.getResponseBody().setBody("something not gRPC");
+        // When
+        variantGrpc.decodeResponseBody(httpMessage);
+        // Then
+        assertThat(logMessages, hasItem(startsWith(LOG_MSG_ERROR_DECODING_RESPONSE)));
+    }
+
+    private static void handleLog(String message) {
+        logMessages.add(message);
+    }
+
     private static HttpMessage createHttpMessage(String encodedRequestBody)
             throws HttpMalformedHeaderException {
         HttpRequestHeader httpRequestHeader = new HttpRequestHeader();
         httpRequestHeader.setMessage("POST /abc/xyz HTTP/1.1");
-        httpRequestHeader.setHeader(HttpHeader.CONTENT_TYPE, "application/grpc-web-text");
+        setGrpcHeader(httpRequestHeader);
         HttpMessage httpMessage = new HttpMessage(httpRequestHeader);
         httpMessage.setRequestBody(encodedRequestBody);
         return httpMessage;
     }
+
+    private static void setGrpcHeader(HttpHeader header) {
+        header.setHeader(HttpHeader.CONTENT_TYPE, "application/grpc-web-text");
+    }
+
+    static class TestAppender extends AbstractAppender {
+
+        private static final Property[] NO_PROPERTIES = {};
+
+        private final Consumer<String> logConsumer;
+
+        TestAppender(Consumer<String> logConsumer) {
+            super(
+                    APPENDER_NAME,
+                    BurstFilter.newBuilder().setMaxBurst(100).setLevel(Level.WARN).build(),
+                    PatternLayout.newBuilder()
+                            .withDisableAnsi(true)
+                            .withCharset(StandardCharsets.UTF_8)
+                            .withPattern("%m%n")
+                            .build(),
+                    true,
+                    NO_PROPERTIES);
+            this.logConsumer = logConsumer;
+            start();
+        }
+
+        @Override
+        public void append(LogEvent event) {
+            logConsumer.accept(((StringLayout) getLayout()).toSerializable(event));
+        }
+    }
 }