fix: apiKey security issue #33528
fix: apiKey security issue #33528
Conversation
rishabhrathod01
changed the title
github-actions
bot
added
BE Coders Pod
Failed server tests
|
Failed server tests
|
|
Warning Rate Limit Exceeded@rishabhrathod01 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 48 minutes and 1 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe recent changes aim to enhance security by masking API keys in logs. This involves updating methods in several classes to handle Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
...s/src/main/java/com/appsmith/external/helpers/restApiUtils/helpers/RequestCaptureFilter.java
Outdated
Show resolved
Hide resolved
…rnal/helpers/restApiUtils/helpers/RequestCaptureFilter.java Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
NilanshBansal
added
ok-to-test
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/9203592447. |
|
Deploy-Preview-URL: https://ce-33528.dp.appsmith.com |
| @@ -50,8 +56,44 @@ public RequestCaptureFilter(ObjectMapper objectMapper) { | |||
| return next.exchange(request); | |||
| } | |||
|
|
|||
| private URI createURIWithMaskedQueryParam(URI uriToMask, String queryParamKeyToMask) { | |||
|
|
|||
| String query = uriToMask.getQuery(); | |||
There was a problem hiding this comment.
@NilanshBansal @rishabhrathod01 We should always avoid doing custom parsing for URL parameters. Please replace this with a single 1 line function UriComponentsBuilder. Check this SO answer.
Custom parsing like this can lead to security flaws, encoding problems, loopholes that we missed, etc. Best to use an established library.
There was a problem hiding this comment.
Thank You @mohanarpit for highlighting this 🙏 I did raise this in one of my discussions with @rishabhrathod01 on call, but did not press on it much as I didn't foresee the issues that could come up. This is a miss from my end.
Completely agree with you and the concerns raised by Shri on the failing edge cases.
There was a problem hiding this comment.
Thanks @mohanarpit. I was looking for something similar before adding custom logic, I should have done more research on this.
There was a problem hiding this comment.
I replaced the custom parsing code with the UriComponentsBuilder code now, we don't go into an error state but the output value is still incorrect when the secret value was provided as value&othervalue=abc. This is because the value stored in the query param is not encoded which we get using request.url().
There was a problem hiding this comment.
Created a PR to fix this #33720
…33720) ## Description - Add encoding for Api key secret value before storing it as query param to avoid incorrect parsing. - Remove hardcoded "api_key" check to mask headers. - Used standard URL parsing APIs instead of custom - Added unit test to cover the case Fixes #33742 flaws in the implementation [here](#33528) ## Automation /test datasource ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!IMPORTANT] > 🟣 🟣 🟣 Your tests are running. > Tests running at: <https://github.com/appsmithorg/appsmith/actions/runs/9256269257> > Commit: 602b0f5 > Workflow: `PR Automation test suite` > Tags: `@tag.Datasource` <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No
Description
Fixes #30009
Summary:
This PR addresses the issue of masking sensitive information in query parameters or headers based on the authentication type selected by the user. The changes ensure that sensitive data is properly masked before sending back as response.
Changes:
Added logic to check the authentication type and mask the appropriate query parameters or headers.
Testing:
Verified that the masking functionality works as expected for API_KEY authentication types.
Automation
/test datasource
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/9203547387
Commit: 9a7fc9c
Cypress dashboard url: Click here!
Communication
Should the DevRel and Marketing teams inform users about this change?