From dfb0021545000a386188ff8a1cc03d02ec07fd2d Mon Sep 17 00:00:00 2001 From: Jacob Fletcher Date: Fri, 12 Sep 2025 13:43:03 -0400 Subject: [PATCH] fix: client config context inheritance (#13790) Redirecting from login to any non-auth collection crashes the page with the following error: ``` Cannot read properties of null (reading 'fields') ``` TL;DR: the page-level config context was threading stale methods from the root config provider. #### Background The client config is now gated behind authentication as of #13714, so it changes based on authentication status. If the root layout is mounted before authentication, it puts the unauthenticated client config into state for the entire app to consume. On login, the root layout does not re-render, so the page itself needs to generate a fresh client config and sync it up. This leads to race conditions, however, where if the login page included a `?redirect=` param, the redirect would take place _before_ the page-level client config could sync to the layout, and ultimately crash the page. This was addressed in #13786. While this fixed redirects to the "users" collection, this collection is _already_ included in the client config (soon to be omitted by #13785). So if you redirect to any other collection, the above error occurs. #### Problem The page-level config context is only overriding the `config` property, keeping stale methods from the root config provider. This means calling `getEntityConfig` during this moment in the time would reference the stale config, although `config` itself would be fresh. #### Solution Wrap the page with an entirely new context provider. Do not thread inherited methods from the root provider, this way all new methods get instantiated using the fresh config. --- - To see the specific tasks where the Asana app for GitHub is being used, see below: - https://app.asana.com/0/0/1211332845301596 --- packages/next/src/views/Root/index.tsx | 6 +- packages/payload/src/config/client.ts | 2 +- packages/ui/src/exports/client/index.ts | 2 +- packages/ui/src/providers/Config/index.tsx | 39 +++---- test/access-control/e2e.spec.ts | 2 +- test/admin-root/e2e.spec.ts | 2 +- test/auth/e2e.spec.ts | 51 +++++++-- test/helpers.ts | 117 +------------------ test/helpers/e2e/auth/login.ts | 126 +++++++++++++++++++++ test/helpers/e2e/auth/logout.ts | 12 ++ test/plugin-multi-tenant/e2e.spec.ts | 8 +- 11 files changed, 210 insertions(+), 157 deletions(-) create mode 100644 test/helpers/e2e/auth/login.ts create mode 100644 test/helpers/e2e/auth/logout.ts diff --git a/packages/next/src/views/Root/index.tsx b/packages/next/src/views/Root/index.tsx index 5b6a9b90e..ff50572dd 100644 --- a/packages/next/src/views/Root/index.tsx +++ b/packages/next/src/views/Root/index.tsx @@ -10,7 +10,7 @@ import type { SanitizedGlobalConfig, } from 'payload' -import { RootPageConfigProvider } from '@payloadcms/ui' +import { PageConfigProvider } from '@payloadcms/ui' import { RenderServerComponent } from '@payloadcms/ui/elements/RenderServerComponent' import { getClientConfig } from '@payloadcms/ui/utilities/getClientConfig' import { notFound, redirect } from 'next/navigation.js' @@ -300,7 +300,7 @@ export const RootPage = async ({ }) return ( - + {!templateType && {RenderedView}} {templateType === 'minimal' && ( {RenderedView} @@ -331,6 +331,6 @@ export const RootPage = async ({ {RenderedView} )} - + ) } diff --git a/packages/payload/src/config/client.ts b/packages/payload/src/config/client.ts index 7d410ecdd..324ed63c7 100644 --- a/packages/payload/src/config/client.ts +++ b/packages/payload/src/config/client.ts @@ -63,7 +63,7 @@ export type UnauthenticatedClientConfig = { globals: [] routes: ClientConfig['routes'] serverURL: ClientConfig['serverURL'] - unauthenticated: ClientConfig['unauthenticated'] + unauthenticated: true } export const serverOnlyAdminConfigProperties: readonly Partial[] = [] diff --git a/packages/ui/src/exports/client/index.ts b/packages/ui/src/exports/client/index.ts index ad62e9f3c..e6e68373a 100644 --- a/packages/ui/src/exports/client/index.ts +++ b/packages/ui/src/exports/client/index.ts @@ -303,7 +303,7 @@ export { RouteTransitionProvider, useRouteTransition, } from '../../providers/RouteTransition/index.js' -export { ConfigProvider, RootPageConfigProvider, useConfig } from '../../providers/Config/index.js' +export { ConfigProvider, PageConfigProvider, useConfig } from '../../providers/Config/index.js' export { DocumentEventsProvider, useDocumentEvents } from '../../providers/DocumentEvents/index.js' export { DocumentInfoProvider, useDocumentInfo } from '../../providers/DocumentInfo/index.js' export { useDocumentTitle } from '../../providers/DocumentTitle/index.js' diff --git a/packages/ui/src/providers/Config/index.tsx b/packages/ui/src/providers/Config/index.tsx index 20e3bfc8b..45769a93e 100644 --- a/packages/ui/src/providers/Config/index.tsx +++ b/packages/ui/src/providers/Config/index.tsx @@ -100,41 +100,42 @@ export const ConfigProvider: React.FC<{ export const useConfig = (): ClientConfigContext => use(RootConfigContext) /** - * This provider shadows the ConfigProvider on the _page_ level, allowing us to + * This provider shadows the `ConfigProvider` on the _page_ level, allowing us to * update the config when needed, e.g. after authentication. - * The layout ConfigProvider is not updated on page navigation / authentication, + * The layout `ConfigProvider` is not updated on page navigation / authentication, * as the layout does not re-render in those cases. * * If the config here has the same reference as the config from the layout, we * simply reuse the context from the layout to avoid unnecessary re-renders. + * + * @experimental This component is experimental and may change or be removed in future releases. Use at your own discretion. */ -export const RootPageConfigProvider: React.FC<{ +export const PageConfigProvider: React.FC<{ readonly children: React.ReactNode readonly config: ClientConfig }> = ({ children, config: configFromProps }) => { - const rootLayoutConfig = useConfig() - const { config, getEntityConfig, setConfig } = rootLayoutConfig + const { config: rootConfig, setConfig: setRootConfig } = useConfig() /** - * This useEffect is required in order for the _page_ to be able to refresh the client config, - * which may have been cached on the _layout_ level, where the ConfigProvider is managed. + * This `useEffect` is required in order for the _page_ to be able to refresh the client config, + * which may have been cached on the _layout_ level, where the `ConfigProvider` is managed. * Since the layout does not re-render on page navigation / authentication, we need to manually * update the config, as the user may have been authenticated in the process, which affects the client config. */ useEffect(() => { - setConfig(configFromProps) - }, [configFromProps, setConfig]) + setRootConfig(configFromProps) + }, [configFromProps, setRootConfig]) - if (config !== configFromProps && config.unauthenticated !== configFromProps.unauthenticated) { - // Between the unauthenticated config becoming authenticated (or the other way around) and the useEffect - // running, the config will be stale. In order to avoid having the wrong config in the context in that - // brief moment, we shadow the context here on the _page_ level and provide the updated config immediately. - return ( - - {children} - - ) + // If this component receives a different config than what is in context from the layout, it is stale. + // While stale, we instantiate a new context provider that provides the new config until the root context is updated. + // Unfortunately, referential equality alone does not work bc the reference is lost during server/client serialization, + // so we need to also compare the `unauthenticated` property. + if ( + rootConfig !== configFromProps && + rootConfig.unauthenticated !== configFromProps.unauthenticated + ) { + return {children} } - return {children} + return children } diff --git a/test/access-control/e2e.spec.ts b/test/access-control/e2e.spec.ts index 4e58d4738..05b49f291 100644 --- a/test/access-control/e2e.spec.ts +++ b/test/access-control/e2e.spec.ts @@ -17,10 +17,10 @@ import { ensureCompilationIsDone, exactText, initPageConsoleErrorCatch, - login, saveDocAndAssert, } from '../helpers.js' import { AdminUrlUtil } from '../helpers/adminUrlUtil.js' +import { login } from '../helpers/e2e/auth/login.js' import { initPayloadE2ENoConfig } from '../helpers/initPayloadE2ENoConfig.js' import { POLL_TOPASS_TIMEOUT, TEST_TIMEOUT_LONG } from '../playwright.config.js' import { diff --git a/test/admin-root/e2e.spec.ts b/test/admin-root/e2e.spec.ts index dd4a65a24..e8ff5d1ca 100644 --- a/test/admin-root/e2e.spec.ts +++ b/test/admin-root/e2e.spec.ts @@ -8,11 +8,11 @@ import { fileURLToPath } from 'url' import { ensureCompilationIsDone, initPageConsoleErrorCatch, - login, saveDocAndAssert, // throttleTest, } from '../helpers.js' import { AdminUrlUtil } from '../helpers/adminUrlUtil.js' +import { login } from '../helpers/e2e/auth/login.js' import { initPayloadE2ENoConfig } from '../helpers/initPayloadE2ENoConfig.js' import { TEST_TIMEOUT_LONG } from '../playwright.config.js' diff --git a/test/auth/e2e.spec.ts b/test/auth/e2e.spec.ts index c6217a172..598f253e9 100644 --- a/test/auth/e2e.spec.ts +++ b/test/auth/e2e.spec.ts @@ -2,6 +2,8 @@ import type { BrowserContext, Page } from '@playwright/test' import { expect, test } from '@playwright/test' import { devUser } from 'credentials.js' +import { login } from 'helpers/e2e/auth/login.js' +import { logout } from 'helpers/e2e/auth/logout.js' import { openNav } from 'helpers/e2e/toggleNav.js' import path from 'path' import { fileURLToPath } from 'url' @@ -15,7 +17,6 @@ import { exactText, getRoutes, initPageConsoleErrorCatch, - login, saveDocAndAssert, } from '../helpers.js' import { AdminUrlUtil } from '../helpers/adminUrlUtil.js' @@ -388,21 +389,24 @@ describe('Auth', () => { await saveDocAndAssert(page) }) - test('ensure login page with redirect to users document redirects properly after login, without client error', async () => { - await page.goto(url.admin) - - await page.goto(`${serverURL}/admin/logout`) - - await expect(page.locator('.login')).toBeVisible() - + test('ensure `?redirect=` param is injected into the URL and handled properly after login', async () => { const users = await payload.find({ collection: slug, limit: 1, }) + const userDocumentRoute = `${serverURL}/admin/collections/users/${users?.docs?.[0]?.id}` + await logout(page, serverURL) + + // This will send the user back to the login page with a `?redirect=` param await page.goto(userDocumentRoute) + await expect + .poll(() => page.url(), { timeout: POLL_TOPASS_TIMEOUT }) + .toContain('/admin/login?redirect=') + + // Important: do not use the login helper here, as this may clear the redirect param await expect(page.locator('#field-email')).toBeVisible() await expect(page.locator('#field-password')).toBeVisible() @@ -414,8 +418,37 @@ describe('Auth', () => { .toBe(userDocumentRoute) // Previously, this would crash the page with a "Cannot read properties of undefined (reading 'match')" error - await expect(page.locator('#field-roles')).toBeVisible() + + // Now do this again, only with a page that is not in the user's collection + const notInUserCollection = await payload.create({ + collection: 'relationsCollection', + data: {}, + }) + + await logout(page, serverURL) + + await page.goto( + `${serverURL}/admin/collections/relationsCollection/${notInUserCollection.id}`, + ) + + await expect + .poll(() => page.url(), { timeout: POLL_TOPASS_TIMEOUT }) + .toContain('/admin/login?redirect=') + + // Important: do not use the login helper here, as this may clear the redirect param + await expect(page.locator('#field-email')).toBeVisible() + await expect(page.locator('#field-password')).toBeVisible() + + await page.locator('.form-submit > button').click() + + // Expect to be redirected to the correct page + await expect + .poll(() => page.url(), { timeout: POLL_TOPASS_TIMEOUT }) + .toBe(`${serverURL}/admin/collections/relationsCollection/${notInUserCollection.id}`) + + // Previously, this would crash the page with a "Cannot read properties of null (reading 'fields')" error + await expect(page.locator('#field-rel')).toBeVisible() }) }) }) diff --git a/test/helpers.ts b/test/helpers.ts index 27c0c1701..ce1687ba6 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -18,18 +18,7 @@ import { devUser } from './credentials.js' import { openNav } from './helpers/e2e/toggleNav.js' import { POLL_TOPASS_TIMEOUT } from './playwright.config.js' -type AdminRoutes = NonNullable['routes'] - -type LoginArgs = { - customAdminRoutes?: AdminRoutes - customRoutes?: Config['routes'] - data?: { - email: string - password: string - } - page: Page - serverURL: string -} +export type AdminRoutes = NonNullable['routes'] const random = (min: number, max: number) => Math.floor(Math.random() * (max - min + 1)) + min @@ -177,110 +166,6 @@ export async function throttleTest({ return client } -/** - * Logs a user in by navigating via click-ops instead of using page.goto() - */ -export async function loginClientSide(args: LoginArgs): Promise { - const { customAdminRoutes, customRoutes, data = devUser, page, serverURL } = args - const { - routes: { admin: incomingAdminRoute } = {}, - admin: { routes: { login: incomingLoginRoute, createFirstUser } = {} }, - } = getRoutes({ customAdminRoutes, customRoutes }) - - const adminRoute = formatAdminURL({ serverURL, adminRoute: incomingAdminRoute, path: '' }) - const loginRoute = formatAdminURL({ - serverURL, - adminRoute: incomingAdminRoute, - path: incomingLoginRoute, - }) - const createFirstUserRoute = formatAdminURL({ - serverURL, - adminRoute: incomingAdminRoute, - path: createFirstUser, - }) - - if ((await page.locator('#nav-toggler').count()) > 0) { - // a user is already logged in - log them out - await openNav(page) - await expect(page.locator('.nav__controls [aria-label="Log out"]')).toBeVisible() - await page.locator('.nav__controls [aria-label="Log out"]').click() - - if (await page.locator('dialog#leave-without-saving').isVisible()) { - await page.locator('dialog#leave-without-saving #confirm-action').click() - } - - await page.waitForURL(loginRoute) - } - - await wait(500) - await page.fill('#field-email', data.email) - await page.fill('#field-password', data.password) - await wait(500) - await page.click('[type=submit]') - - await expect(page.locator('.step-nav__home')).toBeVisible() - if ((await page.locator('a.step-nav__home').count()) > 0) { - await page.locator('a.step-nav__home').click() - } - - await page.waitForURL(adminRoute) - - await expect(() => expect(page.url()).not.toContain(loginRoute)).toPass({ - timeout: POLL_TOPASS_TIMEOUT, - }) - await expect(() => expect(page.url()).not.toContain(createFirstUserRoute)).toPass({ - timeout: POLL_TOPASS_TIMEOUT, - }) -} - -export async function login(args: LoginArgs): Promise { - const { customAdminRoutes, customRoutes, data = devUser, page, serverURL } = args - - const { - admin: { - routes: { createFirstUser, login: incomingLoginRoute, logout: incomingLogoutRoute } = {}, - }, - routes: { admin: incomingAdminRoute } = {}, - } = getRoutes({ customAdminRoutes, customRoutes }) - - const logoutRoute = formatAdminURL({ - serverURL, - adminRoute: incomingAdminRoute, - path: incomingLogoutRoute, - }) - - await page.goto(logoutRoute) - await wait(500) - - const adminRoute = formatAdminURL({ serverURL, adminRoute: incomingAdminRoute, path: '' }) - const loginRoute = formatAdminURL({ - serverURL, - adminRoute: incomingAdminRoute, - path: incomingLoginRoute, - }) - const createFirstUserRoute = formatAdminURL({ - serverURL, - adminRoute: incomingAdminRoute, - path: createFirstUser, - }) - - await page.goto(loginRoute) - await wait(500) - await page.fill('#field-email', data.email) - await page.fill('#field-password', data.password) - await wait(500) - await page.click('[type=submit]') - await page.waitForURL(adminRoute) - - await expect(() => expect(page.url()).not.toContain(loginRoute)).toPass({ - timeout: POLL_TOPASS_TIMEOUT, - }) - - await expect(() => expect(page.url()).not.toContain(createFirstUserRoute)).toPass({ - timeout: POLL_TOPASS_TIMEOUT, - }) -} - export async function saveDocHotkeyAndAssert(page: Page): Promise { const ua = page.evaluate(() => navigator.userAgent) const isMac = (await ua).includes('Mac OS X') diff --git a/test/helpers/e2e/auth/login.ts b/test/helpers/e2e/auth/login.ts new file mode 100644 index 000000000..e49943357 --- /dev/null +++ b/test/helpers/e2e/auth/login.ts @@ -0,0 +1,126 @@ +import type { AdminRoutes } from 'helpers.js' +import type { Config } from 'payload' +import type { Page } from 'playwright/test' + +import { devUser } from 'credentials.js' +import { getRoutes } from 'helpers.js' +import { formatAdminURL, wait } from 'payload/shared' +import { POLL_TOPASS_TIMEOUT } from 'playwright.config.js' +import { expect } from 'playwright/test' + +import { openNav } from '../toggleNav.js' + +type LoginArgs = { + customAdminRoutes?: AdminRoutes + customRoutes?: Config['routes'] + data?: { + email: string + password: string + } + page: Page + serverURL: string +} + +export async function login(args: LoginArgs): Promise { + const { customAdminRoutes, customRoutes, data = devUser, page, serverURL } = args + + const { + admin: { + routes: { createFirstUser, login: incomingLoginRoute, logout: incomingLogoutRoute } = {}, + }, + routes: { admin: incomingAdminRoute } = {}, + } = getRoutes({ customAdminRoutes, customRoutes }) + + const logoutRoute = formatAdminURL({ + serverURL, + adminRoute: incomingAdminRoute, + path: incomingLogoutRoute, + }) + + await page.goto(logoutRoute) + await wait(500) + + const adminRoute = formatAdminURL({ serverURL, adminRoute: incomingAdminRoute, path: '' }) + const loginRoute = formatAdminURL({ + serverURL, + adminRoute: incomingAdminRoute, + path: incomingLoginRoute, + }) + const createFirstUserRoute = formatAdminURL({ + serverURL, + adminRoute: incomingAdminRoute, + path: createFirstUser, + }) + + await page.goto(loginRoute) + await wait(500) + await page.fill('#field-email', data.email) + await page.fill('#field-password', data.password) + await wait(500) + await page.click('[type=submit]') + await page.waitForURL(adminRoute) + + await expect(() => expect(page.url()).not.toContain(loginRoute)).toPass({ + timeout: POLL_TOPASS_TIMEOUT, + }) + + await expect(() => expect(page.url()).not.toContain(createFirstUserRoute)).toPass({ + timeout: POLL_TOPASS_TIMEOUT, + }) +} + +/** + * Logs a user in by navigating via click-ops instead of using page.goto() + */ +export async function loginClientSide(args: LoginArgs): Promise { + const { customAdminRoutes, customRoutes, data = devUser, page, serverURL } = args + const { + routes: { admin: incomingAdminRoute } = {}, + admin: { routes: { login: incomingLoginRoute, createFirstUser } = {} }, + } = getRoutes({ customAdminRoutes, customRoutes }) + + const adminRoute = formatAdminURL({ serverURL, adminRoute: incomingAdminRoute, path: '' }) + const loginRoute = formatAdminURL({ + serverURL, + adminRoute: incomingAdminRoute, + path: incomingLoginRoute, + }) + const createFirstUserRoute = formatAdminURL({ + serverURL, + adminRoute: incomingAdminRoute, + path: createFirstUser, + }) + + if ((await page.locator('#nav-toggler').count()) > 0) { + // a user is already logged in - log them out + await openNav(page) + await expect(page.locator('.nav__controls [aria-label="Log out"]')).toBeVisible() + await page.locator('.nav__controls [aria-label="Log out"]').click() + + if (await page.locator('dialog#leave-without-saving').isVisible()) { + await page.locator('dialog#leave-without-saving #confirm-action').click() + } + + await page.waitForURL(loginRoute) + } + + await wait(500) + await page.fill('#field-email', data.email) + await page.fill('#field-password', data.password) + await wait(500) + await page.click('[type=submit]') + + await expect(page.locator('.step-nav__home')).toBeVisible() + if ((await page.locator('a.step-nav__home').count()) > 0) { + await page.locator('a.step-nav__home').click() + } + + await page.waitForURL(adminRoute) + + await expect(() => expect(page.url()).not.toContain(loginRoute)).toPass({ + timeout: POLL_TOPASS_TIMEOUT, + }) + await expect(() => expect(page.url()).not.toContain(createFirstUserRoute)).toPass({ + timeout: POLL_TOPASS_TIMEOUT, + }) +} diff --git a/test/helpers/e2e/auth/logout.ts b/test/helpers/e2e/auth/logout.ts new file mode 100644 index 000000000..b35da897a --- /dev/null +++ b/test/helpers/e2e/auth/logout.ts @@ -0,0 +1,12 @@ +import type { Page } from 'playwright' + +import { POLL_TOPASS_TIMEOUT } from 'playwright.config.js' +import { expect } from 'playwright/test' + +export const logout = async (page: Page, serverURL: string) => { + await page.goto(`${serverURL}/admin/logout`) + + await expect.poll(() => page.url(), { timeout: POLL_TOPASS_TIMEOUT }).toContain('/admin/login') + + await expect(page.locator('.login')).toBeVisible() +} diff --git a/test/plugin-multi-tenant/e2e.spec.ts b/test/plugin-multi-tenant/e2e.spec.ts index 86d9d7f03..ede000fcd 100644 --- a/test/plugin-multi-tenant/e2e.spec.ts +++ b/test/plugin-multi-tenant/e2e.spec.ts @@ -8,13 +8,9 @@ import { fileURLToPath } from 'url' import type { Config } from './payload-types.js' -import { - ensureCompilationIsDone, - initPageConsoleErrorCatch, - loginClientSide, - saveDocAndAssert, -} from '../helpers.js' +import { ensureCompilationIsDone, initPageConsoleErrorCatch, saveDocAndAssert } from '../helpers.js' import { AdminUrlUtil } from '../helpers/adminUrlUtil.js' +import { loginClientSide } from '../helpers/e2e/auth/login.js' import { goToListDoc } from '../helpers/e2e/goToListDoc.js' import { clearSelectInput,