Skip to content

Commit

Permalink
fix(storage-local): path traversal guard should include File.separator
Browse files Browse the repository at this point in the history
Today, we check that a file didn't contains '..' which is too aggressive, we should check that it didn't contains '../' or '..\' only.
  • Loading branch information
loicmathieu committed Jan 3, 2025
1 parent ed93abc commit 33e01ff
Show file tree
Hide file tree
Showing 3 changed files with 34 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -100,4 +100,13 @@ default URI from(Execution execution, String input, File file) throws IOExceptio
URI uri = StorageContext.forInput(execution, input, file.getName()).getContextStorageURI();
return this.put(execution.getTenantId(), execution.getNamespace(), uri, new BufferedInputStream(new FileInputStream(file)));
}

/**
* Throws an IllegalArgumentException if the URI is not absolute: a.k.a., if it contains <code>".." + File.separator</code>.
*/
default void parentTraversalGuard(URI uri) {
if (uri != null && uri.toString().contains(".." + File.separator)) {
throw new IllegalArgumentException("File should be accessed with their full path and not using relative '..' path.");
}
}
}
25 changes: 25 additions & 0 deletions core/src/test/java/io/kestra/plugin/core/http/DownloadTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,25 @@ void failed() throws Exception {
}
}

@Test
void contentDispositionWithDoubleDot() throws Exception {
EmbeddedServer embeddedServer = applicationContext.getBean(EmbeddedServer.class);
embeddedServer.start();

Download task = Download.builder()
.id(DownloadTest.class.getSimpleName())
.type(DownloadTest.class.getName())
.uri(embeddedServer.getURI() + "/content-disposition-double-dot")
.build();

RunContext runContext = TestsUtils.mockRunContext(this.runContextFactory, task, ImmutableMap.of());

Download.Output output = task.run(runContext);

assertThat(output.getUri().toString(), not(containsString("/secure-path/")));
assertThat(output.getUri().toString(), endsWith("filename..jpg"));
}

@Controller()
public static class SlackWebController {
@Get("500")
Expand All @@ -202,5 +221,11 @@ public HttpResponse<byte[]> contentDispositionWithPath() {
return HttpResponse.ok("Hello World".getBytes())
.header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"/secure-path/filename.jpg\"");
}

@Get("content-disposition-double-dot")
public HttpResponse<byte[]> contentDispositionWithDoubleDot() {
return HttpResponse.ok("Hello World".getBytes())
.header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"/secure-path/filename..jpg\"");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -240,10 +240,4 @@ private URI getKestraUri(String tenantId, Path path) {
Path.of(basePath.toAbsolutePath().toString(), tenantId);
return URI.create("kestra:///" + prefix.relativize(path).toString().replace("\\", "/"));
}

private void parentTraversalGuard(URI uri) {
if (uri.toString().contains("..")) {
throw new IllegalArgumentException("File should be accessed with their full path and not using relative '..' path.");
}
}
}

0 comments on commit 33e01ff

Please sign in to comment.