query validation: explicit confirm_delete_all + clear too-deep error#7697
Closed
ar2rsawseen wants to merge 1 commit into
Closed
query validation: explicit confirm_delete_all + clear too-deep error#7697ar2rsawseen wants to merge 1 commit into
ar2rsawseen wants to merge 1 commit into
Conversation
…p sentinel to a clear message via common.unsafeQueryError Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up hardening pass on the “reject unsafe Mongo query operators” work, tightening the /i/app_users/delete near-total-delete guard and standardizing unsafe-query rejection messaging so internal sentinels aren’t exposed to clients/logs.
Changes:
- Require
confirm_delete_allto be explicitlytrue(boolean) or"true"(string) when usingforceand the delete query matches nearly all users. - Introduce
common.unsafeQueryError(bad)to map theUNSAFE_QUERY_TOO_DEEPsentinel to a clear client-safe message. - Update several endpoints/plugins to use the centralized unsafe-query error formatter (partially complete in this diff).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
plugins/remote-config/api/api.js |
Switches unsafe-query rejection log to common.unsafeQueryError (response still needs alignment). |
plugins/push/api/legacy.js |
Switches unsafe-query rejection log to common.unsafeQueryError (returned error still needs alignment). |
plugins/push/api/api-tx.js |
Switches unsafe-query rejection log to common.unsafeQueryError (thrown error still needs alignment). |
plugins/push/api/api-message.js |
Switches unsafe-query rejection log to common.unsafeQueryError (thrown error still needs alignment). |
plugins/dbviewer/api/api.js |
Uses common.unsafeQueryError for both logging and client response. |
api/utils/requestProcessor.js |
Tightens /i/app_users/delete near-total-delete guard to require explicit confirm_delete_all=true. |
api/utils/common.js |
Adds common.unsafeQueryError and routes parseUserQuery errors through it. |
api/parts/data/exports.js |
Uses common.unsafeQueryError for both logging and client response. |
| if (badOp) { | ||
| log.d("Rejected user query" + common.reqInfo(params) + ": " + "Query contains disallowed operator: " + badOp); | ||
| log.d("Rejected user query" + common.reqInfo(params) + ": " + common.unsafeQueryError(badOp)); | ||
| common.returnMessage(params, 400, 'Query contains disallowed operator: ' + badOp); |
| if (badOp) { | ||
| log.d("Rejected user query" + common.reqInfo(params) + ": " + "Query contains disallowed operator: " + badOp); | ||
| log.d("Rejected user query" + common.reqInfo(params) + ": " + common.unsafeQueryError(badOp)); | ||
| return [{error: 'Query contains disallowed operator: ' + badOp}]; |
| if (badOp) { | ||
| log.d("Rejected user query" + common.reqInfo(params) + ": " + "Query contains disallowed operator: " + badOp); | ||
| log.d("Rejected user query" + common.reqInfo(params) + ": " + common.unsafeQueryError(badOp)); | ||
| throw new ValidationError('Query contains disallowed operator: ' + badOp); |
| if (badOp) { | ||
| log.d("Rejected user query" + common.reqInfo(params) + ": " + "Query contains disallowed operator: " + badOp); | ||
| log.d("Rejected user query" + common.reqInfo(params) + ": " + common.unsafeQueryError(badOp)); | ||
| throw new ValidationError('Query contains disallowed operator: ' + badOp); |
Member
Author
|
Superseded — folding these review fixes into the already-open #7695 (same branch |
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
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.
Follow-up to #7694 / #7695 addressing review comments raised on the 24.05 backport (#7696) that also apply to master.
/i/app_users/deletenear-total guard: require an explicitconfirm_delete_alloftrue(or"true"). Previously any truthy value (e.g. the string"false") bypassed the guard.common.unsafeQueryError(bad)which maps the internalUNSAFE_QUERY_TOO_DEEPsentinel to a clear "Query is nested too deeply" message.parseUserQueryand all directfindUnsafeMongoOperatorcallers (dbviewer, exports, remote-config, push ×3) now use it, so$__nestedTooDeepis no longer surfaced to logs/clients.No behavior change for valid queries. Cherry-pick of the same fix applied to the 24.05 backport (#7696).