swagger-parser icon indicating copy to clipboard operation
swagger-parser copied to clipboard

Relative $refs fail on Windows due to potential issue in ExternalRefProcessor#processRefToExternalSchema

Open kerhard opened this issue 2 years ago • 4 comments

Hi, I have some $ref references in my OpenApi specification that work fine on Linux, but fail on Windows.

The problem seems to be in ExternalRefProcessor#processRefToExternalSchema (https://github.com/swagger-api/swagger-parser/blob/master/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java#L127) within the following code:

String parent = (file.contains("/")) ? file.substring(0, file.lastIndexOf('/')) : "";
if (!parent.isEmpty()) {
	if (schemaFullRef.contains("#/")) {
		String[] parts = schemaFullRef.split("#/");
		String schemaFullRefFilePart = parts[0];
		String schemaFullRefInternalRefPart = parts[1];
		schemaFullRef = Paths.get(parent, schemaFullRefFilePart).normalize().toString() + "#/" + schemaFullRefInternalRefPart;
	} else {
		schemaFullRef = Paths.get(parent, schemaFullRef).normalize().toString();
	}
}

In the first line the parent folder is determined by cutting the path at the last forward slash. "openapi/schemas/model.yaml" = > "openapi/schemas"

But the later Paths.get invocation will replace any forward slashes to backslashes on Windows. Paths.get("openapi/schemas", "ref.yaml").normalize().toString() => "openapi\schemas\ref.yaml"

The replaced path with backslashes seems to then be used for any further embedded references. So even when I am only using forward slashes in my openapi files, the call to Paths.get will mess up the $ref resolution eventually.

Proposal: Replace the backslashes with forward slashes after Paths.get is used or make the parent folder determination platform idependent.

kerhard avatar Feb 15 '23 15:02 kerhard

By:

embedded references

Are you relying on $ref's pointing to items on the classpath where it breaks? Sounds very similar to what I have in #1878 (+ somewhat of a fix in #1879)

i.e. Path with backslashes after Paths.get(..) is fine when attempting to locate via the Windows filesystem, but once it attempts to retrieve via the classpath will always fail.

adrianhj avatar Feb 16 '23 10:02 adrianhj

I am loading from the filesystem, not from the classpath. The check from https://github.com/swagger-api/swagger-parser/blob/master/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/util/RefUtils.java#L190 returns false in my case , because the combined (empty parent) path was determined incorrectly in ExternalRefProcessor. The PR from the other issues doesn't fix my issue.

i.e. Path with forward slashes after Paths.get(..) is fine when attempting to locate via the Windows filesystem, but once it attempts to retrieve via the classpath will always fail.

Yes, but it will also fail to determine the parent folder the next time (when resolved references contain other references) the statement (String parent = (file.contains("/")) ? file.substring(0, file.lastIndexOf('/')) : "";) is executed, because there are no longer forward slashes present and it will just use an empty string as parent.

At least for me the following patch using FilenameUtils.separatorsToUnix seems to work fine:

diff --git a/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java b/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java
index 286f9cca..39110785 100644
--- a/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java
+++ b/modules/swagger-parser-v3/src/main/java/io/swagger/v3/parser/processors/ExternalRefProcessor.java
@@ -29,6 +29,7 @@ import io.swagger.v3.oas.models.security.SecurityScheme;
 import io.swagger.v3.parser.ResolverCache;
 import io.swagger.v3.parser.models.RefFormat;
 import io.swagger.v3.parser.models.RefType;
+import org.apache.commons.io.FilenameUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.slf4j.LoggerFactory;

@@ -124,6 +125,7 @@ public final class ExternalRefProcessor {
                 if (isAnExternalRefFormat(ref)) {
                     if (!ref.equals(RefFormat.URL)) {
                         String schemaFullRef = schema.get$ref();
+                        file = FilenameUtils.separatorsToUnix(file);
                         String parent = (file.contains("/")) ? file.substring(0, file.lastIndexOf('/')) : "";
                         if (!parent.isEmpty()) {
                             if (schemaFullRef.contains("#/")) {
@@ -406,6 +408,7 @@ public final class ExternalRefProcessor {
                 if (isAnExternalRefFormat(format)) {
                     String fullRef = response.get$ref();
                     if (!format.equals(RefFormat.URL)) {
+                        file = FilenameUtils.separatorsToUnix(file);
                         String parent = file.substring(0, file.lastIndexOf('/'));
                         if (!parent.isEmpty()) {
                             if (fullRef.contains("#/")) {
@@ -793,6 +796,7 @@ public final class ExternalRefProcessor {
                 if (isAnExternalRefFormat(format)) {
                     String fullRef = parameter.get$ref();
                     if (!format.equals(RefFormat.URL)) {
+                        file = FilenameUtils.separatorsToUnix(file);
                         String parent = file.substring(0, file.lastIndexOf('/'));
                         if (!parent.isEmpty()) {
                             if (fullRef.contains("#/")) {

kerhard avatar Feb 16 '23 11:02 kerhard

Hi, we have merged this PR https://github.com/swagger-api/swagger-parser/pull/1879, please git it a try to see if that fixes your issue. @kerhard

gracekarina avatar Feb 28 '23 01:02 gracekarina

This issue impacts other open source project (see https://github.com/Backbase/backbase-openapi-tools/issues/600) @gracekarina @adrianhj could you please take a look on my PR? Thanks

walaniam avatar Aug 16 '23 07:08 walaniam