Closed Bug 224327 Opened 17 years ago Closed 16 years ago

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

Categories

(Core :: Networking: Cookies, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mconnor, Assigned: mvl)

References

Details

Attachments

(2 files, 1 obsolete file)

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."
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.
-> mvl :)
Assignee: darin → mvl
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.
Patch that does part 1, domainwalk when removing a permission.
Attachment #138502 - Flags: review?(dwitte)
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.
Attachment #138590 - Flags: review?(neil.parkwaycc.co.uk)
Blocks: 216743
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 != ? :)
mvl: nothing, :)  I don't know why I did it that way, it can easily be changed
before landing
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 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 ;-)
Attachment #138590 - Flags: review?(neil.parkwaycc.co.uk) → review-
Comment on attachment 138590 [details] [diff] [review]
fix cookieviewer to not allow duplicating permissions

Oh, and (!x == y) is silly.
Attachment #138590 - Attachment is obsolete: true
Attachment #139237 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #139237 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #139237 - Flags: superreview?(alecf)
Comment on attachment 139237 [details] [diff] [review]
fix cookieviewer to not allow duplicating permissions

sr=alecf
Attachment #139237 - Flags: superreview?(alecf) → superreview+
cookieviewer patch landed at 01/27/2004 08:42
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
Attachment #138502 - Flags: review?(dwitte) → review+
Attachment #138502 - Flags: superreview?(darin)
Attachment #138502 - Flags: superreview?(darin) → superreview+
backend patch checked in.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.