feat: Improve Artifactory handler log message (#7838)

Author: aikebah

core/src/main/java/org/owasp/dependencycheck/data/artifactory/ArtifactorySearch.java

@@ -52,6 +52,13 @@
 @ThreadSafe
 public class ArtifactorySearch {
 
+    /**
+     * Required to add all extra information of the found artifact.
+     * Source: https://jfrog.com/help/r/jfrog-rest-apis/property-search
+     */
+    @SuppressWarnings("JavadocLinkAsPlainText")
+    public static final String X_RESULT_DETAIL_HEADER = "X-Result-Detail";
+
     /**
      * Used for logging.
      */
@@ -108,9 +115,13 @@ public List<MavenArtifact> search(Dependency dependency) throws IOException {
         final StringBuilder msg = new StringBuilder("Could not connect to Artifactory at")
                 .append(url);
         try {
-            final BasicHeader artifactoryResultDetail = new BasicHeader("X-Result-Detail", "info");
-            return Downloader.getInstance().fetchAndHandle(url, new ArtifactorySearchResponseHandler(dependency), List.of(artifactoryResultDetail),
-                    allowUsingProxy);
+            final BasicHeader artifactoryResultDetail = new BasicHeader(X_RESULT_DETAIL_HEADER, "info");
+            return Downloader.getInstance().fetchAndHandle(
+                    url,
+                    new ArtifactorySearchResponseHandler(url, dependency),
+                    List.of(artifactoryResultDetail),
+                    allowUsingProxy
+            );
         } catch (TooManyRequestsException e) {
             throw new IOException(msg.append(" (429): Too manu requests").toString(), e);
         } catch (URISyntaxException e) {

core/src/main/java/org/owasp/dependencycheck/data/artifactory/ArtifactorySearchResponseHandler.java

@@ -32,13 +32,16 @@
 import java.io.FileNotFoundException;
 import java.io.IOException;
 import java.io.InputStreamReader;
+import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Optional;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
+import static org.owasp.dependencycheck.data.artifactory.ArtifactorySearch.X_RESULT_DETAIL_HEADER;
+
 class ArtifactorySearchResponseHandler implements HttpClientResponseHandler<List<MavenArtifact>> {
     /**
      * Pattern to match the path returned by the Artifactory AQL API.
@@ -61,13 +64,20 @@ class ArtifactorySearchResponseHandler implements HttpClientResponseHandler<List
     private final Dependency expectedDependency;
 
     /**
-     * Creates a responsehandler for the response on a single dependency-search attempt.
+     * The search request URL i.e., the location at which to search artifacts with checksum information
+     */
+    private final URL sourceUrl;
+
+    /**
+     * Creates a response handler for the response on a single dependency-search attempt.
      *
+     * @param sourceUrl The search request URL
      * @param dependency The dependency that is expected to be in the response when found (for validating the FileItem(s) in the response)
      */
-    ArtifactorySearchResponseHandler(Dependency dependency) {
+    ArtifactorySearchResponseHandler(URL sourceUrl, Dependency dependency) {
         this.fileImplReader = new ObjectMapper().configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false).readerFor(FileImpl.class);
         this.expectedDependency = dependency;
+        this.sourceUrl = sourceUrl;
     }
 
     protected boolean init(JsonParser parser) throws IOException {
@@ -114,7 +124,7 @@ private boolean checkHashes(ChecksumsImpl checksums) {
             match = false;
         }
         final String sha256sum = expectedDependency.getSha256sum();
-        /* For sha256 we need to validate that the checksum is non-null in the artifactory response.
+        /* For SHA-256, we need to validate that the checksum is non-null in the artifactory response.
          * Extract from Artifactory documentation:
          * New artifacts that are uploaded to Artifactory 5.5 and later will automatically have their SHA-256 checksum calculated.
          * However, artifacts that were already hosted in Artifactory before the upgrade will not have their SHA-256 checksum in the database yet.
@@ -132,7 +142,7 @@ private boolean checkHashes(ChecksumsImpl checksums) {
      * Process the Artifactory response.
      *
      * @param response the HTTP response
-     * @return a list of the Maven Artifact informations that match the searched dependency hash
+     * @return a list of the Maven Artifact information that matches the searched dependency hash
      * @throws FileNotFoundException When a matching artifact is not found
      * @throws IOException           thrown if there is an I/O error
      */
@@ -149,8 +159,11 @@ public List<MavenArtifact> handleResponse(ClassicHttpResponse response) throws I
                     final FileImpl file = fileImplReader.readValue(parser);
 
                     if (file.getChecksums() == null) {
-                        LOGGER.warn("No checksums found in artifactory search result of uri {}. Please make sure that header X-Result-Detail is retained on any (reverse)-proxy, loadbalancer or WebApplicationFirewall in the network path to your Artifactory Server",
-                                file.getUri());
+                        LOGGER.warn(
+                                "No checksums found in Artifactory search result for '{}'. " +
+                                "Specifically, the result set contains URI '{}' but it is missing the 'checksums' property. " +
+                                "Please make sure that the '{}' header is retained on any (reverse-)proxy, load-balancer or Web Application Firewall in the network path to your Artifactory server.",
+                                sourceUrl, file.getUri(), X_RESULT_DETAIL_HEADER);
                         continue;
                     }
 
@@ -174,18 +187,18 @@ public List<MavenArtifact> handleResponse(ClassicHttpResponse response) throws I
         }
         if (result.isEmpty()) {
             throw new FileNotFoundException("Artifact " + expectedDependency
-                    + " not found in Artifactory; discovered sha1 hits not recognized as matching maven artifacts");
+                    + " not found in Artifactory; discovered SHA1 hits not recognized as matching Maven artifacts");
         }
         return result;
     }
 
     /**
      * Validate the FileImpl result for usability as a dependency.
      * <br/>
-     * Checks that the actually matches all known hashes and the path appears to match a maven repository G/A/V pattern.
+     * Checks that the file actually matches all known hashes and the path appears to match a maven repository G/A/V pattern.
      *
      * @param file The FileImpl from an Artifactory search response
-     * @return An Optional with the Matcher for the file path to retrieve the Maven G/A/V coordinates in case result is usable for further
+     * @return An Optional with the Matcher for the file path to retrieve the Maven G/A/V coordinates in case the result is usable for further
      * processing, otherwise an empty Optional.
      */
     private Optional<Matcher> validateUsability(FileImpl file) {

core/src/test/java/org/owasp/dependencycheck/data/artifactory/ArtifactorySearchResponseHandlerTest.java

@@ -33,6 +33,8 @@
 import java.io.ByteArrayInputStream;
 import java.io.FileNotFoundException;
 import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.util.List;
 
@@ -41,8 +43,20 @@
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
+@SuppressWarnings("resource")
 class ArtifactorySearchResponseHandlerTest extends BaseTest {
 
+    private static final URL TEST_URL;
+    private static final String EXCEPTION_MESSAGE = "Artifact Dependency{ fileName='null', actualFilePath='null', filePath='null', packagePath='null'} not found in Artifactory; discovered SHA1 hits not recognized as matching Maven artifacts";
+
+    static {
+        try {
+            TEST_URL = new URL("https://example.com/artifactory/api/search/checksum?sha1=43515aa4b2f4bce7c431145e8c0a7bcc441e0532");
+        } catch (MalformedURLException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
     @BeforeEach
     @Override
     public void setUp() throws Exception {
@@ -88,7 +102,7 @@ void shouldProcessCorrectlyArtifactoryAnswerWithoutSha256() throws IOException {
 
 
         // When
-        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(dependency);
+        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(TEST_URL, dependency);
         final List<MavenArtifact> mavenArtifacts = handler.handleResponse(response);
 
         // Then
@@ -118,7 +132,7 @@ void shouldProcessCorrectlyArtifactoryAnswerWithMultipleMatches() throws IOExcep
 
 
         // When
-        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(dependency);
+        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(TEST_URL, dependency);
         final List<MavenArtifact> mavenArtifacts = handler.handleResponse(response);
 
         // Then
@@ -152,6 +166,7 @@ void shouldProcessCorrectlyForMissingXResultDetailHeader() throws IOException {
         final ListAppender<ILoggingEvent> listAppender = new ListAppender<>();
         listAppender.start();
         sutLogger.addAppender(listAppender);
+        final String logMessage = "No checksums found in Artifactory search result for '{}'. Specifically, the result set contains URI '{}' but it is missing the 'checksums' property. Please make sure that the '{}' header is retained on any (reverse-)proxy, load-balancer or Web Application Firewall in the network path to your Artifactory server.";
 
         // Given
         final Dependency dependency = new Dependency();
@@ -167,32 +182,32 @@ void shouldProcessCorrectlyForMissingXResultDetailHeader() throws IOException {
 
 
         // When
-        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(dependency);
+        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(TEST_URL, dependency);
 
         FileNotFoundException e = assertThrows(FileNotFoundException.class, () -> handler.handleResponse(response),
                 "Result with no details due to missing X-Result-Detail header, should throw an exception!");
 
         // Then
-        assertEquals("Artifact Dependency{ fileName='freemarker-2.3.33.jar', actualFilePath='null', filePath='null', packagePath='null'} not found in Artifactory; discovered sha1 hits not recognized as matching maven artifacts",
+        assertEquals("Artifact Dependency{ fileName='freemarker-2.3.33.jar', actualFilePath='null', filePath='null', packagePath='null'} not found in Artifactory; discovered SHA1 hits not recognized as matching Maven artifacts",
                 e.getMessage());
 
-        // There should be a WARN-log for for each of the results regarding the absence of X-Result-Detail header driven attributes
+        // There should be a WARN-log for each of the results regarding the absence of X-Result-Detail header driven attributes
         final List<ILoggingEvent> logsList = listAppender.list;
         assertEquals(2, logsList.size(), "Number of log entries for the ArtifactorySearchResponseHandler");
 
         ILoggingEvent logEvent = logsList.get(0);
         assertEquals(Level.WARN, logEvent.getLevel());
-        assertEquals("No checksums found in artifactory search result of uri {}. Please make sure that header X-Result-Detail is retained on any (reverse)-proxy, loadbalancer or WebApplicationFirewall in the network path to your Artifactory Server", logEvent.getMessage());
+        assertEquals(logMessage, logEvent.getMessage());
         Object[] args = logEvent.getArgumentArray();
-        assertEquals(1, args.length);
-        assertEquals("https://artifactory.example.com:443/artifactory/api/storage/maven-central-cache/org/freemarker/freemarker/2.3.33/freemarker-2.3.33.jar", args[0]);
+        assertEquals(3, args.length);
+        assertEquals("https://artifactory.example.com:443/artifactory/api/storage/maven-central-cache/org/freemarker/freemarker/2.3.33/freemarker-2.3.33.jar", args[1]);
 
         logEvent = logsList.get(1);
         assertEquals(Level.WARN, logEvent.getLevel());
-        assertEquals("No checksums found in artifactory search result of uri {}. Please make sure that header X-Result-Detail is retained on any (reverse)-proxy, loadbalancer or WebApplicationFirewall in the network path to your Artifactory Server", logEvent.getMessage());
+        assertEquals(logMessage, logEvent.getMessage());
         args = logEvent.getArgumentArray();
-        assertEquals(1, args.length);
-        assertEquals("https://artifactory.example.com:443/artifactory/api/storage/gradle-plugins-extended-cache/org/freemarker/freemarker/2.3.33/freemarker-2.3.33.jar", args[0]);
+        assertEquals(3, args.length);
+        assertEquals("https://artifactory.example.com:443/artifactory/api/storage/gradle-plugins-extended-cache/org/freemarker/freemarker/2.3.33/freemarker-2.3.33.jar", args[1]);
 
         // Remove our manually injected additional appender
         sutLogger.detachAppender(listAppender);
@@ -212,7 +227,7 @@ void shouldHandleNoMatches() throws IOException {
         when(responseEntity.getContent()).thenReturn(new ByteArrayInputStream(payload));
 
         // When
-        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(dependency);
+        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(TEST_URL, dependency);
         FileNotFoundException e = assertThrows(FileNotFoundException.class, () -> handler.handleResponse(response),
                 "No Match found, should throw an exception!");
         // Then
@@ -298,7 +313,7 @@ void shouldProcessCorrectlyArtifactoryAnswer() throws IOException {
         when(responseEntity.getContent()).thenReturn(new ByteArrayInputStream(payload));
 
         // When
-        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(dependency);
+        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(TEST_URL, dependency);
         final List<MavenArtifact> mavenArtifacts = handler.handleResponse(response);
 
         // Then
@@ -419,13 +434,13 @@ void shouldProcessCorrectlyArtifactoryAnswerMisMatchMd5() throws IOException {
         when(responseEntity.getContent()).thenReturn(new ByteArrayInputStream(payload));
 
         // When
-        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(dependency);
+        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(TEST_URL, dependency);
         FileNotFoundException e = assertThrows(FileNotFoundException.class, () -> handler.handleResponse(response),
                 "MD5 mismatching should throw an exception!");
 
         // Then
         assertEquals("Artifact " + dependency
-                + " not found in Artifactory; discovered sha1 hits not recognized as matching maven artifacts", e.getMessage());
+                + " not found in Artifactory; discovered SHA1 hits not recognized as matching Maven artifacts", e.getMessage());
     }
 
     @Test
@@ -442,12 +457,12 @@ void shouldProcessCorrectlyArtifactoryAnswerMisMatchSha1() throws IOException {
         when(responseEntity.getContent()).thenReturn(new ByteArrayInputStream(payload));
 
         // When
-        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(dependency);
+        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(TEST_URL, dependency);
         FileNotFoundException e = assertThrows(FileNotFoundException.class, () -> handler.handleResponse(response),
                 "SHA1 mismatching should throw an exception!");
 
         // Then
-        assertEquals("Artifact Dependency{ fileName='null', actualFilePath='null', filePath='null', packagePath='null'} not found in Artifactory; discovered sha1 hits not recognized as matching maven artifacts", e.getMessage());
+        assertEquals(EXCEPTION_MESSAGE, e.getMessage());
     }
 
     @Test
@@ -464,12 +479,12 @@ void shouldProcessCorrectlyArtifactoryAnswerMisMatchSha256() throws IOException
         when(responseEntity.getContent()).thenReturn(new ByteArrayInputStream(payload));
 
         // When
-        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(dependency);
+        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(TEST_URL, dependency);
         FileNotFoundException e = assertThrows(FileNotFoundException.class, () -> handler.handleResponse(response),
                 "SHA256 mismatching should throw an exception!");
 
         // Then
-        assertEquals("Artifact Dependency{ fileName='null', actualFilePath='null', filePath='null', packagePath='null'} not found in Artifactory; discovered sha1 hits not recognized as matching maven artifacts", e.getMessage());
+        assertEquals(EXCEPTION_MESSAGE, e.getMessage());
     }
 
     @Test
@@ -487,12 +502,12 @@ void shouldThrowNotFoundWhenPatternCannotBeParsed() throws IOException {
         when(responseEntity.getContent()).thenReturn(new ByteArrayInputStream(payload));
 
         // When
-        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(dependency);
+        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(TEST_URL, dependency);
         FileNotFoundException e = assertThrows(FileNotFoundException.class, () -> handler.handleResponse(response),
                 "Maven GAV pattern mismatch for filepath should throw a not found exception!");
 
             // Then
-            assertEquals("Artifact Dependency{ fileName='null', actualFilePath='null', filePath='null', packagePath='null'} not found in Artifactory; discovered sha1 hits not recognized as matching maven artifacts", e.getMessage());
+            assertEquals(EXCEPTION_MESSAGE, e.getMessage());
     }
 
     @Test
@@ -509,7 +524,7 @@ void shouldSkipResultsWherePatternCannotBeParsed() throws IOException {
         when(responseEntity.getContent()).thenReturn(new ByteArrayInputStream(payload));
 
         // When
-        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(dependency);
+        final ArtifactorySearchResponseHandler handler = new ArtifactorySearchResponseHandler(TEST_URL, dependency);
         List<MavenArtifact> result = handler.handleResponse(response);
         // Then
         assertEquals(1, result.size());