Skip to content

Commit

Permalink
Add notes on security
Browse files Browse the repository at this point in the history
  • Loading branch information
@wooorm
wooorm committed Jul 24, 2019
1 parent 5ff04d4 commit 490fe10
Showing 1 changed file with 24 additions and 0 deletions.
24 changes: 24 additions & 0 deletions readme.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ metadata found in options, a module, or `package.json` and calls
* [Supported Properties](#supported-properties)
* [API](#api)
* [`remark().use(gitContributors[, options])`](#remarkusegitcontributors-options)
* [Security](#security)
* [Contribute](#contribute)
* [Contributors](#contributors)
* [License](#license)
Expand Down Expand Up @@ -177,6 +178,23 @@ Working directory from which to resolve a `contributors` module, if any

Inject Contributors section if there is none (`boolean`, default: `false`).

## Security

`options.contributors` (or `contributors` in `package.json`) and `author` from
`package.json` are used and injected into the tree.
`git log` also runs in the current working directory.
This could open you up to a [cross-site scripting (XSS)][xss] attack if you pass
user provided content in or store user provided content in `package.json` or
Git.

This may become a problem if the Markdown later transformed to
[**rehype**][rehype] ([**hast**][hast]) or opened in an unsafe Markdown viewer.

If `contributors` is a string, it is handled as a module identifier and
loaded with `require`.
This could also be very dangerous if an attacker was able to inject code in
that package.

This comment has been minimized.

Sign in to view

Copy link
@vweevers

vweevers Jul 27, 2019

Collaborator

Be careful not to sow fear, which is already plaguing the JS ecosystem. I'm all for informing people about potential security risks, if they are realistic. Having user-provided input for options.contributors is realistic (and warning people about that is a good call), but I've not heard of anyone having user-provided input in package.json. If someone is doing that, I would call it a general bad practice that needs to be warned against elsewhere (like nodejs/package-maintenance) because it opens up many more holes.

Perhaps start this Security section with "It is assumed that remark-git-contributors is used on a project under your control, which includes its markdown files and package.json".

This comment has been minimized.

Sign in to view

Copy link
@wooorm

wooorm Jul 27, 2019

Author Member

Be careful not to sow fear, which is already plaguing the JS ecosystem.

Well, security incidents are also a bug problem in the JS ecosystem. And unified is fine for now, but with many people, a ton of projects, and a lot of use, we or our users are targets. I think it’s good to describe security.

I think it’s good to mention the attack vector: there are projects to automate that information (mostly from Git or GH), similar to what we do here. They could get “hacked”. Or links added by previously active users could turn sour.
Links may lead to unsafe websites. This could become bigger problem when for example adding target=_blank to links, leading to window.opener attacks.

Perhaps start this Security section with "It is assumed that remark-git-contributors is used on a project under your control, which includes its markdown files and package.json".

The security section already lists when this would become a problem (two last paragraphs) and uses very careful language (could, may). Do you think another “if clause” is going to help? And, if you’re working on a project under your control, don’t the last two paragraphs already answer that you’re fine?

This comment has been minimized.

Sign in to view

Copy link
@wooorm

wooorm Jul 27, 2019

Author Member

@vweevers oh btw, do you know of other projects that do security breakdowns? Looking for inspiration!

This comment has been minimized.

Sign in to view

Copy link
@vweevers

vweevers Jul 27, 2019

Collaborator

I think it’s good to describe security.

Fully agree. I'm only asking to tweak the text, to:

  1. Focus on the highest risks, that are specific to remark-git-contributors. Risks like injection via package.json detract attention from the more relevant risk of injection via options.contributors, IMO.
  2. Open with a sentence that quickly explains for which readers this section is relevant. If I was to read "It is assumed that [..] is under your control", it would put me at ease because it tells me I fit typical usage. Without that sentence, especially if I was new to this project, my first thought would be: "This thing has security risks? Bye" (exaggerated).

oh btw, do you know of other projects that do security breakdowns? Looking for inspiration!

First time I'm seeing something like this :) Which is why I commented actually, because this could become the inspiration for other projects!

This comment has been minimized.

Sign in to view

Copy link
@wooorm

wooorm Jul 29, 2019

Author Member

Opened a PR to work on a fix.

First time I'm seeing something like this :) Which is why I commented actually, because this could become the inspiration for other projects!

Coool! Yeah, I haven’t seen anything like this yet so I’m not sure how such a section should look

## Contribute

See [`contributing.md`][contributing] in [`remarkjs/.github`][health] for ways
Expand Down Expand Up @@ -269,3 +287,9 @@ abide by its terms.
[mailmap]: https://git-scm.com/docs/git-shortlog#_mapping_authors

[cwd]: https://github.com/vfile/vfile#vfilecwd

[xss]: https://en.wikipedia.org/wiki/Cross-site_scripting

[rehype]: https://github.com/rehypejs/rehype

[hast]: https://github.com/syntax-tree/hast

0 comments on commit 490fe10

Please sign in to comment.