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

Status: RESOLVED FIXED

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

Description

[Thomas Nguyen (:tnguyen)]

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 add to Loadinfo
- Remote ip
- Url
- referrer

Comment 1

[Thomas Nguyen (:tnguyen)]

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

Comment 2

[Thomas Nguyen (:tnguyen)]

Attachment: WIP - Add more information to redirect chains

MozReview-Commit-ID: 3pv1Z59DgXi

Comment 3

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1351146 - P1 - Add more information to redirect chains

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

Review commit: https://reviewboard.mozilla.org/r/126634/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126634/

Comment 4

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1351146 - P2 - Update test case and usage of nsIRedirectHistoryEntry interface

Review commit: https://reviewboard.mozilla.org/r/126636/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/126636/

Comment 5

[Thomas Nguyen (:tnguyen)]

Patrick, could you please take a look? Or do you know who would help to look at this?
Thanks

Comment 6

[Dragana Damjanovic [:dragana]]

Comment on attachment 8860833 [details]
Bug 1351146 - P1 - Add more information to redirect chains

https://reviewboard.mozilla.org/r/126634/#review137948

Comment 7

[Dragana Damjanovic [:dragana]]

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.

Comment 8

[Thomas Nguyen (:tnguyen)]

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 :)

Comment 9

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8860833 [details]
Bug 1351146 - P1 - Add more information to redirect chains

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126634/diff/1-2/

Comment 10

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8860834 [details]
Bug 1351146 - P2 - Update test case and usage of nsIRedirectHistoryEntry interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126636/diff/1-2/

Comment 11

[Thomas Nguyen (:tnguyen)]

Plz ignore the rebased commit :)

Comment 12

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8860834 [details]
Bug 1351146 - P2 - Update test case and usage of nsIRedirectHistoryEntry interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126636/diff/2-3/

Comment 13

[Dragana Damjanovic [:dragana]]

Comment on attachment 8860834 [details]
Bug 1351146 - P2 - Update test case and usage of nsIRedirectHistoryEntry interface

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

Comment 14

[Thomas Nguyen (:tnguyen)]

Thanks Dragana

Comment 15

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8860833 [details]
Bug 1351146 - P1 - Add more information to redirect chains

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126634/diff/2-3/

Comment 16

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8860834 [details]
Bug 1351146 - P2 - Update test case and usage of nsIRedirectHistoryEntry interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126636/diff/3-4/

Comment 17

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85794cc906a857772b7fd0e75ffa979b3c69879f

Comment 18

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8860833 [details]
Bug 1351146 - P1 - Add more information to redirect chains

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126634/diff/3-4/

Comment 19

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8860834 [details]
Bug 1351146 - P2 - Update test case and usage of nsIRedirectHistoryEntry interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126636/diff/4-5/

Comment 20

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8860834 [details]
Bug 1351146 - P2 - Update test case and usage of nsIRedirectHistoryEntry interface

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126636/diff/5-6/

Comment 21

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a9a918a82bc4d243adc9191c66e07a9ecc3f2964

Comment 22

[Thomas Nguyen (:tnguyen)]

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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Autoland can't push this until it has review in MozReview from someone with Level 3 commit access.

Comment 24

[Thomas Nguyen (:tnguyen)]

Ah, I see, thanks Ryan

Comment 25

[Thomas Nguyen (:tnguyen)]

Patrick, could you please take a look?

Comment 26

[Patrick McManus [: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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

MozReview says it doesn't have sufficient reviews to land. You'll need to ask the people who maintain it then.

Comment 28

[Patrick McManus [:mcmanus]]

comment 27

Comment 29

[Gregory Szorc [: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

Comment 30

[Patrick McManus [:mcmanus]]

comment 29.

Comment 31

[Thomas Nguyen (:tnguyen)]

Attachment: P2 - Update test case and usage of nsIRedirectHistoryEntry interface

MozReview-Commit-ID: s61VV5CLx8

Comment 32

[Thomas Nguyen (:tnguyen)]

Attachment: P1 - Add more information to redirect chains

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

Comment 33

[Thomas Nguyen (:tnguyen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4205709516f264d70931f72440c6813c72709e8

Comment 34

[Thomas Nguyen (:tnguyen)]

Changed to raw file, carry r+ and I am running try to see if it breaks any tests.

Comment 35

[Thomas Nguyen (:tnguyen)]

Attachment: P2 - Update test case and usage of nsIRedirectHistoryEntry interface. r=dragana

MozReview-Commit-ID: s61VV5CLx8

Comment 36

[Thomas Nguyen (:tnguyen)]

Attachment: P1 - Add more information to redirect chains. r=dragana

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

Comment 37

[Pulsebot]

Pushed by [e-mail]:
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

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9db4c2096741
https://hg.mozilla.org/mozilla-central/rev/a0eddece0662