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

Only allow cite attribute on blockquote and restrict schemes #223

Merged
merged 2 commits into from
Sep 28, 2015
Merged

Only allow cite attribute on blockquote and restrict schemes #223

merged 2 commits into from
Sep 28, 2015

Conversation

btoews
Copy link

@btoews btoews commented Sep 26, 2015

This limits the schemes that can be used with the cite attribute on blockquote elements.

/cc @jch @ytrezq @oreoshake @ptoomey3 @gregose

@ytrezq
Copy link

ytrezq commented Sep 26, 2015

😎I hope this change will also takes place on GitHub’s owned web sites !

@mastahyeti : Did you opened it because you checked September 26 e‑mails or because I updated the comment of the previous ᴘʀ. (it seems you replied to it only partially)

Also why restrict it only to blockquote ? Why not doing it the same way as sanitize.rb inside it’s basic configuration ?

@btoews
Copy link
Author

btoews commented Sep 28, 2015

I hope this change will also takes place on GitHub’s owned web sites !

Yeah, we've run into some trouble updating the Gem on our servers because of some incompatibilities with internal libraries. We're working on fixing this.

Also why restrict it only to blockquote ?

From my searches, the cite attribute is only allowed on blockquote tags.

Why not doing it the same way as sanitize.rb inside it’s basic configuration ?

Because GitHub doesn't make use of the whitelist in that Gem. I'm sure rgrove would appreciate a pull request though.

@ytrezq
Copy link

ytrezq commented Sep 28, 2015

@mastahyeti : As per current ᴡ3ᴄ spec for ʜᴛᴍʟ 5.1. It’s a valid attribute on “blockquote; del; ins; q”.
This is the same since ʜᴛᴍʟ4.

There’s a pending ᴘʀ on your repository for reflecting the spec on this ᴘʀ.

@btoews
Copy link
Author

btoews commented Sep 28, 2015

Thanks for finding that @ytrezq. Apparently, I didn't Google hard enough 😄. I updated this PR.

@ytrezq
Copy link

ytrezq commented Sep 28, 2015

:boulet: @mastahyeti : I told “There’s a pending ᴘʀ on your repository for reflecting the spec on this ᴘʀ.” in my previous comment

But apparently you’re prefering doing the work twice. 😄

Given this can lead to true xss for blind peoples using screen readers, I would have fixed this internally first.

@jch
Copy link
Contributor

jch commented Sep 28, 2015

Diff is easier to read when turning off whitespace: https://github.com/jch/html-pipeline/pull/223/files?w=1

Looks good to me 👍

jch added a commit that referenced this pull request Sep 28, 2015
Only allow cite attribute on blockquote and restrict schemes
@jch jch merged commit 0fe5776 into gjtorikian:master Sep 28, 2015
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.

4 participants