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

use:enhance compatibility on form whose action isn't a SvelteKit action #10855

Open
eltigerchino opened this issue Oct 10, 2023 · 12 comments · May be fixed by #13197
Open

use:enhance compatibility on form whose action isn't a SvelteKit action #10855

eltigerchino opened this issue Oct 10, 2023 · 12 comments · May be fixed by #13197
Labels
feature / enhancement New feature or request
Milestone

Comments

@eltigerchino
Copy link
Member

eltigerchino commented Oct 10, 2023

Describe the problem

An unhelpful error message presents itself when we have a form with use:enhance posting to an ordinary POST endpoint instead of a SvelteKit action. This occurs because the enhance action tries to parse the fetch response body as a JSON. If that succeeds, it looks for a data property and attempts to parse it as JSON too, which can throw an error. If there is no data property, submitting the form results in nothing happening, which can be confusing.

https://stackblitz.com/edit/sveltejs-kit-template-default-nmxvlc?file=src%2Froutes%2F%2Blayout.svelte

Describe the proposed solution

Maybe we can handle the outcome better or have a dev-only warning when this occurs?

Alternatives considered

Document that use:enhance should only be used with SvelteKit actions

Importance

nice to have

Additional Information

No response

@eltigerchino eltigerchino added the feature / enhancement New feature or request label Oct 10, 2023
@eltigerchino eltigerchino added this to the non-urgent milestone Oct 10, 2023
@eltigerchino eltigerchino changed the title use:enhance on form whose action isn't a SvelteKit action use:enhance compatibility on form whose action isn't a SvelteKit action Oct 10, 2023
@Rishab49
Copy link

strange that when i downloaded the code and used it inside playgrounds by replacing the @sveltejs/kit and adapter-node versions with workspace:* , the page got redirected to /somewhere instead of showing the error on same page.

@ITenthusiasm
Copy link

ITenthusiasm commented Jan 10, 2024

Yes! Can we please go with the option of "Alternatively, we could handle the non-JSON responses better" (mentioned in the StackBlitz link). For me, this actually is somewhat urgent, not a nice-to-have. Here's the background of my use case:

I'm working with an open source authentication solution called SuperTokens, and I'm intentionally doing auth on the backend (not the fronted) because it's more secure. In the authentication model of SuperTokens, they have short-lived Access Tokens that can be refreshed with long-lived Refreshed Tokens when the user makes requests to a server. When supporting users with or without JS in Remix, I'm able to handle this model just fine. In SvelteKit, things break. Here's the specific scenario that breaks...

Note: The /auth/session/refresh route is a regular API route since there's no <form> associated with it.

The Scenario

Imagine a user is on a page that displays new data on the current screen depending on what they submit to the server. This is a perfect use case for use:enhance. Everything will work fine for a non-JS user. And JS users will be able to see the new data on the current screen without a page refresh. However, this page requires authentication.

Where SvelteKit Works

Imagine a non-JS user is on this webpage. They submit a form with an expired Access Token and a valid Refresh Token. (Note that all tokens are expressed as HTTP-only cookies.) Here's what will happen:

  1. The server (with the help of SuperTokens) will see that the Access Token is expired, but a Refresh Token is present. It will redirect with a 307 to /auth/session/refresh. (This is the only route where Refresh Token HTTP-only cookies can be used and/or created, for security reasons. So there's no point in attempting a refresh on any other route.)
  2. The server (with the help of SuperTokens) will see that a valid Refresh Token is present at the /auth/session/refresh route, so a new Access Token and a new Refresh Token (for security reasons) will be returned to the user as HTTP-only cookies. It will redirect back the the original POST route with a 307.
  3. Because the non-JS users was redirected back to the original page with a 307, the POST will continue as normal. Since they are now authenticated with a valid Access Token, they will get to see the new data on the screen without having to inconveniently resubmit their form data.

Where SvelteKit Fails

Now imagine a JS user is on this webpage. They submit a form with an expired Access Token and an expired Refresh Token. Here's what will happen:

  1. The server (with the help of SuperTokens) will see that the Access Token is expired, but a Refresh Token is present. It will redirect with a 307 to /auth/session/refresh.
  2. The server (with the help of SuperTokens) will see that a valid Refresh Token is not present at the /auth/session/refresh route. So all of the user's Auth Cookies will be deleted by the server. Additionally, the user will be redirected to the /login page with a 303. (By redirecting with a 303, there's no risk of inappropriately POST-ing to the /login route. This also removes the unlikely-but-possible scenario where someone could accidentally re-authenticate as a different user during the redirect because of the values in the original form POST.)
  3. Because the JS user was redirected to the the /login page with a 303, the the page's HTML is returned.
  4. SvelteKit errors out (as shown in the StackBlitz link).

@JonathonRP
Copy link

I've run into an issue today with redirect form actions and setting headers. I thought form actions would work like post server endpoint and returning a response with redirect settings would redirect but no get an error because form actions only expects data to return and using set headers redirect doesn't keep headers.

So it would be nice even with actions to be able to return redirect responses that are not data to or from form.

@eltigerchino
Copy link
Member Author

I've run into an issue today with redirect form actions and setting headers. I thought form actions would work like post server endpoint and returning a response with redirect settings would redirect but no get an error because form actions only expects data to return and using set headers redirect doesn't keep headers.

So it would be nice even with actions to be able to return redirect responses that are not data to or from form.

Do you mind elaborating on this with a code example?

@JonathonRP
Copy link

I've run into an issue today with redirect form actions and setting headers. I thought form actions would work like post server endpoint and returning a response with redirect settings would redirect but no get an error because form actions only expects data to return and using set headers redirect doesn't keep headers.

So it would be nice even with actions to be able to return redirect responses that are not data to or from form.

Do you mind elaborating on this with a code example?

I can make a repo later and share, no problem.

@ITenthusiasm
Copy link

@JonathonRP is the issue that you're talking about similar to what's shown in the StackBlitz link in the OP? Or is it something different?

@JonathonRP
Copy link

@ITenthusiasm similar, I'm using form actions with use enhance and also not working if I return a response that doesn't have data prop or is sveltekit redirect(), I'm attempting to have redirect and settings authorization header.

@JonathonRP
Copy link

JonathonRP commented Feb 21, 2024

@ITenthusiasm
Copy link

ITenthusiasm commented Oct 31, 2024

I know this has been marked as non-urgent, but I still consider this a pretty valuable feature. It's more or less necessary for an authenticated application as I described earlier.

Would it be possible for SvelteKit to simply read the response type (or some custom headers) to see if it should navigate to the provided URL instead of trying to unwrap some JSON? I believe Remix does something like this.

ITenthusiasm added a commit to ITenthusiasm/svelte-kit-supertokens that referenced this issue Dec 6, 2024
We documented our findings in `NOTES.md` at:
ITenthusiasm/remix-supertokens@2726466

NOTE: Due to limitations in SvelteKit, there are some tests that
won't work. Specifically, any scenario which tests that an HTML page
is returned in response to a form submission (with `use:enhance`)
will fail. (This can happen, for example, if a user is redirected
to a login page during form submission because their session expired.)

Other frameworks like Remix do not have this problem. The Svelte
team seems to be aware of this issue, though its priority is
unknown. Track sveltejs/kit#10855 for additional details.
@Mardoxx
Copy link

Mardoxx commented Dec 18, 2024

What a pain in the proverbial this has been today....

+page.svelte

<form method="post" action="/logout" use:enhance>
  <button>Sign out</button>
</form>

logout/+server.js

export const POST: RequestHandler = (event: RequestEvent) => {
  // do the needful
  return new Response(null, {
    status: 302,
    headers: {
      Location: "/"
    }
  });
};

use:enhance should only be used with SvelteKit actions should be in big red text, not in some year old github issue.

Another oddity, I shall make a repro for this, if you have

+page.svelte

<a href="/login/google">Sign in with Google</a>

login/blah/+server.js

export const GET: RequestHandler = (event: RequestEvent) => {
  // do the needful
  return new Response(null, {
    status: 302,
    headers: {
      Location: "/"
    }
  });
};

When you click the link you get a nice little

Error: Not found: /login/blah
    SvelteKitError XXXXXXXXX/node_modules/.pnpm/@[email protected]_@[email protected][email protected][email protected]_@types+node_XXXXXXXX/node_modules/@sveltejs/kit/src/runtime/control.js?v=XXXX:45

Which comes from here presumably because the route does not exist /as a page/

@Mardoxx
Copy link

Mardoxx commented Dec 18, 2024

https://stackblitz.com/edit/sveltejs-kit-template-default-ipxflmtx

Here's an up to date repro of both

@eltigerchino
Copy link
Member Author

eltigerchino commented Dec 26, 2024

Note: The /auth/session/refresh route is a regular API route since there's no

associated with it.

Wouldn't it work if you implemented it as a page action instead of using +server?

I've run into an issue today with redirect form actions and setting headers. I thought form actions would work like post server endpoint and returning a response with redirect settings would redirect but no get an error because form actions only expects data to return and using set headers redirect doesn't keep headers.

So it would be nice even with actions to be able to return redirect responses that are not data to or from form.

The only thing you should return from a SvelteKit form action is a plain object or optionally using the fail helper. If you want to redirect, you should always use the redirect helper.

See https://svelte.dev/docs/kit/form-actions for more information.

I'm attempting to have redirect and settings authorization header.

You can still set the headers using event.setHeaders before you redirect. For example:

import { redirect } from "@sveltejs/kit";

export const actions = {
  default: async ({ setHeaders }) => {
    setHeaders({ token: "1" });
    redirect(303, "/dashboard");
  },
};

Would it be possible for SvelteKit to simply read the response type (or some custom headers) to see if it should navigate to the provided URL instead of trying to unwrap some JSON? I believe Remix does something like this.

It's tricky. A redirect causes the fetch to respond with the result of fetching the location header's URL rather than the original endpoint so we can't just check the status code. We could try checking the request URL against the action URL and if it's different, we've probably been redirected, but it feels somewhat hacky:

if (response.url !== action.toString()) {
	result = { type: 'redirect', status: response.status, location: response.url };
}

We'd need to add this check in between the fetch and the deserialise:

const response = await fetch(action, {
method: 'POST',
headers,
cache: 'no-store',
body,
signal: controller.signal
});
result = deserialize(await response.text());

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature / enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants