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 custom path for preview mode cookies #38313

Merged
merged 8 commits into from Aug 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/advanced-features/preview-mode.md
Expand Up @@ -194,10 +194,12 @@ Then, send a request to `/api/clear-preview-mode-cookies` to invoke the API Rout
`setPreviewData` takes an optional second parameter which should be an options object. It accepts the following keys:

- `maxAge`: Specifies the number (in seconds) for the preview session to last for.
- `path`: Specifies the path the cookie should be applied under. Defaults to `/` enabling preview mode for all paths.

```js
setPreviewData(data, {
maxAge: 60 * 60, // The preview mode cookies expire in 1 hour
path: '/about', // The preview mode cookies apply to paths with /about
})
```

Expand Down
7 changes: 7 additions & 0 deletions packages/next/server/api-utils/node.ts
Expand Up @@ -469,6 +469,7 @@ function setPreviewData<T>(
data: object | string, // TODO: strict runtime type checking
options: {
maxAge?: number
path?: string
Copy link
Contributor Author

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 as string in this setPreviewData function here without extra parsing/validation on it because developers would control the setPreviewData call and its parameters, just like they do with redirects inside the preview API endpoint. The security concerns around path remain the same though - https://datatracker.ietf.org/doc/html/rfc6265#section-4.1.2.4

} & __ApiPreviewProps
): NextApiResponse<T> {
if (isNotValidData(options.previewModeId)) {
Expand Down Expand Up @@ -522,6 +523,9 @@ function setPreviewData<T>(
...(options.maxAge !== undefined
? ({ maxAge: options.maxAge } as CookieSerializeOptions)
: undefined),
...(options.path !== undefined
? ({ path: options.path } as CookieSerializeOptions)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While checking CookieSerializeOptions type definitions, I noticed that it'd be good to update it to v0.5.1 to get the latest (and more complete definitions). /@types/cookie/0.5.1 has correct links to https://datatracker.ietf.org/doc/html/rfc6265#section-5.2.4 which is way more helpful to understand how this path and maxAge behave. I thought about doing it in the same PR, but I think it's rather better to keep deps updates isolated

: undefined),
Comment on lines +526 to +528
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also considering doing something like

Suggested change
...(options.path !== undefined
? ({ path: options.path } as CookieSerializeOptions)
: undefined),
...(options.path !== undefined
? ({ path: options.path || '/' } as CookieSerializeOptions)
: undefined),

but I found out that there's a normalization function for the cookie serialize function params.

Also, I though about refactoring this maxAge and path props to have a single object like

Suggested change
...(options.path !== undefined
? ({ path: options.path } as CookieSerializeOptions)
: undefined),
...({ path: options.path , maxAge: options.maxAge }),

But when path or maxAge is empty, we'd be passing { path: undefined, maxAge: undefined } instead of { }. Those are actually different, and I'm not certain how the options object properties are treated down the chain

}),
serialize(COOKIE_NAME_PRERENDER_DATA, payload, {
httpOnly: true,
Expand All @@ -531,6 +535,9 @@ function setPreviewData<T>(
...(options.maxAge !== undefined
? ({ maxAge: options.maxAge } as CookieSerializeOptions)
: undefined),
...(options.path !== undefined
? ({ path: options.path } as CookieSerializeOptions)
: undefined),
}),
])
return res
Expand Down
5 changes: 5 additions & 0 deletions packages/next/shared/lib/utils.ts
Expand Up @@ -252,6 +252,11 @@ export type NextApiResponse<T = any> = ServerResponse & {
* when the client shuts down (browser is closed).
*/
maxAge?: number
/**
* Specifies the path for the preview session to work under. By default,
* the path is considered the "default path", i.e., any pages under "/".
*/
path?: string
}
) => NextApiResponse<T>
clearPreviewData: () => NextApiResponse<T>
Expand Down
14 changes: 6 additions & 8 deletions test/integration/prerender-preview/pages/api/preview.js
Expand Up @@ -6,14 +6,12 @@ export default (req, res) => {
return res.status(500).end('too big')
}
} else {
res.setPreviewData(
req.query,
req.query.cookieMaxAge
? {
maxAge: req.query.cookieMaxAge,
}
: undefined
)
res.setPreviewData(req.query, {
...(req.query.cookieMaxAge
? { maxAge: req.query.cookieMaxAge }
: undefined),
...(req.query.cookiePath ? { path: req.query.cookiePath } : undefined),
})
}

res.status(200).end()
Expand Down
20 changes: 20 additions & 0 deletions test/integration/prerender-preview/test/index.test.js
Expand Up @@ -121,6 +121,26 @@ function runTests(startServer = nextStart) {
expect(cookies[1]).toHaveProperty('__next_preview_data')
expect(cookies[1]['Max-Age']).toBe(expiry)
})
it('should set custom path cookies', async () => {
const path = '/path'
const res = await fetchViaHTTP(appPort, '/api/preview', {
cookiePath: path,
})
expect(res.status).toBe(200)

const originalCookies = res.headers.get('set-cookie').split(',')
const cookies = originalCookies.map(cookie.parse)

expect(originalCookies.every((c) => c.includes('; Secure;'))).toBe(true)

expect(cookies.length).toBe(2)
expect(cookies[0]).toMatchObject({ Path: path, SameSite: 'None' })
expect(cookies[0]).toHaveProperty('__prerender_bypass')
expect(cookies[0]['Path']).toBe(path)
expect(cookies[0]).toMatchObject({ Path: path, SameSite: 'None' })
expect(cookies[1]).toHaveProperty('__next_preview_data')
expect(cookies[1]['Path']).toBe(path)
})
it('should not return fallback page on preview request', async () => {
const res = await fetchViaHTTP(
appPort,
Expand Down
27 changes: 27 additions & 0 deletions test/unit/split-cookies-string.test.ts
Expand Up @@ -44,6 +44,16 @@ describe('splitCookiesString', () => {
expect(result).toEqual(expected)
})

it('should parse path', () => {
const { joined, expected } = generateCookies({
name: 'foo',
value: 'bar',
path: '/path',
})
const result = splitCookiesString(joined)
expect(result).toEqual(expected)
})

it('should parse with all the options', () => {
const { joined, expected } = generateCookies({
name: 'foo',
Expand Down Expand Up @@ -111,6 +121,23 @@ describe('splitCookiesString', () => {
expect(result).toEqual(expected)
})

it('should parse path', () => {
const { joined, expected } = generateCookies(
{
name: 'foo',
value: 'bar',
path: '/path',
},
{
name: 'x',
value: 'y',
path: '/path',
}
)
const result = splitCookiesString(joined)
expect(result).toEqual(expected)
})

it('should parse with all the options', () => {
const { joined, expected } = generateCookies(
{
Expand Down