Skip to content

GFM strikethrough compatibility - #1258

Merged
styfle merged 2 commits into
markedjs:masterfrom
tomtheisen:gfm-strike
Jul 9, 2018
Merged

GFM strikethrough compatibility#1258
styfle merged 2 commits into
markedjs:masterfrom
tomtheisen:gfm-strike

Conversation

@tomtheisen

@tomtheisentomtheisen commented May 9, 2018•
edited by styfle
Loading

Copy link
Copy Markdown

Marked version: 0.3.19

Markdown flavor: GitHub Flavored Markdown

Description

This pull request brings GFM strikethrough compatibility.

Contributor

  • Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR); or,
  • no tests required for this PR.
  • If submitting new feature, it has been documented in the appropriate places.

Committer

In most cases, this should be a different person than the contributor.

  • Draft GitHub release notes have been updated.
  • CI is green (no forced merge required).
  • Merge PR

@tomtheisen

tomtheisen commented May 9, 2018

Copy link
Copy Markdown
Author

I don't think this is catastrophically backtracking regex.

UziTech
UziTech approved these changes May 9, 2018
View reviewed changes

@UziTechUziTech left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🥇

@UziTech
UziTech requested a review from davisjamMay 9, 2018 04:25
davisjam
davisjam reviewed May 9, 2018
View reviewed changes
Comment threadlib/marked.js
.getRegex(),
_backpedal: /(?:[^?!.,:;*_~()&]+|\([^)]*\)|&(?![a-zA-Z0-9]+;$)|[?!.,:;*_~)]+(?!$))+/,
del: /^~~(?=\S)([\s\S]*?\S)~~/,
del: /^~+(?=\S)([\s\S]*?\S)~+/,

@davisjamdavisjamMay 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  1. Could this be simplified (no look-ahead) to /^~+([^\s~][\s\S]*?\S)~+/?
  2. Why not ~~ leading/trailing spaces ~~? Seems like the spec permits this.

@UziTechUziTechMay 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. The spec might allow it but github doesn't

without space:
asdf

with space:
~ asdf ~

@davisjamdavisjamMay 9, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Interesting. Would this work? /^~+([^~]+)~+/.

(This regex will accept the "with space" version which I think is appropriate despite GitHub's treatment of it).

The spec says "No nesting" which I think means that the first ~ should terminate the strike-through.

@UziTechUziTechMay 9, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we should act like GitHub when the spec is ambiguous

@davisjamdavisjamMay 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

To answer my own question, /^~+([^\s~][\s\S]*?\S)~+/ won't work -- it requires a minimum of two characters between the ~s.

@davisjamdavisjamMay 9, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned that this regex (old and new versions) will match ~~~. How about

/^~+(?=[^\s~])([^~]*[^\s~])~+/

which will:

  1. Ensure that the "text" neither begins nor ends with whitespace or a ~; and
  2. Explicitly prevent nesting without needing to rely on the non-greedy *? behavior.

Thoughts?

@tomtheisentomtheisenMay 9, 2018

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Is there any particular reason to avoid the non-greedy modifier?

@davisjamdavisjamMay 9, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm nervous about the structure /~+...[\s\S]*?...~+/. I do not believe that as written the regex is vulnerable, but a slight modification would make it so. If the middle group excludes a ~ character then the risk is removed.

@styflestyfleJul 6, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@davisjam If this is not vulnerable as written then I think we should merge it because it brings us into compliance. Adding @joshbruce to take a look.

@styfle
styfle requested a review from joshbruceJuly 6, 2018 19:51
@styflestyfle changed the title Gfm strikethrough compatibilityGFM strikethrough compatibilityJul 9, 2018
@styfle

styfle commented Jul 9, 2018

Copy link
Copy Markdown
Member

Thanks Tom! 🎉

@styfle
styfle merged commit c7d487e into markedjs:masterJul 9, 2018
@StephenHogstenStephenHogsten mentioned this pull request Dec 14, 2018
Closed
@snyk-botsnyk-bot mentioned this pull request Aug 19, 2019
Open
This was referenced Aug 30, 2019
Open
Open
@snyk-botsnyk-bot mentioned this pull request Sep 24, 2019
Open
This was referenced Oct 26, 2019
Open
Open
Open
Closed
Open
@snyk-botsnyk-bot mentioned this pull request Nov 4, 2019
Open
This was referenced Nov 18, 2019
Open
Open
Open
Open
This was referenced Mar 29, 2020
Open
Open
Open
Open
Open
Open
Open
Open
Open
This was referenced Apr 5, 2020
Open
Open
Open
Open
Open
Open
Open
Open
Open
Closed
Open
Open
Closed
Open
Open
Open
This was referenced Apr 12, 2020
Open
Open
Open
Open
zhenalexfan pushed a commit to zhenalexfan/MarkdownHan that referenced this pull request Nov 8, 2021
@styfle
Merge pull requestmarkedjs#1258from tomtheisen/gfm-strike
a2827b8
GFM strikethrough compatibility
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UziTechUziTechUziTech approved these changes
@styflestyflestyfle approved these changes
@davisjamdavisjamdavisjam approved these changes
@joshbrucejoshbruceAwaiting requested review from joshbruce

Assignees

No one assigned

Labels

NFS - new feature (spec related)parser: GFM

Projects

None yet

Milestone

0.5.0 - Commonmark Compliance

Development

Successfully merging this pull request may close these issues.

4 participants

@tomtheisen@styfle@UziTech@davisjam