fix: add security hardening to Redis plugin command execution#41570
Open
RaikaSurendra wants to merge 3 commits intoappsmithorg:releasefrom
Open
fix: add security hardening to Redis plugin command execution#41570RaikaSurendra wants to merge 3 commits intoappsmithorg:releasefrom
RaikaSurendra wants to merge 3 commits intoappsmithorg:releasefrom
Conversation
Add a blocklist of dangerous Redis commands to prevent authenticated users from executing server administration, data destruction, scripting, replication, and other high-risk commands through the Redis plugin. Blocked command categories: - Server admin: SHUTDOWN, DEBUG, MONITOR, SAVE, BGSAVE, BGREWRITEAOF - Config manipulation: CONFIG - Replication/exfiltration: SLAVEOF, REPLICAOF - Module loading (RCE): MODULE - Data destruction: FLUSHALL, FLUSHDB - Scripting/RCE: EVAL, EVALSHA, SCRIPT - ACL manipulation: ACL - Cluster/migration: CLUSTER, MIGRATE - Client manipulation: CLIENT - DoS/deserialization: KEYS, RESTORE, SWAPDB, SYNC, PSYNC, SENTINEL Updated existing tests that relied on blocked CLIENT command, and added new tests verifying blocked commands return appropriate error responses.
Contributor
|
No actionable comments were generated in the recent review. 🎉 WalkthroughAdds an early security filter in the Redis plugin that extracts the command name, blocks a defined set of unsafe Redis commands (case-insensitive), returns a specific blocked-command error, tightens typing for command payloads, and updates tests to verify rejection cases and SELECT db behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Plugin as RedisPlugin
participant Errors as RedisErrorMessages
participant Redis as RedisServer
Client->>Plugin: execute(command + args)
Plugin->>Plugin: parse payload -> cmdAndArgs (Map<String,Object>)\nextract commandName (normalize case)
alt commandName in BLOCKED_COMMANDS
Plugin->>Errors: format(BLOCKED_COMMAND_ERROR_MSG, commandName)
Plugin-->>Client: respond(PLUGIN_EXECUTE_ARGUMENT_ERROR, formattedError)
else
Plugin->>Redis: resolve Protocol.Command using commandName\nforward command + args
Redis-->>Plugin: result / error
Plugin-->>Client: respond(result/error)
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (1)
377-415: Assert the error message content, not just the error type.Both tests only verify the error title (
PLUGIN_EXECUTE_ARGUMENT_ERROR). Since the same error type is returned for empty body, parse failures, and invalid commands, a regression could swap the rejection reason without failing the test. The error message does include the command name, so assert that it's present:Suggested assertion additions
// In itShouldRejectBlockedCommand: assertFalse(result.getIsExecutionSuccess()); assertEquals(AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR.getTitle(), result.getTitle()); + assertTrue(result.getBody().toString().contains("FLUSHALL")); // In itShouldRejectBlockedCommandWithArgs: assertFalse(result.getIsExecutionSuccess()); assertEquals(AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR.getTitle(), result.getTitle()); + assertTrue(result.getBody().toString().contains("CONFIG"));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java` around lines 377 - 415, The tests itShouldRejectBlockedCommand and itShouldRejectBlockedCommandWithArgs currently only assert the error title; add an assertion that the returned error message includes the offending command text so regressions changing rejection reason will fail. After the existing title assertions, assert that the error payload (e.g. result.getBody() or result.getMessage(), whichever holds the plugin error text) is not null and contains "FLUSHALL" in it for itShouldRejectBlockedCommand and contains "CONFIG GET requirepass" for itShouldRejectBlockedCommandWithArgs; keep the existing title checks intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java`:
- Around line 65-91: Update the BLOCKED_COMMANDS set in RedisPlugin
(BLOCKED_COMMANDS) to include "FUNCTION" as a blocked command (add the literal
"FUNCTION" into the Set.of(...) alongside the other commands) and update the
surrounding comment to note this is a forward-looking block for Redis 7+ due to
persistent FUNCTION LOAD risks (CVE-style sandbox escape). Do not add "EVAL_RO"
or "EVALSHA_RO" to the blocklist; instead add a brief comment that read-only
eval variants should be controlled via ACL roles when/if support for Redis 7+
and a newer Jedis client is added.
---
Nitpick comments:
In
`@app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java`:
- Around line 377-415: The tests itShouldRejectBlockedCommand and
itShouldRejectBlockedCommandWithArgs currently only assert the error title; add
an assertion that the returned error message includes the offending command text
so regressions changing rejection reason will fail. After the existing title
assertions, assert that the error payload (e.g. result.getBody() or
result.getMessage(), whichever holds the plugin error text) is not null and
contains "FLUSHALL" in it for itShouldRejectBlockedCommand and contains "CONFIG
GET requirepass" for itShouldRejectBlockedCommandWithArgs; keep the existing
title checks intact.
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java
Show resolved
Hide resolved
- Add FUNCTION to blocked commands set (Redis 7+ forward-looking: persistent server-side code loading with CVE-2025-49844 sandbox escape risk). Add comment noting EVAL_RO/EVALSHA_RO should be controlled via Redis ACL roles rather than blanket-blocked. - Strengthen blocked command test assertions to verify the error message contains the offending command name, not just the error type. Prevents regressions that swap rejection reasons silently.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java (1)
125-125: RawMaptype — pre-existing but worth a future cleanup.
getCommandAndArgsreturns a rawMap. Since you're touching the call site, consider typing it asMap<String, Object>to match the declaration at line 242 and avoid unchecked casts.Suggested fix
- Map cmdAndArgs = getCommandAndArgs(query.trim()); + Map<String, Object> cmdAndArgs = getCommandAndArgs(query.trim());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java` at line 125, The call site uses a raw Map for the result of getCommandAndArgs; change the declaration of cmdAndArgs to a parameterized type (Map<String, Object> cmdAndArgs = getCommandAndArgs(query.trim())) to match the method's intended return and the existing typed declaration elsewhere, eliminating the raw type and any unchecked cast warnings; update any subsequent uses of cmdAndArgs if necessary to compile with the generic type.app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (1)
377-399: Solid negative tests for both no-arg and with-arg blocked commands.Both tests verify the error title and that the body contains the offending command name — good for regression detection.
Minor suggestion: consider adding a case-sensitivity test (e.g.,
"flushall"lowercase) to explicitly prove the uppercase normalization ingetCommandAndArgsworks as expected. Not critical since existing code uppercases, but it would document the contract.Also applies to: 401-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java` around lines 377 - 399, Add a new negative test to assert case-insensitive blocking by calling pluginExecutor.execute with a lowercase (or mixed-case) blocked command body (e.g., actionConfiguration.setBody("flushall")) — similar assertions to itShouldRejectBlockedCommand: assert execution failed, title equals AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR.getTitle(), body is not null and contains the offending command name; this documents and verifies the uppercase normalization done in getCommandAndArgs and protects against regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java`:
- Line 125: The call site uses a raw Map for the result of getCommandAndArgs;
change the declaration of cmdAndArgs to a parameterized type (Map<String,
Object> cmdAndArgs = getCommandAndArgs(query.trim())) to match the method's
intended return and the existing typed declaration elsewhere, eliminating the
raw type and any unchecked cast warnings; update any subsequent uses of
cmdAndArgs if necessary to compile with the generic type.
In
`@app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java`:
- Around line 377-399: Add a new negative test to assert case-insensitive
blocking by calling pluginExecutor.execute with a lowercase (or mixed-case)
blocked command body (e.g., actionConfiguration.setBody("flushall")) — similar
assertions to itShouldRejectBlockedCommand: assert execution failed, title
equals AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR.getTitle(), body is not
null and contains the offending command name; this documents and verifies the
uppercase normalization done in getCommandAndArgs and protects against
regressions.
- Parameterize raw Map return type in getCommandAndArgs() and its call site to Map<String, Object>, eliminating unchecked cast warnings. - Add itShouldRejectBlockedCommandCaseInsensitive test to document and verify that lowercase command input is normalized to uppercase before blocklist lookup.
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.
Summary
SHUTDOWN,DEBUG,MONITOR,SAVE,BGSAVE,BGREWRITEAOF), config manipulation (CONFIG), replication/data exfiltration (SLAVEOF,REPLICAOF), module loading/RCE (MODULE), data destruction (FLUSHALL,FLUSHDB), scripting/RCE (EVAL,EVALSHA,SCRIPT), ACL manipulation (ACL), cluster/migration (CLUSTER,MIGRATE), client manipulation (CLIENT), DoS/deserialization risks (KEYS,RESTORE,SWAPDB,SYNC,PSYNC,SENTINEL)GET,SET,HGET,LPUSH,SCAN,TTL,INFO,SUBSCRIBE,PUBLISH, etc.) remain fully functionalChanges
RedisPlugin.java: AddedBLOCKED_COMMANDSset and validation check beforejedis.sendCommand()— blocked commands return a clear error message without reaching the Redis serverRedisErrorMessages.java: AddedBLOCKED_COMMAND_ERROR_MSGconstant with security-section banner (matching MongoDB plugin pattern)RedisPluginTest.java: FixedtestSelectedDatabaseandtestDefaultDatabasetests (previously used blockedCLIENT INFOcommand, replaced withSET/GETandPINGrespectively); addeditShouldRejectBlockedCommandanditShouldRejectBlockedCommandWithArgstestsTest plan
PING,GET,SET,MGET,MSET,DEL,HGET,LPUSH,SCAN,TTL,INFO,DBSIZE,SELECT,SUBSCRIBE,PUBLISHall work normallyFLUSHALL,SHUTDOWN,CONFIG GET requirepass,EVAL "return 1" 0,SLAVEOF,MODULE LOADall return the blocked command errormvn test -pl appsmith-plugins/redisPluginSummary by CodeRabbit
Bug Fixes
Tests