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

Response timing #315

Closed
wants to merge 2 commits into from
Closed

Response timing #315

wants to merge 2 commits into from

Conversation

bkeepers
Copy link
Contributor

@bkeepers bkeepers commented Nov 3, 2017

This adds the X-Response-Time header to report how long a request takes. I also noticed the X-Powered-By header was reporting Express. I debated about updating it to Probot, but it is generally considered bad practice to include this header, so I just disabled it entirely.

This is generally considered bad security practice.
@bkeepers bkeepers requested a review from a team November 3, 2017 19:27
Copy link
Contributor

@hiimbex hiimbex left a comment

Choose a reason for hiding this comment

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

LGTM!

const path = require('path')

module.exports = function (webhook) {
const app = express()
app.set('x-powered-by', false)
Copy link
Contributor

Choose a reason for hiding this comment

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

I encourage you to read this statement from an Express maintainer about this issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't disagree with his point about security by obscurity.

I would be more sympathetic to the "give credit to the hard-working people who have enabled you to write your code so fast" argument if it also acknowledged all the tools that express has used for free that enabled it to receive such prominence (like Node.js, npm and GitHub for hosting the project for free, Sinatra and rack from ruby since that heavily influence the design, etc).

The header is still frivolous, and there are better ways to acknowledge people who made it possible.

Copy link
Member

Choose a reason for hiding this comment

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

I'm all for surfacing the packages we use somewhere to give ample credit (they certainly deserve it), but I don't think anyone looks at headers and says "Hey what's Express?" - so I'm okay with doing this.

@bkeepers
Copy link
Contributor Author

I'm going to close this in favor of #322, which has to do its own timing.

@bkeepers bkeepers closed this Nov 21, 2017
@bkeepers bkeepers deleted the request-timing branch November 21, 2017 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants