added validation for suppress regex #46
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: NxPKG <[email protected]>
Reviewer's Guide by SourceryThis PR adds validation for the suppress regex to ensure that the suppression format is valid. It checks for the correct number of parts, and validates the pluginId, region, and resourceId against specific patterns. Sequence diagram for suppression validation flowsequenceDiagram
participant C as Client
participant V as Validator
participant R as Regex Generator
C->>V: Send suppression expression
activate V
Note over V: Split expression into parts
V->>V: Validate pluginId pattern
V->>V: Validate region pattern
V->>V: Validate resourceId pattern
alt Invalid format
V-->>C: Throw validation error
else Valid format
V->>R: Create regex pattern
R-->>V: Return compiled regex
V-->>C: Return validation result
end
deactivate V
Flow diagram for suppression validation processflowchart TD
A[Start] --> B[Split expression by :]
B --> C{Check parts length}
C -->|!= 3| D[Throw Error: Invalid format]
C -->|= 3| E[Validate pluginId]
E --> F{pluginId valid?}
F -->|No| G[Throw Error: Invalid pluginId]
F -->|Yes| H[Validate region]
H --> I{region valid?}
I -->|No| J[Throw Error: Invalid region]
I -->|Yes| K[Validate resourceId]
K --> L{resourceId valid?}
L -->|No| M[Throw Error: Invalid resourceId]
L -->|Yes| N[Escape special regex chars]
N --> O[Replace * with .*]
O --> P[Create RegExp]
P --> Q[End]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@NxPKG has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 6 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. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
CI Feedback 🧐(Feedback updated until commit d0ea77a)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @NxPKG - I've reviewed your changes - here's some feedback:
Overall Comments:
- The logic for escaping regex special characters and replacing '' with '.' is applied inline. Consider refactoring this transformation into a helper function to avoid duplication and improve maintainability.
- The error messages currently describe the format requirements but don't provide context. Adding an example (or two) for valid suppression strings—especially for resourceId—could help users quickly understand the expected format.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| throw new Error(`Invalid suppression format: ${expr}. Expected format: pluginId:region:resourceId`); | ||
| } | ||
|
|
||
| const pluginPattern = /^[A-Za-z0-9]{1,255}$/; // eslint-disable-line |
There was a problem hiding this comment.
suggestion (performance): Consider defining regex patterns at module level to improve performance
These regex patterns are constant and could be defined once at the module level rather than being recreated on each function call.
Suggested implementation:
const PLUGIN_PATTERN = /^[A-Za-z0-9]{1,255}$/; // eslint-disable-line
const REGION_PATTERN = /^[A-Za-z0-9\-_]{1,255}$/; // eslint-disable-line
const RESOURCE_PATTERN = /^[ A-Za-z0-9._~()'!*:@,;+?#$%^&={}\\[\]\\|\"/-]{1,255}$/; // eslint-disable-line
if (!suppressions) suppressions = [];
const [pluginId, region, resourceId] = parts;
// Validate pluginId
if (!PLUGIN_PATTERN.test(pluginId)) { if (!REGION_PATTERN.test(region)) {You'll also need to:
- Update any other usage of resourcePattern in the code that we can't see (likely used to validate resourceId)
- Replace it with RESOURCE_PATTERN
Comment on lines
42
to
57
| var expressions = suppressions | ||
| .map(function(expr) { | ||
| // Validate the expression format | ||
| validateSuppression(expr); | ||
|
|
||
| // Escape special regex characters except * which we handle specially | ||
| const escapedExpr = expr | ||
| .replace(/[.+?^${}()|[\]\\]/g, '\\$&') // Escape special regex chars | ||
| .split('*') | ||
| .join('.*'); // Replace * with .* | ||
|
|
||
| return [ | ||
| expr, | ||
| new RegExp('^' + expr.split('*').join('.*') + '$') | ||
| new RegExp('^' + escapedExpr + '$') | ||
| ]; | ||
| }); |
There was a problem hiding this comment.
issue (code-quality): Use const or let instead of var. (avoid-using-var)
Explanation
`const` is preferred as it ensures you cannot reassign references (which can lead to buggy and confusing code). `let` may be used if you need to reassign references - it's preferred to `var` because it is block- rather than function-scoped.From the Airbnb JavaScript Style Guide
Signed-off-by: NxPKG <[email protected]>
Signed-off-by: NxPKG <[email protected]>
Signed-off-by: NxPKG <[email protected]>
Signed-off-by: NxPKG <[email protected]>
Signed-off-by: NxPKG <[email protected]>
|
Signed-off-by: NxPKG <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
User description
Notes for Reviewers
This PR fixes #
Signed commits
PR Type
Enhancement, Bug fix
Description
Added validation for suppression format in
suppress.js.Introduced detailed error messages for invalid suppression inputs.
Enhanced regex handling by escaping special characters properly.
Improved robustness of suppression creation logic.
Changes walkthrough 📝
suppress.js
Added validation and improved regex handling in suppressionspostprocess/suppress.js
validateSuppressionfunction to validate suppression format.pluginId,region, andresourceId.