fix: add security hardening to MongoDB plugin RAW and form commands#41569
Open
RaikaSurendra wants to merge 3 commits intoappsmithorg:releasefrom
Open
fix: add security hardening to MongoDB plugin RAW and form commands#41569RaikaSurendra wants to merge 3 commits intoappsmithorg:releasefrom
RaikaSurendra wants to merge 3 commits intoappsmithorg:releasefrom
Conversation
Restrict RAW query mode to a data-operation allowlist (find, insert, update, delete, aggregate, count, distinct, findAndModify, getMore, bulkWrite) to prevent arbitrary admin commands like dropDatabase, eval, and shutdown. Block dangerous aggregation pipeline stages ($out, $merge, $function, $accumulator) and query operators ($where, $function, $accumulator) in form-mode commands.
Contributor
WalkthroughImplements security validations for the MongoDB plugin: adds a command whitelist, blocks dangerous aggregation pipeline stages and query operators, and integrates recursive document validation across command parsing and execution paths. Changes
Sequence DiagramsequenceDiagram
participant Client as Client
participant Exec as MongoPluginExecutor
participant Cmd as CommandClass
participant Utils as MongoPluginUtils
participant DB as MongoDB
Client->>Exec: executeCommon(commandPayload)
Exec->>Exec: parse payload -> commandDoc
Exec->>Exec: extract commandName
Exec->>Exec: check ALLOWED_COMMANDS
alt command not allowed
Exec-->>Client: AppsmithPluginException (DISALLOWED_COMMAND_ERROR_MSG)
else command allowed
Exec->>Cmd: route to specific command (find/aggregate/...)
Cmd->>Cmd: parse query/projection/update/pipeline into Document/Bson
Cmd->>Utils: validateQueryDocument(fieldName, document)
Cmd->>Utils: (for aggregate) validatePipelineStage(stageDoc)
alt validation fails
Utils-->>Cmd: AppsmithPluginException (DISALLOWED_*)
Cmd-->>Client: Error
else validation passes
Cmd->>DB: execute validated command
DB-->>Cmd: result
Cmd-->>Client: result
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 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 |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Find.java (1)
84-90:⚠️ Potential issue | 🟡 Minor
projectionis not validated — minor gap on MongoDB 5.0+.MongoDB 5.0+ supports aggregation expressions in projections, including
$function. If a user controls theprojectionfield (e.g. via a mustache binding), they could inject:{"computed": {"$function": {"body": "function() {...}", "args": [], "lang": "js"}}}Passing
parseSafely("Projection", this.projection)result throughvalidateQueryDocumentwould close this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Find.java` around lines 84 - 90, The projection field isn't being validated for unsafe MongoDB aggregation expressions; after parsing this.projection with parseSafely("Projection", this.projection) (used in the Find class), pass the parsed result into validateQueryDocument before putting it into document (the same way sort is handled) so document.put("projection", ...) uses the validated document; reference parseSafely, validateQueryDocument, the projection variable and the document.put("projection", ...) call to locate and update the code.app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java (1)
342-364:⚠️ Potential issue | 🟠 MajorSecurity error message swallowed — disallowed command produces generic "query failed" instead of the specific rejection message.
The
AppsmithPluginExceptionthrown for a blocked command (lines 351–354) is anException, so it is caught by thecatch (Exception error)block on line 359 and re-wrapped:return Mono.error(new AppsmithPluginException( MongoPluginError.QUERY_EXECUTION_FAILED, MongoPluginErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG, // ← generic message error)); // ← original buried as causeThe carefully crafted
DISALLOWED_COMMAND_ERROR_MSGbecomes a buried cause.onErrorResumesees anAppsmithPluginExceptionand passes it through, but the user-visible message is "Your Mongo query failed to execute." The command IS blocked (security is intact), but the error type is wrong (QUERY_EXECUTION_FAILEDvsPLUGIN_EXECUTE_ARGUMENT_ERROR) and the descriptive message is lost.The simplest fix is to pull the synchronous validation above the
tryblock, since it has no async or resource-cleanup dependency:🔒 Proposed fix — hoist validation above the try block
query = PluginUtils.getDataValueSafelyFromFormData(formData, BODY, STRING_TYPE); + + // Validate command before entering the reactive try block so that + // AppsmithPluginExceptions are not swallowed by the generic catch. + Document commandDoc = Document.parse(query); + if (commandDoc.isEmpty()) { + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + MongoPluginErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG); + } + String commandName = commandDoc.keySet().iterator().next().toLowerCase(); + if (!ALLOWED_COMMANDS.contains(commandName)) { + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + String.format(MongoPluginErrorMessages.DISALLOWED_COMMAND_ERROR_MSG, commandName)); + } + try { log.debug(Thread.currentThread().getName() + ": mongoClient.getDatabase from Mongo plugin."); MongoDatabase database = mongoClient.getDatabase(getDatabaseName(datasourceConfiguration)); final Map<String, Object> formData = actionConfiguration.getFormData(); query = PluginUtils.getDataValueSafelyFromFormData(formData, BODY, STRING_TYPE); - Document commandDoc = Document.parse(query); - if (commandDoc.isEmpty()) { - throw new AppsmithPluginException( - AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, - MongoPluginErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG); - } - String commandName = - commandDoc.keySet().iterator().next().toLowerCase(); - if (!ALLOWED_COMMANDS.contains(commandName)) { - throw new AppsmithPluginException( - AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, - String.format(MongoPluginErrorMessages.DISALLOWED_COMMAND_ERROR_MSG, commandName)); - } Bson command = commandDoc;Note:
queryis used later in the.map(...)block via the captured variable, so keep thequery = ...assignment inside thetryblock as well (or refactor accordingly).Alternatively, add a targeted catch before the generic one:
+ } catch (AppsmithPluginException e) { + return Mono.error(e); } catch (Exception error) { return Mono.error(new AppsmithPluginException(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java` around lines 342 - 364, The synchronous command validation is being swallowed by the broad catch, so hoist the validation that checks Document.parse(query) emptiness and ALLOWED_COMMANDS membership out of (or before) the try block (referencing Document.parse, commandDoc, ALLOWED_COMMANDS, commandName and the AppsmithPluginException thrown with MongoPluginErrorMessages.DISALLOWED_COMMAND_ERROR_MSG) so that blocked-command cases throw the correct AppsmithPluginError (PLUGIN_EXECUTE_ARGUMENT_ERROR) and message; alternatively, add a specific catch for AppsmithPluginException/argument errors before the generic catch so the original exception type and DISALLOWED_COMMAND_ERROR_MSG are propagated instead of being re-wrapped as MongoPluginError.QUERY_EXECUTION_FAILED.app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/UpdateMany.java (1)
90-95:⚠️ Potential issue | 🟠 Major
$function/$accumulatorcan be injected via the Update pipeline — update document is not validated.MongoDB 4.4+ accepts an aggregation pipeline as the
"u"value (aList<Document>). A user could bypass the query-level block with an update like:[{ "$replaceWith": { "$function": { "body": "function() {...}", "args": [], "lang": "js" } } }]
parseSafelyDocumentAndArrayOfDocumentshappily parses this butvalidateQueryDocumentis never called on the result. The fix is straightforward — extend the same validation to the update document/pipeline.🔒 Proposed fix — validate the update document
Object updateDocument = parseSafelyDocumentAndArrayOfDocuments("Update", this.update); + if (updateDocument instanceof Document) { + validateQueryDocument("Update", (Document) updateDocument); + } else if (updateDocument instanceof List) { + for (Object item : (List<?>) updateDocument) { + if (item instanceof Document) { + validateQueryDocument("Update", (Document) item); + } + } + } Document update = new Document();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/UpdateMany.java` around lines 90 - 95, The update pipeline/document parsed by parseSafelyDocumentAndArrayOfDocuments("Update", this.update) is not being validated, allowing dangerous stages like $function/$accumulator to slip through; ensure the same validation used for queries is applied to updates by invoking validateQueryDocument on the parsed updateDocument (and if updateDocument is a List<Document> iterate and validate each pipeline stage Document) before putting it into the outgoing update Document (the local variable update used with keys "q","u","multi" in UpdateMany.java).
🧹 Nitpick comments (2)
app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java (1)
50-69:fieldNameparameter is silently unused inside the method body.
fieldNameis threaded through every recursive call butDISALLOWED_QUERY_OPERATOR_ERROR_MSGonly formats the operator key —fieldNamenever appears in the output. Error messages like "Query operator '$where' is not permitted…" give no clue which command field (Query, Update, Pipeline…) contains the violation.♻️ Suggested: include fieldName in error message
Update
DISALLOWED_QUERY_OPERATOR_ERROR_MSGinMongoPluginErrorMessages.java:- public static final String DISALLOWED_QUERY_OPERATOR_ERROR_MSG = - "Query operator '%s' is not permitted for security reasons."; + public static final String DISALLOWED_QUERY_OPERATOR_ERROR_MSG = + "Operator '%s' in field '%s' is not permitted for security reasons.";Then in
validateQueryDocument:- String.format(MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG, key)); + String.format(MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG, key, fieldName));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java` around lines 50 - 69, The validateQueryDocument method currently passes fieldName through recursive calls but never uses it in the thrown AppsmithPluginException; update the DISALLOWED_QUERY_OPERATOR_ERROR_MSG in MongoPluginErrorMessages to accept both the field name and operator (e.g. "Field '%s' contains disallowed query operator '%s'") and change the throw in validateQueryDocument (method name: validateQueryDocument in MongoPluginUtils) to format that message with fieldName and key so the exception includes which command field (Query/Update/Pipeline) contains the violation; keep recursive calls as-is.app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java (1)
44-45: Consider adding$unionWithto the blocklist.
$unionWithaccepts an optional nestedpipelineand allows reading from an arbitrary collection, which can be a cross-collection data exfiltration vector. While fixing the recursive scan (above) will catch$function/$accumulatorembedded inside a$unionWithpipeline, the stage itself may warrant explicit blocking depending on your threat model.♻️ Suggested addition
- private static final Set<String> BLOCKED_PIPELINE_STAGES = - Set.of("$out", "$merge", "$function", "$accumulator"); + private static final Set<String> BLOCKED_PIPELINE_STAGES = + Set.of("$out", "$merge", "$function", "$accumulator", "$unionWith");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java` around lines 44 - 45, The BLOCKED_PIPELINE_STAGES set in Aggregate currently omits "$unionWith", which can allow cross-collection reads via its nested pipeline; update the BLOCKED_PIPELINE_STAGES constant (in class Aggregate, symbol BLOCKED_PIPELINE_STAGES) to include "$unionWith" in the Set.of(...) initializer so the stage is explicitly blocked. Ensure any unit tests or callers that depend on this blocklist are updated accordingly.
🤖 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/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java`:
- Around line 90-103: The current top-level-only stage check in Aggregate must
be replaced with a recursive document scanner: add a private helper method
(e.g., validatePipelineStage(BsonDocument doc)) that walks each entry, checks
entry.getKey().toLowerCase() against BLOCKED_PIPELINE_STAGES and throws the
existing AppsmithPluginException with
MongoPluginErrorMessages.DISALLOWED_PIPELINE_STAGE_ERROR_MSG when matched, and
recurses into value.asDocument() and iterates value.asArray() elements to
recurse on embedded documents; then call validatePipelineStage(...) from the
loop over arrayListFromInput (replacing the existing top-level key loop) and
from the single-document path so nested expression operators like $function and
$accumulator are detected.
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/exceptions/MongoPluginErrorMessages.java`:
- Around line 95-97: The error message constant DISALLOWED_COMMAND_ERROR_MSG
shows a mixed-case allowed-commands list while the rejected command name is
lowercased before formatting; update the allowed-commands list in
DISALLOWED_COMMAND_ERROR_MSG to use lowercase names (e.g., find, insert, update,
delete, aggregate, count, distinct, findandmodify, getmore, bulkwrite) so the
casing matches the lowercased commandName and the message reads consistently.
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java`:
- Around line 61-66: The current validation in MongoPluginUtils only recurses
into List items that are Document instances inside validateQueryDocument, so
nested List-in-List cases are skipped; add a private helper method
validateQueryList(String fieldName, List<?> list) that iterates items and calls
validateQueryDocument(fieldName, (Document) item) for Document instances and
recursively calls validateQueryList(fieldName, (List<?>) item) for List
instances, then replace the existing List handling in validateQueryDocument to
call validateQueryList(fieldName, (List<?>) value).
---
Outside diff comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Find.java`:
- Around line 84-90: The projection field isn't being validated for unsafe
MongoDB aggregation expressions; after parsing this.projection with
parseSafely("Projection", this.projection) (used in the Find class), pass the
parsed result into validateQueryDocument before putting it into document (the
same way sort is handled) so document.put("projection", ...) uses the validated
document; reference parseSafely, validateQueryDocument, the projection variable
and the document.put("projection", ...) call to locate and update the code.
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/UpdateMany.java`:
- Around line 90-95: The update pipeline/document parsed by
parseSafelyDocumentAndArrayOfDocuments("Update", this.update) is not being
validated, allowing dangerous stages like $function/$accumulator to slip
through; ensure the same validation used for queries is applied to updates by
invoking validateQueryDocument on the parsed updateDocument (and if
updateDocument is a List<Document> iterate and validate each pipeline stage
Document) before putting it into the outgoing update Document (the local
variable update used with keys "q","u","multi" in UpdateMany.java).
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java`:
- Around line 342-364: The synchronous command validation is being swallowed by
the broad catch, so hoist the validation that checks Document.parse(query)
emptiness and ALLOWED_COMMANDS membership out of (or before) the try block
(referencing Document.parse, commandDoc, ALLOWED_COMMANDS, commandName and the
AppsmithPluginException thrown with
MongoPluginErrorMessages.DISALLOWED_COMMAND_ERROR_MSG) so that blocked-command
cases throw the correct AppsmithPluginError (PLUGIN_EXECUTE_ARGUMENT_ERROR) and
message; alternatively, add a specific catch for
AppsmithPluginException/argument errors before the generic catch so the original
exception type and DISALLOWED_COMMAND_ERROR_MSG are propagated instead of being
re-wrapped as MongoPluginError.QUERY_EXECUTION_FAILED.
---
Duplicate comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java`:
- Around line 124-131: The current check in Aggregate.java iterates only over
the top-level keys (for (String key : document.keySet())) allowing blocked
pipeline stages to be hidden in nested Documents or Lists; update the validation
used here to mirror the recursive sanitizer used earlier by recursively
traversing the document structure (handling org.bson.Document and java.util.List
nodes), lowercasing each encountered map key and checking against
BLOCKED_PIPELINE_STAGES, and throwing the same AppsmithPluginException
(AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR with
MongoPluginErrorMessages.DISALLOWED_PIPELINE_STAGE_ERROR_MSG) when a blocked
stage is found; ensure the logic is implemented as a reusable helper (e.g.,
validateNoBlockedPipelineStages(Document) or similar) and invoked from the
Aggregate code path so nested keys cannot bypass the check.
---
Nitpick comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java`:
- Around line 44-45: The BLOCKED_PIPELINE_STAGES set in Aggregate currently
omits "$unionWith", which can allow cross-collection reads via its nested
pipeline; update the BLOCKED_PIPELINE_STAGES constant (in class Aggregate,
symbol BLOCKED_PIPELINE_STAGES) to include "$unionWith" in the Set.of(...)
initializer so the stage is explicitly blocked. Ensure any unit tests or callers
that depend on this blocklist are updated accordingly.
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java`:
- Around line 50-69: The validateQueryDocument method currently passes fieldName
through recursive calls but never uses it in the thrown AppsmithPluginException;
update the DISALLOWED_QUERY_OPERATOR_ERROR_MSG in MongoPluginErrorMessages to
accept both the field name and operator (e.g. "Field '%s' contains disallowed
query operator '%s'") and change the throw in validateQueryDocument (method
name: validateQueryDocument in MongoPluginUtils) to format that message with
fieldName and key so the exception includes which command field
(Query/Update/Pipeline) contains the violation; keep recursive calls as-is.
...rver/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java
Show resolved
Hide resolved
...gins/mongoPlugin/src/main/java/com/external/plugins/exceptions/MongoPluginErrorMessages.java
Outdated
Show resolved
Hide resolved
.../appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java
Outdated
Show resolved
Hide resolved
- Aggregate: recursive scan of pipeline stage bodies to catch $function and $accumulator used as expression operators inside $project/$group - MongoPlugin: catch AppsmithPluginException before generic Exception so the specific disallowed-command error message surfaces to users - Find: validate projection field to block $function on MongoDB 5.0+ - MongoPluginUtils: recurse into nested List-in-List in query validation - MongoPluginErrorMessages: lowercase command names in error message to match the lowercased input
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java (1)
342-364:⚠️ Potential issue | 🟠 Major
AppsmithPluginExceptionfrom the ALLOWED_COMMANDS check is silently re-wrapped, burying the user-facing error message.When
!ALLOWED_COMMANDS.contains(commandName)fires, it throws anAppsmithPluginExceptionwith the helpfulDISALLOWED_COMMAND_ERROR_MSG. That exception is immediately caught by the outercatch (Exception error)block and re-wrapped inside a new, genericQUERY_EXECUTION_FAILEDexception — so the user sees "Your Mongo query failed to execute." as the primary message instead of the specific "MongoDB command 'X' is not permitted..." text. The same applies to the empty-command check at Line 344.Move the validation before the
tryblock, or add a typed catch forAppsmithPluginExceptionto let it propagate untouched:🐛 Proposed fix — propagate plugin exceptions directly
+ } catch (AppsmithPluginException error) { + return Mono.error(error); } catch (Exception error) { return Mono.error(new AppsmithPluginException( MongoPluginError.QUERY_EXECUTION_FAILED, MongoPluginErrorMessages.QUERY_EXECUTION_FAILED_ERROR_MSG, error)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java` around lines 342 - 364, The current catch-all is wrapping user-facing AppsmithPluginException (thrown during Document.parse/commandDoc validation and ALLOWED_COMMANDS check) into a generic QUERY_EXECUTION_FAILED, losing the specific DISALLOWED_COMMAND_ERROR_MSG; fix by either moving the Document.parse + empty check and ALLOWED_COMMANDS.contains(commandName) validation out of the try block so they can throw AppsmithPluginException directly, or add a typed catch for AppsmithPluginException before the generic catch to rethrow it unchanged (do not wrap), ensuring AppsmithPluginException and its message (e.g., from MongoPluginErrorMessages.DISALLOWED_COMMAND_ERROR_MSG) propagate to the caller.
🧹 Nitpick comments (1)
app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java (1)
50-57:fieldNameis threaded through recursive calls but never surfaced in the error message.The parameter is declared, propagated to all recursive invocations, yet the thrown
AppsmithPluginExceptionformats onlykey—fieldNameis never used. Either include it in the error message for better context (e.g.,"Query operator '%s' found in field '%s' is not permitted"), or drop the parameter entirely if it adds no value.♻️ Option A – use fieldName in the error message
- String.format(MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG, key)); + String.format(MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG, key, fieldName));Update
DISALLOWED_QUERY_OPERATOR_ERROR_MSGto include a second%s:- public static final String DISALLOWED_QUERY_OPERATOR_ERROR_MSG = - "Query operator '%s' is not permitted for security reasons."; + public static final String DISALLOWED_QUERY_OPERATOR_ERROR_MSG = + "Query operator '%s' in field '%s' is not permitted for security reasons.";♻️ Option B – drop the unused parameter
- public static void validateQueryDocument(String fieldName, Document queryDoc) { + public static void validateQueryDocument(Document queryDoc) { if (queryDoc == null) return; for (String key : queryDoc.keySet()) { if (BLOCKED_QUERY_OPERATORS.contains(key.toLowerCase())) { ... if (value instanceof Document) { - validateQueryDocument(fieldName, (Document) value); + validateQueryDocument((Document) value); } else if (value instanceof List) { for (Object item : (List<?>) value) { if (item instanceof Document) { - validateQueryDocument(fieldName, (Document) item); + validateQueryDocument((Document) item); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java` around lines 50 - 57, The validateQueryDocument method currently accepts fieldName but never uses it in the thrown AppsmithPluginException; update the thrown error to include fieldName for context by changing MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG to accept two placeholders and passing both key and fieldName into String.format (i.e., String.format(DISALLOWED_QUERY_OPERATOR_ERROR_MSG, key, fieldName)); keep the existing recursive calls to validateQueryDocument as-is so fieldName is threaded through, and update any tests/messages that expect the old single-argument format string.
🤖 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/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java`:
- Around line 342-355: The parsed raw command Document is only allowlisted by
command name but not scanned for dangerous query operators; update the
executeCommon block (the code that creates Document commandDoc and sets Bson
command) to recursively validate any nested query/filter subdocuments before
passing to database.runCommand(). Reuse the existing validateQueryDocument(...)
helper (or implement a small recursive wrapper that calls validateQueryDocument
on nested Documents/Lists) to inspect keys such as "filter", "query",
"pipeline", and any nested Documents within commandDoc for disallowed operators
like $where/$function, and throw an AppsmithPluginException when validation
fails; do this check right after Document.parse(query) and before assigning Bson
command = commandDoc so RAW mode commands are sanitized the same way as
form-mode handlers.
---
Outside diff comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java`:
- Around line 342-364: The current catch-all is wrapping user-facing
AppsmithPluginException (thrown during Document.parse/commandDoc validation and
ALLOWED_COMMANDS check) into a generic QUERY_EXECUTION_FAILED, losing the
specific DISALLOWED_COMMAND_ERROR_MSG; fix by either moving the Document.parse +
empty check and ALLOWED_COMMANDS.contains(commandName) validation out of the try
block so they can throw AppsmithPluginException directly, or add a typed catch
for AppsmithPluginException before the generic catch to rethrow it unchanged (do
not wrap), ensuring AppsmithPluginException and its message (e.g., from
MongoPluginErrorMessages.DISALLOWED_COMMAND_ERROR_MSG) propagate to the caller.
---
Duplicate comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java`:
- Around line 90-103: The current top-level-only scan in class Aggregate misses
nested operators like $function/$accumulator and $out/$merge inside embedded
documents/arrays; implement a recursive helper (e.g.,
validatePipelineDocument(BsonDocument)) that iterates each entry, checks key
against BLOCKED_PIPELINE_STAGES, and when a value is a document or array
recurses into contained documents, and then replace the existing top-level loop
to call this helper for each stage (and for arrays inside stages) so nested
pipeline fields (including $lookup.pipeline) are validated too.
- Around line 124-131: The top-level-only check over document.keySet() misses
nested operators (e.g., $function/$accumulator); replace the direct loop with a
call to the recursive validator used for the array path: for each entry in the
document invoke validatePipelineDocument(entry.getValue(),
BLOCKED_PIPELINE_STAGES) and if it detects a blocked stage throw the same
AppsmithPluginException with
MongoPluginErrorMessages.DISALLOWED_PIPELINE_STAGE_ERROR_MSG; ensure the
validatePipelineDocument helper (the same one used for the array path) is
present and used here so nested maps/arrays are traversed rather than only
checking top-level keys.
- Around line 44-45: BLOCKED_PIPELINE_STAGES incorrectly includes
expression-level operators ($function, $accumulator) that will never match
top-level stage keys in parseCommand(); remove those two from
BLOCKED_PIPELINE_STAGES (keep only actual stage names like $out and $merge) and
add a separate expression-level blocked set (e.g., BLOCKED_EXPRESSION_OPERATORS)
and a recursive visitor that scans each pipeline stage document's values to
detect nested occurrences of expression operators; update parseCommand() (and
the downstream checks that currently rely on BLOCKED_PIPELINE_STAGES) to 1)
check top-level stage names against BLOCKED_PIPELINE_STAGES and 2) recursively
scan stage value documents using the new visitor and
BLOCKED_EXPRESSION_OPERATORS to reject pipelines containing forbidden expression
operators such as $function and $accumulator.
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/exceptions/MongoPluginErrorMessages.java`:
- Around line 95-97: The error message in DISALLOWED_COMMAND_ERROR_MSG conflicts
with how MongoPlugin.executeCommon lowercases commandName before formatting (so
rejected commands show as "findandmodify"), create consistency by matching
casing: either stop lowercasing the command name in MongoPlugin.executeCommon
when building the error, or update DISALLOWED_COMMAND_ERROR_MSG to use a
lowercased allowed-list (e.g., "find, insert, update, delete, aggregate, count,
distinct, findandmodify, getmore, bulkwrite") so the formatted %s and the list
use the same casing; update the code at the symbol MongoPlugin.executeCommon or
the constant DISALLOWED_COMMAND_ERROR_MSG accordingly.
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java`:
- Around line 61-66: The list branch in MongoPluginUtils.validateQueryDocument
silently skips items that are themselves Lists; update the loop in the "else if
(value instanceof List)" block to detect when an item is a List (instanceof
List) and recursively iterate into it (handling nested List<?> contents), and
when an item is a Document call validateQueryDocument(fieldName, (Document)
item) as before; this ensures nested List-within-List structures (e.g., $or:
[[...]] ) are validated rather than ignored.
---
Nitpick comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java`:
- Around line 50-57: The validateQueryDocument method currently accepts
fieldName but never uses it in the thrown AppsmithPluginException; update the
thrown error to include fieldName for context by changing
MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG to accept two
placeholders and passing both key and fieldName into String.format (i.e.,
String.format(DISALLOWED_QUERY_OPERATOR_ERROR_MSG, key, fieldName)); keep the
existing recursive calls to validateQueryDocument as-is so fieldName is threaded
through, and update any tests/messages that expect the old single-argument
format string.
app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java (1)
50-65:fieldNameparameter is accepted but never used — optional cleanup.Both
validateQueryDocumentandvalidateQueryListreceivefieldNameand forward it recursively, but it is never incorporated into the thrown exception's message. Either drop the parameter or include it in the error string (e.g.,"Query operator '%s' in field '%s' is not permitted...") so callers get richer diagnostics.♻️ Proposed refactor — incorporate fieldName in error message
- throw new AppsmithPluginException( - AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, - String.format(MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG, key)); + throw new AppsmithPluginException( + AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, + String.format(MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG, key) + + " (in field: " + fieldName + ")");Or, if
fieldNameisn't needed at all, remove it from both method signatures and all call sites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java` around lines 50 - 65, The fieldName parameter is accepted but never used; update validateQueryDocument and validateQueryList to include fieldName in the thrown AppsmithPluginException so errors show which field contains the disallowed operator. Modify the String.format call that uses MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG to include fieldName (e.g., include "%s" for fieldName) when constructing the error, and ensure every recursive call (validateQueryDocument and validateQueryList) continues to pass the current fieldName; keep using BLOCKED_QUERY_OPERATORS and the same exception type (AppsmithPluginException) so callers get richer diagnostics without changing control flow.
🤖 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/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java`:
- Around line 136-137: The single-document pipeline path currently parses the
pipeline into a Document via parseSafely("Array of pipelines", this.pipeline)
and calls validateQueryDocument("Pipeline", document), which only checks
BLOCKED_QUERY_OPERATORS and misses pipeline-stage blocks like $out/$merge;
update this branch to also validate top-level pipeline stages by invoking the
same pipeline-stage checks used for arrays (e.g., call validateBsonValue or
validatePipelineStage on the parsed Document or iterate the Document's top-level
keys and ensure none are in BLOCKED_PIPELINE_STAGES) so that $out/$merge and
other blocked pipeline stages are rejected for single-document pipelines as
well.
---
Nitpick comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java`:
- Around line 50-65: The fieldName parameter is accepted but never used; update
validateQueryDocument and validateQueryList to include fieldName in the thrown
AppsmithPluginException so errors show which field contains the disallowed
operator. Modify the String.format call that uses
MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG to include
fieldName (e.g., include "%s" for fieldName) when constructing the error, and
ensure every recursive call (validateQueryDocument and validateQueryList)
continues to pass the current fieldName; keep using BLOCKED_QUERY_OPERATORS and
the same exception type (AppsmithPluginException) so callers get richer
diagnostics without changing control flow.
Comment on lines
136
to
+137
| Document document = parseSafely("Array of pipelines", this.pipeline); | ||
| validateQueryDocument("Pipeline", document); |
Contributor
There was a problem hiding this comment.
$out and $merge bypass validation on the single-document pipeline path — security gap.
When the pipeline input is a bare JSON object (not a JSON array), execution falls into this branch, parses it as a Document, then calls validateQueryDocument("Pipeline", document). That method only consults BLOCKED_QUERY_OPERATORS ({"$where", "$function", "$accumulator"}), which does not include $out or $merge. A user supplying {"$out": "exfiltration"} takes this path, passes validation, and the aggregation writes results to an arbitrary collection — exactly the data-exfiltration scenario this PR is meant to prevent.
The array path correctly calls validateBsonValue → validatePipelineStage which covers BLOCKED_PIPELINE_STAGES. Per MongoDB docs, $out and $merge are always top-level stage keys, so a top-level key scan on the Document is sufficient to close the gap.
🛡️ Proposed fix — add BLOCKED_PIPELINE_STAGES check on the single-doc path
Document document = parseSafely("Array of pipelines", this.pipeline);
-validateQueryDocument("Pipeline", document);
+// Block $out/$merge (top-level stage operators not covered by validateQueryDocument)
+for (String key : document.keySet()) {
+ if (BLOCKED_PIPELINE_STAGES.contains(key.toLowerCase())) {
+ throw new AppsmithPluginException(
+ AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR,
+ String.format(MongoPluginErrorMessages.DISALLOWED_PIPELINE_STAGE_ERROR_MSG, key));
+ }
+}
+// Block nested JS operators ($function, $accumulator, $where)
+validateQueryDocument("Pipeline", document);
ArrayList<Document> documentArrayList = new ArrayList<>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java`
around lines 136 - 137, The single-document pipeline path currently parses the
pipeline into a Document via parseSafely("Array of pipelines", this.pipeline)
and calls validateQueryDocument("Pipeline", document), which only checks
BLOCKED_QUERY_OPERATORS and misses pipeline-stage blocks like $out/$merge;
update this branch to also validate top-level pipeline stages by invoking the
same pipeline-stage checks used for arrays (e.g., call validateBsonValue or
validatePipelineStage on the parsed Document or iterate the Document's top-level
keys and ensure none are in BLOCKED_PIPELINE_STAGES) so that $out/$merge and
other blocked pipeline stages are rejected for single-document pipelines as
well.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/UpdateMany.java (1)
88-100:⚠️ Potential issue | 🟠 MajorUpdate body is not validated —
$function/$accumulatorcan bypass via update pipeline.The query filter is correctly guarded, but
updateDocument(returned byparseSafelyDocumentAndArrayOfDocuments) is passed directly into the command with no operator checks. MongoDB 4.2+ accepts an aggregation pipeline as the update argument — e.g.,[{$addFields: {x: {$function: {body: "...", lang: "js"}}}}]— which executes server-side JavaScript and bypasses the protections applied to the query filter.🛡️ Proposed fix — validate the update document/pipeline
Object updateDocument = parseSafelyDocumentAndArrayOfDocuments("Update", this.update); + if (updateDocument instanceof Document) { + validateQueryDocument("Update", (Document) updateDocument); + } else if (updateDocument instanceof List) { + for (Object stage : (List<?>) updateDocument) { + if (stage instanceof Document) { + validateQueryDocument("Update", (Document) stage); + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/UpdateMany.java` around lines 88 - 100, The updateDocument produced by parseSafelyDocumentAndArrayOfDocuments is not validated and can be an aggregation pipeline (array) that includes dangerous stages like $function/$accumulator; modify UpdateMany to validate the update before packaging it into the command: add a call (e.g., validateUpdateDocument or reuse validateQueryDocument-style checks) after parseSafelyDocumentAndArrayOfDocuments to reject or sanitize aggregation-pipeline updates and to ensure no top-level update operators or pipeline stages contain disallowed operators ($function, $accumulator, etc.); ensure this validation runs before creating the update Document (the code that builds update with keys "q","u","multi" and the list assigned to document.put("updates", updates)).
🧹 Nitpick comments (2)
app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java (1)
50-75:fieldNameis passed through all recursive calls but never referenced in any error message.
DISALLOWED_QUERY_OPERATOR_ERROR_MSGis formatted withkeyonly;fieldNameis dead weight in bothvalidateQueryDocumentandvalidateQueryList. Either drop the parameter entirely, or surface it in the error message for better diagnostics (e.g.,"Query operator '%s' is not permitted in field '%s'").♻️ Option A — remove unused parameter
- public static void validateQueryDocument(String fieldName, Document queryDoc) { + public static void validateQueryDocument(Document queryDoc) { if (queryDoc == null) return; for (String key : queryDoc.keySet()) { if (BLOCKED_QUERY_OPERATORS.contains(key.toLowerCase())) { throw new AppsmithPluginException( AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR, String.format(MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG, key)); } Object value = queryDoc.get(key); if (value instanceof Document) { - validateQueryDocument(fieldName, (Document) value); + validateQueryDocument((Document) value); } else if (value instanceof List) { - validateQueryList(fieldName, (List<?>) value); + validateQueryList((List<?>) value); } } } - private static void validateQueryList(String fieldName, List<?> list) { + private static void validateQueryList(List<?> list) { for (Object item : list) { if (item instanceof Document) { - validateQueryDocument(fieldName, (Document) item); + validateQueryDocument((Document) item); } else if (item instanceof List) { - validateQueryList(fieldName, (List<?>) item); + validateQueryList((List<?>) item); } } }All call sites would drop the first string argument.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java` around lines 50 - 75, The fieldName parameter is unused in the error thrown by validateQueryDocument/validateQueryList; update the error message to include the field name instead of removing the parameter: modify MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG to include two placeholders (e.g., for operator and field), then change the String.format call in validateQueryDocument to pass both key and fieldName (String.format(..., key, fieldName)); keep recursive calls to validateQueryDocument and validateQueryList passing fieldName unchanged so diagnostics show the originating field when the AppsmithPluginException is thrown.app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/exceptions/MongoPluginErrorMessages.java (1)
94-103: Consider adding a section banner for the new security-related constants.All other logical groups in this file are preceded by a
***comment block. The three new constants are appended after the datasource section without one, making them harder to spot during future maintenance.♻️ Suggested addition
+ /* + ************************************************************************************************************************************************ + Error messages related to security validation of commands and operators. + ************************************************************************************************************************************************ + */ + public static final String DISALLOWED_COMMAND_ERROR_MSG =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/exceptions/MongoPluginErrorMessages.java` around lines 94 - 103, Add a section banner comment (matching the file's existing '***' style) above the three new security constants to group them as "Security-related error messages" for clarity; locate the constants DISALLOWED_COMMAND_ERROR_MSG, DISALLOWED_PIPELINE_STAGE_ERROR_MSG, and DISALLOWED_QUERY_OPERATOR_ERROR_MSG and insert the banner comment block immediately before them so they are visually separated from the datasource section and consistent with other logical groups in the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/UpdateMany.java`:
- Around line 88-100: The updateDocument produced by
parseSafelyDocumentAndArrayOfDocuments is not validated and can be an
aggregation pipeline (array) that includes dangerous stages like
$function/$accumulator; modify UpdateMany to validate the update before
packaging it into the command: add a call (e.g., validateUpdateDocument or reuse
validateQueryDocument-style checks) after parseSafelyDocumentAndArrayOfDocuments
to reject or sanitize aggregation-pipeline updates and to ensure no top-level
update operators or pipeline stages contain disallowed operators ($function,
$accumulator, etc.); ensure this validation runs before creating the update
Document (the code that builds update with keys "q","u","multi" and the list
assigned to document.put("updates", updates)).
---
Duplicate comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java`:
- Around line 136-137: The single-document pipeline path in Aggregate uses
parseSafely and validateQueryDocument which only checks BLOCKED_QUERY_OPERATORS,
allowing dangerous pipeline stages like $out/$merge to slip through; update the
single-doc path to run the same pipeline validation used for array pipelines by
converting the parsed Document into a BsonValue and passing it through
validateBsonValue and validatePipelineStage (reusing the same logic that
enforces BLOCKED_PIPELINE_STAGES) before converting to documentArrayList, or
call the existing array-path validation helper directly so
parseSafely/validateQueryDocument are augmented to reject blocked pipeline
stages such as $out and $merge (reference parseSafely, validateQueryDocument,
validateBsonValue, validatePipelineStage, BLOCKED_PIPELINE_STAGES, and
documentArrayList).
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java`:
- Around line 342-355: RAW mode currently checks ALLOWED_COMMANDS but passes
commandDoc (from Document.parse) straight to database.runCommand() without
inspecting nested operators, allowing dangerous keys like
$where/$function/$accumulator; fix this by reusing validateQueryDocument to
recursively validate sub-documents before creating Bson command: after computing
commandName and verifying ALLOWED_COMMANDS, call validateQueryDocument on
relevant fields in commandDoc (e.g., "filter", "query", and any pipeline stages
under "pipeline" or stage-specific sub-docs such as in "group" or "aggregate")
and reject/throw AppsmithPluginException for any invalid operators before
assigning Bson command and calling database.runCommand().
---
Nitpick comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/exceptions/MongoPluginErrorMessages.java`:
- Around line 94-103: Add a section banner comment (matching the file's existing
'***' style) above the three new security constants to group them as
"Security-related error messages" for clarity; locate the constants
DISALLOWED_COMMAND_ERROR_MSG, DISALLOWED_PIPELINE_STAGE_ERROR_MSG, and
DISALLOWED_QUERY_OPERATOR_ERROR_MSG and insert the banner comment block
immediately before them so they are visually separated from the datasource
section and consistent with other logical groups in the file.
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/utils/MongoPluginUtils.java`:
- Around line 50-75: The fieldName parameter is unused in the error thrown by
validateQueryDocument/validateQueryList; update the error message to include the
field name instead of removing the parameter: modify
MongoPluginErrorMessages.DISALLOWED_QUERY_OPERATOR_ERROR_MSG to include two
placeholders (e.g., for operator and field), then change the String.format call
in validateQueryDocument to pass both key and fieldName (String.format(..., key,
fieldName)); keep recursive calls to validateQueryDocument and validateQueryList
passing fieldName unchanged so diagnostics show the originating field when the
AppsmithPluginException is thrown.
- Add RAW mode operator validation: recursively scan command body for blocked operators ($where, $function, $accumulator, $out, $merge) before passing to runCommand() - Expand blocked operator set to include $out and $merge, closing the single-document aggregate pipeline bypass for these stages - Validate update document/pipeline in UpdateMany to block $function and $accumulator injection via MongoDB 4.2+ update pipelines - Validate sort document in Find for defense-in-depth consistency - Add $where to Aggregate BLOCKED_PIPELINE_STAGES for array path - Include field name in operator error messages for richer diagnostics - Add section banner for security constants in error messages file
Contributor
Author
Security Disclosure: MongoDB Plugin Command InjectionSeverity: Critical (CWE-77: Command Injection) SummaryThe Appsmith MongoDB plugin's RAW query mode ( Affected Component
Fix Implemented — 4-Layer DefenseLayer 1: Command Allowlist in Layer 2: RAW Mode Operator Validation in Layer 3: Pipeline Stage Blocklist in Layer 4: Query/Update Operator Blocklist in Form Commands Files Modified (9 files, 134 insertions, 6 deletions)
Impact Coverage
Reported by Surendra Raika (RaikaSurendra) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java (1)
46-47: Duplicate blocked-operator sets — consider consolidating.
BLOCKED_PIPELINE_STAGEShere andBLOCKED_OPERATORSinMongoPluginUtils.javaare identical ($out,$merge,$function,$accumulator,$where). SinceAggregate.javaalready imports fromMongoPluginUtils, define the set once there and reference it from both validators to reduce maintenance burden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java` around lines 46 - 47, Duplicate blocked-operator sets exist: remove the local BLOCKED_PIPELINE_STAGES constant from Aggregate.java and reuse the canonical set defined as BLOCKED_OPERATORS in MongoPluginUtils; update Aggregate.java to reference MongoPluginUtils.BLOCKED_OPERATORS (or import it statically) where BLOCKED_PIPELINE_STAGES was used, ensure any tests or validators still compile and behave the same, and delete the now-unused BLOCKED_PIPELINE_STAGES symbol to avoid redundancy.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java`:
- Around line 136-137: Single-document pipeline path must block $out/$merge;
ensure the code around Document document = parseSafely("Array of pipelines",
this.pipeline) calls validateQueryDocument("Pipeline", document) and that
MongoPluginUtils.BLOCKED_OPERATORS includes "$out" and "$merge" so
validateQueryDocument enforces the block for the single-document pipeline path
(check the parseSafely, validateQueryDocument and
MongoPluginUtils.BLOCKED_OPERATORS symbols).
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/MongoPlugin.java`:
- Around line 349-357: The current change validates the entire RAW-mode command
document but still extracts commandName with
commandDoc.keySet().iterator().next() which will throw if the document is empty;
add an explicit empty-check on commandDoc before extracting the first key and
throw an AppsmithPluginException (similar to the existing error style) for a
missing/empty command, then proceed to check ALLOWED_COMMANDS, call
validateQueryDocument("Command", commandDoc) and assign Bson command =
commandDoc; reference commandDoc, ALLOWED_COMMANDS, validateQueryDocument, and
MongoPluginErrorMessages.DISALLOWED_COMMAND_ERROR_MSG when making the fix.
- Around line 343-357: Add a null-check before parsing the query string so
Document.parse(query) is never called with a null value: if the value returned
from getDataValueSafelyFromFormData (the local variable query) is null or empty,
throw an AppsmithPluginException with
AppsmithPluginError.PLUGIN_EXECUTE_ARGUMENT_ERROR using a clear message from
MongoPluginErrorMessages (e.g., a new or existing null/empty query message)
instead of letting Document.parse throw; place this guard immediately before the
Document.parse(query) call in the MongoPlugin class and keep the subsequent
logic (commandName check, ALLOWED_COMMANDS, validateQueryDocument, Bson command
assignment) unchanged.
---
Nitpick comments:
In
`@app/server/appsmith-plugins/mongoPlugin/src/main/java/com/external/plugins/commands/Aggregate.java`:
- Around line 46-47: Duplicate blocked-operator sets exist: remove the local
BLOCKED_PIPELINE_STAGES constant from Aggregate.java and reuse the canonical set
defined as BLOCKED_OPERATORS in MongoPluginUtils; update Aggregate.java to
reference MongoPluginUtils.BLOCKED_OPERATORS (or import it statically) where
BLOCKED_PIPELINE_STAGES was used, ensure any tests or validators still compile
and behave the same, and delete the now-unused BLOCKED_PIPELINE_STAGES symbol to
avoid redundancy.
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
find,insert,update,delete,aggregate,count,distinct,findandmodify,getmore,bulkwrite) are permitted viadatabase.runCommand()$out,$merge,$function,$accumulator) inAggregate.java$where,$function,$accumulator) with recursive validation in Find, Count, Delete, Distinct, and UpdateMany form commandsDescription
The MongoDB plugin's RAW query mode passes user-supplied BSON directly to
database.runCommand()with no command validation. Any authenticated Appsmith user can execute arbitrary MongoDB administrative commands includingdropDatabase,createUser,shutdown,eval, andgetLog. The form-based mode also accepts dangerous operators ($wherefor server-side JS execution) and pipeline stages ($outfor data exfiltration).This PR adds a 3-layer defense:
MongoPlugin.executeCommon()— validates the first key of the parsed BSON document against a set of permitted data operations before passing torunCommand()Aggregate.java— rejects$out,$merge,$function,$accumulatorstages in both form-parsed and raw pipeline arraysMongoPluginUtils.validateQueryDocument()— recursively scans parsed query documents for$where,$function,$accumulatoracross all form commands (Find, Count, Delete, Distinct, UpdateMany)Test plan
find,insert,update,delete,aggregate,count,distinctdropDatabase,createUser,shutdown,eval,getLog{"$where": "sleep(5000)"}in query filter[{"$match": {}}, {"$out": "exfil"}]pipeline$where/$functionin querySummary by CodeRabbit