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.

Sign up for GitHub

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

XSS escaping completely removes src/href attributes on images and links #66

Closed
ramboz opened this issue May 31, 2019 · 5 comments · Fixed by #67
Closed

XSS escaping completely removes src/href attributes on images and links #66

ramboz opened this issue May 31, 2019 · 5 comments · Fixed by #67
Assignees
@ramboz
Labels
released

Comments

@ramboz
Copy link
Contributor

ramboz commented May 31, 2019

With #53 the XSS escaping introduced ends up being a bit too zealous and removes all:

  • src attributes on images
  • href attributes on links
@ramboz ramboz self-assigned this May 31, 2019
ramboz added a commit that referenced this issue May 31, 2019
@ramboz
fix(xss): Allow src/href attributes on images and links
06c677e 
Relax the XSS sanitization to allow src attributes on images and href attributes on links

fix #66
This was referenced May 31, 2019
Merged
Closed
ramboz added a commit that referenced this issue May 31, 2019
@ramboz
test(xss): Add test to improve coverage
0d4f4a0 
Adding a test for an anchor without a link, but with a title to improve branch code coverage

fix #66
@tripodsan
Copy link
Contributor

  • hmm, what's the behavior in sling when you output <a href="www.github.com"> in the HTML context?
  • what's the performance impact?

@ramboz
Copy link
Contributor Author

ramboz commented Jun 1, 2019 •
edited
Loading

  • ${'<a href="www.google.com">Foo</a>' @ context='unsafe'} or html both output only Foo as far as I can tell, which surprises me actually.
  • impact should be small as we only apply a minimal overhead of img/a tags on the existing sanitization logic. the tags are parsed anyway, we only treat their links differently

So if we follow the sling behavior, we actually can't even output html from the markdown inside the template it would seem…

@tripodsan
Copy link
Contributor

So if we follow the sling behavior, we actually can't even output html from the markdown inside the template it would seem…

that would be weird, if unsafe doesn't work as expected in sling...

In any case, once we add the DOM support (#62), we the XSS escaping become less important.

@ramboz
Copy link
Contributor Author

ramboz commented Jun 3, 2019

that would be weird, if unsafe doesn't work as expected in sling...

Maybe our expectations are wrong, I'm not sure, but my tests clearly didn't let me inject any complex HTML via a variable.

tripodsan pushed a commit that referenced this issue Jun 5, 2019
@ramboz @tripodsan
fix(xss): Allow src/href attributes on images and links
f99d0a5 
Relax the XSS sanitization to allow src attributes on images and href attributes on links

fix #66
tripodsan pushed a commit that referenced this issue Jun 5, 2019
@ramboz @tripodsan
test(xss): Add test to improve coverage
cf2c8c3 
Adding a test for an anchor without a link, but with a title to improve branch code coverage

fix #66
adobe-bot pushed a commit that referenced this issue Jun 5, 2019
@semantic-release-bot
chore(release): 2.3.2 [skip ci]
5a140d3 
## [2.3.2](v2.3.1...v2.3.2) (2019-06-05)

### Bug Fixes

* **xss:** Allow src/href attributes on images and links ([f99d0a5](f99d0a5)), closes [#66](#66)
@adobe-bot
Copy link

🎉 This issue has been resolved in version 2.3.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ramboz ramboz

Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix(xss): Allow src/href attributes on images and links adobe/htlengine
3 participants
@tripodsan @ramboz @adobe-bot