feat(fetch#Request): Implements `determineRequestReferrer`

[metcoder95]

Relates to #958

Pendings:

• [ ] Tests

[metcoder95]

Sure thing, I actually have two doubts in here:

1. 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?

2. 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]

Codecov Report

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

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

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

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

[metcoder95]

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

[mcollina]

  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]

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 🙂

[metcoder95]

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

[ronag]

@metcoder95 CI is still failing.

[metcoder95]

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

[metcoder95]

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]

@KhafraDev are we good here?