Fix GFM tables not breaking on block-level structures

[calculuschild]

Marked version: 8.0

Markdown flavor: GitHub Flavored Markdown

Description

• Fixes Heading immediately following a table is being rendered in the table element #1467

What was attempted

Edited rules.js to break GFM tables properly using similar syntax used to break paragraphs.

Contributor

• Test(s) exist to ensure functionality and minimize regression (if no tests added, list tests covering this PR);

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

[vercel]

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click on the icon next to each commit.

[vercel]

Deployment failed with the following error:

Configuring `regions` is not necessary for Static Files. Deploying Serverless Functions to multiple regions is restricted to paid plans. Upgrade here: https://zeit.co/teams/markedjs/settings/billing.

[UziTech]

By including it all in the regex I'm a little worried about this introducing a new ReDos vulnerability.

I'll look into this but it might take some time.

/cc @davisjam

[vercel]

Deployment failed with the following error:

Configuring `regions` is not necessary for Static Files. Deploying Serverless Functions to multiple regions is restricted to paid plans. Upgrade here: https://zeit.co/teams/markedjs/settings/billing.

[calculuschild]

By including it all in the regex I'm a little worried about this introducing a new ReDos vulnerability.

If it helps, the only change was in the detection of table cells (the third captured regex group) where I replaced

 *[^>\n ]

with

(?!^|>|\\n| |hr|heading|lheading|code|fences|list|html)

and then apply the helper.js "edit" function in identical manner to the paragraph breaking logic earlier in the document. Note that this new bit of code does not apply repetition (“+”, “*”) to a complex subexpression

Given that, if the paragraph code has been deemed safe from REDOS attacks there shouldn't be anything different here except that I also include regex for code and lheading which were not included in the paragraph regex. Of course, I'm not a Regex expert so please do double check.

[styfle]

GitHub seems to render this one differently

[calculuschild]

Hmmm.... The spec says "The table is broken at the first empty line, or beginning of another block-level structure", so that would seem to break their rule, as the setext headings fall under the "Leaf Block" category.

At that point do you follow the spec, or do you follow GitHub's implementation of the spec? Either way, it's a simple change of just removing the associated lines that check for lheading.

[styfle]

Good point, your test seems correct. I also double checked to make sure our GFM spec tests included 201 [extension] Tables and it is passing.

[UziTech]

from the spec:

In general, a setext heading need not be preceded or followed by a blank line. However, it cannot interrupt a paragraph, so when a setext heading comes after a paragraph, a blank line is needed between them.

Maybe since it cannot interrupt a paragraph it also shouldn't interrupt a table? I'm leaning toward following the implementation and posing this as an issue to GitHub to see what they have to say. github/cmark-gfm#179

We could leave the regex in and just comment it out with a note so we can change it easily in the future.

[styfle]

Great work, thanks! 🎉

[davisjam]

Is this stable enough for me to look at the regexes for ReDoS?

…

On Mon, Feb 10, 2020, 9:55 AM Steven ***@***.***> wrote: ***@***.**** approved this pull request. Great work, thanks! 🎉 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1598?email_source=notifications&email_token=AFOD3LZYGJC6B5QRUFGPGOTRCFTFVA5CNFSM4KQEZVZKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCU4CAUY#pullrequestreview-355999827>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFOD3L4NI62KB5O6GNMSMDDRCFTFVANCNFSM4KQEZVZA> .

[UziTech]

A few edits to make the regex faster and less susceptible to a redos attack.

[UziTech]

@davisjam yes that would be great. I kind of looked it over but I'm sure you are better at finding them.

[calculuschild]

Is this stable enough for me to look at the regexes for ReDoS?

Has there been any progress on this step? Is there anything I can help with?

[UziTech]

@davisjam have you had time to look at these?