Skip to content

lib: add navigator.userAgent - #50200

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:add-navigator-useragent
Oct 20, 2023
Merged

lib: add navigator.userAgent#50200
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
anonrig:add-navigator-useragent

Conversation

@anonrig

@anonriganonrig commented Oct 16, 2023

Copy link
Copy Markdown
Member

Follow-up for #47769

cc @nodejs/tsc

@anonrig
anonrigforce-pushed the add-navigator-useragent branch from 1704a23 to 218c1a1CompareOctober 16, 2023 17:32
@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Oct 16, 2023
@anonriganonrig added dont-land-on-v18.x notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. labels Oct 16, 2023
@github-actions

Copy link
Copy Markdown
Contributor

The notable-changePRs with changes that should be highlighted in changelogs. label has been added by @anonrig.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment.

@aduh95

Copy link
Copy Markdown
Contributor

Two questions:

  • Why is that semver-major?
  • How is it relevant for Node.js?

Comment threadtest/parallel/test-navigator.js Outdated
@anonrig

Copy link
Copy Markdown
MemberAuthor

Two questions:

  • Why is that semver-major?

Some packages might depend on navigator.userAgent to detect if it's not Node.js or not. I don't want to introduce breaking change.

  • How is it relevant for Node.js?

navigator.userAgent is included in WINTERCG Minimum Common Web Platform API - https://common-min-api.proposal.wintercg.org

@meyfa

Copy link
Copy Markdown
Contributor

The default value contained in this PR is currently different from the value used in native fetch, see: nodejs/undici#2306

There is also some discussion about not including the version number that may be of relevance here.

I think if Node.js exposes a user agent in multiple places, it needs to be the same everywhere.

@KhafraDev

Copy link
Copy Markdown
Member

the version should be removed, so either "node.js" or "node". Undici uses "node" so that would make our lives easier (the fetch user-agent and navigator user-agent seem like they should be equal, although I'm not entirely sure).

@anonrig

Copy link
Copy Markdown
MemberAuthor

the version should be removed, so either "node.js" or "node". Undici uses "node" so that would make our lives easier (the fetch user-agent and navigator user-agent seem like they should be equal, although I'm not entirely sure).

Referencing WINTERCG, it's recommended to have a product version with the user agent. I'm more in favor of having the version:

User-Agent = product *( RWS ( product / comment ) )
product = token ["/" product-version]
product-version = token

@anonrig
anonrigforce-pushed the add-navigator-useragent branch from 218c1a1 to b92833eCompareOctober 16, 2023 18:49
@KhafraDev

Copy link
Copy Markdown
Member

There are legitimate security concerns by adding the node version, maybe something like what Chrome is doing would be acceptable? If you don't want to click that they're replacing minor version numbers with zeros (ie. v20.1.1 -> v20.0.0). Is there any discussion about why a version number is required?

Comment threaddoc/api/globals.md Outdated
Comment threaddoc/api/globals.md Outdated
@UzlopakUzlopak mentioned this pull request Oct 17, 2023
@tniessentniessen added the experimental Issues and PRs related to experimental features. label Oct 19, 2023
@Uzlopak

Copy link
Copy Markdown
Contributor

I thought about the version.

I am +1 for adding the version.

First of all, the userAgent is in browsers used for requests. We dont expose the version via a http request nor via a http response. Browsers expose this information with their version via request and they dont consider it as a security issue. If it would be a security issue chrome, safari and co would have removed the version information long time ago.

Also comparing to deno and bun, they also expose the version.

So I am 100% hoping that the version gets exposed.

@GeoffreyBooth

Copy link
Copy Markdown
Member

Browsers expose this information with their version via request and they dont consider it as a security issue.

Actually Chrome only exposes the major version; they consider exposing the minor/patch versions to be a security concern. See the Chrome/118.0.0.0 in the example above. All of the Chrome 118.x versions return 118.0.0.0 as the version.

But we can do the same, it’s better than not including a version.

@KhafraDev

Copy link
Copy Markdown
Member

Chrome did limit it, from the link I posted above:

From Chrome 101 we replaced the minor version number with zeros, e.g. Chrome/101.3.2.1 became Chrome/101.0.0.0.

Firefox also limits some information (windows & macOS versions are capped at 10 and 10.15 respectively, and some architectures don't include any information in their user-agent header). (Source) I'd assume they have plans to limit it further.

@KhafraDev

Copy link
Copy Markdown
Member

But we can do the same, it’s better than not including a version.

Yep, this was my proposed alternative.

@anonrig

Copy link
Copy Markdown
MemberAuthor

I'll update the PR with the proposed change.

@GeoffreyBooth

Copy link
Copy Markdown
Member

This isn’t semver-major, the semver-major change was adding navigator in the first place. Now that navigator exists we should add all the properties people expect on it.

@GeoffreyBoothGeoffreyBooth added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-major PRs that contain breaking changes and should be released in the next major version. dont-land-on-v21.x labels Oct 20, 2023
@anonriganonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 20, 2023
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 20, 2023
Comment threadlib/internal/navigator.js
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@UzlopakUzlopak 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.

RSLGTM

@styfle

Copy link
Copy Markdown
Member

the fetch user-agent and navigator user-agent seem like they should be equal

💯 agree!

Also something to consider: it looks like there is existing code out there that looks for Node.js string matching.

For example:

@mcollinamcollina 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.

lgtm

@GeoffreyBoothGeoffreyBooth added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 20, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 20, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 05a7810 into nodejs:mainOct 20, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 05a7810

@Ethan-Arrowood

Copy link
Copy Markdown
Contributor

I really think the user-agent keys should match #50200 (comment)

Can we update it ?

@KhafraDev

Copy link
Copy Markdown
Member

honestly i'm starting to question the benefit of having navigator whatsoever. I think there should be an agreement on if node should even have it before it's changed again.

targos pushed a commit that referenced this pull request Oct 23, 2023
PR-URL: #50200
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
targos added a commit that referenced this pull request Oct 23, 2023
Notable changes:
doc:
* add H4ad to collaborators (Vinícius Lourenço) #50217
esm:
* (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096
fs:
* (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095
lib:
* (SEMVER-MINOR) add `navigator.userAgent` (Yagiz Nizipli) #50200
stream:
* (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187
* call helper function from push and unshift (Raz Luvaton) #50173
PR-URL: #50335
@targostargos mentioned this pull request Oct 23, 2023
targos added a commit that referenced this pull request Oct 24, 2023
Notable changes:
doc:
* add H4ad to collaborators (Vinícius Lourenço) #50217
esm:
* (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096
fs:
* (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095
lib:
* (SEMVER-MINOR) add `navigator.userAgent` (Yagiz Nizipli) #50200
stream:
* (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187
* call helper function from push and unshift (Raz Luvaton) #50173
PR-URL: #50335
targos added a commit that referenced this pull request Oct 24, 2023
Notable changes:
doc:
* add H4ad to collaborators (Vinícius Lourenço) #50217
esm:
* (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096
fs:
* (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095
lib:
* (SEMVER-MINOR) add `navigator.userAgent` (Yagiz Nizipli) #50200
stream:
* (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187
* call helper function from push and unshift (Raz Luvaton) #50173
PR-URL: #50335
targos added a commit that referenced this pull request Oct 24, 2023
Notable changes:
doc:
* add H4ad to collaborators (Vinícius Lourenço) #50217
esm:
* (SEMVER-MINOR) detect ESM syntax in ambiguous JavaScript (Geoffrey Booth) #50096
fs:
* (SEMVER-MINOR) add flush option to appendFile() functions (Colin Ihrig) #50095
lib:
* (SEMVER-MINOR) add `navigator.userAgent` (Yagiz Nizipli) #50200
stream:
* (SEMVER-MINOR) allow pass stream class to `stream.compose` (Alex Yang) #50187
* call helper function from push and unshift (Raz Luvaton) #50173
PR-URL: #50335
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.experimentalIssues and PRs related to experimental features.needs-ciPRs that need a full CI run.notable-changePRs with changes that should be highlighted in changelogs.semver-minorPRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

12 participants

@anonrig@aduh95@meyfa@KhafraDev@Uzlopak@GeoffreyBooth@nodejs-github-bot@styfle@Ethan-Arrowood@mcollina@H4ad@tniessen