[TT-15830] Plugin loading failure error is ignored for certain types of plugins

[MaciekMis]

User description

TT-15830

Summary	Plugin loading failure error is ignored for certain types of plugins
Type	Bug
Status	In Dev
Points	N/A
Labels	codilime_refined

---

Description

In case of gRPC, Python and Lua plugins the plugin loading failure error is ignored and the API request is still processed. This may cause unexpected behaviour as well as security issues, e.g., when Custom Plugin is chosen as an authentication method.

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

Bug fix

---

Description

• Validate driver loaded during request processing

• Return 500 when driver missing

• Remove premature check from enable phase

• Improve logging for missing drivers

---

Diagram Walkthrough

flowchart LR
  Enabled["EnabledForSpec: support check only"] --> Process["ProcessRequest: verify driver loaded"]
  Process -- "driver missing" --> Error["Log error + return 500"]
  Process -- "driver present" --> Continue["Proceed with hooks"]

Loading

File Walkthrough

	Relevant files
Bug fix	coprocess.go Move driver check to request path with 500 on failure        gateway/coprocess.go Remove driver-loaded check from EnabledForSpec. Add driver-loaded validation at ProcessRequest start. Log error when driver is not loaded. Return HTTP 500 with error on missing driver. +9/-7

---

[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: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Behavior Change The check for an unloaded driver was moved from EnabledForSpec to ProcessRequest, which changes when and how failures are surfaced (now at request time with 500). Confirm this aligns with desired UX/telemetry and won’t cause noisy logs or unexpected 500s compared to previously disabling the middleware. if d, _ := loadedDrivers[m.Spec.CustomMiddleware.Driver]; d == nil { log.WithFields(logrus.Fields{ "prefix": "coprocess", }).Errorf("Driver '%s' isn't loaded", m.Spec.CustomMiddleware.Driver) respCode := http.StatusInternalServerError return errors.New(http.StatusText(respCode)), respCode } Error Message Returning errors.New(http.StatusText(500)) loses context about which driver failed; consider wrapping the error with driver details or using a sentinel/error type to aid debugging and error metrics. return errors.New(http.StatusText(respCode)), respCode }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Return descriptive error details Return a concrete error describing the missing driver instead of a generic HTTP status text. This preserves context for upstream handlers and logs. Also include the driver name in the error for easier debugging. gateway/coprocess.go [309-316] if d, _ := loadedDrivers[m.Spec.CustomMiddleware.Driver]; d == nil { + driver := m.Spec.CustomMiddleware.Driver log.WithFields(logrus.Fields{ "prefix": "coprocess", - }).Errorf("Driver '%s' isn't loaded", m.Spec.CustomMiddleware.Driver) + "driver": driver, + }).Errorf("Driver isn't loaded") respCode := http.StatusInternalServerError - - return errors.New(http.StatusText(respCode)), respCode + return fmt.Errorf("coprocess driver not loaded: %s", driver), respCode } Suggestion importance[1-10]: 5 __ Why: The existing_code matches the new hunk lines 309-316. Replacing a generic status-text error with a descriptive fmt.Errorf including the driver aids debugging, but it’s a minor improvement and slightly changes log format; also requires ensuring fmt is imported.	Low
Possible issue	Avoid double map lookup Avoid recomputing the lookup and potential data race by storing loadedDrivers[m.Spec.CustomMiddleware.Driver] into a variable and reusing it after the check. This prevents discrepancies if loadedDrivers is modified concurrently between checks. gateway/coprocess.go [309-316] -if d, _ := loadedDrivers[m.Spec.CustomMiddleware.Driver]; d == nil { +d, _ := loadedDrivers[m.Spec.CustomMiddleware.Driver] +if d == nil { + driver := m.Spec.CustomMiddleware.Driver log.WithFields(logrus.Fields{ "prefix": "coprocess", - }).Errorf("Driver '%s' isn't loaded", m.Spec.CustomMiddleware.Driver) + "driver": driver, + }).Errorf("Driver isn't loaded") respCode := http.StatusInternalServerError - - return errors.New(http.StatusText(respCode)), respCode + return fmt.Errorf("coprocess driver not loaded: %s", driver), respCode } Suggestion importance[1-10]: 3 __ Why: The existing_code matches lines 309-316, but the claim about preventing concurrent modification issues is weak without synchronization context; reducing a single inline lookup to a separate variable offers negligible benefit and does not address real concurrency safety.	Low

[github-actions]

API Changes

no api changes detected

[probelabs]

🔍 Code Analysis Results

1. Change Impact Analysis

What this PR accomplishes

This pull request addresses a critical security vulnerability where the failure to load a custom plugin (gRPC, Python, or Lua) was silently ignored. Previously, if a required plugin driver was missing, the middleware would simply be disabled for the request, allowing it to proceed without executing the intended logic, such as custom authentication. This created a significant risk of bypassing security controls.

The PR implements a fail-closed security posture. Now, if a configured plugin driver is not loaded at request time, the process is immediately halted, and a 500 Internal Server Error is returned to the client. This ensures that a misconfiguration or failure in the plugin system cannot result in a security bypass.

Key Technical Changes

The core technical change involves relocating the plugin driver validation logic within the CoProcessMiddleware:

• From EnabledForSpec(): The check for a loaded driver was removed from this function. EnabledForSpec is called during the API loading phase, and returning false from it only disables the middleware for the request chain without stopping the request itself, which was the source of the vulnerability.
• To ProcessRequest(): A new validateDriver() function is now called at the beginning of ProcessRequest(). This function checks if the driver is supported and loaded. Since ProcessRequest() is executed for every incoming API request, this change ensures the validation occurs at runtime. If the validation fails, it immediately returns an error, halting the middleware chain and preventing the request from reaching the upstream service.

Affected System Components

• Tyk Gateway (CoProcessMiddleware): The primary component affected. Its behavior for handling custom plugins is now more secure and robust, preventing fail-open scenarios.
• API Definitions with Custom Plugins: Any API configured to use gRPC, Python, or Lua plugins will be impacted. The change guarantees these APIs will fail safely if their plugin dependencies are not met.
• API Consumers: Clients of an affected API will now receive a clear 500 Internal Server Error if the backend plugin is misconfigured, which is a safer and more explicit signal of failure than an unexpected successful response.

2. Architecture Visualization

The following diagrams illustrate the change in the request lifecycle when a plugin driver fails to load.

Before Change: Security Bypass on Plugin Load Failure

The original logic disabled the middleware but allowed the request to continue, potentially bypassing critical security checks.

sequenceDiagram
    participant Client
    participant Tyk Gateway
    participant CoProcessMiddleware
    participant Upstream Service

    Client->>Tyk Gateway: API Request
    Tyk Gateway->>CoProcessMiddleware: EnabledForSpec()?
    Note over CoProcessMiddleware: Driver not loaded, returns false
    CoProcessMiddleware-->>Tyk Gateway: Middleware Disabled
    Note over Tyk Gateway: Request proceeds without plugin logic
    Tyk Gateway->>Upstream Service: Forwards Request (Auth Bypassed!)
    Upstream Service-->>Tyk Gateway: 200 OK
    Tyk Gateway-->>Client: 200 OK

Loading

After Change: Fail-Closed on Plugin Load Failure

The new logic halts the request immediately within ProcessRequest, preventing any security bypass and returning a clear error.

sequenceDiagram
    participant Client
    participant Tyk Gateway
    participant CoProcessMiddleware
    participant Upstream Service

    Client->>Tyk Gateway: API Request
    Tyk Gateway->>CoProcessMiddleware: ProcessRequest()
    Note over CoProcessMiddleware: Driver not loaded, returns error!
    CoProcessMiddleware-->>Tyk Gateway: 500 Internal Server Error
    Note over Tyk Gateway: Request processing is halted
    Tyk Gateway-->>Client: 500 Internal Server Error
    Note right of Client: Request never reaches the Upstream Service

Loading

---

Powered by Visor from Probelabs

Last updated: 2025-10-02T13:22:43.273Z | Triggered by: synchronize | Commit: 7fcc173

[probelabs]

🔍 Code Analysis Results

Security Issues (2)

Severity	Location	Issue
🟢 Info	gateway/coprocess.go:313	This change remediates a critical authentication bypass vulnerability. Previously, if a required plugin driver failed to load, the middleware was silently disabled, allowing requests to proceed without intended security checks (e.g., custom authentication). By moving the check to `ProcessRequest` and returning a 500 error, the change correctly implements a fail-closed security posture. 💡 Suggestion The implemented change is the correct and necessary approach to fix this critical vulnerability. No further action is required.
🟢 Info	gateway/coprocess.go:520-529	The error returned to the client is generic ('Internal Server Error'), which is a security best practice. This prevents leaking internal state, such as the specific driver name that failed to load, which could be useful to an attacker. 💡 Suggestion This is a positive finding. This approach correctly prevents sensitive data exposure in error responses and should be maintained.

Performance Issues (1)

Severity	Location	Issue
🟢 Info	gateway/coprocess.go:314	The driver validation logic, which includes a map lookup and an iteration over a small, three-element slice, has been moved into the `ProcessRequest` function. This function is on the hot path and is executed for every API request. While this adds a small, constant-time overhead to each request, the performance impact is negligible and is an acceptable trade-off for the critical security fix it provides. The benefit of ensuring the system fails safely far outweighs the minor performance cost.

Quality Issues (1)

Severity	Location	Issue
🟢 Info	gateway/coprocess.go:314-322	The driver validation logic has been moved to the `ProcessRequest` function, which is executed on every API request. This introduces a small, constant-time overhead (a map lookup and an iteration over a small, 3-element slice) to the request hot path. The performance impact is negligible and is an acceptable trade-off for fixing a critical security vulnerability by ensuring the system fails safely.

Style Issues (2)

Severity	Location	Issue
🟢 Info	gateway/coprocess.go:504	The error returned when a driver is unsupported or not loaded is generic (`errors.New(http.StatusText(respCode))`). While the detailed error is logged, returning a more specific error object can improve internal debugging and error handling capabilities for any code that might consume this error. Consider using fmt.Errorf to create a more descriptive error message that includes context, such as the driver name. This doesn't affect the client response but improves internal diagnostics. 💡 Suggestion Replace errors.New(http.StatusText(respCode)) with a more descriptive error, for example: For an unsupported driver: return fmt.Errorf("coprocess: unsupported driver '%s'", m.Spec.CustomMiddleware.Driver), respCode For an unloaded driver: return fmt.Errorf("coprocess: driver '%s' not loaded", m.Spec.CustomMiddleware.Driver), respCode
🟡 Warning	gateway/coprocess.go:497	The return signature of `validateDriver` is `(int, error)`, while the `ProcessRequest` function that calls it has a signature of `(error, int)`. This inconsistency requires swapping the return values at the call site in `ProcessRequest` (line 294: `return err, errorCode`), which can be confusing and reduce readability. To improve consistency, validateDriver should return (error, int) to match the convention used by ProcessRequest. 💡 Suggestion Change the function signature to func (m *CoProcessMiddleware) validateDriver() (error, int) and adjust the return statements accordingly. This will make the call site in ProcessRequest more straightforward (err, errorCode := m.validateDriver()).

Dependency Issues (2)

Severity	Location	Issue
🟢 Info	gateway/coprocess.go:525	The function returns a generic error for an unsupported driver (`errors.New("Internal Server Error")`), which loses valuable context for internal debugging and error tracking. While the error is logged, propagating a more specific error object within the middleware chain is a best practice. 💡 Suggestion Return a more descriptive error that includes the name of the unsupported driver. This will improve traceability without exposing internal details to the end-user. For example: `return respCode, fmt.Errorf("unsupported coprocess driver: '%s'", m.Spec.CustomMiddleware.Driver)`
🟢 Info	gateway/coprocess.go:534	The function returns a generic error when a required driver is not loaded. This obscures the root cause during debugging. A specific error message would make it easier to identify and resolve configuration issues. 💡 Suggestion Return a more descriptive error that includes the name of the driver that failed to load. For example: `return respCode, fmt.Errorf("coprocess driver not loaded: '%s'", m.Spec.CustomMiddleware.Driver)`

✅ Connectivity Check Passed

No connectivity issues found – changes LGTM.

---

Powered by Visor from Probelabs

Last updated: 2025-10-02T13:22:44.336Z | Triggered by: synchronize | Commit: 7fcc173

[sonarqubecloud]

Quality Gate failed

Failed conditions
40.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud