1525941 - Can't change permissions from network/cert error pages using page info dialog

Status: VERIFIED FIXED

(Firefox :: Page Info Window, defect, P3)

Description

[:Gijs (he/him)]

Example STR:

0. clean profile
1. open google maps
2. click the "my location" button in the bottom right to get google to prompt you if it can have access to your location
3. accept
4. file > work offline (or turn off internet connection or whatever)
5. reopen the page somehow (you will get a network error page)
6. open site identity popup

ER:
you can see that you've given https://www.google.com/ geolocation permission and can revoke this permission

AR:
no permissions are listed.

(Noted in https://phabricator.services.mozilla.com/D18586#475945 )

Comment 1

[Jan Henning [:JanH]]

Hmm. I had originally tried this with https://www.openstreetmap.org, and there this seems to be working - I turn off my network connection, clear the cache just to be sure, open a new tab, enter www.openstreetmap.org, and on the resulting error page, the site identity popup stll shows an option to "Edit Site Settings".

Comment 2

[Jan Henning [:JanH]]

Oh, I misread this - this bug is for Desktop. In that case never mind.

Comment 3

[Johann Hofmann [:johannh]]

Hey Danielle, this is very similar to bug 1501955, I think, would you like to take a stab at it?

Thanks!

Comment 4

[danielleleb12]

Attachment: Bug 1525941 - Adds ability to change permissions on network/cert error pages.

Comment 5

[danielleleb12]

Hi Johann, just created the patch, but now I realize that the "Permission for:" value is displaying as "about:" rather than "https://www.google.com" - working on that now.

Comment 6

[danielleleb12]

I'm having some trouble getting the correct value for the host (the issue I mentioned in my previous comment - "Permissions for: https://www.google.com). The value being used currently is gPermURI.displayPrePath, which of course is not the value that should be displayed when on the "about:neterror/certerror" pages. I've tried various ways of getting the host value off of gPermURI (gPermURI.host/hostName/displayHost), as well as document/window.location and gPermPrincipal.URI.host/hostName - but still am unable to get the correct value.

Is it possible to get that value with the information currently being provided to onLoadPermission (uri and principal) - or should I be passing something else if it is a about:certerror/neterror page?

Comment 7

[:Gijs (he/him)]

Hi! This bug is actually to do with the site identity popup (the thing that shows up if you click the lock and/or (i) icon in the location bar), not the page info dialog. The code lives in browser-siteIdentity.js .

Fixing the page info dialog would also be useful, and we could do it in one patch if it's easy... For the page info case, I think we need to pass more info to onLoadPermission from the code that calls it in pageInfo.js . It looks like windowInfo has the host name, but we still wouldn't have a principal. I'm not sure what the best way of obtaining it would be... Johann, can you help?

Comment 8

[Johann Hofmann [:johannh]]

Oh, sorry, that was my bad, I thought this was about page info, but of course it's in the site identity component.

I can not reproduce the problem with the site identity popup, Gijs, are you sure that you've given permanent permission (remember this decision)? Otherwise the permission will not be shown in any case...

Thanks!

Comment 9

[Johann Hofmann [:johannh]]

(In reply to danielleleb12 from comment #6)

I'm having some trouble getting the correct value for the host (the issue I mentioned in my previous comment - "Permissions for: https://www.google.com). The value being used currently is gPermURI.displayPrePath, which of course is not the value that should be displayed when on the "about:neterror/certerror" pages. I've tried various ways of getting the host value off of gPermURI (gPermURI.host/hostName/displayHost), as well as document/window.location and gPermPrincipal.URI.host/hostName - but still am unable to get the correct value.

Is it possible to get that value with the information currently being provided to onLoadPermission (uri and principal) - or should I be passing something else if it is a about:certerror/neterror page?

Yeah, I think browser.currentURI is what you're looking to pass with onLoadPermission, as well as a new principal based on that URI, which you can create with Services.scriptSecurityManager.createCodebasePrincipal(uri, {}); (technically we're losing data that way but it's all a bit messed up anyway and won't matter in practice right now).

Comment 10

[Johann Hofmann [:johannh]]

Whatever we end up fixing here (page info and/or site identity), thanks for working on it :)

Comment 11

[:Gijs (he/him)]

(In reply to Johann Hofmann [:johannh] from comment #8)

Oh, sorry, that was my bad, I thought this was about page info, but of course it's in the site identity component.

I can not reproduce the problem with the site identity popup, Gijs, are you sure that you've given permanent permission (remember this decision)? Otherwise the permission will not be shown in any case...

Thanks!

Uh, wow, that's... yep. OK.

So let's morph this to be about the page info window then...

(In reply to Johann Hofmann [:johannh] from comment #9)

(In reply to danielleleb12 from comment #6)

Yeah, I think browser.currentURI is what you're looking to pass with onLoadPermission, as well as a new principal based on that URI, which you can create with Services.scriptSecurityManager.createCodebasePrincipal(uri, {}); (technically we're losing data that way but it's all a bit messed up anyway and won't matter in practice right now).

For the principal creation, I guess we could use the userContextId from the tab to make sure we get the right container ?

Comment 12

[Johann Hofmann [:johannh]]

(In reply to :Gijs (he/him) from comment #11)

For the principal creation, I guess we could use the userContextId from the tab to make sure we get the right container ?

We could, though permissions are currently stripped of OAs anyway, so it won't make a difference at the moment https://searchfox.org/mozilla-central/rev/358f816f63da072145c593e9e2ac36b7250ecd25/extensions/cookie/nsPermissionManager.cpp#162-164

Bug 1330467 is trying to get the first party attribute un-stripped at least, but I don't think first party context matters for error pages.

OTOH it might be nice to future-proof this code and it's easy to do, I think. If I'm not missing something we could just do:

Services.scriptSecurityManager.createCodebasePrincipal(browser.currentURI, browser.contentPrincipal.originAttributes);

Unless there's a reason why we need it from the tab instead.

Comment 13

[Johann Hofmann [:johannh]]

(Thanks for confirming that it works for site identity :)

Comment 14

[danielleleb12]

Thank you both for all of the info!

I'm updating the patch now.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/990ff091adb3
Adds ability to change permissions on network/cert error pages. r=johannh,Gijs

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/990ff091adb3

Comment 17

[Julien Cristau [:jcristau]]

Johann, is this worth uplifting to beta 67?

Comment 18

[Johann Hofmann [:johannh]]

Comment on attachment 9052031 [details]
Bug 1525941 - Adds ability to change permissions on network/cert error pages.

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Very old
• User impact if declined: Users can not change permissions via page info on certificate error pages.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Well understood patch, a few lines of JS, limited to rarely used page info dialog, has full test coverage.
• String changes made/needed: None

Comment 19

[Pascal Chevrel (relman team) -> :pascalc]

Old regression and P3, the patch looks not risky but let's get it verified by QA before uplifting.

Comment 20

[Gyula Palko[:gpalko]]

Verified, that the issue is not reproducible on Nightly 68(20190416095014), on Win 10 x64, Ubuntu 18.01 and MacOS 10.14

Comment 21

[Pascal Chevrel (relman team) -> :pascalc]

Comment on attachment 9052031 [details]
Bug 1525941 - Adds ability to change permissions on network/cert error pages.

Low risk patch, uplift approved for 67 beta 12, thanks.

Comment 22

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/d49678c63d88

Comment 23

[Gyula Palko[:gpalko]]

Verified, that the issue is not reproducible on Beta 67.0b12(20190418160535), on Win 10 x64, Ubuntu 18.01 and MacOS 10.14