[TT-12741] Looped ap is wrongfully inherit the caller's authentication key when using url rewrite

[buraksezer]

User description

TT-12741

Summary	Looped APIs wrongfully inherit the caller's Authentication key when using URL rewrite
Type	Bug
Status	In Dev
Points	N/A
Labels	'24Bugsmash, customer_bug, jira_escalated

---

PR to see CI/CD result, please don't merge it.

---

PR Type

Bug fix, Tests

---

Description

• Introduced a new context constant SelfLooping and methods ctxSetSelfLooping and ctxSelfLooping to manage self-looping state in requests.
• Updated ctxCheckLimits to bypass rate limits and quotas for self-looping requests.
• Modified API loader to set self-looping state for self-referencing requests.
• Enhanced the test TestQuotaNotAppliedWithURLRewrite to include scenarios for self-looping and URL rewrite, ensuring proper behavior.

---

Changes walkthrough 📝

	Relevant files
Enhancement	ctx.go Add support for managing self-looping state in context      ctx/ctx.go Added a new constant SelfLooping to the context. Introduced new methods ctxSetSelfLooping and ctxSelfLooping for managing self-looping state in requests. +1/-0
Bug fix	api.go Update rate limit and quota checks for self-looping requests gateway/api.go Modified ctxCheckLimits to skip rate limits and quotas for self-looping requests. Added logic to check and set self-looping state in requests. +20/-1    api_loader.go Set self-looping state for self-referencing requests          gateway/api_loader.go Added logic to set self-looping state when the hostname is "self". +1/-0
Tests	middleware_test.go Enhance tests to cover self-looping and URL rewrite scenarios gateway/middleware_test.go Updated TestQuotaNotAppliedWithURLRewrite to include extended paths and self-looping scenarios. Added a loader to create a merged API spec for testing. +7/-1

---

💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

[buger]

I'm a bot and I 👍 this PR title. 🤖

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Code Smell The addition of the SelfLooping constant should be reviewed to ensure it is necessary and aligns with the existing constants. Verify its usage across the codebase for consistency. Possible Bug The ctxCheckLimits function now skips limit checks if ctxSelfLooping is true. Ensure this logic does not introduce unintended behavior or bypass critical checks. Code Smell The ctxSetSelfLooping and ctxSelfLooping functions introduce new context handling. Verify their necessity and ensure they follow the existing context management patterns. Possible Bug The ctxSetSelfLooping call in the ServeHTTP method might have unintended side effects. Ensure that setting this context value does not interfere with other request handling logic. Test Coverage The test TestQuotaNotAppliedWithURLRewrite has been updated. Ensure that it adequately tests the new behavior introduced by the ctxSetSelfLooping logic and covers edge cases.

[github-actions]

API Changes

--- prev.txt	2024-12-19 12:34:28.081919637 +0000
+++ current.txt	2024-12-19 12:34:23.616824756 +0000
@@ -6847,6 +6847,7 @@
 	// CacheOptions holds cache options required for cache writer middleware.
 	CacheOptions
 	OASDefinition
+	SelfLooping
 )
 # Package: ./dlpython
 

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Possible issue	Add a fallback to handle non-boolean context values in ctxSelfLooping Ensure that the ctxSelfLooping function properly handles cases where the context value is not a boolean to avoid potential runtime errors. gateway/api.go [3275-3279] if v := r.Context().Value(ctx.SelfLooping); v != nil { if boolVal, ok := v.(bool); ok { return boolVal } + return false // Handle non-boolean values gracefully } Suggestion importance[1-10]: 9 Why: The suggestion addresses a potential runtime error by adding a fallback for non-boolean context values in ctxSelfLooping. This is a critical improvement to ensure robustness and prevent unexpected crashes.	9
General	Improve error handling in the test to ensure failures are reported explicitly Ensure that the loader.MakeSpec call in the test handles errors gracefully to prevent test failures from unhandled exceptions. gateway/middleware_test.go [433-434] spec, err := loader.MakeSpec(&model.MergedAPI{APIDefinition: preSpec.APIDefinition}, nil) -require.NoError(t, err) +if err != nil { + t.Fatalf("Failed to create spec: %v", err) // Handle error explicitly +} Suggestion importance[1-10]: 8 Why: The suggestion enhances error handling in the test by explicitly reporting failures, which improves test reliability and debugging. This is a meaningful improvement for maintaining high-quality test coverage.	8
	Add validation to ensure ctxSetSelfLooping is only invoked for valid hostnames Verify that ctxSetSelfLooping is only called when the r.URL.Hostname() value is validated to avoid unintended side effects. gateway/api_loader.go [601-602] if r.URL.Hostname() == "self" { - ctxSetSelfLooping(r, true) + if isValidHostname(r.URL.Hostname()) { // Add validation for hostname + ctxSetSelfLooping(r, true) + } +} Suggestion importance[1-10]: 7 Why: Adding validation for the hostname before invoking ctxSetSelfLooping reduces the risk of unintended side effects. While the suggestion is valid and improves code safety, the current implementation may already assume the hostname is valid, slightly reducing its criticality.	7

[titpetric]

I think this also can be moved to httpctx if the usage is low (complete looping context global funcs).

[jeffy-mathew]

Adding this comment here for future reference.
Rate limit isn't checked while looping unless explicitly specified with check_limits query parameter. Refer https://tyk.io/docs/advanced-configuration/transform-traffic/looping/#rate-limiting-in-looping.

With the changes related to quota limits not respected while self looping, we added the same check ctxCheckLimits in auth key check to skip auth check.
Customers were relying on this behaviour to have different authentication key for the second (looped) API.
So, instead of checking with ctxCheckLimits, use ctxSelfLoop to skip auth check only when the API is using a self loop.
In other cases, the auth check can happen if it is specified in corresponding API definition.

[jeffy-mathew]

approving despite the race on tests, considering this comment
https://github.com/TykTechnologies/tyk/blob/master/gateway/looping_test.go#L4

[titpetric]

Behaviour 1: Limits are checked on first (root) request, and not on subsequent children.

ctxLoopingEnabled => ctxLoopLevel(r) > 0

It unwraps the function and makes it clearer to read without the negation.

[titpetric]

Missed code path for internal routing requests.

As a request is invoked with tyk:// schema it falls into two code branches; this branch didn't consider incrementing the loop level checks; copied from slightly above.

Naming of this service object is severely misfortunate: DummyProxyHandler (nothing dummy about it).

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
84.8% Coverage on New Code
0.2% Duplication on New Code

See analysis details on SonarQube Cloud

[buraksezer]

/release to release-5.3

[tykbot]

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

[tykbot]

@buraksezer Succesfully merged PR