Skip to content

fetch: send 'sec-purpose' header for prefetch requests#3694

Closed
trivikr wants to merge 3 commits into
nodejs:mainfrom
trivikr:header-sec-purpose-prefetch
Closed

fetch: send 'sec-purpose' header for prefetch requests#3694
trivikr wants to merge 3 commits into
nodejs:mainfrom
trivikr:header-sec-purpose-prefetch

Conversation

@trivikr

@trivikr trivikr commented Oct 7, 2024

Copy link
Copy Markdown
Member

This relates to...

Step 14 in WHATWG fetch specification https://fetch.spec.whatwg.org/#http-network-or-cache-fetch, added in whatwg/fetch#1576

Changes

Sends 'sec-purpose' header for prefetch requests as per WHATWG fetch specification

Features

N/A

Bug Fixes

N/A

Breaking Changes and Deprecations

N/A

Status

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a test or enable one in the wpts?

@KhafraDev

Copy link
Copy Markdown
Member

There are no viable WPTs for server implementations

Uzlopak
Uzlopak previously requested changes Oct 7, 2024

@Uzlopak Uzlopak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comments are not arbitrarely made by us, but are direct citations of the spec. Please revert the comments to match the spec.

Comment thread lib/web/fetch/index.js
@Uzlopak

Uzlopak commented Oct 7, 2024

Copy link
Copy Markdown
Contributor

@trivikr
I pushed changes, PTAL

@Uzlopak Uzlopak dismissed their stale review October 7, 2024 22:41

I modified it

Comment thread lib/web/fetch/request.js Outdated
Comment thread lib/web/fetch/request.js

// 4. Set request to a new request whose URL is parsedURL.
request = makeRequest({ urlList: [parsedURL] })
request = makeRequest({ urlList: [parsedURL], ...init })

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@KhafraDev Is this right???

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No.

Comment thread lib/web/fetch/request.js Outdated
Co-authored-by: Trivikram Kamat <16024985+trivikr@users.noreply.github.com>

@KhafraDev KhafraDev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request.initiator will always be null therefore sec-purpose will never be added. It's fine to have the condition that adds it in case the spec ever changes, but most of the other stuff should be removed.

Comment thread lib/web/fetch/request.js
Comment on lines +1063 to +1068
{
key: 'initiator',
converter: webidl.converters.DOMString,
// https://fetch.spec.whatwg.org/#concept-request-initiator
allowedValues: requestInitiator
},

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. A Request's request (internal state) contains an initiator, not RequestInit

Comment thread lib/web/fetch/request.js

// 4. Set request to a new request whose URL is parsedURL.
request = makeRequest({ urlList: [parsedURL] })
request = makeRequest({ urlList: [parsedURL], ...init })

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is wrong

@trivikr trivikr closed this Apr 20, 2025
@trivikr trivikr deleted the header-sec-purpose-prefetch branch April 20, 2025 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants