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

Fix `<em>` issue with mixed content #1410 #1451

Merged
merged 11 commits into from May 22, 2019

Conversation

@x13machine
Copy link
Contributor

commented Mar 18, 2019

Marked version:

Markdown flavor: Markdown.pl|CommonMark|GitHub Flavored Markdown|n/a

Description

  • Fixes this bug #1410
  • I added a test case for this bug.

It was caused _ and * parsed being differently in the em regex. I just replaced the part that searches for *test* and replaced it with the part searches for _test_ and then just charged the characters. I don't know why they were different. Maybe there's a reason for it. All the tests pass, so I dunno.

Contributor

  • Test(s) exist to ensure functionality and minimize regression

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
@x13machine

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

ran wrong test command

@x13machine

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Wait, is it expecting a bug to be there? If it is, I seemed to have fixed it.

@UziTech

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Ya the spec tests that are currently failing will fail if they start passing. You will need to remove shouldFail: true from /test/specs/commonmark/commonmark.0.28.json line #3730

It looks like you are also failing some tests that used to pass.

@UziTech

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

The shouldFail lets us know which tests are fixed by each PR and let us keep track of the spec tests that we need to work on.

@x13machine

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

ok I figured out why they are different. It was to prevent it from being interpreted as an ordered List.

Also the underscore group has a bug: https://marked.js.org/demo/?text=_%3Cimg%20src%3D%22https%3A%2F%2Fwww.google.com%2Ffavicon.ico%22%20title%3D%22*_%22%2F%3E%0A%0A*%3Cimg%20src%3D%22https%3A%2F%2Fwww.google.com%2Ffavicon.ico%22%20title%3D%22*%22%2F%3E&options=&version=master

The asterisk was processed correctly. While the underscore was not. So the bug was transferred over.

So the underscore and asterisk groups need to be rewritten.

x13machine added some commits Mar 21, 2019

@x13machine

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

Ok I think, I fixed it this time.

lib/marked.js Outdated Show resolved Hide resolved
@UziTech

This comment has been minimized.

Copy link
Member

commented Mar 21, 2019

@davisjam could you check if these regexps are secure from redos?

Here are the new and old derived regexps for em:

New:

/^_([^\s_])_(?!_)|^\*([^\s*<\[])\*(?!\*)|^_([^\s<][\s\S]*?[^\s_])_(?!_|[^\s!"#$%&'()*+,\-.\/:;<=>?@\[^_{|}~])|^_([^\s_<][\s\S]*?[^\s])_(?!_|[^\s!"#$%&'()*+,\-.\/:;<=>?@\[^_{|}~])|^\*([^\s<"][\s\S]*?[^\s\*])\*(?!\*|[^\s!"#$%&'()*+,\-.\/:;<=>?@\[^_{|}~])|^\*([^\s*"<\[][\s\S]*?[^\s])\*(?!\*)/

Old:

/^_([^\s_])_(?!_)|^\*([^\s*"<\[])\*(?!\*)|^_([^\s][\s\S]*?[^\s_])_(?!_|[^\s!"#$%&'()*+,\-.\/:;<=>?@\[^_{|}~])|^_([^\s_][\s\S]*?[^\s])_(?!_|[^\s!"#$%&'()*+,\-.\/:;<=>?@\[^_{|}~])|^\*([^\s"<\[][\s\S]*?[^\s*])\*(?!\*)|^\*([^\s*"<\[][\s\S]*?[^\s])\*(?!\*)/

@UziTech UziTech requested a review from davisjam Mar 21, 2019

@UziTech

This comment has been minimized.

Copy link
Member

commented May 21, 2019

@x13machine could you rebase this so we could get it merged?

@x13machine

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

Ok I'll do that today

x13machine added some commits May 21, 2019

@x13machine

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

How do I fix the errors? 😕

@UziTech

This comment has been minimized.

Copy link
Member

commented May 21, 2019

looks like example 455 should be "shouldFail": true and the "shouldFail" line should be removed on example 418 and 477

Also move the new test files to /test/specs/new/

test/new/em_list_links.html Outdated Show resolved Hide resolved
test/new/em_list_links.md Outdated Show resolved Hide resolved
@UziTech

This comment has been minimized.

Copy link
Member

commented May 22, 2019

I'm not sure why the latest commit didn't trigger travis to run the tests, but I ran it locally and everything passed.

@UziTech UziTech requested review from styfle and joshbruce May 22, 2019

@UziTech

This comment has been minimized.

Copy link
Member

commented May 22, 2019

This still doesn't fix *[link*](url* but that can be a separate PR.

marked demo
commonmark demo

@styfle

This comment has been minimized.

Copy link
Member

commented May 22, 2019

I'm not sure why the latest commit didn't trigger travis to run the tests, but I ran it locally and everything passed.

GitHub was having issues earlier today https://twitter.com/githubstatus/status/1131242516479709185

@styfle

styfle approved these changes May 22, 2019

@styfle styfle changed the title Em bug fix #1410 Fix `<em>` issue #1410 May 22, 2019

@styfle styfle changed the title Fix `<em>` issue #1410 Fix `<em>` issue with mixed content #1410 May 22, 2019

@styfle styfle merged commit 92d0124 into markedjs:master May 22, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@203x

This comment has been minimized.

Copy link

commented Jul 15, 2019

Fix this and have a new problem

now
*test*t
*test*

parse
<p><em>test*t
*test</em></p>

right
<p><em>test</em>t
<em>test</em></p>

@UziTech

This comment has been minimized.

Copy link
Member

commented Jul 15, 2019

@203x looks like that issue is tracked in #520

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.