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

Add X-Powered-By header. #416

Merged
merged 4 commits into from
Dec 19, 2016
Merged

Add X-Powered-By header. #416

merged 4 commits into from
Dec 19, 2016

Conversation

arunoda
Copy link
Contributor

@arunoda arunoda commented Dec 17, 2016

See:

sh-3.2$ curl -v http://localhost:3000
* Rebuilt URL to: http://localhost:3000/
*   Trying ::1...
* Connected to localhost (::1) port 3000 (#0)
> GET / HTTP/1.1
> Host: localhost:3000
> User-Agent: curl/7.49.0
> Accept: */*
>
< HTTP/1.1 200 OK
< Content-Type: text/html
< Content-Length: 3172
-------------------------------
< X-Powered-By: Next.js 1.2.3
-------------------------------
< Date: Sat, 17 Dec 2016 11:38:52 GMT
< Connection: keep-alive

@coveralls
Copy link

coveralls commented Dec 17, 2016

Coverage Status

Coverage increased (+0.01%) to 57.143% when pulling 948c348 on arunoda:x-powered-by into b62a0e8 on zeit:master.

@STRML
Copy link

STRML commented Dec 17, 2016

This feels like a bad idea - in general you don't want to leak information about the server in any production app - the framework name is bad enough, the exact version is worse because it makes it that much easier to target exploits.

@@ -102,6 +104,7 @@ export async function renderErrorJSON (err, res, { dir = process.cwd(), dev = fa
export function sendHTML (res, html) {
res.setHeader('Content-Type', 'text/html')
res.setHeader('Content-Length', Buffer.byteLength(html))
res.setHeader('X-Powered-By', `Next.js ${pkg.version}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

could use pkg.name to DRY

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But it only says next. Not Next.js.

@jamo
Copy link
Contributor

jamo commented Dec 17, 2016

Yeah, +1 for obfuscating the version. Tho if the page downloads next.js/<version>/next.min.js it's pretty obvious which version and framework is being used...

@arunoda
Copy link
Contributor Author

arunoda commented Dec 17, 2016

Securing apps via obfuscating is a good start but it doesn't hold any good hacker to do his/her thing. (If there's an issue).

This is something Express does all the times.

What they have a way to turn it off. I'll think about something like that.

@rauchg
Copy link
Member

rauchg commented Dec 17, 2016

All major web servers do this.
It allows us to in the future actually responsibly disclose security problems!
That said, @arunoda it'd be helpful to introduce a config flag poweredByHeader: false to turn it off

@arunoda
Copy link
Contributor Author

arunoda commented Dec 17, 2016

@rauchg That's a good suggestion.
I'll hold this PR and do it once we've land #222

@arunoda
Copy link
Contributor Author

arunoda commented Dec 17, 2016

@rauchg check now.
We could disable setting the header.
Additionally, we could access the config via app.config if the user uses #310

@coveralls
Copy link

coveralls commented Dec 17, 2016

Coverage Status

Coverage decreased (-0.06%) to 57.422% when pulling ce5605c on arunoda:x-powered-by into 5ab7463 on zeit:master.

@rauchg
Copy link
Member

rauchg commented Dec 17, 2016

And a test would be nice to add as well!

@arunoda
Copy link
Contributor Author

arunoda commented Dec 18, 2016

@rauchg added some test cases.

@coveralls
Copy link

coveralls commented Dec 18, 2016

Coverage Status

Coverage increased (+3.2%) to 60.635% when pulling e2cb912 on arunoda:x-powered-by into 5ab7463 on zeit:master.

@rauchg rauchg merged commit 26c485a into vercel:master Dec 19, 2016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 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.

6 participants