Skip to content
This repository has been archived by the owner on Jul 15, 2019. It is now read-only.

improvement over PR/14 #16

Closed
wants to merge 2 commits into from

Conversation

adon-at-work
Copy link
Contributor

This is to improve #14:

  • a user customizing whitelist should be able to really stop style attributes
  • fix a vulnerable test case
  • converted attribute list to lower case, otherwise, purifier can be bypassed
  • simplified the config logics, it's reasonable to skip copying things around in constructor
  • hrefAttributes should be a hardcoded list (ultimately should rely on a single source of truth and a tag-specific list) to judge whether url filter is needed

cleanup based on review comments, add missing href attributes

bump up version to v1.1.0
@adon-at-work
Copy link
Contributor Author

@maditya , @neraliu , please review.
note: this PR is designed to be first absorbed by taglist-configurable, which will ultimately be (rebased and) merged into master.

@yahoocla
Copy link

CLA is valid!

"autoplay",
"cellpadding",
"cellspacing",
// "charset",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I commented out autofocus and charset temporarily in order to pass their corresponding test cases. They were bypassed by the test cases before this list is lowercased.

Let's discuss the default whitelist tags and attributes. I imagine we'd have a much simpler list.
The current setting is vulnerable. For instance, it enables <iframe srcdoc="">, which is actually scriptable. Allowing <iframe src="http://attacker.com"> is not ideal too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Allowing <iframe src="http://attacker.com"> is not ideal too.

We can handle this better by allowing user to specify a whitelist of domains. For eg: we should allow <iframe src="http://mydomain.com"> but filter the value for <iframe src="http://attacker.com">

@maditya
Copy link
Contributor

maditya commented Jul 13, 2015

Merging all these changes via cli into #14 and fixing pending items. Closing the PR

maditya added a commit that referenced this pull request Jul 13, 2015
cleanup based on review comments, add missing href attributes

bump up version to v1.1.0

- fix test cases for autofocus and charset as pointed out in
  #16
@maditya maditya closed this Jul 13, 2015
@adon-at-work adon-at-work deleted the whitelist-configurable branch July 14, 2015 05:35
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants