-
Notifications
You must be signed in to change notification settings - Fork 209
Fix false positive in send HTTP request #1099
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
base: master
Are you sure you want to change the base?
Conversation
mike-hunhoff
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @saniyafatima07 ! Quick question for your review.
| - string: "GET " | ||
| - string: "POST " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about instances where the HTTP request is a single string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s a good point, thank you for highlighting it.
My initial goal was to reduce the false positives caused by the generic /HTTP/i match, and this led me to the current approach using explicit HTTP method strings.
Your question about cases where the entire HTTP request is sent as a single string made me reconsider coverage, as relying only on standalone "GET " / "POST " strings may miss some valid client requests.
To address this more efficiently, I think a regex-based approach that matches HTTP methods could be used, as it would improve coverage while still avoiding the original false positives.
If that sounds reasonable, I can update the rule accordingly. Please let me know your preference.
I can also take up any suggestions from your end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will come up with a refined approach. Thank you for the review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about instances where the HTTP request is a single string?
Went through the documentation regarding string, substring and regex. As per my understanding i
-
Instead of using 'string' if we use 'substring', this problem would be solved.
-
Another approach would be regex based , I tried replicating the capa-rules for regex
/GET\s+/.*HTTP/\d+.\d+/i and came up with this format which also covers case-senstive matching.
Could you please take a look at this approach and let me know if I am going in the right direction?
Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about anchoring the HTTP verbs to the start of the string using a regular expression?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would probably cover all the cases, as anchoring the verb to the start of the string avoids matching incidental GET occurrences while still handling single-string HTTP requests.
|
@mike-hunhoff Refined the approach by using anchored regex patterns. |
| - string: /^GET\s+\/\S*\s+HTTP\/\d+(\.\d+)?/i | ||
| - string: /^POST\s+\/\S*\s+HTTP\/\d+(\.\d+)?/i |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good. Please combine into a single expression and include other common HTTP request verbs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Thank you for the review!
|
@mike-hunhoff Done with the changes. Could you please review. |
Closes #1093
Fixes false positive in send HTTP request using string literals "GET " and "POST "