Closed Bug 313079 Opened 19 years ago Closed 9 days ago

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

Categories

(Camino Graveyard :: Bookmarks, defect)

All
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED INCOMPLETE

People

(Reporter: bugzilla-graveyard, Unassigned)

References

Details

Attachments

(1 file)

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.
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
Taking; I'll take a look at this shortly.

cl
Assignee: mikepinkerton → bugzilla
Status: NEW → ASSIGNED
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?
Is it better to add -setTitleWithCleanString:, or to change -setTitle: to
-setTitle:(NSString*) strippingControlChars:(BOOL) ?
(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 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
Target Milestone: Camino1.6 → ---
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 ;)
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 → ---
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
We'd still need to clean the drags, but that by itself would be pretty easy.
(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.
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
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)
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?
Status: NEW → RESOLVED
Closed: 9 days ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: