Closed
Bug 723808
Opened 13 years ago
Closed 13 years ago
disallow inheriting of system principal in type=content docshells
Categories
(Core :: DOM: Navigation, defect)
Core
DOM: Navigation
Tracking
()
RESOLVED
FIXED
mozilla13
People
(Reporter: Gavin, Assigned: Gavin)
References
Details
(Whiteboard: [sg:want P1][qa-])
Attachments
(4 files, 1 obsolete file)
(deleted),
patch
|
bzbarsky
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lsblakk
:
approval-mozilla-esr10+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
lsblakk
:
approval1.9.2.28+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
We've had a number of bugs over the years that were the result of somehow managing to load principal-inheriting URIs (data: and javascript:) on chrome-privileged pages loaded in the browser content area (e.g. about:config). Now that we have more and more chrome-privileged "in-content" pages, that's even more likely to be possible. As a precautionary measure, it makes sense to disallow these loads from inheriting the system principal - it seems quite unlikely that there are any valid use cases for this.
Assignee | ||
Comment 1•13 years ago
|
||
Assignee | ||
Comment 2•13 years ago
|
||
oops, uploaded the wrong patch. Ran into a couple of tests that depend on this, but none for good reason (and this is unlikely to occur in practice outside of the test harness).
Attachment #594059 -
Attachment is obsolete: true
Attachment #594059 -
Flags: review?(bzbarsky)
Attachment #594061 -
Flags: review?(bzbarsky)
![]() |
||
Comment 3•13 years ago
|
||
Comment on attachment 594061 [details] [diff] [review] real patch The setTimeout in that last test needs to happen from onload, not sync from script execution. r=me with that fixed.
Attachment #594061 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 4•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fc8e624ab13
Flags: in-testsuite+
Target Milestone: --- → mozilla13
Comment 5•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1fc8e624ab13
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 7•13 years ago
|
||
Comment on attachment 594061 [details] [diff] [review] real patch [Approval Request Comment] This patch: - has been on m-c for 10 days, with no reported regressions - is moderately risky: it blocks certain types of loads that are quite unlikely to be user-triggered, but could potentially affect extensions doing weird things.
Attachment #594061 -
Flags: approval-mozilla-beta?
Attachment #594061 -
Flags: approval-mozilla-aurora?
Comment 8•13 years ago
|
||
Comment on attachment 594061 [details] [diff] [review] real patch [Triage Comment] Based upon the moderate risk evaluation due to possible add-on regressions caused by this patch, we'd prefer this rides the trains. Although any regressions sound like they'd be related to add-on misbehavior, we know that add-on blocklisting post-release is a pain. Let's try to avoid that.
Attachment #594061 -
Flags: approval-mozilla-beta?
Attachment #594061 -
Flags: approval-mozilla-beta-
Attachment #594061 -
Flags: approval-mozilla-aurora?
Attachment #594061 -
Flags: approval-mozilla-aurora-
Comment 9•13 years ago
|
||
Comment on attachment 594061 [details] [diff] [review] real patch [Triage Comment] Upon further review and discussions with Gavin, this is only risky in terms of scope. We feel that the testing received in Firefox 11 beta 3 and up (along with Aurora 12 and m-c 13) will uncover regressions if caused by this bug. Approved.
Attachment #594061 -
Flags: approval-mozilla-beta-
Attachment #594061 -
Flags: approval-mozilla-beta+
Attachment #594061 -
Flags: approval-mozilla-aurora-
Attachment #594061 -
Flags: approval-mozilla-aurora+
Assignee | ||
Comment 10•13 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/56eb942d7c13 https://hg.mozilla.org/releases/mozilla-aurora/rev/07b25bee8bd1
status-firefox11:
--- → fixed
status-firefox12:
--- → fixed
Comment 11•13 years ago
|
||
we'll want to include this on the ESR for consistency with 10. Potentially 1.9.2 has the same problem, we'll have to think about whether it's worth taking at this point in the lifecycle.
blocking1.9.2: --- → ?
status1.9.2:
--- → wanted
status-firefox-esr10:
--- → affected
status-firefox13:
--- → fixed
tracking-firefox-esr10:
--- → 11+
Comment 12•13 years ago
|
||
Is there something QA can do to help verify this bug?
Whiteboard: [sg:want P1] → [sg:want P1][qa?]
Assignee | ||
Comment 13•13 years ago
|
||
verifiying bug 719994 is sufficient
Whiteboard: [sg:want P1][qa?] → [sg:want P1][qa-]
Updated•13 years ago
|
blocking1.9.2: ? → .28+
Assignee | ||
Updated•13 years ago
|
Attachment #594061 -
Flags: approval-mozilla-esr10?
Updated•13 years ago
|
Attachment #594061 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Updated•13 years ago
|
Attachment #601357 -
Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
Assignee | ||
Updated•13 years ago
|
Attachment #594061 -
Flags: approval-mozilla-esr10+
Assignee | ||
Comment 16•13 years ago
|
||
Followup bustage fix for ESR10: https://hg.mozilla.org/releases/mozilla-esr10/rev/bc4a7e406d63
Assignee | ||
Comment 17•13 years ago
|
||
Changes from the trunk version: - browser_463205.js doesn't exist on 1.9.2 (no need to fix it) - need to use PRBool instead of bool - chrome test harness on 1.9.2 requires use of packed.js, and explicit calls to enablePrivilege - I removed one of the tests that I added that wasn't strictly related to this change, since I couldn't figure out how to get it to work on 1.9.2 (it passed, but it wasn't effective at catching a bug I introduced in the code). The rest of the tests work fine locally. I wish I could push this to try, but try is missing ~3000 changesets from 1.9.2, and I don't think its build configuration would work with 1.9.2 builds.
Attachment #602502 -
Flags: approval1.9.2.28?
Comment 18•13 years ago
|
||
Comment on attachment 602502 [details] [diff] [review] 1.9.2 patch Let's get this landed and see how the builds & tests turn out. Sorry there's no try for 1.9.2 but soon enough we won't need that, so yay! :)
Attachment #602502 -
Flags: approval1.9.2.28? → approval1.9.2.28+
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #19) > https://hg.mozilla.org/releases/mozilla-1.9.2/rev/855980ed722d That changeset broke shared builds (e.g., my daily development debug build, Camino's cb-mini* tindeboxen) with Undefined symbols: "nsContentUtils::IsSystemPrincipal(nsIPrincipal*)", referenced from: nsDocShell::GetInheritedPrincipal(int) in nsDocShell.o linking libdocshell.dylib. I "fixed" that error by adding $(DEPTH)/content/base/src/$(LIB_PREFIX)gkconbase_s.$(LIB_SUFFIX) \ to docshell/build/Makefile.in's SHARED_LIBRARY_LIBS (and the equivalent directive to LOCAL_INCLUDES; not sure if that was needed or not, but it didn't make any difference). That, however, led to 440 lines of undefined symbol spew (see http://pastebin.mozilla.org/1504374) :( Worse, when I tried to perform the same trick to resolve the first unresolved symbol in that list of 440, I ended up with 790 lines of unresolved symbol spew :( I'm over my head here in terms of figuring out the right way to resolve these symbols correctly in shared builds; obviously continuing to add shared libs for each unresolved symbol is not it :( Someone more familiar with the build system and dependencies needs to look at it. This really is a blocker and needs to be resolved or backed out (in the short term, I can back this out locally to be able to continue to debug, but it's not a solution for more than a couple of days).
(In reply to comment #20) > This really is a blocker and needs to be resolved or backed out (in the > short term, I can back this out locally to be able to continue to debug, but > it's not a solution for more than a couple of days). A) Reading back over this, it sounds a lot more stringent and hand-wavy than I intended (the unfortunate side effects of spending an evening staring at linker errors rather than reviewing patches) B) For anyone who wasn't on irc, Gavin says he has a fix that he'll land tomorrow morning sometime after builds start or something like that :)
Comment 22•13 years ago
|
||
To clarify, this means that we can't use bookmarklets on chrome pages no more? I'm asking because I've used that as a hack to get good dialog sizes for l10n, see https://wiki.mozilla.org/L10n:Dialog_sizes.
![]() |
||
Comment 23•13 years ago
|
||
> That changeset broke shared builds
Er... do we still support those? I was under the impression that we could assume that docshell is linked into libxul...
Comment 24•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #23) > > That changeset broke shared builds > > Er... do we still support those? I was under the impression that we could > assume that docshell is linked into libxul... On 1.9.2 builds yes we do. We currently can't go for the next Thunderbird build as the test builders are busted and hence we have no test results. Although we shipped Thunderbird as static, there was no way in that timeframe that we were able to shift to libxul.
Assignee | ||
Comment 25•13 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #22) > To clarify, this means that we can't use bookmarklets on chrome pages no > more? Yes.
![]() |
||
Comment 27•13 years ago
|
||
Comment on attachment 603327 [details] [diff] [review] inline the code for 1.9.2 r=me
Attachment #603327 -
Flags: review?(bzbarsky) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•