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

SvelteKit live dev with dynamic routing fails #591

Closed
devpikachu opened this issue Dec 16, 2023 · 20 comments · Fixed by #737
Closed

SvelteKit live dev with dynamic routing fails #591

devpikachu opened this issue Dec 16, 2023 · 20 comments · Fixed by #737
Assignees
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Milestone

Comments

@devpikachu
Copy link

Describe the bug

Having added a SvelteKit 2 app (running with Vite 5) and then a dynamic route to it (routes/activate/[activationToken]/+page.svelte) results in a 500 error being displayed in browser.
image

Inspecting the Network tab, it seems like there's 400 errors being thrown when Svelte tries to load the code for the dynamic route:
image

Looking at the CLI output of Quarkus, it seems like Quinoa is not intercepting these 2 calls at all, which leads me to believe it is Quinoa returning these 400 responses. As a note, I set the logging level to TRACE and found no indication of these 2 calls being handled by the server.

Having tried running Vite directly (pnpm run dev), the page can be accessed as normal and it works just fine.

I've already did the config change as per #574 .
image

Calling Quarkus:

curl -v 'http://localhost:8080/src/routes/activate/\[activationToken\]/+page.ts'
* processing: http://localhost:8080/src/routes/activate/[activationToken]/+page.ts
*   Trying [::1]:8080...
* connect to ::1 port 8080 failed: Connection refused
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080
> GET /src/routes/activate/[activationToken]/+page.ts HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/8.2.1
> Accept: */*
> 
< HTTP/1.1 400 Bad Request
< content-length: 0
< 
* Connection #0 to host localhost left intact

Calling Vite directly:

curl -v 'http://localhost:5173/src/routes/activate/\[activationToken\]/+page.ts'
* processing: http://localhost:5173/src/routes/activate/[activationToken]/+page.ts
*   Trying [::1]:5173...
* Connected to localhost (::1) port 5173
> GET /src/routes/activate/[activationToken]/+page.ts HTTP/1.1
> Host: localhost:5173
> User-Agent: curl/8.2.1
> Accept: */*
> 
< HTTP/1.1 200 OK
< Access-Control-Allow-Origin: *
< Content-Type: application/javascript
< Cache-Control: no-cache
< Etag: W/"16a-ntZf16MXhkhvJbKqOZZm+fpwNDQ"
< Date: Sat, 16 Dec 2023 15:46:57 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< Content-Length: 1037
< 
import { redirect } from "/node_modules/.pnpm/@sveltejs+kit@2.0.0_@sveltejs+vite-plugin-svelte@3.0.1_svelte@4.2.8_vite@5.0.10/node_modules/@sveltejs/kit/src/exports/index.js?v=1905be8b";
export const load = async ({ fetch, params }) => {
  const regex = /^[0-9a-fA-F]{32}$/;
  if (!regex.test(params.activationToken)) {
    throw redirect(301, "/login");
  }
};

//# sourceMappingURL=data:application/json;base64,eyJ2ZXJzaW9uIjozLCJzb3VyY2VzIjpbIitwYWdlLnRzIl0sInNvdXJjZXNDb250ZW50IjpbImltcG9ydCB7IHJlZGlyZWN0IH0gZnJvbSAnQHN2ZWx0ZWpzL2tpdCc7XG5cbmV4cG9ydCBjb25zdCBsb2FkID0gYXN5bmMgKHsgZmV0Y2gsIHBhcmFtcyB9KSA9PiB7XG5cdGNvbnN0IHJlZ2V4ID0gL15bMC05YS1mQS1GXXszMn0kLztcblxuXHRpZiAoIXJlZ2V4LnRlc3QocGFyYW1zLmFjdGl2YXRpb25Ub2tlbikpIHtcblx0XHR0aHJvdyByZWRpcmVjdCgzMDEsICcvbG9naW4nKTtcblx0fVxufTtcbiJdLCJtYXBwaW5ncyI6IkFBQUEsU0FBUyxnQkFBZ0I7QUFFbEIsYUFBTSxPQUFPLE9BQU8sRUFBRSxPQUFPLE9BQU8sTUFBTTtBQUNoRCxRQUFNLFFBQVE7QUFFZCxNQUFJLENBQUMsTUFBTSxLQUFLLE9BQU8sZUFBZSxHQUFHO0FBQ3hDLFVBQU0sU0FBUyxLQUFLLFFBQVE7QUFBQSxFQUM3QjtBQUNEOyIsIm5* Connection #0 to host localhost left intact
hbWVzIjpbXX0=

Currently the only workaround to this is setting quarkus.quinoa.dev-server to false and running Vite separately, which defeats the purpose of Quinoa's DX during development.

Quinoa version

2.2.1

Quarkus version

3.6.3

Build / Runtime

Vite

Package Manager

PNPM

Steps to reproduce the behavior

  1. Create a Quarkus & Quinoa project
  2. Add a SvelteKit project to it and follow docs for disabling SSR
  3. Add a dynamic route (routes/[slug]/+page.svelte)
  4. Attempt to access the route (http://localhost:8080/test-slug)

Expected behavior

The page is loaded as normal

@devpikachu
Copy link
Author

devpikachu commented Dec 16, 2023

Upon further investigation I've found the root cause, and it seems like this is a limitation within Java itself.

The 400 status is set here: https://github.com/quarkusio/quarkus/blob/7cf3e4e8f484aefed9ea97b08ebb2164093baa4e/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java#L122

if (!uriValid(httpServerRequest)) {
    httpServerRequest.response().setStatusCode(400).end();
    return;
}

The uriValid method only constructs a new URI object:

private boolean uriValid(HttpServerRequest httpServerRequest) {
    if (DISABLE_URI_VALIDATION) {
        return true;
    }
    try {
        // we simply need to know if the URI is valid
        new URI(httpServerRequest.uri());
        return true;
    } catch (URISyntaxException e) {
        return false;
    }
}

This can be disabled by setting the vertx.disableURIValidation JVM system property to true, but that's a really bad idea to do in production.

Having said that, I'll do some more tests to see if the production build requires this kind of path - if not, a workaround could be to disable URI validation in dev only.

@devpikachu
Copy link
Author

I can confirm that setting vertx.disableURIValidation to true mitigates this issue in development environments.

quarkus dev -Dvertx.disableURIValidation=true

image

@melloware
Copy link
Contributor

melloware commented Dec 16, 2023

Agreed this is not a good long term solution let's see what @ia3andy says.

@melloware melloware added the bug Something isn't working label Dec 16, 2023
@ia3andy
Copy link
Collaborator

ia3andy commented Dec 18, 2023

This is a tricky one 🤪, my take on this is:

  • If this url is invalid (this needs to be checked), then it's a bad practice from Vite to actually use such a URL and we should investigate ways to provide a fix in Vite itself
  • If this url is valid we should patch vertx to allow it

As a workaround this could maybe be set as a %dev setting in the application.properties. @geoand are the vertx option configurable through the application.properties (if not maybe we should provide a way)?

@ia3andy
Copy link
Collaborator

ia3andy commented Dec 18, 2023

cc @vietj

@geoand
Copy link
Contributor

geoand commented Dec 18, 2023

If this url is valid we should patch vertx to allow it

It's Vertx that is failing, it's a Quarkus specific check that utilizes the code pasted in OP.

If there is some Vertx utility to check the validity of a URI, let's by all means use that.

@ia3andy
Copy link
Collaborator

ia3andy commented Dec 18, 2023

Ok, the URL is not valid in the RFC:
"This URL pattern is commonly used in modern JavaScript frameworks for dynamic routing. In this case, "[activationToken]" is a dynamic segment, meaning it can change based on the data passed to the route. The "+" symbol before "page.svelte" is a special syntax used in Vite to indicate that this is a nested route. However, please note that this URL pattern is specific to the development environment and the routing library you are using. It may not be directly usable as a URL in a production environment or with different routing libraries."

So... the best solution would be to allow it in dev only, or through QUinoa, I wonder if I can set this for the request.

@ia3andy
Copy link
Collaborator

ia3andy commented Dec 18, 2023

I think we should use a quarkus configuration instead of a system property in Quarkus:
https://github.com/quarkusio/quarkus/blob/main/extensions/vertx-http/runtime/src/main/java/io/quarkus/vertx/http/runtime/VertxHttpRecorder.java#L117

This would solve the issue as we could enable this only in dev mode.

Also we could keep backward compat on this with the system props: disabled if Boolean.getBoolean("vertx.disableURIValidation") || fromConfig(quarkus.http.disableURIValidation)

@geoand agreed?

@geoand
Copy link
Contributor

geoand commented Dec 18, 2023

That was really meant to be a hidden flag, but I guess since we now have a valid use case, we can make it a real configuration option

@ia3andy
Copy link
Collaborator

ia3andy commented Dec 18, 2023

I'll create an issue on Quarkus..

@devpikachu so this means:

  • Until the fix is available in Quarkus core, you can use the quarkus dev -Dvertx.disableURIValidation=true as a workaround
  • Then you will be able to use the application.properties with %dev.quarkus.http.disableURIValidation=true (or similar)

@devpikachu
Copy link
Author

Super, thanks for the quick investigation @ia3andy .

FYI, I've also opened an issue on Quarkus' side, more-so for the decoding failing silently rather than throwing to output: quarkusio/quarkus#37789

@ia3andy
Copy link
Collaborator

ia3andy commented Dec 18, 2023

Here is the underlying issue: quarkusio/quarkus#37804

@geoand
Copy link
Contributor

geoand commented Dec 18, 2023

I'll take a look

@melloware
Copy link
Contributor

@all-contributors add @devpikachu for bug

Copy link
Contributor

@melloware

I've put up a pull request to add @devpikachu! 🎉

@melloware
Copy link
Contributor

From Quarkus Team "disableURIValidation is not exposed for a reason: you want to validate URI. It's an attack vector."

@treo
Copy link
Contributor

treo commented Feb 17, 2024

I guess this should then be documented on how to get around it in dev, as it is only a concern there.

@melloware
Copy link
Contributor

@treo is this ticket safe to close now that there are documented workarounds?

@treo
Copy link
Contributor

treo commented Feb 17, 2024

At the moment this is only documented in this particular issue. #640 was for an unrelated problem.

I'm not sure where exactly this particular flag needs to be documented, as I think it isn't necessarily a SvelteKit only thing.

@melloware
Copy link
Contributor

The Quarkus Team has decided not to take action on this as its considered too risky from a security perspective.

@melloware melloware added documentation Improvements or additions to documentation good first issue Good for newcomers and removed bug Something isn't working labels Mar 13, 2024
melloware added a commit to melloware/quarkus-quinoa that referenced this issue Aug 4, 2024
@melloware melloware added this to the 2.4.6 milestone Aug 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants