G
YouTube TikTok Games
Skip to content

fix(node): reject port 0 in connection string - #4846

Open
Dageus wants to merge 3 commits into
apache:masterfrom
Dageus:fix/node-sdk-port-validation
Open

Dageus wants to merge 3 commits into
apache:masterfrom
Dageus:fix/node-sdk-port-validation

Conversation

@Dageus

@Dageus Dageus commented Nov 10, 2026

Copy link
Copy Markdown

Which issue does this PR address?

Closes #4845

Rationale

The Rust SDK rejects port 0 when parsing a connection string, but the Node SDK accepted it and only failed later on connect. Both SDKs should reject the same inputs at parse time.

What changed?

Previously, the Node SDK's parseConnectionString accepted port 0 , which is a reserved port. This PR adds a small check for port == 0, blocking that path.

The malformed-input test now covers :0 and :65536 (replacing :70000, so the upper bound is tested exactly), and a new test checks that :1 and :65535 still parse, although the happy path test may be unnecessary and am happy to remove it if it's overkill.

Local Execution

  • Passed: npm run test:unit and npm run lint in foreign/node (Node 24)
  • Pre-commit hooks ran: prek run passed (license headers, typos, node eslint, whitespace/newline, binary artifacts, skills)

AI Usage

Claude was used to make my PR message clearer.

@codecov

codecov Bot commented Nov 10, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.34%. Comparing base (167dcd1) to head (548ba05).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #4846   +/-   ##
=========================================
  Coverage     91.34%   91.34%           
  Complexity     3875     3875           
=========================================
  Files          1451     1451           
  Lines        334425   334425           
  Branches     268282   268282           
=========================================
+ Hits         305495   305496    +1     
+ Misses        22716    22715    -1     
  Partials       6214     6214           
Components Coverage Δ
Rust Core 92.99% <ø> (ø)
Java SDK 79.36% <ø> (ø)
C# SDK 84.48% <ø> (ø)
Python SDK 86.52% <ø> (ø)
PHP SDK 92.23% <ø> (ø)
Node SDK 96.76% <100.00%> (+<0.01%) ⬆️
Go SDK 71.80% <ø> (ø)
C++ SDK 86.84% <ø> (ø)
Files with missing lines Coverage Δ
...oreign/node/src/client/client.connection-string.ts 99.17% <100.00%> (ø)

... and 1 file 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.

@github-actions

Copy link
Copy Markdown

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

If AI assistance was used to create this PR, you are required to run the team-review skill before opening it.

Slash commands (own line, regular comment) move it around the queue:

  • /ready - back to S-waiting-on-review after addressing feedback
  • /author - flip to S-waiting-on-author while you finish changes
  • /request-review @user-or-team - request a reviewer
  • /pin - exempt the PR from the stale bot, /unpin to undo

See CONTRIBUTING.md for details.

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label Nov 10, 2026
@justinmclean

Copy link
Copy Markdown
Member

Thanks @Dageus, and welcome. Matching the Rust SDK, which already rejects port 0, is a good fix, and testing 65536 instead of 70000 pins the upper bound exactly. The happy path test for 1 and 65535 is worth keeping.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review PR is waiting on a reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Node SDK: connection string accepts port 0

2 participants