Opened 4 years ago

Closed 4 years ago

#16448 closed defect (fixed)

isolate favicon requests and caching by URL bar domain

Reported by: arthuredelstein Owned by: boklm
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: ff38-esr, TorBrowserTeam201507R, tbb-testcase
Cc: mikeperry, gk, boklm, arthuredelstein Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Favicon images are requested by the tab element inside the main chrome window, so we need a patch to make sure they are isolated by the first party domain of the loading document. This ticket has been spun off from #13670.

Child Tickets

Change History (7)

comment:1 Changed 4 years ago by arthuredelstein

Status: newneeds_review

Here's my latest patch for review (against FF38-ESR):
https://github.com/arthuredelstein/tor-browser/commits/13670_favicons

Please note that this needs to by applied after the #13670 OCSP patch.

Last edited 4 years ago by arthuredelstein (previous) (diff)

comment:2 Changed 4 years ago by mikeperry

This patch looks OK to me. We should make sure to add tests both for default favicons, as well as favicons specified in HTML (as the latter may change if the OwnerDoc is chrome).

comment:3 Changed 4 years ago by mikeperry

Keywords: TorBrowserTeam201507R added; TorBrowserTeam201506R removed

Transfer review tickets to next month.

comment:4 Changed 4 years ago by mikeperry

Keywords: tbb-testcase added

comment:5 Changed 4 years ago by mikeperry

Cc: boklm added
Status: needs_reviewneeds_revision

arthur - Can you and boklm work out who will write the test cases for this? I think we should cover both HTML and default favicons in the tests. Not sure if that means it requires a full integration test or if it can still be done with a unit test... If we do decide we need a heavyweight integration test, perhaps we should cover OCSP too?

comment:6 Changed 4 years ago by boklm

Cc: arthuredelstein added
Owner: changed from tbb-team to boklm
Status: needs_revisionassigned

I can add that (the test) to my TODO list.

comment:7 Changed 4 years ago by mikeperry

Resolution: fixed
Status: assignedclosed

I filed #16728 for the test cases. Closing this ticket.

Note: See TracTickets for help on using tickets.