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

RESOLVED FIXED in mozilla21

Status

()

Core
Permission Manager
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

(Blocks: 1 bug)

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Created attachment 685642 [details] [diff] [review]
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).
Attachment #685642 - Flags: review?(jonas)
(Assignee)

Updated

5 years ago
status-firefox17: --- → affected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox-esr17: --- → affected
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Updated

5 years ago
Depends on: 815650
(Assignee)

Updated

5 years ago
tracking-firefox17: --- → ?
tracking-firefox18: --- → ?
tracking-firefox19: --- → ?
tracking-firefox-esr17: --- → ?
Blocks: 670577, 204285
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.
(Assignee)

Comment 3

5 years ago
(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.
(Assignee)

Comment 4

5 years ago
Created attachment 686124 [details] [diff] [review]
Patch

Same patch with tests.
Attachment #685642 - Attachment is obsolete: true
Attachment #685642 - Flags: review?(jonas)
Attachment #686124 - Flags: review?(jonas)
As discussed with Mounir and Jonas out of bug - we'll only do this one for branches, not for the 17.0.1 respin.
tracking-firefox17: ? → -
tracking-firefox18: ? → +
tracking-firefox19: ? → +
(Assignee)

Comment 6

5 years ago
Not sure if we should push that to beta but aurora clearly.
Attachment #686124 - Flags: review?(jonas) → review+

Updated

5 years ago
tracking-firefox-esr17: ? → +
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.
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
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.
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?
(Assignee)

Comment 11

5 years ago
(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).
(Assignee)

Comment 12

5 years ago
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 :(
status-firefox17: affected → ---
status-firefox18: affected → ---
status-firefox19: affected → ---
status-firefox20: affected → ---
status-firefox-esr17: affected → ---
tracking-firefox17: - → ---
tracking-firefox18: + → ---
tracking-firefox19: + → ---
tracking-firefox-esr17: + → ---
(Assignee)

Updated

5 years ago
No longer blocks: 814554
(Assignee)

Updated

5 years ago
Blocks: 817007
Depends on: 816956
(Assignee)

Comment 13

5 years ago
Created attachment 687114 [details] [diff] [review]
Patch v2

This patch keeps "<file>" supports.
Attachment #687114 - Flags: review?(jonas)
Attachment #687114 - Flags: review?(jonas) → review+
(Assignee)

Comment 14

5 years ago
This bug is blocked on bug 816956. I will hopefully get back on those in two weeks.
(Assignee)

Updated

5 years ago
Component: General → Permission Manager
(Assignee)

Comment 15

5 years ago
Created attachment 694337 [details] [diff] [review]
Patch (r=sicking)

Updated patch to make it apply on top of my queue.
Attachment #686124 - Attachment is obsolete: true
Attachment #687114 - Attachment is obsolete: true
(Assignee)

Comment 16

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cfd082020dfa
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/cfd082020dfa
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Blocks: 836730
You need to log in before you can comment on or make changes to this bug.