200644 - rework port handling in permissions code

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[[e-mail]]

from a discussion in bug 176950, it's evident that the port handling in the
permission backend isn't really implemented properly. so, opening this bug to
figure out what to do about ports. pasted from bug 176950 comment 7, though I've
left out a lot of stuff:

so we could do something like:
1) given a URI e.g. "http://foo.com/", first check the permission list for the
host foo.com on the default http port, which would be "foo.com:80".
2) if no entry was found, then check for foo.com with no port, which would be
"foo.com".

the code to do this is very simple; the only issue that bsmedberg pointed out
was that we now have to do twice the number of hash lookups, but it's the most
flexible and powerful option. if we decide we're okay with assuming a specific
port for everything (i.e. blocking "http://foo.com" will only block port 80,
which would make doug's library example impossible to solve), then we can assume
default ports for unspecified entries, and eliminate step 2. we could also drop
ports altogether, and keep things simple. either of these three options sounds
valid to me.

Comment 1

[Michiel van Leeuwen (email: mvl+moz@)]

I suggest to just ignore ports while looking up permissions. When you block
images for domain.com, you most likely also wants to block images when you are
pointed to port 8080 instead of the normal port.
Checking the port would make it confusing for the user. It would also have an
impact on speed, as we need twice as much lookups.

Comment 2

[benc]

you might be able to store the "block all ports" as the entry port=0...

I don't know if that helps w/ the search optimization or not.

I would like to keep ports, even though I've been told cookies are cross-port
states, and the utility for ad blocking is limited (people want expansive domain
blocking, not more granularity).

The fundamental building block of the IP network is the socket, and that is a
hostip:port in the modern world. If we are careless now and ignore port,
something bad will happen later, I just can't think of an example right now.

Comment 3

[[e-mail]]

the way you store the "block all ports" wildcard doesn't really matter, the
problem is it forces you to do twice the number of lookups. e.g. "foo.com:80"
needs to check if there's an entry for its specific port first (80), then check
if there's an entry for all ports.

it's a nice thing to have... question is, do we want to do it now, or wait until
someone demonstrates a need for it (which may be never)?

i like mvl's suggestion for now. less bloat, more perf :)

Comment 4

[benc]

if that is what you want. please make it so we can add port support in the
future w/o having to gut everything you put into place now. (aka cookies?)

Comment 5

[[e-mail]]

well, what I think really means nothing until other more-qualified people
approve. ;)

but as regards to adding it in the future, a) we don't have it in a working
state right now, so it's hard to make things worse b) even so, it's quite
trivial to add (in principle, it's just one extra lookup, but in practice we
might want a new hash entryclass and hashing function to store the port as an
integer, like bsmedberg suggested).

Comment 6

[[e-mail]]

*** Bug 69486 has been marked as a duplicate of this bug. ***

Comment 7

[[e-mail]]

cc'ing mstoltz - can you take a look at this and give us your thoughts?

Comment 8

[Mitchell Stoltz (not reading bugmail)]

I don't have much to add. I think it would be a nice power-user feature to block
based on prts, but I'm not sure it's all that necessary for the average user.

Comment 9

[Michiel van Leeuwen (email: mvl+moz@)]

*** Bug 224606 has been marked as a duplicate of this bug. ***

Comment 10

[Scott Garfinkle]

This discussion has been going on way too long. Just ignore the port, for now,
which is obviously what anyone you would expect as a default. Then, later, if
someone really wants to implement port-based blocking, great. In the meantime,
this is just another place people can point to and say, "See, IE really is better."

Comment 11

[[e-mail]]

so who wants to write the patch?

Comment 12

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: ingore the port

This patch just makes the permission manager ignore the port.
The downside of it is that current entries with a port are not used anymore. Do
we consider that a problem?

Comment 13

[Scott Garfinkle]

IMO, Since port-based popup blocking was never, AFAIK, advertised to work, it
doesn't seem crucial to worry about breaking it. If anybody complains, then a
new bug could be opened to look at actually implementing one of the various
port-based popup blocking options discussed above.

Comment 14

[Michiel van Leeuwen (email: mvl+moz@)]

Attachment: also update the js

Forgot to update the javascript...

Comment 15

[[e-mail]]

Comment on attachment 134773 [details] [diff] [review]
ingore the port

r=me... simple patch ;)

Comment 16

[Darin Fisher]

Comment on attachment 134773 [details] [diff] [review]
ingore the port

code looks fine.  are there compatibility issues to worry about?  what happens
when someone goes back to an older version of moz?

Comment 17

[Michiel van Leeuwen (email: mvl+moz@)]

The only problem can be when someone allow say domain.com:8080 to show popups.
He now has to allow just domain.com. If he leaves the old entry in his
cookperm.txt, nothing bad will happen when he goes back to an old version.

Comment 18

[Michiel van Leeuwen (email: mvl+moz@)]

Both patches are checked in.
Leaving the bug open to discuss more advanced port handling.

Comment 19

[[e-mail]]

do we even want more advanced port handling? i think "no ports" is just fine ;)

your call...

Comment 20

[Michiel van Leeuwen (email: mvl+moz@)]

If that is the convlusion of the discussion, ok :)
But there were some comments wanting better stuff. I personally think it is ok
as it is now.

Comment 21

[[e-mail]]

then wontfix it, you're the MO :)

<mvl>   don't spam me :)

ok ;)

Comment 22

[Michiel van Leeuwen (email: mvl+moz@)]

closing this.
If someone disagree, reopen or file a new bug.