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

Form helper doesn't escape value attribute for values entities #96

Merged
merged 2 commits into from
Jan 26, 2017
Merged

Form helper doesn't escape value attribute for values entities #96

merged 2 commits into from
Jan 26, 2017

Conversation

davydovanton
Copy link
Member

@davydovanton davydovanton commented Jan 13, 2017 •
edited by jodosha
Loading

@davydovanton davydovanton self-assigned this Jan 13, 2017
@davydovanton davydovanton force-pushed the escape-input-value branch 3 times, most recently from ac2e72b to c5461e9 Compare January 13, 2017 00:40
@davydovanton davydovanton force-pushed the escape-input-value branch 3 times, most recently from 6d92692 to 591e9ad Compare January 13, 2017 01:04
@davydovanton davydovanton changed the title [wip] Form helper doesn't escape value attribute for values entities Form helper doesn't escape value attribute for values entities Jan 13, 2017
@davydovanton davydovanton added this to the v1.0.0.beta1 milestone Jan 13, 2017
AlfonsoUceda
AlfonsoUceda reviewed Jan 13, 2017
View reviewed changes
lib/hanami/helpers/html_helper/empty_html_node.rb
if attribute_name.to_sym == :value
%(#{ATTRIBUTES_SEPARATOR}#{attribute_name}="#{Hanami::Utils::Escape.html(value)}")
else
%(#{ATTRIBUTES_SEPARATOR}#{attribute_name}="#{value}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we escape the value in all cases?

Copy link
Member

Choose a reason for hiding this comment

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

Probably yes.

Please also consider to use html_attribute instead https://github.com/hanami/utils/blob/ca91c336ffebb43f1632ffd398b4281b3b7315a5/lib/hanami/utils/escape.rb#L453

Copy link
Member Author

Choose a reason for hiding this comment

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

We have some attributes like a script. If we escape this attributes we will have a error in layout. What do you think what we should do with this attributes?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we simply make a list of the attributes that need escaping? (like value, title, ...)

Copy link
Member

Choose a reason for hiding this comment

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

@TiteiKo We can't predict where developers are going to interpolate user's input.

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively we can make this change in _value:

helpers/lib/hanami/helpers/form_helper/form_builder.rb

Line 984 in 8f10dbd

def _value(name)

WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

@jodosha good idea 👍

@davydovanton davydovanton force-pushed the escape-input-value branch 2 times, most recently from c61c99a to f6ed96f Compare January 18, 2017 00:42
@davydovanton
Copy link
Member Author

Okay, I updated #_attributes form helper because #_value sets default value and we need to escape all values. Also I used #html_attribute method instead #html.

@jodosha
Copy link
Member

jodosha commented Jan 18, 2017

@davydovanton I don't understand the difference. Why it can't be in _value?

@davydovanton
Copy link
Member Author

I fixed CI build.

_value used only for the default value. If you have a value attribute in your form, you will redefine this value. That's why I escape value after the merge.

@davydovanton
Copy link
Member Author

Also, I checked this in my local app, and all worked correctly 🎉

@jodosha jodosha added fix and removed bug labels Jan 26, 2017
jodosha
jodosha approved these changes Jan 26, 2017
View reviewed changes
Copy link
Member

@jodosha jodosha left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@davydovanton
Copy link
Member Author

@jodosha thanks for help and review 👍

@davydovanton davydovanton merged commit a328552 into hanami:master Jan 26, 2017
@davydovanton davydovanton deleted the escape-input-value branch January 26, 2017 17:19
@jodosha jodosha mentioned this pull request Jan 26, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TiteiKo TiteiKo TiteiKo left review comments

@AlfonsoUceda AlfonsoUceda AlfonsoUceda left review comments

@jodosha jodosha jodosha approved these changes

Assignees

@davydovanton davydovanton

Labels
fix
Projects
None yet
Milestone
v1.0.0.beta1
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@davydovanton @jodosha @TiteiKo @AlfonsoUceda