Add security improvement - you can (and should) define a list of allo…

…wed url prefixes for the REST api to prevent users to call malicious urls
otto-de · Aug 7, 2024 · 8819fe3 · 8819fe3
1 parent 43141e6
commit 8819fe3
Show file tree

Hide file tree

Showing 8 changed files with 79 additions and 4 deletions.
diff --git a/web/src/main/java/de/otto/jlineup/web/JLineupController.java b/web/src/main/java/de/otto/jlineup/web/JLineupController.java
@@ -157,4 +157,9 @@ public ResponseEntity<String> exceptionHandler(final InvalidDefinitionException
     public ResponseEntity<String> exceptionHandler(final ValidationError exception) {
         return new ResponseEntity<>(exception.getMessage(), HttpStatus.UNPROCESSABLE_ENTITY);
     }
+
+    @ExceptionHandler(IllegalArgumentException.class)
+    public ResponseEntity<String> exceptionHandler(final IllegalArgumentException exception) {
+        return new ResponseEntity<>(exception.getMessage(), HttpStatus.UNPROCESSABLE_ENTITY);
+    }
 }
diff --git a/web/src/main/java/de/otto/jlineup/web/JLineupRunnerFactory.java b/web/src/main/java/de/otto/jlineup/web/JLineupRunnerFactory.java
@@ -4,6 +4,7 @@
 import de.otto.jlineup.RunStepConfig;
 import de.otto.jlineup.config.JobConfig;
 import de.otto.jlineup.config.RunStep;
+import de.otto.jlineup.config.UrlConfig;
 import de.otto.jlineup.service.BrowserNotInstalledException;
 import de.otto.jlineup.web.configuration.JLineupWebProperties;
 import org.springframework.beans.factory.annotation.Autowired;
@@ -60,12 +61,20 @@ private JLineupRunner createRun(String id, JobConfig jobConfig, RunStep step) th
                 .build());
     }
 
-    JobConfig sanitizeJobConfig(final JobConfig jobConfig) throws BrowserNotInstalledException {
+    JobConfig sanitizeJobConfig(final JobConfig jobConfig) throws BrowserNotInstalledException, IllegalArgumentException {
 
         if (!properties.getInstalledBrowsers().contains(jobConfig.browser)) {
             throw new BrowserNotInstalledException(jobConfig.browser);
         }
 
+        if (!properties.getAllowedUrlPrefixes().isEmpty()) {
+            for (UrlConfig urlConfig : jobConfig.urls.values()) {
+                if (properties.getAllowedUrlPrefixes().stream().noneMatch(urlConfig.getUrl()::startsWith)) {
+                    throw new IllegalArgumentException("URL " + urlConfig.url + " is not allowed. Allowed prefixes are: " + properties.getAllowedUrlPrefixes());
+                }
+            }
+        }
+
         return JobConfig.copyOfBuilder(jobConfig)
                 .withThreads(calculateNumberOfThreads(jobConfig))
                 .withDebug(false)

diff --git a/web/src/main/java/de/otto/jlineup/web/configuration/JLineupWebProperties.java b/web/src/main/java/de/otto/jlineup/web/configuration/JLineupWebProperties.java
@@ -42,6 +42,7 @@ public String toString() {
                 ", chromeLaunchParameters=" + chromeLaunchParameters +
                 ", firefoxLaunchParameters=" + firefoxLaunchParameters +
                 ", installedBrowsers=" + installedBrowsers +
+                ", allowedUrlPrefixes=" + allowedUrlPrefixes +
                 ", maxPersistedRuns=" + maxPersistedRuns +
                 ", lambda=" + lambda +
                 '}';
@@ -52,14 +53,16 @@ public boolean equals(Object o) {
         if (this == o) return true;
         if (o == null || getClass() != o.getClass()) return false;
         JLineupWebProperties that = (JLineupWebProperties) o;
-        return cleanupProfile == that.cleanupProfile && maxParallelJobs == that.maxParallelJobs && maxThreadsPerJob == that.maxThreadsPerJob && maxPersistedRuns == that.maxPersistedRuns && Objects.equals(workingDirectory, that.workingDirectory) && Objects.equals(screenshotsDirectory, that.screenshotsDirectory) && Objects.equals(reportDirectory, that.reportDirectory) && Objects.equals(chromeLaunchParameters, that.chromeLaunchParameters) && Objects.equals(firefoxLaunchParameters, that.firefoxLaunchParameters) && Objects.equals(installedBrowsers, that.installedBrowsers) && Objects.equals(lambda, that.lambda);
+        return cleanupProfile == that.cleanupProfile && maxParallelJobs == that.maxParallelJobs && maxThreadsPerJob == that.maxThreadsPerJob && maxPersistedRuns == that.maxPersistedRuns && Objects.equals(workingDirectory, that.workingDirectory) && Objects.equals(screenshotsDirectory, that.screenshotsDirectory) && Objects.equals(reportDirectory, that.reportDirectory) && Objects.equals(chromeLaunchParameters, that.chromeLaunchParameters) && Objects.equals(firefoxLaunchParameters, that.firefoxLaunchParameters) && Objects.equals(installedBrowsers, that.installedBrowsers) && Objects.equals(allowedUrlPrefixes, that.allowedUrlPrefixes) && Objects.equals(lambda, that.lambda);
     }
 
     @Override
     public int hashCode() {
-        return Objects.hash(workingDirectory, screenshotsDirectory, reportDirectory, cleanupProfile, maxParallelJobs, maxThreadsPerJob, chromeLaunchParameters, firefoxLaunchParameters, installedBrowsers, maxPersistedRuns, lambda);
+        return Objects.hash(workingDirectory, screenshotsDirectory, reportDirectory, cleanupProfile, maxParallelJobs, maxThreadsPerJob, chromeLaunchParameters, firefoxLaunchParameters, installedBrowsers, allowedUrlPrefixes, maxPersistedRuns, lambda);
     }
 
+    private List<String> allowedUrlPrefixes = emptyList();
+
     private int maxPersistedRuns = DEFAULT_MAX_PERSISTED_RUNS;
 
     private JLineupWebLambdaProperties lambda = new JLineupWebLambdaProperties();
@@ -151,4 +154,12 @@ public int getMaxPersistedRuns() {
     public void setMaxPersistedRuns(int maxPersistedRuns) {
         this.maxPersistedRuns = maxPersistedRuns;
     }
+
+    public List<String> getAllowedUrlPrefixes() {
+        return allowedUrlPrefixes;
+    }
+
+    public void setAllowedUrlPrefixes(List<String> allowedUrlPrefixes) {
+        this.allowedUrlPrefixes = allowedUrlPrefixes;
+    }
 }
diff --git a/web/src/main/resources/META-INF/additional-spring-configuration-metadata.json b/web/src/main/resources/META-INF/additional-spring-configuration-metadata.json
@@ -0,0 +1,8 @@
+{
+  "properties": [
+    {
+      "name": "jlineup.allowedUrlPrefixes",
+      "type": "java.util.List<java.lang.String>",
+      "description": "All URLs that you tell JLineup to call via REST api have to begin with one of these prefixes. This is a security feature to prevent JLineup from calling arbitrary URLs."
+  }
+] }
diff --git a/web/src/main/resources/application.yml b/web/src/main/resources/application.yml
@@ -5,6 +5,10 @@ jlineup:
   maxParallelJobs: 1
   chromeLaunchParameters: --use-spdy=off, --disable-dev-shm-usage
   cleanupProfile: true
+  allowedUrlPrefixes:
+    - https://www.otto.de
+    - https://develop.otto.de
+    - https://www.example.com
   lambda:
     functionName: jlineup-lambda
 

diff --git a/web/src/test/java/de/otto/jlineup/web/JLineupRunnerFactoryTest.java b/web/src/test/java/de/otto/jlineup/web/JLineupRunnerFactoryTest.java
@@ -1,12 +1,15 @@
 package de.otto.jlineup.web;
 
 import de.otto.jlineup.config.JobConfig;
+import de.otto.jlineup.config.UrlConfig;
 import de.otto.jlineup.service.BrowserNotInstalledException;
 import de.otto.jlineup.web.configuration.JLineupWebProperties;
 import org.junit.Before;
 import org.junit.Test;
 
 import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
 
 import static de.otto.jlineup.browser.Browser.Type.*;
 import static de.otto.jlineup.config.JobConfig.*;
@@ -23,11 +26,12 @@ public void setup() {
         jLineupWebProperties = new JLineupWebProperties();
         jLineupWebProperties.setInstalledBrowsers(Arrays.asList(CHROME, CHROME_HEADLESS));
         jLineupWebProperties.setMaxThreadsPerJob(10);
+        jLineupWebProperties.setAllowedUrlPrefixes(List.of("https://www.example.com"));
         jLineupRunnerFactory = new JLineupRunnerFactory(jLineupWebProperties);
     }
 
     @Test
-    public void shouldSanitizeJobConfig() throws BrowserNotInstalledException {
+    public void shouldSanitizeJobConfigForInstalledBrowsers() throws BrowserNotInstalledException {
 
         //Given
         JobConfig evilJobConfig = copyOfBuilder(exampleConfig())
@@ -59,6 +63,30 @@ public void shouldThrowBrowserNotInstalledException() throws BrowserNotInstalled
         jLineupRunnerFactory.sanitizeJobConfig(jobConfig);
     }
 
+    @Test(expected = IllegalArgumentException.class)
+    public void shouldThrowIllegalArgumentExceptionBecauseURLIsNotAllowed() throws IllegalArgumentException, BrowserNotInstalledException {
+        // Given
+        JobConfig jobConfig = copyOfBuilder(exampleConfig())
+                .withUrls(Map.of("Some Url", UrlConfig.copyOfBuilder(exampleConfig().urls.values().stream().findFirst().get()).withUrl("https://www.notallowed.org").build()))
+                .build();
+
+        //When
+        jLineupRunnerFactory.sanitizeJobConfig(jobConfig);
+    }
+
+    @Test
+    public void shouldNotThrowIllegalArgumentExceptionWhenURLPrefixesAreNotSet() throws BrowserNotInstalledException {
+        // Given
+        JobConfig jobConfig = copyOfBuilder(exampleConfig())
+                .withUrls(Map.of("Some Url", UrlConfig.copyOfBuilder(exampleConfig().urls.values().stream().findFirst().get()).withUrl("https://www.notallowed.org").build()))
+                .build();
+
+        jLineupWebProperties.setAllowedUrlPrefixes(List.of());
+
+        //When
+        jLineupRunnerFactory.sanitizeJobConfig(jobConfig);
+    }
+
     @Test
     public void shouldUseMaxThreadsPerJobIfNoThreadsAreConfigured() throws BrowserNotInstalledException {
         //Given

diff --git a/web/src/test/java/de/otto/jlineup/web/JLineupWebApplicationTests.java b/web/src/test/java/de/otto/jlineup/web/JLineupWebApplicationTests.java
@@ -127,6 +127,12 @@ public void shouldFailIfBrowserIsNotConfigured() {
         expectStatusCodeForConfig(jobConfig, UNPROCESSABLE_ENTITY.value());
     }
 
+    @Test
+    public void shouldFailForForbiddenUrl() {
+        JobConfig jobConfig = copyOfBuilder(createTestConfig()).addUrlConfig("https://www.forbidden.com", UrlConfig.urlConfigBuilder().build()).build();
+        expectStatusCodeForConfig(jobConfig, UNPROCESSABLE_ENTITY.value());
+    }
+
     private void assertReportExists(JLineupRunStatus finalState) {
         when()
                 .get(contextPath + "/reports/report-" + finalState.getId() + "/report.json")

diff --git a/web/src/test/resources/application-test.yml b/web/src/test/resources/application-test.yml
@@ -1,6 +1,10 @@
 jlineup:
   installedBrowsers: chrome, chrome-headless
   chromeLaunchParameters: --user-data-dir=/tmp/jlineup/chrome-profile-{id}, --force-device-scale-factor=1
+  allowedUrlPrefixes:
+    - https://www.otto.de
+    - https://develop.otto.de
+    - https://www.example.com
 
 server:
   servlet: