From 3c3534e7da239bb21d72d4c5fa8626774266ad95 Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Mon, 2 Oct 2017 17:01:16 +0200 Subject: [PATCH 01/11] CentralAnalyzer: Implement retry for fetching MavenArtifacts due to sporadic issues --- .../analyzer/CentralAnalyzer.java | 58 +++++++- .../data/central/CentralSearch.java | 10 +- .../analyzer/CentralAnalyzerTest.java | 127 ++++++++++++++++++ 3 files changed, 187 insertions(+), 8 deletions(-) create mode 100644 dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java index 55d267832..638693d88 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java @@ -74,6 +74,13 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { */ private static final String SUPPORTED_EXTENSIONS = "jar"; + /** + * There may be temporary issues when connecting to MavenCentral. + * In order to compensate for 99% of the issues, we perform a retry + * before finally raising the {@link #errorFlag}. + */ + private static final int NUMBER_OF_TRIES = 5; + /** * The analyzer should be disabled if there are errors, so this is a flag to * determine if such an error has occurred. @@ -83,7 +90,8 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { /** * The searcher itself. */ - private CentralSearch searcher; + CentralSearch searcher; + /** * Field indicating if the analyzer is enabled. */ @@ -212,7 +220,7 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { } try { - final List mas = searcher.searchSha1(dependency.getSha1sum()); + final List mas = fetchMavenArtifacts(dependency); final Confidence confidence = mas.size() > 1 ? Confidence.HIGH : Confidence.HIGHEST; for (MavenArtifact ma : mas) { LOGGER.debug("Central analyzer found artifact ({}) for dependency ({})", ma, dependency.getFileName()); @@ -256,8 +264,52 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { } catch (FileNotFoundException fnfe) { LOGGER.debug("Artifact not found in repository: '{}", dependency.getFileName()); } catch (IOException ioe) { - LOGGER.debug("Could not connect to Central search", ioe); + LOGGER.warn("Could not connect to Central search. Disabling this analyzer.", ioe); errorFlag = true; } } + + /** + * Downloads the corresponding list of MavenArtifacts of the given + * dependency from MavenCentral. + *

+ * As the connection to MavenCentral is known to be unreliable, we implement + * a simple retry logic in order to compensate for 99% of the issues. + * + * @param dependency the dependency to analyze + * @return the downloaded list of MavenArtifacts + * @throws FileNotFoundException if the specified artifact is not found + * @throws IOException if connecting to MavenCentral finally failed + */ + List fetchMavenArtifacts(Dependency dependency) throws IOException { + IOException lastException = null; + long sleepingTimeBetweenRetriesInMillis = 1000; + int triesLeft = NUMBER_OF_TRIES; + while (triesLeft-- > 0) { + try { + return searcher.searchSha1(dependency.getSha1sum()); + } catch (FileNotFoundException fnfe) { + // retry does not make sense, just throw the exception + throw fnfe; + } catch (IOException ioe) { + LOGGER.debug("Could not connect to Central search (tries left: {}): {}", + triesLeft, ioe.getMessage()); + lastException = ioe; + + if (triesLeft > 0) { + try { + Thread.sleep(sleepingTimeBetweenRetriesInMillis); + sleepingTimeBetweenRetriesInMillis *= 2; + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + } + } + } + + LOGGER.warn("Finally failed connecting to Central search." + + " Giving up after {} tries. Last exception was: {}", + NUMBER_OF_TRIES, lastException); + throw lastException; + } } diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java index 8466741ef..5c1d46557 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java @@ -101,8 +101,9 @@ public class CentralSearch { * * @param sha1 the SHA-1 hash string for which to search * @return the populated Maven GAV. - * @throws IOException if it's unable to connect to the specified repository - * or if the specified artifact is not found. + * @throws FileNotFoundException if the specified artifact is not found + * @throws IOException if it's unable to connect to the specified + * repository */ public List searchSha1(String sha1) throws IOException { if (null == sha1 || !sha1.matches("^[0-9A-Fa-f]{40}$")) { @@ -178,9 +179,8 @@ public class CentralSearch { throw new FileNotFoundException("Artifact not found in Central"); } } else { - LOGGER.debug("Could not connect to Central received response code: {} {}", - conn.getResponseCode(), conn.getResponseMessage()); - throw new IOException("Could not connect to Central"); + String errorMessage = "Could not connect to MavenCentral (" + conn.getResponseCode() + "): " + conn.getResponseMessage(); + throw new IOException(errorMessage); } return result; } diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java new file mode 100644 index 000000000..e7a129a06 --- /dev/null +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java @@ -0,0 +1,127 @@ +package org.owasp.dependencycheck.analyzer; + +import mockit.Expectations; +import mockit.Mock; +import mockit.MockUp; +import mockit.Mocked; +import org.junit.BeforeClass; +import org.junit.Test; +import org.owasp.dependencycheck.data.central.CentralSearch; +import org.owasp.dependencycheck.data.nexus.MavenArtifact; +import org.owasp.dependencycheck.dependency.Dependency; + +import java.io.FileNotFoundException; +import java.io.IOException; +import java.util.Collections; +import java.util.List; + +import static org.junit.Assert.assertEquals; + +/** + * Tests for the CentralAnalyzer. + */ +public class CentralAnalyzerTest { + + private static final String SHA1_SUM = "my-sha1-sum"; + + @BeforeClass + public static void beforeClass() { + doNotSleepBetweenRetries(); + } + + @Test + public void testFetchMavenArtifactsWithoutException(@Mocked final CentralSearch centralSearch, + @Mocked final Dependency dependency) + throws IOException { + + CentralAnalyzer instance = new CentralAnalyzer(); + instance.searcher = centralSearch; + + final List expectedMavenArtifacts = Collections.emptyList(); + new Expectations() {{ + dependency.getSha1sum(); + returns(SHA1_SUM); + + centralSearch.searchSha1(SHA1_SUM); + returns(expectedMavenArtifacts); + }}; + + final List actualMavenArtifacts = instance.fetchMavenArtifacts(dependency); + + assertEquals(expectedMavenArtifacts, actualMavenArtifacts); + } + + @Test + public void testFetchMavenArtifactsWithSporadicIOException(@Mocked final CentralSearch centralSearch, + @Mocked final Dependency dependency) + throws IOException { + + CentralAnalyzer instance = new CentralAnalyzer(); + instance.searcher = centralSearch; + + final List expectedMavenArtifacts = Collections.emptyList(); + new Expectations() {{ + dependency.getSha1sum(); + returns(SHA1_SUM); + + centralSearch.searchSha1(SHA1_SUM); + result = new IOException("Could not connect to MavenCentral (500): Internal Server Error"); + result = new IOException("Could not connect to MavenCentral (500): Internal Server Error"); + result = expectedMavenArtifacts; + }}; + + final List actualMavenArtifacts = instance.fetchMavenArtifacts(dependency); + + assertEquals(expectedMavenArtifacts, actualMavenArtifacts); + } + + @Test(expected = FileNotFoundException.class) + public void testFetchMavenArtifactsRethrowsFileNotFoundException(@Mocked final CentralSearch centralSearch, + @Mocked final Dependency dependency) + throws IOException { + + CentralAnalyzer instance = new CentralAnalyzer(); + instance.searcher = centralSearch; + + new Expectations() {{ + dependency.getSha1sum(); + returns(SHA1_SUM); + + centralSearch.searchSha1(SHA1_SUM); + result = new FileNotFoundException("Artifact not found in Central"); + }}; + + instance.fetchMavenArtifacts(dependency); + } + + @Test(expected = IOException.class) + public void testFetchMavenArtifactsAlwaysThrowsIOException(@Mocked final CentralSearch centralSearch, + @Mocked final Dependency dependency) + throws IOException { + + CentralAnalyzer instance = new CentralAnalyzer(); + instance.searcher = centralSearch; + + new Expectations() {{ + dependency.getSha1sum(); + returns(SHA1_SUM); + + centralSearch.searchSha1(SHA1_SUM); + result = new IOException("no internet connection"); + }}; + + instance.fetchMavenArtifacts(dependency); + } + + /** + * We do not want to waste time in unit tests. + */ + private static void doNotSleepBetweenRetries() { + new MockUp() { + @Mock + void sleep(long millis) { + // do not sleep + } + }; + } +} From 1835355f4dc141ef0e35eded6dd63459bfc02f0d Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Mon, 2 Oct 2017 17:02:38 +0200 Subject: [PATCH 02/11] Cleanup: formatting and typo --- .../org/owasp/dependencycheck/analyzer/CentralAnalyzer.java | 2 +- .../org/owasp/dependencycheck/data/central/CentralSearch.java | 2 +- .../java/org/owasp/dependencycheck/dependency/Dependency.java | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java index 638693d88..bf3596067 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java @@ -210,7 +210,7 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { * Performs the analysis. * * @param dependency the dependency to analyze - * @param engine the engine + * @param engine the engine * @throws AnalysisException when there's an exception during analysis */ @Override diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java index 5c1d46557..0db54763b 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/data/central/CentralSearch.java @@ -74,7 +74,7 @@ public class CentralSearch { * * @param settings the configured settings * @throws MalformedURLException thrown if the configured URL is - * invalid + * invalid */ public CentralSearch(Settings settings) throws MalformedURLException { this.settings = settings; diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/dependency/Dependency.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/dependency/Dependency.java index 83e114ed0..0165e4649 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/dependency/Dependency.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/dependency/Dependency.java @@ -148,7 +148,7 @@ public class Dependency extends EvidenceCollection implements Serializable, Comp * Constructs a new Dependency object. */ public Dependency() { - //empty contructor + //empty constructor } /** From 142eb41312980803e2a33e76cf8d8f69911b5b5b Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Mon, 2 Oct 2017 17:05:55 +0200 Subject: [PATCH 03/11] Cleanup: remove useless overwrite As the default is "true" again, we do not need to overwrite it here. And even if we changed the default back to "false", then there would probably be a good reason why we would not want to overwrite it to "true" for this specific analyzer. --- .../dependencycheck/analyzer/ArchiveAnalyzer.java | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/ArchiveAnalyzer.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/ArchiveAnalyzer.java index d542171e6..c1e15538c 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/ArchiveAnalyzer.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/ArchiveAnalyzer.java @@ -217,20 +217,6 @@ public class ArchiveAnalyzer extends AbstractFileTypeAnalyzer { } } - /** - * Does not support parallel processing as it both modifies and iterates - * over the engine's list of dependencies. - * - * @return true if the analyzer supports parallel processing; - * otherwise false - * @see #analyzeDependency(Dependency, Engine) - * @see #findMoreDependencies(Engine, File) - */ - @Override - public boolean supportsParallelProcessing() { - return true; - } - /** * Analyzes a given dependency. If the dependency is an archive, such as a * WAR or EAR, the contents are extracted, scanned, and added to the list of From a38f8b447c06cd8cfd1de8de34b57caab1ef7f95 Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Mon, 2 Oct 2017 17:06:27 +0200 Subject: [PATCH 04/11] Cleanup: remove dead code --- .../dependencycheck/dependency/EvidenceCollection.java | 6 ------ 1 file changed, 6 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/dependency/EvidenceCollection.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/dependency/EvidenceCollection.java index eacb5d40c..93862d579 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/dependency/EvidenceCollection.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/dependency/EvidenceCollection.java @@ -25,8 +25,6 @@ import javax.annotation.concurrent.ThreadSafe; import org.apache.commons.lang3.builder.EqualsBuilder; import org.apache.commons.lang3.builder.HashCodeBuilder; import org.owasp.dependencycheck.utils.Filter; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; /** * Used to maintain a collection of Evidence. @@ -40,10 +38,6 @@ class EvidenceCollection implements Serializable { * The serial version UID for serialization. */ private static final long serialVersionUID = 1L; - /** - * The logger. - */ - private static final Logger LOGGER = LoggerFactory.getLogger(EvidenceCollection.class); /** * A collection of vendor evidence. */ From 6fc15984b854d60f47f8f471b12ece1666f97205 Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Wed, 4 Oct 2017 14:43:12 +0200 Subject: [PATCH 05/11] Please PMD --- .../org/owasp/dependencycheck/analyzer/CentralAnalyzer.java | 4 ++-- .../owasp/dependencycheck/analyzer/CentralAnalyzerTest.java | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java index bf3596067..a54fbff90 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java @@ -90,7 +90,7 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { /** * The searcher itself. */ - CentralSearch searcher; + protected CentralSearch searcher; /** * Field indicating if the analyzer is enabled. @@ -281,7 +281,7 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { * @throws FileNotFoundException if the specified artifact is not found * @throws IOException if connecting to MavenCentral finally failed */ - List fetchMavenArtifacts(Dependency dependency) throws IOException { + protected List fetchMavenArtifacts(Dependency dependency) throws IOException { IOException lastException = null; long sleepingTimeBetweenRetriesInMillis = 1000; int triesLeft = NUMBER_OF_TRIES; diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java index e7a129a06..24002da65 100644 --- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java @@ -30,6 +30,7 @@ public class CentralAnalyzerTest { } @Test + @SuppressWarnings("PMD.NonStaticInitializer") public void testFetchMavenArtifactsWithoutException(@Mocked final CentralSearch centralSearch, @Mocked final Dependency dependency) throws IOException { From eacf3ac906dc6bccc8c52d9f19bafa244840030f Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Wed, 4 Oct 2017 15:04:53 +0200 Subject: [PATCH 06/11] Please PMD --- .../owasp/dependencycheck/analyzer/CentralAnalyzerTest.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java index 24002da65..b95f274a2 100644 --- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java @@ -53,6 +53,7 @@ public class CentralAnalyzerTest { } @Test + @SuppressWarnings("PMD.NonStaticInitializer") public void testFetchMavenArtifactsWithSporadicIOException(@Mocked final CentralSearch centralSearch, @Mocked final Dependency dependency) throws IOException { @@ -77,6 +78,7 @@ public class CentralAnalyzerTest { } @Test(expected = FileNotFoundException.class) + @SuppressWarnings("PMD.NonStaticInitializer") public void testFetchMavenArtifactsRethrowsFileNotFoundException(@Mocked final CentralSearch centralSearch, @Mocked final Dependency dependency) throws IOException { @@ -96,6 +98,7 @@ public class CentralAnalyzerTest { } @Test(expected = IOException.class) + @SuppressWarnings("PMD.NonStaticInitializer") public void testFetchMavenArtifactsAlwaysThrowsIOException(@Mocked final CentralSearch centralSearch, @Mocked final Dependency dependency) throws IOException { From 6ddc0bfa27ac9c6e7a253c24e85da567a5fc1f53 Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Wed, 4 Oct 2017 20:11:38 +0200 Subject: [PATCH 07/11] Add license information --- .../analyzer/CentralAnalyzerTest.java | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java index b95f274a2..7b37864d7 100644 --- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java @@ -1,3 +1,20 @@ +/* + * This file is part of dependency-check-core. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + * Copyright (c) 2017 Jeremy Long. All Rights Reserved. + */ package org.owasp.dependencycheck.analyzer; import mockit.Expectations; From bfbec1d0a6fdd9c98966cb4238f2e48c65a81d9a Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Wed, 4 Oct 2017 20:34:39 +0200 Subject: [PATCH 08/11] Cleanup: Remove enabled flag (reuse flag from AbstractAnalyzer class) --- .../analyzer/CentralAnalyzer.java | 19 ++----------------- 1 file changed, 2 insertions(+), 17 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java index a54fbff90..462049b18 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java @@ -92,11 +92,6 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { */ protected CentralSearch searcher; - /** - * Field indicating if the analyzer is enabled. - */ - private boolean enabled = true; - /** * Initializes the analyzer with the configured settings. * @@ -105,17 +100,7 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { @Override public void initialize(Settings settings) { super.initialize(settings); - enabled = checkEnabled(); - } - - /** - * Determine whether to enable this analyzer or not. - * - * @return whether the analyzer should be enabled - */ - @Override - public boolean isEnabled() { - return enabled; + setEnabled(checkEnabled()); } /** @@ -215,7 +200,7 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { */ @Override public void analyzeDependency(Dependency dependency, Engine engine) throws AnalysisException { - if (errorFlag || !isEnabled()) { + if (errorFlag) { return; } From 98f9628e2731fcf6b27ac7c276271e4b4277403d Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Wed, 4 Oct 2017 20:48:16 +0200 Subject: [PATCH 09/11] Fail analysis/build in case of recurring IOExceptions when connecting to MavenCentral --- .../analyzer/CentralAnalyzer.java | 24 ++++++------------- .../analyzer/CentralAnalyzerTest.java | 21 ++++++++++++++++ 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java index 462049b18..1ab49f346 100644 --- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java +++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/CentralAnalyzer.java @@ -77,16 +77,10 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { /** * There may be temporary issues when connecting to MavenCentral. * In order to compensate for 99% of the issues, we perform a retry - * before finally raising the {@link #errorFlag}. + * before finally failing the analysis. */ private static final int NUMBER_OF_TRIES = 5; - /** - * The analyzer should be disabled if there are errors, so this is a flag to - * determine if such an error has occurred. - */ - private volatile boolean errorFlag = false; - /** * The searcher itself. */ @@ -200,10 +194,6 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { */ @Override public void analyzeDependency(Dependency dependency, Engine engine) throws AnalysisException { - if (errorFlag) { - return; - } - try { final List mas = fetchMavenArtifacts(dependency); final Confidence confidence = mas.size() > 1 ? Confidence.HIGH : Confidence.HIGHEST; @@ -249,8 +239,9 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { } catch (FileNotFoundException fnfe) { LOGGER.debug("Artifact not found in repository: '{}", dependency.getFileName()); } catch (IOException ioe) { - LOGGER.warn("Could not connect to Central search. Disabling this analyzer.", ioe); - errorFlag = true; + final String message = "Could not connect to Central search. Analysis failed."; + LOGGER.error(message, ioe); + throw new AnalysisException(message, ioe); } } @@ -292,9 +283,8 @@ public class CentralAnalyzer extends AbstractFileTypeAnalyzer { } } - LOGGER.warn("Finally failed connecting to Central search." + - " Giving up after {} tries. Last exception was: {}", - NUMBER_OF_TRIES, lastException); - throw lastException; + final String message = "Finally failed connecting to Central search." + + " Giving up after " + NUMBER_OF_TRIES + " tries."; + throw new IOException(message, lastException); } } diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java index 7b37864d7..3aace07d4 100644 --- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java @@ -23,6 +23,7 @@ import mockit.MockUp; import mockit.Mocked; import org.junit.BeforeClass; import org.junit.Test; +import org.owasp.dependencycheck.analyzer.exception.AnalysisException; import org.owasp.dependencycheck.data.central.CentralSearch; import org.owasp.dependencycheck.data.nexus.MavenArtifact; import org.owasp.dependencycheck.dependency.Dependency; @@ -134,6 +135,26 @@ public class CentralAnalyzerTest { instance.fetchMavenArtifacts(dependency); } + @Test(expected = AnalysisException.class) + @SuppressWarnings("PMD.NonStaticInitializer") + public void testFetchMavenArtifactsAlwaysThrowsIOExceptionLetsTheAnalysisFail(@Mocked final CentralSearch centralSearch, + @Mocked final Dependency dependency) + throws AnalysisException, IOException { + + CentralAnalyzer instance = new CentralAnalyzer(); + instance.searcher = centralSearch; + + new Expectations() {{ + dependency.getSha1sum(); + returns(SHA1_SUM); + + centralSearch.searchSha1(SHA1_SUM); + result = new IOException("no internet connection"); + }}; + + instance.analyze(dependency, null); + } + /** * We do not want to waste time in unit tests. */ From ed49251310e4cd7b0072b350b4212312e15ba786 Mon Sep 17 00:00:00 2001 From: Stefan Neuhaus Date: Wed, 4 Oct 2017 20:54:02 +0200 Subject: [PATCH 10/11] Cleanup test --- .../analyzer/CentralAnalyzerTest.java | 33 ++++++++++--------- 1 file changed, 18 insertions(+), 15 deletions(-) diff --git a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java index 3aace07d4..3a52a769b 100644 --- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java +++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CentralAnalyzerTest.java @@ -55,12 +55,10 @@ public class CentralAnalyzerTest { CentralAnalyzer instance = new CentralAnalyzer(); instance.searcher = centralSearch; + specifySha1SumFor(dependency); final List expectedMavenArtifacts = Collections.emptyList(); new Expectations() {{ - dependency.getSha1sum(); - returns(SHA1_SUM); - centralSearch.searchSha1(SHA1_SUM); returns(expectedMavenArtifacts); }}; @@ -78,12 +76,10 @@ public class CentralAnalyzerTest { CentralAnalyzer instance = new CentralAnalyzer(); instance.searcher = centralSearch; + specifySha1SumFor(dependency); final List expectedMavenArtifacts = Collections.emptyList(); new Expectations() {{ - dependency.getSha1sum(); - returns(SHA1_SUM); - centralSearch.searchSha1(SHA1_SUM); result = new IOException("Could not connect to MavenCentral (500): Internal Server Error"); result = new IOException("Could not connect to MavenCentral (500): Internal Server Error"); @@ -103,11 +99,9 @@ public class CentralAnalyzerTest { CentralAnalyzer instance = new CentralAnalyzer(); instance.searcher = centralSearch; + specifySha1SumFor(dependency); new Expectations() {{ - dependency.getSha1sum(); - returns(SHA1_SUM); - centralSearch.searchSha1(SHA1_SUM); result = new FileNotFoundException("Artifact not found in Central"); }}; @@ -123,11 +117,9 @@ public class CentralAnalyzerTest { CentralAnalyzer instance = new CentralAnalyzer(); instance.searcher = centralSearch; + specifySha1SumFor(dependency); new Expectations() {{ - dependency.getSha1sum(); - returns(SHA1_SUM); - centralSearch.searchSha1(SHA1_SUM); result = new IOException("no internet connection"); }}; @@ -143,11 +135,9 @@ public class CentralAnalyzerTest { CentralAnalyzer instance = new CentralAnalyzer(); instance.searcher = centralSearch; + specifySha1SumFor(dependency); new Expectations() {{ - dependency.getSha1sum(); - returns(SHA1_SUM); - centralSearch.searchSha1(SHA1_SUM); result = new IOException("no internet connection"); }}; @@ -166,4 +156,17 @@ public class CentralAnalyzerTest { } }; } + + /** + * Specifies the mock dependency's SHA1 sum. + * + * @param dependency then dependency + */ + @SuppressWarnings("PMD.NonStaticInitializer") + private void specifySha1SumFor(final Dependency dependency) { + new Expectations() {{ + dependency.getSha1sum(); + returns(SHA1_SUM); + }}; + } } From 818b8b295f25e4b3affd94f248b0e57d2efc3c99 Mon Sep 17 00:00:00 2001 From: Stephen Date: Thu, 5 Oct 2017 08:53:33 -0700 Subject: [PATCH 11/11] fix spelling --- .../owasp/dependencycheck/maven/BaseDependencyCheckMojo.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dependency-check-maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java b/dependency-check-maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java index 73a8875f7..c61f0274e 100644 --- a/dependency-check-maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java +++ b/dependency-check-maven/src/main/java/org/owasp/dependencycheck/maven/BaseDependencyCheckMojo.java @@ -803,7 +803,7 @@ public abstract class BaseDependencyCheckMojo extends AbstractMojo implements Ma dependencyNode.getArtifact().getId(), dependencyNode.getArtifact().getScope(), project.getName()); getLog().debug(msg); } else { - final String msg = String.format("No analzer could be found for '%s:%s' in project %s", + final String msg = String.format("No analyzer could be found for '%s:%s' in project %s", dependencyNode.getArtifact().getId(), dependencyNode.getArtifact().getScope(), project.getName()); getLog().warn(msg); }