feat: Add Parse Server option enableSanitizedErrorResponse to remove detailed error messages from responses sent to clients
#9944
Conversation
|
I will reformat the title to use the proper commit message syntax. |
parse-github-assistant
bot
changed the title
|
🚀 Thanks for opening this pull request! |
📝 WalkthroughWalkthroughAdds a new option to control sanitized error responses (enableSanitizedErrorResponse) and threads server/request Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/RestWrite.js (1)
193-214: User/class permission errors now obey disableSanitizeErrorUsing
createSanitizedError(this.config)in:
validateClientClassCreation(non-existent client classes),checkRestrictedFields(manualemailVerifiedupdates), andrunDatabaseOperation(unauthenticated_Userupdates)keeps the default behavior but allows detailed messages when
disableSanitizeErroris true. Note that, with the flag enabled, these messages will expose class names, field names, and userobjectIds; that’s fine for debugging but should be documented as dev/debug-only guidance for production operators.Also applies to: 659-671, 1457-1463
src/RestQuery.js (1)
414-435: Query-time OPERATION_FORBIDDEN errors now use createSanitizedError with configIn
_UnsafeRestQuery.validateClientClassCreationanddenyProtectedFields, switching tocreateSanitizedError(..., this.config)brings read-side permission failures (non-existent classes, querying protected fields) in line with write-side behavior. WithdisableSanitizeError = true, clients will see which class/field was blocked, so it’s worth calling out in docs that this flag is intended for development/debugging rather than locked-down production environments.Also applies to: 789-812
src/Error.js (2)
3-10: Update Error helper JSDoc for the new config / disableSanitizeError behaviorThe docblocks for
createSanitizedErrorandcreateSanitizedHttpErrorstill describe only two parameters and don’t mention theconfigargument or thatconfig.disableSanitizeErrorcontrols whether the detailed message is returned. Updating the JSDoc to document the third parameter and its effect will avoid confusion for contributors reading this file.Also applies to: 22-29
1-44: Ensure disableSanitizeError is exposed in Options and regenerated definitionsSince this module now relies on
config.disableSanitizeError, please double‑check that:
src/Options/index.jsexposes adisableSanitizeErrorParse Server option, andnpm run definitionshas been run sosrc/Options/docs.jsandsrc/Options/Definitions.jsinclude it.README.md documentation for the new option is optional but would be helpful for users enabling detailed errors in development. Based on learnings, this is the expected process for new Parse Server options.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
spec/ParseFile.spec.js(1 hunks)spec/Utils.spec.js(2 hunks)src/Controllers/SchemaController.js(3 hunks)src/Error.js(3 hunks)src/GraphQL/loaders/schemaMutations.js(3 hunks)src/GraphQL/loaders/schemaQueries.js(2 hunks)src/GraphQL/loaders/usersQueries.js(2 hunks)src/GraphQL/parseGraphQLUtils.js(1 hunks)src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)src/Options/index.js(1 hunks)src/RestQuery.js(4 hunks)src/RestWrite.js(4 hunks)src/Routers/ClassesRouter.js(1 hunks)src/Routers/FilesRouter.js(1 hunks)src/Routers/GlobalConfigRouter.js(1 hunks)src/Routers/GraphQLRouter.js(1 hunks)src/Routers/PurgeRouter.js(1 hunks)src/Routers/PushRouter.js(1 hunks)src/Routers/SchemasRouter.js(3 hunks)src/Routers/UsersRouter.js(3 hunks)src/SharedRest.js(2 hunks)src/middlewares.js(2 hunks)src/rest.js(7 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:24.824Z
Learning: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion with every new commit to the PR.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
src/Options/docs.jssrc/Options/index.jssrc/Options/Definitions.js
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Applied to files:
src/Options/docs.jssrc/Options/index.jssrc/Options/Definitions.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/ParseFile.spec.jsspec/Utils.spec.js
📚 Learning: 2025-09-21T15:43:32.265Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9858
File: src/GraphQL/ParseGraphQLServer.js:176-178
Timestamp: 2025-09-21T15:43:32.265Z
Learning: The GraphQL playground feature in ParseGraphQLServer.js (applyPlayground method) is intended for development environments only, which is why it includes the master key in client-side headers.
Applied to files:
src/GraphQL/parseGraphQLUtils.jssrc/Routers/GraphQLRouter.js
📚 Learning: 2025-08-27T12:33:06.237Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:467-477
Timestamp: 2025-08-27T12:33:06.237Z
Learning: In the Parse Server codebase, maybeRunAfterFindTrigger is called in production with Parse.Query objects constructed via withJSON(), so the plain object query handling bug only affects tests, not production code paths.
Applied to files:
src/RestQuery.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/Utils.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/Utils.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/Utils.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/Utils.spec.js
🧬 Code graph analysis (8)
src/GraphQL/loaders/usersQueries.js (2)
src/Routers/GlobalConfigRouter.js (1)
config(9-9)src/middlewares.js (3)
config(208-208)config(642-642)config(644-644)
src/RestWrite.js (3)
src/Controllers/SchemaController.js (1)
config(724-724)src/middlewares.js (3)
config(208-208)config(642-642)config(644-644)src/cloud-code/Parse.Cloud.js (2)
config(602-602)config(690-690)
src/GraphQL/parseGraphQLUtils.js (1)
src/middlewares.js (4)
enforceMasterKeyAccess(503-511)config(208-208)config(642-642)config(644-644)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
src/Controllers/SchemaController.js (2)
src/Routers/GlobalConfigRouter.js (1)
config(9-9)src/Config.js (1)
Config(40-829)
src/SharedRest.js (2)
src/Controllers/SchemaController.js (2)
config(724-724)Parse(18-18)src/middlewares.js (3)
config(208-208)config(642-642)config(644-644)
src/GraphQL/loaders/schemaQueries.js (2)
src/GraphQL/parseGraphQLUtils.js (1)
enforceMasterKeyAccess(5-13)src/middlewares.js (4)
enforceMasterKeyAccess(503-511)config(208-208)config(642-642)config(644-644)
src/GraphQL/loaders/schemaMutations.js (1)
src/GraphQL/parseGraphQLUtils.js (1)
enforceMasterKeyAccess(5-13)
⏰ 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). (16)
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Node 22
- GitHub Check: Node 18
- GitHub Check: Node 20
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Docker Build
- GitHub Check: Code Analysis (javascript)
- GitHub Check: Benchmarks
🔇 Additional comments (29)
src/Controllers/SchemaController.js (4)
1402-1402: LGTM! Config retrieval pattern is appropriate.Retrieving the config via
Config.get(Parse.applicationId)is the correct pattern for this static method context where instance state is not available. The config is then properly propagated to all error construction calls below.
1408-1418: LGTM! Error construction properly includes config context.Both error paths correctly pass the config as the third argument to
createSanitizedError, enabling the newdisableSanitizeErroroption to control error sanitization for authentication-related permission failures.
1432-1436: LGTM! Operation forbidden error properly includes config.The error construction correctly passes the config context, allowing the sanitization behavior to be controlled by the
disableSanitizeErroroption.
1456-1460: LGTM! Final permission error properly includes config.The error construction correctly passes the config context for this final permission denial path.
src/Routers/PushRouter.js (1)
12-18: LGTM! Error construction properly includes config context.The read-only masterKey error path correctly passes
req.configas the third argument tocreateSanitizedError, enabling the newdisableSanitizeErroroption to control error sanitization.src/Routers/PurgeRouter.js (1)
8-14: LGTM! Error construction properly includes config context.The read-only masterKey error path correctly passes
req.configas the third argument tocreateSanitizedError, enabling the newdisableSanitizeErroroption to control error sanitization.src/GraphQL/loaders/schemaQueries.js (2)
29-45: LGTM! MasterKey enforcement properly includes config context.The
enforceMasterKeyAccesscall correctly passes bothauthandconfigparameters, matching the updated function signature. This enables the error thrown byenforceMasterKeyAccessto respect thedisableSanitizeErrorconfiguration option.
56-70: LGTM! MasterKey enforcement properly includes config context.The
enforceMasterKeyAccesscall correctly passes bothauthandconfigparameters, matching the updated function signature. This enables the error thrown byenforceMasterKeyAccessto respect thedisableSanitizeErrorconfiguration option.src/Routers/GlobalConfigRouter.js (1)
43-50: LGTM! Error construction properly includes config context.The read-only masterKey error path correctly passes
req.configas the third argument tocreateSanitizedError, enabling the newdisableSanitizeErroroption to control error sanitization.src/Options/docs.js (1)
40-40: LGTM! Documentation is clear and accurate.The documentation clearly describes the purpose of the
disableSanitizeErroroption. Based on learnings, ensure that this file was generated by runningnpm run definitionsrather than manually edited, as this file should be auto-generated from the definitions insrc/Options/index.js.Based on learnings, for new Parse Server options, verify that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js.
src/Options/index.js (1)
350-352: Verification complete—option definitions properly generated.All required components are in place:
- ✓ Option documented in
src/Options/index.js(lines 350-352)- ✓ Definition in
src/Options/Definitions.js(lines 202-207) with environment variablePARSE_SERVER_DISABLE_SANITIZE_ERROR, parser, help text, and default value- ✓ Documentation in
src/Options/docs.js(line 40)The
npm run definitionshas been executed, and all changes are correctly reflected.src/Routers/GraphQLRouter.js (1)
17-22: LGTM!The config parameter is correctly passed to
createSanitizedError, enabling config-aware error sanitization for read-only masterKey restrictions.spec/ParseFile.spec.js (1)
769-779: LGTM!The test correctly validates the new error handling behavior where invalid application IDs now return detailed error messages instead of the generic "Permission denied" response.
src/Routers/ClassesRouter.js (1)
109-116: LGTM!The config parameter is correctly propagated to
createSanitizedErrorfor the invalid object ID restriction.src/Routers/SchemasRouter.js (1)
73-81: LGTM!The config parameter is correctly passed to all three schema operation error paths (create, update, delete) for read-only masterKey restrictions.
Also applies to: 96-104, 113-120
src/GraphQL/loaders/schemaMutations.js (1)
29-42: LGTM!The GraphQL schema mutations correctly propagate the config parameter to both
enforceMasterKeyAccessandcreateSanitizedError, ensuring consistent config-aware error handling across CreateClass, UpdateClass, and DeleteClass operations.Also applies to: 79-92, 131-144
src/middlewares.js (1)
503-511: LGTM!Both middleware functions correctly pass the config parameter to
createSanitizedHttpErrorfor master key access enforcement.Also applies to: 513-518
spec/Utils.spec.js (2)
1-2: LGTM!The import changes are correct. Tests should import from
../lib(compiled code) rather than../src.
178-204: LGTM!The test suites comprehensively validate the new
disableSanitizeErrorfunctionality for bothcreateSanitizedErrorandcreateSanitizedHttpError, covering both enabled and disabled states.src/Options/Definitions.js (1)
202-208: No issues found. Option is properly documented and definitions are in sync.The
disableSanitizeErroroption is correctly documented insrc/Options/index.js(lines 350–352) and present in bothsrc/Options/docs.jsandsrc/Options/Definitions.jswith synchronized descriptions, indicating thatnpm run definitionshas been executed.src/Routers/UsersRouter.js (1)
173-201: Config-aware sanitized errors in UsersRouter look correctUsing
req.configin thecreateSanitizedErrorcalls for/users/meand/loginAscorrectly aligns these paths with the newdisableSanitizeErroroption without changing authorization logic.Also applies to: 336-343
src/GraphQL/parseGraphQLUtils.js (1)
5-13: GraphQL master-key enforcement now supports disableSanitizeError via configPassing
configintocreateSanitizedErrorhere is consistent with REST/middleware behavior; just ensure allenforceMasterKeyAccesscall sites have been updated to provide the newconfigargument so the helper doesn’t receiveundefined.src/SharedRest.js (1)
12-44: enforceRoleSecurity now config-aware; behavior preservedWiring
configthroughenforceRoleSecurityand intocreateSanitizedErrorfor installation/master-only/read-only cases cleanly integrates the new sanitization flag without altering existing permission checks.src/RestWrite.js (1)
31-38: Constructor read-only master key guard aligned with sanitization helperThrowing
createSanitizedErrorwithconfigwhenauth.isReadOnlyblocks writes from the read-only master key in all call sites and ensures the newdisableSanitizeErroroption applies consistently here as well.src/GraphQL/loaders/usersQueries.js (1)
9-13: Config passed into sanitized INVALID_SESSION_TOKEN errors for GraphQL viewerBoth the early “no session token” branch and the “no user found” branch now call
createSanitizedErrorwithconfig, matching REST andUsersRouterbehavior while enablingdisableSanitizeError.Also applies to: 55-67
src/rest.js (2)
137-149: Propagating config into enforceRoleSecurity across REST verbs
find,get,del,create, andupdatenow passconfigintoenforceRoleSecurity, so all class-level and read-only master-key permission denials participate in thedisableSanitizeErrorbehavior without changing which operations are allowed.Also applies to: 152-164, 176-177, 267-268, 276-277
193-263: handleSessionMissingError now emits config-aware SESSION_MISSING errorsUsing
handleSessionMissingError(error, className, auth, config)in thedelandupdatechains and mapping_UserOBJECT_NOT_FOUNDerrors tocreateSanitizedError(Parse.Error.SESSION_MISSING, 'Insufficient auth.', config)preserves the previous semantics while letting operators opt into detailed messages viadisableSanitizeError; the synchronousthrowinside the helper still correctly rethrows from the Promisecatch.Also applies to: 322-331
src/RestQuery.js (1)
39-80: RestQuery role enforcement and _Session INVALID_SESSION_TOKEN now config-awareCalling
enforceRoleSecurity(method, className, auth, config)and usingcreateSanitizedError(..., config)when a non-master user queries_Sessionensures both role-based access control and invalid-session errors respect the newdisableSanitizeErrorflag, with no change to the underlying authorization rules.Also applies to: 121-139
src/Error.js (1)
11-20: ****The concern about missing
configparameters is unfounded. Verification of all call sites in the codebase shows:
- 40+ calls to
createSanitizedError— every single one passes aconfigparameter (config,this.config, orreq.config)- 2 calls to
createSanitizedHttpError— both passrequest.configorreq.config- Tests pass, confirming that function signatures and call sites are aligned
There are no legacy or future call sites that pass only
(code, message)orundefined. The functions are correctly called throughout the codebase.Minor note: The JSDoc comments in Error.js (lines 8–10, 27–28) do not document the
configparameter, but this is a documentation gap, not a functional issue.Likely an incorrect or invalid review comment.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9944 +/- ##
==========================================
+ Coverage 93.08% 93.75% +0.66%
==========================================
Files 188 188
Lines 15298 15297 -1
Branches 177 177
==========================================
+ Hits 14240 14341 +101
+ Misses 1046 944 -102
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
spec/Utils.spec.js (1)
178-214: Good coverage of config-driven behavior; consider tightening test namesThe new specs nicely cover:
- default/sanitized behavior (
disableSanitizeErrorfalse or missing), and- detailed behavior (
disableSanitizeErrortrue)
for bothcreateSanitizedErrorandcreateSanitizedHttpError.Two minor nits:
- The first test names say
"false or undefined"but only coverfalse; undefined is covered separately. You could rename to"when disableSanitizeError is false"for clarity.- The
"should not crash with config undefined"tests are also verifying that the message is sanitized; renaming to something like"should sanitize message when config is undefined"would better describe the assertion.Also, these tests rely on a global
Parse(forParse.Error.OPERATION_FORBIDDEN), which is standard in this repo; just confirm there’s no intention to change that test setup.src/Error.js (1)
11-20: Update JSDoc to document the newconfigparameterThe new
configargument is not yet reflected in the JSDoc. To keep tooling and future readers aligned with the implementation, consider adding it, e.g.:/** * @param {Object} [config] - Parse Server config; when config.disableSanitizeError is true, * the detailed message is exposed to the client instead of being sanitized. */ function createSanitizedError(errorCode, detailedMessage, config) {Same applies to the JSDoc above
createSanitizedHttpError.If you want to be strict that only an explicit boolean
truedisables sanitization, you could also tighten the condition toconfig?.disableSanitizeError === true, but the current truthy check is fine if that’s the intended semantics.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
spec/Utils.spec.js(2 hunks)src/Error.js(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:24.824Z
Learning: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion with every new commit to the PR.
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/Utils.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/Utils.spec.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/Utils.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/Utils.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/Utils.spec.js
🧬 Code graph analysis (2)
spec/Utils.spec.js (1)
src/Error.js (1)
error(38-38)
src/Error.js (1)
src/middlewares.js (5)
config(208-208)config(642-642)config(644-644)error(252-252)error(505-505)
⏰ 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). (16)
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Node 20
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Node 22
- GitHub Check: Node 18
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Code Analysis (javascript)
- GitHub Check: Benchmarks
🔇 Additional comments (2)
spec/Utils.spec.js (1)
1-2: Import updates look consistent with lib build usageRequiring
../lib/Utilsand../lib/Erroraligns these tests with the built artifacts; this looks fine and keeps tests exercising the same entry points as production.src/Error.js (1)
30-41: Config-driven sanitization logic is correct; ensure option wiring/docs are in syncThe HTTP helper mirrors the Parse.Error helper correctly:
- always logs the detailed message server-side, and
- conditionally exposes the detailed message vs
"Permission denied"to clients based onconfig?.disableSanitizeError.This matches the intended
disableSanitizeErrorbehavior. Given this is controlled by a config flag that can weaken error-hiding, please ensure:
- The new option is clearly described as development/debug only in the relevant docs (Options docs and, optionally, README).
src/Options/index.js,src/Options/docs.js, andsrc/Options/Definitions.jsare consistent and thatnpm run definitionshas been run so generated files match the source. Based on learnings, this is the expected workflow for new options.Behavior itself looks good.
|
@mtrezza do we need to be enable by default or not ? Use case A:
Use case B:
Basically, does the option should be an opt-in or an opt-out ? |
|
If disabled by default, we could merge in PS8 which makes it available for more users, but we'd want to add a deprecation notice. If enabled by default, and since we don't consider changing error messages a breaking change, there would be no breaking change notice in the change log, hence it could be overlooked when upgrading to PS9, as per our discussion on Slack. |
|
So what you prefer disabled or enabled by default ? If we enabled it by default this will need to disable the value by default in tests to keep test like they are. And maybe depreciate the option in ps9 |
|
@coratgerl What variant requires the least work for you? |
mtrezza
changed the title
enableSanitizedErrorResponse to remove detailed error messages from responses sent to clients
|
The opt-out is simple for me, this is how I implement the current PR |
|
Then let's do it this way. If you could just rename the option, see my review above. |
Signed-off-by: Manuel <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Options/docs.js (1)
48-48: Option doc forenableSanitizedErrorResponselooks good; consider discoverability + title tweakThe description is clear and matches the behavior / default in
src/Options/Definitions.js. As an optional improvement, consider:
- Adding a short mention of this option (and its env var) to the README configuration section so users can easily discover it.
- Using an Angular-style PR title for the changelog, for example:
feat(security): add option to sanitize client error responsesBased on learnings, new options should be in
index.js+ generated intodocs.js/Definitions.js, and README coverage is a nice-to-have.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
spec/Utils.spec.js(2 hunks)src/Error.js(3 hunks)src/Options/Definitions.js(1 hunks)src/Options/docs.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Error.js
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:48.786Z
Learning: For Parse Server PRs, always suggest an Angular commit convention PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion on every commit. The format should be: type(scope): description. Common types include feat, fix, perf, refactor, docs, test, chore. The scope should identify the subsystem (e.g., graphql, rest, push, security). The description should be action-oriented and clearly convey the change's impact to developers.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-17T15:02:24.824Z
Learning: For Parse Server PRs, always suggest an Angular-style PR title that would make a meaningful changelog entry for developers. Update the PR title suggestion with every new commit to the PR.
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: For new Parse Server options, verify that the option is documented in src/Options/index.js and that npm run definitions has been executed to reflect changes in src/Options/docs.js and src/Options/Definitions.js. README.md documentation is a bonus but not required for new options.
Applied to files:
src/Options/Definitions.jssrc/Options/docs.js
📚 Learning: 2025-11-08T13:46:04.940Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 0
File: :0-0
Timestamp: 2025-11-08T13:46:04.940Z
Learning: When reviewing Parse Server PRs that add new features, always check whether the feature is documented in the README.md file, though for new Parse Server options this is optional rather than required.
Applied to files:
src/Options/Definitions.jssrc/Options/docs.js
📚 Learning: 2025-04-30T19:31:35.344Z
Learnt from: RahulLanjewar93
Repo: parse-community/parse-server PR: 9744
File: spec/ParseLiveQuery.spec.js:0-0
Timestamp: 2025-04-30T19:31:35.344Z
Learning: In the Parse Server codebase, the functions in QueryTools.js are typically tested through end-to-end behavior tests rather than direct unit tests, even though the functions are exported from the module.
Applied to files:
spec/Utils.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: Tests in the parse-server repository should use promise-based approaches rather than callback patterns with `done()`. Use a pattern where a Promise is created that resolves when the event occurs, then await that promise.
Applied to files:
spec/Utils.spec.js
📚 Learning: 2025-10-16T19:27:05.311Z
Learnt from: Moumouls
Repo: parse-community/parse-server PR: 9883
File: spec/CloudCodeLogger.spec.js:410-412
Timestamp: 2025-10-16T19:27:05.311Z
Learning: In spec/CloudCodeLogger.spec.js, the test "should log cloud function triggers using the silent log level" (around lines 383-420) is known to be flaky and requires the extra `await new Promise(resolve => setTimeout(resolve, 100))` timeout after awaiting `afterSavePromise` for reliability, even though it may appear redundant.
Applied to files:
spec/Utils.spec.js
📚 Learning: 2025-05-09T09:59:06.289Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1340-1375
Timestamp: 2025-05-09T09:59:06.289Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`. The preferred pattern is to create a Promise that resolves when an expected event occurs, then await that Promise.
Applied to files:
spec/Utils.spec.js
📚 Learning: 2025-05-04T20:41:05.147Z
Learnt from: mtrezza
Repo: parse-community/parse-server PR: 9445
File: spec/ParseLiveQuery.spec.js:1312-1338
Timestamp: 2025-05-04T20:41:05.147Z
Learning: New tests in the parse-server repository should use async/await with promise-based patterns rather than callback patterns with `done()`.
Applied to files:
spec/Utils.spec.js
🧬 Code graph analysis (2)
src/Options/Definitions.js (1)
resources/buildConfigDefinitions.js (1)
parsers(12-12)
spec/Utils.spec.js (1)
src/Error.js (1)
error(38-38)
🔇 Additional comments (2)
src/Options/Definitions.js (1)
250-256: Config definition forenableSanitizedErrorResponseis consistent and secure by defaultEnv name, help text, boolean parser, and
default: trueall align with the JSDoc indocs.jsand the intended “secure-by-default” behavior (sanitized errors unless explicitly disabled).Based on learnings, this indicates
Options/index.jswas updated andnpm run definitionsre-run as expected.spec/Utils.spec.js (1)
1-2: Switch to../libimports for Utils/Error is appropriateUsing
../lib/Utilsand../lib/Errorkeeps these tests aligned with the built artifacts that Parse Server actually ships, which is consistent with how other specs in this repo are structured.
mtrezza
changed the title
enableSanitizedErrorResponse to remove detailed error messages from responses sent to clientsenableSanitizeErrorResponse to remove detailed error messages from responses sent to clients
mtrezza
changed the title
enableSanitizeErrorResponse to remove detailed error messages from responses sent to clientsenableSanitizedErrorResponse to remove detailed error messages from responses sent to clients
|
Yes ready for merge ! |
# [8.5.0-alpha.16](8.5.0-alpha.15...8.5.0-alpha.16) (2025-11-28) ### Features * Add Parse Server option `enableSanitizedErrorResponse` to remove detailed error messages from responses sent to clients ([#9944](#9944)) ([4752197](4752197))
|
🎉 This change has been released in version 8.5.0-alpha.16 |
4 participants
Pull Request
This PR introduces a new configuration option, disableSanitizeError, to the ParseServerOptions interface. When enabled (disableSanitizeError: true), the server will return detailed error messages to clients instead of the generic "Permission denied" message. This can be useful for debugging or development environments where detailed feedback is required.
Summary by CodeRabbit
New Features
Behavior Changes
Tests
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.