[TT-5790] Update EnsureTransport and related tests by titpetric · Pull Request #6243 · TykTechnologies/tyk

titpetric · 2024-04-23T10:11:22Z

User description

Several tests encountered parsing errors where the URL parser complained that "the first path segment in URL cannot contain colon". This occurred because the URLs were missing the scheme (e.g., http://) and couldn't be parsed.

This PR improves EnsureTransport to better construct a valid URL from the host and protocol parameters.

Improves test with more test cases for IP:port and protocol pairs
Verifies intended behaviour with h2c URL parsing (coalescing to http scheme) with a unit test

https://tyktech.atlassian.net/browse/TT-5790

Type

enhancement, tests

Description

Refactored EnsureTransport to automatically handle missing protocols by defaulting to "http" and to prepend the protocol if it's missing in the host string.
Removed outdated switch-case logic in EnsureTransport, simplifying the URL scheme handling.
Expanded unit tests in reverse_proxy_test.go to cover new scenarios including IP addresses with ports, and protocol handling.
Improved test assertions to use assert.Equal and added checks to ensure URLs are parsable after processing.

Changes walkthrough

Relevant files

Enhancement

reverse_proxy.go `Refactor EnsureTransport to handle URL schemes more robustly` gateway/reverse_proxy.go Added logic to `EnsureTransport` to handle missing protocols by defaulting to "http". Amended the function to prepend protocol if missing in the host string. Replaced specific scheme handling with a more generic approach. Removed old switch-case logic that handled schemes.	+14/-9

Tests

reverse_proxy_test.go `Extend tests for EnsureTransport with more scenarios` gateway/reverse_proxy_test.go Expanded test cases for `EnsureTransport` to cover scenarios with IP addresses and ports. Added tests for default protocol handling and h2c to http conversion. Replaced error checks with `assert.Equal` and added URL parse verification.	+22/-3

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions · 2024-04-23T10:12:18Z

API Changes

--- prev.txt	2024-05-14 11:46:26.886532924 +0000
+++ current.txt	2024-05-14 11:46:23.658522169 +0000
@@ -7199,6 +7199,8 @@
     it to base64 and store it in an Event object
 
 func EnsureTransport(host, protocol string) string
+    EnsureTransport sanitizes host/protocol pairs and returns a valid URL.
+
 func GenerateTestBinaryData() (buf *bytes.Buffer)
 func GetAccessDefinitionByAPIIDOrSession(currentSession *user.SessionState, api *APISpec) (accessDef *user.AccessDefinition, allowanceScope string, err error)
 func GetTLSClient(cert *tls.Certificate, caCert []byte) *http.Client

github-actions · 2024-04-23T10:12:20Z

PR Description updated to latest commit (5478721)

github-actions · 2024-04-23T10:12:57Z

PR Review

⏱️ Estimated effort to review [1-5]	3, because the PR involves both logic changes in URL handling and extensive updates to unit tests. Understanding the implications of these changes on existing functionalities and ensuring no regressions occur would require a moderate level of effort.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: The replacement of "h2c://" with "http://" in `EnsureTransport` function might not handle cases where other parts of the codebase explicitly rely on "h2c" as a distinct protocol.
🔒 Security concerns	No

Code feedback:

relevant file	gateway/reverse_proxy.go
suggestion	Consider handling the case where `url.Parse` fails more gracefully. Currently, if `url.Parse` fails, the function returns the original `host` string, which might not be a valid URL. This could lead to further errors down the line. Instead, consider logging the error or even returning an error from the function to handle it upstream. [important]
relevant line	if err != nil {

relevant file	gateway/reverse_proxy.go
suggestion	Add a normalization step for the `protocol` to handle different cases (e.g., "HTTP", "http") uniformly. This can prevent issues with case-sensitive protocol handling elsewhere in the code or when interfacing with other systems that might expect lowercase protocols. [medium]
relevant line	protocol = strings.TrimSpace(protocol)

relevant file	gateway/reverse_proxy.go
suggestion	Consider verifying the scheme after appending it to `host`. This can ensure that the resulting URL is valid and the scheme is correctly formatted before attempting to parse it with `url.Parse`. This could prevent subtle bugs related to malformed URLs. [important]
relevant line	host = protocol + "://" + host

relevant file	gateway/reverse_proxy_test.go
suggestion	Expand the test cases to include more edge cases, such as hosts with unusual characters or international domain names (IDN). This will ensure that the `EnsureTransport` function robustly handles a wider variety of inputs. [medium]
relevant line	{"httpbin.org:2000 ", "", "http://httpbin.org:2000"},

✨ Review tool usage guide:

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

github-actions · 2024-04-23T10:13:42Z

PR Code Suggestions

Category	Suggestions
Enhancement	Improve the robustness of protocol replacement in URLs. Replace the string replacement for 'h2c' protocol with a more robust approach using URL parsing and scheme modification. This ensures that only the scheme part of the URL is modified without affecting other parts of the URL that might accidentally contain the substring 'h2c://'. gateway/reverse_proxy.go [142] -host = strings.Replace(host, "h2c://", "http://", 1) +u, _ := url.Parse(host) +if u.Scheme == "h2c" { + u.Scheme = "http" + host = u.String() +}
Enhancement	Enhance test coverage for `EnsureTransport` with more complex URL scenarios. Add more test cases to cover scenarios where the input host includes unusual characters or patterns to ensure `EnsureTransport` handles these gracefully. gateway/reverse_proxy_test.go [1599] {"httpbin.org:2000 ", "", "http://httpbin.org:2000"}, +{"http://example.com/with_path?query=value", "https", "https://example.com/with_path?query=value"}, +{"[email protected]", "https", "https://[email protected]"},
Error handling	Add error handling for URL parsing failures. Add error handling for URL parsing to prevent returning an invalid URL when an error occurs. This ensures that the function behaves predictably in error scenarios. gateway/reverse_proxy.go [144-146] u, err := url.Parse(host) if err != nil { - return host + return "", err }
Maintainability	Refactor protocol handling to use a switch statement for better extensibility. Use a switch statement to handle different protocols more cleanly and extendibly, instead of using an if condition. This makes it easier to add new protocols in the future. gateway/reverse_proxy.go [133-135] -if protocol == "" { +switch protocol { +case "": protocol = "http" +case "h2c": + protocol = "http" +default: + // no change }
Maintainability	Improve test case identification in `EnsureTransport` tests. Replace the manual test case naming with a more descriptive approach using the host and protocol for easier identification of failing cases. gateway/reverse_proxy_test.go [1612] -t.Run(fmt.Sprintf("case-%d", i), func(t testing.T) { +t.Run(fmt.Sprintf("host-%s_protocol-%s", v.host, v.protocol), func(t testing.T) {

✨ Improve tool usage guide:

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

github-actions · 2024-04-23T10:20:30Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

…/ports

sonarqubecloud · 2024-05-14T11:58:44Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

titpetric · 2024-05-14T14:49:22Z

/release to release-5.3

tykbot · 2024-05-14T14:49:34Z

Working on it! Note that it can take a few minutes.

## **User description** Several tests encountered parsing errors where the URL parser complained that "the first path segment in URL cannot contain colon". This occurred because the URLs were missing the scheme (e.g., `http://`) and couldn't be parsed. This PR improves EnsureTransport to better construct a valid URL from the `host` and `protocol` parameters. - Improves test with more test cases for IP:port and protocol pairs - Verifies intended behaviour with h2c URL parsing (coalescing to http scheme) with a unit test ___ ## **Type** enhancement, tests ___ ## **Description** - Refactored `EnsureTransport` to automatically handle missing protocols by defaulting to "http" and to prepend the protocol if it's missing in the host string. - Removed outdated switch-case logic in `EnsureTransport`, simplifying the URL scheme handling. - Expanded unit tests in `reverse_proxy_test.go` to cover new scenarios including IP addresses with ports, and protocol handling. - Improved test assertions to use `assert.Equal` and added checks to ensure URLs are parsable after processing. ___ ## **Changes walkthrough** <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>reverse_proxy.go</strong><dd><code>Refactor EnsureTransport to handle URL schemes more robustly</code></dd></summary> <hr> gateway/reverse_proxy.go <li>Added logic to <code>EnsureTransport</code> to handle missing protocols by <br>defaulting to "http".<br> <li> Amended the function to prepend protocol if missing in the host <br>string.<br> <li> Replaced specific scheme handling with a more generic approach.<br> <li> Removed old switch-case logic that handled schemes. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6243/files#diff-e6e07722257f7e41691e471185ad6d84fd56dc9e5459526ea32e9a5e8fa1a01b">+14/-9</a>    </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>reverse_proxy_test.go</strong><dd><code>Extend tests for EnsureTransport with more scenarios</code>          </dd></summary> <hr> gateway/reverse_proxy_test.go <li>Expanded test cases for <code>EnsureTransport</code> to cover scenarios with IP <br>addresses and ports.<br> <li> Added tests for default protocol handling and h2c to http conversion.<br> <li> Replaced error checks with <code>assert.Equal</code> and added URL parse <br>verification. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6243/files#diff-ce040f6555143f760fba6059744bc600b6954f0966dfb0fa2832b5eabf7a3c3f">+22/-3</a>    </td> </tr> </table></td></tr></tr></tbody></table> ___ > ✨ **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions --------- Co-authored-by: Tit Petric <[email protected]> (cherry picked from commit 9ee0598)

tykbot · 2024-05-14T14:49:59Z

@titpetric Succesfully merged PR

…tests (#6243) [TT-5790] Update EnsureTransport and related tests (#6243) ## **User description** Several tests encountered parsing errors where the URL parser complained that "the first path segment in URL cannot contain colon". This occurred because the URLs were missing the scheme (e.g., `http://`) and couldn't be parsed. This PR improves EnsureTransport to better construct a valid URL from the `host` and `protocol` parameters. - Improves test with more test cases for IP:port and protocol pairs - Verifies intended behaviour with h2c URL parsing (coalescing to http scheme) with a unit test ___ ## **Type** enhancement, tests ___ ## **Description** - Refactored `EnsureTransport` to automatically handle missing protocols by defaulting to "http" and to prepend the protocol if it's missing in the host string. - Removed outdated switch-case logic in `EnsureTransport`, simplifying the URL scheme handling. - Expanded unit tests in `reverse_proxy_test.go` to cover new scenarios including IP addresses with ports, and protocol handling. - Improved test assertions to use `assert.Equal` and added checks to ensure URLs are parsable after processing. ___ ## **Changes walkthrough** <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>reverse_proxy.go</strong><dd><code>Refactor EnsureTransport to handle URL schemes more robustly</code></dd></summary> <hr> gateway/reverse_proxy.go <li>Added logic to <code>EnsureTransport</code> to handle missing protocols by <br>defaulting to "http".<br> <li> Amended the function to prepend protocol if missing in the host <br>string.<br> <li> Replaced specific scheme handling with a more generic approach.<br> <li> Removed old switch-case logic that handled schemes. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6243/files#diff-e6e07722257f7e41691e471185ad6d84fd56dc9e5459526ea32e9a5e8fa1a01b">+14/-9</a>    </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>reverse_proxy_test.go</strong><dd><code>Extend tests for EnsureTransport with more scenarios</code>          </dd></summary> <hr> gateway/reverse_proxy_test.go <li>Expanded test cases for <code>EnsureTransport</code> to cover scenarios with IP <br>addresses and ports.<br> <li> Added tests for default protocol handling and h2c to http conversion.<br> <li> Replaced error checks with <code>assert.Equal</code> and added URL parse <br>verification. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6243/files#diff-ce040f6555143f760fba6059744bc600b6954f0966dfb0fa2832b5eabf7a3c3f">+22/-3</a>    </td> </tr> </table></td></tr></tr></tbody></table> ___ > ✨ **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions --------- Co-authored-by: Tit Petric <[email protected]>

## **User description** Several tests encountered parsing errors where the URL parser complained that "the first path segment in URL cannot contain colon". This occurred because the URLs were missing the scheme (e.g., `http://`) and couldn't be parsed. This PR improves EnsureTransport to better construct a valid URL from the `host` and `protocol` parameters. - Improves test with more test cases for IP:port and protocol pairs - Verifies intended behaviour with h2c URL parsing (coalescing to http scheme) with a unit test ___ ## **Type** enhancement, tests ___ ## **Description** - Refactored `EnsureTransport` to automatically handle missing protocols by defaulting to "http" and to prepend the protocol if it's missing in the host string. - Removed outdated switch-case logic in `EnsureTransport`, simplifying the URL scheme handling. - Expanded unit tests in `reverse_proxy_test.go` to cover new scenarios including IP addresses with ports, and protocol handling. - Improved test assertions to use `assert.Equal` and added checks to ensure URLs are parsable after processing. ___ ## **Changes walkthrough** <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>reverse_proxy.go</strong><dd><code>Refactor EnsureTransport to handle URL schemes more robustly</code></dd></summary> <hr> gateway/reverse_proxy.go <li>Added logic to <code>EnsureTransport</code> to handle missing protocols by <br>defaulting to "http".<br> <li> Amended the function to prepend protocol if missing in the host <br>string.<br> <li> Replaced specific scheme handling with a more generic approach.<br> <li> Removed old switch-case logic that handled schemes. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6243/files#diff-e6e07722257f7e41691e471185ad6d84fd56dc9e5459526ea32e9a5e8fa1a01b">+14/-9</a>    </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>reverse_proxy_test.go</strong><dd><code>Extend tests for EnsureTransport with more scenarios</code>          </dd></summary> <hr> gateway/reverse_proxy_test.go <li>Expanded test cases for <code>EnsureTransport</code> to cover scenarios with IP <br>addresses and ports.<br> <li> Added tests for default protocol handling and h2c to http conversion.<br> <li> Replaced error checks with <code>assert.Equal</code> and added URL parse <br>verification. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6243/files#diff-ce040f6555143f760fba6059744bc600b6954f0966dfb0fa2832b5eabf7a3c3f">+22/-3</a>    </td> </tr> </table></td></tr></tr></tbody></table> ___ > ✨ **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions --------- Co-authored-by: Tit Petric <[email protected]>

github-actions Bot added enhancement Tests labels Apr 23, 2024

github-actions Bot added the Review effort [1-5]: 3 label Apr 23, 2024

titpetric requested review from andrei-tyk and jeffy-mathew April 23, 2024 10:54

titpetric force-pushed the fix/tt-5790/verify-ip-hosts-in-ensure-transport branch from 5478721 to 0e86433 Compare May 14, 2024 08:29

titpetric enabled auto-merge (squash) May 14, 2024 09:39

Tit Petric added 2 commits May 14, 2024 13:45

Add test case

8e47b15

Update EnsureTransport to properly sanitize host/procol pairs with ip…

1a53c1b

…/ports

titpetric force-pushed the fix/tt-5790/verify-ip-hosts-in-ensure-transport branch from 0e86433 to 1a53c1b Compare May 14, 2024 11:45

jeffy-mathew approved these changes May 14, 2024

View reviewed changes

titpetric merged commit 9ee0598 into master May 14, 2024

titpetric deleted the fix/tt-5790/verify-ip-hosts-in-ensure-transport branch May 14, 2024 14:42

buger mentioned this pull request May 14, 2024

Merging to release-5.3: [TT-5790] Update EnsureTransport and related tests (#6243) #6278

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TT-5790] Update EnsureTransport and related tests#6243

[TT-5790] Update EnsureTransport and related tests#6243
titpetric merged 2 commits into
masterfrom
fix/tt-5790/verify-ip-hosts-in-ensure-transport

titpetric commented Apr 23, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 23, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented Apr 23, 2024

Uh oh!

github-actions Bot commented Apr 23, 2024

Uh oh!

github-actions Bot commented Apr 23, 2024

Uh oh!

github-actions Bot commented Apr 23, 2024

Uh oh!

sonarqubecloud Bot commented May 14, 2024

Uh oh!

titpetric commented May 14, 2024

Uh oh!

tykbot Bot commented May 14, 2024

Uh oh!

tykbot Bot commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

titpetric commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Type

Description

Changes walkthrough

Uh oh!

github-actions Bot commented Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 23, 2024

Uh oh!

github-actions Bot commented Apr 23, 2024

PR Review

Uh oh!

github-actions Bot commented Apr 23, 2024

PR Code Suggestions

Uh oh!

github-actions Bot commented Apr 23, 2024

💥 CI tests failed 🙈

git-state

Uh oh!

sonarqubecloud Bot commented May 14, 2024

Quality Gate passed

Uh oh!

titpetric commented May 14, 2024

Uh oh!

tykbot Bot commented May 14, 2024

Uh oh!

tykbot Bot commented May 14, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

titpetric commented Apr 23, 2024 •

edited

Loading

github-actions Bot commented Apr 23, 2024 •

edited

Loading