Fix loading jars with encoded paths (#2759)

Fixes #2576
and #2103, which are both
that the model assembler can't load jars if the jar's path has
percent-encoded characters. This may happen for artifacts resolved by
coursier (see linked issues).

The model assembler can load Smithy files from a jar - model discovery.
This works by looking for a 'META-INF/smithy/manifest'
file within the jar, which contains the paths of smithy model files
within the jar. You can give the model assembler a class loader to use
for model discovery, but you can also add a jar to discover models from
using a regular `addImport`. The problem is with the latter case.
Because jars are like zip files, you can't read files within the jar
using regular file system operations. Instead, you can either use the
JarFile api to open the jar, and read specific entries (like a zip
file), or you can directly read specific entries via a special URL with
the format `jar:file:/path/to/jar.jar!/path/in/jar`. The latter is the
way that our model discovery apis work. The problem was that URL doesn't
perform any encoding or decoding itself, so callers/consumers have to
take care of it, which we were not doing. For example, if the jar file
has a path of `/foo/bar%45baz.jar`, we would create a URL like:
`jar:file:/foo/bar%45baz.jar!/META-INF/smithy/manifest`, and when the
JDK tried to use this URL to read from the file system, it would decode
it and attempt to read `/foo/[email protected]`. Since we weren't encoding the
URL to begin with, this was only a problem when the path contained `%`.

The URL class has a section in its javadoc about
this footgun, strongly recommending using `Path::toURI`, `URI::toURL`,
when you want to make URLs from paths. And in more recent jdk versions,
URL constructors are deprecated in favor of `URI::toURL`. I found that
the `Path::toURI` part is particularly important, because it handles
encoding for paths correctly, and makes sure windows paths are handled
properly. I couldn't use `URI::toURL` though, because the manifest path
needs to be appended, and the scheme changed to `jar:` (technically the
`file:/...` is all the "scheme-specific part"). So instead, when creating
a manifest URL from a path, `createSmithyJarManifestUrl` will now create
a `Path` and convert that to `URI` to get an encoded `file:/` URI that
is also correct for windows, then constructs the final `URL` using the
string form of that `URI`. This should still meet `URL`'s requirement
that it be created with an already-correct URL.

Another thing to note is that `createSmithyJarManifestUrl` can accept
stuff like `file:/foo.jar` or `jar:file:/foo.jar`. This change does
not apply to those - they aren't paths obviously, and we don't know
if they were properly encoded by the caller. If they were, trying to
extract the path and do the same path -> URI -> URL conversion would
result in a double-encoded URL that doesn't point to the correct
location, and trying to instead just do URI -> URL is pointless (also
`jar:file:/foo.jar` can't be directly turned into a `URI` - the `jar:`
scheme requires the `!/` part). So for these, the previous behavior
is kept, and I added a bit to the javadoc to say they need to be valid
URLs.

This technically changes the `SourceLocation::filename` for jars that
are `addImport`ed, when the jar path has reserved characters (like
spaces). Such jars were importable before because URL doesn't validate
encoding, and the JDK decoding the URL would be a no-op since it wasn't
encoded to begin with. So we could _just_ encode `%`, meaning only the
newly-allowed paths would have any encoding. Disregarding the fact that
this side-steps the assumed pre-conditions of URL, it would actually
exacerbate an existing inconsistency between `SourceLocation::filename`
of models discovered on the classpath, and models discovered from
imports. The JDK encodes the URLs of resources on the classpath, so
their models' source locations will be encoded. There was an additional
inconsistency on windows, where imported jar models would have a URL
like `jar:file:C:\foo\bar.jar!/META-INF/smithy/foo.smithy`, which the
jdk could still gracefully read from, but is incorrect and inconsistent
with discovered models which correctly look like
`jar:file:/C:/foo/bar.jar!/META-INF/smithy/foo.smithy`. So this change
means that all `jar:file:` `SourceLocation::filename`s will look the
same and are well-formed URLs/URIs.

As a consequence of changing `SourceLocation::filename`, I had to fix
some code in PluginContext which was relying on being able to compare
the source location filename with a stringified version of each
configured source path, in order to figure out which shapes/metadata
are part of sources. This was working because 1. Encoded paths didn't
work at all, and 2. `jar:file:` filenames on windows had windows path
separators. So I updated this comparison code to compare `jar:file:`
filenames to a string version of each configured source's _URI_.

The fact that the specific format of the filename was being relied on is
mildly concerning, but considering the fact that models discovered on
the classpath already had a different format, I think this is ok. I
would also be surprised if there's a lot of code out there manually
importing jars, instead of providing a classloader.

Author: milesziemer

smithy-build/src/main/java/software/amazon/smithy/build/PluginContext.java

@@ -5,6 +5,7 @@
 package software.amazon.smithy.build;
 
 import java.nio.file.Path;
+import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
@@ -37,6 +38,7 @@ public final class PluginContext implements ToSmithyBuilder<PluginContext> {
     private final ClassLoader pluginClassLoader;
     private final Set<Path> sources;
     private final String artifactName;
+    private final List<String> sourceUris;
     private Model nonTraitsModel;
 
     private PluginContext(Builder builder) {
@@ -50,6 +52,15 @@ private PluginContext(Builder builder) {
         settings = builder.settings;
         pluginClassLoader = builder.pluginClassLoader;
         sources = builder.sources.copy();
+        // Normalize the source paths into URI strings, which are URI encoded and use '/' separators,
+        // which is the format of paths in 'jar:file:/' filenames, so they can be compared in the
+        // 'isSource*' methods.
+        // This is done preemptively so we don't have to do the same work repeatedly, and the number
+        // of configured sources is typically very low.
+        sourceUris = new ArrayList<>(sources.size());
+        for (Path source : sources) {
+            sourceUris.add(source.toUri().toString());
+        }
     }
 
     /**
@@ -223,6 +234,21 @@ private boolean isSource(FromSourceLocation sourceLocation) {
         String location = sourceLocation.getSourceLocation().getFilename();
         int offsetFromStart = findOffsetFromStart(location);
 
+        if (offsetFromStart > 0) {
+            // Offset means the filename is a URI, so compare to the URI paths to account for encoding and windows.
+            for (String sourceUri : sourceUris) {
+                // Compare starting after the protocol (the source uri will always be "file", because we created it
+                // from a path).
+                int regionCompareLength = sourceUri.length() - 5;
+                if (location.regionMatches(offsetFromStart, sourceUri, 5, regionCompareLength)) {
+                    return true;
+                }
+            }
+
+            return false;
+        }
+
+        // Filename is a regular path, so we can compare to sources directly.
         for (Path path : sources) {
             String pathString = path.toString();
             int offsetFromStartInSource = findOffsetFromStart(pathString);

smithy-build/src/test/java/software/amazon/smithy/build/plugins/SourcesPluginTest.java

@@ -10,9 +10,16 @@
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.not;
 
+import java.io.File;
+import java.io.IOException;
 import java.net.URISyntaxException;
+import java.nio.file.Files;
+import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.Comparator;
+import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import software.amazon.smithy.build.MockManifest;
 import software.amazon.smithy.build.PluginContext;
@@ -26,6 +33,19 @@
 import software.amazon.smithy.utils.ListUtils;
 
 public class SourcesPluginTest {
+
+    private Path tempDirectory;
+
+    @BeforeEach
+    public void before() throws IOException {
+        tempDirectory = Files.createTempDirectory(getClass().getSimpleName());
+    }
+
+    @AfterEach
+    public void after() throws IOException {
+        Files.walk(tempDirectory).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
+    }
+
     @Test
     public void copiesFilesForSourceProjection() throws URISyntaxException {
         Model model = Model.assembler()
@@ -236,4 +256,69 @@ public void omitsUnsupportedFilesFromManifest() throws URISyntaxException {
         assertThat(manifestString, containsString("a.smithy\n"));
         assertThat(manifestString, not(containsString("foo.md")));
     }
+
+    @Test
+    public void copiesModelFromJarWithEncodedPathWithSourceProjection()
+            throws IOException, URISyntaxException {
+        Path source = Paths.get(getClass().getResource("sources/jar-import.jar").toURI());
+        Path jarPath = tempDirectory.resolve(Paths.get("special%45path", "special.jar"));
+
+        Files.createDirectories(jarPath.getParent());
+        Files.copy(source, jarPath);
+
+        Model model = Model.assembler()
+                .addImport(jarPath)
+                .addImport(getClass().getResource("notsources/d.smithy"))
+                .assemble()
+                .unwrap();
+        MockManifest manifest = new MockManifest();
+        PluginContext context = PluginContext.builder()
+                .fileManifest(manifest)
+                .model(model)
+                .originalModel(model)
+                .sources(ListUtils.of(jarPath))
+                .build();
+        new SourcesPlugin().execute(context);
+        String manifestString = manifest.getFileString("manifest").get();
+
+        assertThat(manifestString, containsString("special/a.smithy\n"));
+        assertThat(manifestString, containsString("special/b/b.smithy\n"));
+        assertThat(manifestString, containsString("special/b/c/c.json\n"));
+        assertThat(manifestString, not(containsString("jar-import/d.json")));
+        assertThat(manifest.getFileString("special/a.smithy").get(), containsString("string A"));
+        assertThat(manifest.getFileString("special/b/b.smithy").get(), containsString("string B"));
+        assertThat(manifest.getFileString("special/b/c/c.json").get(), containsString("\"foo.baz#C\""));
+    }
+
+    @Test
+    public void copiesModelFromJarWithEncodedPathWithNonSourceProjection()
+            throws IOException, URISyntaxException {
+        Path source = Paths.get(getClass().getResource("sources/jar-import.jar").toURI());
+        Path jarPath = tempDirectory.resolve(Paths.get("special%45path", "special.jar"));
+
+        Files.createDirectories(jarPath.getParent());
+        Files.copy(source, jarPath);
+
+        Model model = Model.assembler()
+                .addImport(jarPath)
+                .assemble()
+                .unwrap();
+        MockManifest manifest = new MockManifest();
+        ProjectionConfig projection = ProjectionConfig.builder().build();
+        PluginContext context = PluginContext.builder()
+                .fileManifest(manifest)
+                .projection("foo", projection)
+                .model(model)
+                .originalModel(model)
+                .sources(ListUtils.of(jarPath))
+                .build();
+        new SourcesPlugin().execute(context);
+        String manifestString = manifest.getFileString("manifest").get();
+
+        assertThat(manifestString, containsString("model.json"));
+        assertThat(manifestString, not(containsString("special")));
+        assertThat(manifest.getFileString("model.json").get(), containsString("\"foo.baz#A\""));
+        assertThat(manifest.getFileString("model.json").get(), containsString("\"foo.baz#B\""));
+        assertThat(manifest.getFileString("model.json").get(), containsString("\"foo.baz#C\""));
+    }
 }

smithy-model/src/main/java/software/amazon/smithy/model/loader/ModelDiscovery.java

@@ -10,9 +10,11 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
+import java.net.URI;
 import java.net.URL;
 import java.net.URLConnection;
 import java.nio.charset.StandardCharsets;
+import java.nio.file.Paths;
 import java.util.ArrayList;
 import java.util.Enumeration;
 import java.util.LinkedHashSet;
@@ -180,27 +182,36 @@ public static String getSmithyModelPathFromJarUrl(URL modelUrl) {
      * to a file, (e.g., "/foo/baz.jar"), a file URL (e.g., "file:/baz.jar"),
      * or a JAR URL (e.g., "jar:file:/baz.jar").
      *
+     * <p>If {@code fileOrUrl} is a URL, it must be absolute and percent-encoded
+     * to ensure the manifest is read from the correct location.
+     *
      * @param fileOrUrl Filename or URL that points to a JAR.
      * @return Returns the computed URL.
      */
     public static URL createSmithyJarManifestUrl(String fileOrUrl) {
         try {
-            return new URL(getFilenameWithScheme(fileOrUrl) + "!/" + MANIFEST_PATH);
+            // URL expects callers to perform percent-encoding before construction,
+            // and for consumers to perform decoding. When given a URL-like string,
+            // (e.g. file:/foo.jar), we have to assume it is properly encoded because
+            // otherwise we risk double-encoding. For file paths, we have to encode
+            // to make sure that when the consumer of the URL performs decoding (i.e.
+            // to read from the file system), they will get back the original path.
+            String jarUrl;
+            if (fileOrUrl.startsWith("jar:")) {
+                jarUrl = fileOrUrl;
+            } else if (fileOrUrl.startsWith("file:")) {
+                jarUrl = "jar:" + fileOrUrl;
+            } else {
+                URI jarUri = Paths.get(fileOrUrl).toUri();
+                jarUrl = "jar:" + jarUri;
+            }
+
+            return new URL(jarUrl + "!/" + MANIFEST_PATH);
         } catch (IOException e) {
             throw new ModelImportException(e.getMessage(), e);
         }
     }
 
-    private static String getFilenameWithScheme(String filename) {
-        if (filename.startsWith("jar:")) {
-            return filename;
-        } else if (filename.startsWith("file:")) {
-            return "jar:" + filename;
-        } else {
-            return "jar:file:" + filename;
-        }
-    }
-
     private static Set<String> parseManifest(URL location) throws IOException {
         Set<String> models = new LinkedHashSet<>();
         URLConnection connection = location.openConnection();

smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelAssemblerTest.java

@@ -1459,4 +1459,57 @@ public void handlesSameModelWhenBuiltAndImported() throws Exception {
         assertTrue(combinedModel.expectShape(ShapeId.from("smithy.example#MachineData$machineId"), MemberShape.class)
                 .hasTrait(RequiredTrait.ID));
     }
+
+    @Test
+    public void importsJarFromPathWithEncodedChars() throws Exception {
+        Path source = Paths.get(getClass().getResource("assembler-valid-jar").toURI());
+        Path jarPath = outputDirectory.resolve(Paths.get("path%20with%45encoded%25chars", "test.jar"));
+
+        Files.createDirectories(jarPath.getParent());
+        Files.copy(JarUtils.createJarFromDir(source), jarPath);
+
+        Model model = Model.assembler()
+                .addImport(jarPath)
+                .assemble()
+                .unwrap();
+
+        assertTrue(model.getShape(ShapeId.from("smithy.example#ExampleString")).isPresent());
+    }
+
+    @Test
+    public void importsJarFromUrlWithEncodedChars() throws Exception {
+        Path source = Paths.get(getClass().getResource("assembler-valid-jar").toURI());
+        Path jarPath = outputDirectory.resolve(Paths.get("path%20with%45encoded%25chars", "test.jar"));
+
+        Files.createDirectories(jarPath.getParent());
+        Files.copy(JarUtils.createJarFromDir(source), jarPath);
+
+        Model model = Model.assembler()
+                .addImport(jarPath.toUri().toURL())
+                .assemble()
+                .unwrap();
+
+        assertTrue(model.getShape(ShapeId.from("smithy.example#ExampleString")).isPresent());
+    }
+
+    @Test
+    public void importedJarsWithEncodedCharsHaveCorrectFilename() throws Exception {
+        Path source = Paths.get(getClass().getResource("assembler-valid-jar").toURI());
+        Path jarPath = outputDirectory.resolve(Paths.get("path%20with%45encoded%25chars", "test.jar"));
+
+        Files.createDirectories(jarPath.getParent());
+        Files.copy(JarUtils.createJarFromDir(source), jarPath);
+
+        Model model = Model.assembler()
+                .addImport(jarPath)
+                .assemble()
+                .unwrap();
+
+        String filename = model.expectShape(ShapeId.from("smithy.example#ExampleString"))
+                .getSourceLocation()
+                .getFilename();
+        String fileContents = IoUtils.readUtf8Url(new URL(filename));
+
+        assertThat(fileContents, containsString("string ExampleString"));
+    }
 }

smithy-model/src/test/java/software/amazon/smithy/model/loader/ModelDiscoveryTest.java

@@ -13,6 +13,7 @@
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLClassLoader;
+import java.nio.file.Paths;
 import java.util.List;
 import java.util.stream.Collectors;
 import org.junit.jupiter.api.Assertions;
@@ -132,8 +133,10 @@ public void requiresModelNameToBeValidWhenParsing() throws IOException {
 
     @Test
     public void createSmithyManifestUrlFromPath() throws IOException {
+        // Accounts for windows drive letters and path separators
+        String normalizedJarPath = Paths.get("/foo.jar").toUri().getPath();
         assertThat(ModelDiscovery.createSmithyJarManifestUrl("/foo.jar"),
-                equalTo(new URL("jar:file:/foo.jar!/META-INF/smithy/manifest")));
+                equalTo(new URL("jar:file:" + normalizedJarPath + "!/META-INF/smithy/manifest")));
     }
 
     @Test
@@ -147,4 +150,19 @@ public void createSmithyManifestUrlFromJarUrl() throws IOException {
         assertThat(ModelDiscovery.createSmithyJarManifestUrl("jar:file:/foo.jar"),
                 equalTo(new URL("jar:file:/foo.jar!/META-INF/smithy/manifest")));
     }
+
+    @Test
+    public void encodesSmithyManifestUrlFromPath() throws IOException {
+        String normalizedAndEncodedJarPath = Paths.get("/foo bar.jar").toUri().getRawPath();
+        assertThat(ModelDiscovery.createSmithyJarManifestUrl("/foo bar.jar"),
+                equalTo(new URL("jar:file:" + normalizedAndEncodedJarPath + "!/META-INF/smithy/manifest")));
+    }
+
+    @Test
+    public void preservesEncodingOfSmithyManifestUrlFromUrl() throws IOException {
+        assertThat(ModelDiscovery.createSmithyJarManifestUrl("file:/foo%20bar.jar"),
+                equalTo(new URL("jar:file:/foo%20bar.jar!/META-INF/smithy/manifest")));
+        assertThat(ModelDiscovery.createSmithyJarManifestUrl("jar:file:/foo%20bar.jar"),
+                equalTo(new URL("jar:file:/foo%20bar.jar!/META-INF/smithy/manifest")));
+    }
 }

smithy-model/src/test/resources/software/amazon/smithy/model/loader/assembler-valid-jar/META-INF/MANIFEST.MF

@@ -0,0 +1,2 @@
+Manifest-Version: 1.0
+Created-By: 11.0.6 (Amazon.com Inc.)

smithy-model/src/test/resources/software/amazon/smithy/model/loader/assembler-valid-jar/META-INF/smithy/manifest

@@ -0,0 +1 @@
+valid.smithy

smithy-model/src/test/resources/software/amazon/smithy/model/loader/assembler-valid-jar/META-INF/smithy/valid.smithy

@@ -0,0 +1,5 @@
+$version: "2.0"
+
+namespace smithy.example
+
+string ExampleString