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 custom path for preview mode cookies #38313
Changes from 6 commits
3f571b1
944e993
7412322
2d1d7fc
5e78444
62f8656
a4b6b4d
1d14604
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -461,6 +461,7 @@ function setPreviewData<T>( | |||||||||||||||||||||
data: object | string, // TODO: strict runtime type checking | ||||||||||||||||||||||
options: { | ||||||||||||||||||||||
maxAge?: number | ||||||||||||||||||||||
path?: string | ||||||||||||||||||||||
} & __ApiPreviewProps | ||||||||||||||||||||||
): NextApiResponse<T> { | ||||||||||||||||||||||
if (isNotValidData(options.previewModeId)) { | ||||||||||||||||||||||
|
@@ -510,19 +511,23 @@ function setPreviewData<T>( | |||||||||||||||||||||
httpOnly: true, | ||||||||||||||||||||||
sameSite: process.env.NODE_ENV !== 'development' ? 'none' : 'lax', | ||||||||||||||||||||||
secure: process.env.NODE_ENV !== 'development', | ||||||||||||||||||||||
path: '/', | ||||||||||||||||||||||
...(options.maxAge !== undefined | ||||||||||||||||||||||
? ({ maxAge: options.maxAge } as CookieSerializeOptions) | ||||||||||||||||||||||
: undefined), | ||||||||||||||||||||||
...(options.path !== undefined | ||||||||||||||||||||||
? ({ path: options.path } as CookieSerializeOptions) | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While checking |
||||||||||||||||||||||
: undefined), | ||||||||||||||||||||||
Comment on lines
+526
to
+528
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was also considering doing something like
Suggested change
but I found out that there's a normalization function for the cookie serialize function params. Also, I though about refactoring this
Suggested change
But when |
||||||||||||||||||||||
}), | ||||||||||||||||||||||
serialize(COOKIE_NAME_PRERENDER_DATA, payload, { | ||||||||||||||||||||||
httpOnly: true, | ||||||||||||||||||||||
sameSite: process.env.NODE_ENV !== 'development' ? 'none' : 'lax', | ||||||||||||||||||||||
secure: process.env.NODE_ENV !== 'development', | ||||||||||||||||||||||
path: '/', | ||||||||||||||||||||||
...(options.maxAge !== undefined | ||||||||||||||||||||||
? ({ maxAge: options.maxAge } as CookieSerializeOptions) | ||||||||||||||||||||||
: undefined), | ||||||||||||||||||||||
...(options.path !== undefined | ||||||||||||||||||||||
? ({ path: options.path } as CookieSerializeOptions) | ||||||||||||||||||||||
: undefined), | ||||||||||||||||||||||
}), | ||||||||||||||||||||||
]) | ||||||||||||||||||||||
return res | ||||||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it'd be good enough to just accept raw
path
asstring
in thissetPreviewData
function here without extra parsing/validation on it because developers would control thesetPreviewData
call and its parameters, just like they do with redirects inside thepreview
API endpoint. The security concerns aroundpath
remain the same though - https://datatracker.ietf.org/doc/html/rfc6265#section-4.1.2.4