Don't implicitly unwrap NSURLs read from user bookmarks

RESOLVED FIXED in 3.0

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

(Blocks: 1 bug)

unspecified
All
iOS

Firefox Tracking Flags

(fxios3.0+)

Details

Attachments

(1 attachment)

48 bytes, text/x-github-pull-request
sleroux
: review+
tecgirl
: ui-review+
Details | Review | Splinter Review
(Assignee)

Description

3 years ago
I have a record. It appears as a BookmarkItem in the node tree.

The URL is "http://www.amazon.com/s/ref=nb_sb_noss?url=search-alias%3Ddigital-music&field-keywords=%s"

The data is:

id          guid          type        is_deleted  parentid      parentName      feedUri     siteUri     pos         title       description  bmkUri      tags        keyword     folderName  queryId     server_modified  hasDupe
----------  ------------  ----------  ----------  ------------  --------------  ----------  ----------  ----------  ----------  -----------  ----------  ----------  ----------  ----------  ----------  ---------------  ----------
4932        MUr6CtisszP0  1           0           6Kxq2EXxLzDa  Searches                                        Amazon MP3  Shop over 20 million songs and play your music on your Kindle Fire, Android device, PC, Mac, or iPad with Amazon Cloud Drive and Amazon Cloud Player.  http://www.amazon.com/s/ref=nb_sb_noss?url=search-alias%3Ddigital-music&field-keywords=%s  []          mp3                                 1456279946010    0


BookmarksPanel does this:

        case let item as BookmarkItem:
            homePanelDelegate?.homePanel(self, didSelectURL: NSURL(string: item.url)!, visitType: VisitType.Bookmark)
            break



so we fail to unwrap the URL, because it doesn't parse, because it has a substitution. We should be safer here and elsewhere.
(Assignee)

Comment 1

3 years ago
Created attachment 8724306 [details] [review]
Pull req.

Stacking pull requests as high as they'll go. These are the three commits now on the end -- check the bug numbers.

The behavior now is that a bookmark with a malformed URL will result in _searching_ for that string on the user's default search engine. DuckDuckGo and Google in particular do a good job of this. This is exactly what happens if you copy and paste into the URL bar.

The alternatives are:

* Do nothing -- your tap is wasted.
* Show some kind of toast.
* Put a lot more work into URLFixer (but there are likely to be edge cases always).

ui-review to Robin to make sure she's happy with this experience. (Sure beats a crash, at least.)
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #8724306 - Flags: ui-review?(randersen)
Attachment #8724306 - Flags: review?(sleroux)
Comment on attachment 8724306 [details] [review]
Pull req.

LGTM!
Attachment #8724306 - Flags: review?(sleroux) → review+
(Assignee)

Comment 3

3 years ago
721a963
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.0
Attachment #8724306 - Flags: ui-review?(randersen) → ui-review+
You need to log in before you can comment on or make changes to this bug.