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

Jump to bottom

fix image links with escaped brackets #1683

Merged
merged 2 commits into from May 20, 2020
Merged

fix image links with escaped brackets #1683

merged 2 commits into from May 20, 2020

Conversation

UziTech
Copy link
Member

@UziTech UziTech commented May 19, 2020 •
edited

Marked version: 1.1.0

Description

Fix image link with \[ or \] in alt text

demo

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

@vercel
Copy link

vercel bot commented May 19, 2020 •
edited

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

🔍 Inspect: https://vercel.com/markedjs/markedjs/qpzfg7jxn
✅ Preview: https://markedjs-git-fork-uzitech-image-links.markedjs.now.sh

UziTech
UziTech commented May 19, 2020
View reviewed changes
src/rules.js Outdated
inline._label = /(?:\[[^\[\]]*\]|\\.|`[^`]*`|[^\[\]\\`])*?/;
inline._label = /(?:\[(?:\\.|[^\[\]])*\]|\\.|`[^`]*`|[^\[\]\\`])*?/;
Copy link
Member Author

Choose a reason for hiding this comment

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

@davisjam is this regex safe from ReDOS?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly, it has moved from linear to exponential.

Visualization -- The problem is in the top group. The OR can be satisfied on the top or the bottom using \\a, and if the subsequent ] is missing then you get exponential backtracking.

Example misbehavior:

/\[(?:\\.|[^\[\]])*\]/.exec("[" + "\\a".repeat(300));

Copy link
Member Author

@UziTech UziTech May 20, 2020 •
edited

Choose a reason for hiding this comment

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

what about removing \\ from the second path (/\[(?:\\.|[^\\\[\]])*\]/)? that way the only path it can take is determined by whether it sees a \\

Copy link
Member Author

Choose a reason for hiding this comment

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

@davisjam thanks for your help on this. Your regex experience is invaluable.

davisjam
davisjam requested changes May 20, 2020
View reviewed changes
Copy link
Contributor

@davisjam davisjam left a comment

Choose a reason for hiding this comment

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

Introduces an exponential regex

davisjam
davisjam approved these changes May 20, 2020
View reviewed changes
Copy link
Contributor

@davisjam davisjam left a comment

Choose a reason for hiding this comment

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

New regex is back to linear time.

@UziTech UziTech merged commit 04ea199 into markedjs:master May 20, 2020
@UziTech UziTech deleted the image-links branch May 20, 2020 15:49
@UziTech UziTech mentioned this pull request Jul 13, 2020
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@styfle styfle styfle approved these changes

@davisjam davisjam davisjam approved these changes

@joshbruce joshbruce Awaiting requested review from joshbruce

Assignees
No one assigned
Labels
category: images
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Alt text with escaped characters breaks image links
3 participants
@UziTech @styfle @davisjam