sveltejs / kit

web development, streamlined
https://svelte.dev/docs/kit
MIT License
18.72k stars 1.94k forks source link

Cookie signing powered by SK #6996

Open bato3 opened 2 years ago

bato3 commented 2 years ago

Describe the problem

I can see examples on the web where raw userId is used as session key: https://dev.to/theether0/sveltekit-changes-session-and-cookies-enb

cookies.set('session', user.entityId, {
            path: '/',
            httpOnly: true,
            sameSite: 'strict',
            secure: !dev,
            maxAge: 60 * 60 * 24 * 30
        });
export const handle: Handle = async ({ event, resolve }) => {
    const session = event.cookies.get('session');
    // ...
    const currentUser = await userRepository.fetch(session as string);
    // ...
}

Describe the proposed solution

Cookies should be signed by default, or the option to sign cookies should be integrated in SK.

Alternatives considered

No response

Importance

would make my life easier

Additional Information

No response

Conduitry commented 2 years ago

Cookies should definitely not be signed by default, because there's no such thing in the cookie spec as a signed cookie - it's just a cookie with extra data appended to it. And if you're setting cookies with a certain value of a form that's expected by some external service, and then suddenly the cookie isn't actually of that form, that's not going to work.

I'd also argue that this is a topic where the framework doesn't want to be this opinionated. There's no standard way to sign cookies. In the spirit of code over configuration, the user should just read and write the cookie values they want (running them through their library of choice), rather than configuring what cookie signing mechanism they want to use.

bato3 commented 2 years ago

I understand your point of view, but I can see that regular users have trouble creating safe applications. (As in the linked "course").

More than once I have seen instructions that would be a perfect example of how to make leaky applications ...

May choose an indirect approach: by default, cookies will be signed, but this mechanism can be easily disabled / changed.

I believe that some behaviors must be forced to improve security.

theetherGit commented 2 years ago

@bato3 Brother there is a difference between demo code and a production ready app. And other than that cookies are not meant to be signed you have to do that.

const userData: User = {
    id: user.entityId,
    name: user.name,
    email: user.email,
    phone: user.phone,
    type: user.user_type,
    active: user.active
};
const token = jwt.sign(userData, JWT_KEY, { algorithm: 'HS256' });
cookies.set('session', token, {
    path: '/',
    httpOnly: true,
        sameSite: 'strict',
    secure: !dev,
    maxAge: 60 * 60 * 24 * 30
});

You can do this for authentication and then in hooks you can validate the sign and then decode it and assign to locals.

codenoid commented 9 months ago

Cookies should be signed

I disagree that this should be a default option

bato3 commented 5 months ago

Some frameworks offer the option to have cookies signed.

If I understand the current situation correctly, in SK this scenario should be handled independently.

Due to the objection against obligatory cookie signing, I am modifying the proposal to:

Cookie signing powered by SK.

The advantage of implementing this in the SK engine will be improved security in applications created by less experienced developers.