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 security.txt file #1589

Merged
merged 2 commits into from
Mar 13, 2018
Merged

Add security.txt file #1589

merged 2 commits into from
Mar 13, 2018

Conversation

MarcinHoppe
Copy link
Contributor

security.txt (https://securitytxt.org/) files are getting an accepted industry standard and it would be beneficial to adopt this standard for Node.js Web site as well.

This addition to the Web site has already been endorsed by the Security WG.

This PR resolves nodejs/security-wg#143.

security.txt (https://securitytxt.org/) files are getting an
accepted industry standard and it would be beneficial to
adopt this standard for Node.js Web site as well.

This addition to the Web site has already been endorsed by
the Security WG:

nodejs/security-wg#143
server.js Outdated
@@ -89,6 +96,7 @@ statics.on('add', (filePath) => {

// Initializes the server and mounts it in the generated build directory.
http.createServer((req, res) => {
if (wellknown(req, res)) return
Copy link
Member

Choose a reason for hiding this comment

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

This server is only used for development so I'm not sure if it's worth adding. In production the site is served by NGINX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then in production it would be good to add /security.txt -> /.well-known/security.txt redirect as recommended by the spec and employed by a couple of bigger sites on the Web.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@lpinca security.txt should be in the repo, file would be served in production like our static files.

We just need to add

location /security.txt {
    alias /home/www/nodejs/.well-known/security.txt;
    default_type text/plain;
}

in the nginx config.

Copy link
Member

Choose a reason for hiding this comment

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

Yes exactly that's what I was suggesting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't mind it at all. Keeps things simple. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, so the bottom line is to put security.txt into the static folder and handle /security.txt and /.well-known/security.txt in NGINX config and get rid of everything else in server.js and build.js?

Copy link
Member

Choose a reason for hiding this comment

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

That works for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for all the feedback, I will make a change and submit a second PR against nodejs/build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for all the feedback, I will make a change and submit a second PR against nodejs/build.

NGINX configuration in the nodejs/build repo will be updated to
serve this file on URLs recommended in the specification.
@MarcinHoppe
Copy link
Contributor Author

I left the bare security.txt file here and updated NGINX configuration in a separate PR: nodejs/build#1181.

All feedback appreciated!

@lpinca lpinca merged commit 7e6ee85 into nodejs:master Mar 13, 2018
@lpinca
Copy link
Member

lpinca commented Mar 13, 2018

Thank you!

@MarcinHoppe MarcinHoppe deleted the security-txt branch March 14, 2018 07:15
@MarcinHoppe
Copy link
Contributor Author

My pleasure!

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.

security.txt file on nodejs.org
7 participants