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

Cookies from header not parsed by AstroCookies #13078

Open
1 task
tstern-bolindalabs opened this issue Jan 27, 2025 · 6 comments
Open
1 task

Cookies from header not parsed by AstroCookies #13078

tstern-bolindalabs opened this issue Jan 27, 2025 · 6 comments
Labels
- P2: has workaround Bug, but has workaround (priority) core pkg: astro Related to the core `astro` package (scope)

Comments

@tstern-bolindalabs
Copy link

Astro Info

Astro                    v5.1.10
Node                     v20.17.0
System                   Linux (x64)
Package Manager          unknown
Output                   server
Adapter                  none
Integrations             none

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

When inside of a middleware calling context.cookies.get('test', { decode }) the decode function is not invoked and the cookie value is still encoded.

The cookie is set within the request header. In the linked minimal reproducible example the page must be reloaded to set the cookie.

What's the expected result?

I would expect that context.cookies.get will parse and decode cookie values from request header.

Link to Minimal Reproducible Example

https://stackblitz.com/~/github.com/tstern-bolindalabs/just-the-basics

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Jan 27, 2025
@ematipico
Copy link
Member

I can't access the reproduction

@ematipico ematipico added the needs repro Issue needs a reproduction label Jan 27, 2025
Copy link
Contributor

Hello @tstern-bolindalabs. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with needs repro will be closed if they have no activity within 3 days.

@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Jan 27, 2025
@tstern-bolindalabs
Copy link
Author

I forgot to make the github repo public. It should now be accessible.

@joshmkennedy
Copy link

joshmkennedy commented Jan 29, 2025

From reading, core/cookies/cookies.ts it seems that the initial call of context.cookies.has('test') is the reason it is not being decoded. This is because the cookies are cached in a map when the has method is called. If you make sure to pass in the same decode function as you pass in the get function this will work as expected.

//                               👇
if (context.cookies.has('test', { decode })) {
         const test = context.cookies.get('test', { decode });
         console.log('test:', test?.value);
    } else {
....
}

This does still seem to be a bug, but at least there is a workaround

@tstern-bolindalabs
Copy link
Author

Thank you for response. This would be a workaround.

Additional it seems that other cookies.get calls are also not getting decoded. I added a second test2 cookie to the repo and this is also not getting decoded. It's like the cookie logics sticks to either the first call used decode or not.

@ematipico ematipico added needs triage Issue needs to be triaged pkg: astro Related to the core `astro` package (scope) - P2: has workaround Bug, but has workaround (priority) core and removed needs repro Issue needs a reproduction labels Jan 30, 2025
@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Jan 30, 2025
@joshmkennedy
Copy link

joshmkennedy commented Jan 30, 2025

Currently as soon as you call context.cookies.get or context.cookies.has the decode option is set. This isn't ideal as now all your cookies have to be encoded and decoded in the same way.

Here is an example I made of your middleware example that introduces an additional middleware that checks if it needs to be decoded by checking for a prefix.

import { defineMiddleware, sequence } from "astro/middleware";

export const encode = (data: unknown) => {
    console.log('encode:', data);
    const dataSerialized = typeof data === 'string' ? data : JSON.stringify(data);
	return `base64_${Buffer.from(dataSerialized).toString('base64')}`;
};

export const decode = (str: string) => {
		if(str.startsWith('base64_')){
			return Buffer.from(str.replace('base64_', ''), 'base64').toString();
		} else {
			return decodeURIComponent(str)// this is the default implemented by 'cookie' package 
                        // https://github.com/jshttp/cookie?tab=readme-ov-file#decode
		}
};

const initializeOurCookies = defineMiddleware((context,next)=>{
	context.cookies.get('doesnt-matter', {decode}) // just need to have the cookies parsed
	next()
})

const testCookies = defineMiddleware( async (context, next) => {
    console.log('headers#cookie:', context.request.headers.get('cookie'));
    if (context.cookies.has('test')) {
        const test = context.cookies.get('test');
        console.log('test:', test?.value);
        const test2 = context.cookies.get('test2');
        console.log('test2:', test2?.value);
    } else {
        const tomorrow = new Date(Date.now() + 1000 * 60 * 60 * 24);
        context.cookies.set('test', 'test_value', { encode, path: '/', expires: tomorrow });
        context.cookies.set('test2', 'test_value_2', {  path: '/', expires: tomorrow });
    }

    return next();
})

export const onRequest = sequence(initializeOurCookies, testCookies);

Im not sure how the current API should be fixed, but this what I would do for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: has workaround Bug, but has workaround (priority) core pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

No branches or pull requests

3 participants