fix(utils): santize host when target has host port#6759
Conversation
WalkthroughAdds host:port normalization logic via a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/protocols/utils/fields_test.go:
- Around line 9-72: Add a new table-driven test case in
TestGetJsonFieldsFromURL_HostPortExtraction that covers a URL with an explicit
standard HTTPS port (e.g., "https://example.com:443/path") so
GetJsonFieldsFromURL is validated against the exact bug; set name to something
like "https with explicit 443", input to "https://example.com:443/path",
expectedHost to "example.com" and expectedPort to "443", then run the existing
assertions (assert.Equal on fields.Host and fields.Port) to ensure the function
returns the correct values.
🧹 Nitpick comments (1)
pkg/protocols/utils/fields.go (1)
84-107: Consider adding documentation for port precedence logic.The implementation correctly handles various host formats (standard host:port, IPv6 with/without brackets). However, the port precedence behavior (lines 90-92, 101-102) where an existing
portparameter takes precedence over a port extracted fromhostcould benefit from a comment explaining this design choice.📝 Suggested documentation
+// extractHostPort separates hostname and port from a host string. +// If port parameter is non-empty, it takes precedence over any port in the host string. +// Handles IPv6 addresses with brackets (e.g., "[::1]:8080" or "[::1]"). func extractHostPort(host, port string) (string, string) {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/protocols/utils/fields.gopkg/protocols/utils/fields_test.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code usinggo fmt ./...
Run static analysis usinggo vet ./...on Go code
Files:
pkg/protocols/utils/fields.gopkg/protocols/utils/fields_test.go
pkg/protocols/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
pkg/protocols/**/*.go: Each protocol implementation should implement the Request interface with Compile(), ExecuteWithResults(), Match(), and Extract() methods
Protocol implementations should embed Operators for matching/extraction functionality
Files:
pkg/protocols/utils/fields.gopkg/protocols/utils/fields_test.go
🧬 Code graph analysis (1)
pkg/protocols/utils/fields_test.go (2)
pkg/protocols/utils/fields.go (1)
GetJsonFieldsFromURL(23-50)pkg/protocols/utils/variables.go (2)
Host(45-45)Port(46-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (4)
pkg/protocols/utils/fields.go (3)
4-5: LGTM: Necessary imports for host/port parsing.The
netandstringspackages are appropriately used in theextractHostPortfunction for host/port separation and IPv6 bracket handling.
35-48: LGTM: Correctly normalizes host field.The host normalization logic properly addresses the bug reported in issue #6758. By extracting and using the normalized
hostvalue (without port) for field assignments, theHostfield will now contain only the hostname whilePortcontains the port number separately.
67-80: LGTM: Consistent host normalization.The changes mirror the normalization logic in
GetJsonFieldsFromURL, ensuring consistent behavior across both functions.pkg/protocols/utils/fields_test.go (1)
74-138: LGTM: Comprehensive unit tests for extractHostPort.The test cases provide good coverage of the host/port extraction logic, including:
- Standard host:port parsing
- IPv6 bracket formats (with/without port)
- Port precedence behavior (line 99-104)
- IPv4 addresses
The port precedence test (line 99-104) is particularly valuable as it documents that an existing port parameter takes precedence over port extracted from the host string.
closes #6758
Proposed changes
Proof
Checklist
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.