224327 - if domain.com is blocked, sub.domain.com is blocked, but cannot be unblocked using remove

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect)

Description

[Mike Connor [:mconnor]]

spun off from http://bugzilla.mozilla.org/show_bug.cgi?id=205541#c19

"The problem is that you can't find out what to remove. For example, if you
block domain.com from setting cookies, TestPermissions will return blocked for
sub.domain.com. But calling Remove with sub.domain.com won't do anything. We
need to decide what should happen."

Comment 1

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

The current nsIPermissionManager api worked when it was written. When you add a
permission a site, you are visiting the site, so passing a uri to Add made sense.
When you are deleting a permission, you are in the manager, and know the string
that you want to remove. So, that made sense too.

Now, you can add permissions when you are not visiting a site, and remove
permissions when you are visiting. So it is not that clear anymore.

And when path matching, wildcards or regexps come into play, it is even more
complicated. So it clearly needs some thought.

Comment 2

[Darin Fisher]

-> mvl :)

Comment 3

[Mike Connor [:mconnor]]

bit of discussion with mvl on IRC about this led to the following plan:

1) turn domainwalking on for .remove, and if we have to domainwalk then prompt
before removing the perm on the parent domain.  The prompt is optional, we could
include a pref to not show again.

2) make sure throughout the UI to not set the same permission for a subdomain so
that we minimize situation where foo.bar.baz.com is getting blocked because of
bar.baz.com and baz.com.  If baz.com is already set, don't bother setting
bar.baz.com.  We could always not break on match for the domainwalk and continue
upwards, not sure if this is a perf issue or not.

Comment 4

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

Attachment: domainwalk in remove

Patch that does part 1, domainwalk when removing a permission.

Comment 5

[Mike Connor [:mconnor]]

Attachment: fix cookieviewer to not allow duplicating permissions

covers part 2 above, the Tools->Cookie Manager options already won't allow you
to set the permission if its the same as the current permission.  This extends
this to cookiemgr

On further review of these changes, I do not believe anything here is of
sufficient severity to warrant a dialog, despite my earlier comments.

Comment 6

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

Comment on attachment 138590 [details] [diff] [review]
fix cookieviewer to not allow duplicating permissions

>Index: mozilla/extensions/wallet/cookieviewer/resources/content/CookieViewer.js
>+  if (!permissionmanager.testPermission(uri, "cookie") == action)

What is wrong with != ? :)

Comment 7

[Mike Connor [:mconnor]]

mvl: nothing, :)  I don't know why I did it that way, it can easily be changed
before landing

Comment 8

[[e-mail]]

Let's see if I got this right:
if domain.com is blocked and sub.domain.com is unblocked, then when you're
visiting sub.domain.com and you say "set to default" it changes to blocked, then
when you say "set to default" again it sets it to default?

Comment 9

[[e-mail]]

Comment on attachment 138590 [details] [diff] [review]
fix cookieviewer to not allow duplicating permissions

>-  permissionmanager.add(uri, dialogType, action);
>+  // only set the permission if the permission doesn't already exist
>+  if (!permissionmanager.testPermission(uri, "cookie") == action)
>+    permissionmanager.add(uri, dialogType, action);
>+  
  ^Space?
In the unlikely event that you think you can persuade me that the literal
cookie string is correct feel free to rerequest review ;-)

Comment 10

[[e-mail]]

Comment on attachment 138590 [details] [diff] [review]
fix cookieviewer to not allow duplicating permissions

Oh, and (!x == y) is silly.

Comment 11

[Mike Connor [:mconnor]]

Attachment: fix cookieviewer to not allow duplicating permissions

Comment 12

[Alec Flett]

Comment on attachment 139237 [details] [diff] [review]
fix cookieviewer to not allow duplicating permissions

sr=alecf

Comment 13

[Mike Connor [:mconnor]]

cookieviewer patch landed at 01/27/2004 08:42

Comment 14

[[e-mail]]

Comment on attachment 138502 [details] [diff] [review]
domainwalk in remove

nice! just one nit.

>Index: extensions/cookie/nsPermissionManager.cpp
>===================================================================
>+nsHostEntry *
>+nsPermissionManager::GetHostEntry(const nsAFlatCString &aHost,
>+                                  PRUint32              aType)
>+{
>   PRUint32 offset = 0;
>+  nsHostEntry *entry = nsnull;

no need to init |entry| to null here :)

r=dwitte

Comment 15

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

backend patch checked in.