Amend the check on IllegalAttachmentFileNameException

Netflix · May 3, 2024 · 90c5e8e · 90c5e8e
1 parent 0dbdd3d
commit 90c5e8e
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 3 deletions.
diff --git a/...c/main/java/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImpl.java b/...c/main/java/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImpl.java
@@ -27,6 +27,7 @@
 import org.springframework.core.io.Resource;
 
 import javax.annotation.Nullable;
+import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.net.URI;
@@ -94,9 +95,10 @@ public Set<URI> saveAttachments(
                 final long attachmentSize = attachment.contentLength();
                 final String filename = attachment.getFilename();
 
-                if (filename != null && (filename.contains("/") || filename.contains("\\"))) {
-                    throw new IllegalAttachmentFileNameException("Attachment filename " + filename + " is illegal. "
-                        + "Filenames should not contain / or \\.");
+                if (filename != null && ((filename.contains("/") || filename.contains("\\")
+                        || filename.equals(".") || filename.equals("..")))) {
+                        throw new IllegalAttachmentFileNameException("Attachment filename " + filename + " is illegal. "
+                            + "Filenames should not be . or .., or contain /, \\.");
                 }
 
                 if (attachmentSize > this.attachmentServiceProperties.getMaxSize().toBytes()) {
@@ -113,6 +115,15 @@ public Set<URI> saveAttachments(
                     filename != null ? filename : UUID.randomUUID().toString()
                 );
 
+                if (filename != null) {
+                    final File file = new File(String.valueOf(attachmentsBasePath), filename);
+                    if (!file.getCanonicalPath().startsWith(String.valueOf(attachmentsBasePath))
+                        || file.getCanonicalPath().equals(String.valueOf(attachmentsBasePath))) {
+                        throw new IllegalAttachmentFileNameException("Attachment filename " + filename + " is illegal. "
+                            + "Filenames should not be a relative path.");
+                    }
+                }
+
                 Files.copy(inputStream, attachmentPath);
 
                 setBuilder.add(attachmentPath.toUri());

diff --git a/...roovy/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImplSpec.groovy b/...roovy/com/netflix/genie/web/services/impl/LocalFileSystemAttachmentServiceImplSpec.groovy
@@ -179,4 +179,30 @@ class LocalFileSystemAttachmentServiceImplSpec extends Specification {
         then:
         thrown(IllegalAttachmentFileNameException)
     }
+
+    def "reject attachments with illegal filename is ."() {
+        Set<Resource> attachments = new HashSet<Resource>()
+        Resource attachment = Mockito.mock(Resource.class)
+        Mockito.doReturn(".").when(attachment).getFilename()
+        attachments.add(attachment)
+
+        when:
+        service.saveAttachments(null, attachments)
+
+        then:
+        thrown(IllegalAttachmentFileNameException)
+    }
+
+    def "reject attachments with illegal filename is .."() {
+        Set<Resource> attachments = new HashSet<Resource>()
+        Resource attachment = Mockito.mock(Resource.class)
+        Mockito.doReturn("..").when(attachment).getFilename()
+        attachments.add(attachment)
+
+        when:
+        service.saveAttachments(null, attachments)
+
+        then:
+        thrown(IllegalAttachmentFileNameException)
+    }
 }