Skip to content

doc: add security section to debugging guides #1613

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

Merged
merged 2 commits into from
Apr 14, 2018

Conversation

ofrobots
Copy link
Contributor

@ofrobots ofrobots commented Apr 3, 2018

Copy link
Contributor

@marswong marswong left a comment

Choose a reason for hiding this comment

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

good job. but was this too long for a "getting started" guide of "debugging"? what about creating a new doc for security and link here?

Copy link
Contributor

@daxlab daxlab left a comment

Choose a reason for hiding this comment

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

👍

### Browsers, WebSockets and same-origin policy

Websites open in a web-browser can make WebSocket and HTTP requests under the
browser security model. A initial HTTP connection is necessary to obtain a
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: A initial -> An initial.

$ node --inspect server.js
```

Now, on your local machine from where you want initiate a debug client
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: want initiate -> want to initiate.

threat. We suggest you ensure appropriate firewalls and access controls in place
to prevent a security exposure.

See the section on 'Enabling remote debugging scenarios' on some advice on how
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 3, 2018

Choose a reason for hiding this comment

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

Nit: maybe it is worth to add a link to #enabling-remote-debugging-scenarios?

@ofrobots
Copy link
Contributor Author

ofrobots commented Apr 3, 2018

I think a discussion of the security risks needs to be up-front in the debugging guide. It might be possible have a summary section and link to a detailed document elsewhere. Let me take a look at refactoring.

@ofrobots
Copy link
Contributor Author

@marswong I took a deeper look at refactoring this PR into two document, and I don't think there is a refactor into two documents that would be satisfactory:

  • We need to make people aware that there are security implications. These should be in the primary debugging guide.
  • The details about how to do remote debugging must also be in the primary debugging guide. Those are instructions.
  • The only thing that can be split out is the nature of the security issues. I tried extracting these into a standalone doc, but I wasn't satisfied with the result because the security doc has to refer back to the guide on how to enable remote debugging in light of the security issues.

In summary, I think we should keep this a single doc. Even though this doc is called the 'Debugging Getting Started' guide, in reality it is actually the 'Debugging Guide'. I think security implications should stay in this doc.

I would like to land this as-is. WDYT?

@fhemberger
Copy link
Contributor

@ofrobots Can you please fix the little nits @vsemozhetbyt mentioned, and I think we are good to go. If we feel the need to restructure the document, we can do so at a later point.

@fhemberger fhemberger merged commit d92273e into nodejs:master Apr 14, 2018
@fhemberger
Copy link
Contributor

Thank you!

@marswong
Copy link
Contributor

@ofrobots good job :)

@ofrobots ofrobots deleted the inspector-security branch April 16, 2018 18:18
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.

7 participants