Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix($parse): function call context should be safe #4417

Closed
wants to merge 1 commit into from

Conversation

chirayuk
Copy link
Contributor

No description provided.

@mary-poppins
Copy link

Oh Chirayu!  Nice to see you sending PRs again!  :)

@@ -754,6 +754,7 @@ Parser.prototype = {
}
var fnPtr = fn(scope, locals, context) || noop;

ensureSafeObject(context, parser.text);
ensureSafeObject(fnPtr, parser.text);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to check the fnPtr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, to check for the Function constructor.

@vojtajina
Copy link
Contributor

LGTM.

I would change the commit msg (something like fix($parse): check function call context to be safe) - imagine, it's gonna be in changelog...

@chirayuk
Copy link
Contributor Author

@IgorMinar – I think it's a good idea to check the return value.  If someone can get a window/document object returned from a function – even if they can't use it directly in a function call, they could pass it to another function that invokes someone on it, etc. While we could remove it, I prefer keeping it considering the overhead of the function call path.

@IgorMinar
Copy link
Contributor

lgtm

@IgorMinar IgorMinar closed this Oct 15, 2013
@IgorMinar IgorMinar reopened this Oct 15, 2013
@buunguyen
Copy link
Contributor

@chirayuk
Shouldn't the following comment be updated per this commit? (Emphases are mine. These are the parts that seem not to be correct anymore.)

This sandboxing technique is not perfect and doesn't aim to be. The goal is to prevent exploits against the expression language, but not to prevent exploits that were enabled by exposing sensitive JavaScript or browser apis on Scope. Exposing such objects on a Scope is never a good practice and therefore we are not even trying to protect against interaction with an object explicitly exposed in this way.

@chirayuk chirayuk closed this in 6d324c7 Oct 15, 2013
chirayuk added a commit to chirayuk/angular.js that referenced this pull request Oct 15, 2013
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this pull request Jan 27, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants