Skip to content

Commit

Permalink
Fix wrong source path resolved from the stack frame to not expose int…
Browse files Browse the repository at this point in the history
…ernal code (vercel#23203)

Currently Next.js exposes internal code in the error overlay if certain errors were created from the user code. Some examples were attached in vercel#20776.

We can clearly see that the path is wrong (`../next-server`), it should be `./node_modules/next/dist/next-server`:

![CleanShot 2021-03-19 at 01 33 04](https://user-images.githubusercontent.com/3676859/111670728-1ae7e400-8853-11eb-9213-3b359798900e.png)

The root cause is the `__nextjs_original-stack-frame` middleware resolves the file path with the following code:

```js
path.resolve(
  rootDirectory,
  getSourcePath(sourcePosition.source)
)
```

where `rootDirectory` is the **app root**, but `sourcePosition.source` comes from the module path, which is relative to the path of the `next` binary, not the app root. 

That explains why we see `../next-server` from the error above, because it's relative to `./node_modules/next/bin/next`.

Because of that, the resolved result will never have `node_modules` in its path and it won't be filtered by the error overlay in the UI. Here's a screenshot of the frame object in the UI:

![CleanShot 2021-03-18 at 23 01 29@2x](https://user-images.githubusercontent.com/3676859/111670062-65b52c00-8852-11eb-9293-3a6e5b7c4b9b.png)

And the filter we use to determine if a frame is expanded or not only depends on `body.originalStackFrame`:

```js
expanded: !Boolean(
  body.originalStackFrame?.file?.includes('node_modules') ?? true
)
```

So this PR also adds `source.file` check to ensure they will be ignored (not necessary because we fixed the path resolving).

Fixes vercel#20776.
  • Loading branch information
shuding authored Mar 20, 2021
1 parent 8c72806 commit 89ec21e
Show file tree
Hide file tree
Showing 3 changed files with 32 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ export function getOriginalStackFrame(
external: false,
expanded: !Boolean(
/* collapsed */
body.originalStackFrame?.file?.includes('node_modules') ?? true
(source.file?.includes('node_modules') ||
body.originalStackFrame?.file?.includes('node_modules')) ??
true
),
sourceStackFrame: source,
originalStackFrame: body.originalStackFrame,
Expand Down
5 changes: 4 additions & 1 deletion packages/react-dev-overlay/src/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,14 @@ export async function createOriginalStackFrame({
line,
column,
source,
modulePath,
rootDirectory,
frame,
}: {
line: number
column: number | null
source: any
modulePath?: string
rootDirectory: string
frame: any
}): Promise<OriginalStackFrameResponse | null> {
Expand All @@ -140,7 +142,7 @@ export async function createOriginalStackFrame({

const filePath = path.resolve(
rootDirectory,
getSourcePath(sourcePosition.source)
modulePath || getSourcePath(sourcePosition.source)
)

const originalFrame: StackFrame = {
Expand Down Expand Up @@ -285,6 +287,7 @@ function getOverlayMiddleware(options: OverlayMiddlewareOptions) {
column: frameColumn,
source,
frame,
modulePath: moduleId,
rootDirectory: options.rootDirectory,
})

Expand Down
25 changes: 25 additions & 0 deletions test/acceptance/ReactRefreshLogBox.dev.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -630,6 +630,31 @@ test('boundaries', async () => {
await cleanup()
})

// https://github.com/vercel/next.js/pull/23203
test('internal package errors', async () => {
const [session, cleanup] = await sandbox()

// Make a react build-time error.
await session.patch(
'index.js',
`
import * as React from 'react';
export default function FunctionNamed() {
return <div>{{}}</div>
}`
)

expect(await session.hasRedbox(true)).toBe(true)
// We internally only check the script path, not including the line number
// and error message because the error comes from an external library.
// This test ensures that the errored script path is correctly resolved.
expect(await session.getRedboxSource()).toContain(
`../../../../packages/next/dist/pages/_document.js`
)

await cleanup()
})

test('unterminated JSX', async () => {
const [session, cleanup] = await sandbox()

Expand Down

0 comments on commit 89ec21e

Please sign in to comment.