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

fix(security): do not auto-bootstrap when loaded from an extension. #15346

Closed
wants to merge 2 commits into from
Closed

fix(security): do not auto-bootstrap when loaded from an extension. #15346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ca88480
fix(security): do not auto-bootstrap when loaded from an extension.
mprobst Oct 21, 2016
c2364d2
chore(): fix global console usage.
mprobst Nov 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions src/Angular.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1444,6 +1444,26 @@ function getNgAttribute(element, ngAttr) {
return null;
}

function allowAutoBootstrap(document) {
if (!document.currentScript) {
return true;
}
var src = document.currentScript.getAttribute('src');
var link = document.createElement('a');
link.href = src;
var scriptProtocol = link.protocol;
var docLoadProtocol = document.location.protocol;
if ((scriptProtocol === 'resource:' ||
scriptProtocol === 'chrome-extension:') &&
docLoadProtocol !== scriptProtocol) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So we do allow auto bootstrap if the protocol of the document matches that of the script?
Why is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because that's the intended use case - an extension that has a page (within the extension) that loads angular, and contains ng-app. We could disable that, but if we support auto bootstrap, I think we should support it within extensions, shouldn't we?

return false;
}
return true;
}

// Cached as it has to run during loading so that document.currentScript is available.
var isAutoBootstrapAllowed = allowAutoBootstrap(window.document);

/**
* @ngdoc directive
* @name ngApp
Expand Down Expand Up @@ -1602,6 +1622,11 @@ function angularInit(element, bootstrap) {
}
});
if (appElement) {
if (!isAutoBootstrapAllowed) {
Copy link
Member

Choose a reason for hiding this comment

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

This check could be moved at the top (unless we expect anyone to load angular via resource:/chrome-extension: in a non-extention page and bootstrap it manually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't do this intentionally - otherwise you'll get an error message even if you intend to manually bootstrap by calling angular.bootstrap. Which I guess is a legit use case, if somebody does inject Angular in the page but also controls it to call angular.bootstrap (not a security issue because they can already execute JS in the page in that case).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I thought it might be intentional. I don't expect anyone to be loading angular over resource: on non-extension pages, but you never know 😃

Hm...so, if it is a supported usecase (loading angular.js over resource: on non-extension pages), isn't this chage breaking? In that case, we should have a breaking change notice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Loading angular.js over resource: from a non-extension page is not supported, and not a use case (that's almost certainly a security exploit, which is why this change is blocking it). I don't think we need to document that.

However, for the rare case where a web page really actually wants to load via resource://, it still can, by embedding an appropriate angular.bootstrap call. If you can embed JS into the hosting page, you can do evil things anyway, so that's not a security boundary. This behaviour is easy to implement, doesn't do harm or lower security (it should, at least...), and overall seems fine.

A secondary effect is that the console spam only triggers if the user actually has an ng-app in their app, so it's more precise, which I think is a good thing.

So I don't think we need to document anything. Most users will never notice this.

Copy link

Choose a reason for hiding this comment

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

So I don't think we need to document anything. Most users will never notice this.

This should be documented, assuming that most users won't notice it means that when someone does notice it, there is no help available on how to go about making it work.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mmccallum - feel free to send in a PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmccallum FWIW, when auto bootstrap gets disabled, Angular prints out a pretty clear message in the console.

window.console.error('Angular: disabling automatic bootstrap. <script> protocol indicates ' +
'an extension, document.location.href does not match.');
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel that this message could be clearer or that it should link to a webpage/issue that explains what is happening?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a suggestion? This should really only happen when somebody tries to exploit the security hole, so I'm not too worried about usability here. I don't think we need docs or a website.

Copy link
Contributor

Choose a reason for hiding this comment

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

But it shouldn't block merging this.

return;
}
config.strictDi = getNgAttribute(appElement, 'strict-di') !== null;
bootstrap(appElement, module ? [module] : [], config);
}
Expand Down
2 changes: 2 additions & 0 deletions test/.eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,8 @@
"getBlockNodes": false,
"createMap": false,
"VALIDITY_STATE_PROPERTY": true,
"allowAutoBootstrap": false,
"isAutoBootstrapAllowed": false,

/* AngularPublic.js */
"version": false,
Expand Down
21 changes: 21 additions & 0 deletions test/AngularSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1682,6 +1682,27 @@ describe('angular', function() {

dealoc(appElement);
});

it('should not bootstrap from an extension into a non-extension document', function() {
var src = 'resource://something';
// Fake a minimal document object (the actual document.currentScript is readonly).
var fakeDoc = {
currentScript: { getAttribute: function() { return src; } },
location: {protocol: 'http:'},
createElement: document.createElement.bind(document)
};
expect(allowAutoBootstrap(fakeDoc)).toBe(false);

src = 'file://whatever';
expect(allowAutoBootstrap(fakeDoc)).toBe(true);
});

it('should not bootstrap if bootstrapping is disabled', function() {
isAutoBootstrapAllowed = false;
angularInit(jqLite('<div ng-app></div>')[0], bootstrapSpy);
expect(bootstrapSpy).not.toHaveBeenCalled();
isAutoBootstrapAllowed = true;
});
});


Expand Down