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

Remove attributes without a value. #51

Merged
merged 1 commit into from
Jul 21, 2013
Merged

Remove attributes without a value. #51

merged 1 commit into from
Jul 21, 2013

Conversation

kaspth
Copy link
Contributor

@kaspth kaspth commented Jul 3, 2013

Changes elements like <img src=""></img> to <img></img>.

//@rafaelfranca

@kaspth
Copy link
Contributor Author

kaspth commented Jul 3, 2013

What I'd really like is for Loofah to remove the attributes which have blank values.
For instance:

Loofah.fragment(%(<IMG SRC=" &#14;  javascript:alert('XSS');">))

Returns an element which have " " as the value for src. Which makes the current commit not remove the attribute.

@rafaelfranca
Copy link

Is not " ".empty? true?

@kaspth kaspth mentioned this pull request Jul 3, 2013
20 tasks
@kaspth
Copy link
Contributor Author

kaspth commented Jul 3, 2013

Nope.

irb(main):001:0> " ".empty?
=> false

@rafaelfranca
Copy link

Ah. Right. If we really need this we can reimplement blank? if we are not depending on active_support already

@kaspth
Copy link
Contributor Author

kaspth commented Jul 3, 2013

There's no ActiveSupport dependency at the moment.

The above pasted snippet is from this test:
https://github.com/kaspth/rails/blob/loofah-integration/actionview/test/template/sanitizers_test.rb#L156-L175

That's the reason it's failing.
(Also number 4 in the list is failing.)

@kaspth
Copy link
Contributor Author

kaspth commented Jul 4, 2013

@rafaelfranca, I changed it to reimplement blank? Hope that's okay.

@flavorjones
Copy link
Owner

Looks like this might introduce 1.8 support issues, since the ActiveSupport String#blank? implementation uses 0x3000 instead of :space: for 1.8.

I'm fine with it if we're leaving 1.8 behind, but then we should update the gemspec to explicitly only support 1.9.1 and above.

Thoughts?

@kaspth
Copy link
Contributor Author

kaspth commented Jul 18, 2013

Rails requires 1.9.3+, so I definitely think moving along the same lines is a good idea.

You want me to update this PR and change the required version in the gemspec? Or in another PR?

@flavorjones
Copy link
Owner

I'll take care of the gemspec -- I just wanted to point out the implication on Ruby version support.

flavorjones added a commit that referenced this pull request Jul 21, 2013
Remove attributes without a value.
@flavorjones flavorjones merged commit 789f82c into flavorjones:master Jul 21, 2013
@kaspth kaspth deleted the remove-empty-attr branch July 21, 2013 08:12
@kaspth
Copy link
Contributor Author

kaspth commented Jul 21, 2013

Sounds good.

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.

3 participants