nodejs
feat(esm): expose `User-Agent` header
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 https://github.com/nodejs/node/pull/43852#issuecomment-1185802138, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZJI4X6SL2KPYWLL3DEUTVUGVB5ANCNFSM53WM77PQ . You are receiving this because you are on a team that was mentioned.Message ID: @.***>
Perhaps this default with an environment variable to customize makes sense. Definitely seems a useful feature.
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 https://github.com/nodejs/node/commit/c0f4289c65
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 https://github.com/nodejs/node/blob/3fcf2fe78e0b2af6f67dff767bb440a5265f661a/test/parallel/test-fetch.mjs#L32 and add an assertion for the User-Agent
assert.strictEqual(response.headers["User-Agent"], `Node.js/${process.version}`);
WDYT?
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 https://github.com/nodejs/node/blob/472edc775d683aed2d9ed39ca7cf381f3e7e3ce2/test/es-module/test-http-imports.mjs#L68
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?
It seems the test you made is returning undefined. Was it working on your local environment? Lmk if you need something 🤩
@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.
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.
i think you're misunderstanding. the purpose of my PR is to implement the User-Agent header only when fetching remote modules -- URL imports
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.
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! 🤗
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! 🤗
https://github.com/nodejs/node/issues/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.
