Closed Bug 1524669 Opened 2 years ago Closed 2 years ago

Add protocol checking to link URLs for pocket new tab

Categories

(Firefox :: New Tab Page, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 67
Iteration:
67.1 - Jan 28 - Feb 10
Tracking Status
firefox66 + verified
firefox67 --- verified

People

(Reporter: k88hudson, Assigned: pdahiya)

References

Details

(Keywords: github-merged)

Attachments

(2 files)

This would be nice to have as another check to prevent javascript: or other non-http[s] links.

Iteration: --- → 67.1 - Jan 28 - Feb 10
Priority: -- → P2
Priority: P2 → P1

Something just like this would work, but I think we would want to limit it to http[s] especially for links:

https://searchfox.org/mozilla-central/source/browser/components/newtab/content-src/asrouter/template-utils.js#1

Assignee: nobody → pdahiya
See Also: → 1521203

Images don't need to be additionally checked as they're already covered by our CSP, e.g., manually changing the https url to ftp:

Content Security Policy: The page’s settings blocked the loading of a resource at ftp://img-getpocket.cdn.mozilla.net/direct?url=https%3A%2F%2Fcdn-images-1.medium.com%2Fmax%2F1200%2F1%2ArrEPDWN-EtPxvxE6N7Qp1A.jpeg&resize=w450 (“img-src”).
Summary: Add protocol checking to images/link URLs for pocket new tab → Add protocol checking to link URLs for pocket new tab
Keywords: github-merged
Blocks: 1527504
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

How to test using test feed:

  1. go to about:config
  2. search for discoverystream.config and set preference to {"enabled":true,"show_spocs":true,"layout_endpoint":"https://gist.githubusercontent.com/punamdahiya/566a2665e2cc92e691d8bfb3ce555926/raw/39ead6a18de9a6f58976f8e3a4ddefcf884720bf/test-feed.json"}
  3. Open new tab and search for vox.com item with title 'Bill Gates...' and it should not be linked.
  4. Open test recommendations feed in new tab
    https://gist.githubusercontent.com/punamdahiya/82869c8196cdd62c8b47e44b16c9b903/raw/8760995d19ced7bdcdff24e9eb9bfad1dd0cc663/test-recs.json
  5. Search for respective item with title 'Bill Gates...' and url should be 'javascript://www.vox.com/future-perfect/2019/2/12/18215534/bill-gates-global-poverty-chart'

NI Brahmini for helping QA the fix

Flags: needinfo?(bnagabandi)
Blocks: 1527701

QA Results: Pass

Tested on :

FF Nightly version : 67.0a1 (2019-02-14)
OS : Mac and Windows 10 Pro

Observations:

  • When I hover on the card having title Bill gates..., I donot see the link for url at bottom of page
  • On opening test recommendations' feed in new tab, there is one result found for url'javascript://www.vox.com/future-perfect/2019/2/12/18215534/bill-gates-global-poverty-chart'` for this item.

QA Recording :
https://www.dropbox.com/s/8ejo6sgoamuz3xw/QA%20Results%20%3A%20bug%20-%201524669.mp4?dl=0

Question :
I'm seeing that from Top Sites section, when I hover on Amazon url doesn't showup. Is this because of search? Do we expect to see url or is this as expected?

Flags: needinfo?(bnagabandi) → needinfo?(pdahiya)

Had offline discussion with Punam and she says "this patch is not touching top sites".

Marking as verified.

Status: RESOLVED → VERIFIED
Flags: needinfo?(pdahiya)

Can you request uplift? Thanks.

Flags: needinfo?(pdahiya)

Comment on attachment 9044253 [details]
Bug 1524669 - Add protocol checking to link urls for pocket new tab

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

Bug 1512725

User impact if declined

Patch implements security feedback and ensures at UI end that links shown on new tab in pocket new tab experiments are using protocol http[s]

Is this code covered by automated tests?

Yes

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

Yes

If yes, steps to reproduce

See https://bugzilla.mozilla.org/show_bug.cgi?id=1524669#c5

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

This patch touches UI components for pocket new tab discovery experiment that's not turned on by default in beta

String changes made/needed

None

Flags: needinfo?(pdahiya)
Attachment #9044253 - Flags: approval-mozilla-beta?

Comment on attachment 9044253 [details]
Bug 1524669 - Add protocol checking to link urls for pocket new tab

Planned work for pocket/new tab. Verified in Nightly.
OK for beta uplift, should land for beta 9.
Landing order: bug 1519879, bug 1525494, bug 1526861, bug 1524669, bug 1527195, bug 1525391, bug 1527347, bug 1525366, bug 1527626, bug 1527397, bug 1518258, bug 1527701, bug 1527370.

Attachment #9044253 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

(In reply to Brahmini Nagabandi from comment #7)

QA Results: Pass

Tested on :

FF Nightly version : 67.0a1 (2019-02-14)
OS : Mac and Windows 10 Pro

Observations:

  • When I hover on the card having title Bill gates..., I donot see the
    link for url at bottom of page
  • On opening test recommendations' feed in new tab, there is one result found for url'javascript://www.vox.com/future-perfect/2019/2/12/18215534/bill-gates-
    global-poverty-chart'` for this item.

QA Recording :
https://www.dropbox.com/s/8ejo6sgoamuz3xw/QA%20Results%20%3A%20bug%20-
%201524669.mp4?dl=0

Question :
I'm seeing that from Top Sites section, when I hover on Amazon url
doesn't showup. Is this because of search? Do we expect to see url or is
this as expected?

Can you please verify this issue on Firefox 66 Beta 9 (https://archive.mozilla.org/pub/firefox/candidates/66.0b9-candidates/build1/)?

Flags: qe-verify+
Flags: needinfo?(bnagabandi)

BETA Testing:

QA Results: Pass

Tested on :

FF Beta version : 66.0b9 (64-bit)
OS : Mac and Windows 10 Pro
Date : Feb 21

Verified on the latest Beta. Works as expected.

Marking as verified.

Flags: needinfo?(bnagabandi)

(In reply to Brahmini Nagabandi from comment #15)

BETA Testing:

QA Results: Pass

Tested on :

FF Beta version : 66.0b9 (64-bit)
OS : Mac and Windows 10 Pro
Date : Feb 21

Verified on the latest Beta. Works as expected.

Marking as verified.

Thanks for verifying this!

Flags: qe-verify+
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.