Do the right thing when expanding cookie exceptions for numeric (dotted-decimal) IPs

ASSIGNED

Status

Camino Graveyard
Preferences
ASSIGNED
8 years ago
6 years ago

People

(Reporter: Smokey Ardisson (offline for a while; not following bugs - do not email), Assigned: Chris Lawson (gone))

Tracking

Details

Attachments

(1 attachment, 2 obsolete attachments)

1.74 KB, patch
Stuart Morgan
: superreview-
Details | Diff | Splinter Review
While working on deciphering bug 515837 and following the trail to bug 480779 and bug 480892, I noticed that we propose to expand numeric IP exceptions the same way we do domain names, namely chopping off the stuff before the first dot

192.168.0.1 --> "Expand Exception to All of “168.0.1”"

which is assuredly wrong.

I don't know if the permissions code has support for blocking IP blocks/subnets/whatever, so we need to investigate that first, but if it does, we need to figure out the correct syntax for permissions and use it ;-)

[9:45pm] cl: because expanding an exception for "192.168.0.240" should first expand to 192.168.0.1/255 (I think I have that syntax right)

If permissions doesn't have support for blocking IP blocks, we need to check and see if the selected item is four octets and then disable the "expand exception" menu item.
Summary: Do the right thing when expanding exceptions for numeric (dotted-decimal) IPs → Do the right thing when expanding cookie exceptions for numeric (dotted-decimal) IPs
(Assignee)

Comment 1

6 years ago
(In reply to Smokey Ardisson (not following bugs - do not email) from comment #0)
> If permissions doesn't have support for blocking IP blocks, we need to check

I'm about 99% certain this is the case. I haven't been able to confirm with any Core devs yet, but it sure doesn't look like the nsIPermissionsManager code is written to account for this situation at all.

> and see if the selected item is four octets and then disable the "expand
> exception" menu item.

Patch coming shortly.
(Assignee)

Comment 2

6 years ago
Created attachment 596400 [details] [diff] [review]
fix v1.0

This oughta do it.
Assignee: nobody → cl-bugs-new2
Status: NEW → ASSIGNED
Attachment #596400 - Flags: feedback?(alqahira)
Comment on attachment 596400 [details] [diff] [review]
fix v1.0

This appears to work as expected.  Some comments/questions, though:

1) Instead of using the enumerator, why not just use stringByRemovingCharactersInSet: to strip the periods from the whole host string.  It seems like it'd accomplish the same thing with a lot less work/code (the internet says that RFC 1700 says that 0.0.0.0/8 is reserved for broadcast messages on the local network, so 0.0.0.0, while a valid IP, should never be running a webserver or setting cookies, so we wouldn't have to worry about it failing the intValue test).

2) Do we care about preventing the continued expansion of bogus exceptions we'd already expanded, e.g. from 192.168.0.1 to 168.0.1?  If so, the check for 4 components is going to fail, because they may be 3 or 2 at this point.  (I think we don't care, but I wanted to mention it.)

3) The CM UI looks a little weird, I guess, but it's the same weird UI we have if you already have expanded an exception all the way down to com.

4) Isn't this comment wrong, post-bug 480892?

-  if (dotRange.location == 0) // .bar.com == bar.com
+  }
+  if (dotRange.location == 0) { // .bar.com == bar.com

We no longer set .bar.com permissions (or .baz.bar.com, or any other leading-period syntax), so this is the bit that expands "bar.com" to "com".  Also, since you're messing with this, flip the comment above the code.

5) Post-bug 480892, isn't " || dotRange.location == ([host length] - 1)" also superfluous?

-  if (dotRange.location == NSNotFound || dotRange.location == ([host length] - 1))
+  if (dotRange.location == NSNotFound || dotRange.location == ([host length] - 1)) {
Attachment #596400 - Flags: feedback?(alqahira) → feedback+
(Assignee)

Comment 4

6 years ago
Created attachment 596534 [details] [diff] [review]
fix v1.1

(In reply to Smokey Ardisson (not following bugs - do not email) from comment #3)
> Comment on attachment 596400 [details] [diff] [review]
> fix v1.0

> 1) Instead of using the enumerator, why not just use
> stringByRemovingCharactersInSet: to strip the periods from the whole host

Yeah, I forgot about that. This version uses it...

> 2) Do we care about preventing the continued expansion of bogus exceptions
> we'd already expanded, e.g. from 192.168.0.1 to 168.0.1?  If so, the check
> for 4 components is going to fail, because they may be 3 or 2 at this point.

...and dispenses entirely with the 4-component check. As a nice side effect, any exceptions already stored like this won't be expanded further (because they'll evaluate to a non-zero intValue), but there's no cleanup code to get rid of them.

> 4) Isn't this comment wrong, post-bug 480892?
> 
> -  if (dotRange.location == 0) // .bar.com == bar.com

That comment was *always* sort of wrong, or at the very least extremely confusing. I've rewritten it to be much clearer.

> 5) Post-bug 480892, isn't " || dotRange.location == ([host length] - 1)"
> also superfluous?

Yep, so I removed it.

Thanks for the feedback, Smokey!
Attachment #596400 - Attachment is obsolete: true
Attachment #596534 - Flags: superreview?(stuart.morgan+bugzilla)
(In reply to Chris Lawson from comment #4)
> any exceptions already stored like this won't be expanded further (because
> they'll evaluate to a non-zero intValue), but there's no cleanup code to get
> rid of them.

If we really wanted to do that, it would be relatively simple; we'd just do roughly this code in reverse.  Check to see if each exception item evaluates to a non-zero integer when the periods are removed, and, if so, then check to see if it's 4 components; if not, then delete the exception.  Like I said, though, not sure if it's worth it.

Comment 6

6 years ago
Comment on attachment 596534 [details] [diff] [review]
fix v1.1

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

::: PreferencePanes/Privacy/PrivacyPane.mm
@@ +593,4 @@
>      return nil;
> +  }
> +  // Ignore any leading dot in a hostname; i.e., ".baz.foo.com" has a
> +  // superdomain of "foo.com".

Move both these comments inside the blocks they are about; putting anything between a } and an else really hurts readability.

@@ +601,5 @@
> +  // which fails this test but shouldn't ever set cookies.
> +  else {
> +    NSCharacterSet* dotSet = [NSCharacterSet characterSetWithCharactersInString:@"."];
> +    NSString* strippedHost = [host stringByRemovingCharactersInSet:dotSet];
> +    if ([strippedHost intValue] != 0)

stringByRemovingCharactersInSet and then parsing the string is an awfully big hammer just to avoid a couple more lines of code.

Loop over the string with characterAtIndex:, and check whether the character is either in the decimal set, or is a ".". If not, then it's not a numeric host, and you can set a BOOL and break.

(That makes the normal case a singe character comparison instead of a character-by-character check-and-copy; obviously this is not perf-critical code, but we shouldn't do things that are both less performant and less clear to a reader.)
Attachment #596534 - Flags: superreview?(stuart.morgan+bugzilla) → superreview-
(Assignee)

Comment 7

6 years ago
Created attachment 607457 [details] [diff] [review]
fix v1.11

Like this, then?
Attachment #596534 - Attachment is obsolete: true
Attachment #607457 - Flags: superreview?(stuart.morgan+bugzilla)

Comment 8

6 years ago
Comment on attachment 607457 [details] [diff] [review]
fix v1.11

>diff --git a/PreferencePanes/Privacy/PrivacyPane.mm b/PreferencePanes/Privacy/PrivacyPane.mm
>+    for(unsigned i = 0; i < [host length]; i++) {

Missing a space before (

>+      if (([host characterAtIndex:i] != '.') &&
>+          (![[NSCharacterSet decimalDigitCharacterSet] characterIsMember:[host characterAtIndex:i]]))

Move [NSCharacterSet decimalDigitCharacterSet] to outside the loop in a local variable instead of calling every time. Move [host characterAtIndex:i] to a local variable just before the |if| instead of calling it twice per character.
Attachment #607457 - Flags: superreview?(stuart.morgan+bugzilla) → superreview-
You need to log in before you can comment on or make changes to this bug.