Recorded flow support - #1712
Open
Mzack9999 wants to merge 4 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRecorded Chrome DevTools flows and explicit login steps can now be validated, replayed in headless mode, and used before crawling. The change adds CLI configuration, authentication fixtures, functional tests, and browser-level coverage. ChangesRecorded authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Runner
participant Headless
participant Crawler
participant AuthLab
CLI->>Runner: pass --recorded-flow and credentials
Runner->>Headless: load validated login steps
Headless->>Crawler: configure AuthSteps
Crawler->>AuthLab: replay login actions
AuthLab-->>Crawler: session cookie
Crawler->>AuthLab: crawl protected routes
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+94
to
+100
| http.SetCookie(w, &http.Cookie{ | ||
| Name: CookieName, | ||
| Value: CookieValue, | ||
| Path: "/", | ||
| HttpOnly: true, | ||
| SameSite: http.SameSiteLaxMode, | ||
| }) |
Comment on lines
+104
to
+110
| http.SetCookie(w, &http.Cookie{ | ||
| Name: CookieName, | ||
| Value: "", | ||
| Path: "/", | ||
| MaxAge: -1, | ||
| HttpOnly: true, | ||
| }) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
pkg/engine/headless/auth/e2e_test.go (1)
18-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the browser check.
You can avoid the dummy variable assignment by adopting the cleaner style used in
recorded_flow_e2e_test.go.♻️ Proposed refactor
func skipIfNoBrowser(t *testing.T) { t.Helper() - path, found := launcher.LookPath() - if !found { + if path, found := launcher.LookPath(); !found || path == "" { t.Skip("chrome/chromium not found, skipping recorded-flow e2e") } - _ = path }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/engine/headless/auth/e2e_test.go` around lines 18 - 25, Update skipIfNoBrowser to use the browser lookup only for its found-status check, avoiding the unused path variable and dummy assignment; match the cleaner browser-check style already used in recorded_flow_e2e_test.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/testutils/auth_cases.go`:
- Around line 32-38: Update the explicit flow test’s CompareFunc in the
authentication cases to reuse the simple flow’s validation logic for detecting
discovered gated pages, rather than only checking that got is non-empty.
Preserve the existing authenticated-output error behavior while ensuring a
/app/dashboard URL alone cannot satisfy the assertion.
In `@internal/testutils/authlab/lab.go`:
- Around line 64-73: Update Lab.Close to return l.server.Close() directly when
l.server is non-nil, and only close and return l.listener when no server exists.
Preserve the nil fallback returning nil and avoid closing the listener after the
server has already closed it.
In `@pkg/engine/headless/auth/e2e_test.go`:
- Line 156: Update the page-load setup in the affected test to assert that
page.WaitLoad() succeeds instead of discarding its error. Use the test’s
existing assertion style and test context, ensuring failures from timeouts or
browser crashes stop the test before page.HTML() is evaluated.
In `@pkg/engine/headless/auth/replay.go`:
- Around line 93-96: Update the "submit" branch in the replay flow to honor
step.Selector when it is non-empty, passing the matching element to submitForm;
only use findVisible(page, `input[type="password"]`) for form discovery when no
selector was supplied. Preserve the existing error wrapping and return behavior.
In `@pkg/engine/headless/recorded_flow_e2e_test.go`:
- Line 134: Update the negative test around c.Crawl to assert that crawling
succeeds before checking SecretHits, using the test’s existing assertion
mechanism. Do not discard the Crawl error; fail the test when c.Crawl returns an
error while preserving the existing SecretHits validation.
---
Nitpick comments:
In `@pkg/engine/headless/auth/e2e_test.go`:
- Around line 18-25: Update skipIfNoBrowser to use the browser lookup only for
its found-status check, avoiding the unused path variable and dummy assignment;
match the cleaner browser-check style already used in recorded_flow_e2e_test.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ca1f319b-4345-49db-a564-c6072d1a4b04
📒 Files selected for processing (18)
README.mdcmd/functional-test/main.gocmd/katana/main.gointernal/runner/options.gointernal/runner/options_test.gointernal/testutils/auth_cases.gointernal/testutils/authlab/lab.gointernal/testutils/authlab/lab_test.gopkg/engine/common/base.gopkg/engine/headless/auth/e2e_test.gopkg/engine/headless/auth/recording.gopkg/engine/headless/auth/recording_test.gopkg/engine/headless/auth/replay.gopkg/engine/headless/auth/step.gopkg/engine/headless/crawler/crawler.gopkg/engine/headless/headless.gopkg/engine/headless/recorded_flow_e2e_test.gopkg/types/options.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/testutils/integration.go`:
- Around line 11-40: Replace the shell-based command construction in
RunKatanaBinaryAndGetResults with exec.Command(katanaBinary, args...) and assign
strings.NewReader(target+"\n") to cmd.Stdin. Remove shellQuote, cmdLine, and the
bash -c invocation while preserving debug output, stdout/stderr capture, and
error reporting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9eb3085-9d80-4130-8823-2848c5b57cdf
📒 Files selected for processing (6)
internal/testutils/auth_cases.gointernal/testutils/authlab/lab.gointernal/testutils/integration.gopkg/engine/headless/auth/e2e_test.gopkg/engine/headless/auth/replay.gopkg/engine/headless/recorded_flow_e2e_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/testutils/authlab/lab.go
- pkg/engine/headless/auth/replay.go
- pkg/engine/headless/auth/e2e_test.go
Comment on lines
+11
to
+40
| // shellQuote quotes s for bash so Windows paths with backslashes and values | ||
| // containing @/: don't get mangled by bash -c. | ||
| func shellQuote(s string) string { | ||
| s = filepath.ToSlash(s) | ||
| return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" | ||
| } | ||
|
|
||
| func RunKatanaBinaryAndGetResults(target string, katanaBinary string, debug bool, args []string) ([]string, error) { | ||
| cmd := exec.Command("bash", "-c") | ||
| cmdLine := fmt.Sprintf(`echo %s | %s `, target, katanaBinary) | ||
| cmdLine += strings.Join(args, " ") | ||
| quotedArgs := make([]string, 0, len(args)) | ||
| for _, a := range args { | ||
| quotedArgs = append(quotedArgs, shellQuote(a)) | ||
| } | ||
|
|
||
| cmd.Args = append(cmd.Args, cmdLine) | ||
| data, err := cmd.Output() | ||
| if err != nil { | ||
| return nil, err | ||
| cmdLine := fmt.Sprintf(`echo %s | %s %s`, | ||
| shellQuote(target), | ||
| shellQuote(katanaBinary), | ||
| strings.Join(quotedArgs, " "), | ||
| ) | ||
| if debug { | ||
| fmt.Printf("cmd: %s\n", cmdLine) | ||
| } | ||
|
|
||
| cmd := exec.Command("bash", "-c", cmdLine) | ||
| var stdout, stderr bytes.Buffer | ||
| cmd.Stdout = &stdout | ||
| cmd.Stderr = &stderr | ||
| if err := cmd.Run(); err != nil { | ||
| return nil, fmt.Errorf("katana failed: %w\nstderr:\n%s\nstdout:\n%s", err, stderr.String(), stdout.String()) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Avoid using bash -c to pipe input; use cmd.Stdin directly instead.
Using bash -c with dynamically built strings is an anti-pattern that can lead to command injection (as flagged by static analysis tools), even when custom shell-quoting functions are used. Additionally, it makes the code less cross-platform.
Since the only shell feature being used here is piping echo <target> into the binary's standard input, you can achieve the same result safely and cleanly in native Go by assigning a string reader to cmd.Stdin and passing arguments directly to exec.Command.
🛡️ Proposed fix
-// shellQuote quotes s for bash so Windows paths with backslashes and values
-// containing `@/`: don't get mangled by bash -c.
-func shellQuote(s string) string {
- s = filepath.ToSlash(s)
- return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
-}
-
func RunKatanaBinaryAndGetResults(target string, katanaBinary string, debug bool, args []string) ([]string, error) {
- quotedArgs := make([]string, 0, len(args))
- for _, a := range args {
- quotedArgs = append(quotedArgs, shellQuote(a))
- }
-
- cmdLine := fmt.Sprintf(`echo %s | %s %s`,
- shellQuote(target),
- shellQuote(katanaBinary),
- strings.Join(quotedArgs, " "),
- )
if debug {
- fmt.Printf("cmd: %s\n", cmdLine)
+ fmt.Printf("cmd: echo %s | %s %s\n", target, katanaBinary, strings.Join(args, " "))
}
- cmd := exec.Command("bash", "-c", cmdLine)
+ cmd := exec.Command(katanaBinary, args...)
+ cmd.Stdin = strings.NewReader(target + "\n")
var stdout, stderr bytes.Buffer
cmd.Stdout = &stdout
cmd.Stderr = &stderr📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // shellQuote quotes s for bash so Windows paths with backslashes and values | |
| // containing @/: don't get mangled by bash -c. | |
| func shellQuote(s string) string { | |
| s = filepath.ToSlash(s) | |
| return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" | |
| } | |
| func RunKatanaBinaryAndGetResults(target string, katanaBinary string, debug bool, args []string) ([]string, error) { | |
| cmd := exec.Command("bash", "-c") | |
| cmdLine := fmt.Sprintf(`echo %s | %s `, target, katanaBinary) | |
| cmdLine += strings.Join(args, " ") | |
| quotedArgs := make([]string, 0, len(args)) | |
| for _, a := range args { | |
| quotedArgs = append(quotedArgs, shellQuote(a)) | |
| } | |
| cmd.Args = append(cmd.Args, cmdLine) | |
| data, err := cmd.Output() | |
| if err != nil { | |
| return nil, err | |
| cmdLine := fmt.Sprintf(`echo %s | %s %s`, | |
| shellQuote(target), | |
| shellQuote(katanaBinary), | |
| strings.Join(quotedArgs, " "), | |
| ) | |
| if debug { | |
| fmt.Printf("cmd: %s\n", cmdLine) | |
| } | |
| cmd := exec.Command("bash", "-c", cmdLine) | |
| var stdout, stderr bytes.Buffer | |
| cmd.Stdout = &stdout | |
| cmd.Stderr = &stderr | |
| if err := cmd.Run(); err != nil { | |
| return nil, fmt.Errorf("katana failed: %w\nstderr:\n%s\nstdout:\n%s", err, stderr.String(), stdout.String()) | |
| } | |
| func RunKatanaBinaryAndGetResults(target string, katanaBinary string, debug bool, args []string) ([]string, error) { | |
| if debug { | |
| fmt.Printf("cmd: echo %s | %s %s\n", target, katanaBinary, strings.Join(args, " ")) | |
| } | |
| cmd := exec.Command(katanaBinary, args...) | |
| cmd.Stdin = strings.NewReader(target + "\n") | |
| var stdout, stderr bytes.Buffer | |
| cmd.Stdout = &stdout | |
| cmd.Stderr = &stderr | |
| if err := cmd.Run(); err != nil { | |
| return nil, fmt.Errorf("katana failed: %w\nstderr:\n%s\nstdout:\n%s", err, stderr.String(), stdout.String()) | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 32-32: A shell (sh/bash) is invoked with -c and a dynamically built command string (string concatenation, fmt.Sprintf, or a variable holding the command) passed to exec.Command / exec.CommandContext. Untrusted input embedded in the command lets an attacker inject arbitrary shell commands. Avoid the shell: call the target binary directly with exec.Command(name, arg1, arg2, ...) so each argument is passed as a separate, non-interpreted token, and never build a shell command string from external input.
Context: exec.Command("bash", "-c", cmdLine)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(command-injection-exec-sh-c-go)
🪛 OpenGrep (1.25.0)
[ERROR] 33-33: Dynamic command passed to exec.Command with a shell invocation. Pass arguments directly to exec.Command without a shell wrapper.
(coderabbit.command-injection.go-exec-command)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/testutils/integration.go` around lines 11 - 40, Replace the
shell-based command construction in RunKatanaBinaryAndGetResults with
exec.Command(katanaBinary, args...) and assign strings.NewReader(target+"\n") to
cmd.Stdin. Remove shellQuote, cmdLine, and the bash -c invocation while
preserving debug output, stdout/stderr capture, and error reporting.
Source: Linters/SAST tools
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Closes #1629
Adds
-rf/-recorded-flowto replay Chrome DevTools Recorder (or explicit step) logins once before headless crawl. Includes AuthLab e2e + docs.Summary by CodeRabbit
--recorded-flow/-rfoption to replay Chrome DevTools Recorder (or explicit step scripts) for authentication before crawling.-auto-loginand-recorded-flowusage details.