Skip to content

[TT-11048] Regex on endpoint list issue poc #7266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shults
Copy link
Contributor

@shults shults commented Jul 30, 2025

User description

TT-11048
Summary Regex on endpoint list issue
Type Bug Bug
Status In Dev
Points N/A
Labels codilime_refined, jira_escalated

User description

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

  • I ensured that the documentation is up to date
  • I explained why this PR updates go.mod in detail with reasoning why it's required
  • I would like a code coverage CI quality gate exception and have explained why

PR Type

enhancement


Description

  • Refactored OAS filling functions to accept options parameter

  • Introduced fillOptions and transform strategy for regex handling

  • Added ensureOperationId and related helpers for operation management

  • Updated tests and internal calls to use new options-based signatures


Diagram Walkthrough


flowchart 
  LR 
  "OAS.Fill" -- "now takes FillOption" --> "fillPathsAndOperations"
  "fillPathsAndOperations" -- "passes fillOptions" --> "all fill* methods"
  "fill* methods" -- "use ensureOperationId" --> "operation/Path management"
  "Tests" -- "updated to use defaultFillOptions" --> "fill* methods"

File Walkthrough

Relevant files


PR Type

Enhancement


Description

  • Refactored OAS fill methods to accept optional fill options

  • Introduced fillOptions and FillOption for flexible endpoint transformation

  • Added strategy to control regex-based endpoint translation

  • Updated tests and internal logic to use new fillOptions pattern


Diagram Walkthrough

flowchart LR
  "OAS.Fill()" -- "now accepts FillOption" --> "fillPathsAndOperations(..., fillOptions)"
  "fillPathsAndOperations" -- "passes fillOptions" --> "all fill* methods"
  "fill* methods" -- "use ensureOperationId(..., fillOptions)" --> "operation id logic"
  "fillOptions" -- "strategy: regexTransform/regexIgnoreTransform" --> "ensureOperationId"
  "Tests" -- "updated to use defaultFillOptions()" --> "fill* methods"
Loading

File Walkthrough

Relevant files

@shults shults marked this pull request as ready for review July 30, 2025 10:33
Copy link
Contributor

github-actions bot commented Jul 30, 2025

PR Reviewer Guide 🔍

(Review updated until commit 8a40ed4)

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

Backward Compatibility

The refactoring of the Fill and fillPathsAndOperations methods introduces a new fillOptions parameter and changes the default behavior for regex transformation. The reviewer should validate that all previous usages of these methods are updated accordingly and that the default behavior does not break existing integrations or expected outputs.

func (s *OAS) Fill(api apidef.APIDefinition, opts ...FillOption) {
	opt := option.New(opts).Build(fillOptions{
		strategy: regexTransform, // transform regex by default
	})

	xTykAPIGateway := s.GetTykExtension()
	if xTykAPIGateway == nil {
		xTykAPIGateway = &XTykAPIGateway{}
		s.SetTykExtension(xTykAPIGateway)
	}

	xTykAPIGateway.Fill(api)
	s.fillPathsAndOperations(api.VersionData.Versions[Main].ExtendedPaths, *opt)
	s.fillSecurity(api)

	if ShouldOmit(xTykAPIGateway) {
		delete(s.Extensions, ExtensionTykAPIGateway)
	}

	if ShouldOmit(s.Extensions) {
		s.Extensions = nil
	}

	// set external docs to nil if populated with default values
	if ShouldOmit(s.ExternalDocs) {
		s.ExternalDocs = nil
	}
}

// ExtractTo extracts *OAS into *apidef.APIDefinition.
func (s *OAS) ExtractTo(api *apidef.APIDefinition) {
	if s.GetTykExtension() == nil {
		s.SetTykExtension(&XTykAPIGateway{})
		defer func() {
			delete(s.Extensions, ExtensionTykAPIGateway)
		}()
	}

	s.GetTykExtension().ExtractTo(api)

	s.extractSecurityTo(api)

	vInfo := api.VersionData.Versions[Main]
	vInfo.UseExtendedPaths = true
	s.extractPathsAndOperations(&vInfo.ExtendedPaths)
	api.VersionData.Versions[Main] = vInfo
}

func (s *OAS) SetTykStreamingExtension(xTykStreaming *XTykStreaming) {
	if s.Extensions == nil {
		s.Extensions = make(map[string]interface{})
	}

	s.Extensions[ExtensionTykStreaming] = xTykStreaming
}

func (s *OAS) GetTykStreamingExtension() *XTykStreaming {
	if s.Extensions == nil {
		return nil
	}

	if ext := s.Extensions[ExtensionTykStreaming]; ext != nil {
		rawTykStreaming, ok := ext.(json.RawMessage)
		if ok {
			var xTykStreaming XTykStreaming
			_ = json.Unmarshal(rawTykStreaming, &xTykStreaming)
			s.Extensions[ExtensionTykStreaming] = &xTykStreaming
			return &xTykStreaming
		}

		mapTykAPIGateway, ok := ext.(map[string]interface{})
		if ok {
			var xTykStreaming XTykStreaming
			dbByte, _ := json.Marshal(mapTykAPIGateway)
			_ = json.Unmarshal(dbByte, &xTykStreaming)
			s.Extensions[ExtensionTykStreaming] = &xTykStreaming
			return &xTykStreaming
		}

		return ext.(*XTykStreaming)
	}

	return nil
}

func (s *OAS) RemoveTykStreamingExtension() {
	if s.Extensions == nil {
		return
	}

	delete(s.Extensions, ExtensionTykStreaming)
}

// SetTykExtension populates our OAS schema extension inside *OAS.
func (s *OAS) SetTykExtension(xTykAPIGateway *XTykAPIGateway) {
	if s.Extensions == nil {
		s.Extensions = make(map[string]interface{})
	}
Operation ID Generation Consistency

The introduction of ensureOperationId and the new strategy pattern for operation ID generation may affect how operation IDs are created, especially with regex and non-regex paths. The reviewer should check that operation IDs remain unique and consistent, and that there are no regressions in how endpoints are mapped.

Error Handling for Unknown Strategy

The ensureOperationId method panics if an unknown strategy is provided. The reviewer should consider whether this is the desired behavior or if a more graceful error handling mechanism is appropriate.

Copy link
Contributor

PR Code Suggestions ✨

No code suggestions found for the PR.

Copy link
Contributor

📦 Impact Review Snapshot

Effort Downstream Updates Compatibility Docs TL;DR
Low ⚠️ 🟡 ⚠️ Refactored OAS filling functions to use options pattern for regex handling
## Impact Assessment

This PR refactors the OAS (OpenAPI Specification) filling functions in the Tyk Gateway to accept an options parameter, introducing a new strategy for handling regex in endpoint paths. The main changes include:

  1. Adding a fillOptions struct with a strategy field that determines how regex paths are transformed
  2. Modifying the Fill method signature to accept optional parameters
  3. Adding a new ensureOperationId function that replaces direct calls to getOperationID
  4. Introducing transform strategies (regexTransform and regexIgnoreTransform) for regex handling

These changes primarily affect how the Tyk Gateway converts classic API definitions to OAS format, particularly for endpoints with regex patterns in their paths.

## Required Updates
  1. tyk-operator:

    • Update any code that directly calls OAS.Fill() to handle the new function signature
    • Ensure CRD validation logic accounts for the new regex handling strategies
  2. tyk-charts:

    • No direct code updates required, but configuration documentation may need updates
  3. portal:

    • Update any API import/export functionality that interacts with the OAS filling process
    • Ensure UI correctly displays regex-based endpoints
  4. tyk-sink (MDCB):

    • Update any code that directly calls OAS.Fill() to handle the new function signature
    • Ensure synchronization of APIs with regex endpoints works correctly
## Compatibility Concerns

The PR introduces a backward-compatible change to the Fill method by using optional parameters with default values. However, there are potential compatibility issues:

  1. The default behavior is to use regexTransform strategy, which might change how some regex paths are processed compared to the previous implementation.

  2. The getOperationID function is now marked as deprecated with a TODO comment indicating it has side effects. Any downstream code directly calling this function should be updated to use the new ensureOperationId function instead.

  3. If downstream repositories have custom implementations that expect the old behavior, they may need to be updated to handle the new options-based approach.

## Summary & Recommendations
  • This PR introduces a more flexible approach to handling regex in endpoint paths, but requires careful testing in downstream repositories to ensure compatibility.
  • The tyk-operator repository should be prioritized for updates since it's most likely to directly interact with the OAS filling functionality.
  • Documentation should be updated to explain the new options pattern and regex handling strategies.
  • Consider adding explicit examples of how to use the new options in downstream repositories.

Tip: Mention me again using /dependency <request>.
Powered by Probe AI
Tyk Gateway Dependency Impact Reviewer

Copy link
Contributor

🛡️ Security Snapshot

Effort Risk Level Tests Compliance TL;DR
Low 🟢 ✔️ Refactoring of regex handling in OAS path operations with no security impact
## Security Impact Analysis

This PR introduces a refactoring of how regex patterns in endpoint paths are handled when converting between classic API definitions and OpenAPI Specification (OAS) format. The changes add an option-based parameter system to control whether regex patterns should be transformed or ignored during conversion. The default behavior (regexTransform) maintains the existing functionality, while a new option (regexIgnoreTransform) allows bypassing the regex transformation when needed.

The changes are well-contained within the OAS conversion logic and don't modify any security-critical code paths. The PR primarily addresses a functional issue with regex handling in endpoint lists rather than introducing new security features or modifying existing security controls.

## Identified Vulnerabilities

No security vulnerabilities were identified in this PR. The changes:

  • Don't modify any authentication or authorization logic
  • Don't change how regex patterns are evaluated at runtime
  • Don't introduce new regex parsing or evaluation code
  • Don't affect input validation or sanitization
  • Don't modify any security-related headers or responses

The refactoring is focused on the internal representation of paths during OAS conversion and doesn't affect how requests are processed or validated at runtime.

## Security Recommendations

While no security issues were identified, I recommend:

  1. Consider adding a comment in the code explaining the security implications (or lack thereof) of the different regex transformation strategies to help future developers understand the design choices.

  2. Ensure comprehensive test coverage for both transformation strategies to verify that path matching behavior remains consistent regardless of which strategy is used.

  3. Document the new option in developer documentation to clarify when each strategy should be used, particularly noting any edge cases where one approach might be preferred over the other.

## OWASP Compliance

The changes in this PR don't impact OWASP Top 10 compliance:

  • Injection (A1): No changes to input handling or validation
  • Broken Authentication (A2): No changes to authentication mechanisms
  • Sensitive Data Exposure (A3): No changes to data handling or exposure
  • XML External Entities (A4): Not applicable
  • Broken Access Control (A5): No changes to access control logic
  • Security Misconfiguration (A6): No changes to security configuration
  • Cross-Site Scripting (A7): No changes to output encoding or content handling
  • Insecure Deserialization (A8): No changes to serialization/deserialization
  • Using Components with Known Vulnerabilities (A9): No new dependencies
  • Insufficient Logging & Monitoring (A10): No changes to logging or monitoring
## Summary
  • The PR introduces a clean refactoring of OAS filling functions to accept options parameters
  • The new fillOptions structure with transformation strategies provides better control over regex handling
  • The changes are well-contained within the OAS conversion logic and don't affect runtime security
  • No security issues identified – change LGTM.

Tip: Mention me again using /security <request>.
Powered by Probe AI
Security Impact Reviewer Prompt

Copy link
Contributor

🚦 Connectivity Review Snapshot

Effort Tests Security Perf TL;DR
🔵 Low 🔒 none 🟢 OAS regex handling refactoring has no impact on connectivity
## Connectivity Assessment
  • Redis Connections: No changes to Redis connection handling or storage patterns. The PR modifies only OAS (OpenAPI Specification) path handling.
  • RPC Connections: No impact on RPC client functionality or MDCB communication. Changes are isolated to API definition processing.
  • Synchronization Mechanisms: No modifications to pub/sub channels or notification mechanisms. The refactoring is contained within the OAS package.
## Test Coverage Validation
  • Redis Tests: Not applicable - no Redis functionality is modified.
  • RPC Tests: Not applicable - no RPC functionality is modified.
  • Failure Scenario Tests: Not applicable - the changes don't affect connectivity failure handling.
## Security & Performance Impact
  • Authentication Changes: None - the PR only refactors how regex patterns in API paths are transformed to OAS format.
  • Performance Considerations: Minimal impact - the changes add a strategy parameter to control regex transformation but don't affect request processing performance.
  • Error Handling: No changes to connectivity error handling - the PR introduces a panic for unknown transform strategies, but this is only triggered by internal programming errors.
## Summary & Recommendations
  • No suggestions to provide – change LGTM.

Tip: Mention me again using /connectivity <request>.
Powered by Probe AI
Connectivity Issues Reviewer Prompt for Tyk Gateway

Copy link
Contributor

🚀 Performance Snapshot

Effort Perf Risk Hot Paths Benchmarks TL;DR
Low 🟢 Refactoring adds option pattern with negligible overhead, but enables performance optimization by skipping regex transformations
## Performance Impact Analysis

This PR refactors the OAS filling functions to accept an options parameter, introducing a strategy pattern for regex handling. The key change is the ability to skip regex transformations when not needed, which can improve performance for APIs with complex regex patterns. The option pattern itself adds minimal overhead during initialization but can lead to performance gains during path processing.

## Critical Areas

The most performance-sensitive area affected is the regex path handling in getOperationID(). This function has side effects and is now properly marked as deprecated. The new ensureOperationId() function provides a cleaner interface with strategy selection. The regex transformation process involves string parsing, regex detection, and parameter creation - all of which can be skipped with the new regexIgnoreTransform strategy when appropriate.

## Optimization Recommendations
  1. Consider adding benchmarks to quantify the performance improvement of skipping regex transformations.
  2. The string concatenation in buildPath() could be optimized using strings.Builder for better memory efficiency.
  3. Consider caching the results of regex detection in high-traffic scenarios to avoid repeated pattern matching.
  4. The parsePathSegment() function could benefit from pre-compiled regex patterns rather than using regexp.MatchString each time.
## Summary
  • The PR introduces a clean way to optimize performance by making regex transformation optional
  • The option pattern adds minimal overhead but enables more efficient path processing
  • String operations and regex handling remain the most expensive operations in this code path
  • The changes are well-structured and unlikely to introduce performance regressions

Tip: Mention me again using /performance <request>.
Powered by Probe AI
Performance Impact Reviewer Prompt

Copy link
Contributor

Persistent review updated to latest commit 8a40ed4

Copy link
Contributor

API Changes

--- prev.txt	2025-07-30 11:12:00.597623070 +0000
+++ current.txt	2025-07-30 11:11:51.199493189 +0000
@@ -2077,6 +2077,10 @@
     acknowledging that some standard Tyk OAS API fields are optional in
     templates.
 
+func WithOmitTranslate() option.Option[fillOptions]
+    WithOmitTranslate omits regex-based endpoints translation during classic to
+    oas translation.
+
 
 TYPES
 
@@ -2872,6 +2876,9 @@
 func (e *ExtractCredentialsFromBody) Fill(api apidef.APIDefinition)
     Fill fills *ExtractCredentialsFromBody from apidef.APIDefinition.
 
+type FillOption = option.Option[fillOptions]
+    FillOption optional parameter for Fill method.
+
 type FromOASExamples struct {
 	// Enabled activates getting a mock response from OAS examples or schemas documented in OAS.
 	Enabled bool `bson:"enabled" json:"enabled"`
@@ -3577,7 +3584,7 @@
 func (s *OAS) ExtractTo(api *apidef.APIDefinition)
     ExtractTo extracts *OAS into *apidef.APIDefinition.
 
-func (s *OAS) Fill(api apidef.APIDefinition)
+func (s *OAS) Fill(api apidef.APIDefinition, opts ...FillOption)
     Fill fills *OAS definition from apidef.APIDefinition.
 
 func (s *OAS) GetTykExtension() *XTykAPIGateway
@@ -5516,6 +5523,8 @@
 
 func (o Options[O]) Build(baseVal O) *O
 
+func (o Options[O]) BuildValue(baseVal O) O
+
 # Package: ./config
 
 package config // import "github.com/TykTechnologies/tyk/config"

Copy link
Contributor

PR Code Suggestions ✨

No code suggestions found for the PR.

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants