quarkiverse / quarkus-quinoa

Quinoa is a Quarkus extension which eases the development, the build and serving of single page apps (built with NodeJS: React, Angular, …) alongside Quarkus . It is possible to use it with a Quarkus backend in a single project
Apache License 2.0
75 stars 34 forks source link

SvelteKit live dev with dynamic routing fails #591

Open devpikachu opened 7 months ago

devpikachu commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

quarkus dev -Dvertx.disableURIValidation=true

image

melloware commented 7 months ago

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

ia3andy commented 7 months ago

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

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 commented 7 months ago

cc @vietj

geoand commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

I'll create an issue on Quarkus..

@devpikachu so this means:

devpikachu commented 7 months ago

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: https://github.com/quarkusio/quarkus/issues/37789

ia3andy commented 7 months ago

Here is the underlying issue: https://github.com/quarkusio/quarkus/issues/37804

geoand commented 7 months ago

I'll take a look

melloware commented 6 months ago

@all-contributors add @devpikachu for bug

allcontributors[bot] commented 6 months ago

@melloware

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

melloware commented 5 months ago

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

treo commented 5 months ago

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

melloware commented 4 months ago

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

treo commented 4 months ago

At the moment this is only documented in this particular issue. https://github.com/quarkiverse/quarkus-quinoa/pull/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 commented 4 months ago

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