Improving lua script with args and values #6561
Conversation
Mzack9999
added
the
Type: Enhancement
WalkthroughAdds keys/args support to the Redis JavaScript RunLuaScript API and a corresponding integration test that runs a Redis Lua script template with retry and validation logic against a started Redis resource. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Nuclei as Nuclei Engine
participant JS as JavaScript Runtime
participant Redis as Redis Server
Test->>Redis: ensure Redis resource / pool available
Test->>Test: derive target URL, defer purge
loop retry (N times)
Test->>Nuclei: run template (redis-lua-script.yaml) against target URL via pool
Nuclei->>JS: execute javascript block -> call redis.RunLuaScript(script, keys, args)
JS->>JS: normalize keys/args to []string / []interface{}
JS->>Redis: EVAL script with keys and args
Redis-->>JS: return result
JS-->>Nuclei: return execution result
Nuclei-->>Test: results (count)
alt result count == 1
Test-->>Test: success, stop retries
else
Test->>Test: sleep then retry
end
end
Test-->>Test: aggregate errors if retries exhausted
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/js/libs/redis/redis.go (1)
223-243: Consider optimizing the args conversion to avoid redundant processing.The current implementation converts args twice:
[]interface{}→[]string→[]interface{}. When args is already[]interface{}, you could skip the intermediate[]stringconversion and directly prepare it for the Eval call.Refactor to eliminate redundant conversion:
// Convert interface{} args directly to []interface{} for Eval argsInterface := []interface{}{} if args != nil { switch v := args.(type) { case []string: argsInterface = make([]interface{}, len(v)) for i, arg := range v { argsInterface[i] = arg } case []interface{}: // Filter to only strings (or handle type conversion) for _, item := range v { if s, ok := item.(string); ok { argsInterface = append(argsInterface, s) } } } }This eliminates the intermediate
argsSlicevariable and avoids double conversion for[]interface{}inputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
integration_tests/protocols/javascript/redis-lua-script.yamlis excluded by!**/*.yamlpkg/js/generated/ts/redis.tsis excluded by!**/generated/**
📒 Files selected for processing (2)
cmd/integration-test/javascript.go(2 hunks)pkg/js/libs/redis/redis.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code using go fmt
Run static analysis with go vet
Files:
pkg/js/libs/redis/redis.gocmd/integration-test/javascript.go
🧬 Code graph analysis (2)
pkg/js/libs/redis/redis.go (1)
pkg/js/generated/ts/redis.ts (1)
RunLuaScript(70-72)
cmd/integration-test/javascript.go (1)
pkg/testutils/integration.go (2)
TestCase(247-250)RunNucleiTemplateAndGetResults(30-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Tests (macOS-latest)
- GitHub Check: Tests (windows-latest)
- GitHub Check: Tests (ubuntu-latest)
🔇 Additional comments (4)
cmd/integration-test/javascript.go (1)
15-15: LGTM!The test case registration is consistent with other JavaScript test cases and properly disables on Windows/OSX platforms.
pkg/js/libs/redis/redis.go (3)
178-182: LGTM!The documentation clearly demonstrates both backward-compatible usage (without keys/args) and the new signature with keys and args parameters.
183-183: LGTM!The function signature correctly uses
interface{}types for keys and args to support backward compatibility (nil values) and flexible type handling from JavaScript.
246-246: LGTM!The Eval call correctly uses the converted
keysSliceandargsInterfaceparameters to execute the Lua script with the provided keys and arguments.
| type javascriptRedisLuaScript struct{} | ||
|
|
||
| func (j *javascriptRedisLuaScript) Execute(filePath string) error { | ||
| if redisResource == nil || pool == nil { | ||
| // skip test as redis is not running | ||
| return nil | ||
| } | ||
| tempPort := redisResource.GetPort("6379/tcp") | ||
| finalURL := "localhost:" + tempPort | ||
| defer purge(redisResource) | ||
| errs := []error{} | ||
| for i := 0; i < defaultRetry; i++ { | ||
| results := []string{} | ||
| var err error | ||
| _ = pool.Retry(func() error { | ||
| //let ssh server start | ||
| time.Sleep(3 * time.Second) | ||
| results, err = testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug) | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if err := expectResultsCount(results, 1); err == nil { | ||
| return nil | ||
| } else { | ||
| errs = append(errs, err) | ||
| } | ||
| } | ||
| return multierr.Combine(errs...) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate code duplication with shared helper function.
This entire test implementation is duplicated from javascriptRedisPassBrute (lines 44-72). The only differences are the type name and test file path.
Consider extracting the common logic into a helper function to reduce maintenance burden and avoid copy-paste errors (such as the incorrect comment on line 89).
Apply this refactor:
// Helper function for Redis-based integration tests
func executeRedisTest(filePath string, redisResource *dockertest.Resource, pool *dockertest.Pool) error {
if redisResource == nil || pool == nil {
// skip test as redis is not running
return nil
}
tempPort := redisResource.GetPort("6379/tcp")
finalURL := "localhost:" + tempPort
defer purge(redisResource)
errs := []error{}
for i := 0; i < defaultRetry; i++ {
results := []string{}
var err error
_ = pool.Retry(func() error {
// wait for redis server to be ready
time.Sleep(3 * time.Second)
results, err = testutils.RunNucleiTemplateAndGetResults(filePath, finalURL, debug)
return nil
})
if err != nil {
return err
}
if err := expectResultsCount(results, 1); err == nil {
return nil
} else {
errs = append(errs, err)
}
}
return multierr.Combine(errs...)
}
type javascriptRedisPassBrute struct{}
func (j *javascriptRedisPassBrute) Execute(filePath string) error {
return executeRedisTest(filePath, redisResource, pool)
}
type javascriptRedisLuaScript struct{}
func (j *javascriptRedisLuaScript) Execute(filePath string) error {
return executeRedisTest(filePath, redisResource, pool)
}🤖 Prompt for AI Agents
In cmd/integration-test/javascript.go around lines 74 to 104, the Execute
implementation for javascriptRedisLuaScript duplicates the logic from
javascriptRedisPassBrute; extract the shared Redis test logic into a helper like
executeRedisTest(filePath string, redisResource *dockertest.Resource, pool
*dockertest.Pool) that performs the nil checks, port resolution, purge defer,
retry loop, pool.Retry call, result collection and multierr.Combine, then change
both javascriptRedisPassBrute.Execute and javascriptRedisLuaScript.Execute to
simply return executeRedisTest(filePath, redisResource, pool); ensure the helper
uses the correct comment ("wait for redis server to be ready") and preserves
existing behavior and imports.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pkg/js/libs/redis/redis.go (3)
205-217: Consider validating key types explicitly.While
fmt.Sprintf("%v", item)prevents silent data loss, it may still produce unexpected string representations for complex types (e.g., maps, structs). For Redis keys, only strings and basic types are typically meaningful.Consider adding a default case to handle unexpected types:
// Convert interface{} to []string for keys (handle backwards compatibility) keysSlice := []string{} if keys != nil { switch v := keys.(type) { case []string: keysSlice = v case []interface{}: keysSlice = make([]string, 0, len(v)) for _, item := range v { keysSlice = append(keysSlice, fmt.Sprintf("%v", item)) } + default: + return nil, fmt.Errorf("keys must be []string or []interface{}, got %T", keys) } }
236-240: Eliminate unnecessary conversion by directly building []interface{}.The current flow converts args through multiple types:
[]interface{}(from JS) →[]string→[]interface{}(for Eval). This is inefficient and adds complexity.Consider refactoring to build
[]interface{}directly:- // Convert interface{} to []string for args (handle backwards compatibility) - argsSlice := []string{} + // Convert interface{} to []interface{} for args (handle backwards compatibility) + argsInterface := []interface{}{} if args != nil { switch v := args.(type) { case []string: - argsSlice = v + argsInterface = make([]interface{}, len(v)) + for i, s := range v { + argsInterface[i] = s + } case []interface{}: - // Convert []interface{} to []string (from JavaScript arrays) - argsSlice = make([]string, 0, len(v)) - for _, item := range v { - argsSlice = append(argsSlice, fmt.Sprintf("%v", item)) - } + argsInterface = v default: return nil, fmt.Errorf("args must be []string or []interface{}, got %T", args) } } - - // Convert []string args to []interface{} for Eval - argsInterface := make([]interface{}, len(argsSlice)) - for i, arg := range argsSlice { - argsInterface[i] = arg - }This preserves the original types from JavaScript (numbers, booleans, etc.) without unnecessary string conversions, which is more correct for Redis Lua scripts.
243-243: Consider using the provided context parameter.The Eval call uses
context.Background()instead of thectxparameter passed to the function. This bypasses any timeout or cancellation configured by the caller.Apply this diff to respect the caller's context:
- infoCmd := client.Eval(context.Background(), script, keysSlice, argsInterface...) + infoCmd := client.Eval(ctx, script, keysSlice, argsInterface...)Note: This pattern is already used in other functions in this file (e.g., line 43, 49, 84).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/integration-test/javascript.go(5 hunks)pkg/js/libs/redis/redis.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/integration-test/javascript.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Format Go code usinggo fmt ./...
Run static analysis usinggo vet ./...
Files:
pkg/js/libs/redis/redis.go
pkg/js/**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
JavaScript runtime custom implementations for code protocol templates should use auto-generated bindings from pkg/js/generated/
Files:
pkg/js/libs/redis/redis.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint
🔇 Additional comments (1)
pkg/js/libs/redis/redis.go (1)
174-183: LGTM! Clear documentation of backward compatibility.The function signature and documentation clearly explain both the old and new usage patterns, making it easy for users to understand the changes.
| // Convert interface{} to []string for args (handle backwards compatibility) | ||
| argsSlice := []string{} | ||
| if args != nil { | ||
| switch v := args.(type) { | ||
| case []string: | ||
| argsSlice = v | ||
| case []interface{}: | ||
| // Convert []interface{} to []string (from JavaScript arrays) | ||
| argsSlice = make([]string, 0, len(v)) | ||
| for _, item := range v { | ||
| if s, ok := item.(string); ok { | ||
| argsSlice = append(argsSlice, s) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical inconsistency: args still silently drop non-string items.
The args conversion at lines 229-231 only keeps string items and silently drops non-strings, while the keys conversion (lines 213-215) uses fmt.Sprintf("%v", item) to preserve all types. This inconsistency will cause confusing bugs where args like ['key1', 123, true] become ['key1'], causing Lua scripts to fail.
Redis Lua scripts commonly use numeric and boolean arguments, so this silent data loss is a significant correctness issue.
Apply this diff to make args handling consistent with keys:
// Convert interface{} to []string for args (handle backwards compatibility)
argsSlice := []string{}
if args != nil {
switch v := args.(type) {
case []string:
argsSlice = v
case []interface{}:
// Convert []interface{} to []string (from JavaScript arrays)
argsSlice = make([]string, 0, len(v))
for _, item := range v {
- if s, ok := item.(string); ok {
- argsSlice = append(argsSlice, s)
- }
+ argsSlice = append(argsSlice, fmt.Sprintf("%v", item))
}
+ default:
+ return nil, fmt.Errorf("args must be []string or []interface{}, got %T", args)
}
}Note: This addresses the same concern raised in the past review comment about silent data loss.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In pkg/js/libs/redis/redis.go around lines 219-234 the args conversion silently
drops non-string items (only appends items asserted as string), causing
inconsistent behavior vs. keys which uses fmt.Sprintf("%v", item); change the
args handling to mirror keys: when args is []string keep as-is, when args is
[]interface{} iterate each item and append fmt.Sprintf("%v", item) so
numbers/booleans are preserved as their string representation, and ensure
args==nil remains handled.
There was a problem hiding this comment.
I think it's worth to address.
There was a problem hiding this comment.
@dwisiswant0, thanks for confirming! You're absolutely right that this inconsistency should be fixed.
The current implementation silently drops non-string arguments (lines 229-231), which would cause Lua scripts to fail when expecting numeric or boolean values. Since Redis Lua scripts commonly use these types as arguments, this creates a significant correctness issue.
@Mzack9999, please apply the suggested fix to make args handling consistent with keys:
Change the args conversion (lines 226-234) to:
// Convert interface{} to []string for args (handle backwards compatibility)
argsSlice := []string{}
if args != nil {
switch v := args.(type) {
case []string:
argsSlice = v
case []interface{}:
// Convert []interface{} to []string (from JavaScript arrays)
argsSlice = make([]string, 0, len(v))
for _, item := range v {
argsSlice = append(argsSlice, fmt.Sprintf("%v", item))
}
default:
return nil, fmt.Errorf("args must be []string or []interface{}, got %T", args)
}
}This ensures that arguments like ['key1', 123, true] are properly converted to their string representations instead of being silently dropped, which aligns with how Redis EVAL expects arguments to be passed.
Proposed changes
Closes #4790
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.