Add sanity check on attachment filename

genie-web/src/main/java/com/netflix/genie/web/apis/rest/v3/controllers/GenieExceptionMapper.java

@@ -30,6 +30,7 @@
 import com.netflix.genie.common.internal.exceptions.unchecked.GenieJobNotFoundException;
 import com.netflix.genie.common.internal.exceptions.unchecked.GenieJobSpecificationNotFoundException;
 import com.netflix.genie.common.internal.exceptions.unchecked.GenieRuntimeException;
+import com.netflix.genie.web.exceptions.checked.IllegalAttachmentFileNameException;
 import com.netflix.genie.web.exceptions.checked.AttachmentTooLargeException;
 import com.netflix.genie.web.exceptions.checked.IdAlreadyExistsException;
 import com.netflix.genie.web.exceptions.checked.JobNotFoundException;
@@ -138,6 +139,8 @@ public ResponseEntity<GenieCheckedException> handleGenieCheckedException(final G
             return new ResponseEntity<>(e, HttpStatus.BAD_REQUEST);
         } else if (e instanceof AttachmentTooLargeException) {
             return new ResponseEntity<>(e, HttpStatus.PAYLOAD_TOO_LARGE);
+        } else if (e instanceof IllegalAttachmentFileNameException) {
+            return new ResponseEntity<>(e, HttpStatus.BAD_REQUEST);
         } else {
             return new ResponseEntity<>(e, HttpStatus.INTERNAL_SERVER_ERROR);
         }

genie-web/src/main/java/com/netflix/genie/web/exceptions/checked/IllegalAttachmentFileNameException.java

@@ -0,0 +1,43 @@
+package com.netflix.genie.web.exceptions.checked;
+
+/**
+ * Exception thrown when the attachment filename is illegal.
+ *
+ * @author bhou
+ */
+public class IllegalAttachmentFileNameException extends SaveAttachmentException {
+    /**
+     * Constructor.
+     */
+    public IllegalAttachmentFileNameException() {
+        super();
+    }
+
+    /**
+     * Constructor.
+     *
+     * @param message The detail message
+     */
+    public IllegalAttachmentFileNameException(final String message) {
+        super(message);
+    }
+
+    /**
+     * Constructor.
+     *
+     * @param message The detail message
+     * @param cause   The root cause of this exception
+     */
+    public IllegalAttachmentFileNameException(final String message, final Throwable cause) {
+        super(message, cause);
+    }
+
+    /**
+     * Constructor.
+     *
+     * @param cause The root cause of this exception
+     */
+    public IllegalAttachmentFileNameException(final Throwable cause) {
+        super(cause);
+    }
+}

genie-web/src/main/java/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImpl.java

@@ -19,6 +19,7 @@
 
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Sets;
+import com.netflix.genie.web.exceptions.checked.IllegalAttachmentFileNameException;
 import com.netflix.genie.web.exceptions.checked.AttachmentTooLargeException;
 import com.netflix.genie.web.exceptions.checked.SaveAttachmentException;
 import com.netflix.genie.web.properties.AttachmentServiceProperties;
@@ -93,6 +94,11 @@ public Set<URI> saveAttachments(
                 final long attachmentSize = attachment.contentLength();
                 final String filename = attachment.getFilename();
 
+                if (filename != null && filename.contains("/")) {
+                    throw new IllegalAttachmentFileNameException("Attachment filename " + filename + " is illegal. "
+                        + "It should not contain /");
+                }
+
                 if (attachmentSize > this.attachmentServiceProperties.getMaxSize().toBytes()) {
                     throw new AttachmentTooLargeException("Attachment is too large: " + filename);
                 }

genie-web/src/test/groovy/com/netflix/genie/web/exceptions/checked/GenieWebCheckedExceptionsSpec.groovy

@@ -66,6 +66,7 @@ class GenieWebCheckedExceptionsSpec extends Specification {
         exceptionClass                        | _
         AgentLaunchException                  | _
         AttachmentTooLargeException           | _
+        IllegalAttachmentFileNameException    | _
         IdAlreadyExistsException              | _
         JobDirectoryManifestNotFoundException | _
         JobNotArchivedException               | _

genie-web/src/test/groovy/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImplSpec.groovy

@@ -18,11 +18,13 @@
 package com.netflix.genie.web.services.impl
 
 import com.google.common.collect.Sets
+import com.netflix.genie.web.exceptions.checked.IllegalAttachmentFileNameException
 import com.netflix.genie.web.exceptions.checked.AttachmentTooLargeException
 import com.netflix.genie.web.exceptions.checked.SaveAttachmentException
 import com.netflix.genie.web.properties.AttachmentServiceProperties
 import org.apache.commons.io.FileUtils
 import org.apache.commons.lang3.RandomStringUtils
+import org.mockito.Mockito
 import org.springframework.core.io.FileSystemResource
 import org.springframework.core.io.Resource
 import org.springframework.util.unit.DataSize
@@ -151,4 +153,17 @@ class LocalFileSystemAttachmentServiceImplSpec extends Specification {
         then:
         thrown(SaveAttachmentException)
     }
+
+    def "reject attachments with illegal filename"() {
+        Set<Resource> attachments = Sets.newHashSet()
+        Resource attachment = Mock(Resource)
+        Mockito.doReturn("../../../root/breakout.file").when(attachment).getFilename()
+        attachments.add(attachment)
+
+        when:
+        service.saveAttachments(null, attachments)
+
+        then:
+        thrown(IllegalAttachmentFileNameException)
+    }
 }