feat: parse port from onConnect handler #45
Conversation
- sets the _originalUrl_ attribute on ctx - updates logger code to send port in case of local and non-http port
|
|
WalkthroughAdds Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/proxy-middleware/middlewares/logger_middleware.js (1)
79-90: Fix getHost() implementation to include port for all non-standard ports, not just localhost.The change correctly uses
getHost(ctx)in HAR generation, but this only partially solves the problem. ThegetHost()function atsrc/components/proxy-middleware/rule_action_processor/utils.js:48-55has incomplete logic:
- Returns with port for localhost-like hosts (localhost, 127.0.0.1, 0.0.0.0) when port exists
- Returns without port for all other hosts, regardless of whether the port is non-standard
This causes:
- HAR URLs to be invalid for non-localhost hosts with non-standard ports (e.g.,
example.com:8080becomes justexample.com)- curl generation via HTTPSnippet to lose port information for those scenarios
- Network interceptor UI to display incomplete host information
Update
getHost()to include port information for all non-standard ports (not just localhost), or document this as a known limitation.
🧹 Nitpick comments (1)
src/components/proxy-middleware/rule_action_processor/utils.js (1)
44-46: Consider adding IPv6 localhost support.The function currently checks for IPv4 localhost variants but doesn't include
::1(IPv6 localhost). Depending on your use case, this might cause ports not to display for IPv6 localhost requests.Apply this diff to add IPv6 support:
function isLocalHost(host) { - return host === "localhost" || host === "127.0.0.1" || host === "0.0.0.0"; + return host === "localhost" || host === "127.0.0.1" || host === "0.0.0.0" || host === "::1"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/proxy-middleware/middlewares/logger_middleware.js(3 hunks)src/components/proxy-middleware/rule_action_processor/utils.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/proxy-middleware/middlewares/logger_middleware.js (1)
src/components/proxy-middleware/rule_action_processor/utils.js (2)
getHost(48-55)getHost(48-55)
🔇 Additional comments (2)
src/components/proxy-middleware/middlewares/logger_middleware.js (2)
3-3: LGTM!The import statement correctly adds
getHostfrom the utils module.
24-24: Improve error handling and investigate HTTPSnippet conversion failure for localhost requests.The code correctly uses
getHost(ctx)(line 83) which includes the port for localhost URLs. However, the comment indicates HTTPSnippet conversion consistently fails despite this. The error is silently caught and only logged to console without capturing details.
- Enhance error logging to capture and preserve the actual error message instead of just stringifying it
- Investigate why HTTPSnippet fails even with properly formatted localhost URLs (e.g.,
http://localhost:8080/path)- Consider whether this is a known HTTPSnippet limitation with localhost or if there's a malformed HAR structure issue
- Consider whether the generated curl should gracefully degrade or if the error should be surfaced differently
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/proxy-middleware/rule_action_processor/utils.js (1)
50-57: Incomplete port display logic - non-localhost hosts with non-standard ports won't be handled.The current implementation only appends the port for localhost-like hosts. However, the PR description mentions "local and non-HTTP ports," suggesting ports should be displayed for any host with non-standard ports (e.g.,
example.com:8080,192.168.1.1:3000).Additionally, this logic doesn't handle these edge cases:
- Duplicate ports: If
ctx.rq.final_request.hostalready contains a port (e.g.,"localhost:3000"), this would result in"localhost:3000:3000".- Standard ports: Ports 80 (HTTP) and 443 (HTTPS) typically shouldn't be displayed.
- Port validation: No check if
portis undefined or already included in the host.Consider refactoring to handle all hosts with non-standard ports:
export const getHost = (ctx) => { const finalHost = ctx.rq.final_request.host; - if(isLocalHost(finalHost) && ctx.rq.final_request.port) { - return `${finalHost}:${ctx.rq.final_request.port}`; - } else { - return finalHost - } + const port = ctx.rq.final_request.port; + const protocol = ctx.isSSL ? "https" : "http"; + const standardPort = protocol === "https" ? 443 : 80; + + // Strip existing port from host to avoid duplicates + const hostWithoutPort = finalHost.split(':')[0]; + + // Only append port if it's non-standard and present + if (port && port !== standardPort) { + return `${hostWithoutPort}:${port}`; + } + return hostWithoutPort; }
🧹 Nitpick comments (1)
src/components/proxy-middleware/rule_action_processor/utils.js (1)
46-48: Add IPv6 localhost support.The
isLocalHostfunction only checks for IPv4 localhost variations but misses IPv6 localhost (::1). This could cause ports to not display for IPv6 localhost requests.Apply this diff to add IPv6 support:
function isLocalHost(host) { - return host === "localhost" || host === "127.0.0.1" || host === "0.0.0.0"; + return host === "localhost" || host === "127.0.0.1" || host === "0.0.0.0" || host === "::1"; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/proxy-middleware/middlewares/logger_middleware.js(3 hunks)src/components/proxy-middleware/rule_action_processor/utils.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/proxy-middleware/middlewares/logger_middleware.js (1)
src/components/proxy-middleware/rule_action_processor/utils.js (2)
getHost(50-57)getHost(50-57)
🪛 Biome (2.1.2)
src/components/proxy-middleware/rule_action_processor/utils.js
[error] 49-57: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
src/components/proxy-middleware/middlewares/logger_middleware.js
[error] 3-4: Illegal use of an import declaration outside of a module
not allowed inside scripts
(parse)
🔇 Additional comments (1)
src/components/proxy-middleware/middlewares/logger_middleware.js (1)
1-4: LGTM - Clean integration of the new utility.The import of
getHostis correctly added and follows the existing import pattern.
Cases
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/proxy-middleware/rule_action_processor/utils.js(1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
src/components/proxy-middleware/rule_action_processor/utils.js
[error] 49-62: Illegal use of an export declaration outside of a module
not allowed inside scripts
(parse)
_originalUrl_attribute on ctxDoes not need updated in desktop consumer or UI. Automatically shows port like this
fixes requestly/requestly#1971
Summary by CodeRabbit