Closed Bug 1351146 Opened 3 years ago Closed 2 years ago

Store referrer, remote address and url to redirect chain in LoadInfo

Categories

(Core :: DOM: Security, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: tnguyen, Assigned: tnguyen)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files, 5 obsolete files)

Follow bug 1331138, if an unsafe Url has encountered, we should provide a context of how the client arrived at the unsafe entry, particularly in redirect case.
The following info should be added to Loadinfo when a channel is redirected
- Remote ip
- Url
- referrer
Blocks: 1331138
MozReview-Commit-ID: 3pv1Z59DgXi
Assignee: nobody → tnguyen
Attachment #8853335 - Attachment description: Add more information to redirect chains → WIP - Add more information to redirect chains
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Patrick, could you please take a look? Or do you know who would help to look at this?
Thanks
Attachment #8860833 - Flags: review?(mcmanus) → review?(dd.mozilla)
Attachment #8860834 - Flags: review?(mcmanus) → review?(dd.mozilla)
Comment on attachment 8860833 [details]
Bug 1351146 - P1 - Add more information to redirect chains

https://reviewboard.mozilla.org/r/126634/#review137948
Attachment #8860833 - Flags: review?(dd.mozilla) → review+
Comment on attachment 8860834 [details]
Bug 1351146 - P2 - Update test case and usage of nsIRedirectHistoryEntry interface

https://reviewboard.mozilla.org/r/126636/#review137950

::: netwerk/test/mochitests/test_loadinfo_redirectchain.html:85
(Diff revision 1)
>      "http://mochi.test:8888/tests/netwerk/test/mochitests/file_loadinfo_redirectchain.sjs?redir-2",
>      "http://mochi.test:8888/tests/netwerk/test/mochitests/file_loadinfo_redirectchain.sjs?redir-1"
>    ];
>  
> +  // TODO we currently will not change referrer and referrer policy of XHR
> +  // redirect. Any specs to do that?

Please figure out the answer to this question.
Attachment #8860834 - Flags: review?(dd.mozilla)
Attachment #8853335 - Attachment is obsolete: true
Comment on attachment 8860834 [details]
Bug 1351146 - P2 - Update test case and usage of nsIRedirectHistoryEntry interface

https://reviewboard.mozilla.org/r/126636/#review138274

::: netwerk/test/mochitests/test_loadinfo_redirectchain.html:85
(Diff revision 1)
>      "http://mochi.test:8888/tests/netwerk/test/mochitests/file_loadinfo_redirectchain.sjs?redir-2",
>      "http://mochi.test:8888/tests/netwerk/test/mochitests/file_loadinfo_redirectchain.sjs?redir-1"
>    ];
>  
> +  // TODO we currently will not change referrer and referrer policy of XHR
> +  // redirect. Any specs to do that?

Thanks you,
I discussed with Francois about that. Generally, that's ok if all elements in chain have the same referrer. 
 
I just would like to remove this TODO and add a comment :)
Attachment #8860833 - Flags: review?(mcmanus)
Attachment #8860834 - Flags: review?(mcmanus)
Plz ignore the rebased commit :)
Comment on attachment 8860834 [details]
Bug 1351146 - P2 - Update test case and usage of nsIRedirectHistoryEntry interface

https://reviewboard.mozilla.org/r/126636/#review139112
Attachment #8860834 - Flags: review?(dd.mozilla) → review+
Thanks Dragana
Depends on: 1362602
No longer depends on: 1362602
Fixed test failure in Android (remote host should be 10.0.2.2, not 127.0.0.1)
Try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9a918a82bc4d243adc9191c66e07a9ecc3f2964&selectedJob=97274978
Keywords: checkin-needed
Autoland can't push this until it has review in MozReview from someone with Level 3 commit access.
Keywords: checkin-needed
Ah, I see, thanks Ryan
Patrick, could you please take a look?
Attachment #8860833 - Flags: review?(mcmanus)
Attachment #8860834 - Flags: review?(mcmanus)
Attachment #8860833 - Flags: review?(mcmanus)
Attachment #8860834 - Flags: review?(mcmanus)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> Autoland can't push this until it has review in MozReview from someone with
> Level 3 commit access.

dragana has level 3 access: https://bugzilla.mozilla.org/show_bug.cgi?id=1286152
Flags: needinfo?(ryanvm)
MozReview says it doesn't have sufficient reviews to land. You'll need to ask the people who maintain it then.
Flags: needinfo?(ryanvm)
comment 27
Flags: needinfo?(gps)
Since the identity systems of Bugzilla/MozReview and LDAP aren't federated, you need to associate your LDAP account with Bugzilla/MozReview by following the instructions at https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install.html#manually-associating-your-ldap-account-with-mozreview
Flags: needinfo?(gps)
comment 29.
Flags: needinfo?(dd.mozilla)
Attachment #8860833 - Attachment is obsolete: true
Attachment #8860834 - Attachment is obsolete: true
Changed to raw file, carry r+ and I am running try to see if it breaks any tests.
Flags: needinfo?(dd.mozilla)
Attachment #8871560 - Flags: review+
In order to provide more details context of how client arrived at the unsafe
page, particularly in redirect case, we may have to add more information to
redirect chains including:
- referrer (if any)
- remote address.
- URL

We may want to use an idl interface instead of nsIPrincipal to store these
information

MozReview-Commit-ID: 3Uh4r06w60C
Attachment #8871560 - Attachment is obsolete: true
Attachment #8871561 - Attachment is obsolete: true
Attachment #8871564 - Flags: review+
Attachment #8871565 - Flags: review+
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9db4c2096741
P1 - Add more information to redirect chains. r=dragana
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0eddece0662
P2 - Update test case and usage of nsIRedirectHistoryEntry interface. r=dragana
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9db4c2096741
https://hg.mozilla.org/mozilla-central/rev/a0eddece0662
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.