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

Allow Cookies to Be Set from Headers #8409

Closed
ITenthusiasm opened this issue Jan 9, 2023 · 9 comments
Closed

Allow Cookies to Be Set from Headers #8409

ITenthusiasm opened this issue Jan 9, 2023 · 9 comments

Comments

@ITenthusiasm
Copy link

ITenthusiasm commented Jan 9, 2023

Describe the problem

Currently, Svelte Kit does not support setting cookies via Headers. Instead, it requires cookies to be set via event.cookies. In most cases, this isn't a problem. (It might even be a nicer experience for some.) However, there still may be use cases where the developer would need the freedom to set cookies from Headers.

In my case, I'm trying to encourage an auth solution (SuperTokens) to navigate away from requiring middleware for authentication (which unfortunately a lot of auth solutions do) and to move towards the simplicity of requesting the correct Headers and returning the appropriate response Headers. (Typically, auth solutions really only need to take cookies from headers as inputs -- maybe with some extra data -- and return new cookies in/with headers as outputs -- if any outputs are needed.) This is huge because it satisfies #4654 in a way that seems to avoid going against Rich Harris's philosophies. (Or, if this is still contrary, it's at least less contrary than requiring interop with middleware.) SuperTokens seems to be up for something like this (in the future), and I've been trying to make POCs to prove the usefulness of this approach. I have a rough POC for Remix, but the idea is currently incompatible with SvelteKit.

There may be other cases where a library could return response Headers containing important cookie information. If that happens, SvelteKit has no way of helping the developers using this package.

(Relates to #4375)

Describe the proposed solution

If there is absolutely no desire to allow Set-Cookie in the Headers returned in Svelte Kit land, then perhaps a new event.cookies.setFromHeader (or similarly named) method could be created. This ensures that event.cookies still maintains control over how cookies are set in the browser, but it also enables Svelte Kit to support libraries that may need to return cookie information in the headers. (Again, if the ideal is reached for all auth solutions in the future and they all become more flexible by navigating away from middleware, then this will be inevitable.)

Alternatives considered

An alternative is simply to allow developers to return their own Responses in Svelte Kit in all relevant use cases -- such as with actions. The reason I ran into this problem is that I was trying to redirect a previously-unauthenticated user back to their target page after they submitted a login form (via SuperTokens). But I couldn't set cookies in the response headers, nor could I return a custom Response.

Allowing developers to return a custom Response that sets the cookies would inherently solve this problem. (Remix and SolidStart already do this.) But I'm not sure how easy that would be for Svelte Kit. And even if it was easy, I imagine it would be awkward to support both event.setHeaders + event.cookies and return new Response().

Importance

would make my life easier

Additional Information

I'm assuming that one thought which may come to mind is, "Why not have these auth solutions return custom headers and custom cookies instead of having them just return headers with cookies inside?" That's a fair question, but it's a little harder to justify. Just as there are inconsistent workarounds for Set-Cookie, so there is not yet a universal standard for a "Cookie Object", let alone a "Cookies Map". But there is a standard for Headers. Consequently, it seems more reasonable to take a headers-based approach in this case.

@ITenthusiasm
Copy link
Author

ITenthusiasm commented Jan 9, 2023

In case someone brings it up, I'm aware that Next Auth is already looking at Svelte Auth. So in some sense, that does alleviate the needs of some of the people that need auth in Svelte land who haven't been able to get it due to middleware limitations.

However, Next/Svelte Auth still lacks some features that other solutions have. And to force developers to use only one particular solution for auth isn't completely fair or reasonable.

@Rich-Harris
Copy link
Member

I'm sorry, I've read this a couple of times and I'm completely stumped. What are you trying to do that you can't currently do? You can certainly append your own set-cookie headers in handle.

@ITenthusiasm
Copy link
Author

Sorry @Rich-Harris!

Currently, I'm trying to have a user login at /login via a form action. The form action looks like this:

export const actions: Actions = {
  default: async (event) => {
    // Form Data
    const formData = await event.request.formData().then(Object.fromEntries);
    const { email, password, mode } = formData;

    // Validate Data
    const errors: ActionData = {};
    if (!email) errors.email = "Email is required";
    else if (!validateEmail(email)) errors.email = "Email is invalid";

    if (!password) errors.password = "Password is required";
    else if (mode === "signup" && !validatePassword(password)) {
      errors.password = "Password must contain at least 8 characters, including a number";
    }

    if (errors.email || errors.password) return fail(400, { errors });

    // Attempt Sign In / Sign Up
    const normalizedMode = mode === "signup" ? "signup" : "signin";
    const { status, responseHeaders } = await SuperTokensHelpers[normalizedMode](email, password);

    // Auth failed
    if (status === "WRONG_CREDENTIALS_ERROR") {
      return fail(401, { errors: { banner: "Incorrect email and password combination" } });
    }

    if (status === "EMAIL_ALREADY_EXISTS_ERROR") {
      return fail(400, { errors: { email: "This email already exists. Please sign in instead." } });
    }

    // Auth succeeded
    responseHeaders.set("Location", new URL(request.url).searchParams.get("returnUrl") || "/");
    return new Response(null, { status: 302, statusText: "OK", headers: responseHeaders });
  },
}

However, according to the docs:

The returned data must be serializable as JSON. Beyond that, the structure is entirely up to you

So the Response I try to return gets rejected by Svelte Kit. From #4375, it seems like I can't do event.setHeaders for Set-Cookie. The docs confirm this. The expectation is that the developer would use event.cookies.set instead from inside the action, but I can't do that because I only have response headers. Theoretically, I could try to extract each individual cookie, but I'd rather not risk messing up the parsing. It would be better if I could set return a custom Response with the headers, if I could event.setHeaders (which I'm assuming isn't an option), or if event.cookies could somehow read from the Headers.

Does that help?

@ITenthusiasm
Copy link
Author

ITenthusiasm commented Jan 9, 2023

Okay. I just saw #7611 (comment), so it seems like returning a new Response() with cookies won't be an option anyway.

In this case, it seems like the only available option would be to add something like event.cookies.setFromHeaders. I guess that's probably better since all the cookie implementation stuff stays in one place.

@Rich-Harris
Copy link
Member

Getting a Headers object back from the auth API feels a bit too... high-level. It would probably be more flexible if it just gave you a cookie name/value. But that's their choice.

My inclination would be to install set-cookie-parser and do it like this:

import * as set_cookie_parser from 'set-cookie-parser';

// in the action...
const { status, responseHeaders } = await SuperTokensHelpers[normalizedMode](email, password);

// ...

// Auth succeeded
for (const str of set_cookie_parser.splitCookiesString(responseHeaders.get('set-cookie'))) {
  const { name, value, ...options } = set_cookie_parser.parseString(str);
  cookies.set(name, value, {...});
}

throw redirect(302, url.searchParams.get('returnUrl') ?? '/');

@Rich-Harris
Copy link
Member

(If you know that there'll only be a single cookie in set-cookie then you can skip the splitCookiesString step, of course — this is just belt and braces of the sort that would be unnecessary with a lower level interface)

@ITenthusiasm
Copy link
Author

ITenthusiasm commented Jan 9, 2023

Nothing is set in stone yet; so I could probably suggest that implementation change if that's what you'd recommend. Part of the point of the POCs that I'm doing is to give them an idea of what the potential inputs and outputs would be for their functions so that people could use their stuff middleware-free in the future (which means nice integration with Svelte Kit handle without having to interact with the underlying server).

I'm not sure what the case would be for other auth options; but for SuperTokens, returning some representation of response headers would still be inevitable due to their current implementation. I guess a simple object could also be returned instead of an instance of Headers. But you're saying -- whatever those extra headers may be -- return a separate Array/Map/"dictionary" of cookies?

// in the action...
const { status, cookies, responseHeaders } = await SuperTokensHelpers[normalizedMode](email, password);
cookies.forEach(/* Call `event.cookies.set` for each cookie */);
Object.entries(responseHeaders).forEach(/* Call `event.setHeaders` for each header */);

@Rich-Harris
Copy link
Member

The Headers interfaces is intrinsically awkward when it comes to cookies, because it was only designed with browser use cases in mind. Specifically, while it has an append method, you can only get a single value (there's no getAll like there is with FormData or URLSearchParams), because the designers didn't anticipate that it would be used with set-cookie since you'd never encounter that header in a browser. (Attempts to rectify the situation have not succeeded thus far.)

Most headers that can have multiple values (like cache-control) simply join them with a ,. That doesn't really work for set-cookie because the , delimiter is very likely to appear in the cookie definition. set-cookie-parser gives us a way to work with set-cookie cookies containing multiple values, but it's awkward, and any library that deals with cookies will do its users a favour by not involving Headers, which only belongs — if anywhere — at the level of framework-specific integrations for those libraries.

So yes, returning an array of cookies (whether that's a string or an object, perhaps one that matches the return value of cookie.parse since that library is very widely used) is a good idea.

@ITenthusiasm
Copy link
Author

Okay great! All of this is really helpful. Thanks for the insights!

ITenthusiasm added a commit to ITenthusiasm/remix-supertokens that referenced this issue Jan 12, 2023
…kensOutput`

After chatting with Rich Harris (creator of Svelte) on
sveltejs/kit#8409, it seems that it's better to expose
any necessary auth cookies _explicitly_ rather than wrap
them in `Headers`. The short reasoning for this is that
it provides more options to developers using SSR
frameworks. (The previous implementation locked some
developers out of using `SuperTokens` in the framework
of their choice.)

Note that the `SuperTokensOutput` utility class now
exposes the `responseHeaders` as a `Map` instead of
a `Headers` object in order to support frameworks
(or servers) that do not have a proper `Headers`
class that they can use. (Node.js doesn't quite
support `Headers` out of the box yet. See the
MDN compatibility table.)
krzysilelek added a commit to krzysilelek/Local_Bargains that referenced this issue Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants