Fix handling default ports from URL#1491
Merged
Merged
Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR updates Host header construction to preserve explicitly provided ports from raw URLs (including IPv6), adds helpers to extract explicit ports and bracket IPv6 hostnames, adds a unit test verifying default ports are omitted and explicit ports preserved, and adds ChangesHost Header Port Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/client-customHttp-test.js (1)
120-131: ⚡ Quick winExtend this test with explicit default-port and IPv6 cases.
To lock the regression down, add cases for explicitly specified default ports and IPv6 literals.
Proposed test additions
it('should not append default ports to Host header', function () { var client = new httpClient(); var httpsReq = client.buildRequest('https://example.com/service?wsdl', null); assert.equal(httpsReq.headers.Host, 'example.com'); var httpReq = client.buildRequest('http://example.com/service?wsdl', null); assert.equal(httpReq.headers.Host, 'example.com'); var explicitPortReq = client.buildRequest('https://example.com:8443/service?wsdl', null); assert.equal(explicitPortReq.headers.Host, 'example.com:8443'); + + var explicitHttpsDefaultPortReq = client.buildRequest('https://example.com:443/service?wsdl', null); + assert.equal(explicitHttpsDefaultPortReq.headers.Host, 'example.com:443'); + + var explicitHttpDefaultPortReq = client.buildRequest('http://example.com:80/service?wsdl', null); + assert.equal(explicitHttpDefaultPortReq.headers.Host, 'example.com:80'); + + var explicitIpv6PortReq = client.buildRequest('https://[2001:db8::1]:8443/service?wsdl', null); + assert.equal(explicitIpv6PortReq.headers.Host, '[2001:db8::1]:8443'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/client-customHttp-test.js` around lines 120 - 131, Extend the existing test "should not append default ports to Host header" by adding assertions for an explicitly specified default port and IPv6 literals: call client.buildRequest with 'https://example.com:443/service?wsdl' and assert its headers.Host is 'example.com:443' (explicit default port preserved), then test IPv6 addresses like 'http://[::1]/service?wsdl' expecting headers.Host '[::1]' and 'http://[::1]:8080/service?wsdl' expecting headers.Host '[::1]:8080'; reference the httpClient constructor and client.buildRequest in the same test block to add these cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http.ts`:
- Around line 85-87: The Host header construction can produce malformed IPv6
addresses when a port is present; update the logic that sets host (where
getPortFromUrl(rurl) and curl.hostname are used) to bracket IPv6 addresses
before appending the port. Add a small helper (e.g., bracketIPv6(hostname)) that
returns hostname unchanged for IPv4/hostname or already-bracketed values, and
returns `[hostname]` for raw IPv6 literals, then use
`${bracketIPv6(curl.hostname)}:${port}` when port is present; keep the existing
fallback of curl.host || curl.hostname when no explicit port is extracted.
---
Nitpick comments:
In `@test/client-customHttp-test.js`:
- Around line 120-131: Extend the existing test "should not append default ports
to Host header" by adding assertions for an explicitly specified default port
and IPv6 literals: call client.buildRequest with
'https://example.com:443/service?wsdl' and assert its headers.Host is
'example.com:443' (explicit default port preserved), then test IPv6 addresses
like 'http://[::1]/service?wsdl' expecting headers.Host '[::1]' and
'http://[::1]:8080/service?wsdl' expecting headers.Host '[::1]:8080'; reference
the httpClient constructor and client.buildRequest in the same test block to add
these cases.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d891523-f4a4-4eb7-b427-96e0a30632e9
📒 Files selected for processing (2)
src/http.tstest/client-customHttp-test.js
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 2 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/http.ts`:
- Around line 41-43: The getPortFromUrl function currently accepts any all-digit
port string and returns it; change its validation to parse the extracted port
into a number and only return it if it is an integer within the 0–65535 range,
otherwise return an empty string; apply the same numeric-range guard to the
other identical port-extraction branch in the file (the second occurrence around
the Host/hostPort handling) so both explicit-port paths enforce 0..65535.
In `@tsconfig.json`:
- Around line 16-22: The tsconfig change disables TypeScript strictness globally
by setting "strict", "noImplicitAny", "strictNullChecks", "strictFunctionTypes",
"strictBindCallApply", "strictPropertyInitialization", and "noImplicitThis" to
false; revert these toggles to their prior baseline (restore "strict": true or
remove the overridden flags) so the PR does not relax type-safety—locate and
update the entries for "strict", "noImplicitAny", "strictNullChecks",
"strictFunctionTypes", "strictBindCallApply", "strictPropertyInitialization",
and "noImplicitThis" in tsconfig.json to match the repository's strictness
baseline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18a6c95f-abec-45b4-ae09-87c4b4ffb0c1
📒 Files selected for processing (2)
src/http.tstsconfig.json
w666
commented
May 11, 2026
w666
force-pushed
the
fix-1488-fix-url-parse
branch
from
1f25ea6 to
a678779
Compare
w666
force-pushed
the
fix-1488-fix-url-parse
branch
from
a678779 to
2982208
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/http.ts (1)
41-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate extracted port range before using it in Host header.
getPortFromUrlaccepts any numeric string, including invalid ports like99999. The past review comment on this issue remains unaddressed. Add a range check (0..65535) so only valid explicit ports are preserved.🛡️ Suggested guard
function getPortFromUrl(url: string): string { + const isValidPort = (value: string): boolean => { + if (!/^\d+$/.test(value)) return false; + const n = Number(value); + return Number.isInteger(n) && n >= 0 && n <= 65535; + }; + // Capture the authority section (everything between :// and the first /?#) const authorityMatch = /^[a-z][a-z0-9+.-]*:\/\/([^/?#]+)/i.exec(url); if (!authorityMatch) { return ''; } const hostPort = authorityMatch[1]; // IPv6 address: [::1] or [::1]:port if (hostPort.startsWith('[')) { const bracketEnd = hostPort.indexOf(']'); if (bracketEnd !== -1 && hostPort[bracketEnd + 1] === ':') { const port = hostPort.slice(bracketEnd + 2); - return /^\d+$/.test(port) ? port : ''; + return isValidPort(port) ? port : ''; } return ''; } // IPv4 / hostname: host or host:port const colonIndex = hostPort.lastIndexOf(':'); if (colonIndex === -1) { return ''; } const port = hostPort.slice(colonIndex + 1); - return /^\d+$/.test(port) ? port : ''; + return isValidPort(port) ? port : ''; }Also applies to: 52-53
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/http.ts` around lines 41 - 43, The extracted port string returned by getPortFromUrl (where port is computed via hostPort.slice(bracketEnd + 2)) is currently only checked with /^\d+$/.test; change this to parse the numeric value (e.g., Number or parseInt) and enforce 0 <= portNum <= 65535, returning '' for out-of-range or non-numeric values so only valid explicit ports are preserved; apply the same numeric+range check to the other similar extraction branch that currently uses /^\d+$/.test to ensure consistent validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tsconfig.json`:
- Line 12: The tsconfig change replaced skipLibCheck with "types": ["node"],
which prevents TypeScript from loading other ambient type packages (e.g.,
`@types/debug`, `@types/sax`, `@types/formidable`, `@types/whatwg-mimetype`) used across
files like wsdl/index.ts, wsdl/elements.ts, http.ts, client.ts and utils.ts; fix
by either restoring skipLibCheck: true and removing the "types" option, or
update the "types" array to include the missing ambient packages ("node",
"debug", "sax", "formidable", "whatwg-mimetype") so those `@types/`* definitions
are picked up during compilation.
---
Duplicate comments:
In `@src/http.ts`:
- Around line 41-43: The extracted port string returned by getPortFromUrl (where
port is computed via hostPort.slice(bracketEnd + 2)) is currently only checked
with /^\d+$/.test; change this to parse the numeric value (e.g., Number or
parseInt) and enforce 0 <= portNum <= 65535, returning '' for out-of-range or
non-numeric values so only valid explicit ports are preserved; apply the same
numeric+range check to the other similar extraction branch that currently uses
/^\d+$/.test to ensure consistent validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52dbe055-5480-4b76-975f-aa443a382277
📒 Files selected for processing (3)
src/http.tstest/client-customHttp-test.jstsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- test/client-customHttp-test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Close #1488
Summary by CodeRabbit