Need a class-level filter to prevent bad data in setting bookmark titles (e.g., newlines or other control characters)

NEW
Unassigned

Status

Camino Graveyard
Bookmarks
12 years ago
7 years ago

People

(Reporter: Chris Lawson (gone), Unassigned)

Tracking

Details

Attachments

(1 attachment)

(Reporter)

Description

12 years ago
If you drag a selected plaintext URL to the bookmarks bar -- for example:

http://www.example.com/

and include the newline in the selection when you drag, a newline gets inserted
at the end of the bookmark's title, which causes a two-line title in the
bookmarks bar. This is, needless to say, quite ugly, and it looks VERY odd.

The same effect can be produced by getting info on any bookmark in the bookmarks
bar, placing the cursor at the end of the Title field, and hitting
option-return, which inserts a carriage return character.

We probably shouldn't be allowing this, as I can't think of any reason why you'd
want a carriage return in the middle of a bookmark title.
(Reporter)

Comment 1

12 years ago
Tweaking summary to reflect full scope of bug.

cl
Summary: When dragging URL selection to bookmarks bar, newline gets appended to bookmark, causing weird text effects → Bookmark titles allow newline characters, causing weird visual effects in bookmarks bar
(Reporter)

Comment 2

12 years ago
Taking; I'll take a look at this shortly.

cl
Assignee: mikepinkerton → bugzilla
(Reporter)

Updated

12 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 3

12 years ago
Created attachment 209193 [details] [diff] [review]
cleans titles/keywords in setting them

Barring any stupidity in generating the patch (entirely possible, as this touches several files, but I'm fairly certain it's kosher), this should do it.

Note: the patch also fixes a bunch of compiler warnings caused by missing casts. These should be fairly obvious in review.

cl
Attachment #209193 - Flags: review?

Comment 4

12 years ago
Is it better to add -setTitleWithCleanString:, or to change -setTitle: to
-setTitle:(NSString*) strippingControlChars:(BOOL) ?
(Reporter)

Comment 5

12 years ago
(In reply to comment #4)
> Is it better to add -setTitleWithCleanString:, or to change -setTitle: to
> -setTitle:(NSString*) strippingControlChars:(BOOL) ?

I can think of arguments in favour of either solution. Stuart suggested the former on IRC last night, but I had also briefly considered the latter.

Do you have a strong preference one way or the other?

cl

Comment 6

12 years ago
Comment on attachment 209193 [details] [diff] [review]
cleans titles/keywords in setting them

+      [mBookmarkItem setKeyword:[[mBookmarkKeywordField stringValue] stringByReplacingCharactersInSet:[NSCharacterSet controlCharacterSet] withString:@" "]];
+        [mBookmarkItem setKeyword:[[mFolderKeywordField stringValue] stringByReplacingCharactersInSet:[NSCharacterSet controlCharacterSet] withString:@" "]];
+    [mBookmarkItem setKeyword:[[changedField stringValue] stringByReplacingCharactersInSet:[NSCharacterSet controlCharacterSet] withString:@" "]];

Keywords should be done in one place (the same way title is done, whichever way that goes).

-      [self setTitle:nil];
+      [self setTitleWithCleanString:nil];

You may as well just remove this line instead of change it; it's a deceptive no-op.

I suggested making setTitle safe, rather than a separate safe function, to help prevent new uses from doing the wrong thing, and to work better with KVC (table editing, for example).  On the other hand this is a lot more changes than I had anticipated when I suggested this approach.

Perhaps some/most/all of the uses outside of the Bookmark, BookmarkItem, and BookmarkFolder classes should just call setTitle, to make the code simpler?  There's minimal hit to cleaning the setup of special folders, for example.  And whether importing should call the clean version should really be thought about--can all other browsers be trusted to prevent newlines?  How much overhead would it add to the import?
Attachment #209193 - Flags: review? → review-
We should think about all these issues for 1.2
QA Contact: bookmarks
Target Milestone: --- → Camino1.2

Updated

11 years ago
Target Milestone: Camino1.6 → ---
(Reporter)

Comment 8

11 years ago
I'd like to go ahead and get this done for 1.6, but we need to have more discussion about which way we want to go in the code.

I doubt the overhead of cleaning up imported bookmarks is going to be significant in the grand scheme of things (a few percent at worst, maybe?), since history seems to suggest we can't trust other browsers to do *anything* logical and our bookmarks importer doesn't need to be any more duct-tape-and-baling-wire than it already is ;)
(Reporter)

Updated

11 years ago
Target Milestone: --- → Camino1.6
Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.
Target Milestone: Camino1.6 → ---
(Reporter)

Comment 10

10 years ago
What about just using a formatter on the field to prevent things like newlines? That would solve the editing portion of this bug without resorting to any cleaning at all, although it wouldn't solve the "do we trust other browsers to prevent newlines on import" bit. (I suspect Stuart's DOM-based rewrite of the bookmarks importer could easily solve that, though.)

cl
(Reporter)

Comment 11

10 years ago
We'd still need to clean the drags, but that by itself would be pretty easy.
(Reporter)

Comment 12

10 years ago
(In reply to comment #11)
> We'd still need to clean the drags, but that by itself would be pretty easy.

Actually, thanks to bug 450537, I don't think the drags can be dirty any more, so a formatter + import rewrite is all we have to worry about here. I'll just assume Stuart will do something sane in the import. Patch coming for the formatter approach.
(Reporter)

Comment 13

10 years ago
Stuart said on IRC that he might still like a class-level filter to ensure clean titles, so we should think about that some more.

The new patch over on bug 450533 does fix this bug as filed, however.
Depends on: 450533, 450537
Hardware: Macintosh → All
(Reporter)

Updated

10 years ago
Summary: Bookmark titles allow newline characters, causing weird visual effects in bookmarks bar → Need a class-level filter to prevent bad data in setting bookmark titles (e.g., newlines or other control characters)
(Reporter)

Comment 14

9 years ago
Someone else can think about what to do, but in the meantime this doesn't need to be assigned to me any more (at least not until we decide something).
Assignee: cl-bugs-new → nobody
Status: ASSIGNED → NEW
Can we decide what we want to do here, since we have a real-live, in the wild case of newlines-in-titles courtesy Google Reader over in bug 618995.  Or, alternatively, can we agree to fix the dragging case separately over there while we debate what to do here to handle editing cases?
You need to log in before you can comment on or make changes to this bug.