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

Make URL handling consistent between links and images #1359

Merged
merged 5 commits into from
Oct 19, 2018

Conversation

aprotim
Copy link
Contributor

@aprotim aprotim commented Oct 17, 2018

Marked version:

b6d5a67

Markdown flavor: all

Description

Expectation

With sanitization on, I'd expect

[Link](javascript:alert('xss'))
![Image](javascript:alert('xss'))

To result in

<p>Link
Image</p>

Result

<p>Link <img src="javascript:alert('xss')" alt="Image"></p>

What was attempted

marked("[Link](javascript:alert('xss'))\n![Image](javascript:alert('xss'))", {sanitize: true})

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

@UziTech
Copy link
Member

UziTech commented Oct 18, 2018

I think we are trying to get rid of the sanitize option. Sanitizing can be very difficult to maintain and there are other packages that do a great job.

#1232 (comment)

@aprotim
Copy link
Contributor Author

aprotim commented Oct 18, 2018

Even if that's the eventual goal, this change will still increase consistency, make it easier to remove sanitization later (only one place to change the code), and make sure all other URL munging (like baseUrl resolution) stays consistent between links and images.

@styfle
Copy link
Member

styfle commented Oct 18, 2018

@UziTech I agree we should get rid of the sanitize option.

But I think @aprotim has some valid points here.

The way I understand this PR, it makes sure both link urls and image urls are sanitized in the same way. I would think everyone would benefit from this change. It just might send the wrong message if the release notes show we are making changes to the sanitizer and the next release it gets removed.

@aprotim aprotim changed the title Fix url sanitization Make URL handling consistent between links and images Oct 18, 2018
@aprotim
Copy link
Contributor Author

aprotim commented Oct 18, 2018

Is the rename of this PR sufficient to address that concern, @styfle?

@aprotim
Copy link
Contributor Author

aprotim commented Oct 18, 2018

I'm not super familiar with the flow here, so just making sure there's nothing else I'm supposed to do before these PRs get merged?

@styfle
Copy link
Member

styfle commented Oct 19, 2018

@aprotim We need 2 approvals before we can merge this. I approved, so one other person will need to approve. @UziTech expressed some reservations so we'll see what the rest of the team thinks. There's nothing you need to do besides wait 👍

---
sanitize: true
---
[URL](javascript:alert)
Copy link
Contributor

Choose a reason for hiding this comment

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

These are links, not images. ! are missing

@aprotim
Copy link
Contributor Author

aprotim commented Oct 19, 2018

Whoops! I definitely thought I'd changed those. Maybe I accidentally blew away my changes while I was trying to figure out how to do the branching right. Fixed?

Copy link
Member

@joshbruce joshbruce left a comment

Choose a reason for hiding this comment

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

Agree with @UziTech on sanitizer in general. Let's revisit when we get to 1.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants