feat(headless): eval DSL exprs in args #6017
Conversation
WalkthroughThe pull request introduces significant changes to the headless engine's context and variable handling in the Nuclei project. The modifications primarily focus on enhancing the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
pkg/protocols/headless/engine/page_actions.go (4)
244-257: Time parsing considerations.
Returningtime.Duration(0)on error is acceptable, but consider logging or clarifying the error for better traceability, especially if an invalid user input leads to a zero timeout silently.
264-285: Header addition logic looks sound; consider validating keys.
Since headers can be dynamically set, you may want to restrict or sanitize keys to avoid malformed or injection-prone headers in certain contexts.
343-359: SetBody action is correct; consider data sanitization.
If the body is used in subsequent requests or rendered in a downstream application, ensure it doesn’t open vectors for XSS or other exploits.
419-448: Executing scripts on a page is effective but be mindful of security.
Running arbitrary user-provided code can introduce security risks. Use caution to ensure scripts come from a trusted source.pkg/protocols/headless/engine/util.go (1)
9-11: Deprecated function remains in the code.
Static analysis flagsreplaceWithValuesas unused. Consider removing it if truly no longer needed to keep the codebase clean.-// Deprecated: Not used anymore. -func replaceWithValues(data string, values map[string]interface{}) string { - return fasttemplate.ExecuteStringStd(data, marker.ParenthesisOpen, marker.ParenthesisClose, values) -}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pkg/protocols/headless/engine/page.go(8 hunks)pkg/protocols/headless/engine/page_actions.go(17 hunks)pkg/protocols/headless/engine/rules.go(2 hunks)pkg/protocols/headless/engine/util.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Lint
pkg/protocols/headless/engine/util.go
[failure] 12-12:
func replaceWithValues is unused (unused)
🪛 golangci-lint (1.62.2)
pkg/protocols/headless/engine/util.go
12-12: func replaceWithValues is unused
(unused)
🔇 Additional comments (25)
pkg/protocols/headless/engine/page_actions.go (16)
5-5: Import usage is appropriate.
No issues found with the added"fmt"import, as it’s later used for error formatting.
69-69: Good refactor in calling NavigateURL.
The simplified call toNavigateURLimproves readability.
292-313: Consistent pattern for setting headers.
The logic for adding/changing headers is consistent with ActionAddHeader. Same note regarding potential header key validation applies.
320-336: Header deletion approach is straightforward.
No issues found; it aligns with the pattern of other header actions.
367-385: HTTP method assignment looks good.
The approach is straightforward and properly stores the rule. No concurrency concerns spotted.
469-474: Keyboard action logic is straightforward.
No obvious issues; properly retrieves and types the keys.
494-526: Screenshot logic is thorough.
Cleanly handles file paths, permissions, and local file access checks. Good job verifying the target directory and discouraging file overwrites.
574-577: InputElement action is consistent.
Checks for a non-empty value before proceeding with the element input. No concerns.
596-599: Time input handling is correct.
Parses time with RFC3339 and ensures correct usage. Looks good.
Line range hint
622-652: SelectInputElement is well structured.
Retrieves required parameters, sets optional selection, and applies them correctly. No concerns.
656-656: Clean exit of function.
Returning a nil error here is concise and consistent.
Line range hint
782-827: Event-wait logic is comprehensive.
Allows a maximum duration, checks event validity, and properly uses context timeouts. No issues.
833-847: Dialog handling with configurable duration is robust.
Utilizes a context-based timeout, captures dialog data, and sets relevant output. Nicely done.
939-962: Argument retrieval and expression evaluation are well designed.
Ensures any unresolved variable or evaluation error is raised early. Just be cautious about untrusted or complex expressions to avoid possible injection or performance pitfalls.
387-413: 🛠️ Refactor suggestionPotential concurrency concern with requestLog map.
Accessingp.instance.requestLog(likely a map) without synchronization can be risky in concurrent scenarios. Consider using a mutex or a sync.Map if this code can run concurrently.
48-48: Method signature update requires verification.
The removal of thevariablesparameter indicates a shift top.variables. Please confirm that any callers ofExecuteActionshave been updated accordingly to avoid potential breakages.✅ Verification successful
Signature change verification completed successfully
The removal of the
variablesparameter is consistent with all callers in the codebase. The only usage ofExecuteActionsalready matches the new two-parameter signature.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for calls to ExecuteActions to ensure they match the new signature rg -A 3 "ExecuteActions\("Length of output: 707
Script:
#!/bin/bash # Check the git history for the ExecuteActions method git log -L :ExecuteActions:pkg/protocols/headless/engine/page_actions.goLength of output: 0
Script:
#!/bin/bash # Get the full implementation of ExecuteActions ast-grep --pattern 'func (p *Page) ExecuteActions($$$) { $$$ }' # Get the context of the call site ast-grep --pattern '$_.ExecuteActions($$$)'Length of output: 113
Script:
#!/bin/bash # Get extended context for both definition and usage rg -B 5 -A 15 "ExecuteActions\("Length of output: 3175
pkg/protocols/headless/engine/util.go (2)
4-4: New import for expressions is appropriate.
The code referencesexpressionsfor extracting DSL expressions, which aligns with usage inpage_actions.go.
16-18: Efficient extraction of DSL expressions.
getExpressionscleanly delegates parsing toexpressions.FindExpressions. No issues found.pkg/protocols/headless/engine/rules.go (2)
45-45: Cookies are correctly set from ctx.CookieJar.
Ensures the request inherits cookies from the shared jar if cookies are enabled.
57-57: Cookie jar synchronization.
Retrieving updated cookies and feeding them back intop.ctx.CookieJaris consistent. Confirm concurrency safety if multiple goroutines share a Page.pkg/protocols/headless/engine/page.go (5)
14-22: LGTM! Import changes align with new functionality.The new imports support the added context handling, variable management, and improved error handling capabilities.
27-27: LGTM! Struct changes enhance context and variable handling.The replacement of
inputwithctxand addition ofvariablesfield align with the PR's objective of supporting DSL expression evaluation in arguments. The changes follow good design practices by using context for request-scoped data.Also applies to: 38-38
68-81: LGTM! Robust variable handling implementation.The implementation properly merges variables from multiple sources (payloads, options, URL, and context) ensuring comprehensive variable support for DSL expressions.
Line range hint
137-167: LGTM! Robust cookie synchronization between browser and context.The implementation properly:
- Preserves all cookie attributes during browser-context synchronization
- Maintains cookie security properties (HTTPOnly, Secure, SameSite)
- Handles cookie updates bidirectionally
Also applies to: 170-190
Line range hint
55-93: Verify integration with DSL evaluation system.The changes look good and align with the PR objectives. To ensure proper integration:
- Verify that DSL expressions in arguments are properly evaluated
- Confirm that variable scope is maintained correctly
Run this script to check DSL integration:
✅ Verification successful
DSL integration and variable handling verified successfully
The implementation correctly integrates with the DSL evaluation system and maintains proper variable scope:
- Variables are properly evaluated through the evaluation system
- Payload and variable scoping is correctly implemented
- Action execution maintains proper context and variable access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check DSL evaluation integration ast-grep --pattern 'func Eval($_, $_, $_) { $$$ }' # Check variable scope handling ast-grep --pattern 'func ($_ *Page) ExecuteActions($_, $_) { $$$ }'Length of output: 126
Script:
#!/bin/bash # Check DSL-related imports and usage rg -l "dsl" pkg/protocols/headless/ rg "import.*dsl" pkg/protocols/headless/ -A 5 # Check action execution and variable handling rg "func.*ExecuteActions|func.*Run" pkg/protocols/headless/ -A 5 # Check variable evaluation patterns rg "variables.*:=|payloads.*:=" pkg/protocols/headless/ -A 2Length of output: 2502
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Now, `getTimeParameter` tries to get the parameter as an integer, then as a `time.Duration`, and finally falls back to the default value (multiplied by the unit). Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
Signed-off-by: Dwi Siswanto <[email protected]>
dwisiswant0
force-pushed
the
dwisiswant0/feat/headless/eval-DSL-expr-in-args
branch
from
ea34794 to
5b8505f
Compare
Signed-off-by: Dwi Siswanto <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
pkg/protocols/headless/engine/page.go (2)
Line range hint
56-77: Refine Run method parameter and local variable naming
The method signature change from using aninputparameter to usingctxis appropriate and aligns with the new context-based design. For clarity and to avoid confusion with the struct fieldinputURL, consider renaming the local variable currently namedinput(obtained from URL parsing) to something likeparsedURL. For example:- target := ctx.MetaInput.Input - input, err := urlutil.Parse(target) + target := ctx.MetaInput.Input + parsedURL, err := urlutil.Parse(target)This small renaming can improve code readability.
Line range hint
923-945: Ensure robust evaluation in getActionArg
ThegetActionArgfunction evaluates expressions and checks for unresolved variables, which is essential for DSL expression handling. If similar functionality exists elsewhere in the codebase, consider refactoring this logic into a shared utility. This would reduce duplication and ease maintenance.pkg/protocols/headless/engine/page_actions.go (1)
923-945: Refactor getActionArg to reduce code duplication
ThegetActionArgmethod evaluates action arguments by replacing interactsh URLs and processing expressions. If this function logic is similar (or identical) to that inpkg/protocols/headless/engine/page.go, consider refactoring it into a shared utility function. This reuse would promote consistency and easier maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
pkg/protocols/headless/engine/page.go(8 hunks)pkg/protocols/headless/engine/page_actions.go(18 hunks)pkg/protocols/headless/engine/rules.go(2 hunks)pkg/protocols/headless/engine/util.go(1 hunks)pkg/protocols/headless/request.go(0 hunks)
💤 Files with no reviewable changes (1)
- pkg/protocols/headless/request.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/protocols/headless/engine/rules.go
- pkg/protocols/headless/engine/util.go
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Integration tests (macOS-latest)
- GitHub Check: Integration tests (windows-latest)
- GitHub Check: Release test
- GitHub Check: Functional tests (windows-latest)
- GitHub Check: Integration tests (ubuntu-latest)
🔇 Additional comments (6)
pkg/protocols/headless/engine/page.go (4)
27-28: Update Page struct with enhanced context fields
The new fields (ctx,inputURL, andvariables) in thePagestruct clearly support the improved handling of DSL evaluation and context management. However, please ensure that any sensitive data contained invariablesis sanitized before logging (see next comment regarding debug logging).Also applies to: 39-39
139-145: Consistent use of context for URL parsing in cookie injection
Usingctx.MetaInput.Inputto parse the URL for cookie injection is consistent with the overall refactoring. Ensure that the error handling here is robust and that the parsed URL stored ininputURLis used wherever needed later in the flow.
192-192: Cookie jar update via context
The update of cookies viactx.CookieJar.SetCookies(URL, httpCookies)is correct and leverages the new context field. Just double-check thatURL(from the earlier parsing) is consistently defined and that no race conditions occur when accessing the shared cookie jar.
83-85:⚠️ Potential issueSecurity caution: Debug logging of variables
The new debug logging block prints the evaluatedvariablesusingvardump.DumpVariables, which might expose sensitive information if not masked. Please ensure that the underlyingvardumpimplementation properly handles sensitive fields (as noted in previous reviews).pkg/protocols/headless/engine/page_actions.go (2)
368-386: Validate ActionSetMethod implementation
TheActionSetMethodfunction correctly retrieves thepartandmethodarguments and appends a new rule with async.Onceinstance. This ensures idempotent execution where needed. Confirm that the error messages fromgetActionArgeffectively communicate missing or malformed arguments.
388-414: Review NavigateURL for correct query parameter merging
TheNavigateURLfunction now merges parameters from the parsed URL with those fromp.inputURL.Params. This design is good for ensuring that target parameters take precedence. Additionally, logging the navigated URL inp.instance.requestLogis useful; however, please verify that this logging mechanism is thread-safe and does not introduce any performance bottlenecks.
There was a problem hiding this comment.
@dwisiswant0 we can add integration test / template for the same.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Dwi Siswanto <[email protected]>
dwisiswant0
force-pushed
the
dwisiswant0/feat/headless/eval-DSL-expr-in-args
branch
from
bfaf51f to
a7fbba2
Compare
Proposed changes
Close #6013
Checklist
Summary by CodeRabbit
Release Notes
Improvements
Bug Fixes
Refactoring