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

Update middleware eval checks #30883

Merged
merged 7 commits into from Nov 5, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
22 changes: 17 additions & 5 deletions packages/next/server/dev/next-dev-server.ts
Expand Up @@ -514,7 +514,13 @@ export default class DevServer extends Server {
parsed: UrlWithParsedQuery
}): Promise<FetchEventResult | null> {
try {
const result = await super.runMiddleware(params)
const result = await super.runMiddleware({
...params,
onWarning: (warn) => {
this.logErrorWithOriginalStack(warn, 'warning', 'client')
},
})

result?.waitUntil.catch((error) =>
this.logErrorWithOriginalStack(error, 'unhandledRejection', 'client')
)
Expand Down Expand Up @@ -589,7 +595,7 @@ export default class DevServer extends Server {

private async logErrorWithOriginalStack(
err?: unknown,
type?: 'unhandledRejection' | 'uncaughtException',
type?: 'unhandledRejection' | 'uncaughtException' | 'warning',
stats: 'server' | 'client' = 'server'
) {
let usedOriginalStack = false
Expand Down Expand Up @@ -630,11 +636,15 @@ export default class DevServer extends Server {
const { file, lineNumber, column, methodName } = originalStackFrame

console.error(
chalk.red('error') +
(type === 'warning' ? chalk.yellow('warn') : chalk.red('error')) +
' - ' +
`${file} (${lineNumber}:${column}) @ ${methodName}`
)
console.error(`${chalk.red(err.name)}: ${err.message}`)
console.error(
`${(type === 'warning' ? chalk.yellow : chalk.red)(err.name)}: ${
err.message
}`
)
console.error(originalCodeFrame)
usedOriginalStack = true
}
Expand All @@ -647,7 +657,9 @@ export default class DevServer extends Server {
}

if (!usedOriginalStack) {
if (type) {
if (type === 'warning') {
Log.warn(err + '')
Copy link
Member

Choose a reason for hiding this comment

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

Why no "warning" prefix?

Copy link
Member Author

Choose a reason for hiding this comment

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

@styfle The prefix is added by Log. The one that is interpolated in the line below is for unhandledRejection and uncaughtException. The message will show up prefixed.

} else if (type) {
Log.error(`${type}:`, err + '')
} else {
Log.error(err + '')
Expand Down
7 changes: 7 additions & 0 deletions packages/next/server/next-server.ts
Expand Up @@ -611,6 +611,7 @@ export default class Server {
response: ServerResponse
parsedUrl: ParsedNextUrl
parsed: UrlWithParsedQuery
onWarning?: (warning: Error) => void
}): Promise<FetchEventResult | null> {
this.middlewareBetaWarning()

Expand Down Expand Up @@ -672,6 +673,12 @@ export default class Server {
page: page,
},
ssr: !!this.nextConfig.experimental.concurrentFeatures,
onWarning: (warning: Error) => {
if (params.onWarning) {
warning.message += ` "./${middlewareInfo.name}"`
params.onWarning(warning)
}
},
})

for (let [key, value] of result.response.headers) {
Expand Down
14 changes: 10 additions & 4 deletions packages/next/server/web/sandbox/sandbox.ts
Expand Up @@ -10,6 +10,7 @@ import vm from 'vm'
let cache:
| {
context: { [key: string]: any }
onWarning: (warn: Error) => void
paths: Map<string, string>
require: Map<string, any>
sandbox: vm.Context
Expand All @@ -34,6 +35,7 @@ export function clearSandboxCache(path: string, content: Buffer | string) {

export async function run(params: {
name: string
onWarning: (warn: Error) => void
paths: string[]
request: RequestData
ssr: boolean
Expand Down Expand Up @@ -91,6 +93,7 @@ export async function run(params: {

cache = {
context,
onWarning: params.onWarning,
paths: new Map<string, string>(),
require: new Map<string, any>([
[require.resolve('next/dist/compiled/cookie'), { exports: cookie }],
Expand Down Expand Up @@ -121,6 +124,8 @@ export async function run(params: {
map: { Request: 'Request' },
},
])
} else {
cache.onWarning = params.onWarning
}

for (const paramPath of params.paths) {
Expand Down Expand Up @@ -233,12 +238,13 @@ function isRequestLike(obj: unknown): obj is Request {
function __next_eval__(fn: Function) {
const key = fn.toString()
if (!cache?.warnedEvals.has(key)) {
console.warn(
new Error(
`Dynamic Code Evaluation (e. g. 'eval', 'new Function') not allowed in Middleware`
).stack
const warning = new Error(
`Dynamic Code Evaluation (e. g. 'eval', 'new Function') not allowed in Middleware`
)
warning.name = 'DynamicCodeEvaluationWarning'
Error.captureStackTrace(warning, __next_eval__)
cache?.warnedEvals.add(key)
cache?.onWarning(warning)
}
return fn()
}
12 changes: 12 additions & 0 deletions test/integration/middleware/with-eval/test/index.test.js
@@ -1,12 +1,14 @@
/* eslint-env jest */

import stripAnsi from 'next/dist/compiled/strip-ansi'
import { join } from 'path'
import {
fetchViaHTTP,
findPort,
killApp,
launchApp,
nextBuild,
waitFor,
} from 'next-test-utils'

const context = {}
Expand All @@ -22,6 +24,7 @@ describe('Middleware usage of dynamic code evaluation', () => {
beforeAll(async () => {
context.appPort = await findPort()
context.app = await launchApp(context.appDir, context.appPort, {
env: { __NEXT_TEST_WITH_DEVTOOL: 1 },
onStdout(msg) {
output += msg
},
Expand All @@ -37,13 +40,20 @@ describe('Middleware usage of dynamic code evaluation', () => {
it('shows a warning when running code with eval', async () => {
const res = await fetchViaHTTP(context.appPort, `/using-eval`)
const json = await res.json()
await waitFor(500)
expect(json.value).toEqual(100)
expect(output).toContain(DYNAMIC_CODE_ERROR)
javivelasco marked this conversation as resolved.
Show resolved Hide resolved
expect(output).toContain('DynamicCodeEvaluationWarning')
expect(output).toContain('pages/_middleware')
expect(output).toContain('lib/utils.js')
expect(output).toContain('usingEval')
expect(stripAnsi(output)).toContain("value: eval('100')")
})

it('does not show warning when no code uses eval', async () => {
const res = await fetchViaHTTP(context.appPort, `/not-using-eval`)
const json = await res.json()
await waitFor(500)
expect(json.value).toEqual(100)
expect(output).not.toContain(DYNAMIC_CODE_ERROR)
})
Expand All @@ -61,6 +71,8 @@ describe('Middleware usage of dynamic code evaluation', () => {

it('should have middleware warning during build', () => {
expect(buildResult.stderr).toContain(`Failed to compile`)
expect(buildResult.stderr).toContain(`Used by usingEval`)
expect(buildResult.stderr).toContain(`./pages/_middleware.js`)
expect(buildResult.stderr).toContain(DYNAMIC_CODE_ERROR)
javivelasco marked this conversation as resolved.
Show resolved Hide resolved
})
})
Expand Down