Skip to content

Commit b7fb9a9

Browse files
maxandersenclaude
maxandersen
and
claude
committed
security: harden splash screen path validation and add comprehensive tests
Security hardening for SplashScreen-Image extraction: - Reject absolute paths (e.g., /tmp/splash.png) - Reject path traversal attempts (e.g., ../../../etc/passwd) - Reject directory entries (must be a file) - Validate path is not empty or whitespace - Normalize backslashes to forward slashes for Windows compatibility New unit tests: - KeyValueTest: Tests for the split('=', 2) fix that enables values containing '=' signs (e.g., Add-Reads=mod1=mod2) - testSplashScreenPathTraversal: Verifies path traversal rejection - testSplashScreenAbsolutePath: Verifies absolute path rejection - testSplashScreenEmptyPath: Verifies empty path handling - testSplashScreenCaching: Verifies no re-extraction when jar unchanged and re-extraction when jar is modified - testAddReadsMalformed: Verifies proper handling of extra whitespace All security validations fail gracefully with warnings rather than throwing exceptions, maintaining backward compatibility. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1 parent 9f92987 commit b7fb9a9

File tree

3 files changed

+236
-3
lines changed

3 files changed

+236
-3
lines changed

‎src/main/java/dev/jbang/source/generators/JarCmdGenerator.java‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -362,17 +362,36 @@ private static void addPropertyFlags(Map<String, String> properties, String def,
362362
* @return Path to extracted image, or null if extraction failed
363363
*/
364364
private static Path extractSplashImage(Path jarPath, String imagePath) {
365+
// Validate image path for security
366+
if (imagePath == null || imagePath.trim().isEmpty()) {
367+
Util.warnMsg("Splash screen image path is empty");
368+
return null;
369+
}
370+
371+
// Normalize and validate path to prevent directory traversal attacks
372+
String normalizedPath = imagePath.replace('\\', '/');
373+
if (normalizedPath.startsWith("/") || normalizedPath.contains("../") || normalizedPath.contains("..\\")) {
374+
Util.warnMsg("Invalid splash screen image path (absolute or contains '..'): " + imagePath);
375+
return null;
376+
}
377+
365378
try (java.util.jar.JarFile jar = new java.util.jar.JarFile(jarPath.toFile())) {
366-
java.util.jar.JarEntry entry = jar.getJarEntry(imagePath);
379+
java.util.jar.JarEntry entry = jar.getJarEntry(normalizedPath);
367380
if (entry == null) {
368381
Util.warnMsg("Splash screen image not found in jar: " + imagePath);
369382
return null;
370383
}
371384

385+
// Reject directory entries
386+
if (entry.isDirectory()) {
387+
Util.warnMsg("Splash screen image path is a directory: " + imagePath);
388+
return null;
389+
}
390+
372391
// Extract to jar's directory with unique name
373392
String jarName = jarPath.getFileName().toString();
374-
int extIndex = imagePath.lastIndexOf('.');
375-
String imageExt = (extIndex > 0) ? imagePath.substring(extIndex) : "";
393+
int extIndex = normalizedPath.lastIndexOf('.');
394+
String imageExt = (extIndex > 0) ? normalizedPath.substring(extIndex) : "";
376395
Path targetPath = jarPath.getParent().resolve(jarName + ".splash" + imageExt);
377396

378397
// Extract if not cached or jar is newer

‎src/test/java/dev/jbang/cli/TestRun.java‎

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2806,6 +2806,153 @@ void testMissingSplashScreenImage(@TempDir Path output) throws IOException {
28062806
assertThat(cmd, not(containsString("-splash:")));
28072807
}
28082808

2809+
@Test
2810+
void testSplashScreenPathTraversal(@TempDir Path output) throws IOException {
2811+
// Test path traversal attack prevention
2812+
Path jar = createJar(output, Integer.toString(Runtime.version().feature()),
2813+
Collections.singletonMap(Project.ATTR_SPLASH_SCREEN_IMAGE, "../../../etc/passwd"));
2814+
2815+
CommandLine.ParseResult pr = JBang.getCommandLine().parseArgs("run", jar.toString());
2816+
Run run = (Run) pr.subcommand().commandSpec().userObject();
2817+
2818+
ProjectBuilder pb = run.createProjectBuilderForRun();
2819+
Project code = pb.build(jar.toString());
2820+
2821+
// Should reject the path and not include -splash flag
2822+
String cmd = CmdGenerator.builder(code).build().generate();
2823+
assertThat(cmd, not(containsString("-splash:")));
2824+
}
2825+
2826+
@Test
2827+
void testSplashScreenAbsolutePath(@TempDir Path output) throws IOException {
2828+
// Test absolute path rejection
2829+
Path jar = createJar(output, Integer.toString(Runtime.version().feature()),
2830+
Collections.singletonMap(Project.ATTR_SPLASH_SCREEN_IMAGE, "/tmp/splash.png"));
2831+
2832+
CommandLine.ParseResult pr = JBang.getCommandLine().parseArgs("run", jar.toString());
2833+
Run run = (Run) pr.subcommand().commandSpec().userObject();
2834+
2835+
ProjectBuilder pb = run.createProjectBuilderForRun();
2836+
Project code = pb.build(jar.toString());
2837+
2838+
// Should reject absolute paths
2839+
String cmd = CmdGenerator.builder(code).build().generate();
2840+
assertThat(cmd, not(containsString("-splash:")));
2841+
}
2842+
2843+
@Test
2844+
void testSplashScreenEmptyPath(@TempDir Path output) throws IOException {
2845+
// Test empty path handling
2846+
Path jar = createJar(output, Integer.toString(Runtime.version().feature()),
2847+
Collections.singletonMap(Project.ATTR_SPLASH_SCREEN_IMAGE, ""));
2848+
2849+
CommandLine.ParseResult pr = JBang.getCommandLine().parseArgs("run", jar.toString());
2850+
Run run = (Run) pr.subcommand().commandSpec().userObject();
2851+
2852+
ProjectBuilder pb = run.createProjectBuilderForRun();
2853+
Project code = pb.build(jar.toString());
2854+
2855+
// Should handle empty path gracefully
2856+
String cmd = CmdGenerator.builder(code).build().generate();
2857+
assertThat(cmd, not(containsString("-splash:")));
2858+
}
2859+
2860+
@Test
2861+
void testSplashScreenCaching(@TempDir Path output) throws IOException, InterruptedException {
2862+
// Create a simple 1x1 PNG image
2863+
byte[] pngData = new byte[] {
2864+
(byte) 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, // PNG header
2865+
0x00, 0x00, 0x00, 0x0D, 0x49, 0x48, 0x44, 0x52, // IHDR chunk
2866+
0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x01, // 1x1 dimensions
2867+
0x08, 0x02, 0x00, 0x00, 0x00, (byte) 0x90, (byte) 0x77, 0x53, (byte) 0xDE, 0x00, 0x00, 0x00, 0x0C,
2868+
0x49, 0x44, 0x41, 0x54, // IDAT chunk
2869+
0x08, (byte) 0xD7, 0x63, (byte) 0xF8, (byte) 0xCF, (byte) 0xC0, 0x00, 0x00, 0x03, 0x01, 0x01, 0x00,
2870+
0x18, (byte) 0xDD, (byte) 0x8D, (byte) 0xB4, 0x00, 0x00, 0x00, 0x00, 0x49, 0x45, 0x4E, 0x44,
2871+
(byte) 0xAE, 0x42, 0x60, (byte) 0x82
2872+
};
2873+
2874+
Path jar = output.resolve("cache-test.jar");
2875+
Manifest manifest = new Manifest();
2876+
Attributes attrs = manifest.getMainAttributes();
2877+
attrs.put(Attributes.Name.MANIFEST_VERSION, "1.0");
2878+
attrs.put(Attributes.Name.MAIN_CLASS, "test.Main");
2879+
attrs.putValue(Project.ATTR_SPLASH_SCREEN_IMAGE, "splash.png");
2880+
2881+
try (JarOutputStream jos = new JarOutputStream(Files.newOutputStream(jar), manifest)) {
2882+
jos.putNextEntry(new ZipEntry("splash.png"));
2883+
jos.write(pngData);
2884+
jos.closeEntry();
2885+
}
2886+
2887+
// First run - extract splash
2888+
CommandLine.ParseResult pr1 = JBang.getCommandLine().parseArgs("run", jar.toString());
2889+
Run run1 = (Run) pr1.subcommand().commandSpec().userObject();
2890+
ProjectBuilder pb1 = run1.createProjectBuilderForRun();
2891+
Project code1 = pb1.build(jar.toString());
2892+
String cmd1 = CmdGenerator.builder(code1).build().generate();
2893+
2894+
assertThat(cmd1, containsString("-splash:"));
2895+
String splashArg1 = Arrays.stream(cmd1.split(" "))
2896+
.filter(s -> s.startsWith("-splash:"))
2897+
.findFirst()
2898+
.orElse(null);
2899+
assertNotNull(splashArg1);
2900+
Path extractedPath = Paths.get(splashArg1.substring("-splash:".length()));
2901+
assertTrue(Files.exists(extractedPath), "Splash image should be extracted");
2902+
2903+
// Get the timestamp of the extracted file
2904+
java.nio.file.attribute.FileTime firstTimestamp = Files.getLastModifiedTime(extractedPath);
2905+
2906+
// Wait a bit to ensure different timestamp if re-extracted
2907+
Thread.sleep(1500);
2908+
2909+
// Second run - should use cached splash (no re-extraction)
2910+
CommandLine.ParseResult pr2 = JBang.getCommandLine().parseArgs("run", jar.toString());
2911+
Run run2 = (Run) pr2.subcommand().commandSpec().userObject();
2912+
ProjectBuilder pb2 = run2.createProjectBuilderForRun();
2913+
Project code2 = pb2.build(jar.toString());
2914+
String cmd2 = CmdGenerator.builder(code2).build().generate();
2915+
2916+
java.nio.file.attribute.FileTime secondTimestamp = Files.getLastModifiedTime(extractedPath);
2917+
assertEquals(firstTimestamp, secondTimestamp,
2918+
"Splash should not be re-extracted if jar hasn't changed");
2919+
2920+
// Update jar timestamp to force re-extraction
2921+
Files.setLastModifiedTime(jar,
2922+
java.nio.file.attribute.FileTime.fromMillis(System.currentTimeMillis() + 10000));
2923+
2924+
// Third run - should re-extract splash
2925+
CommandLine.ParseResult pr3 = JBang.getCommandLine().parseArgs("run", jar.toString());
2926+
Run run3 = (Run) pr3.subcommand().commandSpec().userObject();
2927+
ProjectBuilder pb3 = run3.createProjectBuilderForRun();
2928+
Project code3 = pb3.build(jar.toString());
2929+
String cmd3 = CmdGenerator.builder(code3).build().generate();
2930+
2931+
java.nio.file.attribute.FileTime thirdTimestamp = Files.getLastModifiedTime(extractedPath);
2932+
assertTrue(thirdTimestamp.compareTo(secondTimestamp) > 0,
2933+
"Splash should be re-extracted when jar is newer");
2934+
}
2935+
2936+
@Test
2937+
void testAddReadsMalformed(@TempDir Path output) throws IOException {
2938+
assumeTrue(Runtime.version().feature() >= 9, "requires Java 9+");
2939+
// Test with spaces in Add-Reads - should generate multiple flags
2940+
String reads = "java.base=java.logging module.a=module.b";
2941+
Path jar = createJar(output, Integer.toString(Runtime.version().feature()),
2942+
Collections.singletonMap(Project.ATTR_ADD_READS, reads));
2943+
2944+
CommandLine.ParseResult pr = JBang.getCommandLine().parseArgs("run", jar.toString());
2945+
Run run = (Run) pr.subcommand().commandSpec().userObject();
2946+
2947+
ProjectBuilder pb = run.createProjectBuilderForRun();
2948+
Project code = pb.build(jar.toString());
2949+
String cmd = CmdGenerator.builder(code).build().generate();
2950+
2951+
// Should handle extra whitespace gracefully
2952+
assertThat(cmd, containsString("--add-reads=java.base=java.logging"));
2953+
assertThat(cmd, containsString("--add-reads=module.a=module.b"));
2954+
}
2955+
28092956
private Path createJar(Path outputDir, String buildJdk, Map<String, String> manifestAttributes) throws IOException {
28102957
Path jar = outputDir.resolve("manifest-test.jar");
28112958
Manifest manifest = new Manifest();

‎src/test/java/dev/jbang/source/parser/KeyValueTest.java‎

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
package dev.jbang.source.parser;
2+
3+
import static org.junit.jupiter.api.Assertions.assertEquals;
4+
import static org.junit.jupiter.api.Assertions.assertNull;
5+
6+
import org.junit.jupiter.api.Test;
7+
8+
public class KeyValueTest {
9+
10+
@Test
11+
void testSimpleKeyValue() {
12+
KeyValue kv = KeyValue.of("key=value");
13+
assertEquals("key", kv.getKey());
14+
assertEquals("value", kv.getValue());
15+
}
16+
17+
@Test
18+
void testKeyOnly() {
19+
KeyValue kv = KeyValue.of("key");
20+
assertEquals("key", kv.getKey());
21+
assertNull(kv.getValue());
22+
}
23+
24+
@Test
25+
void testValueWithEquals() {
26+
// This is the fix for Add-Reads=module1=module2
27+
KeyValue kv = KeyValue.of("Add-Reads=java.base=java.logging");
28+
assertEquals("Add-Reads", kv.getKey());
29+
assertEquals("java.base=java.logging", kv.getValue());
30+
}
31+
32+
@Test
33+
void testValueWithMultipleEquals() {
34+
KeyValue kv = KeyValue.of("key=value1=value2=value3");
35+
assertEquals("key", kv.getKey());
36+
assertEquals("value1=value2=value3", kv.getValue());
37+
}
38+
39+
@Test
40+
void testEmptyValue() {
41+
KeyValue kv = KeyValue.of("key=");
42+
assertEquals("key", kv.getKey());
43+
assertEquals("", kv.getValue());
44+
}
45+
46+
@Test
47+
void testWhitespaceInKey() {
48+
KeyValue kv = KeyValue.of("my key=value");
49+
assertEquals("my key", kv.getKey());
50+
assertEquals("value", kv.getValue());
51+
}
52+
53+
@Test
54+
void testWhitespaceInValue() {
55+
KeyValue kv = KeyValue.of("key=my value");
56+
assertEquals("key", kv.getKey());
57+
assertEquals("my value", kv.getValue());
58+
}
59+
60+
@Test
61+
void testModuleReadsFormat() {
62+
// Real-world example from Add-Reads manifest attribute
63+
KeyValue kv = KeyValue.of("Add-Reads=module.a=module.b");
64+
assertEquals("Add-Reads", kv.getKey());
65+
assertEquals("module.a=module.b", kv.getValue());
66+
}
67+
}

0 commit comments

Comments
 (0)