815640 - Make permission manager aware of "file://" (without dirty hacks)

Status: RESOLVED FIXED

(Core :: Permission Manager, defect)

Description

[Mounir Lamouri (:mounir)]

Attachment: Patch

Follow-up from bug 814554. "file://" handling might no longer be working in Firefox 17. The way it was being done was completely hacky (and insecure). This bug is about making "file://" handling less hacky and more secure. Basically, it will use the origin of the URI, which, for a "file://" is the spec. That way, you can have a better granularity and given how the permission manager is currently being designed, there are just two methods to change (principal -> host and host -> principal).

Comment 1

[Mounir Lamouri (:mounir)]

Couple of notes:
- there might be UI bits using uri.host, I'm not planning to fix all of them but I will attach a patch to fix the popup blocker;
- with this patch, previous ways to allow file:// popups will be ignored and will stay in the database. I tend to think we shouldn't really worry about that.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 685642 [details] [diff] [review]
Patch

Review of attachment 685642 [details] [diff] [review]:
-----------------------------------------------------------------

Don't you need to also migrate existing data? Leaving as r? until that's answered.

There used to be mochitests which relied on the "<file>" syntax working. But I guess those tests have been changed or removed since they didn't break with the original patch.

Do make sure to run this through try though.

Comment 3

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #2)
> Comment on attachment 685642 [details] [diff] [review]
> Patch
> 
> Review of attachment 685642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Don't you need to also migrate existing data? Leaving as r? until that's
> answered.

We can't. As said inn comment 0, this patch will allow per-file permissions (which is better for security) while the previous hack was allowing any file which means, I can't convert the current rules to the new ones.
Also, regarding migration, as said in comment 2, something that we might want to do is to delete all file related rules because they would no longer be relevant.

> There used to be mochitests which relied on the "<file>" syntax working. But
> I guess those tests have been changed or removed since they didn't break
> with the original patch.
> 
> Do make sure to run this through try though.

Actually, I was even thinking of writing test for this.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch

Same patch with tests.

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

As discussed with Mounir and Jonas out of bug - we'll only do this one for branches, not for the 17.0.1 respin.

Comment 6

[Mounir Lamouri (:mounir)]

Not sure if we should push that to beta but aurora clearly.

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Mounir: With the fixes in bug 814554, what are the remaining regressions from bug 777072? I.e. what does the patch in this bug *fix* vs. what features does it *add*?

Knowing what is broken I think will affect how far we uplift this patch.

Comment 8

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #7)
> Mounir: With the fixes in bug 814554, what are the remaining regressions
> from bug 777072? I.e. what does the patch in this bug *fix* vs. what
> features does it *add*?
> 
> Knowing what is broken I think will affect how far we uplift this patch.

AFAIK, bug 777072 created two regressions: the permissions loading became pedantic and the file:// permissions became unusable.
bug 814554 fixed the former problem and this bug is here to fix the later.

Comment 9

[Mounir Lamouri (:mounir)]

This bug isn't really about adding a feature. It is fixing the file:// permission handling to make it work and, in the meantime, it changes how it is working because it is too hacky.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But based on the bugs marked as blocking this one, and which have been open for a very long time, things like cookie exceptions and popup unblocking couldn't be done for file:// before either. So what *did* work for file URLs in the past?

Comment 11

[Mounir Lamouri (:mounir)]

(In reply to Jonas Sicking (:sicking) from comment #10)
> But based on the bugs marked as blocking this one, and which have been open
> for a very long time, things like cookie exceptions and popup unblocking
> couldn't be done for file:// before either. So what *did* work for file URLs
> in the past?

There was an official hack that allowed to set a "<file>" permission. It required using an extension reading the permission database IIRC.

As I said, I do not think this should go to the beta channel. This is too risky. Actually, the current patch doesn't even pass tests (I think there are crashes). I think it could go to aurora just to make sure the users relying on the "<file>" hack have a channel to switch on (more stable than Nightly).

Comment 12

[Mounir Lamouri (:mounir)]

Arf... <file> is actually still working. My patch actually doesn't produce crashes: it removes the hack so some tests are failing and a talos run too :(

I don't think we should track anything then. However, I still want to fix that and it's quite annoying that our own test suite is going to make it harder :(

Comment 13

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

This patch keeps "<file>" supports.

Comment 14

[Mounir Lamouri (:mounir)]

This bug is blocked on bug 816956. I will hopefully get back on those in two weeks.

Comment 15

[Mounir Lamouri (:mounir)]

Attachment: Patch (r=sicking)

Updated patch to make it apply on top of my queue.

Comment 16

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfd082020dfa

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cfd082020dfa