450533 - Use a formatter to prevent insertion of illegal characters in bookmark locations/URLs

Status: RESOLVED WONTFIX

(Camino Graveyard :: Bookmarks, defect)

Description

[Chris Lawson (gone)]

hendy pointed out an interesting bug on IRC tonight:

[11:49pm] <hendy> ahh, but if I bookmark Google and put it in the bookmark bar
[11:50pm] <hendy> then edit the location to put a linebreak in the middle
[11:50pm] <hendy> then drag it to the location bar, I get the linebreak
[11:50pm] <hendy> http://www.goo
[11:50pm] <hendy> gle.co.nz/

We should use a formatter to prevent this sort of thing from happening. There are no circumstances in which control characters or newlines are valid characters in a URL, so we shouldn't be allowing people to type them into the Location field in the Bookmark Info window (or in the Manager). We probably shouldn't allow them in the Title field either, which sort of gets into bug 313079 territory.

Comment 1

[Chris Lawson (gone)]

Attachment: fix v1

This should do it. Thanks for the help with that NSAttributedString issue, Stuart.

Comment 2

[Chris Lawson (gone)]

Attachment: fix v1.1

Sorry for the review-spam, murph. I didn't think I was going to be this productive :-p

Comment 3

[Chris Lawson (gone)]

To sum up, this patch does the following:

1) Prevents control characters from being entered in bookmark titles in
  a) the Add Bookmark dialog
  b) the Bookmark Manager's Title column
  c) the Bookmark Info window

2) Prevents control characters and leading/trailing whitespace from being entered in bookmark URLs in
  a) the Bookmark Manager's Location column
  b) the Bookmark Info window

Whitespace inside a bookmark's URL is still allowed. We might want to consider further refining this to use NSURL to ensure the validity of any input in the location field, though I haven't tried it with bookmarklets to see if it returns valid (and we definitely don't want to break bookmarklets with this formatter).

The only way you should be able to get newlines or other control characters into these fields now is by manually editing the plist (not sure if there's a way we can reasonably prevent or account for this) or by importing a bookmarks file that contains them in the appropriate place (which should be fixed with the DOM-based bookmarks import rewrite).

Comment 4

[Christopher Henderson]

If I try to add a newline in the title before a space the formatter removes the space. For example: Use a formatter to prevent... becomes Usea formatter to prevent...

I can also paste in a url with trailing whitespace that is not stripped and then one backspace will remove the n whitespace characters.

Some of the functions included in the patch for 450537 could possibly be utilised in the formatters.

Comment 5

[Sean Murphy]

Comment on attachment 333800 [details] [diff] [review]
fix v1.1

>+- (BOOL)isPartialStringValid:(NSString *)partialString newEditingString:(NSString **)newString errorDescription:(NSString **)error
>+{
>+  if ([partialString rangeOfCharacterFromSet:[NSCharacterSet controlCharacterSet]].location != NSNotFound) {
>+    *newString = [partialString stringByRemovingCharactersInSet:[NSCharacterSet controlCharacterSet]];
>+    return NO;
>+  }
>+  if (![[partialString stringByTrimmingWhitespace] isEqualToString:partialString]) {
>+    *newString = [partialString stringByTrimmingWhitespace];
>+    return NO;
>+  }
>+  return YES;
>+}

Due to the immediate method returns, if both trailing/ending whitespace and control characters exist in partialString, only the latter will be removed.  Also, when checking for and removing whitespace, rather than create a new string object twice, the first check would probably be better off just taking the above approach of looking for the range of whitespace character set.

Otherwise, the formatters work as advertised.  r=me with the above comment addressed.

Comment 6

[Chris Lawson (gone)]

Attachment: fix v1.2

I can't reproduce the pasting issue in comment 4 with the changes from bug 450537 in my tree, so I think it's safe to say that's solved by the other bug.

I can't reproduce the newline-deletes-space issue at all, but I wouldn't rule out that having been affected by one of these other bugs I've been working on lately.

I've addressed the early return part of comment 5, but I left the |stringByTrimmingWhitespace| check in there for two reasons: I couldn't think of a better way to check for whitespace at the beginning and/or end of a string without getting really complicated, and I'm pretty sure we need a second string object, at least temporarily, to ensure we can remove both whitespace *and* control characters if they exist.

cl

Comment 7

[Chris Lawson (gone)]

Comment on attachment 335908 [details] [diff] [review]
fix v1.2

Can't request second-r when uploading a patch, so murph, you'll want to read comment 6.

Comment 8

[Sean Murphy]

Comment on attachment 335908 [details] [diff] [review]
fix v1.2

>+  BOOL isValid = YES;
>+  // If partialString has no leading/trailing whitespace, this is a no-op, so tempString and partialString are equal.
>+  NSString* tempString = [partialString stringByTrimmingWhitespace];
>+
>+  if (![tempString isEqualToString:partialString]) {
>+    *newString = tempString;
>+    isValid = NO;
>+  }

Rather than creating a temp string, would this approach be a better alternative?

if ([partialString hasPrefix:@" "] || [partialString hasSuffix:@" "]) {
  *newString = [partialString stringByTrimmingWhitespace];
  isValid = NO;
}

With that considered, r=murph.

Comment 9

[Chris Lawson (gone)]

Comment on attachment 335908 [details] [diff] [review]
fix v1.2

Murph and I talked on IRC and figured out that I wasn't effectively communicating the possibility that a string could have both whitespace *and* control characters in it, thus the need for doing both. He's OK with this, so asking for sr.

Comment 10

[Stuart Morgan]

Comment on attachment 335908 [details] [diff] [review]
fix v1.2

>+    return nil;

Add a comment to this line that says "fall through to stringForObjectValue:" so that it's obvious what is happening.

>+  BOOL isValid = YES;
>+  // If partialString has no leading/trailing whitespace, this is a no-op, so tempString and partialString are equal.
>+  NSString* tempString = [partialString stringByTrimmingWhitespace];
>+
>+  if (![tempString isEqualToString:partialString]) {
>+    *newString = tempString;
>+    isValid = NO;
>+  }
>+  // But if partialString had leading/trailing whitespace *and* control characters, we want to remove both,
>+  // and doing it this way guarantees we get rid of all of it.
>+  if ([partialString rangeOfCharacterFromSet:[NSCharacterSet controlCharacterSet]].location != NSNotFound) {
>+    *newString = [tempString stringByRemovingCharactersInSet:[NSCharacterSet controlCharacterSet]];
>+    isValid = NO;
>+  }
>+  return isValid;

I'm with murph; the logic here seems confusing. Also, the order needs to flip in case the string has something like "foo \b "; the current version would leave "foo ". How about:

  NSString* tempString = [[partialString stringByRemovingCharactersInSet:[NSCharacterSet controlCharacterSet] stringByTrimmingWhitespace];
  if ([tempString length] != [partialString length]) { // <- much cheaper than comparing every character
    *newString = tempString;
    return NO
  }
  return YES;

>   [[[mBookmarksOutlineView tableColumnWithIdentifier:@"shortcut"] dataCell] setFormatter:shortcutFormatter];
>+  [[[mBookmarksOutlineView tableColumnWithIdentifier:@"url"] dataCell] setFormatter:urlFormatter];
>+  [[[mBookmarksOutlineView tableColumnWithIdentifier:@"title"] dataCell] setFormatter:titleFormatter];

Can we set these from the nib instead? I don't remember why I didn't do that at the time.

Comment 11

[Chris Lawson (gone)]

(In reply to comment #10)
> >   [[[mBookmarksOutlineView tableColumnWithIdentifier:@"shortcut"] dataCell] setFormatter:shortcutFormatter];
> >+  [[[mBookmarksOutlineView tableColumnWithIdentifier:@"url"] dataCell] setFormatter:urlFormatter];
> >+  [[[mBookmarksOutlineView tableColumnWithIdentifier:@"title"] dataCell] setFormatter:titleFormatter];
> 
> Can we set these from the nib instead? I don't remember why I didn't do that at
> the time.

I'll look into it tomorrow. I took care of the other issues but I'll wait to post a new patch until I know whether or not the nib thing is gonna work.

cl

Comment 12

[Chris Lawson (gone)]

Attachment: fix v1.3

Stuart and I talked about this on IRC and it turns out setting the formatters in the nib is a giant pain in the arse. I forget exactly what we determined the problem was, just that it wasn't going to be easy and would have been beyond the scope of this bug.

The new patch uses the |isValidURI| method added in bug 453623 to do the string validation in the URL formatter, which simplifies the logic there a bit.

Comment 13

[Chris Lawson (gone)]

Attachment: fix v1.31

Non-broken version of the patch. Oops.

Comment 14

[Stuart Morgan]

Comment on attachment 339821 [details] [diff] [review]
fix v1.31

>+  if (![partialString isValidURI]) {
>+    *newString = [[partialString stringByRemovingCharactersInSet:[NSCharacterSet controlCharacterSet]] stringByTrimmingWhitespace];
>+    return NO;
>+  }

isValidURI is not equivalent to what this formatter is supposed to do, nor should we have a formatter that can returns strings that it itself deems invalid.

Comment 15

[Gordon P. Hemsley [:GPHemsley]]

This bug has been buried in the graveyard and has not been updated in over 5 years. It is probably safe to assume that it will never be fixed, so resolving as WONTFIX.

[Mass-change filter: graveyard-wontfix-2014-09-24]