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-10797 Hex validation for long keys on TokenOrg #5876

Merged
merged 6 commits into from
Dec 18, 2023
Merged

Conversation

tbuchaillot
Copy link
Contributor

@tbuchaillot tbuchaillot commented Dec 15, 2023

Description

When Gateway try to extract the orgId of custom keys with len > 24 characters, it will extract part of the custom key independent if it's the actual orgID or not.
This PR adds a validation if the token > 24 and it's not a hex string ( orgId's are mongoId's ); it will return an empty string.

Related Issue

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

Motivation and Context

If the custom key has more than 24 characters, it is get deleted from Edge Redis on update.

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

Copy link
Contributor

github-actions bot commented Dec 15, 2023

API Changes

--- prev.txt	2023-12-18 09:05:45.705547420 +0000
+++ current.txt	2023-12-18 09:05:43.085571770 +0000
@@ -10086,6 +10086,7 @@
 const B64JSONPrefix = "ey"
     `{"` in base64
 
+const MongoBsonIdLength = 24
 
 VARIABLES
 

Copy link
Contributor

PR Analysis

  • 🎯 Main theme: Adding hex validation for long keys in the TokenOrg function.
  • 📝 PR summary: This PR introduces a hex validation for long keys in the TokenOrg function. If the key is longer than 24 characters, it is truncated and then validated as a hex string. If the validation passes, the truncated key is returned. The PR also includes new tests for this functionality.
  • 📌 Type of PR: Bug fix
  • 🧪 Relevant tests added: Yes
  • ⏱️ Estimated effort to review [1-5]: 2, because the changes are straightforward and include relevant tests.
  • 🔒 Security concerns: No security concerns found

PR Feedback

  • 💡 General suggestions: The PR is well-structured and includes relevant tests. However, it would be beneficial to include more context in the PR description, such as why this change is necessary and what problem it solves.

  • 🤖 Code feedback:
    relevant filestorage/storage.go
    suggestion      Consider adding a comment explaining why the token is truncated to 24 characters and why it needs to be a valid hex string. This will make the code easier to understand for future developers. [medium]
    relevant linenewToken := token[:24]

    relevant filestorage/storage_test.go
    suggestion      It would be beneficial to add a test case for a key that is exactly 24 characters long to ensure that the function behaves as expected in this edge case. [medium]
    relevant linefunc Test_TokenOrg(t *testing.T) {

How to use

Instructions

To invoke the PR-Agent, add a comment using one of the following commands:
/review: Request a review of your Pull Request.
/describe: Update the PR title and description based on the contents of the PR.
/improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
/ask <QUESTION>: Ask a question about the PR.
/update_changelog: Update the changelog based on the PR's contents.
/add_docs: Generate docstring for new components introduced in the PR.
/generate_labels: Generate labels for the PR based on the PR's contents.
see the tools guide for more details.

To edit any configuration parameter from the configuration.toml, add --config_path=new_value.
For example: /review --pr_reviewer.extra_instructions="focus on the file: ..."
To list the possible configuration parameters, add a /config comment.

Copy link

sweep-ai bot commented Dec 15, 2023

Apply Sweep Rules to your PR?

  • Apply: All new business logic should have corresponding unit tests.
  • Apply: Refactor large functions to be more modular.
  • Apply: Add docstrings to all functions and file headers.

@buger
Copy link
Member

buger commented Dec 15, 2023

API tests result: success
Branch used: refs/pull/5876/merge
Commit:
Triggered by: pull_request (@tbuchaillot)
Execution page

@buger
Copy link
Member

buger commented Dec 15, 2023

API tests result: success
Branch used: refs/pull/5876/merge
Commit: 9b4e20e
Triggered by: pull_request (@tbuchaillot)
Execution page

@buger
Copy link
Member

buger commented Dec 15, 2023

API tests result - postgres15-murmur64 env: success
Branch used: refs/heads/master
Commit: bfaf48c TT-10797 Hex validation for long keys on TokenOrg (#5876)

Description

When Gateway try to extract the orgId of custom keys with len > 24
characters, it will extract part of the custom key independent if it's
the actual orgID or not.
This PR adds a validation if the token > 24 and it's not a hex string (
orgId's are mongoId's ); it will return an empty string.

Related Issue

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

Motivation and Context

If the custom key has more than 24 characters, it is get deleted from
Edge Redis on update.

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
    Triggered by: push (@tbuchaillot)
    Execution page

@buger
Copy link
Member

buger commented Dec 15, 2023

API tests result - postgres15-sha256 env: success
Branch used: refs/heads/master
Commit: bfaf48c TT-10797 Hex validation for long keys on TokenOrg (#5876)

Description

When Gateway try to extract the orgId of custom keys with len > 24
characters, it will extract part of the custom key independent if it's
the actual orgID or not.
This PR adds a validation if the token > 24 and it's not a hex string (
orgId's are mongoId's ); it will return an empty string.

Related Issue

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

Motivation and Context

If the custom key has more than 24 characters, it is get deleted from
Edge Redis on update.

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
    Triggered by: push (@tbuchaillot)
    Execution page

@buger
Copy link
Member

buger commented Dec 15, 2023

API tests result - mongo44-sha256 env: success
Branch used: refs/heads/master
Commit: bfaf48c TT-10797 Hex validation for long keys on TokenOrg (#5876)

Description

When Gateway try to extract the orgId of custom keys with len > 24
characters, it will extract part of the custom key independent if it's
the actual orgID or not.
This PR adds a validation if the token > 24 and it's not a hex string (
orgId's are mongoId's ); it will return an empty string.

Related Issue

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

Motivation and Context

If the custom key has more than 24 characters, it is get deleted from
Edge Redis on update.

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
    Triggered by: push (@tbuchaillot)
    Execution page

@buger
Copy link
Member

buger commented Dec 15, 2023

API tests result - mongo44-murmur64 env: success
Branch used: refs/heads/master
Commit: bfaf48c TT-10797 Hex validation for long keys on TokenOrg (#5876)

Description

When Gateway try to extract the orgId of custom keys with len > 24
characters, it will extract part of the custom key independent if it's
the actual orgID or not.
This PR adds a validation if the token > 24 and it's not a hex string (
orgId's are mongoId's ); it will return an empty string.

Related Issue

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

Motivation and Context

If the custom key has more than 24 characters, it is get deleted from
Edge Redis on update.

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
    Triggered by: push (@tbuchaillot)
    Execution page

@buger
Copy link
Member

buger commented Dec 15, 2023

API tests result: success
Branch used: refs/pull/5876/merge
Commit: 8a80e90
Triggered by: pull_request (@tbuchaillot)
Execution page

@buger
Copy link
Member

buger commented Dec 18, 2023

API tests result: success
Branch used: refs/pull/5876/merge
Commit: 59d45b0
Triggered by: pull_request (@tbuchaillot)
Execution page

storage/storage.go Outdated Show resolved Hide resolved
Copy link

sonarcloud bot commented Dec 18, 2023

@buger
Copy link
Member

buger commented Dec 18, 2023

API tests result: success
Branch used: refs/pull/5876/merge
Commit: 145f649
Triggered by: pull_request (@tbuchaillot)
Execution page

@tbuchaillot tbuchaillot enabled auto-merge (squash) December 18, 2023 09:26
@tbuchaillot tbuchaillot merged commit bfaf48c into master Dec 18, 2023
20 of 21 checks passed
@tbuchaillot tbuchaillot deleted the fix/token-org branch December 18, 2023 09:28
@tbuchaillot
Copy link
Contributor Author

/release to release-5-lts

@tbuchaillot
Copy link
Contributor Author

/release to release-5.2

Copy link

tykbot bot commented Dec 18, 2023

Working on it! Note that it can take a few minutes.

Copy link

tykbot bot commented Dec 18, 2023

Working on it! Note that it can take a few minutes.

tykbot bot pushed a commit that referenced this pull request Dec 18, 2023
<!-- Provide a general summary of your changes in the Title above -->

## Description
When Gateway try to extract the orgId of custom keys with len > 24
characters, it will extract part of the custom key independent if it's
the actual orgID or not.
This PR adds a validation if the token > 24 and it's not a hex string (
orgId's are mongoId's ); it will return an empty string.

<!-- Describe your changes in detail -->

## Related Issue

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->
https://tyktech.atlassian.net/browse/TT-10797
## Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
If the custom key has more than 24 characters, it is get deleted from
Edge Redis on update.
## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->

## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [ ] 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

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the 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

(cherry picked from commit bfaf48c)
Copy link

tykbot bot commented Dec 18, 2023

@tbuchaillot Succesfully merged PR

tykbot bot pushed a commit that referenced this pull request Dec 18, 2023
<!-- Provide a general summary of your changes in the Title above -->

## Description
When Gateway try to extract the orgId of custom keys with len > 24
characters, it will extract part of the custom key independent if it's
the actual orgID or not.
This PR adds a validation if the token > 24 and it's not a hex string (
orgId's are mongoId's ); it will return an empty string.

<!-- Describe your changes in detail -->

## Related Issue

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->
https://tyktech.atlassian.net/browse/TT-10797
## Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
If the custom key has more than 24 characters, it is get deleted from
Edge Redis on update.
## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->

## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [ ] 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

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the 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

(cherry picked from commit bfaf48c)
Copy link

tykbot bot commented Dec 18, 2023

Still working...

Copy link

tykbot bot commented Dec 18, 2023

@tbuchaillot Succesfully merged PR

buger added a commit that referenced this pull request Dec 18, 2023
…kenOrg (#5876)

TT-10797 Hex validation for long keys on TokenOrg (#5876)

<!-- Provide a general summary of your changes in the Title above -->

## Description
When Gateway try to extract the orgId of custom keys with len > 24
characters, it will extract part of the custom key independent if it's
the actual orgID or not.
This PR adds a validation if the token > 24 and it's not a hex string (
orgId's are mongoId's ); it will return an empty string.

<!-- Describe your changes in detail -->

## Related Issue

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->
https://tyktech.atlassian.net/browse/TT-10797
## Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
If the custom key has more than 24 characters, it is get deleted from
Edge Redis on update.
## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->

## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [ ] 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

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the 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
buger added a commit that referenced this pull request Dec 18, 2023
…nOrg (#5876)

TT-10797 Hex validation for long keys on TokenOrg (#5876)

<!-- Provide a general summary of your changes in the Title above -->

## Description
When Gateway try to extract the orgId of custom keys with len > 24
characters, it will extract part of the custom key independent if it's
the actual orgID or not.
This PR adds a validation if the token > 24 and it's not a hex string (
orgId's are mongoId's ); it will return an empty string.

<!-- Describe your changes in detail -->

## Related Issue

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->
https://tyktech.atlassian.net/browse/TT-10797
## Motivation and Context
<!-- Why is this change required? What problem does it solve? -->
If the custom key has more than 24 characters, it is get deleted from
Edge Redis on update.
## How This Has Been Tested

<!-- Please describe in detail how you tested your changes -->
<!-- Include details of your testing environment, and the tests -->
<!-- you ran to see how your change affects other areas of the code,
etc. -->
<!-- This information is helpful for reviewers and QA. -->

## Screenshots (if appropriate)

## Types of changes

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [ ] 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

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants