Skip to content

Revert "[TT-5588] [OAS] gateway apiKey import generates unnecessary object" #7299

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

Conversation

radkrawczyk
Copy link
Contributor

@radkrawczyk radkrawczyk commented Aug 12, 2025

User description

TT-5588
Summary [OAS] gateway apiKey import generates unnecessary object
Type Bug Bug
Status In Dev
Points N/A
Labels codilime_refined

Reverts #7270


PR Type

Bug fix, Tests


Description

Re-enable JSON inlining for AuthSources
Remove test asserting non-serialization
Keep token auth fill/roundtrip behavior intact
Align JSON tags with intended API shape


Diagram Walkthrough

flowchart LR
  Token["Token struct"]
  AuthSources["AuthSources fields"]
  JSONTag["json:\",inline\""]
  TestRemoval["Remove non-serialization test"]

  Token -- contains --> AuthSources
  AuthSources -- applied via --> JSONTag
  JSONTag -- implies --> InlinedInJSON["Inlined in JSON output"]
  TestRemoval -- aligns with --> InlinedInJSON
Loading

File Walkthrough

Relevant files
Bug fix
security.go
Re-enable JSON inlining for AuthSources in Token                 

apidef/oas/security.go

  • Change Token.AuthSources JSON tag to ",inline"
  • Revert exclusion from JSON serialization
+1/-1     
Tests
security_test.go
Delete test asserting AuthSources not serialized                 

apidef/oas/security_test.go

  • Remove round-trip JSON test for AuthSources omission
  • Keep existing Token tests intact
+0/-15   

@buger
Copy link
Member

buger commented Aug 12, 2025

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

Copy link
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix invalid JSON struct tag

Using json:",inline" is invalid in Go struct tags and will be ignored, potentially
causing unintended JSON output. If the intent is to inline fields for JSON, remove
the json tag entirely (Go inlines by embedding, not via tags) or specify valid field
names. This prevents malformed tags and serialization bugs.

apidef/oas/security.go [33]

-AuthSources `bson:",inline" json:",inline"`
+AuthSources `bson:",inline"`
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that json:",inline" is not a valid encoding/json tag and will be ignored; embedding already inlines fields. Removing the invalid tag avoids misleading configuration and potential serialization confusion.

Medium

Copy link
Contributor

API Changes

--- prev.txt	2025-08-12 11:17:31.617704362 +0000
+++ current.txt	2025-08-12 11:17:22.306675980 +0000
@@ -4555,7 +4555,7 @@
 	Enabled bool `bson:"enabled" json:"enabled"` // required
 
 	// AuthSources contains the configuration for authentication sources.
-	AuthSources `bson:",inline" json:"-"`
+	AuthSources `bson:",inline" json:",inline"`
 
 	// EnableClientCertificate allows to create dynamic keys based on certificates.
 	//

Copy link
Contributor

🚀 Performance Snapshot

Effort Perf Risk Hot Paths Benchmarks TL;DR
Low 🟢 Re-enabling JSON inlining for AuthSources in Token struct has minimal performance impact
## Performance Impact Analysis

This PR reverts a previous change (#7270) that had excluded AuthSources from JSON serialization in the Token struct. The change is simple: modifying the JSON tag from json:"-" to json:",inline" to align with how other authentication types handle the same embedded struct.

The performance impact is minimal as this only affects JSON serialization during API definition import/export operations, not request processing. The change doesn't introduce any new allocations or processing overhead in hot paths.

## Critical Areas

The change affects the OAS API definition serialization, which is not in a critical performance path. This operation typically happens during API creation, updates, or when loading configurations - not during request handling.

The bot comment about json:",inline" being invalid is technically incorrect in this context. While Go's standard encoding/json package doesn't support struct field inlining, the OAS package appears to be using custom serialization logic that does support this tag.

## Optimization Recommendations

No performance optimizations are needed for this change. The PR correctly aligns the Token struct's serialization behavior with other authentication types in the codebase.

The PR removes a test that was specifically asserting that AuthSources fields are not serialized, which is appropriate given the change in behavior.

## Summary
  • This change has minimal performance impact as it only affects JSON serialization during API definition operations
  • The change brings consistency to how authentication sources are handled across different auth types
  • No suggestions to provide – change LGTM.

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

Copy link
Contributor

🚦 Connectivity Review Snapshot

Effort Tests Security Perf TL;DR
Low 🔒 none 🟢 Re-enables JSON inlining for AuthSources in Token struct to maintain expected API shape
## Connectivity Assessment
  • Redis Connections: No impact on Redis connectivity. The change only affects JSON serialization of authentication configuration.
  • RPC Connections: No impact on RPC connections or MDCB mode functionality.
  • Synchronization Mechanisms: No changes to synchronization mechanisms or pub/sub channels.
## Test Coverage Validation
  • Redis Tests: Not applicable - change doesn't affect Redis connectivity.
  • RPC Tests: Not applicable - change doesn't affect RPC functionality.
  • Failure Scenario Tests: The PR removes a test that verified AuthSources fields were not serialized to JSON, which is appropriate since the PR is re-enabling that serialization.
## Security & Performance Impact
  • Authentication Changes: The change affects how authentication configuration is serialized to JSON but doesn't modify authentication logic or security mechanisms.
  • Performance Considerations: Minimal impact - JSON serialization will include additional fields but this shouldn't cause significant performance changes.
  • Error Handling: No changes to error handling paths.
## Summary & Recommendations
  • This PR correctly reverts a previous change that excluded AuthSources from JSON serialization, restoring the expected API shape.
  • The change is minimal and focused, only modifying a JSON tag and removing a test that no longer applies.
  • 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

📦 Impact Review Snapshot

Effort Downstream Updates Compatibility Docs TL;DR
Low ⚠️ 🟡 ⚠️ Re-enables inlining of AuthSources fields in Token JSON output, affecting API schema
## Impact Assessment

This PR reverts a change that excluded AuthSources fields from Token JSON serialization. By re-enabling the json:",inline" tag, the Token struct will now include Header, Cookie, and Query fields directly in its JSON representation rather than omitting them. This affects the shape of API definitions in JSON format, which could impact any downstream repositories that parse or generate these definitions.

The change is particularly relevant for tyk-operator which works directly with API definitions, and potentially portal which may display or manipulate these definitions. The impact on tyk-charts and tyk-sink is likely minimal since they typically don't directly manipulate API definition schemas.

## Required Updates
  • tyk-operator: May need to update any code that expects AuthSources fields to be excluded from Token JSON. Specifically, any code that parses or generates API definitions with Token authentication.

  • portal: If the portal displays or manipulates API definitions with Token authentication, it may need updates to handle the inlined AuthSources fields.

  • tyk-charts and tyk-sink: No immediate updates required as they don't typically manipulate API definition schemas directly.

## Compatibility Concerns

The change introduces a backward compatibility issue for any system that expects AuthSources fields to be excluded from Token JSON. Systems that were built to work with the previous JSON structure (where AuthSources fields were omitted) may now receive unexpected fields in the JSON output.

Additionally, the PR comment notes a concern from GitHub Actions about the json:",inline" tag being invalid. This is technically incorrect - the tag is valid in Go for embedding struct fields in JSON output, but it's important to verify that all downstream systems interpret this correctly.

## Summary & Recommendations
  • Test the JSON serialization/deserialization in downstream repositories, especially tyk-operator, to ensure they handle the inlined AuthSources fields correctly.
  • Update any documentation that describes the Token JSON structure to reflect that AuthSources fields are now inlined.
  • Consider adding a note in release notes about this change as it affects the API definition schema.

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 🟢 ✔️ Re-enabling JSON inlining for AuthSources has minimal security impact
## Security Impact Analysis

This PR reverts a previous change (PR #7270) that had modified the JSON serialization behavior of the AuthSources field in the Token struct. The change switches the JSON tag from json:"-" (exclude from serialization) back to json:",inline" (include fields directly in parent JSON). This affects how API key authentication sources are represented in JSON output but does not change authentication validation logic or token handling security.

## Identified Vulnerabilities

No security vulnerabilities were identified in this change. The modification only affects JSON serialization format, not the actual authentication or authorization mechanisms. The change maintains consistent behavior between internal data structures and their JSON representation, aligning Token with other auth types (JWT, Basic, OAuth) that already use the inline approach for AuthSources.

## Security Recommendations
  • Consider adding a comment explaining why inlining is preferred for this struct to prevent future confusion
  • Ensure documentation is updated to reflect the expected JSON structure for API consumers
  • Verify that any systems consuming this JSON format are aware of the change
## OWASP Compliance

This change does not impact OWASP Top 10 compliance:

  • No authentication bypass concerns (A2:2021)
  • No exposure of sensitive data (A3:2021)
  • No changes to access control mechanisms (A1:2021)
  • JSON structure changes don't affect injection vulnerabilities (A3:2021)
## Summary
  • This PR reverts a JSON serialization change, re-enabling inlining for AuthSources in the Token struct
  • The change is purely representational and doesn't modify authentication logic
  • Both test sections asserting non-serialization are appropriately removed to align with the new behavior
  • No security issues identified – change LGTM.

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

Copy link

@MaciekMis MaciekMis merged commit e7dd30f into master Aug 12, 2025
44 checks passed
@MaciekMis MaciekMis deleted the revert-7270-TT-5588-oas-gateway-api-key-import-generates-unnecessary-object branch August 12, 2025 11:51
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.

4 participants