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 expression sandbox #15094

Closed

Conversation

@petebacondarwin
Copy link
Member

petebacondarwin commented Sep 5, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

refactor

What is the current behavior? (You can also link to an open issue here)

The angular expression parsers attempts to sandbox the expressions to prevent unrestricted access to the global context.

What is the new behavior (if this is a feature change)?

The sandbox is removed, as it was not a real security feature.

Does this PR introduce a breaking change?

No

Please check if the PR fulfills these requirements

Other information:

TODO:

  • fill out the commit message
  • more refactoring to remove unnecessary vars
  • look into further performance improvements
@mgol
Copy link
Member

mgol commented Sep 5, 2016

Once it has a proper commit message and a couple of unused vars at the top of the file are removed (I only found some cached methods, that's all) I think it's ready to land. Performance improvements can be addressed in subsequent PRs/commits, I like that this one mostly cleanly removes stuff.

Other than that, LGTM.

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:remove-sandbox branch from fdc3de8 to 794b770 Sep 6, 2016
@gkalpak
Copy link
Member

gkalpak commented Sep 6, 2016

The changes LGTM. Wrt the commit message:

  • According to this well-respected resource, the type should be refactor 😁
  • Update with URL to blog post. (Just putting it here as a reminder.)
@petebacondarwin
Copy link
Member Author

petebacondarwin commented Sep 7, 2016

@gkalpak - I always get that wrong :-(

the type should be refactor

The angular expression parser (`$parse`) attempts to sandbox expressions
to prevent unrestricted access to the global context.

While the sandbox was not on the frontline of the security defense,
developers kept relying upon it as a security feature even though it was
always possible to access arbitrary JavaScript code if a malicious user
could control the content of Angular templates in applications.

This commit removes this sandbox, which has the following benefits:

* it sends a clear message to developers that they should not rely on
the sandbox to prevent XSS attacks; that they must prevent control of
expression and templates instead.
* it allows performance and size improvements in the core Angular 1
library.
* it simplifies maintenance and provides opportunities to make the
parser more capable.

Please see the [Sandbox Removal Blog Post](http://angularjs.blogspot.com/2016/09/angular-16-expression-sandbox-removal.html)
for more detail on what you should do to ensure that your application is
secure.
@petebacondarwin petebacondarwin force-pushed the petebacondarwin:remove-sandbox branch from 794b770 to 10597de Sep 7, 2016
@petebacondarwin petebacondarwin modified the milestones: 1.6.x, 1.6.0 Sep 12, 2016
gkalpak added a commit to gkalpak/angular.js that referenced this pull request Feb 5, 2017
gkalpak added a commit that referenced this pull request Feb 5, 2017
…rse()`

This is a follow-up to #15094.

Closes #15680
gkalpak added a commit that referenced this pull request Feb 5, 2017
…rse()`

This is a follow-up to #15094.

Closes #15680
ellimist added a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.