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

Fixed custom 404 pages #218

Merged
merged 6 commits into from
Jul 2, 2017
Merged

Fixed custom 404 pages #218

merged 6 commits into from
Jul 2, 2017

Conversation

rosszurowski
Copy link
Contributor

@rosszurowski rosszurowski commented Jun 8, 2017

This PR fixes an issue with serve v5.2.2 where a custom 404 page (ie. 404.html) wasn't being sent.

From my triaging, it seems like the issue is with this line, which checks if flags.single === undefined. Since the default value is actually false this block never gets triggered and we never end up sending the custom 404 page.

This change doesn't seem to introduce regressions even with different options, but it's hard to actually tell without automated tests 😢

@DonnieWest
Copy link

@leo Is there anything I can do to help get this merged in? This seems like it'll fix my custom 404 page issues and also my service worker on my website

@leo
Copy link
Contributor

leo commented Jun 20, 2017

The problem is that we need to ensure that this doesn't happen again. I've recently merged this and it brought the problem back after it was fixed. Since your PR looked similar, I thought it might create the same problem again.

@rosszurowski
Copy link
Contributor Author

@leo What exactly is that issue? I can't find any details about it other than that:

The issue was associated with the usage of the send middleware without supplying a root option.

If you let me know more (eg. what kind of request can cause the issue, how it's related to the 404 page), I'm happy to manually test this PR to ensure that there's no regression.

Also, would you be open to a PR introducing automated testing? Seems like a good way to ensure that kind of regression never happens again.

@d4rky-pl
Copy link

From what I see there's exactly the same code few lines below - https://github.com/zeit/serve/blob/master/lib/server.js#L138 - so one way or another there's a bug!

I've been trying to figure out what the security issue is and how that change could reintroduce it. From what I see in next.js changes in 2.4.1 version there is only one change related to security and I'm afraid I'm not that knowledgeable in internals of both next.js and this library to guess how those two are connected :(

@leo it would be massively helpful if you provided more info on what the security issue in this case was and if there's some way of mitigating it. I'm happy to dive into it and provide necessary patch but I need more info, POC, description, anything :)

@leo leo changed the title Fix custom 404 page Fixed custom 404 pages Jul 2, 2017
@leo leo merged commit 0234ba4 into vercel:master Jul 2, 2017
@leo
Copy link
Contributor

leo commented Jul 2, 2017

Sweet! I checked it manually and everything seems to work correctly. Thanks! 😊

@vercel vercel locked and limited conversation to collaborators Jul 2, 2017
@rosszurowski rosszurowski deleted the fix-custom-404 branch July 7, 2017 15:09
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