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

chore(deps): update marked to ^0.7.0 #102

Merged
merged 3 commits into from Aug 2, 2019
Merged

chore(deps): update marked to ^0.7.0 #102

merged 3 commits into from Aug 2, 2019

Conversation

@curbengh
Copy link
Contributor

curbengh commented Jul 8, 2019

https://snyk.io/vuln/SNYK-JS-MARKED-451341

sanitize: and tables: options have been deprecated.

tables: is now part of gfm:.

Changelog
  • Security

    • Sanitize paragraph and text tokens #1504
    • Fix ReDOS for links with backticks (issue #1493) #1515
  • Breaking Changes

    • Deprecate sanitize and sanitizer options #1504
    • Move fences to CommonMark #1511
    • Move tables to GFM #1511
    • Remove tables option #1511
    • Single backtick in link text needs to be escaped #1515
  • Fixes

    • Fix parentheses around a link #1509
    • Fix headings (issue #1510) #1511
  • Tests

    • Run tests with correct options #1511

Closes #101

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jul 8, 2019

Coverage Status

Coverage remained the same at 83.721% when pulling ce94875 on weyusi:marked into e6752e5 on hexojs:master.

@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Jul 8, 2019

Upstream recommends third-party packages (e.g. DOMPurify, sanitize-html or insane) to sanitize html.

I don't see the need of it in the context of server-side rendering, but can be considered if there are interests.

@tomap

This comment has been minimized.

Copy link
Contributor

tomap commented Jul 11, 2019

I believe you should keep the sanitize option as it applies to the plugin option, not marked.
I don't see why you removed it?

@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Jul 11, 2019

I think it applies to both plugin and marked. I'll do some more testing.

@curbengh curbengh force-pushed the curbengh:marked branch from d95a1c0 to 8b90ad5 Jul 12, 2019
@curbengh curbengh force-pushed the curbengh:marked branch from 8b90ad5 to dd25e42 Jul 12, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Jul 12, 2019

I can confirm sanitize also applies to marked,
sanitize

I restored plugin's sanitize, but renamed it to sanitizeUrl to avoid confusion.

@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Jul 12, 2019

I'm still not sure what these lines do (which I restored in the last commit),

try {
prot = decodeURIComponent(unescape(href))
.replace(/[^\w:]/g, '')
.toLowerCase();
} catch (e) {
return '';
}

I tried [test-link](http://exAmple.com/%D1%88%D0%B5%D0%BB%D0%BB%D1%8B), but it didn't convert to lowercase nor the rest decoded.

Above line added by @NoahDragon #34


Edit: I got it working through

    href = decodeURIComponent(href)
          .replace(/[^\w:\/\.]/g, '')
          .toLowerCase() || '';

not sure if it's what the original code intended.

@curbengh curbengh force-pushed the curbengh:marked branch from dd25e42 to ce94875 Jul 12, 2019
@tomap

This comment has been minimized.

Copy link
Contributor

tomap commented Jul 12, 2019

LGTM. It should trigger a minor version update when we publish a new version (due to change of behavior)

@tomap
tomap approved these changes Jul 12, 2019
@curbengh

This comment has been minimized.

Copy link
Contributor Author

curbengh commented Jul 15, 2019

It should be a major version due to #98.

@tomap

This comment has been minimized.

Copy link
Contributor

tomap commented Aug 2, 2019

Agreed. I merge

@tomap tomap merged commit 483d99d into hexojs:master Aug 2, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 83.721%
Details
@tomap tomap mentioned this pull request Aug 2, 2019
@tomap

This comment has been minimized.

Copy link
Contributor

tomap commented Aug 2, 2019

@curbengh curbengh deleted the curbengh:marked branch Aug 2, 2019
@YoshinoriN YoshinoriN added this to the v2.0.0 milestone Aug 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.