817531 - Wrong Referer is sent for table background images

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[mkrinke]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0
Build ID: 20121120062532

Steps to reproduce:

Visited http://www.fotocommunity.com/contest


Actual results:

Background images for the tables (Different photo contests listed) are not beeing loaded. 


Expected results:

Requests for those images are beeing made and result in a 403 since a referer check is made for those requests and the referer beeing sent along with those requests is the request url (for the image asset) itself, not the url for the website including those images.

This seems to be introduced in FF 17, we verified it on Mac, Linux and Windows.
In FF 16 it still worked.

Comment 1

[mkrinke]

just wanted to add example html in question:

<table width="100%" border="0" background="http://cdn.fotocommunity.com/photos/27941525.jpg" height="120" cellspacing="5" cellpadding="0">

Comment 2

[Matthias Versen [:Matti]]

>Background images for the tables (Different photo contests listed) are not beeing 
>loaded. 

Either I'm missing something our your steps to reproduce are incomplete.
The page http://www.fotocommunity.com/contest looks identical for me in IE9, FF17 and Seamonkey trunk.
Please include steps to reproduce, step by step and attach a screenshot
Is it possible that I have to be logged in ?

Comment 3

[Matthias Versen [:Matti]]

Attachment: Screenshot with Firefox17

Comment 4

[mkrinke]

Matthias,

sorry, but it seems we already deployed a fix for this on our side using css.

I put up some example html: http://www.fotocommunity.net/ff-bug-817531.html

In Firebug you can validate the the Referer beeing sent with the request for http://cdn.fotocommunity.com/photos/27941525.jpg is "http://cdn.fotocommunity.com/photos/27941525.jpg", not the URL to the main request.

Thanks,
Moritz

Comment 5

[Matthias Versen [:Matti]]

Firefox17: Referer: http://cdn.fotocommunity.com/photos/27941525.jpg
Firefox10ESR: Referer: http://www.fotocommunity.net/ff-bug-817531.html

We have 2 regressions here.
The first regression is that we stop loading the image over the net

Last good nightly: 2012-08-15
First bad nightly: 2012-08-16

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=86ee4deea55b&tochan
ge=50e4ff05741e

I suspect bug 697230

Some builds later we start to load the image again but with the wrong referer header
Last good nightly: 2012-08-24
First bad nightly: 2012-08-25

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1c0ac073dc65&tochan
ge=f077de66e52d

I suspect bug 783162

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, the wrong referrer thing is definitely from bug 783162.  This part in nsGenericHTMLElement::ParseBackgroundAttribute:

+    mozilla::css::URLValue *url =
+      new mozilla::css::URLValue(buffer, baseURI, uri, NodePrincipal());

should be:

    mozilla::css::URLValue *url =
      new mozilla::css::URLValue(uri, buffer, doc->GetDocumentURI(), NodePrincipal());

I think.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix the referrer header for background image loads.

Comment 8

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d26382bc6411

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 688325 [details] [diff] [review]
Fix the referrer header for background image loads.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 783162.
User impact if declined: Referrer for background images via HTML "background"
   attributes is the image itself, not the document, possibly breaking some sites.
Testing completed (on m-c, etc.): Passes automated test.
Risk to taking this patch (and alternatives if risky): Risk should be very low.
String or UUID changes made by this patch: None

Comment 10

[Ed Morley [:emorley]]

Backed out for:
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.232:30278/tests/layout/reftests/backgrounds/background-referrer.html | image comparison (==), max difference: 255, number of differing pixels: 800000
https://tbpl.mozilla.org/php/getParsedLog.php?id=17684273&tree=Mozilla-Inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/dee7d08baf04

Comment 11

[Boris Zbarsky [:bzbarsky]]

I'm going to reland with the test disabled on android, for now, and work on debugging why the test fails there.

Comment 12

[Boris Zbarsky [:bzbarsky]]

And it's a bit worrisome that I never got bugmail for comment 10.  :(

Comment 13

[Boris Zbarsky [:bzbarsky]]

Relanded as https://hg.mozilla.org/integration/mozilla-inbound/rev/a381a84d7af2

Comment 14

[Boris Zbarsky [:bzbarsky]]

Looks like Android uses a different hostname.  Fixed the test to deal with that and reenabled it on Android: https://hg.mozilla.org/integration/mozilla-inbound/rev/732f04efe8f5

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a381a84d7af2
https://hg.mozilla.org/mozilla-central/rev/732f04efe8f5

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 688325 [details] [diff] [review]
Fix the referrer header for background image loads.

low risk regression fix, approving.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/09cd0914f357
https://hg.mozilla.org/releases/mozilla-aurora/rev/9418dbcb8336

https://hg.mozilla.org/releases/mozilla-beta/rev/a918092d9d51
https://hg.mozilla.org/releases/mozilla-beta/rev/272e116f4ee7

Comment 18

[Loic]

Tracking for QA.

Comment 19

[Mihai Morar, (:MihaiMorar)]

I confirm the fix is verified on MacOS 10.8, Windows 7 x64 and Ubuntu 12.04 on FF 20RC:

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0