Skip to content

[TT-11997] Header case insensitivity#6316

Merged
buraksezer merged 2 commits into
masterfrom
fix/TT-11997/header-case-insensitivity
May 29, 2024
Merged

[TT-11997] Header case insensitivity#6316
buraksezer merged 2 commits into
masterfrom
fix/TT-11997/header-case-insensitivity

Conversation

@buraksezer

@buraksezer buraksezer commented May 29, 2024

Copy link
Copy Markdown
Contributor

User description

This PR addresses the problem explained here https://tyktech.atlassian.net/browse/TT-11997?focusedCommentId=54161


PR Type

Bug fix, Tests


Description

  • Added a new test case to verify request headers rewrite with case insensitivity.
  • Canonicalized header names in the request_headers_rewrite configuration to ensure consistent handling.
  • Removed unnecessary Header.Del operation in the header rewrite logic.

Changes walkthrough 📝

Relevant files
Tests
mw_graphql_test.go
Add test for request headers rewrite case insensitivity   

gateway/mw_graphql_test.go

  • Added a new test case to verify request headers rewrite with case
    insensitivity.
  • Configured the test to check if headers are correctly rewritten before
    hitting the upstream.
  • +47/-0   
    Bug fix
    reverse_proxy.go
    Canonicalize header names in request headers rewrite configuration

    gateway/reverse_proxy.go

  • Canonicalized header names in the request_headers_rewrite
    configuration.
  • Ensured headers are rewritten using the canonical MIME header key
    format.
  • +7/-1     
    transport.go
    Remove unnecessary header deletion in rewrite rules           

    internal/graphengine/transport.go

  • Removed unnecessary Header.Del operation.
  • Ensured header rewrite rules are applied correctly without deletion.
  • +2/-1     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Description updated to latest commit (fd64dfb)

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves changes across multiple files with modifications in test cases and core logic handling headers. Understanding the context and ensuring the changes meet the requirements needs a careful review.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The test case in mw_graphql_test.go assumes headers are case-insensitive, but the actual implementation in reverse_proxy.go might not handle all edge cases if headers are manipulated elsewhere in the code.

    🔒 Security concerns

    No

    Code feedback:
    relevant filegateway/reverse_proxy.go
    suggestion      

    Consider adding error handling for the textproto.CanonicalMIMEHeaderKey function to catch any potential issues with header key transformations. This is important to ensure robustness in header processing. [important]

    relevant linerequestHeadersRewrite[textproto.CanonicalMIMEHeaderKey(key)] = value

    relevant fileinternal/graphengine/transport.go
    suggestion      

    Ensure that the removal of r.Header.Del(key) does not introduce any side effects where headers might need explicit deletion to prevent processing old values in subsequent requests. This is important for maintaining correct header states. [important]

    relevant line- r.Header.Del(key)

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    API Changes

    no api changes detected

    @github-actions

    Copy link
    Copy Markdown
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Add a test case to ensure header case insensitivity is handled correctly

    Consider adding a test case to verify the behavior when headers are provided in
    non-canonical form, as the implementation now explicitly handles case insensitivity using
    textproto.CanonicalMIMEHeaderKey.

    gateway/mw_graphql_test.go [828-844]

     _, _ = g.Run(t, []test.TestCase{
         {
             Data:   request,
             Method: http.MethodPost,
             Headers: map[string]string{
    -            "X-Tyk-Key":   "tyk-value",
    -            "X-Other-Key": "other-value",
    -            "X-Tyk-Test":  "value-from-consumer",
    +            "x-tyk-key":   "tyk-value",
    +            "x-other-key": "other-value",
    +            "x-tyk-test":  "value-from-consumer",
             },
             Code:      http.StatusOK,
             BodyMatch: `{"data":{"hello":"World","httpMethod":"POST"}}`,
             HeadersMatch: map[string]string{
                 "X-Tyk-Key":   "tyk-value",
                 "X-Other-Key": "other-value",
                 "X-Tyk-Test":  "value-from-rewrite-config",
             },
         },
     }...)
     
    Suggestion importance[1-10]: 9

    Why: The suggestion is relevant and useful as it ensures that the new functionality handling case insensitivity in headers is properly tested. This enhances the robustness of the test suite.

    9

    @buraksezer
    buraksezer enabled auto-merge (squash) May 29, 2024 13:14
    @sonarqubecloud

    Copy link
    Copy Markdown

    @buraksezer
    buraksezer merged commit 853b395 into master May 29, 2024
    @buraksezer
    buraksezer deleted the fix/TT-11997/header-case-insensitivity branch May 29, 2024 13:25
    nerdydread pushed a commit that referenced this pull request Sep 6, 2024
    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.

    2 participants