-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix #1748 where allowed prototype methods are not called #1958
base: 4.x
Are you sure you want to change the base?
Fix #1748 where allowed prototype methods are not called #1958
Conversation
Note: failing test was not caused by this PR and was fixed in #1963. Needs rebase. |
fa0420e
to
b1d1aa8
Compare
@jaylinski rebased, but I've no idea why this PR now picks up changes in If you want me to recreate the PR let me know. |
b8f7a6d
to
5cae846
Compare
No problem, I fixed it. |
@@ -136,6 +136,9 @@ export function template(templateSpec, env) { | |||
} | |||
|
|||
if (resultIsAllowed(result, container.protoAccessControl, propertyName)) { | |||
if (typeof result === 'function') { | |||
return parent[propertyName](); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you use parent[propertyName]()
instead of result()
? Any specific reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't work with result()
, complains that:
TypeError: String.prototype.trim called on null or undefined
I'm not 100% sure why, but as I understand result
loses this
context.
This fix isn't right - handlebars.js/lib/handlebars/runtime.js Lines 146 to 148 in f422bfd
They are called with the Handlebars context, not with the object preceding the dot as in regular JavaScript. Changing this would be a breaking change, and it's not clear for what benefit. You can create a |
I'm still curious why the documentation states that it should work. 🤔 |
That was added somewhat recently to the documentation with the security fixes. I think it was just a mistake. |
I don't know why I added this example to the docs and did not notice that it does not work. The Handlebars way to solve the issue is to register a "trim" helper and use "{{trim aString}}" instead. |
I'll keep this issue open until we corrected the wrong documentation and close it afterwards. |
Fixes #1748 where allowed prototype methods are not called.
Without this fix the official documentation is incorrect:
The result is
[object Object]
, notabc
.