[TT-14791] log error on missing vault path in api definition

[chrisanderton]

Fix gateway panic when API definition contains vault:// reference and vault path does not exist

Description

When an API definition (Classic or OAS) contains a vault:// reference in any string field, the gateway attempts to load secrets from a hardcoded vault path (secret/data/tyk-apis) during API load. If this path does not exist in Vault, the gateway crashes with a nil pointer dereference panic.

Related Issue

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

Motivation and Context

• Add a nil check for the vault secret response in replaceVaultSecrets() before accessing secret.Data
• Return a descriptive error message when the vault path does not exist
• The existing error handling in replaceSecrets() will log this error and continue, preventing the panic

How This Has Been Tested

Test cases in jira

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

Ticket Details

TT-14791

Status	In Code Review
Summary	[Innersource] Gateway panics when API definition contains vault:// reference and vault path does not exist

Generated at: 2026-02-02 08:29:24

[probelabs]

This PR addresses a critical bug that causes the gateway to panic and crash if an API definition contains a vault:// reference to a secret path that does not exist in Vault. The fix prevents this panic by introducing nil checks to gracefully handle responses for non-existent secrets, logging a descriptive error, and allowing the gateway to continue its operation without interruption.

A key architectural improvement in this PR is the introduction of a SecretReader interface. This decouples the secret loading logic from the concrete Vault implementation, allowing for robust unit testing with mocks, as demonstrated by the comprehensive new tests added for various failure scenarios.

Files Changed Analysis

• gateway/api_definition.go: Implements the core fix in the replaceVaultSecrets function. It now checks if the secret or its data is nil before access, preventing the nil pointer dereference. It also adopts the new SecretReader interface to fetch secrets.
• storage/kv/vault.go: Defines the new SecretReader interface and adds a ReadSecret method to the Vault struct, satisfying the interface and abstracting the direct Vault client call.
• gateway/api_definition_test.go: Adds a substantial new test suite (TestReplaceVaultSecrets) that uses a mock implementation of SecretReader to verify the correctness of the new error handling for various scenarios, such as a non-existent path, empty data, and Vault server errors.
• storage/kv/store_test.go: Adds new unit tests for the Vault.ReadSecret method itself, using a mocked HTTP server to validate its behavior against different Vault API responses (success, not found, server error).

Architecture & Impact Assessment

• What this PR accomplishes: It enhances the gateway's stability and resilience by preventing a crash caused by a common misconfiguration (referencing a non-existent secret), thus avoiding a potential denial-of-service.
• Key technical changes introduced:
  1. Addition of nil checks in replaceVaultSecrets to handle missing Vault paths gracefully.
  2. Introduction of the SecretReader interface to abstract Vault read operations, improving testability through dependency injection.
• Affected system components: The change is localized to the API definition loading process within the gateway. The impact is entirely positive, improving the system's robustness.

sequenceDiagram
    participant APILoader
    participant VaultClient as SecretReader
    participant Gateway

    APILoader->>VaultClient: ReadSecret("non/existent/path")

    alt Old Behavior (Panic)
        VaultClient-->>APILoader: Returns (nil, nil)
        APILoader->>APILoader: Accesses secret.Data (nil pointer dereference)
        APILoader--xGateway: Panic!
    end

    alt New Behavior (Graceful Error)
        VaultClient-->>APILoader: Returns (nil, nil)
        APILoader->>APILoader: Checks if secret is nil
        APILoader-->>Gateway: Returns descriptive error
        Note right of Gateway: Logs error and continues operation
    end

Loading

Scope Discovery & Context Expansion

• The scope of this change is well-contained within the secret replacement logic for API definitions. The error returned by replaceVaultSecrets is handled by its caller, replaceSecrets, which logs the issue and allows the gateway to continue loading the API without the secret. This prevents a single misconfigured API from causing a service-wide outage.
• The introduction of the SecretReader interface is a valuable architectural improvement. It enhances testability by allowing the Vault dependency to be mocked easily, setting a good precedent for decoupling other parts of the system from external services.

Metadata

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

---

Powered by Visor from Probelabs

Last updated: 2026-02-02T08:31:44.105Z | Triggered by: pr_updated | Commit: 6667ed0

💡 TIP: You can chat with Visor using /visor ask <your question>

[probelabs]

✅ Security Check Passed

No security issues found – changes LGTM.

Architecture Issues (1)

Severity	Location	Issue
🟢 Info	storage/kv/vault.go:13-15	The `SecretReader` interface is a good architectural improvement for decoupling, but it currently leaks an implementation detail by returning `*vaultapi.Secret`. This couples any consumer of the interface to the HashiCorp Vault client library, which could complicate swapping the secret backend for a different technology in the future. 💡 Suggestion For a more complete abstraction, the interface could be defined to return a generic type like `map[string]interface{}` or a custom struct. The implementation in `vault.go` would then be responsible for extracting the relevant data from the `*vaultapi.Secret` object. This would fully encapsulate the Vault-specific logic within the `kv` package and make consumers like `APIDefinitionLoader` completely independent of the underlying secret store's client library. Example of a more abstract interface: type SecretDataReader interface { ReadSecretData(path string) (map[string]interface{}, error) }

✅ Performance Check Passed

No performance issues found – changes LGTM.

✅ Quality Check Passed

No quality issues found – changes LGTM.

---

Powered by Visor from Probelabs

Last updated: 2026-02-02T08:31:47.169Z | Triggered by: pr_updated | Commit: 6667ed0

💡 TIP: You can chat with Visor using /visor ask <your question>

[github-actions]

API Changes

--- prev.txt	2026-02-02 08:30:19.605032471 +0000
+++ current.txt	2026-02-02 08:30:08.902040415 +0000
@@ -13913,6 +13913,11 @@
 
 func (c *Consul) Store() *consulapi.KV
 
+type SecretReader interface {
+	ReadSecret(path string) (*vaultapi.Secret, error)
+}
+    SecretReader allows mocking Vault in tests without a real instance.
+
 type Store interface {
 	Get(key string) (string, error)
 	// Put writes a value to the KV store
@@ -13929,7 +13934,7 @@
 type Vault struct {
 	// Has unexported fields.
 }
-    Vault is an implementation of a KV store which uses Consul as it's backend
+    Vault is an implementation of a KV store which uses Vault as its backend
 
 func (v *Vault) Client() *vaultapi.Client
 
@@ -13937,6 +13942,8 @@
 
 func (v *Vault) Put(key string, value string) error
 
+func (v *Vault) ReadSecret(path string) (*vaultapi.Secret, error)
+
 # Package: ./storage/mock
 
 package mock // import "github.com/TykTechnologies/tyk/storage/mock"

[chrisanderton]

Not easy to add test case for this new nil check without broader refactoring. This ticket is a targetted nil check bug fix, the change is low risk vs. the risk introduced by a refactor to add test coverage for 3 lines of code. Test coverage for vault should be a factor when kv interfaces are consolidated (a different piece of work to this bug fix).

[pvormste]

While it is a small change, we still need to provide coverage. No major refactoring is needed, look at this code snippet right before the change:

secret, err := a.Gw.vaultKVStore.(*kv.Vault).Client().Logical().Read(vaultSecretPath + prefixKeys)
if err != nil {
	return err
}

This needs to be changed, so that it can return a dummy implementation that can be used for tests.
One way could be to have an extended interface like

type VaultKVStore interface {
    kv.Store
    Client() *api.Client
}

and then add a function to GW that handles the interface/implemetation provision:

func (gw *Gateway) GetVaultKVStore() (vaultStore VaultKVStore, ok bool)

Use the dummy implemetation then to setup your tests

[chrisanderton]

@pvormste how about just doing the read path?

In storage/kv/vault.go:

type SecretReader interface {
    ReadSecret(path string) (*vaultapi.Secret, error)
}

func (v *Vault) ReadSecret(path string) (*vaultapi.Secret, error) {
    return v.client.Logical().Read(path)
}

In gateway/api_definition.go:

vault, ok := a.Gw.vaultKVStore.(kv.SecretReader)
...
secret, err := vault.ReadSecret(vaultSecretPath + prefixKeys)

keeps the interface minimal vs. full client

[chrisanderton]

Path traversal check is outside the scope of this ticket, we've already ballooned in complexity vs the initial nil check only.

[chrisanderton]

Note from/to visor - it initially suggested a path traversal risk, then disagreed with the todo. Comment has been removed.

The TODO comment suggests a path traversal vulnerability exists, but the implementation in replaceVaultSecrets does not seem to support this. The Vault path is hardcoded to secret/data/tyk-apis, and keys from the API definition are not used to construct this path. This comment is misleading and could cause confusion or misdirect future security efforts.

[edsonmichaque]

LGTM!
@pvormste since you raised some questions, would you give an approval if you are happy?

[pvormste]

Thank you, this looks much better now, great job!
Please note that tests are absolutely required even for such small changes, otherwise we will not be able to catch possible regressions in the future.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[edsonmichaque]

/release to release-5.8

[probelabs]

✅ Cherry-pick successful. A PR was created: #7726