Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
kerhard opened this issue Feb 15, 2023 · 4 comments · Fixed by #2064
Assignees

Comments

@kerhard
Copy link

kerhard commented Feb 15, 2023

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.

@adrianhj
Copy link
Contributor

adrianhj commented Feb 16, 2023

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.

@kerhard
Copy link
Author

kerhard commented Feb 16, 2023

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("#/")) {

@gracekarina
Copy link
Contributor

gracekarina commented Feb 28, 2023

Hi, we have merged this PR #1879, please git it a try to see if that fixes your issue. @kerhard

@walaniam
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants