Closed Bug 1484741 Opened Last year Closed Last year

javascript: bookmarklets via keywords with Alt+Enter no longer work after disallowInheritPrincipal bug 1466801

Categories

(Firefox :: Address Bar, defect, P1)

63 Branch
defect

Tracking

()

RESOLVED FIXED
Firefox 64
Tracking Status
firefox-esr60 --- unaffected
firefox61 --- unaffected
firefox62 --- unaffected
firefox63 + wontfix
firefox64 --- fixed

People

(Reporter: janmoesen_=-bugzilla-=+spamtrap, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [fxsearch])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:63.0) Gecko/20100101 Firefox/63.0
Build ID: 20180816220128

Steps to reproduce:

Filing a new bug as per :standard8's request in bug 1483148 comment 5.

Bookmarklets (bookmarks with a javascript: URI) no longer work after disallowInheritPrincipal bug 1466801. Executing those same bookmarklets with a keyword still works. Executing those same bookmarklets with a keyword but in a new tab with Alt+Enter no longer works.

1) Create a bookmarklet with URI: javascript:alert(%s) 
2) Give it a keyword: xxx
3) Save
4) In the location bar, enter xxx 1 followed by Enter
5) In the location bar, enter xxx 2 followed by Alt+Enter


Actual results:

4) Pops up an alert box with “1”
5) Opens a new tab, but does not show the alert
Component: Untriaged → Address Bar
jkt, can you pick this up as well?
Flags: needinfo?(jkt)
Flags: needinfo?(jkt)
Yeah will take a look next.
Assignee: nobody → jkt
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P1
[Tracking Requested - why for this release]:

What's blocking the fix here? This is riding with 63 atm, but given it's a regression I don't think we should be shipping this.
Flags: needinfo?(jkt)
Flags: needinfo?(jkt)
Jonathan, do you have any update? Thanks
Flags: needinfo?(jkt)
No update currently. I was looking at this last week and got confused by some edge cases that I still need to look into.
Flags: needinfo?(jkt)
So I'm seeing that the contentPrincipal of the about:blank page is a null principal. It looks like we can't create about:blank principals (we explicitly test for this in: caps/tests/unit/test_origin.js).
We call addTab and loadUri into it, loadURIWithOptions 

In nsJSProtocolHandler.cpp we check the principal subsumes the object principal before executing and return NS_ERROR_DOM_RETVAL_UNDEFINED because nothing but a system principal executes into null here other than the exact same. (https://searchfox.org/mozilla-central/rev/a23c3959b62cd3f616435e02810988ef5bac9031/dom/jsurl/nsJSProtocolHandler.cpp#249-255)

The submitted solution instead forces allowInheritPrincipal which copies the created null principal of the about:blank page as we can't inherit the system principal (both docshell and the protocol code are preventing this).

I'm leaning on just ignoring the location this should be created into given I don't understand the use-case of doing this really.

> I'm a little confused why this fixes anything. I also think we need to ensure we actually have a principal here, which AFAICT we don't in the normal return case, so presumably then this would inherit system principal, which seems bad. Can you elaborate on how this fix works?

Checking and preventing system in the urlbarBindings to allowInheritPrincipal actually breaks the code as docShell does the right thing for us when the about:blank is created.

I don't think I can pass anything into docshell here that is the correct triggeringPrincipal as my understanding is that docshell is creating the new principal within the load.

I still need to diagnose further how we are passing the correct null principal even when system is passed though to see if we can do something better here.
Any progress on the further diagnosing?
Flags: needinfo?(jkt)
The code that make this safe is after:
> One more twist: Don't inherit the principal for external loads.
https://searchfox.org/mozilla-central/rev/ce57be88b8aa2ad03ace1b9684cd6c361be5109f/docshell/base/nsDocShell.cpp#9361

:Gijs Unless you have another direction of fixing this, I don't see any alternative to the already submitted patch (minus something more drastic into the docshell code).

Essentially the problem is we create a new tab with a null principal and anything passed in doesn't match that. I think in the long run we should instead pass in a null triggeringPrincipal and create the tab with that.
Flags: needinfo?(jkt) → needinfo?(gijskruitbosch+bugs)
Comment on attachment 9006191 [details]
Bug 1484741 - Fixing bookmarklet keywords from loading in a new tab.

:Gijs (he/him) has approved the revision.
Attachment #9006191 - Flags: review+
Flags: needinfo?(gijskruitbosch+bugs)
Jonathan, do you want to land this patch and then request an uplift to 63?
Flags: needinfo?(jkt)
Whiteboard: [fxsearch]
I personally think we shouldn't uplift the patch I would rather we test it more in Nightly first. Despite the code working I'm not 100% on if we are doing the right thing still.

Sorry for the delay here.
Flags: needinfo?(jkt)
(In reply to Jonathan Kingston [:jkt] from comment #12)
> I personally think we shouldn't uplift the patch I would rather we test it
> more in Nightly first. Despite the code working I'm not 100% on if we are
> doing the right thing still.
> 
> Sorry for the delay here.

This hasn't actually landed on nightly, AFAICT... Did you intend for it to land on 64 nightly? :-)
Flags: needinfo?(jkt)
I triggered it on lando with that comment, it's queued to go in now.
Flags: needinfo?(jkt)
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7d8d3fc3e9f4
Fixing bookmarklet keywords from loading in a new tab. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/7d8d3fc3e9f4
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
See Also: → 1502072
You need to log in before you can comment on or make changes to this bug.