Skip to content
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

[TT-11762] fix nil err return #6365

Merged
merged 2 commits into from
Jun 20, 2024
Merged

Conversation

jeffy-mathew
Copy link
Contributor

@jeffy-mathew jeffy-mathew commented Jun 20, 2024

User description

Description

Fix sonarcloud reported issue where err is returned when it is nil

Related Issue

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

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


Description

  • Fixed an issue where a nil error was incorrectly handled in the validateSignature method within the ProcessRequest function in gateway/mw_auth_key.go.

Changes walkthrough 📝

Relevant files
Bug fix
mw_auth_key.go
Fix nil error check in `validateSignature` method               

gateway/mw_auth_key.go

  • Fixed the condition to correctly handle non-nil errors in
    validateSignature.
  • +1/-1     

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

    @jeffy-mathew jeffy-mathew changed the title [TT-11762] fix nil err check [TT-11762] fix nil err return Jun 20, 2024
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 1
    🧪 Relevant tests No
    🔒 Security concerns No
    ⚡ Key issues to review None

    Copy link
    Contributor

    API Changes

    no api changes detected

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Handle potential errors from storage.TokenID to prevent undefined behavior

    Consider handling the case where err is not nil after the call to storage.TokenID(key).
    This would improve error handling and avoid proceeding with an undefined keyID.

    gateway/mw_auth_key.go [175-177]

     keyID, err := storage.TokenID(key)
    -if err == nil {
    -    err, statusCode := k.validateSignature(r, keyID)
    +if err != nil {
    +    return err, http.StatusBadRequest // or another appropriate error code
    +}
    +err, statusCode := k.validateSignature(r, keyID)
     
    Suggestion importance[1-10]: 10

    Why: This suggestion improves error handling by ensuring that the function does not proceed with an undefined keyID, which could lead to unpredictable behavior. This is a crucial fix for stability.

    10
    Possible bug
    Ensure a meaningful status code is returned when an error occurs

    The condition if err != nil should be followed by a return statement that includes a
    default or meaningful status code if statusCode is not set, to avoid returning an
    uninitialized statusCode.

    gateway/mw_auth_key.go [178-179]

     if err != nil {
    +    if statusCode == 0 {
    +        statusCode = http.StatusInternalServerError // or another appropriate default code
    +    }
         return err, statusCode
     }
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential bug where an uninitialized status code might be returned, ensuring that a meaningful status code is always provided. This is important for robust error handling.

    9
    Best practice
    Adjust the order of return values from validateSignature to follow Go's convention

    The function validateSignature should return values in the order (statusCode, err) to
    follow Go's convention of returning the error as the last value. This will also require
    updating the handling of the returned values.

    gateway/mw_auth_key.go [177]

    -err, statusCode := k.validateSignature(r, keyID)
    +statusCode, err := k.validateSignature(r, keyID)
     
    Suggestion importance[1-10]: 8

    Why: This suggestion follows Go's convention of returning the error as the last value, which improves code readability and consistency. It is a best practice but not a critical fix.

    8
    Maintainability
    Refactor the nested conditionals to improve code readability and maintainability

    Refactor the nested conditionals for better readability and maintainability. Flatten the
    structure by handling error cases first and returning early.

    gateway/mw_auth_key.go [176-180]

    -if err == nil {
    -    err, statusCode := k.validateSignature(r, keyID)
    -    if err != nil {
    -        return err, statusCode
    -    }
    +if err != nil {
    +    return nil, http.StatusOK // Assuming no error means success
    +}
    +statusCode, err := k.validateSignature(r, keyID)
    +if err != nil {
    +    return err, statusCode
     }
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves code readability and maintainability by flattening nested conditionals. While it enhances the code structure, it is not as critical as fixing functional issues.

    7

    Copy link
    Contributor

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    @jeffy-mathew jeffy-mathew enabled auto-merge (squash) June 20, 2024 16:02
    @TykTechnologies TykTechnologies deleted a comment from github-actions bot Jun 20, 2024
    Copy link
    Contributor

    💥 CI tests failed 🙈

    git-state

    all ok

    Please look at the run or in the Checks tab.

    @jeffy-mathew jeffy-mathew merged commit cadda82 into master Jun 20, 2024
    35 checks passed
    @jeffy-mathew jeffy-mathew deleted the chores/TT-11762/sonarcloud-nil-err branch June 20, 2024 16:39
    Copy link

    sonarcloud bot commented Jun 20, 2024

    3 similar comments
    Copy link

    sonarcloud bot commented Jun 20, 2024

    Copy link

    sonarcloud bot commented Jun 20, 2024

    Copy link

    sonarcloud bot commented Jun 20, 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.

    None yet

    2 participants