Merging to release-5.10: [TT-14814] fix bundle loading issue (#7436)

[buger]

User description

TT-14814 fix bundle loading issue (#7436)

User description

TT-14814

Summary	[Security] Gateway Uses Bundles with Invalid/non-existent Manifests
Type	Bug
Status	In Dev
Points	N/A
Labels	2025_r5_candidate, AI-Complexity-High, AI-Priority-High, customer_bug, jira_escalated, security, stability_theme_security

---

This PR fixes an issue while loading bundles.

---

PR Type

Bug fix, Tests

---

Description

• Switch bundle I/O to afero filesystem

• Enforce manifest verification on existing bundles

• Add in-memory FS tests for manifest checks

• Refactor loading to injectable FS

---

Diagram Walkthrough

flowchart LR
  GW["Gateway.loadBundleWithFs"] -- "fetch via" --> Fetch["FetchBundle(afero.Fs)"]
  GW -- "save via" --> Save["saveBundle(afero.Fs)"]
  GW -- "validate via" --> Manifest["loadBundleManifest(afero.Fs)"]
  Manifest -- "if signed" --> Verify["Bundle.Verify(afero.Fs)"]
  Save -- "ZipBundleSaver uses" --> FS["afero.Fs"]
  Fetch -- "file:// getter uses" --> FS

Loading

File Walkthrough

	Relevant files
Enhancement	coprocess_bundle.go Bundle I/O refactor with FS injection and stricter verification gateway/coprocess_bundle.go Inject afero.Fs across bundle operations Verify manifest even for existing bundles Add loadBundleWithFs and refactor helpers Update ZipBundleSaver and FileBundleGetter to use FS +48/-31
Tests	coprocess_bundle_test.go Tests for FS-backed loading and manifest validation            gateway/coprocess_bundle_test.go Add tests using in-memory afero FS Factor PEM creation helper Test mandatory manifest verification Update Verify calls to pass FS +101/-25

---

---

PR Type

Bug fix, Tests

---

Description

• Inject afero.Fs for bundle I/O

• Enforce manifest verification always

• Refactor saver/getter to FS-backed

• Add in-memory FS verification tests

---

Diagram Walkthrough

flowchart LR
  GW["Gateway.loadBundleWithFs(afero.Fs)"]
  Fetch["FetchBundle(afero.Fs)"]
  Save["saveBundle(afero.Fs)"]
  Manifest["loadBundleManifest(afero.Fs)"]
  Verify["Bundle.Verify(afero.Fs)"]
  FS["afero.Fs"]

  GW -- "fetch via" --> Fetch
  GW -- "save via" --> Save
  GW -- "validate via" --> Manifest
  Manifest -- "verify signature" --> Verify
  Save -- "ZipBundleSaver uses" --> FS
  Fetch -- "file:// getter uses" --> FS

Loading

File Walkthrough

	Relevant files
Bug fix	coprocess_bundle.go FS-backed bundle I/O and stricter verification                      gateway/coprocess_bundle.go Inject afero.Fs across bundle ops Verify existing bundles' manifests/signatures Refactor saver/getter; add Zip extraction helper Add loadBundleWithFs and FS-based paths +86/-49
Tests	coprocess_bundle_test.go Tests for FS-backed loading and validation                              gateway/coprocess_bundle_test.go Add in-memory FS tests for manifest checks Factor PEM helper; use require assertions Update Verify calls to pass afero.Fs +101/-25

---

[probelabs]

🔍 Code Analysis Results

This PR addresses a security vulnerability where the Tyk gateway could load custom middleware bundles from the disk without re-verifying their manifest or signature. The core of the change is a refactoring of the bundle loading mechanism to use an abstracted filesystem (afero.Fs), which enables more robust testing and ensures that manifest verification is performed on every load, not just on the initial download.

Files Changed Analysis

• gateway/coprocess_bundle.go (+86, -49): This is the central part of the change. All direct filesystem operations (e.g., os.Open, os.Stat, os.MkdirAll) have been replaced with calls to an injected afero.Fs interface. The key security fix is within the loadBundle logic, which now calls loadBundleManifest with verification enabled even for bundles that already exist on disk, closing the security loophole. Functions like FetchBundle, saveBundle, and Bundle.Verify have been updated to accept and use the filesystem interface.
• gateway/coprocess_bundle_test.go (+101, -25): The tests are significantly expanded to validate the new behavior. By leveraging an in-memory filesystem (afero.NewMemMapFs), new test cases confirm that signature verification is correctly enforced for pre-existing bundles and that errors are handled properly for invalid or missing manifests. This demonstrates the testability benefits of the filesystem abstraction.
• gateway/reverse_proxy_test.go (+0, -1): A minor, unrelated change removing a time.Sleep, likely to improve test stability.

Architecture & Impact Assessment

What this PR accomplishes

This PR resolves the security vulnerability described in ticket TT-14814 by ensuring that all custom middleware bundles are signature-checked before being loaded by the gateway. This check now applies universally, whether the bundle is being freshly downloaded or is already present on the local filesystem. It also improves the overall code quality by decoupling the bundle loading logic from the physical filesystem, enhancing its testability.

Key technical changes introduced

1. Filesystem Abstraction: The afero library is introduced to abstract all filesystem interactions. Key functions now depend on an afero.Fs interface instead of directly calling os package functions.
2. Enforced Manifest Verification: The loadBundle function has been modified to always perform signature verification. Previously, this check was skipped for bundles already on disk, which created the vulnerability.
3. Improved Testability: The filesystem abstraction allows for robust in-memory unit tests. New tests have been added to cover scenarios that were previously difficult to test, such as loading from a pre-populated but invalid bundle directory.

Affected system components

The primary component affected is the Gateway's Custom Middleware Bundle Loader. This change directly impacts the loading and validation process for any API that utilizes custom middleware bundles (e.g., Python, gRPC plugins). The change is triggered during the gateway's startup and on API reloads.

Component Interaction Diagram

The following diagram illustrates how the afero.Fs abstraction is injected and used across the bundle loading workflow.

flowchart LR
  subgraph API Loading
    direction LR
    loadBundle(spec) -- "calls with os.Fs" --> loadBundleWithFs(spec, fs)
  end

  subgraph Bundle Operations
    direction LR
    loadBundleWithFs -- "injects" --> FS["afero.Fs"]
    loadBundleWithFs -- "uses" --> FetchBundle
    loadBundleWithFs -- "uses" --> saveBundle
    loadBundleWithFs -- "uses" --> loadBundleManifest
  end
  
  subgraph Filesystem Interactions
    direction LR
    FetchBundle -- "reads from" --> FS
    saveBundle -- "writes to" --> FS
    loadBundleManifest -- "reads from" --> FS
    loadBundleManifest -- "calls" --> Verify
    Verify -- "reads from" --> FS
  end

Loading

Scope Discovery & Context Expansion

The changes are well-contained within the gateway package, specifically affecting the coprocess bundle handling logic. The main entry point to this functionality, (gw *Gateway) loadBundle, is invoked during the API loading process. This means the fix is applied universally whenever an API definition containing a custom middleware bundle is loaded or reloaded by the gateway.

The architectural shift to an abstracted filesystem is a significant improvement that not only fixes the immediate security issue but also enhances the long-term maintainability and security posture of this critical component by making it easier to write comprehensive tests.

Metadata

• Review Effort: 3 / 5
• Primary Label: bug

---

Powered by Visor from Probelabs

Last updated: 2025-10-14T15:49:53.353Z | Triggered by: synchronize | Commit: 7ef0378

[probelabs]

🔍 Code Analysis Results

✅ Security Check Passed

No security issues found – changes LGTM.

Performance Issues (1)

Severity	Location	Issue
🟢 Info	unknown:1	💡 Suggestion The change now mandates signature and checksum verification for custom middleware bundles that already exist on the filesystem, a step that was previously skipped. This correctly prioritizes security but introduces additional I/O (reading all bundle files) and CPU (hashing, signature verification) overhead during gateway startup and API reloads for each API that uses a pre-existing bundle. While this is a necessary trade-off for security, the performance impact on the control plane should be acknowledged. No change is recommended as this is an intentional and critical security fix.

Quality Issues (2)

Severity	Location	Issue
🟠 Error	gateway/coprocess_bundle.go:228-234	Errors from `newFile.Close()` are logged but not propagated. A failure to close a file (e.g., due to a disk write error during flush) will not be reported to the caller, potentially leading to corrupted or incomplete bundle files being treated as valid. 💡 Suggestion Handle the errors from `Close()` calls explicitly and return them. This ensures that I/O failures during file closing are properly propagated and handled. 🔧 Suggested Fix if _, err = io.Copy(newFile, rc); err != nil { _ = newFile.Close() // Attempt to close, but the copy error is more important. return err } return newFile.Close()</code></pre>
🟡 Warning	gateway/coprocess_bundle.go:427-433	When an existing bundle fails manifest verification, the error is correctly returned, but the invalid bundle directory and its contents are left on the filesystem. This could lead to confusion or potential issues on subsequent gateway restarts if the invalid artifacts are not cleaned up. 💡 Suggestion To ensure a clean state and prevent the use of invalid artifacts, remove the bundle directory if manifest validation fails for an existing bundle. This behavior would be consistent with how newly fetched bundles are handled on verification failure. 🔧 Suggested Fix err = loadBundleManifest(bundleFs, &bundle, spec, false) if err != nil { log.WithFields(logrus.Fields{ "prefix": "main", }).Info("----> Couldn't load bundle: ", spec.CustomMiddlewareBundle, " ", err) // Also remove the invalid bundle if removeErr := bundleFs.RemoveAll(bundle.Path); removeErr != nil { bundleError(spec, removeErr, &#34;Couldn&#39;t remove invalid bundle&#34;) } return err }</code></pre>

✅ Style Check Passed

No style issues found – changes LGTM.

✅ Dependency Check Passed

No dependency issues found – changes LGTM.

✅ Connectivity Check Passed

No connectivity issues found – changes LGTM.

---

Powered by Visor from Probelabs

Last updated: 2025-10-14T15:49:54.993Z | Triggered by: synchronize | Commit: 7ef0378

[probelabs]

🔍 Code Analysis Results

---

Powered by Visor from Probelabs

Last updated: 2025-10-14T11:05:39.868Z | Triggered by: opened | Commit: 6873728

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Create parent directories safely Ensure parent directories are created before file extraction to avoid failures when nested paths exist. Use MkdirAll for directories and also before creating files to guarantee the directory tree. gateway/coprocess_bundle.go [209-213] if f.FileHeader.Mode().IsDir() { - return z.Fs.Mkdir(destPath, 0700) + return z.Fs.MkdirAll(destPath, 0700) } +if err := z.Fs.MkdirAll(filepath.Dir(destPath), 0700); err != nil { + return err +} + Suggestion importance[1-10]: 7 __ Why: Using MkdirAll for directories and ensuring parent dirs exist before file creation prevents extraction failures for nested paths; this is accurate and improves robustness of extractFile.	Medium
	Handle unexpected Stat errors Differentiate between a missing directory and other Stat errors. Only treat "exists" as success; if Stat returns an error other than not-exist, propagate it to avoid masking permission or IO issues. gateway/coprocess_bundle.go [417-435] // Skip if the bundle destination path already exists. // The bundle exists, load and return: if _, err := bundleFs.Stat(destPath); err == nil { log.WithFields(logrus.Fields{ "prefix": "main", }).Info("Loading existing bundle: ", spec.CustomMiddlewareBundle) bundle := Bundle{ Name: spec.CustomMiddlewareBundle, Path: destPath, Spec: spec, Gw: gw, } err = loadBundleManifest(bundleFs, &bundle, spec, false) if err != nil { log.WithFields(logrus.Fields{ "prefix": "main", }).Info("----> Couldn't load bundle: ", spec.CustomMiddlewareBundle, " ", err) return err } + return nil +} else if !errors.Is(err, afero.ErrFileNotFound) && !os.IsNotExist(err) { + // Unexpected error accessing destPath; propagate + return bundleError(spec, err, "Couldn't access bundle directory") +} Suggestion importance[1-10]: 5 __ Why: Differentiating not-exist from other Stat errors is reasonable, but the proposed code introduces new dependencies (os, afero.ErrFileNotFound) and behavior not present; impact is moderate.	Low
General	Restore extracted file permissions Preserve file permissions from the ZIP entry to avoid incorrect execution or access issues. After creating the file, set its mode from the header. gateway/coprocess_bundle.go [203-242] func (z *ZipBundleSaver) extractFile(f *zip.File, bundlePath string) error { if err := sanitize.ZipFilePath(f.Name, bundlePath); err != nil { return err } destPath := filepath.Join(bundlePath, f.Name) if f.FileHeader.Mode().IsDir() { - return z.Fs.Mkdir(destPath, 0700) + return z.Fs.MkdirAll(destPath, 0700) + } + + // Ensure parent directories exist + if err := z.Fs.MkdirAll(filepath.Dir(destPath), 0700); err != nil { + return err } rc, err := f.Open() if err != nil { return err } defer func() { if err := rc.Close(); err != nil { log.WithFields(logrus.Fields{ "prefix": "main", }).WithError(err).Error("Couldn't close file") } }() newFile, err := z.Fs.Create(destPath) if err != nil { return err } - defer func() { if err := newFile.Close(); err != nil { log.WithFields(logrus.Fields{ "prefix": "main", }).WithError(err).Error("Couldn't close file") } }() if _, err = io.Copy(newFile, rc); err != nil { return err } + + // Apply file mode from the archive entry if present + if mode := f.Mode(); mode != 0 { + if err := z.Fs.Chmod(destPath, mode); err != nil { + return err + } + } + return nil } Suggestion importance[1-10]: 6 __ Why: Preserving file modes after extraction is beneficial and the changes align with the surrounding code; it's a solid maintainability/usability improvement though not critical.	Low

[github-actions]

API Changes

--- prev.txt	2025-10-14 15:29:11.868302923 +0000
+++ current.txt	2025-10-14 15:29:02.334233274 +0000
@@ -9432,8 +9432,8 @@
 func (b *Bundle) AddToSpec()
     AddToSpec attaches the custom middleware settings to an API definition.
 
-func (b *Bundle) Verify() error
-    Verify performs a signature verification on the bundle file.
+func (b *Bundle) Verify(bundleFs afero.Fs) error
+    Verify performs signature verification on the bundle file.
 
 type BundleGetter interface {
 	Get() ([]byte, error)
@@ -9843,6 +9843,7 @@
 func (k *ExternalOAuthMiddleware) ProcessRequest(w http.ResponseWriter, r *http.Request, _ interface{}) (error, int)
 
 type FileBundleGetter struct {
+	Fs                 afero.Fs
 	URL                string
 	InsecureSkipVerify bool
 }
@@ -9952,7 +9953,7 @@
     EventHandlerByName is a convenience function to get event handler instances
     from an API Definition
 
-func (gw *Gateway) FetchBundle(spec *APISpec) (Bundle, error)
+func (gw *Gateway) FetchBundle(bundleFs afero.Fs, spec *APISpec) (Bundle, error)
     FetchBundle will fetch a given bundle, using the right BundleGetter.
     The first argument is the bundle name, the base bundle URL will be used as
     prefix.
@@ -12257,10 +12258,12 @@
 
 func (e *XPathExtractor) ExtractAndCheck(r *http.Request) (SessionID string, returnOverrides ReturnOverrides)
 
-type ZipBundleSaver struct{}
+type ZipBundleSaver struct {
+	Fs afero.Fs
+}
     ZipBundleSaver is a BundleSaver for ZIP files.
 
-func (ZipBundleSaver) Save(bundle *Bundle, bundlePath string, spec *APISpec) error
+func (z *ZipBundleSaver) Save(bundle *Bundle, bundlePath string, _ *APISpec) error
     Save implements the main method of the BundleSaver interface. It makes use
     of archive/zip.
 

[sonarqubecloud]

Quality Gate failed

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

See analysis details on SonarQube Cloud