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

fix($compile) : Add base#href to the list of RESOURCE_URL context attrs #15597

Closed
wants to merge 1 commit into from

Conversation

rjamet
Copy link
Contributor

@rjamet rjamet commented Jan 11, 2017

This will put the same security constraints to bindings on base#ref as on iframes or script srcs, since it changes the behavior of relative URLs across the page. Currently, they don't have a $sce context. Also, it's more generally a good idea in Angular itself, as since #15144 and its fix #15145 we'll consider that baseURI as a trusted origin.

Does this PR introduce a breaking change?
Yup: something like <base href="{{myUrl}}"/> will send myUrl to the $sce's RESOURCE_URL checks. By default, it will break if myUrl isn't same-origin. Also, concatenation in trusted contexts is disabled: "/{{myPathComponent}}/something" won't work at all.

This follows the discussion in #15537 and the initial issue in #15144, fixed in #15145.

…ributes.

This will put the same security constraints to bindings on base#ref as
on iframes or script srcs, since it changes the behavior of relative
URLs across the page.
Copy link
Member

@gkalpak gkalpak left a comment

Choose a reason for hiding this comment

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

LGTM, but the commit message needs a proper breaking change notice.

gkalpak added a commit that referenced this pull request Jan 12, 2017
This reverts commit 5e28b6e.
Reverting while investigating security implications of 5e28b6e without #15597
(which is possibly a breaking change, thus not suitable for this branch).
gkalpak added a commit that referenced this pull request Jan 12, 2017
This reverts commit cce98ff.
Reverting while investigating security implications of cce98ff without #15597
(which is possibly a breaking change, thus not suitable for this branch).
@gkalpak
Copy link
Member

gkalpak commented Jan 12, 2017

@rjamet, what is your take on #15145 (comment)? Is it OK to release #15145 without this change, or should they rather go together (or not at all)?

@petebacondarwin
Copy link
Contributor

For anyone wondering this question was answered in #15145 (comment)

both PRs at the same time would be the safest.

Copy link
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

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

LGTM

@gkalpak gkalpak closed this in 1cf728e Jan 18, 2017
ellimist pushed a commit to ellimist/angular.js that referenced this pull request Mar 15, 2017
…attributes

Previously, `base[href]` didn't have an SCE context. This was not ideal, because
`base[href]` affects the behavior of all relative URLs across the page.
Furthermore, since angular#15145, `base[href]` also affects which URLs are considered
trusted under the 'self' policy for white- or black-listed resource URLs.

This commit tightens the security of Angular apps, by adding `base[href]` to the
list of RESOURCE_URL context attributes, essentially putting the same
constraints on bindings to `base[href]` as on iframe or script sources.

Refer to the
[`$sce` API docs](https://code.angularjs.org/snapshot/docs/api/ng/service/$sce)
for more info on SCE trusted contexts.

Closes angular#15597

BREAKING CHANGE:

Previously, `<base href="{{ $ctrl.baseUrl }}" />` would not require `baseUrl` to
be trusted as a RESOURCE_URL. Now, `baseUrl` will be sent to `$sce`'s
RESOURCE_URL checks. By default, it will break unless `baseUrl` is of the same
origin as the application document.

Refer to the
[`$sce` API docs](https://code.angularjs.org/snapshot/docs/api/ng/service/$sce)
for more info on how to trust a value in a RESOURCE_URL context.

Also, concatenation in trusted contexts is not allowed, which means that the
following won't work: `<base href="https://github.com/something/{{ $ctrl.partialPath }}" />`.

Either construct complex values in a controller (recommended):

```js
this.baseUrl = '/something/' + this.partialPath;
```
```html
<base href="{{ $ctrl.baseUrl }}" />
```

Or use string concatenation in the interpolation expression (not recommended
except for the simplest of cases):

```html
<base href="{{ '/something/' + $ctrl.partialPath }}" />
```
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.

4 participants