-
-
Notifications
You must be signed in to change notification settings - Fork 448
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
new: add option to skip discovery #644
Conversation
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.
Hello @noperator, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request introduces a new --skip-discovery
option to dalfox, addressing issue #643. When this flag is used, dalfox will skip the initial discovery phase and proceed directly to XSS scanning, relying on parameters provided via the -p
flag. The changes involve modifying cmd/args.go
, cmd/root.go
, pkg/model/options.go
, and pkg/scanning/scan.go
to add the new flag, configure it, and adjust the scanning logic accordingly. The PR author has also raised several questions regarding the implications of skipping discovery, such as handling downstream dependencies on discovery results, the necessity of the -p
option, bypassing content-type checks, and populating parameter fields normally determined during discovery.
Highlights
- New Feature:
--skip-discovery
flag: Adds a new command-line option--skip-discovery
to bypass the discovery phase and directly initiate XSS scanning using user-provided parameters. - Parameter Handling with
--skip-discovery
: When--skip-discovery
is enabled, the-p
flag becomes essential for specifying parameters to be scanned. The code now checks for the presence of parameters when--skip-discovery
is used. - Content-Type Check Bypass: The PR modifies the content-type check logic to allow bypassing it when
--skip-discovery
is enabled, allowing custom payloads to be used even if the content type is not typically allowed. - Parameter Type Handling: When skipping discovery, the parameter type is defaulted to URL, reflection is assumed to be true, and special characters are not determined.
Changelog
Click here to see the changelog
- cmd/args.go
- Added
SkipDiscovery
field to theArgs
struct to store the value of the--skip-discovery
flag.
- Added
- cmd/root.go
- Added the
--skip-discovery
flag to the command-line options, binding it to theSkipDiscovery
field in theArgs
struct. - Configured the
SkipDiscovery
option to be passed to theOptions
struct.
- Added the
- pkg/model/options.go
- Added
SkipDiscovery
field to theOptions
struct to store the skip discovery setting.
- Added
- pkg/scanning/scan.go
- Implemented the logic to skip the discovery phase when
options.SkipDiscovery
is true. - Added a check to ensure that parameters are provided via the
-p
flag when skipping discovery. - Populated the
params
map with user-provided parameters when skipping discovery, setting default values for parameter attributes. - Bypassed content-type checks when skipping discovery.
- Modified the conditional logic for custom payload and common payload setting to account for the skip discovery option.
- Implemented the logic to skip the discovery phase when
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
The scanner sleeps, no need to roam,
Discovery's path, it leaves alone.
With parameters set, and targets in sight,
It leaps to the scan, with all of its might.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
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.
Code Review
The pull request introduces a new feature to skip the discovery phase and proceed directly to XSS scanning. This is a useful addition for users who know the parameters they want to test. The implementation is generally well-structured, but there are a few areas that could be improved for clarity and robustness.
Summary of Findings
- Parameter Type Handling: When skipping discovery, the parameter type is hardcoded to "URL". It might be beneficial to allow users to specify the parameter type (e.g., URL, FORM, COOKIE) for more accurate testing.
- Content-Type Bypass: The code sets a dummy content type of "text/html" to bypass content type checks when skipping discovery. While this works, it might be better to either skip the content type check entirely or allow the user to specify a content type.
- Error Handling: The error message for missing parameters when using
--skip-discovery
could be more user-friendly. Suggesting how to use the-p
flag would be helpful.
Merge Readiness
The code changes introduce a useful feature, but there are a few suggestions for improvement. I would recommend addressing these before merging to enhance the user experience and robustness of the tool. I am unable to approve this pull request, and recommend that others review and approve this code before merging.
@noperator
Also, please note that we’re currently in the process of refactoring the entire codebase, so even after merging this feature, improvements and changes may continue to happen. Thanks for keeping this in mind! |
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
@noperator Thanks again, and see you in v2.10.0! |
Fixes Skip discovery/analysis · Issue #643 · hahwul/dalfox.
Successfully tested this change on Lab: Stored XSS into HTML context with nothing encoded | Web Security Academy:
@hahwul, a few questions I had while making this change:
-p
option by mandatory if--skip-discovery
is used (I assume yes)? since we won't have discovered any paramsContent-Type
checks when skipping discovery (I assume yes)?-p username:form
or something)?