feat: Sanitize error for security #9937
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! |
📝 WalkthroughWalkthroughCentralizes sanitized error handling by adding helpers that log detailed server-side messages and return a generic "Permission denied" to clients. Replaces many direct Parse.Error throws across routers, controllers, core logic, middleware, and GraphQL. Tests updated to expect the generic message and to assert sanitized logging. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Server
participant Logger
note over Client,Server: Authorization failure path (sanitized)
Client->>Server: Request lacking permissions
Server->>Logger: log("Sanitized error: " + detailedMessage)
alt Client response
Server-->>Client: Error(code) + "Permission denied"
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)spec/ParseGraphQLServer.spec.jsspec/ParseUser.spec.jsspec/schemas.spec.jsThanks 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: 3
🧹 Nitpick comments (11)
src/SecurityError.js (2)
13-28: Consider extracting the logger selection logic to reduce duplication.Both
createSanitizedErrorandcreateSanitizedHttpErrorcontain identical logic for selecting the logger. This violates the DRY principle and makes maintenance harder.Apply this diff to extract the common logic:
+/** + * Selects the appropriate logger from the provided parameter + * @param {Object} loggerOrConfig - Optional logger instance or config object + * @returns {Object} The selected logger instance + */ +function selectLogger(loggerOrConfig) { + if (loggerOrConfig) { + if (loggerOrConfig.loggerController) { + return loggerOrConfig.loggerController; + } else if (loggerOrConfig.error) { + return loggerOrConfig; + } + } + return defaultLogger; +} + export function createSanitizedError(errorCode, detailedMessage, loggerOrConfig = null) { - let log = defaultLogger; - if (loggerOrConfig) { - if (loggerOrConfig.loggerController) { - log = loggerOrConfig.loggerController; - } else if (loggerOrConfig.error) { - // It's a logger instance - log = loggerOrConfig; - } - } + const log = selectLogger(loggerOrConfig); // Keep log on server side log.error('Security error:', detailedMessage); return new Parse.Error(errorCode, 'Permission denied'); } export function createSanitizedHttpError(statusCode, detailedMessage, loggerOrConfig = null) { - let log = defaultLogger; - if (loggerOrConfig) { - if (loggerOrConfig.loggerController) { - log = loggerOrConfig.loggerController; - } else if (loggerOrConfig.error) { - log = loggerOrConfig; - } - } + const log = selectLogger(loggerOrConfig); // Keep log on server side log.error('Security error:', detailedMessage); const error = new Error(); error.status = statusCode; error.message = 'Permission denied'; return error; }Also applies to: 39-56
1-57: Suggest improved PR title following Angular commit convention.Based on learnings
The current PR title "feat: sanitize error for security" could be more descriptive and follow the Angular commit convention more precisely. Consider:
feat(security): sanitize error messages to prevent information disclosureThis title:
- Includes the scope (
security)- Clearly conveys the security improvement
- Makes a meaningful changelog entry for developers
src/Routers/FilesRouter.js (1)
8-9: Avoid double‑logging the security error ingetHandler
createSanitizedErroralready logs'Security error:'with the detailed message, so the explicitlog.error('Security error:', detailedError);causes duplicate log entries for the same event.You can rely on
createSanitizedError’s internal logging and drop both the explicit log call and the extra logger argument:-import { createSanitizedError } from '../SecurityError'; -import defaultLogger from '../logger'; +import { createSanitizedError } from '../SecurityError'; @@ if (!config) { res.status(403); - const detailedError = 'Invalid application ID.'; - const log = defaultLogger; - log.error('Security error:', detailedError); - const err = createSanitizedError(Parse.Error.OPERATION_FORBIDDEN, detailedError, log); + const detailedError = 'Invalid application ID.'; + const err = createSanitizedError(Parse.Error.OPERATION_FORBIDDEN, detailedError); res.json({ code: err.code, error: err.message }); return; }This keeps behavior the same (code + sanitized message) while logging only once.
Also applies to: 48-52
spec/ParseGraphQLServer.spec.js (1)
47-61: PR title: suggest Angular-style scoped variantTo make the changelog entry more informative and follow the Angular convention from this repo, consider updating the PR title to something like:
feat(security): sanitize permission error messagesThis reflects the security impact and the main behavior change more clearly.
src/RestQuery.js (1)
432-435: Use config-aware logger consistently when sanitizing errors in RestQuery
denyProtectedFieldscorrectly deriveslogfromthis.config.loggerController || defaultLogger, butvalidateClientClassCreationcurrently callscreateSanitizedErrorwithout passing any logger/config, so it always falls back to the global default.To keep logging consistent and app-scoped, consider:
- if (hasClass !== true) { - throw createSanitizedError( - Parse.Error.OPERATION_FORBIDDEN, - 'This user is not allowed to access ' + 'non-existent class: ' + this.className - ); - } + if (hasClass !== true) { + const detailedError = + 'This user is not allowed to access non-existent class: ' + this.className; + const log = (this.config && this.config.loggerController) || defaultLogger; + throw createSanitizedError( + Parse.Error.OPERATION_FORBIDDEN, + detailedError, + log + ); + }Also applies to: 811-816
src/triggers.js (1)
4-6: Validator now distinguishes security vs generic validation errorsThe updated
maybeRunValidatorlogic:
- Correctly treats
"Master key is required"and"Please login to continue"failures as security-sensitive, logging the detailed reason viadefaultLoggerand returning a genericVALIDATION_ERRORwith message'Permission denied'.- Preserves the existing
'Validation failed.'behavior for non-security validation problems.If you want to further centralize sanitization, you could consider reusing the shared helper instead of hand-rolling:
- if (isSecurityError) { - // Log detailed error server-side and sanitize for client - defaultLogger.error('Security validation error:', errorMessage); - error = resolveError(e, { - code: Parse.Error.VALIDATION_ERROR, - message: 'Permission denied', - }); - } else { + if (isSecurityError) { + defaultLogger.error('Security validation error:', errorMessage); + error = resolveError(e, { + code: Parse.Error.VALIDATION_ERROR, + message: 'Permission denied', + }); + } else {(or swap in
createSanitizedErrorif you decide to standardize on that helper for all security errors.)Also applies to: 696-735
src/RestWrite.js (1)
20-22: RestWrite sanitization is sound; consider minor logger and read-only consistency tweaksThe new sanitized error paths in RestWrite are functionally correct:
validateClientClassCreationand the unauthenticated_Userupdate guard now log detailed reasons via(this.config && this.config.loggerController) || defaultLoggerand expose only a generic permission error to clients.checkRestrictedFieldscorrectly blocks client-side updates toemailVerifiedwithOPERATION_FORBIDDEN.A few small follow-ups would make this file more consistent with the rest of the PR:
- Use the config-aware logger in
checkRestrictedFieldsRight now it relies on
createSanitizedError’s internal default logger. To align with the other call sites:if (!this.auth.isMaintenance && !this.auth.isMaster && 'emailVerified' in this.data) { - throw createSanitizedError(Parse.Error.OPERATION_FORBIDDEN, 'Clients aren\'t allowed to manually update email verification.'); + const log = (this.config && this.config.loggerController) || defaultLogger; + throw createSanitizedError( + Parse.Error.OPERATION_FORBIDDEN, + "Clients aren't allowed to manually update email verification.", + log + ); }
- Deduplicate logger imports
You currently import the same default export twice:
import logger from './logger'; import defaultLogger from './logger';You can keep just one default import and reuse it (e.g. stick with
loggerand dropdefaultLogger, or vice versa), which avoids confusion and potential lint warnings.
- Optional: sanitize the read-only masterKey constructor guard
The constructor still throws a raw
Parse.ErrormentioningreadOnlyMasterKey. Given thatSharedRest.enforceRoleSecurityshould block external read-only writes first, this is mostly an internal edge case, but you may still want to route it throughcreateSanitizedErrorfor consistency with other read-only enforcement paths.Also applies to: 191-216, 662-669, 1456-1463
src/SharedRest.js (3)
1-15: Configurable logger integration and signature change look correct; consider minor DRY refactor
- Importing
createSanitizedError/defaultLoggerand extendingenforceRoleSecurityto acceptconfig = nullis backward compatible and aligns with the new sanitization helper.- To avoid repeating
config || defaultLoggerin each branch, consider computing it once at the top of the function, e.g.const loggerOrConfig = config || defaultLogger;, and reuse it below.
17-20: Sanitized errors across all forbidden paths align with the security objective; optional helper could reduce duplication
- Each forbidden-operation path now builds a clear
detailedError, selectsloggerOrConfig, and throwscreateSanitizedError(Parse.Error.OPERATION_FORBIDDEN, …, loggerOrConfig), which should log server-side and return a generic “Permission denied” to clients—this matches the goal of not leaking details while preserving the error code.- The three branches share the same pattern; you could optionally factor a tiny local helper (or reuse a precomputed
loggerOrConfig) to reduce duplication and keep future changes to the sanitization behavior in one place.Also applies to: 29-31, 36-38
1-44: PR-level: title suggestion and documentation note for this security behavior change
- Given this is a security-focused behavioral change that developers should notice in the changelog, consider renaming the PR to an Angular-style title such as:
feat(security): sanitize permission errors- It may also be worth adding a short note to the security/docs/README explaining that certain authorization errors are now always surfaced as generic “Permission denied” while details are only logged server-side, so integrators know not to rely on error message text.
Based on learnings
src/Routers/SchemasRouter.js (1)
8-9: Sanitized error utilities imported correctly; minor DRY opportunity and PR title suggestionImports for
createSanitizedErroranddefaultLoggerlook good and match the shared helper pattern. To avoid repeating the same logger expression in multiple handlers, you could optionally introduce a small helper (e.g.const log = req.config?.loggerController || defaultLogger;) or passreq.configdirectly intocreateSanitizedErrorand let it resolve the logger internally, for slightly cleaner code.Side note for the PR itself: an Angular-style title that would read well in the changelog could be:
fix(security): sanitize read-only schema error messages
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
spec/AudienceRouter.spec.js(5 hunks)spec/LogsRouter.spec.js(1 hunks)spec/ParseAPI.spec.js(1 hunks)spec/ParseFile.spec.js(3 hunks)spec/ParseGlobalConfig.spec.js(1 hunks)spec/ParseGraphQLServer.spec.js(7 hunks)spec/ParseInstallation.spec.js(1 hunks)spec/ParseQuery.Aggregate.spec.js(1 hunks)spec/ParseUser.spec.js(3 hunks)spec/RestQuery.spec.js(2 hunks)spec/Schema.spec.js(5 hunks)spec/features.spec.js(1 hunks)spec/rest.spec.js(11 hunks)spec/schemas.spec.js(10 hunks)spec/vulnerabilities.spec.js(1 hunks)src/Controllers/SchemaController.js(4 hunks)src/GraphQL/loaders/schemaMutations.js(4 hunks)src/GraphQL/loaders/schemaQueries.js(2 hunks)src/GraphQL/parseGraphQLUtils.js(1 hunks)src/RestQuery.js(5 hunks)src/RestWrite.js(4 hunks)src/Routers/ClassesRouter.js(2 hunks)src/Routers/FilesRouter.js(2 hunks)src/Routers/GlobalConfigRouter.js(2 hunks)src/Routers/GraphQLRouter.js(2 hunks)src/Routers/PurgeRouter.js(1 hunks)src/Routers/PushRouter.js(2 hunks)src/Routers/SchemasRouter.js(4 hunks)src/Routers/UsersRouter.js(2 hunks)src/SecurityError.js(1 hunks)src/SharedRest.js(3 hunks)src/middlewares.js(2 hunks)src/rest.js(6 hunks)src/triggers.js(2 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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: 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: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-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/ParseInstallation.spec.jsspec/Schema.spec.jsspec/schemas.spec.jsspec/rest.spec.jsspec/ParseUser.spec.jsspec/vulnerabilities.spec.jsspec/RestQuery.spec.jsspec/ParseGraphQLServer.spec.jssrc/triggers.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/Schema.spec.jsspec/schemas.spec.jsspec/rest.spec.jsspec/ParseUser.spec.jsspec/vulnerabilities.spec.jsspec/RestQuery.spec.jsspec/ParseGraphQLServer.spec.jssrc/triggers.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/Schema.spec.jsspec/rest.spec.jsspec/ParseUser.spec.jsspec/vulnerabilities.spec.jsspec/RestQuery.spec.jsspec/ParseGraphQLServer.spec.jssrc/triggers.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: 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/SharedRest.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/loaders/schemaMutations.jssrc/Routers/GraphQLRouter.jssrc/GraphQL/parseGraphQLUtils.jsspec/ParseGraphQLServer.spec.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/rest.spec.jssrc/Routers/ClassesRouter.jsspec/RestQuery.spec.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:
spec/rest.spec.jssrc/RestQuery.js
📚 Learning: 2025-08-26T14:06:31.853Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: spec/CloudCode.spec.js:446-469
Timestamp: 2025-08-26T14:06:31.853Z
Learning: In the Parse Server codebase, when handling query objects in maybeRunAfterFindTrigger, objects without a where property that contain options like limit/skip should be treated as query JSON with an empty where clause using the spread pattern { where: {}, ...query }, not nested as { where: query }.
Applied to files:
src/RestQuery.js
📚 Learning: 2025-11-17T15:02:48.786Z
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.
Applied to files:
spec/ParseGraphQLServer.spec.js
🧬 Code graph analysis (24)
src/SharedRest.js (3)
src/RestQuery.js (6)
require(7-7)require(9-9)require(10-10)defaultLogger(11-11)detailedError(125-125)detailedError(811-811)src/rest.js (4)
require(15-15)require(329-329)defaultLogger(330-330)detailedError(331-331)src/SecurityError.js (1)
createSanitizedError(13-28)
spec/schemas.spec.js (1)
src/Routers/SchemasRouter.js (2)
response(51-56)response(64-70)
src/GraphQL/loaders/schemaQueries.js (1)
src/GraphQL/parseGraphQLUtils.js (1)
enforceMasterKeyAccess(6-15)
src/GraphQL/loaders/schemaMutations.js (4)
src/GraphQL/parseGraphQLUtils.js (2)
enforceMasterKeyAccess(6-15)loggerOrConfig(8-8)src/middlewares.js (4)
enforceMasterKeyAccess(503-512)config(208-208)config(644-644)config(646-646)src/SharedRest.js (5)
loggerOrConfig(18-18)loggerOrConfig(30-30)loggerOrConfig(37-37)defaultLogger(11-11)Parse(1-1)src/SecurityError.js (1)
createSanitizedError(13-28)
src/Routers/GraphQLRouter.js (2)
src/RestQuery.js (3)
log(126-126)log(812-812)defaultLogger(11-11)src/SecurityError.js (3)
log(14-14)log(40-40)createSanitizedError(13-28)
src/SecurityError.js (6)
src/SharedRest.js (4)
loggerOrConfig(18-18)loggerOrConfig(30-30)loggerOrConfig(37-37)defaultLogger(11-11)src/RestQuery.js (3)
log(126-126)log(812-812)defaultLogger(11-11)src/RestWrite.js (2)
log(205-205)log(1458-1458)src/Routers/SchemasRouter.js (3)
log(77-77)log(101-101)log(118-118)src/middlewares.js (8)
log(237-237)log(247-247)log(464-464)log(505-505)log(516-516)log(539-539)error(252-252)error(506-506)src/rest.js (2)
log(332-332)defaultLogger(330-330)
src/Routers/FilesRouter.js (3)
src/RestQuery.js (3)
log(126-126)log(812-812)defaultLogger(11-11)src/SharedRest.js (2)
defaultLogger(11-11)Parse(1-1)src/SecurityError.js (3)
log(14-14)log(40-40)createSanitizedError(13-28)
src/middlewares.js (1)
src/SecurityError.js (4)
log(14-14)log(40-40)error(52-52)createSanitizedHttpError(39-56)
src/Routers/ClassesRouter.js (1)
src/SecurityError.js (2)
log(14-14)createSanitizedError(13-28)
spec/features.spec.js (1)
src/middlewares.js (2)
error(252-252)error(506-506)
src/RestWrite.js (5)
src/RestQuery.js (5)
detailedError(125-125)detailedError(811-811)log(126-126)log(812-812)defaultLogger(11-11)src/SharedRest.js (5)
detailedError(17-17)detailedError(29-29)detailedError(36-36)defaultLogger(11-11)Parse(1-1)src/rest.js (3)
detailedError(331-331)log(332-332)defaultLogger(330-330)src/SecurityError.js (3)
log(14-14)log(40-40)createSanitizedError(13-28)src/middlewares.js (6)
log(237-237)log(247-247)log(464-464)log(505-505)log(516-516)log(539-539)
src/Controllers/SchemaController.js (1)
src/SecurityError.js (1)
createSanitizedError(13-28)
src/GraphQL/parseGraphQLUtils.js (3)
src/middlewares.js (4)
enforceMasterKeyAccess(503-512)config(208-208)config(644-644)config(646-646)src/SharedRest.js (5)
loggerOrConfig(18-18)loggerOrConfig(30-30)loggerOrConfig(37-37)defaultLogger(11-11)Parse(1-1)src/SecurityError.js (1)
createSanitizedError(13-28)
src/RestQuery.js (4)
src/SharedRest.js (6)
require(10-10)defaultLogger(11-11)detailedError(17-17)detailedError(29-29)detailedError(36-36)Parse(1-1)src/rest.js (5)
require(15-15)require(329-329)defaultLogger(330-330)detailedError(331-331)log(332-332)src/middlewares.js (9)
config(208-208)config(644-644)config(646-646)log(237-237)log(247-247)log(464-464)log(505-505)log(516-516)log(539-539)src/SecurityError.js (3)
log(14-14)log(40-40)createSanitizedError(13-28)
src/Routers/UsersRouter.js (7)
src/RestQuery.js (3)
log(126-126)log(812-812)defaultLogger(11-11)src/RestWrite.js (2)
log(205-205)log(1458-1458)src/Routers/SchemasRouter.js (3)
log(77-77)log(101-101)log(118-118)src/SecurityError.js (2)
log(14-14)createSanitizedError(13-28)src/middlewares.js (6)
log(237-237)log(247-247)log(464-464)log(505-505)log(516-516)log(539-539)src/rest.js (2)
log(332-332)defaultLogger(330-330)src/SharedRest.js (2)
defaultLogger(11-11)Parse(1-1)
spec/AudienceRouter.spec.js (1)
src/middlewares.js (2)
error(252-252)error(506-506)
spec/ParseAPI.spec.js (1)
src/Routers/SchemasRouter.js (2)
response(51-56)response(64-70)
src/rest.js (3)
src/RestQuery.js (9)
className(1001-1001)require(7-7)require(9-9)require(10-10)defaultLogger(11-11)detailedError(125-125)detailedError(811-811)log(126-126)log(812-812)src/SecurityError.js (4)
error(52-52)log(14-14)log(40-40)createSanitizedError(13-28)src/SharedRest.js (6)
Parse(1-1)require(10-10)defaultLogger(11-11)detailedError(17-17)detailedError(29-29)detailedError(36-36)
src/Routers/PurgeRouter.js (1)
src/SecurityError.js (3)
log(14-14)log(40-40)createSanitizedError(13-28)
src/Routers/GlobalConfigRouter.js (2)
src/Routers/SchemasRouter.js (3)
log(77-77)log(101-101)log(118-118)src/SecurityError.js (3)
log(14-14)log(40-40)createSanitizedError(13-28)
src/triggers.js (5)
src/Auth.js (2)
e(582-585)Parse(1-1)src/SecurityError.js (1)
error(52-52)src/middlewares.js (2)
error(252-252)error(506-506)src/RestQuery.js (1)
defaultLogger(11-11)src/SharedRest.js (2)
defaultLogger(11-11)Parse(1-1)
spec/ParseQuery.Aggregate.spec.js (1)
src/middlewares.js (2)
error(252-252)error(506-506)
src/Routers/SchemasRouter.js (4)
src/RestQuery.js (3)
log(126-126)log(812-812)defaultLogger(11-11)src/SecurityError.js (3)
log(14-14)log(40-40)createSanitizedError(13-28)src/middlewares.js (6)
log(237-237)log(247-247)log(464-464)log(505-505)log(516-516)log(539-539)src/rest.js (2)
log(332-332)defaultLogger(330-330)
src/Routers/PushRouter.js (2)
src/RestQuery.js (3)
log(126-126)log(812-812)defaultLogger(11-11)src/SecurityError.js (3)
log(14-14)log(40-40)createSanitizedError(13-28)
⏰ 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: Node 18
- GitHub Check: Node 20
- GitHub Check: Node 22
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Redis Cache
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: Code Analysis (javascript)
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (24)
spec/LogsRouter.spec.js (1)
64-64: LGTM! Test expectation correctly updated.The error message expectation has been updated to align with the sanitized error handling. This test correctly verifies that unauthorized requests without a master key now receive the generic "Permission denied" message.
spec/ParseGlobalConfig.spec.js (1)
236-236: LGTM! Consistent with sanitized error handling.The test correctly expects "Permission denied" when attempting to update global config without the master key.
spec/vulnerabilities.spec.js (1)
18-18: LGTM! Error message appropriately generalized for security vulnerability.The test for GHSA-8xq9-g7ch-35hg (custom object ID privilege escalation vulnerability) now correctly expects "Permission denied" instead of "Invalid object ID." This generic message prevents leaking implementation details about the security check while maintaining the OPERATION_FORBIDDEN error code.
spec/features.spec.js (1)
35-35: LGTM! Test correctly updated for serverInfo endpoint.The test appropriately expects "Permission denied" when requesting serverInfo without the master key.
spec/ParseQuery.Aggregate.spec.js (1)
80-80: LGTM! Aggregate query error message correctly sanitized.The test correctly expects "Permission denied" for aggregate queries without master key authentication.
spec/ParseAPI.spec.js (1)
1727-1727: LGTM! Purge operation error correctly updated.The test appropriately expects "Permission denied" when attempting to purge objects without the master key.
spec/Schema.spec.js (1)
286-286: LGTM! All schema permission tests correctly updated.All test expectations have been consistently updated to "Permission denied" for:
- Count operations without permission (line 286)
- Required auth scenarios for find, create, get, and find operations (lines 1465, 1564, 1652, 1697)
These changes align with the sanitized error handling across permission-denied scenarios.
Also applies to: 1465-1465, 1564-1564, 1652-1652, 1697-1697
src/Routers/PurgeRouter.js (1)
4-5: Sanitized error for read‑only master key purge looks correctUsing
createSanitizedErrorwith(req.config && req.config.loggerController) || defaultLoggermatches the new security pattern and preserves the original error code while hiding details from clients.Also applies to: 10-15
src/Routers/UsersRouter.js (1)
19-21: Sanitized error for/loginAswithout master key is consistent
handleLogInAsnow routes the missing-master-key case throughcreateSanitizedErrorwith(req.config && req.config.loggerController) || defaultLogger, which matches the new security model and keeps the error code stable while generalizing the client‑facing message.Also applies to: 336-344
src/Routers/GraphQLRouter.js (1)
4-5: GraphQL config updates correctly use sanitized error for read‑only master keyFor read‑only master keys,
updateGraphQLConfignow throws acreateSanitizedErrorwithParse.Error.OPERATION_FORBIDDENand a logger derived fromreq.config.loggerControllerordefaultLogger, which aligns with the new permission‑denied semantics.Also applies to: 17-24
spec/rest.spec.js (1)
316-331: Tests correctly updated to expect sanitized'Permission denied'messagesThe affected tests still assert the correct Parse error codes (e.g.
OPERATION_FORBIDDEN) but now expect the generic'Permission denied'message for:
- Client‑class‑creation being disabled.
- Pointer reads into master‑key‑only classes (
_PushStatus,_GlobalConfig).- All
read-only masterKeywrite operations (REST, schemas, config, push).This matches the new sanitized error behavior while keeping codes stable for clients.
Also applies to: 772-786, 788-802, 945-989, 1024-1138
spec/RestQuery.spec.js (1)
157-172: RestQuery tests align with sanitized forbidden errorsBoth the disabled‑client‑class and protected‑field tests now assert the same error codes as before but with a generic
'Permission denied'message, which is consistent with the new sanitization logic.Also applies to: 216-248
src/Routers/ClassesRouter.js (1)
6-8: Sanitized error for invalid_UserobjectId (role:prefix) is appropriate
handleCreatenow rejects_Userobjects whoseobjectIdstarts with'role:'usingcreateSanitizedErrorand(req.config && req.config.loggerController) || defaultLogger, preserving the forbidden semantics while hiding the reason from clients.Also applies to: 110-119
spec/ParseUser.spec.js (1)
2634-2665: User/session tests correctly updated for generic permission errorsThe affected tests now:
- Keep their original error codes (e.g.
209for invalid/missing session token,OPERATION_FORBIDDENfor forbidden updates and/loginAswithout master key), and- Expect
'Permission denied'instead of more specific messages like'Session token required.',"Clients aren't allowed to manually update email verification.", or'master key is required'.This is consistent with the new sanitized error strategy and encourages clients to key off error codes rather than message text.
Also applies to: 3351-3384, 4374-4397
spec/ParseFile.spec.js (2)
159-159: LGTM! Security improvement correctly tested.The test expectations now correctly validate that generic "Permission denied" messages are returned for both missing and incorrect master key scenarios, preventing information disclosure about authentication requirements.
Also applies to: 172-172
763-763: LGTM! Error code preserved while message sanitized.The test correctly validates that error code 119 is maintained while the message is generalized from "Invalid application ID." to "Permission denied", preventing information disclosure about application IDs while preserving API contract.
spec/ParseGraphQLServer.spec.js (2)
3490-3505: Sanitized forbidden-error expectations are consistent with the new security modelAll updated assertions that now expect
Parse.Error.OPERATION_FORBIDDENwith the message'Permission denied'for class schema operations (create, update, delete, get, list) align with the PR goal of hiding authorization details while keeping codes stable. The tests remain precise and should fail if any endpoint accidentally reintroduces a more verbose error.Also applies to: 3841-3875, 4066-4100, 4126-4142, 4144-4160
6058-6125: Updated permission checks for object creation correctly target sanitized messagesThe
createObjectpermission tests now reject withjasmine.stringMatching('Permission denied')instead of a more detailed unauthorized message, which matches the sanitized error behavior while remaining resilient to small wording changes around that phrase. Looks good.src/GraphQL/parseGraphQLUtils.js (1)
3-13: Config-aware sanitized master-key enforcement looks correctUsing
createSanitizedErrorwithloggerOrConfig = config || defaultLoggerensures clients only see a generic permission error while detailed messages are logged via the app logger when available. The updatedenforceMasterKeyAccess(auth, config)signature is consistent with this pattern.src/RestQuery.js (1)
10-11: RestQuery master/session security sanitization is consistent
- Passing
configintoenforceRoleSecurityand usingcreateSanitizedErrorfor invalid session tokens ensures_Sessionaccess errors are logged with full detail while clients see a generic permission failure.- Use of
(this.config && this.config.loggerController) || defaultLoggerfor the session-token path matches the broader pattern introduced in this PR.Also applies to: 56-56, 122-132
src/Controllers/SchemaController.js (1)
23-24: CLP permission-denial sanitization preserves codes while hiding detailThe new
createSanitizedErrorusage inSchemaController.validatePermissioncorrectly:
- Keeps
OBJECT_NOT_FOUNDfor unauthenticated access andOPERATION_FORBIDDENfor CLP denials.- Logs detailed reasons via
defaultLoggerwhile only exposing a generic permission error to clients.This is consistent with the PR’s security objective and should not break code-based error handling.
Also applies to: 1408-1413, 1426-1428, 1447-1448
src/Routers/SchemasRouter.js (3)
76-82: Read‑only create‑schema error is now properly sanitized and loggedThe switch to
createSanitizedErrorpreserves theOPERATION_FORBIDDENcode, logs the detailed read‑only master key message via the resolved logger, and returns the generic client message. This matches the intended security behavior.
100-106: Consistent sanitized handling for schema updatesThe update path mirrors the create handler: same code, same detailed message confined to logs, and sanitized client error. The consistency across schema mutations is good.
118-123: Delete‑schema read‑only handling aligned with other mutationsThe delete handler now follows the same sanitized error pattern as create/update while keeping the forbidden error code. This keeps behavior uniform across all schema‑modifying operations.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## alpha #9937 +/- ##
==========================================
+ Coverage 93.06% 93.08% +0.02%
==========================================
Files 187 188 +1
Lines 15253 15303 +50
Branches 177 177
==========================================
+ Hits 14195 14245 +50
Misses 1046 1046
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: 1
🧹 Nitpick comments (4)
src/SecurityError.js (3)
12-21: Make logger resolution more defensive and DRY.Both helpers duplicate the same logger resolution logic and treat any truthy
loggerOrConfig.erroras a logger, even if it’s not a function. That’s fine for the expectedconfig/ logger inputs, but it’s brittle for accidental misuse.Consider extracting a small
resolveLogger(loggerOrConfig)helper that:
- Returns
loggerOrConfig.loggerControllerwhen present.- Otherwise returns
loggerOrConfigonly iftypeof loggerOrConfig.error === 'function'.- Falls back to
defaultLoggerin all other cases.That removes duplication and avoids ever ending up with an object that doesn’t actually implement
error().Also applies to: 38-46
23-27: Optional: enrich server‑side logs with error code / status as metadata.Right now the log entry only includes the string message; the returned error carries the code/status, but that information is not in the log call itself. For security auditing and debugging it can be useful to log the numeric
errorCode/statusCodeas well (e.g. as an additional argument or metadata object), while still keeping the client message sanitized.This is not required for correctness, but worth considering for more informative security logs across the codebase.
Also applies to: 48-54
1-55: PR‑level: align title with Angular convention and consider documenting the new error behavior.Per the repo’s conventions, you may want to adjust the PR title to an Angular‑style variant that’s clearer in the changelog, for example:
feat(security): sanitize permission error responsesAlso, since this changes client‑visible error messages (even though codes stay the same), consider a short note in the README or upgrade notes explaining that certain security/permission errors now always return
"Permission denied"while detailed reasons are only available in server logs.src/SharedRest.js (1)
9-10: Access checks look correct; consider centralizing logger resolution.The refactor keeps the existing access rules intact while routing all denials through
createSanitizedError, which is exactly what we want for hiding detailed messages from clients and logging them server‑side. The newconfigparameter is optional, so existing callers remain valid and fall back todefaultLogger.You could optionally compute
const loggerOrConfig = config || defaultLogger;once near the top ofenforceRoleSecurityand reuse it in all three branches to avoid repetition, but that’s purely cosmetic and not required.Also applies to: 13-19, 28-31, 35-37
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SecurityError.js(1 hunks)src/SharedRest.js(2 hunks)src/triggers.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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: 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: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.
🧬 Code graph analysis (2)
src/SharedRest.js (3)
src/RestQuery.js (6)
require(7-7)require(9-9)require(10-10)defaultLogger(11-11)detailedError(125-125)detailedError(811-811)src/rest.js (4)
require(15-15)require(329-329)defaultLogger(330-330)detailedError(331-331)src/SecurityError.js (1)
createSanitizedError(12-27)
src/SecurityError.js (4)
src/SharedRest.js (1)
defaultLogger(10-10)src/RestQuery.js (3)
log(126-126)log(812-812)defaultLogger(11-11)src/rest.js (2)
log(332-332)defaultLogger(330-330)src/middlewares.js (8)
log(237-237)log(247-247)log(464-464)log(505-505)log(516-516)log(539-539)error(252-252)error(506-506)
🪛 GitHub Check: Lint
src/triggers.js
[failure] 1112-1112:
Newline required at end of file but not found
⏰ 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). (15)
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Redis Cache
- GitHub Check: Node 22
- GitHub Check: Node 20
- GitHub Check: Node 18
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (1)
src/triggers.js (1)
1-1112: Verify error sanitization scope in this file.The PR objective is to sanitize error messages for security. This file contains several error handling paths (e.g.,
resolveErrorat lines 669–693, validation error throws at lines 738–741, etc.) that could expose sensitive details. Per the AI summary, this file has "no observable functional changes" aside from the EOF newline.Please confirm whether error sanitization changes were intentionally omitted from
src/triggers.js, or if related changes are in other files within this PR.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/rest.js (1)
171-173: Sanitize the early _User delete guard to align with the rest of the file.Line 172 still throws a raw
Parse.Error(Parse.Error.SESSION_MISSING, 'Insufficient auth to delete user'), exposing operation detail. This contradicts the sanitization applied tohandleSessionMissingErrorat lines 329-332.Apply this diff to sanitize the error:
if (className === '_User' && auth.isUnauthenticated()) { - throw new Parse.Error(Parse.Error.SESSION_MISSING, 'Insufficient auth to delete user'); + const { createSanitizedError } = require('./SecurityError'); + const detailedError = 'Insufficient auth to delete user'; + throw createSanitizedError(Parse.Error.SESSION_MISSING, detailedError); }Based on learnings
🧹 Nitpick comments (8)
src/GraphQL/parseGraphQLUtils.js (2)
5-12: Master-key enforcement now goes throughcreateSanitizedError; verify client message is actually sanitized.Routing this
OPERATION_FORBIDDENthroughcreateSanitizedErroris the right direction for centralizing and sanitizing permission errors, while preserving the error code. One thing to double‑check against #8958: ifcreateSanitizedErrorforwards itsmessageargument to the client unchanged, this would still expose"unauthorized: master key is required". In that case, either:
- have
createSanitizedErrorsanitize to a generic message (e.g."unauthorized"), or- pass a pre‑sanitized message here and log the detailed context inside
createSanitizedError.It would be good to have a GraphQL test asserting that the client no longer sees
"master key is required"in this path.
1-57: PR meta: Angular-style title suggestion and docs note.For the changelog, an Angular-style title that reflects this behavior change could be:
feat(security): sanitize permission errors for GraphQLAlso, since this subtly changes what clients see on authorization failures, consider a brief note in the README or security/operations docs (e.g., “detailed permission error messages are now logged server-side and hidden from clients”), even if no new option is added.
src/GraphQL/loaders/schemaMutations.js (1)
34-41: Read-only masterKey error is now sanitized in CreateClass.The implementation correctly uses
createSanitizedErrorto sanitize the error message. However, unlike similar changes in other files (e.g., src/RestWrite.js lines 204-210), this doesn't pass aconfig-aware logger. The error will still be sanitized, but detailed logging may use a default logger instead of the config's logger.If you want consistent logging with the config's logger, consider:
const { config, auth } = context; enforceMasterKeyAccess(auth, config); if (auth.isReadOnly) { + const log = (config && config.loggerController) || defaultLogger; throw createSanitizedError( Parse.Error.OPERATION_FORBIDDEN, "read-only masterKey isn't allowed to create a schema.", + log ); }(Same pattern applies to UpdateClass at lines 86-89 and DeleteClass at lines 137-140.)
src/Controllers/SchemaController.js (1)
1408-1413: Consider simplifying the error construction pattern.Per past review feedback, these could be more compact without the intermediate
detailedErrorvariable:if (!aclGroup || aclGroup.length == 0) { - const detailedError = 'Permission denied, user needs to be authenticated.'; - throw createSanitizedError(Parse.Error.OBJECT_NOT_FOUND, detailedError); + throw createSanitizedError( + Parse.Error.OBJECT_NOT_FOUND, + 'Permission denied, user needs to be authenticated.' + ); } else if (aclGroup.indexOf('*') > -1 && aclGroup.length == 1) { - const detailedError = 'Permission denied, user needs to be authenticated.'; - throw createSanitizedError(Parse.Error.OBJECT_NOT_FOUND, detailedError); + throw createSanitizedError( + Parse.Error.OBJECT_NOT_FOUND, + 'Permission denied, user needs to be authenticated.' + ); }This same pattern applies to lines 1426-1427 and 1447-1448 as well.
Based on learnings
src/RestWrite.js (2)
667-669: Good: compact error construction pattern.The single-line
createSanitizedErrorcall is concise and readable, though it doesn't pass a config-aware logger like the implementation at lines 204-210.For consistency with line 205, consider passing a config-aware logger:
- if (!this.auth.isMaintenance && !this.auth.isMaster && 'emailVerified' in this.data) { - throw createSanitizedError(Parse.Error.OPERATION_FORBIDDEN, 'Clients aren\'t allowed to manually update email verification.'); + if (!this.auth.isMaintenance && !this.auth.isMaster && 'emailVerified' in this.data) { + const log = (this.config && this.config.loggerController) || defaultLogger; + throw createSanitizedError( + Parse.Error.OPERATION_FORBIDDEN, + 'Clients aren\'t allowed to manually update email verification.', + log + ); }
1456-1462: User modification error is sanitized, but logger parameter could be added.The implementation correctly uses
createSanitizedError, but unlike the pattern at lines 204-210, it doesn't pass a config-aware logger.For consistency, consider:
if (this.className === '_User' && this.query && this.auth.isUnauthenticated()) { const detailedError = `Cannot modify user ${this.query.objectId}.`; + const log = (this.config && this.config.loggerController) || defaultLogger; throw createSanitizedError( Parse.Error.SESSION_MISSING, detailedError, + log ); }src/Routers/PushRouter.js (2)
13-16: Sanitized error use looks correct; optionally align logging and testsUsing
createSanitizedErrorwithParse.Error.OPERATION_FORBIDDENhere keeps the error code stable while delegating message sanitization, which matches the objective of hiding detailed authorization reasons from clients.Two optional follow-ups:
- If
createSanitizedErrorsupports receiving a logger or request context, consider passing inreq.config.loggerController(or a derived logger) so read-only masterKey misuse on/pushis logged consistently with other sanitized errors in this PR.- Ensure there is a test that exercises this read-only
masterKeypath and asserts the generic client-facing message while still preserving the correct error code.
1-85: PR title & documentation suggestionTo make the changelog clearer and follow Angular-style conventions, consider renaming the PR to something like:
feat(security): sanitize unauthorized error messagesAlso consider a brief note in the changelog (or README, if you think it affects integrators) that certain security-related errors now return generic client messages while logging detailed diagnostics server-side, so users understand why previously detailed messages have become more generic.
Based on learnings
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/Controllers/SchemaController.js(4 hunks)src/GraphQL/loaders/schemaMutations.js(4 hunks)src/GraphQL/parseGraphQLUtils.js(1 hunks)src/RestQuery.js(5 hunks)src/RestWrite.js(4 hunks)src/Routers/ClassesRouter.js(2 hunks)src/Routers/FilesRouter.js(2 hunks)src/Routers/GlobalConfigRouter.js(2 hunks)src/Routers/GraphQLRouter.js(2 hunks)src/Routers/PurgeRouter.js(1 hunks)src/Routers/PushRouter.js(2 hunks)src/Routers/SchemasRouter.js(4 hunks)src/Routers/UsersRouter.js(2 hunks)src/SecurityError.js(1 hunks)src/SharedRest.js(2 hunks)src/middlewares.js(2 hunks)src/rest.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/Routers/SchemasRouter.js
- src/Routers/GraphQLRouter.js
- src/Routers/ClassesRouter.js
- src/Routers/UsersRouter.js
- src/Routers/FilesRouter.js
- src/RestQuery.js
- src/Routers/PurgeRouter.js
- src/SecurityError.js
- src/Routers/GlobalConfigRouter.js
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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: 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: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-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/loaders/schemaMutations.jssrc/GraphQL/parseGraphQLUtils.js
🔇 Additional comments (6)
src/GraphQL/parseGraphQLUtils.js (1)
3-3: Import ofcreateSanitizedErrorlooks correct and aligns with new security helper.The import cleanly plugs this module into the centralized security error handling without changing the public API.
src/rest.js (1)
329-332: LGTM! Session missing error is now sanitized.The implementation correctly uses
createSanitizedErrorto sanitize the error message while preserving the error code and detailed message for server-side logging.src/middlewares.js (1)
503-518: LGTM! Master key enforcement is now sanitized.Both
enforceMasterKeyAccessandpromiseEnforceMasterKeyAccesscorrectly usecreateSanitizedHttpErrorto sanitize the error response while preserving the detailed message for server-side logging.src/SharedRest.js (1)
12-35: LGTM! Role security errors are now sanitized.All three error paths in
enforceRoleSecuritycorrectly usecreateSanitizedErrorto sanitize error messages while preserving detailed error information for server-side logging. The implementation is consistent and maintains the original control flow.src/RestWrite.js (1)
204-210: LGTM! Client class creation error is sanitized with config-aware logging.The implementation correctly uses
createSanitizedErrorwith a config-aware logger, ensuring detailed errors are logged to the configured logger while sending sanitized messages to clients.src/Routers/PushRouter.js (1)
4-4: createSanitizedError import integrates PushRouter into the sanitization layerImporting
createSanitizedErrorhere is appropriate and keeps push routing aligned with the centralized error-sanitization helper; no issues from a dependency or routing perspective.
|
@coratgerl you can treat coderabbit review comments like human review comments; if you followed it, coderabbit will close its comment after checking your next commit. you should only resolve it manually after providing your arguments. if in doubt you can engage in a discussion. That makes it easier for everyone to know what your position on the feedback is. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/schemas.spec.js (1)
2513-2605: Fix mismatched action name in CLP 5 sanitized log expectationIn
validate CLP 5, the first forbidden query is afind()call, but the sanitized log expectation usesaction create:sanitizedErrorCall.checkMessage( 'Permission denied for action create on class AClass', callCountBefore );Given this branch is exercising a
findoperation, the detailed message should stay aligned with the other CLP tests and the underlying enforcement.- sanitizedErrorCall.checkMessage('Permission denied for action create on class AClass', callCountBefore); + sanitizedErrorCall.checkMessage('Permission denied for action find on class AClass', callCountBefore);Without this, the test is likely to fail or become misleading if the implementation correctly reports
findas the action.
♻️ Duplicate comments (2)
src/RestWrite.js (1)
1458-1462: Verify logger usage consistency (same concern as line 666).Similar to the comment at line 666, this
createSanitizedErrorcall doesn't pass a logger parameter, while the call at line 205 does. Ensure this is intentional and that the detailed error message (includingthis.query.objectId) is properly logged server-side.src/rest.js (1)
171-176: Unauthenticated_Userdelete still throws raw error, bypassing sanitization helperThe early guard:
if (className === '_User' && auth.isUnauthenticated()) { throw new Parse.Error(Parse.Error.SESSION_MISSING, 'Insufficient auth to delete user'); }still sends a detailed message directly to the client instead of going through
createSanitizedError, which is inconsistent with the new sanitized session/auth handling and the behavior inhandleSessionMissingError.To fully align delete‑user with the sanitization strategy, route this branch through
createSanitizedErroras well:if (className === '_User' && auth.isUnauthenticated()) { - throw new Parse.Error(Parse.Error.SESSION_MISSING, 'Insufficient auth to delete user'); + const { createSanitizedError } = require('./Error'); + throw createSanitizedError( + Parse.Error.SESSION_MISSING, + 'Insufficient auth to delete user' + ); }(Alternatively, you could delegate to
handleSessionMissingErrorif you prefer a single central helper.)This avoids leaking operation details to clients while keeping richer information in the server logs.
🧹 Nitpick comments (10)
src/TestUtils.js (1)
85-104: Make logger spy resilient to repeated usage and tightly-coupled prefixThe helper works for current tests, but it assumes
spyOn(logger, 'error')is only ever called once per spec and that tests always run withprocess.env.TESTINGset so the first log arg is exactly'Sanitized error:'. If a spec callsgetSanitizedErrorCall()twice on the same logger, Jasmine may throw for double-spying unlessallowRespy(true)is configured, and any future change to the prefix would silently break all these tests.To make this more robust, consider:
- Reusing an existing spy if one already exists instead of always calling
spyOn.- Centralizing the
'Sanitized error:'prefix (e.g. exported constant fromsrc/Error.js) so tests and implementation cannot drift.spec/AudienceRouter.spec.js (1)
5-5: AudienceRouter master-key tests comprehensively validate sanitized errorsAll push_audiences authorization tests now assert a generic
'Permission denied'on the client and usegetSanitizedErrorCallto confirm the detailed master-key message was logged, covering POST/GET/PUT/DELETE consistently. This is a solid, end‑to‑end check of the new sanitization behavior.Also applies to: 266-339
spec/ParseFile.spec.js (1)
766-776: Tidy TestUtils import and confirm unauthorized file-path behaviorTwo small points here:
Inside
"does not crash on file request with invalid app ID", you re-require('../lib/TestUtils')and destructuregetSanitizedErrorCall, even though it is already imported at the top of the file. You can use the existing import and drop the inlinerequireto avoid redundancy.For invalid file paths, the test still expects
{ error: 'unauthorized' }instead of the generic'Permission denied'used for invalid application IDs. If the goal is that all unauthorized file routes share the same sanitized client message, it may be worth aligning this case as well; otherwise, clarifying in a comment or issue which cases are deliberately left as'unauthorized'could prevent confusion later.Also applies to: 782-788
src/Error.js (1)
1-44: Centralized sanitization helpers look good; consider minor hardening and docs/changelogThe new helpers encapsulate the pattern of:
- Logging the detailed message via
defaultLogger.error(...), with a'Sanitized error:'prefix underTESTINGso specs can detect it.- Returning a
Parse.Erroror plainErrorthat always exposes the generic'Permission denied'string while preserving the original error or HTTP status code.A few small follow‑ups to consider:
- Avoid duplicate logging logic: Extract a small internal helper (e.g.
logSanitizedError(detailedMessage)) shared by both functions to keep the logging behavior in one place and reduce drift risk if the format ever changes.- Be explicit about Parse dependency: If feasible within this codebase’s conventions, importing
Parse(rather than relying on a global) would makecreateSanitizedErrormore self‑contained and tooling‑friendly.- Lock in the test contract: The tests rely on the
'Sanitized error:'prefix only being used inTESTING. If any other code path ever starts logging with that same prefix, the TestUtils helper may get false positives; conversely, changing the prefix will break many specs. It may be worth documenting this contract in a short comment near the export or in CONTRIBUTING.Given this is a security‑relevant behavior change visible to clients, please also:
Add or update a short note in the appropriate docs/README or migration/changelog area to mention that certain authorization failures now return a generic
"Permission denied"message while keeping error codes unchanged.Update the PR title to an Angular-style form suitable for the changelog, for example:
feat(security): sanitize unauthorized error messagessrc/RestWrite.js (1)
204-209: Consider using optional chaining for consistency.The logger retrieval works correctly, but for consistency with the patterns in
src/middlewares.js(lines 236, 246, 463, 536), consider using optional chaining:Apply this diff:
- const log = (this.config && this.config.loggerController) || defaultLogger; + const log = this.config?.loggerController || defaultLogger;spec/schemas.spec.js (2)
7-8: Avoid double requiring../lib/TestUtilsYou now import
../lib/TestUtilstwice (asTestUtilsand via destructuring). To keep things consistent with other specs (e.g.spec/ParseAPI.spec.js) and avoid redundancy, consider a single destructured import:-const TestUtils = require('../lib/TestUtils'); -const { getSanitizedErrorCall } = require('../lib/TestUtils'); +const { destroyAllDataPermanently, getSanitizedErrorCall } = require('../lib/TestUtils');and then call
destroyAllDataPermanentlydirectly where needed.
170-190: Strengthen schema auth tests by asserting error code as wellThese two tests now correctly assert the sanitized message
'Permission denied', but they still rely only on status + message. To better pin down the behavior of the sanitized error, also assert the error code (e.g.Parse.Error.OPERATION_FORBIDDEN) in both tests.}).then(fail, response => { expect(response.status).toEqual(403); - expect(response.data.error).toEqual('Permission denied'); + expect(response.data.code).toEqual(Parse.Error.OPERATION_FORBIDDEN); + expect(response.data.error).toEqual('Permission denied');Apply the same pattern to the following test that covers
/schemaswith REST key.spec/Schema.spec.js (1)
1449-1727: Consider asserting error codes in required-auth tests as wellThe required-auth tests (
find,create,get,find not authenticated) now:
- Assert the generic
'Permission denied'message.- Use
getSanitizedErrorCallto verify the detailed log message'Permission denied, user needs to be authenticated.'.To further harden these tests against regressions, you may want to also assert the error code (e.g.,
Parse.Error.OPERATION_FORBIDDEN) alongside the message in each of these cases, similar to the CLP tests above.- e => { - expect(e.message).toEqual('Permission denied'); + e => { + expect(e.code).toEqual(Parse.Error.OPERATION_FORBIDDEN); + expect(e.message).toEqual('Permission denied'); sanitizedErrorCall.checkMessage( 'Permission denied, user needs to be authenticated.', callCountBefore );This is optional but would make the required-auth suite consistent with the other permission tests.
spec/ParseAPI.spec.js (1)
1713-1733: Purge-without-master-key test correctly exercises sanitized error; consider tightening assertionsThe updated test now:
- Uses
getSanitizedErrorCallandcallCountBefore.- Asserts the client-facing
response.data.erroris'Permission denied'.- Verifies the logged detailed message contains
'unauthorized: master key is required'.To fully specify the behavior, you could also assert the HTTP status and/or error code, e.g.:
.catch(response => { - expect(response.data.error).toEqual('Permission denied'); + expect(response.status).toBe(403); + expect(response.data.code).toBe(Parse.Error.OPERATION_FORBIDDEN); + expect(response.data.error).toEqual('Permission denied'); sanitizedErrorCall.checkMessage( 'unauthorized: master key is required', callCountBefore );Not required, but this would make the test more robust.
src/Routers/GraphQLRouter.js (1)
4-24: Read‑only master key now returns a sanitized forbidden error for GraphQL config updatesUsing:
throw createSanitizedError( Parse.Error.OPERATION_FORBIDDEN, "read-only masterKey isn't allowed to update the GraphQL config.", );on
req.auth.isReadOnlyensures:
- Clients only see a generic “Permission denied” (via the helper).
- Logs still carry the detailed reason, matching the rest of the sanitization work.
For the changelog, an Angular-style PR title that reflects this broader change could be:
feat(security): sanitize permission errors
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
spec/AudienceRouter.spec.js(2 hunks)spec/LogsRouter.spec.js(3 hunks)spec/ParseAPI.spec.js(5 hunks)spec/ParseFile.spec.js(6 hunks)spec/ParseGlobalConfig.spec.js(3 hunks)spec/ParseGraphQLServer.spec.js(12 hunks)spec/ParseQuery.Aggregate.spec.js(2 hunks)spec/ParseUser.spec.js(8 hunks)spec/RestQuery.spec.js(3 hunks)spec/Schema.spec.js(15 hunks)spec/features.spec.js(3 hunks)spec/rest.spec.js(12 hunks)spec/schemas.spec.js(19 hunks)src/Controllers/SchemaController.js(4 hunks)src/Error.js(1 hunks)src/GraphQL/loaders/schemaMutations.js(4 hunks)src/GraphQL/parseGraphQLUtils.js(1 hunks)src/RestQuery.js(5 hunks)src/RestWrite.js(4 hunks)src/Routers/ClassesRouter.js(2 hunks)src/Routers/FilesRouter.js(2 hunks)src/Routers/GlobalConfigRouter.js(2 hunks)src/Routers/GraphQLRouter.js(2 hunks)src/Routers/PurgeRouter.js(1 hunks)src/Routers/PushRouter.js(2 hunks)src/Routers/SchemasRouter.js(4 hunks)src/Routers/UsersRouter.js(2 hunks)src/SharedRest.js(2 hunks)src/TestUtils.js(1 hunks)src/middlewares.js(2 hunks)src/rest.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (16)
- src/Routers/FilesRouter.js
- src/Routers/PurgeRouter.js
- src/middlewares.js
- src/Routers/SchemasRouter.js
- spec/ParseQuery.Aggregate.spec.js
- spec/RestQuery.spec.js
- spec/rest.spec.js
- src/Controllers/SchemaController.js
- src/SharedRest.js
- spec/features.spec.js
- src/RestQuery.js
- src/GraphQL/loaders/schemaMutations.js
- spec/ParseGraphQLServer.spec.js
- src/Routers/ClassesRouter.js
- src/GraphQL/parseGraphQLUtils.js
- src/Routers/GlobalConfigRouter.js
🧰 Additional context used
🧠 Learnings (9)
📓 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.
📚 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/Routers/GraphQLRouter.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/AudienceRouter.spec.jsspec/schemas.spec.jsspec/ParseUser.spec.jsspec/LogsRouter.spec.jsspec/Schema.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/AudienceRouter.spec.jsspec/schemas.spec.jsspec/ParseUser.spec.jsspec/Schema.spec.jsspec/ParseAPI.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/AudienceRouter.spec.jsspec/schemas.spec.jsspec/ParseUser.spec.jsspec/Schema.spec.jsspec/ParseAPI.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/AudienceRouter.spec.jsspec/schemas.spec.jsspec/ParseUser.spec.jsspec/Schema.spec.jsspec/ParseAPI.spec.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/AudienceRouter.spec.jsspec/schemas.spec.jsspec/Schema.spec.jsspec/ParseAPI.spec.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:
spec/schemas.spec.jsspec/Schema.spec.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
spec/ParseUser.spec.jsspec/ParseAPI.spec.js
🧬 Code graph analysis (12)
src/rest.js (1)
src/RestQuery.js (3)
require(7-7)require(9-9)require(10-10)
src/TestUtils.js (3)
spec/ParseLiveQuery.spec.js (4)
logger(299-299)logger(589-589)logger(632-632)require(8-8)spec/ParseAPI.spec.js (1)
require(9-9)spec/ParseConfigKey.spec.js (1)
loggerErrorSpy(5-5)
src/Error.js (2)
src/RestQuery.js (1)
defaultLogger(11-11)src/middlewares.js (2)
error(252-252)error(505-505)
spec/AudienceRouter.spec.js (2)
src/TestUtils.js (1)
getSanitizedErrorCall(85-104)src/middlewares.js (2)
error(252-252)error(505-505)
spec/schemas.spec.js (1)
src/TestUtils.js (1)
getSanitizedErrorCall(85-104)
spec/ParseGlobalConfig.spec.js (1)
src/TestUtils.js (1)
getSanitizedErrorCall(85-104)
spec/ParseUser.spec.js (1)
src/TestUtils.js (1)
getSanitizedErrorCall(85-104)
spec/LogsRouter.spec.js (1)
src/TestUtils.js (1)
getSanitizedErrorCall(85-104)
spec/Schema.spec.js (2)
spec/ParseAPI.spec.js (2)
require(9-9)Parse(6-6)src/TestUtils.js (1)
getSanitizedErrorCall(85-104)
src/RestWrite.js (2)
src/middlewares.js (4)
log(237-237)log(247-247)log(464-464)log(537-537)src/RestQuery.js (1)
defaultLogger(11-11)
spec/ParseAPI.spec.js (1)
src/TestUtils.js (2)
destroyAllDataPermanently(8-28)getSanitizedErrorCall(85-104)
spec/ParseFile.spec.js (2)
spec/ParseAPI.spec.js (1)
require(9-9)src/TestUtils.js (1)
getSanitizedErrorCall(85-104)
⏰ 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). (15)
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: Node 18
- GitHub Check: Node 20
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: MongoDB 7, ReplicaSet
🔇 Additional comments (18)
spec/LogsRouter.spec.js (1)
4-5: LogsRouter master-key test correctly validates sanitized vs logged detailThe updated test now asserts the client sees a generic
403/'Permission denied'while also verifying that the logger captured'unauthorized: master key is required'viagetSanitizedErrorCall, which is exactly what this PR aims for.Also applies to: 55-70
spec/ParseGlobalConfig.spec.js (1)
5-5: GlobalConfig master-key test aligns with sanitized error behaviorThe test now correctly expects a generic
'Permission denied'on the client while asserting that the logger recorded'unauthorized: master key is required'usinggetSanitizedErrorCall, matching the new sanitization contract.Also applies to: 223-242
spec/ParseUser.spec.js (4)
15-15: New TestUtils import is appropriate for user-related security testsImporting
getSanitizedErrorCallhere is appropriate, as multiple user/session flows now verify that detailed messages are logged while clients receive only sanitized text.
2635-2670: Confirm intent to sanitize this specific invalid-session-token REST pathIn
it('cannot delete session if no sessionToken'), the firstPUT /sessions/:idwith an invalid session token now returns'Permission denied'(code 209), while the test asserts the detailed'Invalid session token'message is only present in the logs viagetSanitizedErrorCall.Elsewhere in this file, some flows (e.g. "Invalid session tokens are rejected") still expect the full
'Invalid session token'text in the HTTP response. Please confirm that this asymmetry is intentional (RESTPUT /sessions/:idsanitized, other session-token checks not) and not a missed conversion.
3363-3394: Email-verification test correctly distinguishes client vs logged messagesThe
"should not allow updates to emailVerified"test now:
- Installs a sanitized logger spy after reconfiguration.
- Expects the client error message
'Permission denied'.- Verifies the logged detail
"Clients aren't allowed to manually update email verification.".This precisely exercises the new sanitization helper and protects against regressions in the sensitive email-verification path.
4387-4413: loginAs master-key test is well-aligned with sanitized HTTP errorsFor
loginAswithout master key, the test now:
- Tracks sanitized logger calls with
getSanitizedErrorCall.- Expects
OPERATION_FORBIDDENwith client message'Permission denied'.- Asserts the logged detail contains
'master key is required'.Using a substring for the logged message (
toContain) gives some flexibility if the detailed text is extended, while still verifying that the critical part is logged.spec/ParseFile.spec.js (1)
8-8: File deletion tests now robustly cover sanitized master-key errorsThe "blocks file deletions with missing or incorrect master-key header" test:
- Correctly expects
403 / 'Permission denied'for both missing and wrong master key.- Uses
getSanitizedErrorCallwith separatecallCountBeforevalues to assert that detailed'unauthorized: master key is required'messages were logged for each attempt.This gives strong coverage of both branches of the authorization check.
Also applies to: 135-184
src/RestWrite.js (2)
20-21: LGTM!The import statements are correct and follow the same pattern used in other files like
src/middlewares.jsandsrc/RestQuery.js.
666-669: ****The function signature for
createSanitizedError(src/Error.js:11) accepts only 2 parameters:errorCodeanddetailedMessage. The call at line 205 does not pass a logger parameter—it only passes 2 parameters, identical to line 666. All 26+ usages throughout the codebase consistently use 2 parameters only. The code is correct and consistent; no changes are needed.Likely an incorrect or invalid review comment.
spec/schemas.spec.js (2)
1811-1844: Good coverage of sanitized CLP error for addFieldThis test now:
- Asserts the client-facing message
'Permission denied'.- Verifies
err.code === Parse.Error.OPERATION_FORBIDDEN.- Uses
getSanitizedErrorCallto assert the detailed log message ('Permission denied for action addField on class AClass').That combination exercises both the sanitized surface and the logged detail as intended.
2175-2511: CLP find tests now validate both error code and sanitized logThe
validate CLP 1–4tests correctly:
- Capture the logger error call count before the forbidden query.
- Assert the thrown error has message
'Permission denied'and codeParse.Error.OPERATION_FORBIDDEN.- Assert the logged detailed message uses the correct action (
find) and class name.This matches the new sanitized error behavior and addresses the previous request to also test the error code.
spec/Schema.spec.js (2)
5-5: ImportinggetSanitizedErrorCallhere is appropriatePulling
getSanitizedErrorCallfrom../lib/TestUtilsmatches how other specs hook into the sanitized error logger and keeps this file aligned with the new test utilities.
252-297: CLPcounttest now fully validates sanitized error behaviorThe updated
class-level permissions test count:
- Captures logger call count before the forbidden
query.count().- Asserts
err.message === 'Permission denied'.- Asserts
err.code === Parse.Error.OPERATION_FORBIDDEN.- Verifies the logged detailed message (
'Permission denied for action count on class Stuff').This is a solid end-to-end check of both the client-visible and logged aspects of the sanitized error.
spec/ParseAPI.spec.js (2)
9-9: Destructured TestUtils import is clearer and consistentSwitching to:
const { getSanitizedErrorCall, destroyAllDataPermanently } = require('../lib/TestUtils');simplifies call sites and matches the pattern used elsewhere in the suite.
164-208:destroyAllDataPermanently()usage remains correct after refactorThese tests now call
destroyAllDataPermanently()directly instead of viaTestUtils., while preserving the same promise chain. Behavior and semantics remain unchanged.src/Routers/PushRouter.js (1)
11-21: Push with read‑only master key now uses sanitized permission errorThe new guard:
if (req.auth.isReadOnly) { throw createSanitizedError( Parse.Error.OPERATION_FORBIDDEN, "read-only masterKey isn't allowed to send push notifications.", ); }correctly routes this case through the sanitization helper, so clients see a generic forbidden error while logs retain detail.
src/rest.js (1)
321-332: Session‑missing path for_Usernow sanitized throughcreateSanitizedError
handleSessionMissingErrornow transforms an_UserOBJECT_NOT_FOUNDunder non‑master auth into:const { createSanitizedError } = require('./Error'); throw createSanitizedError(Parse.Error.SESSION_MISSING, 'Insufficient auth.');This keeps the error code (
SESSION_MISSING) while ensuring the client only sees the generic sanitized message, with the detailed context available in logs.src/Routers/UsersRouter.js (1)
335-373:/loginAsnow correctly uses sanitized permission error for non‑master callsThe updated guard:
if (!req.auth.isMaster) { throw createSanitizedError( Parse.Error.OPERATION_FORBIDDEN, 'master key is required', ); }ensures
/loginAs:
- Still enforces master‑key only access.
- Returns a sanitized forbidden error to clients, while logging the specific reason, in line with the rest of the sanitization work.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/RestWrite.js (1)
33-37: Consider passing logger for consistency.While
createSanitizedErrorlikely has an internal fallback todefaultLogger, lines 666-669 and 1457-1461 have access tothis.config.loggerControllerbut don't pass it as a parameter. For consistency with line 204-209, consider passing the logger where available:if (auth.isReadOnly) { + const log = (this.config && this.config.loggerController) || defaultLogger; throw createSanitizedError( Parse.Error.OPERATION_FORBIDDEN, 'Cannot perform a write operation when using readOnlyMasterKey', + log ); }Note: At line 33,
this.confighasn't been assigned yet (assigned at line 39), so the fallback pattern is appropriate here. However, lines 666-669 and 1457-1461 should follow the same pattern as line 204-209.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
spec/ParseInstallation.spec.js(3 hunks)spec/rest.spec.js(15 hunks)spec/schemas.spec.js(18 hunks)spec/vulnerabilities.spec.js(2 hunks)src/RestWrite.js(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- spec/schemas.spec.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.
📚 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:
spec/rest.spec.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/rest.spec.jsspec/ParseInstallation.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/rest.spec.jsspec/ParseInstallation.spec.jsspec/vulnerabilities.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/rest.spec.jsspec/ParseInstallation.spec.jsspec/vulnerabilities.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/rest.spec.jsspec/ParseInstallation.spec.jsspec/vulnerabilities.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/rest.spec.jsspec/vulnerabilities.spec.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
spec/ParseInstallation.spec.js
🧬 Code graph analysis (4)
spec/rest.spec.js (1)
src/TestUtils.js (1)
getSanitizedErrorCall(85-104)
src/RestWrite.js (2)
src/middlewares.js (4)
log(237-237)log(247-247)log(464-464)log(537-537)src/RestQuery.js (1)
defaultLogger(11-11)
spec/ParseInstallation.spec.js (1)
src/TestUtils.js (1)
getSanitizedErrorCall(85-104)
spec/vulnerabilities.spec.js (1)
src/TestUtils.js (1)
getSanitizedErrorCall(85-104)
🔇 Additional comments (8)
src/RestWrite.js (2)
20-21: LGTM!The imports are correctly added to support error sanitization.
204-209: LGTM!The logger fallback pattern is correctly implemented, matching the pattern used in middlewares.js.
spec/vulnerabilities.spec.js (1)
16-23: LGTM!The test correctly verifies that:
- The client receives the sanitized error message "Permission denied"
- The detailed error message "Invalid object ID." is logged server-side
This properly validates the error sanitization behavior.
spec/ParseInstallation.spec.js (1)
160-188: LGTM!The test correctly verifies error sanitization for the Installation query restriction. The error code remains OPERATION_FORBIDDEN (119), and the detailed message is properly logged server-side while sending a generic message to the client.
spec/rest.spec.js (4)
317-336: LGTM!The test correctly validates error sanitization when client class creation is disabled. The original detailed message is logged server-side while the client receives a generic "Permission denied" message.
777-794: LGTM!Both tests correctly verify that attempts to access restricted collections (_PushStatus and _GlobalConfig) through pointers are properly sanitized. The detailed error messages are logged server-side while clients receive "Permission denied".
Also applies to: 796-813
957-1005: LGTM!The read-only masterKey tests properly validate error sanitization for write operations. Both the synchronous throw test and the HTTP request test correctly verify that detailed messages are logged while clients receive generic "Permission denied" messages.
1029-1173: LGTM!All read-only masterKey restriction tests (RestWrite creation, schema operations, config updates, and push notifications) correctly implement error sanitization. Each test:
- Captures the call count before the operation
- Expects the sanitized "Permission denied" message
- Verifies the original detailed message was logged server-side
There was a problem hiding this comment.
Is this helper really necessary? Why not spy on logger and check the calls in the expect?
There was a problem hiding this comment.
you can have multiple calls of the logger in a test so we need to get the correct call. This is why I get the number of call just before the operation.
Maybe there is an another solution, but not sure to see how
There was a problem hiding this comment.
I don't understand what you are trying to achieve. Why not spy on the logger once for all the tests, then reset the calls before executing what should trigger the log, then do a simple match across all spy calls for the expected message?
There was a problem hiding this comment.
Ok understand what you mean !
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)
spec/schemas.spec.js (1)
2183-2617: CLP 1–5: good sanitized coverage; double‑check CLP 5 log action labelThe CLP 1–4 tests are nicely updated to:
- Expect
'Permission denied'withParse.Error.OPERATION_FORBIDDEN.- Assert that
logger.errorlogs'Sanitized error:'with a detailed message like'Permission denied for action find on class AClass'.In CLP 5, though, the first forbidden call is a
query.find()by theadminuser, but the expectation checks for:'Permission denied for action create on class AClass'while the second forbidden
find()(user2) expects'... action find on class AClass'.That “create” label for a read operation is likely a copy/paste error in the expectation (or a bug in the underlying log message). I’d suggest:
- Either changing that expectation to
'... action find on class AClass', or- If the implementation truly logs
createthere, revisiting the implementation so the logged action matches the actual operation.Also, consider consistently asserting
err.codein all these CLP tests (you already do for most) to guard against regressions in error-code mapping.
♻️ Duplicate comments (1)
src/rest.js (1)
172-177: _User delete still throws unsanitized SESSION_MISSING errorThis guard still exposes a detailed message (
'Insufficient auth to delete user') directly to clients and bypasses the new sanitization helpers, unlike the rest of the auth/session paths.You can align it with the centralized sanitization by using
createSanitizedErrorhere as well, for example:if (className === '_User' && auth.isUnauthenticated()) { - throw new Parse.Error(Parse.Error.SESSION_MISSING, 'Insufficient auth to delete user'); + throw createSanitizedError( + Parse.Error.SESSION_MISSING, + 'Insufficient auth to delete user' + ); }This preserves the error code while logging the detailed reason via the shared helper.
🧹 Nitpick comments (9)
src/GraphQL/loaders/usersQueries.js (1)
1-20: PR-level: suggest Angular-style scoped title and brief docs noteFor changelog clarity and per repo conventions, consider renaming the PR to something like:
feat(security): sanitize authorization errors
or, if you want to be more specific,feat(security): sanitize master key and permission errorsAlso consider a short note in the README or security-related docs mentioning that certain authorization errors now return a generic “Permission denied” while logging details server-side, so integrators understand the changed messages.
Based on learnings
spec/ParseUser.spec.js (2)
2638-2675: Session DELETE without token: confirm intended message change and logger verificationThe test now expects:
b.code === 209withb.error === 'Permission denied'for DELETE/sessions/:idwithout a session token, andlogger.errorcalled with'Sanitized error:'plus'Invalid session token'in the message.This aligns with the new sanitizer contract (generic client message, detailed log), but changes the previous client-visible text (was a more specific “Session token required.” / “Invalid session token”). Please confirm this behavior change for this specific path is intentional and consistent with other 209 responses (e.g., the nearby “cannot update session if invalid or no session token” test, which still asserts the old strings).
3368-3400: Email verification update test now correctly asserts sanitized error and logIn “should not allow updates to emailVerified” you:
- Capture the shared logger via
require('../lib/logger').default,- Spy on
logger.errorand reset before the forbidden save, and- Expect
err.message === 'Permission denied'plus a logged message containing"Clients aren't allowed to manually update email verification.".This is a good end‑to‑end check that the sanitizer hides internal wording from clients but preserves it in logs. The per-test logger setup is a bit repetitive but acceptable.
spec/rest.spec.js (1)
958-1182: Read-only masterKey suite: consistent sanitization checks, consider DRYing logger setupWithin
describe('read-only masterKey'), multiple tests now:
- Expect
OPERATION_FORBIDDENerrors with the generic'Permission denied'message (or equivalent), and- Assert
logger.errorwas called with'Sanitized error:'plus a descriptive fragment for each forbidden operation (create, schema ops, config, push, RestWrite construction, etc.).This effectively verifies that all read-only-master-key write paths go through
createSanitizedError. The repeatedconst logger = require('../lib/logger').default; const loggerErrorSpy = spyOn(...); loggerErrorSpy.calls.reset();blocks could be factored into a small helper to reduce duplication, but that’s optional.src/rest.js (2)
1-20: PR meta: title & docs suggestionFor the changelog-style title, consider something like:
feat(security): sanitize authorization error responsesAlso, because this changes client-visible error messages across REST/GraphQL, it would be helpful (though not strictly required) to add a short note to the README or security/error-handling docs explaining that many auth/CLP errors now return a generic “Permission denied” while logging the detailed reason server-side.
322-333: Avoid duplicaterequire('./Error')inhandleSessionMissingError
createSanitizedErroris already imported at the top of this module, buthandleSessionMissingErrordoes a second dynamicrequire('./Error'). That’s redundant and slightly obscures intent.You can simplify and avoid confusion by reusing the top-level import:
-function handleSessionMissingError(error, className, auth) { +function handleSessionMissingError(error, className, auth) { // If we're trying to update a user without / with bad session token if ( className === '_User' && error.code === Parse.Error.OBJECT_NOT_FOUND && !auth.isMaster && !auth.isMaintenance ) { - const { createSanitizedError } = require('./Error'); - throw createSanitizedError(Parse.Error.SESSION_MISSING, 'Insufficient auth.'); } throw error; }spec/schemas.spec.js (1)
169-199: Schema master-key tests: sanitized error & logging checks look solidThe updated expectations for these two schema master-key tests correctly:
- Assert the generic client message (
'Permission denied'with 403).- Verify that
logger.errorreceives'Sanitized error:'plus the original detailed text ("unauthorized: master key is required").This matches the PR’s goal of hiding details from clients while keeping them in logs. You may optionally align the logger import with other specs by using
.loggerinstead of.defaultfor consistency.spec/ParseAPI.spec.js (1)
1712-1735: Purge‑without‑master negative test correctly validates sanitized behaviorThe purge test now:
- Expects client error
'Permission denied'when DELETE /purge/TestObject is called with only the REST key.- Asserts that
logger.errorwas called with'Sanitized error:'and a message containing the original"unauthorized: master key is required"detail.That’s exactly the coupling we want between sanitized responses and logged detail. As in other specs, you may optionally align the logger import to use
.loggerinstead of.defaultif that’s the prevailing pattern.spec/Schema.spec.js (1)
1447-1725: Required‑auth tests: sanitized behavior covered; consider asserting error codes tooThe required-auth tests for
find/create/get:
- Correctly exercise unauthenticated paths.
- Assert the generic
'Permission denied'message.- Verify
logger.erroris called with'Sanitized error:'and a detailed reason like'Permission denied, user needs to be authenticated.'.To fully guard against regressions in the error mapping, it would be worthwhile to also assert the error code (e.g.,
Parse.Error.OPERATION_FORBIDDEN) in these tests, as you already do in other CLP-related specs.That keeps behavior non-breaking from the SDK perspective while still allowing you to refactor messages and sanitization details.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
spec/AudienceRouter.spec.js(1 hunks)spec/LogsRouter.spec.js(2 hunks)spec/ParseAPI.spec.js(5 hunks)spec/ParseFile.spec.js(5 hunks)spec/ParseGlobalConfig.spec.js(2 hunks)spec/ParseGraphQLServer.spec.js(15 hunks)spec/ParseInstallation.spec.js(3 hunks)spec/ParseQuery.Aggregate.spec.js(1 hunks)spec/ParseUser.spec.js(8 hunks)spec/RestQuery.spec.js(2 hunks)spec/Schema.spec.js(14 hunks)spec/features.spec.js(2 hunks)spec/rest.spec.js(14 hunks)spec/schemas.spec.js(17 hunks)spec/vulnerabilities.spec.js(1 hunks)src/GraphQL/loaders/usersQueries.js(2 hunks)src/Routers/UsersRouter.js(4 hunks)src/TestUtils.js(1 hunks)src/rest.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- spec/LogsRouter.spec.js
- spec/ParseQuery.Aggregate.spec.js
- spec/ParseInstallation.spec.js
- spec/AudienceRouter.spec.js
- spec/vulnerabilities.spec.js
- spec/ParseFile.spec.js
- spec/ParseGlobalConfig.spec.js
- spec/ParseGraphQLServer.spec.js
- src/TestUtils.js
🧰 Additional context used
🧠 Learnings (7)
📓 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: 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()`.
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.
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.
📚 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/Schema.spec.jsspec/schemas.spec.jsspec/features.spec.jsspec/ParseUser.spec.jsspec/rest.spec.jsspec/RestQuery.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/Schema.spec.jsspec/schemas.spec.jsspec/ParseUser.spec.jsspec/rest.spec.jsspec/RestQuery.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/Schema.spec.jsspec/schemas.spec.jsspec/ParseUser.spec.jsspec/rest.spec.jsspec/ParseAPI.spec.jsspec/RestQuery.spec.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/Schema.spec.jsspec/schemas.spec.jsspec/rest.spec.jsspec/ParseAPI.spec.jsspec/RestQuery.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/Schema.spec.jsspec/schemas.spec.jsspec/ParseUser.spec.jsspec/rest.spec.jsspec/ParseAPI.spec.jsspec/RestQuery.spec.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:
spec/Schema.spec.jsspec/schemas.spec.jsspec/rest.spec.js
🧬 Code graph analysis (9)
spec/Schema.spec.js (2)
spec/ParseLiveQuery.spec.js (3)
logger(299-299)logger(589-589)logger(632-632)src/Adapters/Storage/Postgres/PostgresStorageAdapter.js (1)
logger(19-19)
src/GraphQL/loaders/usersQueries.js (4)
src/middlewares.js (2)
context(76-76)info(351-351)src/GraphQL/loaders/parseClassMutations.js (3)
context(80-80)context(183-183)context(288-288)src/GraphQL/loaders/parseClassQueries.js (2)
context(20-20)context(104-104)src/RestWrite.js (2)
userId(538-538)userId(548-548)
src/rest.js (2)
src/RestQuery.js (3)
require(7-7)require(9-9)require(10-10)src/SharedRest.js (1)
require(9-9)
spec/schemas.spec.js (2)
spec/ParseLiveQuery.spec.js (3)
logger(299-299)logger(589-589)logger(632-632)src/Adapters/Storage/Postgres/PostgresStorageAdapter.js (1)
logger(19-19)
spec/features.spec.js (3)
src/rest.js (3)
require(15-15)require(16-16)require(330-330)src/Error.js (1)
error(38-38)src/middlewares.js (2)
error(252-252)error(505-505)
spec/ParseUser.spec.js (2)
spec/ParseLiveQuery.spec.js (4)
logger(299-299)logger(589-589)logger(632-632)require(8-8)src/Adapters/Storage/Postgres/PostgresStorageAdapter.js (1)
logger(19-19)
spec/rest.spec.js (3)
spec/ParseLiveQuery.spec.js (3)
logger(299-299)logger(589-589)logger(632-632)src/Adapters/Storage/Postgres/PostgresStorageAdapter.js (1)
logger(19-19)src/rest.js (3)
require(15-15)require(16-16)require(330-330)
spec/ParseAPI.spec.js (2)
src/TestUtils.js (1)
destroyAllDataPermanently(8-28)spec/ParseLiveQuery.spec.js (3)
logger(299-299)logger(589-589)logger(632-632)
spec/RestQuery.spec.js (1)
src/RestQuery.js (3)
require(7-7)require(9-9)require(10-10)
🪛 GitHub Check: Lint
spec/ParseUser.spec.js
[warning] 17-17:
More than 2 blank lines not allowed
⏰ 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 15, PostGIS 3.5
- GitHub Check: Node 20
- GitHub Check: Node 22
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: Redis Cache
- GitHub Check: Docker Build
- GitHub Check: Code Analysis (javascript)
- GitHub Check: Benchmarks
🔇 Additional comments (12)
src/GraphQL/loaders/usersQueries.js (1)
7-13: GraphQL session lookup now aligned with sanitized error patternSwitching missing/invalid session-token errors in
getUserFromSessionTokentocreateSanitizedError(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token')keeps the error code but routes messaging through the central sanitizer, consistent with the rest of the PR. No functional issues spotted here.Also applies to: 65-67
spec/features.spec.js (1)
22-41: Master-key feature test correctly validates sanitized error and loggingThe added logger spy and updated expectations (
error.data.error === 'Permission denied'pluslogger.errorcontaining'unauthorized: master key is required') correctly verify that/serverInfonow returns a sanitized message while logging the detailed cause. The instrumentation is sound.spec/ParseUser.spec.js (1)
4392-4420:loginAswithout master key: test matches new sanitized UsersRouter behaviorThe negative “login as other user without the master key” test now:
- Expects
Parse.Error.OPERATION_FORBIDDENwitherr.data.error === 'Permission denied', and- Asserts
logger.errorwas called with'Sanitized error:'and the original'master key is required'fragment.This matches the new
handleLogInAsimplementation usingcreateSanitizedErrorand gives good coverage of both client and log surfaces.spec/rest.spec.js (2)
316-333: Non-existent client class create: sanitized error and logger assertions look correctFor “handles create on non-existent class when disabled client class creation” you now:
- Expect
Parse.Error.OPERATION_FORBIDDENwitherr.message === 'Permission denied', and- Verify
logger.errorlogged a sanitized message containing"This user is not allowed to access non-existent class: ClientClassCreation".This matches the new
createSanitizedErrorusage in the REST layer and keeps the error code stable while hiding details from clients.
777-815: Pointer access to volatile/_GlobalConfig: sanitized denial and logging verifiedBoth tests:
- “cannot get object in volatileClasses if not masterKey through pointer”
- “cannot get object in _GlobalConfig if not masterKey through pointer”
now assert rejection with
'Permission denied'while checkinglogger.errorreceived a sanitized message containing the original collection-specific text. This provides solid regression coverage that pointer expansion respects the new sanitization rules.spec/RestQuery.spec.js (2)
156-176: Disabled client class creation query: sanitizer and logging tested end-to-endThe “query non-existent class when disabled client class creation” test now:
- Uses a logger spy scoped to this test,
- Expects
Parse.Error.OPERATION_FORBIDDENwitherr.message === 'Permission denied', and- Verifies
logger.errorlogged the original “non-existent class: ClientClassCreation” reason.This pairs well with the corresponding create-path test in
rest.spec.jsand confirms the query side also goes throughcreateSanitizedError.
220-250: Protected field access now returns generic 'Permission denied'In “query protected field” you now expect an
OPERATION_FORBIDDENerror with message'Permission denied'when queryingzipfor a protected user, instead of a more descriptive error. This is consistent with the PR’s goal of reducing information leakage. Just be aware that this trades off some client-facing specificity for security; if that’s intentional, the change looks good.src/Routers/UsersRouter.js (1)
19-20: UsersRouter now uses sanitized errors for/users/meand/loginAs
handleMenow callscreateSanitizedError(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token')both when the session token is missing and when the_Sessionlookup fails, so clients get a sanitized message while the detailed cause is logged.handleLogInAsguards on!req.auth.isMasterwithcreateSanitizedError(Parse.Error.OPERATION_FORBIDDEN, 'master key is required'), matching the updated “login as other user without the master key” test expectations.Error codes remain unchanged, so this should be a non-breaking but more secure behavior change.
Also applies to: 172-190, 335-341
src/rest.js (1)
193-201: Sanitizing invalid session token in_Sessiondelete path looks goodRouting the “wrong user deleting _Session” case through
createSanitizedError(Parse.Error.INVALID_SESSION_TOKEN, 'Invalid session token')keeps the error code while avoiding leaking ownership details to clients and still invalidating the session token via the cache adapter.spec/schemas.spec.js (1)
1817-1855: CLP addField negative test now validates code and sanitized logThis test now checks:
err.message === 'Permission denied'.err.code === Parse.Error.OPERATION_FORBIDDEN.- Logger error contains the detailed reason (
'Permission denied for action addField on class AClass').That’s a good balance of API-level and observability coverage for the sanitized CLP path.
spec/ParseAPI.spec.js (1)
9-10: Refined TestUtils import/readability without behavior changeSwitching to:
const { destroyAllDataPermanently } = require('../lib/TestUtils');and calling
destroyAllDataPermanently()directly is a straightforward cleanup and matches the named export style fromTestUtils. The surrounding promise chains and test behavior remain the same.Also applies to: 172-214
spec/Schema.spec.js (1)
251-297: CLP count test now robustly covers sanitized error and log outputThe updated
class-level permissions test countcorrectly:
- Verifies the allowed
find()path still works.- Then checks that
count()fails for this CLP with:
err.message === 'Permission denied'.err.code === Parse.Error.OPERATION_FORBIDDEN.logger.error('Sanitized error:', <msg containing "Permission denied for action count on class Stuff">).This gives strong coverage of both the external contract and the new sanitization/logging behavior.
spec/AudienceRouter.spec.js
Outdated
| const logger = require('../lib/logger').default; | ||
| const loggerErrorSpy = spyOn(logger, 'error').and.callThrough(); |
There was a problem hiding this comment.
Move this outside the tests, to the top of the root describe block; only needs to be done once.
There was a problem hiding this comment.
We need to do this in a beforeEach because some elements in the tests (I identified reconfigureServer, but there are others I haven’t identified yet) reset the logger. We need to re-require the default logger to ensure it works correctly.
I tried using a beforeAll, but it didn’t work, which is why I implemented it in each test. However, a beforeEach solution might be a good alternative
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/ParseUser.spec.js (1)
3361-3403: Avoid double-spying onlogger.errorin emailVerified testIn this test you now have:
- A suite-level
beforeEachthat already doesspyOn(logger, 'error')(vialoggerErrorSpydefined at the top ofdescribe('Parse.User testing')).- Additional test-local
let logger; let loggerErrorSpy;plus anotherspyOn(logger, 'error')inside the promise chain.Calling
spyOntwice on the same method in Jasmine throws (“error is already spied on”), so this test will fail even before reaching the assertions.You can rely on the describe-level spy and just reset its calls in this test:
- it('should not allow updates to emailVerified', done => { - const emailAdapter = { + it('should not allow updates to emailVerified', done => { + const emailAdapter = { sendVerificationEmail: () => {}, sendPasswordResetEmail: () => Promise.resolve(), sendMail: () => Promise.resolve(), }; - - let logger; - let loggerErrorSpy; @@ reconfigureServer({ appName: 'unused', verifyUserEmails: true, emailAdapter: emailAdapter, publicServerURL: 'http://localhost:8378/1', }) .then(() => { - logger = require('../lib/logger').default; - loggerErrorSpy = spyOn(logger, 'error').and.callThrough(); return user.signUp(); }) .then(() => { loggerErrorSpy.calls.reset(); return Parse.User.current().set('emailVerified', true).save(); }) @@ .catch(err => { expect(err.message).toBe('Permission denied'); expect(loggerErrorSpy).toHaveBeenCalledWith( 'Sanitized error:', jasmine.stringContaining("Clients aren't allowed to manually update email verification.") ); done(); }); });This reuses the existing spy and keeps the log assertion intact.
🧹 Nitpick comments (2)
spec/ParseUser.spec.js (2)
1-8: Angular-style PR title suggestion for changelogTo make the changelog entry clearer and follow the Angular convention, consider renaming the PR to something like:
feat(security): sanitize client error messages and log detailed causesThis highlights both the security impact and the main behavior change.
2641-2675: Optional: keep session-related error messaging consistent across endpointsMinor consistency thought: some session-related tests now expect a generic
'Permission denied'(with code 209 and detailed cause only in logs), while others still assert more specific messages like'Invalid session token'or'Session token required.'.If the intention is to fully standardize client-facing messages for authorization/session failures, you might want to review the remaining session endpoints and tests to either:
- also use the generic
'Permission denied'+ detailed logged message, or- keep a deliberate distinction and document which cases intentionally preserve more specific messages.
Not blocking, but worth a quick double-check against the overall sanitization strategy.
Also applies to: 3361-3403, 4399-4423
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
spec/AudienceRouter.spec.js(2 hunks)spec/ParseFile.spec.js(5 hunks)spec/ParseGraphQLServer.spec.js(15 hunks)spec/ParseUser.spec.js(9 hunks)spec/Schema.spec.js(12 hunks)spec/rest.spec.js(15 hunks)spec/schemas.spec.js(12 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- spec/ParseFile.spec.js
- spec/ParseGraphQLServer.spec.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.
📚 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/ParseUser.spec.jsspec/schemas.spec.jsspec/AudienceRouter.spec.jsspec/rest.spec.jsspec/Schema.spec.js
📚 Learning: 2025-08-27T09:08:34.252Z
Learnt from: EmpiDev
Repo: parse-community/parse-server PR: 9770
File: src/triggers.js:446-454
Timestamp: 2025-08-27T09:08:34.252Z
Learning: When analyzing function signature changes in Parse Server codebase, verify that call sites are actually incorrect before flagging them. Passing tests are a strong indicator that function calls are already properly aligned with new signatures.
Applied to files:
spec/ParseUser.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/ParseUser.spec.jsspec/schemas.spec.jsspec/AudienceRouter.spec.jsspec/rest.spec.jsspec/Schema.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/ParseUser.spec.jsspec/schemas.spec.jsspec/AudienceRouter.spec.jsspec/rest.spec.jsspec/Schema.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/ParseUser.spec.jsspec/schemas.spec.jsspec/AudienceRouter.spec.jsspec/rest.spec.jsspec/Schema.spec.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/ParseUser.spec.jsspec/schemas.spec.jsspec/AudienceRouter.spec.jsspec/rest.spec.jsspec/Schema.spec.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:
spec/schemas.spec.jsspec/rest.spec.jsspec/Schema.spec.js
🧬 Code graph analysis (3)
spec/schemas.spec.js (2)
spec/helper.js (5)
reconfigureServer(180-214)require(6-6)require(8-8)require(61-61)Parse(4-4)spec/ParseLiveQuery.spec.js (6)
config(1080-1080)config(1152-1164)config(1193-1211)logger(299-299)logger(589-589)logger(632-632)
spec/AudienceRouter.spec.js (3)
spec/ParseLiveQuery.spec.js (3)
logger(299-299)logger(589-589)logger(632-632)src/rest.js (2)
require(15-15)require(16-16)src/middlewares.js (2)
error(252-252)error(505-505)
spec/rest.spec.js (1)
spec/ParseLiveQuery.spec.js (3)
logger(299-299)logger(589-589)logger(632-632)
⏰ 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). (15)
- GitHub Check: PostgreSQL 15, PostGIS 3.3
- GitHub Check: PostgreSQL 17, PostGIS 3.5
- GitHub Check: PostgreSQL 16, PostGIS 3.5
- GitHub Check: PostgreSQL 15, PostGIS 3.5
- GitHub Check: Node 22
- GitHub Check: Node 18
- GitHub Check: PostgreSQL 18, PostGIS 3.6
- GitHub Check: PostgreSQL 15, PostGIS 3.4
- GitHub Check: MongoDB 7, ReplicaSet
- GitHub Check: Node 20
- GitHub Check: Redis Cache
- GitHub Check: MongoDB 8, ReplicaSet
- GitHub Check: MongoDB 6, ReplicaSet
- GitHub Check: Docker Build
- GitHub Check: Benchmarks
🔇 Additional comments (18)
spec/AudienceRouter.spec.js (2)
7-13: Logger spy setup for sanitized error tests is soundCentralizing
loggerErrorSpyinbeforeEachcleanly instruments all tests in this suite without altering behavior (thanks to.and.callThrough()), and keeps per-test log history isolated.
272-285: Master-key audience tests correctly assert sanitized errors and loggingThese tests now:
- Expect the generic client message
'Permission denied'instead of leaking “master key is required”.- Verify
logger.erroris called with'Sanitized error:'and a message containing'unauthorized: master key is required'.- Reset the spy before each assertion.
This matches the intended security behavior while preserving observability.
Also applies to: 287-297, 299-309, 311-323, 325-335
spec/schemas.spec.js (3)
149-158: Centralized logger spy in schemas suite is appropriateInitializing
loggerErrorSpyonce per test run viabeforeEachgives you consistent instrumentation for all schema tests and keeps log assertions local to each spec.
174-200: REST-key schema access tests now correctly exercise sanitizationFor REST-key access to
/schemasand/schemas/:class, the tests now:
- Expect HTTP 403 with
error === 'Permission denied'.- Assert
logger.errorwas called with'Sanitized error:'and the original"unauthorized: master key is required"text.This is the right balance between not leaking internal details to clients and keeping full diagnostics in logs.
1818-1853: CLP and addField permission tests now assert both client message, code, and logged detailAcross the “addField not allowed” and CLP validation tests you now:
- Reset
loggerErrorSpyimmediately before the forbidden operation.- Assert
err.message === 'Permission denied'.- Assert
err.code === Parse.Error.OPERATION_FORBIDDEN.- Check that
logger.errorreceived'Sanitized error:'plus a detailed message like'Permission denied for action find on class AClass'.This thoroughly verifies the new sanitized error behavior without weakening code-level signaling.
Also applies to: 2213-2225, 2276-2289, 2364-2376, 2443-2455, 2477-2487, 2561-2595
spec/ParseUser.spec.js (3)
41-47: Global logger spy for Parse.User tests is appropriateAdding a
beforeEachthat spies onlogger.errorfor the wholeParse.User testingsuite gives you a single, reusable instrumentation point for all sanitized-error assertions without impacting behavior (thanks to.and.callThrough()).
2641-2675: Session delete without token: sanitized client error and logged root cause are well coveredThis test now:
- Confirms the server still returns code
209for session issues.- Asserts the client-facing message is the generic
'Permission denied'.- Verifies
logger.errorwas called with'Sanitized error:'and a message containing'Invalid session token'.That matches the PR goal of hiding details from clients while keeping full diagnostics in logs.
4297-4303: loginAs unauthorized flow correctly validates sanitization and loggingFor the
login as other usersuite you:
- Introduce a local
loggerErrorSpywith a simplebeforeEachspy onlogger.error.- In the “without the master key” test, reset the spy, assert
code === Parse.Error.OPERATION_FORBIDDENanderror === 'Permission denied', and verify a logged message containing'master key is required'.This mirrors the rest of the PR’s security behavior and keeps the tests well-focused.
Also applies to: 4399-4423
spec/Schema.spec.js (4)
23-29: LGTM: Logger spy setup is correct.The spy infrastructure properly captures sanitized error logs while allowing the original logger to function, which is essential for verifying both client-facing messages and server-side logging.
282-294: LGTM: Count permission test properly verifies sanitized error handling.The test correctly:
- Resets the logger spy to isolate error verification
- Asserts the generic "Permission denied" client message
- Verifies the detailed error is logged server-side with the original message
- Preserves the error code (OPERATION_FORBIDDEN)
1437-1443: LGTM: Logger spy setup for requiredAuth tests.Proper test isolation with separate spy setup for this test suite.
1467-1479: LGTM: Comprehensive requiredAuth error sanitization coverage.All requiredAuth test scenarios properly verify:
- Generic client error messages ("Permission denied")
- Detailed server-side logging (e.g., "user needs to be authenticated")
- Preserved error codes
The consistent pattern across find, create, and get operations provides thorough coverage of authentication requirement violations.
Also applies to: 1567-1580, 1657-1670, 1705-1717
spec/rest.spec.js (6)
14-22: LGTM: Logger spy setup for rest create tests.Proper initialization of error logging verification infrastructure.
325-335: LGTM: Client class creation test properly verifies sanitized errors.The test correctly validates that when client class creation is disabled, users receive a generic "Permission denied" message while the detailed reason ("not allowed to access non-existent class") is logged server-side.
780-794: LGTM: Pointer access tests properly verify sanitized errors.Both tests (_PushStatus and _GlobalConfig) correctly validate that:
- Clients cannot access master-key-only classes through pointers
- Generic "Permission denied" message is returned to clients
- Detailed error messages (e.g., "Clients aren't allowed to perform the get operation") are logged server-side
This provides important security test coverage for indirect access attempts.
Also applies to: 798-813
957-963: LGTM: Logger setup for read-only masterKey tests.The block-level declaration of both
loggerErrorSpyandloggeris intentional to support the re-require pattern needed afterreconfigureServer()(see line 991-992). This is the correct approach for tests that reconfigure the server.
990-992: LGTM: Proper logger re-initialization after reconfigureServer.The code correctly re-requires the logger after
reconfigureServer()because the server reconfiguration resets the logger instance. The comment clearly explains this necessity, and the new spy is properly created for subsequent assertions. This pattern is essential for accurate log verification after server reconfiguration.
966-1172: LGTM: Comprehensive read-only masterKey test coverage.All read-only masterKey tests properly verify sanitized error handling across various operations:
- REST operations (create, update, delete)
- RestWrite instantiation
- Schema operations (create, update, delete)
- Config updates
- Push notifications
Each test consistently validates:
- Generic "Permission denied" message returned to clients
- Detailed error logged server-side (e.g., "read-only masterKey isn't allowed to...")
- Preserved error codes (Parse.Error.OPERATION_FORBIDDEN)
The test suite provides thorough coverage of all operations that should be blocked with a read-only master key.
3 participants
Pull Request
Issue
Fixes: #8958
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.