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

improve sanitization of img tags #1244

Closed
wants to merge 1 commit into from
Closed

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Jul 24, 2017

If you want me to split these into two PRs, I can, but I figured that since they're both small, and related, I would put them together.

- allow width, height, alt, title attributes in img (fixes
  element-hq/element-web#4646)
- replace disallowed URL schemes with the alt or title attribute (fixes
  element-hq/element-web#4044)

Signed-off-by: Hubert Chathi <hubert@uhoreg.ca>
@matrixbot
Copy link
Member

Can one of the admins verify this patch?

@lukebarnard1
Copy link
Contributor

Your first bullet looks like something we should just do, so LGTM on that bit. The second part I'm not sure about because replacing the <img> with a <span> feels like it would be quite confusing, albeit less confusing than not showing any image at all. I'm wondering if we should instead insert a placeholder that indicates the error that's happened or whether we just insert a link that points to the src. @lampholder any UX-y thoughts?

@lampholder
Copy link
Member

I'd say lets take advantage of the first bullet and kick the second point down the road until a time when we can think about what this would look like :P

@uhoreg
Copy link
Member Author

uhoreg commented Jul 25, 2017

OK, I'll close this PR for now, and create a new PR for just the first point.

@uhoreg uhoreg closed this Jul 25, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants