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

Fix #3900 return 404 on asset hash mismatch in prod #3941

Merged
merged 5 commits into from
Mar 31, 2018

Conversation

timteeling
Copy link
Contributor

Per #3900 I'm looking to have Next.js respond with a 404 when someone requests a hash from an asset that doesn't exist in production instead of what it currently does which is 500 Internal server error.

Currently, handleBuildHash in server/index.js throws an error regardless of environment, so I'm looking at tweaking it to send a 404 in production.

Example:
200 -> https://custom-server-koa-wxksegmzhf.now.sh/_next/7c048a193f99703d6180e88866b6e06b/app.js

500 Internal server error -> https://custom-server-koa-wxksegmzhf.now.sh/_next/OLD_HASH_VALUE/app.js

Thanks for your time!

Copy link
Contributor

@arunoda arunoda left a comment

Choose a reason for hiding this comment

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

This is a good one and we need to do 404 instead 500. But I'm looking for a different implementation.

server/index.js Outdated
@@ -467,7 +467,11 @@ export default class Server {
}

if (hash !== this.buildStats[filename].hash) {
throw new Error(`Invalid Build File Hash(${hash}) for chunk: ${filename}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here's console.log the ^^ message and do return this.send404(res).
In the dev mode, there's no buildId and it's been handled in few lines before.

@timteeling
Copy link
Contributor Author

Thanks @arunoda

It looks like #3793 changed how this is handled pretty significantly: https://github.com/zeit/next.js/blob/canary/server/index.js#L177

I tweaked renderScriptError now instead to return a 404 instead of 500 when INVALID_BUILD_ID is the error message

@omaksi
Copy link
Contributor

omaksi commented Mar 28, 2018

I'd like to +1 this, is there something that still needs to be improved to get this released?

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

Awesome, just verified the code 👍 thanks Tim 🙏

@timneutkens timneutkens merged commit 9ec81c0 into vercel:canary Mar 31, 2018
@omaksi
Copy link
Contributor

omaksi commented Mar 31, 2018

Thanks guys, any estimate on when could this be released on npm?

@lock lock bot locked as resolved and limited conversation to collaborators Mar 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants