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

fix: fix client requests with windows. #12279

Closed
wants to merge 6 commits into from

Conversation

caalador
Copy link
Contributor

@caalador caalador commented Nov 5, 2021

Windows vite requests will
contain the path with the drive
which contains : so it cannot be
encoded to %3A as then the
devServer returns a 404

@caalador caalador added the vite Tickets related to vite support label Nov 5, 2021
@caalador caalador requested a review from Artur- November 5, 2021 11:12
@caalador caalador added this to In progress in Frontend build optimization via automation Nov 5, 2021
Artur-
Artur- previously approved these changes Nov 5, 2021
@Artur-
Copy link
Member

Artur- commented Nov 5, 2021

We really really should not be using URLEncoder.encode here. It should be something equivalent to encodeURI in JavaScript:
"
encodeURI() escapes all characters except:

Not Escaped:

A-Z a-z 0-9 ; , / ? : @ & = + $ - _ . ! ~ * ' ( ) #

"

Windows vite requests will
contain the path with the drive
which contains `:` so it cannot be
encoded to `%3A` as then the
devServer returns a 404
Artur-
Artur- previously approved these changes Nov 5, 2021
@@ -695,7 +695,10 @@ public boolean serveDevModeRequest(HttpServletRequest request,
requestFilename = "/VAADIN/static" + requestFilename;
}

String devServerRequestPath = UrlUtil.encodeURI(requestFilename);
// Dev server request can have an absolute system path making
// it required to not encode ':'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the wrong place to change. URLs are URLs, there is nothing special about the dev server?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well then the "fixed" url can not be used with Location as "abc:foo/bar" will read as has scheme so it's relative which is not accepted for a location. Wasn't this change made just to support spaces?

@@ -51,7 +53,7 @@ public static void verifyRelativePath(String path) {
// Ignore forbidden chars supported in route definitions
String strippedPath = path.replaceAll("[{}*]", "");

URI uri = new URI(UrlUtil.encodeURI(strippedPath));
URI uri = new URI(URLEncoder.encode(strippedPath, UTF_8.name()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also wrong. What are you trying to fix here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks to me like this is using the wrong URI constructor. The parameter passed is not a full URL but a path and an appropriate constructor should be used

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it should use public URI(String scheme, String host, String path, String fragment) with everything except the path parameter set to null

@caalador caalador force-pushed the fix/fix_request_with_windows_path branch from 769c81e to 55d2f44 Compare November 5, 2021 12:31
@vaadin-bot
Copy link
Collaborator

SonarQube analysis reported 2 issues

  1. MINOR LocationUtil.java#L173: Replace this if-then-else statement by a single return statement. rule
  2. MINOR LocationUtil.java#L173: Move the ".." string literal on the left side of this string comparison. rule

@Artur-
Copy link
Member

Artur- commented Nov 5, 2021

An alternative fix that tries to address all potential problems in #12291

@caalador caalador removed this from In progress in Frontend build optimization Nov 8, 2021
@caalador caalador added this to the Abandoned milestone Nov 8, 2021
@caalador caalador closed this Nov 8, 2021
@caalador caalador deleted the fix/fix_request_with_windows_path branch November 8, 2021 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vite Tickets related to vite support +0.0.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants