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

Extend sanitize #171

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Extend sanitize #171

wants to merge 3 commits into from

Conversation

kascote
Copy link

@kascote kascote commented Apr 14, 2022

This PR extends sanitize_html to allow custom tags and attributes.

  • Two new callbacks handle the checks
  • The internal functions were normalized using the same parameters as the callbacks.
  • Test added to check for the new functionality
  • There are no changes to how the library worked before and do not require code changes.

An upcoming PR will handle the remove_contents option as observed on #101

Copy link
Member

@jonasfj jonasfj left a comment

Choose a reason for hiding this comment

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

I'm include to think this is a bad idea. I definitely need use-cases.

Yes, it's nice to have more features. But if we add more features we also create options for people to make mistakes.

In a sense I'm very tempted to keep this library as simple and fool proof as possible. Personally, I've found that it's very easy to be a fool and think that allowing a certain tag or attribute through can't possibly be dangerous 🙈

This library is heavily influenced by the choices github makes when santizing rendered markdown. What is the reasonable use case for allowing through more tags and attributes?

And would it perhaps be better to put such logic in a separate package that focuses on being more flexible and less fool proof. It's not like this package has a lot of logic, it just uses package:html.

To a very large extend this package expresses an opinion about how HTML can be sanitized, it aims at safety and simplicity over flexibility.

@@ -200,6 +200,47 @@ final _elementAttributeValidators =
'Q': _citeAttributeValidator,
};

/// Callback function used to check wich tags are allowed
/// The function will return true if the tag is allowed of false if not.
typedef AllowTagCB = bool Function(String tag);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't need a typedef... also why give a callback... why not just make it a list of string?

/// and can be [unchanged], [edit] or [remove].
/// In case of [edit], the [value] field can be used to
/// change the content of the attribute.
class AllowAttributeResponse {
Copy link
Member

Choose a reason for hiding this comment

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

What examples are there of attributes where this makes sense?

@jonasfj
Copy link
Member

jonasfj commented Apr 29, 2022

lol, I just saw the old comment in #101 (comment) 🤣

hmm, ...maybe, I'm should be more open to signatures like:

  • bool allowTag(String tag), and,
  • bool allowAttribute(String tag, String attribute, String value)

Context: I use this library for sanitizing markdown on pub.dev, and I want that to be consistent with how it renders on github.
But yeah, I can also see the value with allowing a few tweaks.. we did that for adding ugc attributes to links.

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

Successfully merging this pull request may close these issues.

None yet

2 participants