Open
Conversation
Contributor
WalkthroughBroadened the build constraint on the CGO jsluice file to require the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![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: 1
🧹 Nitpick comments (1)
pkg/utils/jsluice_stub_test.go (1)
93-103: ExpandisValidEndpointtests for scheme boundary/case variants.Please add cases for exact
data:and mixed-caseJavaScript:so the filter behavior is locked down for edge inputs.Suggested test additions
{ name: "data URI", input: "data:image/png;base64,iVBORw0KGgo...", want: false, }, + { + name: "exact data scheme", + input: "data:", + want: false, + }, { name: "javascript URI", input: "javascript:void(0)", want: false, }, + { + name: "mixed-case javascript URI", + input: "JavaScript:void(0)", + want: false, + },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/jsluice_stub_test.go` around lines 93 - 103, Add two test cases to the isValidEndpoint table-driven tests in jsluice_stub_test.go: one where input is exactly "data:" (to verify a bare data scheme is rejected) and another with mixed-case scheme "JavaScript:alert(1)" (or similar) to verify scheme matching is case-insensitive/correctly normalized; update the test table (the slice of test structs near the existing "data URI" and "javascript URI" entries) with entries named e.g. "data scheme only" and "mixed-case JavaScript scheme" and set want to false so isValidEndpoint is exercised for these boundary/case variants.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/utils/jsluice_stub.go`:
- Around line 72-76: In isValidEndpoint, the current prefix logic is fragile
(checks len>5 and "javas") and misses exact or mixed-case schemes like "data:"
and "JavaScript:". Change it to normalize the input (strings.ToLower(s)) and use
explicit full-scheme checks (e.g., strings.HasPrefix(lower, "data:") ||
strings.HasPrefix(lower, "javascript:")) and remove the brittle length-based
branch so these schemes reliably return false; update references in
jsluice_stub.go's isValidEndpoint accordingly.
---
Nitpick comments:
In `@pkg/utils/jsluice_stub_test.go`:
- Around line 93-103: Add two test cases to the isValidEndpoint table-driven
tests in jsluice_stub_test.go: one where input is exactly "data:" (to verify a
bare data scheme is rejected) and another with mixed-case scheme
"JavaScript:alert(1)" (or similar) to verify scheme matching is
case-insensitive/correctly normalized; update the test table (the slice of test
structs near the existing "data URI" and "javascript URI" entries) with entries
named e.g. "data scheme only" and "mixed-case JavaScript scheme" and set want to
false so isValidEndpoint is exercised for these boundary/case variants.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a3f4bf17-a77b-449e-a275-682a761e951e
📒 Files selected for processing (3)
pkg/utils/jsluice.gopkg/utils/jsluice_stub.gopkg/utils/jsluice_stub_test.go
|
🔧 Hit a snag — please try again. |
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/utils/jsluice_stub.go (1)
5-7:⚠️ Potential issue | 🔴 CriticalMissing
stringsimport causes a build break.Lines 72 and 75 use
strings.ToLowerandstrings.HasPrefix, butstringsis not imported. The import block contains only"regexp", so this file fails to compile withundefined: strings.Proposed fix
import ( "regexp" + "strings" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/jsluice_stub.go` around lines 5 - 7, The build fails due to missing strings import in jsluice_stub.go: add "strings" to the existing import block (alongside "regexp") so calls to strings.ToLower and strings.HasPrefix compile; locate the functions in this file that call strings (the references around where strings.ToLower and strings.HasPrefix are used) and update the import list accordingly, then run go build to verify the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/utils/jsluice_stub.go`:
- Around line 5-7: The build fails due to missing strings import in
jsluice_stub.go: add "strings" to the existing import block (alongside "regexp")
so calls to strings.ToLower and strings.HasPrefix compile; locate the functions
in this file that call strings (the references around where strings.ToLower and
strings.HasPrefix are used) and update the import list accordingly, then run go
build to verify the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 485f88af-e125-47be-8f1b-a5fa3e1b817d
📒 Files selected for processing (1)
pkg/utils/jsluice_stub.go
Author
|
@ Thanks for catching that! I've added the missing
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/utils/jsluice_stub.go (1)
10-16: Consider enforcing matched quote pairs for stricter parsing.The current patterns allow mismatched quotes (e.g.,
"url'). For a fallback implementation this is likely acceptable, but stricter parsing would reduce false positives.♻️ Alternative pattern with matched quotes
var ( // URL patterns for extracting endpoints from JavaScript - urlPattern = regexp.MustCompile(`(?i)(?:"|'|` + "`" + `)((?:https?:)?//[^\s"'` + "`" + `<>]+|/[^\s"'` + "`" + `<>]+)(?:"|'|` + "`" + `)`) + urlPatternDQ = regexp.MustCompile(`(?i)"((?:https?:)?//[^\s"<>]+|/[^\s"<>]+)"`) + urlPatternSQ = regexp.MustCompile(`(?i)'((?:https?:)?//[^\s'<>]+|/[^\s'<>]+)'`) + urlPatternBT = regexp.MustCompile("(?i)`((?:https?:)?//[^\\s`<>]+|/[^\\s`<>]+)`")Then union the match results in
ExtractJsluiceEndpoints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/jsluice_stub.go` around lines 10 - 16, Update the regexes to enforce matching quote pairs by capturing the opening quote and using a backreference instead of allowing any closing quote: modify urlPattern and apiPattern to capture the quote char (e.g., (?P<q>["'`])) and reference it at the end so mismatched quotes like "url' are not accepted, then in ExtractJsluiceEndpoints ensure you union the results from both patterns (urlPattern matches and apiPattern matches) into the final endpoint set to avoid duplicates and preserve stricter parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/utils/jsluice_stub.go`:
- Around line 28-63: The ExtractJsluiceEndpoints stub currently returns generic
types "url" and "api" which differ from the real jsluice CGO semantics (e.g.,
"linkHref", "fetch", "xhr") and breaks downstream formatting that builds
attributes with fmt.Sprintf("jsluice-%s", item.Type); update
ExtractJsluiceEndpoints (and JSLuiceEndpoint.Type values it creates) to map URL
and API regex matches to the concrete jsluice type strings used by the real
library (e.g., map link href matches to "linkHref", fetch calls to "fetch", XHR
to "xhr", etc.), or if you intentionally want a fallback behavior, add a clear
comment in the function declaring that it returns coarse-grained "url"/"api"
types and list the expected CGO-specific type names so callers know the
difference; ensure references in parser_generic.go that format "jsluice-%s" will
receive the expected type strings.
---
Nitpick comments:
In `@pkg/utils/jsluice_stub.go`:
- Around line 10-16: Update the regexes to enforce matching quote pairs by
capturing the opening quote and using a backreference instead of allowing any
closing quote: modify urlPattern and apiPattern to capture the quote char (e.g.,
(?P<q>["'`])) and reference it at the end so mismatched quotes like "url' are
not accepted, then in ExtractJsluiceEndpoints ensure you union the results from
both patterns (urlPattern matches and apiPattern matches) into the final
endpoint set to avoid duplicates and preserve stricter parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff1243d3-a0a7-4d18-a369-4b57bc9dfea5
📒 Files selected for processing (1)
pkg/utils/jsluice_stub.go
Comment on lines
+28
to
+63
| func ExtractJsluiceEndpoints(data string) []JSLuiceEndpoint { | ||
| var endpoints []JSLuiceEndpoint | ||
| seen := make(map[string]bool) | ||
|
|
||
| // Extract URLs using URL pattern | ||
| urlMatches := urlPattern.FindAllStringSubmatch(data, -1) | ||
| for _, match := range urlMatches { | ||
| if len(match) > 1 { | ||
| url := match[1] | ||
| if !seen[url] && isValidEndpoint(url) { | ||
| seen[url] = true | ||
| endpoints = append(endpoints, JSLuiceEndpoint{ | ||
| Endpoint: url, | ||
| Type: "url", | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Extract API endpoints | ||
| apiMatches := apiPattern.FindAllStringSubmatch(data, -1) | ||
| for _, match := range apiMatches { | ||
| if len(match) > 1 { | ||
| endpoint := match[1] | ||
| if !seen[endpoint] && isValidEndpoint(endpoint) { | ||
| seen[endpoint] = true | ||
| endpoints = append(endpoints, JSLuiceEndpoint{ | ||
| Endpoint: endpoint, | ||
| Type: "api", | ||
| }) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return endpoints | ||
| } |
Contributor
There was a problem hiding this comment.
Type classification differs from CGO version semantics.
The stub returns only "url" or "api", while the jsluice library returns specific types like "linkHref", "fetch", "xhr", etc. Downstream code constructs attributes via fmt.Sprintf("jsluice-%s", item.Type) (see parser_generic.go), so endpoints will be labeled differently:
- CGO:
jsluice-linkHref,jsluice-fetch, etc. - Stub:
jsluice-url,jsluice-api
This may affect filtering, output formatting, or any logic that depends on specific type values. If this is intentional fallback behavior, consider documenting it in the function's comments.
📝 Suggested documentation addition
// ExtractJsluiceEndpoints extracts endpoints from JavaScript using pure Go regex.
// This is a fallback implementation when jsluice (which requires CGO) is not available.
//
// Note: This implementation uses regex patterns and may not be as accurate as jsluice,
// but it eliminates the CGO dependency for cross-platform compilation.
+//
+// Type classification: This stub returns generic types ("url" or "api") rather than
+// the specific types returned by jsluice (e.g., "linkHref", "fetch", "xhr").
func ExtractJsluiceEndpoints(data string) []JSLuiceEndpoint {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/utils/jsluice_stub.go` around lines 28 - 63, The ExtractJsluiceEndpoints
stub currently returns generic types "url" and "api" which differ from the real
jsluice CGO semantics (e.g., "linkHref", "fetch", "xhr") and breaks downstream
formatting that builds attributes with fmt.Sprintf("jsluice-%s", item.Type);
update ExtractJsluiceEndpoints (and JSLuiceEndpoint.Type values it creates) to
map URL and API regex matches to the concrete jsluice type strings used by the
real library (e.g., map link href matches to "linkHref", fetch calls to "fetch",
XHR to "xhr", etc.), or if you intentionally want a fallback behavior, add a
clear comment in the function declaring that it returns coarse-grained
"url"/"api" types and list the expected CGO-specific type names so callers know
the difference; ensure references in parser_generic.go that format "jsluice-%s"
will receive the expected type strings.
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
1 participant
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.
Changes
Files Added
pkg/utils/jsluice_stub.go- Pure Go fallback implementationpkg/utils/jsluice_stub_test.go- Tests for fallback implementationFiles Modified
pkg/utils/jsluice.go- Updated build tag to make jsluice optionalNote
go.modwill be automatically cleaned up bygo mod tidyduring CI, which will remove unused indirect dependencies.Summary by CodeRabbit
New Features
Tests