[TT-1417] Custom Key Registration Failing with sha256 enabled when OrgID is empty #7462

shults · 2025-10-17T13:47:10Z

User description

Summary	Custom Key Registration Failing with sha256 enabled when OrgID is empty
Type	Bug
Status	In Code Review
Points	N/A
Labels	2025_long_tail, 2025_r6_candidate, A, AI-Complexity-Low, AI-Priority-Medium, America's, CSE, Gold, codilime_refined, customer_bug, github, jira_escalated

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

Bug fix, Tests

Description

Fix key GET to try org-scoped formats
Add comprehensive hash-function test matrix
Support lookup without org_id using returned key
Preserve legacy key format fallback

Diagram Walkthrough

flowchart LR
  GETReq["GET /tyk/keys/{id}"] -- "try org-scoped key" --> OrgScoped["gw.generateToken(orgID, key)"]
  GETReq -- "fallback to raw key" --> RawKey["origKeyName"]
  OrgScoped -- "found" --> OK["200 OK"]
  RawKey -- "found" --> OK
  RawKey -- "not found" --> NotFound["404 Not Found"]

File Walkthrough

Relevant files

Bug fix

api.go `Robust key GET with org-scoped and raw fallbacks` gateway/api.go On GET, try `generateToken(orgID, origKeyName)`. Fallback to `origKeyName` if first fails. Replace single attempt with ordered tries loop. Targets cases with empty or unknown `org_id`.	+9/-6

Tests

api_keys_hander_test.go `Tests cover key retrieval across hash functions` gateway/api_keys_hander_test.go Add table-driven tests for hash functions. Verify GET with correct org_id succeeds. Ensure wrong/empty org_id returns 404. Validate GET without org_id using returned key works.	+142/-0

buger · 2025-10-17T13:47:29Z

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

github-actions · 2025-10-17T13:48:15Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Logic Change The GET path now tries two key variants only when `origKeyName` is non-empty and does not fall back to legacy format based on `hashKeyFunction`. Validate that this behavior still supports all intended legacy/hashed retrieval scenarios and does not regress cases where `keyName` was previously used directly. if origKeyName != "" { keyTries: for _, key := range []string{ gw.generateToken(orgID, origKeyName), origKeyName, } { if obj, code = gw.handleGetDetail(key, apiID, orgID, isHashed); code == http.StatusOK { break keyTries } } Test Coverage Gap Tests cover retrieval by returned key without org_id and by raw key with correct org_id, but they do not verify the dual-lookup sequence when `origKeyName` is set, nor behavior when `HashKeys` is on with different functions for missing/empty org IDs. Consider adding cases for each hashing function to assert both branches (generated token and original key) are exercised. func TestKeyHandlerHandler(t testing.T) { for _, tc := range []hashKeyFunction{ hashKeyFunctionNone, hashKeyFunctionMurmur64, hashKeyFunctionMurmur128, hashKeyFunctionSha256, } { t.Run(tc.name(), func(t testing.T) { runTestKeyHandler(t, tc) }) } } func runTestKeyHandler(t testing.T, hashFunc hashKeyFunction) { t.Helper() ts := StartTest(func(cnf config.Config) { cnf.HashKeys = hashFunc != hashKeyFunctionNone cnf.HashKeyFunction = hashFunc.value }) t.Cleanup(ts.Close) const keyId = "my-proper-id" const orgId = "some-org" keyDto := createKey(t, ts, keyId, orgId) t.Run("direct get works if provided proper org id", func(t testing.T) { _, err := ts.Run(t, test.TestCase{ Path: fmt.Sprintf("/tyk/keys/%s?org_id=%s", keyId, orgId), Method: http.MethodGet, Code: http.StatusOK, AdminAuth: true, }) assert.NoError(t, err) }) t.Run("access with responded does not work if wrong org_id provided", func(t testing.T) { _, err := ts.Run(t, test.TestCase{ Path: fmt.Sprintf("/tyk/keys/%s?org_id=%s", keyId, ""), Method: http.MethodGet, Code: http.StatusNotFound, AdminAuth: true, }) assert.NoError(t, err) _, err = ts.Run(t, test.TestCase{ Path: fmt.Sprintf("/tyk/keys/%s?org_id=%s", keyId, "wrong-org-id"), Method: http.MethodGet, Code: http.StatusNotFound, AdminAuth: true, }) assert.NoError(t, err) }) t.Run("access with responded key works without passing org_id", func(t *testing.T) { _, err := ts.Run(t, test.TestCase{ Path: fmt.Sprintf("/tyk/keys/%s", keyDto.Key), Method: http.MethodGet, Code: http.StatusOK, AdminAuth: true, }) assert.NoError(t, err) }) }

github-actions · 2025-10-17T13:48:40Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Avoid empty-org token generation Guard against generating and attempting a key with an empty `orgID`, as `gw.generateToken` may output a malformed token and unintentionally succeed or cause collisions. Only include the generated token candidate when `orgID` is non-empty; otherwise, try `origKeyName` alone. gateway/api.go [1605-1616] if origKeyName != "" { keyTries: - for _, key := range []string{ - gw.generateToken(orgID, origKeyName), - origKeyName, - } { + candidates := []string{origKeyName} + if orgID != "" { + candidates = append([]string{gw.generateToken(orgID, origKeyName)}, candidates...) + } + for _, key := range candidates { if obj, code = gw.handleGetDetail(key, apiID, orgID, isHashed); code == http.StatusOK { break keyTries } } } else { Suggestion importance[1-10]: 6 __ Why: The suggestion is contextually valid: it targets the new GET logic and avoids generating a token when `orgID` is empty, which could be safer and clearer. It's a moderate improvement in robustness, though not critical.	Low

github-actions · 2025-10-17T13:48:40Z

API Changes

no api changes detected

probelabs · 2025-10-17T13:56:11Z

🔍 Code Analysis Results

This PR fixes a bug where retrieving a custom key via the Gateway Admin API fails when key hashing (e.g., sha256) is enabled and the org_id is not provided in the GET request.

The fix refactors the key retrieval logic to be more robust. It now systematically attempts to find the key by trying two potential formats in order:

A generated token using the org_id and the original key name.
The raw key ID as provided in the request.

This ensures the key can be found regardless of whether the requestor provides the raw key ID or the hashed key, with or without an org_id. A comprehensive test suite is added to validate this fix across different hashing algorithms (none, murmur64, murmur128, sha256).

Files Changed Analysis

gateway/api.go: Modified the keyHandler function to implement a more resilient key lookup strategy for GET requests. The logic now iterates through potential key formats (hashed and raw) instead of a simple one-time fallback.
gateway/api_keys_hander_test.go: Added a new test file with 142 lines. This file introduces a thorough test suite for the keyHandler's GET functionality, covering various key hashing configurations and scenarios involving the presence or absence of org_id.

Architecture & Impact Assessment

Accomplishment: Fixes a critical bug in the Admin API that prevented retrieval of custom keys when hash_keys is enabled and org_id is omitted from the request.
Technical Changes: The key lookup logic for GET /tyk/keys/{key} is changed from a simple fallback to an explicit loop that tries finding the key first by its potentially hashed form (gw.generateToken(orgID, origKeyName)) and then by its original raw form (origKeyName).
Affected Components: Gateway Admin API, specifically the /tyk/keys/{key_id} endpoint. This impacts API key management operations.

Flow Diagram:

sequenceDiagram
    participant Client
    participant Gateway Admin API
    participant Key Storage

    Client->>Gateway Admin API: GET /tyk/keys/{key_id}
    Gateway Admin API->>Gateway Admin API: Attempt 1: Look up hashed key (generateToken(org_id, key_id))
    Gateway Admin API->>Key Storage: Get(hashed_key)
    alt Key Found
        Key Storage-->>Gateway Admin API: Key Data
        Gateway Admin API-->>Client: 200 OK with Key Data
    else Key Not Found
        Gateway Admin API->>Gateway Admin API: Attempt 2: Look up raw key (key_id)
        Gateway Admin API->>Key Storage: Get(raw_key_id)
        alt Key Found
            Key Storage-->>Gateway Admin API: Key Data
            Gateway Admin API-->>Client: 200 OK with Key Data
        else Key Not Found
            Key Storage-->>Gateway Admin API: Not Found
            Gateway Admin API-->>Client: 404 Not Found
        end
    end

Scope Discovery & Context Expansion

The change is well-contained within the keyHandler in gateway/api.go. Its primary impact is on the GET operation for single keys.
The newly added tests in gateway/api_keys_hander_test.go provide excellent context, demonstrating the intended behavior with different hashing configurations and confirming that the fix works as expected.
The change does not appear to affect other parts of the system, as it only modifies the lookup logic for an existing API endpoint without altering core key generation or storage mechanisms.

Metadata

Review Effort: 2 / 5
Primary Label: bug

Powered by Visor from Probelabs

Last updated: 2025-10-17T13:56:10.918Z | Triggered by: opened | Commit: f723330

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

probelabs · 2025-10-17T13:56:11Z

🔍 Code Analysis Results

Security Issues (1)

Severity	Location	Issue
🟢 Info	`AI_RESPONSE:1`	After a thorough security analysis of the pull request, I have found no vulnerabilities in the proposed changes. Detailed Analysis The core of the change is in `gateway/api.go`, where the logic for handling `GET` requests in the `keyHandler` function is refactored. The previous implementation and the new one both attempt to find an API key by first trying a hashed version of the key (if hashing is enabled) and then falling back to the original key name. The security of this operation relies on the `handleGetDetail` function, which is responsible for fetching the key's session details. My analysis of `handleGetDetail` confirms that it correctly uses the `orgID` from the request to scope the database lookup for the key. The call to `gw.GlobalSessionManager.SessionDetail(orgID, sessionKey, byHash)` ensures that a key is only retrieved if it belongs to the specified organization. This prevents any cross-organization data leakage. The refactoring in this pull request does not alter this fundamental security control. It merely changes the way the two key variations (hashed and original) are tried. Both attempts are still subject to the same authorization check within `handleGetDetail`. The newly added test file, `gateway/api_keys_hander_test.go`, further strengthens the security posture by adding explicit test cases that verify: A key can be successfully retrieved with the correct `org_id`. An attempt to retrieve a key with a wrong or missing `org_id` correctly fails with a "Not Found" error. Retrieving a key by its full token value (which is how clients would use it) works as expected. These tests help prevent future regressions in this critical security logic. Conclusion The code changes are a safe refactoring aimed at fixing a bug and do not introduce any new security risks. The existing authorization mechanisms remain effective. Therefore, I have no security issues to report.

Architecture Issues (1)

Severity	Location	Issue
🟢 Info	`AI_RESPONSE:1`	The architectural changes in this pull request are sound and improve the key retrieval logic. The previous implementation had a conditional fallback that was less robust, especially for cases where the `OrgID` was not provided on a GET request for a key that was created with one. The new implementation in `gateway/api.go` simplifies the logic by using a loop to try potential key formats: A key generated using the provided `orgID` and the key name from the URL path (`origKeyName`). This covers retrieving a key by its ID when hashing is enabled. The raw `origKeyName` from the URL path. This covers retrieving a key by its full token value (which might be a hashed value) or retrieving a key in a legacy/unhashed format. This approach is more robust and extensible. If other key formats are needed in the future, they can be easily added to the list of candidates to try. This is a good example of simplifying logic by replacing complex conditional branches with a simple iteration over possibilities. Key Architectural Improvements: Simplicity: The `for` loop is more straightforward to understand than the previous `if...else` structure for falling back to a legacy key format. Robustness: It correctly handles more cases, such as when the full key token is passed in the URL, or when the `org_id` is omitted from a request for a key that has one. Extensibility: The pattern of iterating through a slice of potential keys makes it easy to add more fallbacks or key formats in the future without major refactoring. The accompanying test file, `gateway/api_keys_hander_test.go`, is comprehensive. It tests the key retrieval logic against multiple hashing algorithms (`none`, `murmur64`, `murmur128`, `sha256`) and validates the scenarios that this change is intended to fix. This provides strong confidence that the change is correct and does not introduce regressions. Overall, the architectural decisions are pragmatic and represent a clear improvement to the codebase. No issues are found.

Performance Issues (1)

Severity	Location	Issue
🟡 Warning	`gateway/api.go:1608-1614`	The introduction of a loop for key lookups causes two sequential calls to `gw.handleGetDetail` when a key is not found. Since `handleGetDetail` performs a storage lookup (e.g., Redis GET), this change doubles the latency for "key not found" responses. Under high load or during security scans, this could increase latency and put additional strain on the storage backend. 💡 Suggestion A more efficient approach would be to use a batch operation (like Redis MGET) to check for both key variants in a single round trip.

Quality Issues (1)

Severity	Location	Issue
🟡 Warning	`gateway/api_keys_hander_test.go:63`	The new tests do not cover the scenario described in the PR title: 'Custom Key Registration Failing with sha256 enabled when OrgID is empty'. All tests are run with a non-empty `orgId` ('some-org'), so the fix for the empty `orgId` case is not verified. 💡 Suggestion Add a new test case within `TestKeyHandlerHandler` that specifically tests the key creation and retrieval when `orgId` is an empty string. This will ensure the bug is actually fixed and prevent regressions.

Style Issues (3)

Severity	Location	Issue
🟡 Warning	`gateway/api.go:1606-1614`	The `for` loop with a labeled `break` is an overly complex and unconventional way to attempt fetching a key with two different values. This structure harms readability. A simpler sequence of `if` statements would be more idiomatic and easier to understand. 💡 Suggestion Replace the loop with a more direct sequence of attempts. For example: `obj, code = gw.handleGetDetail(gw.generateToken(orgID, origKeyName), apiID, orgID, isHashed); if code != http.StatusOK { obj, code = gw.handleGetDetail(origKeyName, apiID, orgID, isHashed) }`
🟡 Warning	`gateway/api_keys_hander_test.go:3-19`	The file contains multiple `import` blocks. Standard Go formatting, as enforced by tools like `goimports`, consolidates imports into a single block, grouped by standard library, third-party, and internal packages. This should be fixed for consistency. 💡 Suggestion Run `goimports` on the file to automatically fix the formatting.
🟡 Warning	`gateway/api_keys_hander_test.go:73`	The variable names `keyId` and `orgId` are used, which is inconsistent with the standard Go convention of capitalizing acronyms like "ID" (e.g., `keyID`, `orgID`). This is also inconsistent with other parts of the codebase, such as the `user.SessionState` struct which uses `OrgID`. 💡 Suggestion Rename `keyId` to `keyID` and `orgId` to `orgID` throughout the new test file for consistency.

✅ 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-17T13:56:11.570Z | Triggered by: opened | Commit: f723330

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

add fix + tests

f723330

buger added bug Tyk Product Development labels Oct 17, 2025

github-actions bot added the Review effort 2/5 label Oct 17, 2025

github-actions bot added the review/effort:2 label Oct 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TT-1417] Custom Key Registration Failing with sha256 enabled when OrgID is empty #7462

[TT-1417] Custom Key Registration Failing with sha256 enabled when OrgID is empty #7462

Uh oh!

shults commented Oct 17, 2025 •

edited by github-actions bot

Loading

Uh oh!

buger commented Oct 17, 2025

Uh oh!

github-actions bot commented Oct 17, 2025

Uh oh!

github-actions bot commented Oct 17, 2025

Uh oh!

github-actions bot commented Oct 17, 2025

Uh oh!

probelabs bot commented Oct 17, 2025

Uh oh!

probelabs bot commented Oct 17, 2025

Detailed Analysis

Conclusion

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[TT-1417] Custom Key Registration Failing with sha256 enabled when OrgID is empty #7462

Are you sure you want to change the base?

[TT-1417] Custom Key Registration Failing with sha256 enabled when OrgID is empty #7462

Uh oh!

Conversation

shults commented Oct 17, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Checklist

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

buger commented Oct 17, 2025

Uh oh!

github-actions bot commented Oct 17, 2025

PR Reviewer Guide 🔍

Uh oh!

github-actions bot commented Oct 17, 2025

PR Code Suggestions ✨

Uh oh!

github-actions bot commented Oct 17, 2025

Uh oh!

probelabs bot commented Oct 17, 2025

🔍 Code Analysis Results

Files Changed Analysis

Architecture & Impact Assessment

Scope Discovery & Context Expansion

Uh oh!

probelabs bot commented Oct 17, 2025

🔍 Code Analysis Results

Security Issues (1)

Detailed Analysis

Conclusion

Architecture Issues (1)

Performance Issues (1)

Quality Issues (1)

Style Issues (3)

✅ Dependency Check Passed

✅ Connectivity Check Passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shults commented Oct 17, 2025 •

edited by github-actions bot

Loading