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

test: security scan #1220

Merged
merged 2 commits into from
Apr 17, 2018
Merged

test: security scan #1220

merged 2 commits into from
Apr 17, 2018

Conversation

davisjam
Copy link
Contributor

  • npm run test:redos now scans for REDOS issues
  • added a Travis stage for 'security scan', which at the moment just checks for REDOS

Fixes: #1201.

@davisjam
Copy link
Contributor Author

Not sure whether the CI stage is configured properly. I don't think CI changes take effect until after it's merged though, so someone should check it over.

@davisjam
Copy link
Contributor Author

davisjam commented Apr 15, 2018

Actually it seems the Travis run is picking up the security scan, here. Results are as expected, cf. #1223

The security scan took 60 seconds, of which running the eslint plugin took 13 seconds. So I don't think querying my REDOS DB is problematic performance-wise.

@UziTech
Copy link
Member

UziTech commented Apr 15, 2018

I still don't think the eslint plugin is all that helpful in our situation because of the way we construct the regexes.

@davisjam
Copy link
Contributor Author

davisjam commented Apr 16, 2018

As you've pointed out earlier, a static analysis won't guarantee protection. I disagree, however, that it "is [not] all that helpful". It has already identified several REDOS issues, and it is certainly much better than nothing. I therefore suggest we adopt it.

We can open an issue to revisit this with a more thorough approach. Exposing the module's regexes and invoking vuln-regex-detector on them from the test suite would be a nice extension.

@styfle
Copy link
Member

styfle commented Apr 16, 2018

@davisjam CI is showing a vulnerability.

557:14  error  Regex pattern vulnerable to catastrophic backtracking: ^!?\[(label)\]\(\s*<?([\s\S]*?)>?(?:\s+(['"][\s\S]*?['"]))?\s*\)  vuln-regex-detector/no-vuln-regex

Is this a false positive? If not, can you fix it?

Ok I see now you have opened #1223
I'll take a look at that one.

@davisjam
Copy link
Contributor Author

Is this a false positive?

This scan will not produce false positives. This tool only flags a regex if it has demonstrated super-linear behavior against it. It will have some false negatives, e.g. it did not catch #1224 .

@joshbruce
Copy link
Member

I like the idea of CI covering us on security. Not going to require this be there for the 0.4.0 release as it seems like there might be some more conversation desired.

@styfle
Copy link
Member

styfle commented Apr 17, 2018

@davisjam Want to rebase on master?

- 'npm run test:redos' now scans for REDOS issues
- added a Travis stage for 'security scan'

Fixes: markedjs#1201 (a step towards it, anyway)
@davisjam
Copy link
Contributor Author

@styfle Rebased in 02b1343.

@styfle styfle requested a review from UziTech April 17, 2018 01:26
Copy link
Member

@UziTech UziTech left a comment

Choose a reason for hiding this comment

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

I agree this is better than nothing, but I think we still need to do more.

.travis.yml Outdated
@@ -13,6 +13,10 @@ jobs:
- node_js: lts/*
- node_js: node

- stage: security scan
Copy link
Member

Choose a reason for hiding this comment

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

security scan 🔐

Copy link
Contributor Author

@davisjam davisjam Apr 17, 2018

Choose a reason for hiding this comment

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

Fixed.

Where do you guys find these cool symbols?

Copy link
Member

Choose a reason for hiding this comment

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

@davisjam If you type : you can start typing words and find emoji 🔍

Otherwise, checkout https://emojipedia.org 😄

zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
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.

4 participants