lib: brand-check Navigator getters to throw ERR_INVALID_THIS

[3zrv]

Fixes: #63513

Add an explicit #field in this brand check to each getter so a proper ERR_INVALID_THIS is thrown, matching browser behavior. This also fixes language, which previously did not error at all on the prototype because it did not touch a private field.

[nodejs-github-bot]

Review requested:

• @nodejs/web-standards

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.33%. Comparing base (2cdea86) to head (a35e81b).
⚠️ Report is 39 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #63601     +/-   ##
=========================================
  Coverage   90.32%   90.33%             
=========================================
  Files         730      732      +2     
  Lines      234669   236513   +1844     
  Branches    43946    44542    +596     
=========================================
+ Hits       211967   213656   +1689     
- Misses      14417    14580    +163     
+ Partials     8285     8277      -8     

Files with missing lines	Coverage Δ
lib/internal/navigator.js	99.39% <100.00%> (+0.02%)	⬆️

... and 81 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[aduh95]

I don't think adding a check is worth it, I think the current behavior is fine actually, the spec requires a TypeError, we're throwing a TypeError (except for .language, indeed that's worth fixing). For reference, Chromium-based browsers throw TypeError: Illegal invocation

[LiviaMedeiros]

+1 on explicit ERR_INVALID_THIS over implicit private field access. Errors from private fields are misleading and not future-proof: it's too easy to break 'implicit throw' unintentionally (in fact, Navigator.prototype.language was initially introduced using private field, too).

[aduh95]

Errors from private fields are misleading and not future-proof: it's too easy to break 'implicit throw' unintentionally

It’s only true if that’s not covered by tests (which I would have expected WPT to cover, but I guess not, or at least not the subset we’re running)

[KhafraDev]

In undici we are lenient with errors too with the expectation that a reviewer would notice if a change similar to the one that changed .language was made.

Since it's not reasonable here, I think the change is fine. Regarding the WPTs, brand checks should be implemented the same across globals in most platforms so the assumption is that if you have it implemented properly in one spec, it's also implemented correctly in the thousands of over browser globals.

[3zrv]

Thanks for the reviews, the test now covers all getters via Object.keys(Navigator.prototype), so either approach is safe against future regressions of the .language kind.

I'm more into keeping the explicit ERR_INVALID_THIS checks for to maintain error message quality, instead of returning a internal field name, ERR_INVALID_THIS('Navigator') produces the same shape of message Chromium and FireFox emit, and matches what the rest of Node's brand-checked APIs throw

[aduh95]

Here's what the error looks like on the browser side:

• Safari: TypeError: The Navigator.language getter can only be used on instances of Navigator
• Firefox: TypeError: 'get language' called on an object that does not implement interface Navigator.
• Chromium: TypeError: Illegal invocation

FWIW I'm in the Chromium camp, I think minimal effort is the appropriate amount of support we should spend on "passing an invalid this" use-case, so I'm not keen on adding a if that will slow down the happy path, ever so slightly – but it's certainly not a hill I want to die on for Navigator

[3zrv]

are we still good to merge this? missing anything? @aduh95 @KhafraDev @LiviaMedeiros

[anonrig]

We explicitly don't do this and rely on private properties for brand validation. This is intentional.

[KhafraDev]

I was previously neutral until I read #63601 (review), which I think is fair. There's already been a regression due to using private properties for brand checks and the more uniform error message is a plus.

[3zrv]

I understand that it's intentional, but the .language regression (introduced as a non-private getter, which silently broke the brand check) is exactly the failure mode that intent didn't survive. The fact that nothing in the class declaration or in CI flagged it is what concerns me about relying on private-field access as the brand-check mechanism.

If you still feel strongly, I can scope the PR down to just fixing .language and rely on the new prototype-iterating test to catch the next regression. But given the reviews, I'd prefer to keep the explicit checks.
@anonrig

[aduh95]

There's already been a regression due to using private properties

Using a custom error code does not protect against that, it's orthogonal. Having coverage protect against that, regardless how we do the brand check.

[3zrv]

Scoped down per the discussion. The PR now only adds an explicit brand check to .language, and leaves the other getters relying on the private-field throw as before.

[KhafraDev]

Using a custom error code does not protect against that

Well yeah, but now there are 2 separate error messages thrown (the brand check for navigator.language and the brand checks for everything else.

The issue linked believed that the v8 error was an "internal [implementation] error", which changing 1 error won't fix either. Ultimately, the v8 error isn't clear and doesn't convey the expected behavior.

, so I'm not keen on adding a if that will slow down the happy path

Tbh we probably shouldn't be implementing web specs if a single if statement is a cause for concern.

[anonrig]

We shouldn't do this. We explicitly avoided doing this because of performance in the past. We went through deprecation cycles multiple times because of this.