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

Unescaped attributes #664

Merged
merged 5 commits into from Jan 23, 2014

Conversation

Projects
None yet
3 participants
@zkat
Contributor

zkat commented Jan 14, 2014

This fixes an issue where, say:

can.view.mustache("<foo title='{{hi}}'")({hi: "x&y"})

would render things such that element.title === "x&amp;y"

This makes it so that {{{x}}} and {{x}}, when applied to attributes, always do the exact same thing. It does not make sense for us to allow unescaped attribute values in can.view

EDIT: I found out after looking closer that this was only affecting things inside {{#foo}} blocks. This does fix that issue, and things continue to work as before. I've added a new test that specifically checks escaping inside blocks within attributes.

// If not, add `doubleParent` to close push and text.
"}));\n"
);
buff.push(insert_cmd, "can.view.txt(\n" +

This comment has been minimized.

@justinbmeyer

justinbmeyer Jan 16, 2014

Contributor

@sykopomp can you identify the change in this section?

@justinbmeyer

justinbmeyer Jan 16, 2014

Contributor

@sykopomp can you identify the change in this section?

This comment has been minimized.

@zkat

zkat Jan 22, 2014

Contributor

All attributes are escaped here. status() is a string when the value is inside an attribute.

@zkat

zkat Jan 22, 2014

Contributor

All attributes are escaped here. status() is a string when the value is inside an attribute.

@@ -200,7 +200,9 @@ can.extend(can.view, {
// If we had no observes just return the value returned by func.
if(!setupLiveBinding || typeof value === "function"){
unbind && unbind();
return ( (escape || typeof status === 'string') && escape !== 2 ? contentEscape : contentText)(value, status === 0 && tag);

This comment has been minimized.

@justinbmeyer

justinbmeyer Jan 16, 2014

Contributor

@sykopomp here too ... what changed?

@justinbmeyer

justinbmeyer Jan 16, 2014

Contributor

@sykopomp here too ... what changed?

This comment has been minimized.

@zkat

zkat Jan 22, 2014

Contributor

This works with the other change so that we don't bother checking for attribute status (except for special attributes, which are special)

@zkat

zkat Jan 22, 2014

Contributor

This works with the other change so that we don't bother checking for attribute status (except for special attributes, which are special)

Josh Marchán added some commits Jan 14, 2014

Josh Marchán
Attribute values should only be escaped when concatenated literally
This takes care of a bug where attribute values would sometimes have
html entities in them when the content was escaped. For example, & in
an attribute value string would show up as &amp; in the actual attribute
value.

The only case where we actually want to escape attributes in can.view
is when they're immediately concatenated because they're simple strings.
@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Jan 22, 2014

Contributor

The current code passes the modified tests in the pull request without any changes.

The escaping really only applies to HTML tag escaping (it doesn't do ampersands). I'd disagree that attributes should always be ampersand escaped, I'd expect that your original content should be "x&y" in that case.

Contributor

andykant commented Jan 22, 2014

The current code passes the modified tests in the pull request without any changes.

The escaping really only applies to HTML tag escaping (it doesn't do ampersands). I'd disagree that attributes should always be ampersand escaped, I'd expect that your original content should be "x&y" in that case.

Josh Marchán and others added some commits Jan 22, 2014

@andykant

This comment has been minimized.

Show comment
Hide comment
@andykant

andykant Jan 23, 2014

Contributor

This is ready to merge pending some external changes involving test running.

Contributor

andykant commented Jan 23, 2014

This is ready to merge pending some external changes involving test running.

@ghost ghost assigned andykant Jan 23, 2014

andykant added a commit that referenced this pull request Jan 23, 2014

@andykant andykant merged commit 12cbd6f into master Jan 23, 2014

1 check passed

default The Travis CI build passed
Details

@andykant andykant deleted the unescaped-attributes branch Jan 23, 2014

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Jan 23, 2014

Contributor

are there changes here or is this just formatting?

Contributor

justinbmeyer commented on view/scanner.js in 7b44cab Jan 23, 2014

are there changes here or is this just formatting?

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