feat(fetch#Request): Implements `determineRequestReferrer` #1236

metcoder95 · 2022-02-15T22:11:56Z

Relates to #958

Pendings:

Tests

ronag

Please paste in the spec test similar to our other implementations.

metcoder95 · 2022-02-19T10:48:29Z

Sure thing, I actually have two doubts in here:

How do we treat step 7 of the step, is not something super clear to me. It quotes:

The user agent MAY alter referrerURL or referrerOrigin at this point to enforce arbitrary policy considerations in the interests of minimizing data leakage. For example, the user agent could strip the URL down to an origin, modify its host, replace it with an empty string, etc.

AFAIU means the agent has full control about the referrerURL, and treats it accordingly to our policies. Then, this step should be totally ignored or just leave it with a note to indicate this part of the spec?

In order to cover all the referrer policy cases is needed to assess if the referrerURL is potentially trustworthy and the same applies for the currentURL. Shall I make a step forward and implement the asses algorithm by the spec? I think is also needed in other parts of Undici when handling the URL

I'll cover your suggestions, add documentation (on code and for implementation) 👍

codecov-commenter · 2022-02-19T11:06:00Z

Codecov Report

Base: 94.89% // Head: 94.72% // Decreases project coverage by -0.17% ⚠️

Coverage data is based on head (e5ff949) compared to base (e5e5b97).
Patch coverage: 77.08% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1236      +/-   ##
==========================================
- Coverage   94.89%   94.72%   -0.18%     
==========================================
  Files          53       53              
  Lines        4803     4850      +47     
==========================================
+ Hits         4558     4594      +36     
- Misses        245      256      +11

Impacted Files	Coverage Δ
lib/fetch/util.js	`83.93% <77.08%> (-2.37%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

metcoder95 · 2022-03-01T22:13:31Z

I'm not sure about those errors, any hint? 🤔

mcollina · 2022-03-03T09:34:58Z

  78) node-fetch
       should not allow cloning a response after its been used:
     TypeError: Cannot read properties of null (reading 'globalObject')
      at determineRequestsReferrer (lib/fetch/util.js:79:117)
      at Fetch.mainFetch (lib/fetch/index.js:152:129)
      at Fetch.fetching (lib/fetch/index.js:135:42)
      at Agent.fetch (lib/fetch/index.js:34:84)
      at fetch (index.js:1:15391)
      at Context.<anonymous> (test/node-fetch/main.js:1391:12)
      at processImmediate (node:internal/timers:466:21)

This seems pretty clear.

metcoder95 · 2022-03-08T12:10:21Z

Thanks for the hint @mcollina, but it seems it was a bad check on the Request environment, as I didn't check for possible undefined from the root object. Now shall be fixed, looking forward for feedback 🙂

lib/fetch/util.js

ronag · 2022-03-08T12:15:17Z

lib/fetch/util.js

+  const policy = request.referrerPolicy
+
+  // Return no-referrer when empty or policy says so
+  if (policy === '' || policy === 'no-referrer') return 'no-referrer'


Suggested change

if (policy === '' || policy === 'no-referrer') return 'no-referrer'

if (policy === '' || policy === 'no-referrer') {

return 'no-referrer'

}

This is not part of spec?

Not explicitly, but if there's no value or the policy explicitly says that there's no-referrer, there's no point in continuing executing the spec algorithm. Any suggestion on how to handle it? 🙂

lib/fetch/util.js

metcoder95 · 2022-04-05T21:06:18Z

Hi @ronag, changes applied and conflicts fixed, let me know if there's anything else to change 👍

ronag · 2022-09-21T08:54:35Z

@metcoder95 CI is still failing.

KhafraDev · 2022-09-21T13:10:15Z

lib/fetch/util.js

+   */
+  if (request.referrer === 'client') {
+    // Not defined in Node but part of the spec
+    if (environment?.globalObject instanceof Window) { // eslint-disable-line


This should be changed to something similar to

undici/lib/fetch/index.js

Line 414 in e5e5b97

request.client?.globalObject?.constructor?.name === 'Window'

Sure, applied in 6ed09c1

this doesn't seem to have been done

True, sorry for that. Change was made but didn't stage it for the commit 😓
Here it is the new one: 681a755

lib/fetch/util.js

metcoder95 · 2022-09-23T07:42:30Z

It seems the testing was failing due to some bad import. Fixed 🙂

KhafraDev

is it possible to add tests that use fetch directly, instead of just testing the utility methods?

KhafraDev · 2022-09-23T15:01:14Z

lib/fetch/util.js

+    let originAsURL
+
+    // If not valid because not semantically correct, return false
+    try { originAsURL = new URL(origin) } catch (e) { return false }


this shouldn't be able to throw

You're right, a bad bad origin cannot happen without being priorly identified. Removed in 2ca0941

metcoder95 · 2022-09-26T07:57:31Z

is it possible to add tests that use fetch directly, instead of just testing the utility methods?

Sure, by just changing to a different value from no-referrer on the Request#referrer when using Fetch should do the work. I'll add some during the day 🙂

ronag · 2022-09-28T05:46:57Z

@KhafraDev are we good here?

* feat: poc of determineRequestReferrer * refactor: apply shortcut * feat(partial): apply switch referrer statement * refactor: add in-code documentation * feat: add check for window * docs: add comments * feat: add check for trustworthy/non-trustworthy urls * docs: add documentation about pottentially trustworthy * feat: expose pottentially trustworthy * test: URL potentially trustworthy * fix: check for possibly undefined * test: initial round * feat: smaller improvements * docs: update in-code docs * lint: ignore line * tests: add more test scenarios * refactor: small improvements * refactor: apply review * tests: adjust testing * refactor: apply PR review * refactor: smaller adjustements

ronag requested changes Feb 16, 2022

View reviewed changes

metcoder95 force-pushed the request/get_request_referrer branch from 626cd04 to 0b653eb Compare February 22, 2022 22:17

metcoder95 marked this pull request as ready for review March 1, 2022 22:13

metcoder95 force-pushed the request/get_request_referrer branch from ab81505 to 3c00c7f Compare March 8, 2022 11:53

ronag reviewed Mar 8, 2022

View reviewed changes

lib/fetch/util.js Show resolved Hide resolved

ronag reviewed Mar 8, 2022

View reviewed changes

lib/fetch/util.js Show resolved Hide resolved

ronag reviewed Mar 8, 2022

View reviewed changes

lib/fetch/util.js Outdated Show resolved Hide resolved

ronag reviewed Mar 8, 2022

View reviewed changes

lib/fetch/util.js Show resolved Hide resolved

metcoder95 force-pushed the request/get_request_referrer branch from 3aca452 to 073ac85 Compare March 23, 2022 22:09

metcoder95 requested a review from ronag March 23, 2022 23:12

metcoder95 force-pushed the request/get_request_referrer branch from e04616c to 9fdeccc Compare April 5, 2022 21:05

ronag force-pushed the main branch from c0f0e0d to ecae314 Compare August 17, 2022 15:09

metcoder95 added 11 commits September 21, 2022 08:54

feat: poc of determineRequestReferrer

8495195

refactor: apply shortcut

8472f00

feat(partial): apply switch referrer statement

10cb439

refactor: add in-code documentation

31cdc42

feat: add check for window

d5add52

docs: add comments

b9f5097

feat: add check for trustworthy/non-trustworthy urls

a8b3106

docs: add documentation about pottentially trustworthy

dbec6e9

feat: expose pottentially trustworthy

6cfb076

test: URL potentially trustworthy

418f119

fix: check for possibly undefined

ff4e6ad

metcoder95 added 6 commits September 21, 2022 08:55

test: initial round

bd54722

feat: smaller improvements

c2bd3eb

docs: update in-code docs

5573e03

lint: ignore line

aaddf0d

tests: add more test scenarios

7bd6182

refactor: small improvements

215dc9a

metcoder95 force-pushed the request/get_request_referrer branch from 9fdeccc to 215dc9a Compare September 21, 2022 06:55

ronag approved these changes Sep 21, 2022

View reviewed changes

ronag requested review from KhafraDev and mcollina September 21, 2022 08:54

KhafraDev requested changes Sep 21, 2022

View reviewed changes

metcoder95 added 2 commits September 23, 2022 09:04

refactor: apply review

6ed09c1

tests: adjust testing

e5ff949

metcoder95 requested review from KhafraDev and removed request for mcollina September 23, 2022 07:05

KhafraDev requested changes Sep 23, 2022

View reviewed changes

metcoder95 added 2 commits September 26, 2022 08:48

refactor: apply PR review

681a755

refactor: smaller adjustements

2ca0941

ronag requested a review from KhafraDev September 28, 2022 05:46

KhafraDev approved these changes Sep 28, 2022

View reviewed changes

ronag merged commit 1b85001 into nodejs:main Sep 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fetch#Request): Implements `determineRequestReferrer` #1236

feat(fetch#Request): Implements `determineRequestReferrer` #1236

metcoder95 commented Feb 15, 2022 •

edited

Loading

ronag left a comment

metcoder95 commented Feb 19, 2022

codecov-commenter commented Feb 19, 2022 •

edited

Loading

metcoder95 commented Mar 1, 2022

mcollina commented Mar 3, 2022

metcoder95 commented Mar 8, 2022

ronag Mar 8, 2022

ronag Mar 8, 2022

metcoder95 Mar 23, 2022

metcoder95 commented Apr 5, 2022

ronag commented Sep 21, 2022

KhafraDev Sep 21, 2022

metcoder95 Sep 23, 2022

KhafraDev Sep 23, 2022

metcoder95 Sep 26, 2022

metcoder95 commented Sep 23, 2022

KhafraDev left a comment

KhafraDev Sep 23, 2022

metcoder95 Sep 26, 2022

metcoder95 commented Sep 26, 2022

ronag commented Sep 28, 2022

feat(fetch#Request): Implements determineRequestReferrer #1236

feat(fetch#Request): Implements determineRequestReferrer #1236

Conversation

metcoder95 commented Feb 15, 2022 • edited Loading

ronag left a comment

Choose a reason for hiding this comment

metcoder95 commented Feb 19, 2022

codecov-commenter commented Feb 19, 2022 • edited Loading

Codecov Report

metcoder95 commented Mar 1, 2022

mcollina commented Mar 3, 2022

metcoder95 commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metcoder95 commented Apr 5, 2022

ronag commented Sep 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metcoder95 commented Sep 23, 2022

KhafraDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

metcoder95 commented Sep 26, 2022

ronag commented Sep 28, 2022

feat(fetch#Request): Implements `determineRequestReferrer` #1236

feat(fetch#Request): Implements `determineRequestReferrer` #1236

metcoder95 commented Feb 15, 2022 •

edited

Loading

codecov-commenter commented Feb 19, 2022 •

edited

Loading