feat(esm): expose `User-Agent` header#43852

Fyko · 2022-07-15T18:31:42Z

nodejs-github-bot · 2022-07-15T18:31:46Z

Review requested:

@nodejs/modules

bmeck · 2022-07-15T18:48:07Z

I'm extremely wary of this being used as a security related issue. I would be more comfortable if this can be disabled or custom configured.

…

On Fri, Jul 15, 2022, 1:31 PM Node.js GitHub Bot ***@***.***> wrote: Review requested: - @nodejs/modules <https://github.com/orgs/nodejs/teams/modules> — Reply to this email directly, view it on GitHub <#43852 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZJI4X6SL2KPYWLL3DEUTVUGVB5ANCNFSM53WM77PQ> . You are receiving this because you are on a team that was mentioned.Message ID: ***@***.***>

guybedford · 2022-07-15T19:42:38Z

Perhaps this default with an environment variable to customize makes sense. Definitely seems a useful feature.

ErickWendel · 2022-08-08T18:22:40Z

hey, @Fyko would you mind adding tests for this feature?

Fyko · 2022-08-08T18:31:03Z

hey, @Fyko would you mind adding tests for this feature?

I would love to! Except... I'm not quite sure how. Could you point me in the right direction? @ErickWendel

Edit: did some searching, this looks relevant c0f4289c65

ErickWendel · 2022-08-08T18:57:10Z

hey, @Fyko would you mind adding tests for this feature?
I would love to! Except... I'm not quite sure how. Could you point me in the right direction? @ErickWendel
Edit: did some searching, this looks relevant c0f4289c65

Sure. I think it'd be easier if you go to this file

node/test/parallel/test-fetch.mjs

Line 32 in 3fcf2fe

assert.strictEqual(response.statusText,'OK');

and add an assertion for the User-Agent

assert.strictEqual(response.headers["User-Agent"],`Node.js/${process.version}`);

WDYT?

Fyko · 2022-08-08T19:00:58Z

Not quite sure if that would work considering the UA logic was added to fetch_module.js.

I found the file https://github.com/nodejs/node/blob/main/test/es-module/test-http-imports.mjs and considered adding:

if(_req.getHeaders()['user-agent']!==`Node.js/${process.version}`){res.writeHead(400);res.end('bad user-agent');return;}

right after the createServer call

node/test/es-module/test-http-imports.mjs

Line 68 in 472edc7

constserver=createServer(function(_req,res){

ErickWendel · 2022-08-08T19:13:05Z

I suggested going to the tests-fetch file as it ensures what are the default values returned from the fetch response.

As your change will affect fetch.get adding by default the User Agent flag I think this might be the best place to check it out.

The file you mentioned has another goal. There you'd test the behavior when importing a module from an URL such as import module from "HTTP://domain/mymodule"

Not quite sure if that would work considering the UA logic was added to fetch_module.js.

Even though the logic was added to fetch_module, the test-fetch actually uses it (but notice that fetch is a global object that's why it's not being imported there)

WDYT?

ErickWendel · 2022-08-09T14:06:24Z

It seems the test you made is returning undefined. Was it working on your local environment? Lmk if you need something 🤩

ErickWendel · 2022-08-23T18:14:47Z

Hey @Fyko do you need any help on this?

Fyko · 2022-08-25T17:07:00Z

I'll get around to this tomorrow. Sorry for the delay!

jasnell · 2022-08-28T14:26:10Z

lib/internal/modules/esm/fetch_module.js

 agent: HTTPSAgent,
+headers: {
+'User-Agent': `Node.js/${process.version}`,
+},


I'm -1 on always setting this. It should be optional and default turned off.

How do other server-side runtimes behave?

My inspiration for this was how Deno sets the User-Agent header when getting packages. I don't quite understand why so many people are against this -- many very popular packages and utilities do this. Curl sets a header. So does node-fetch. List goes on. And it's completely nonsensical to use some type of env var to set the header.

“many people do x” isn’t ever a justification; many people do many ill-advised things.
The advantages i can see of setting it by default is easier debugging on the other end - the disadvantages i see is that a server could identify, track, and treat differently a request coming from node vs from other sources.

Deno sets the User-Agent header when getting packages. I don’t quite understand why so many people are against this – many very popular packages and utilities do this. Curl sets a header. So does node-fetch.
If most non-browser clients set a User-Agent header, then it’s a de facto standard and we should do the same. It doesn’t matter if some of us disagree with the wisdom of the practice; the point of adding APIs like fetch is to be more standards-compliant and similar to other clients. If users want network requests without User-Agent, they can just override the default or use some other API.

Understanding the concerns about default behavior is essential. While many popular utilities set the User-Agent header by default, there are legitimate concerns regarding potential server-side differentiation based on this header. However, aligning with common practices across other clients could indeed enhance standards compliance.
Perhaps we can consider an optional flag for users to toggle the inclusion of the User-Agent header, offering both flexibility and adherence to commonly accepted practices. This could cater to different user preferences without compromising on standards.

Please god not yet another flag. But yes some way to customize this behavior would be good.

Fyko · 2022-09-15T21:24:45Z

@jasnell @guybedford @bmeck

this pr has my attention again, and im adamant about getting this done. if not a default User-Agent, what would you suggest? env var? what would you name it? NODE_MODULE_FETCH_UA=node/16.x.x? i feel it would be better to be a toggle.

GeoffreyBooth · 2022-09-15T22:02:03Z

if not a default User-Agent

I think we should set it by default, and users could override it on a per-fetch basis:

fetch(url,{headers: {'User-Agent': 'whatever'}})

If we need a global override, that could happen later.

Fyko · 2022-09-15T22:07:12Z

i think you're misunderstanding.
the purpose of my PR is to implement the User-Agent header only when fetching remote modules -- URL imports

GeoffreyBooth · 2022-09-15T22:19:33Z

i think you’re misunderstanding. the purpose of my PR is to implement the User-Agent header only when fetching remote modules – URL imports

You mean --experimental-network-imports? I think it should apply to all fetch calls, both as a result of network imports and user code. I don’t think Node’s calls need a way to be customized.

Fyko · 2022-09-15T22:46:22Z

Got it, just making sure we're on the same page. I'm not exactly sure what to do here -- I've got conflicting opinions from different members. I'm going to fix my tests so CI passes and we'll see what happens! 🤗

GeoffreyBooth · 2022-09-15T23:15:23Z

Got it, just making sure we’re on the same page. I’m not exactly sure what to do here – I’ve got conflicting opinions from different members. I’m going to fix my tests so CI passes and we’ll see what happens! 🤗

#43851 doesn’t mention this applying to only --experimental-network-imports, so I think maybe that issue could be used to debate what we want the intended behavior should be, and this PR discussion could be restricted to technical discussion of the implementation of whatever design we settle on. We shouldn’t land something until we have a decision regarding what we want. If we don’t reach consensus in discussion on that thread it can go to the TSC for a vote.

Also, please always rebase on main rather than merging main into your branch.

The issue was that on L133, the headers object being passed through was overwriting the user-agent header. I've reformatted the `opts` spread so the only way to over-write the header is to explicitily define it

renhiyama · 2024-01-07T12:56:50Z

Any updates on this?

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Jul 15, 2022

Fyko force-pushed the feat/esm-ua branch from d0c8223 to 3fcf2feCompare July 15, 2022 18:35

aduh95 added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 17, 2022

jasnell reviewed Aug 28, 2022
View reviewed changes

Fyko force-pushed the feat/esm-ua branch from 233ddb8 to 8204bc5Compare September 15, 2022 23:26

Fyko added 2 commits September 15, 2022 17:43

esm: expose User-Agent header
bfc28e9

chore: fix test
b436709

Fyko force-pushed the feat/esm-ua branch from 8204bc5 to b436709Compare September 15, 2022 23:45

Fyko added 3 commits September 18, 2022 12:07

test: edit casing for header get
cef0770

refactor: standardize lowercase header
75c7015

test: finally fix tests
dc62164
The issue was that on L133, the headers object being passed through was overwriting the user-agent header. I've reformatted the `opts` spread so the only way to over-write the header is to explicitily define it

Trott force-pushed the main branch from 2d76238 to ca3ed36Compare November 12, 2022 01:49

bnoordhuis mentioned this pull request Mar 18, 2023
esm(network imports): expose User-Agent header #43851
Closed

bnoordhuis mentioned this pull request Jul 10, 2023
Change the default user agent of nodejs #48711
Closed

balazsorban44 mentioned this pull request Oct 4, 2023
Change default "user-agent" from undicinodejs/undici#2306
Closed

Uh oh!

feat(esm): expose User-Agent header#43852

Are you sure you want to change the base?

feat(esm): expose User-Agent header #43852

Conversation

Fyko commented Jul 15, 2022

Uh oh!

nodejs-github-bot commented Jul 15, 2022

Uh oh!

bmeck commented Jul 15, 2022 via email

Uh oh!

guybedford commented Jul 15, 2022

Uh oh!

ErickWendel commented Aug 8, 2022

Uh oh!

Fyko commented Aug 8, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErickWendel commented Aug 8, 2022

Uh oh!

Fyko commented Aug 8, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErickWendel commented Aug 8, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErickWendel commented Aug 9, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ErickWendel commented Aug 23, 2022

Uh oh!

Fyko commented Aug 25, 2022

Uh oh!

jasnellAug 28, 2022

Choose a reason for hiding this comment

Uh oh!

GeoffreyBoothAug 31, 2022

Choose a reason for hiding this comment

Uh oh!

FykoSep 7, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharbSep 7, 2022

Choose a reason for hiding this comment

Uh oh!

GeoffreyBoothSep 7, 2022

Choose a reason for hiding this comment

Uh oh!

MifrillNov 27, 2023

Choose a reason for hiding this comment

Uh oh!

GeoffreyBoothNov 27, 2023

Choose a reason for hiding this comment

Uh oh!

Fyko commented Sep 15, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GeoffreyBooth commented Sep 15, 2022

Uh oh!

Fyko commented Sep 15, 2022

Uh oh!

GeoffreyBooth commented Sep 15, 2022

Uh oh!

Fyko commented Sep 15, 2022

Uh oh!

GeoffreyBooth commented Sep 15, 2022• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

renhiyama commented Jan 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

feat(esm): expose `User-Agent` header#43852

feat(esm): expose `User-Agent` header #43852

Fyko commented Aug 8, 2022•
edited
Loading

Fyko commented Aug 8, 2022•
edited
Loading

ErickWendel commented Aug 8, 2022•
edited
Loading

ErickWendel commented Aug 9, 2022•
edited
Loading

FykoSep 7, 2022•
edited
Loading

Fyko commented Sep 15, 2022•
edited
Loading

GeoffreyBooth commented Sep 15, 2022•
edited
Loading