From 21f46906b5874a665cc62d2d71d7830ccfb384ff Mon Sep 17 00:00:00 2001
From: Dominic Saadi <dominiceliassaadi@gmail.com>
Date: Sun, 4 Sep 2022 17:36:36 +0900
Subject: [PATCH] Handle special props `ref` and `key` in path and search
 params (#5537)

Co-authored-by: Tobbe Lundberg <tobbe@tlundberg.com>
---
 packages/router/src/__tests__/router.test.tsx | 20 +++++++++++
 packages/router/src/__tests__/util.test.ts    | 33 +++++++++++++++++++
 packages/router/src/router.tsx                | 10 +++++-
 packages/router/src/util.ts                   | 10 ++++++
 4 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/packages/router/src/__tests__/router.test.tsx b/packages/router/src/__tests__/router.test.tsx
index 4e2198d78d4d..5b328727b7ab 100644
--- a/packages/router/src/__tests__/router.test.tsx
+++ b/packages/router/src/__tests__/router.test.tsx
@@ -1489,3 +1489,23 @@ test('params should be updated if navigated to different route with same page',
     expect(screen.queryByText('hookparams 99')).toBeInTheDocument()
   })
 })
+
+test('should handle ref and key as search params', async () => {
+  const ParamsPage = () => {
+    const { ref, key } = useParams()
+    return <p>{JSON.stringify({ ref, key })}</p>
+  }
+
+  const TestRouter = () => (
+    <Router>
+      <Route path="/params" page={ParamsPage} name="params" />
+    </Router>
+  )
+
+  const screen = render(<TestRouter />)
+  act(() => navigate('/params?ref=1&key=2'))
+
+  await waitFor(() => {
+    expect(screen.queryByText(`{"ref":"1","key":"2"}`)).toBeInTheDocument()
+  })
+})
diff --git a/packages/router/src/__tests__/util.test.ts b/packages/router/src/__tests__/util.test.ts
index 694d54702ef2..3e34aabf27f5 100644
--- a/packages/router/src/__tests__/util.test.ts
+++ b/packages/router/src/__tests__/util.test.ts
@@ -283,6 +283,39 @@ describe('validatePath', () => {
   ])('validates correct path "%s"', (path) => {
     expect(validatePath.bind(null, path)).not.toThrow()
   })
+
+  it.each([
+    '/path/{ref}',
+    '/path/{ref}/bazinga',
+    '/path/{ref:Int}',
+    '/path/{ref:Int}/bazinga',
+    '/path/{key}',
+    '/path/{key}/bazinga',
+    '/path/{key:Int}',
+  ])('rejects paths with ref or key as path parameters: "%s"', (path) => {
+    expect(validatePath.bind(null, path)).toThrowError(
+      [
+        `Route contains ref or key as a path parameter: "${path}"`,
+        "`ref` and `key` shouldn't be used as path parameters because they're special React props.",
+        'You can fix this by renaming the path parameter.',
+      ].join('\n')
+    )
+  })
+
+  it.each([
+    '/path/{reff}',
+    '/path/{reff:Int}',
+    '/path/{reff}/bazinga',
+    '/path/{keys}',
+    '/path/{keys:Int}',
+    '/path/key',
+    '/path/key/bazinga',
+  ])(
+    `doesn't reject paths with variations on ref or key as path parameters: "%s"`,
+    (path) => {
+      expect(validatePath.bind(null, path)).not.toThrowError()
+    }
+  )
 })
 
 describe('parseSearch', () => {
diff --git a/packages/router/src/router.tsx b/packages/router/src/router.tsx
index 10b719e83a52..221c36dde2a8 100644
--- a/packages/router/src/router.tsx
+++ b/packages/router/src/router.tsx
@@ -97,7 +97,7 @@ const InternalRoute = ({
   )
 
   const searchParams = parseSearch(location.search)
-  const allParams = { ...searchParams, ...pathParams }
+  const allParams: Record<string, string> = { ...searchParams, ...pathParams }
 
   if (redirect) {
     const newPath = replaceParams(redirect, allParams)
@@ -113,6 +113,14 @@ const InternalRoute = ({
 
   const Page = activePageContext.loadingState[path]?.page || (() => null)
 
+  // There are two special props in React: `ref` and `key`. (See https://reactjs.org/warnings/special-props.html.)
+  // It's very possible that the URL has `ref` as a search param (e.g. https://redwoodjs.com/?ref=producthunt).
+  // Since we pass URL params to the page, we have to be careful not to pass `ref` or `key`, otherwise the page will break.
+  // (The page won't actually break if `key` is passed, but it feels unclean.)
+  // If users want to access them, they can use `useParams`.
+  delete allParams['ref']
+  delete allParams['key']
+
   // Level 3/3 (InternalRoute)
   return <Page {...allParams} />
 }
diff --git a/packages/router/src/util.ts b/packages/router/src/util.ts
index ebbe62177d9f..f5bcaa13602d 100644
--- a/packages/router/src/util.ts
+++ b/packages/router/src/util.ts
@@ -197,6 +197,16 @@ export const validatePath = (path: string) => {
     throw new Error(`Route path contains spaces: "${path}"`)
   }
 
+  if (/{(?:ref|key)(?::|})/.test(path)) {
+    throw new Error(
+      [
+        `Route contains ref or key as a path parameter: "${path}"`,
+        "`ref` and `key` shouldn't be used as path parameters because they're special React props.",
+        'You can fix this by renaming the path parameter.',
+      ].join('\n')
+    )
+  }
+
   // Check for duplicate named params.
   const matches = path.matchAll(/\{([^}]+)\}/g)
   const memo: Record<string, boolean> = {}