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

Allow users to specify tags that can be used with disallowed raw html #507

Closed
joshbruce opened this issue Jun 22, 2020 · 6 comments · Fixed by #512
Closed

Allow users to specify tags that can be used with disallowed raw html #507

joshbruce opened this issue Jun 22, 2020 · 6 comments · Fixed by #512
Assignees
Labels
enhancement New functionality or behavior implemented Change has been implemented
Milestone

Comments

@joshbruce
Copy link
Contributor

Might be related to #283 though I don't think it is.

Version(s) affected: 1.4.*

Description

And so we did.

<iframe width="100%" height="300" src="//jsfiddle.net/joshbruce/9286LaLx/embedded/" allowfullscreen="allowfullscreen" allowpaymentrequest frameborder="0"></iframe>

And this, I fear is how things start getting complicated to keep track of.

Config:

[
  "html_input" => "allow",
  "allow_unsafe_links" => false,
  "external_link" => [
    "open_in_new_window" => true
  ]
]

Output:

<p>And so we did.</p>

&lt;iframe width="100%" height="300" src="//jsfiddle.net/joshbruce/9286LaLx/embedded/" allowfullscreen="allowfullscreen" allowpaymentrequest frameborder="0">&lt;/iframe>

<p>And this, I fear is how things start getting complicated to keep track of.</p>

I have another set, similar to this one use <dl>, which works as expected. Therefore, it seems to be isolated to <iframe> near as I can tell. Almost doing a partial escape.

How to reproduce

Should only require place markdown containing an <iframe>. I have copied the code. above into two of my other markdown files including some with other HTML as well and get the same result.

Note: I'm not aware of an <iframe> specific configuration setting or requirement, apologies if I missed it somewhere.

@joshbruce
Copy link
Contributor Author

While looking for an abbreviations extension, I think I discovered the issue: https://commonmark.thephpleague.com/1.5/extensions/disallowed-raw-html/

Because I'm using GFM, this extension gets included automatically?

I'm wondering if there is a way to override this or turn it off and still use the GFM extension? I'm the only one creating content; so, the malicious person in this case would be me. :)

@joshbruce
Copy link
Contributor Author

Ah! I get it. Sorry - new to going this deep with Commonmark. Will explicitly set extensions related to GFM instead of the shortcut and just skip disallowed raw html...still might be nice to have a way to allow a specific tag to opt into the less secure posture.

@colinodell
Copy link
Member

I'm open to the idea of allowing the disallowed tag list to be configurable - it could accept an array of strings where each one if the name of a tag like iframe or script. That option would default to GFM's defaults.

@markhalliwell
Copy link
Contributor

@colinodell I agree. I've already run into the scenario where a user selects the GFM parser and then goes to set <iframe> in their custom allowed HTML, only to have it stripped by this extension. Because the GFM extension essentially "requires" this extension, the same is done in the UI, so users are left being a little perplexed as to what's going on.

I've been trying to figure out a way to visually represent that this extension explicitly disallows certain tags, but that would certainly be easier if it were configurable.

@joshbruce joshbruce changed the title iframe being escaped despite html_input set to allow Allow users to specify tags that can be used with disallowed raw html Jun 23, 2020
@joshbruce
Copy link
Contributor Author

I will go ahead and reopen with a change to the title to open it up for wider discussion - please feel free to close again at your discretion.

@joshbruce joshbruce reopened this Jun 23, 2020
@colinodell colinodell added this to the v2.0 milestone Jun 23, 2020
@colinodell colinodell added enhancement New functionality or behavior up-for-grabs Please feel free to take this one! labels Jun 23, 2020
@colinodell colinodell self-assigned this Jun 27, 2020
@colinodell colinodell removed the up-for-grabs Please feel free to take this one! label Jun 27, 2020
@close-label close-label bot added the implemented Change has been implemented label Jun 28, 2020
@colinodell
Copy link
Member

This has been implemented via #512 and will be included in the upcoming 2.0.0 release. Thanks for the idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New functionality or behavior implemented Change has been implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants