fix: _Installation update crashes or orphans row when clearing installationId#10455
Open
AdrianCurtin wants to merge 3 commits into
Open
fix: _Installation update crashes or orphans row when clearing installationId#10455AdrianCurtin wants to merge 3 commits into
AdrianCurtin wants to merge 3 commits into
Conversation
…lationId
handleInstallation runs before Parse __op operators are processed. Two
latent bugs in the installationId path:
- { installationId: { __op: 'Delete' } }: the operator object hits
this.data.installationId.toLowerCase() and throws an uncaught
TypeError, returning 500 to the client.
- { installationId: null }: silently nullifies the row's primary
identifier, orphaning it (no client SDK using X-Parse-Installation-Id
can ever find it again).
installationId is the Installation row's identity, used by the SDK auth
header to bind a request to its row. The codebase already throws
Parse error 136 ("installationId may not be changed in this operation")
on any change between values; clearing is the strongest form of
changing and is rejected for the same reason — even with master key,
matching the existing unconditional 136 guard.
Detect both clearing shapes at the top of handleInstallation. On
update (this.query set), throw 136. On create, drop the value so the
existing 135 "must specify ID" guard fires cleanly. Setting an
installationId for the first time on a row that lacks one, and creating
a row with only deviceToken, both remain unaffected.
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDetects attempts to clear ChangesInstallation ID Protection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 6 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. 🔧 OpenGrep (1.21.0)OpenGrep fatal error (exit code 2): [00.15][ERROR]: Error: exception Unix_error: No such file or directory stat spec/ParseInstallation.spec.js Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR hardens _Installation handling in RestWrite.handleInstallation to prevent invalid attempts to clear installationId (via null or { __op: 'Delete' }) from causing a server crash or orphaning an installation row. It aligns “clear” behavior with the existing protection that disallows changing installationId during updates, and adds regression tests for these scenarios.
Changes:
- Add an early guard in
handleInstallationto detectinstallationIdclearing attempts and reject them on update (136) or drop the field on create so the existing “must specify ID” check returns 135. - Add spec coverage for update + create clearing shapes, including a master-key update case.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/RestWrite.js | Adds early detection of installationId clearing to avoid crashes/orphaned rows and to return consistent Parse error codes. |
| spec/ParseInstallation.spec.js | Adds regression tests covering clearing installationId via null and Delete op on create/update paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
spec/ParseInstallation.spec.js (1)
639-666: ⚡ Quick winConsider adding a
null-variant master-key test to complete the coverage matrixThe master-key test covers only
{ __op: 'Delete' }. Adding a parallel test forinstallationId: nullwould verify the full matrix (both clearing shapes × with/without master key), at the cost of ~15 lines. Both shapes share the same post-predicate code path, so this is low-risk but adds documentation value.➕ Suggested additional test
it('master key cannot clear installationId via null', done => { const installId = '12345678-abcd-abcd-abcd-123456789abc'; const input = { installationId: installId, deviceType: 'ios', }; rest .create(config, auth.master(config), '_Installation', input) .then(() => database.adapter.find('_Installation', installationSchema, {}, {})) .then(results => { expect(results.length).toEqual(1); return rest.update( config, auth.master(config), '_Installation', { objectId: results[0].objectId }, { installationId: null } ); }) .then(() => { fail('Master key clearing of installationId via null should have been rejected.'); done(); }) .catch(error => { expect(error.code).toEqual(136); done(); }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/ParseInstallation.spec.js` around lines 639 - 666, Add a sibling test to the existing "master key cannot clear installationId" case that verifies clearing via null is also rejected; reproduce the same setup used there (use rest.create with auth.master(config) to create an _Installation with installationId and deviceType, then find the created row with database.adapter.find or reuse the existing promise chain) and call rest.update with auth.master(config) where the update body sets installationId: null, then assert the update is rejected with error.code === 136 (mirroring the existing .then/.catch flow and failure message). Ensure you reference the same helpers used in the diff (rest.create, rest.update, auth.master, installationId) and mirror the structure of the original test so both delete-op and null-variant are covered.src/RestWrite.js (1)
1313-1316: 💤 Low valueOptional: clarify the create-path comment — error 135 isn't always guaranteed
The comment says the Delete-guard fires "so the existing 135 error fires," but 135 only fires when neither
deviceTokennorauth.installationIdprovides an alternative ID. When another ID is present the create legitimately succeeds. This is the correct behavior, but the comment could mislead a future maintainer into expecting an unconditional 135 rejection.✏️ Suggested comment wording
- // Create path: drop the operator/null so the "must specify ID" - // guard below fires with the correct 135 error. - delete this.data.installationId; + // Create path: remove the invalid value so the existing "at least + // one ID field must be specified" guard (error 135) fires when no + // other ID (deviceToken, auth.installationId) is available. + delete this.data.installationId;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/RestWrite.js` around lines 1313 - 1316, The comment above the delete this.data.installationId line is misleading about error 135 being guaranteed; update the comment to clarify that deleting installationId is to let the "must specify ID" guard trigger when no alternative ID is supplied, but that error 135 only occurs when neither deviceToken nor auth.installationId (nor any other valid ID) is present — if another ID exists the create will validly succeed. Mention the relevant symbols: delete this.data.installationId, deviceToken, auth.installationId, and error 135 so future maintainers understand the conditional behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@spec/ParseInstallation.spec.js`:
- Around line 639-666: Add a sibling test to the existing "master key cannot
clear installationId" case that verifies clearing via null is also rejected;
reproduce the same setup used there (use rest.create with auth.master(config) to
create an _Installation with installationId and deviceType, then find the
created row with database.adapter.find or reuse the existing promise chain) and
call rest.update with auth.master(config) where the update body sets
installationId: null, then assert the update is rejected with error.code === 136
(mirroring the existing .then/.catch flow and failure message). Ensure you
reference the same helpers used in the diff (rest.create, rest.update,
auth.master, installationId) and mirror the structure of the original test so
both delete-op and null-variant are covered.
In `@src/RestWrite.js`:
- Around line 1313-1316: The comment above the delete this.data.installationId
line is misleading about error 135 being guaranteed; update the comment to
clarify that deleting installationId is to let the "must specify ID" guard
trigger when no alternative ID is supplied, but that error 135 only occurs when
neither deviceToken nor auth.installationId (nor any other valid ID) is present
— if another ID exists the create will validly succeed. Mention the relevant
symbols: delete this.data.installationId, deviceToken, auth.installationId, and
error 135 so future maintainers understand the conditional behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 969a37bd-f3a5-489d-9950-6fca6b7620fc
📒 Files selected for processing (2)
spec/ParseInstallation.spec.jssrc/RestWrite.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/RestWrite.js (1)
1303-1307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard non-string
installationIdbefore lowercasing.At Line 1337, unsupported object payloads (anything truthy other than
{ __op: 'Delete' }) can still hit.toLowerCase()and throw a 500. Please reject non-stringinstallationIdvalues explicitly in this block so malformed payloads fail with a Parse error instead of a TypeError.Suggested patch
const clearingInstallationId = this.data.installationId === null || (typeof this.data.installationId === 'object' && this.data.installationId !== null && this.data.installationId.__op === 'Delete'); + const hasInstallationId = + Object.prototype.hasOwnProperty.call(this.data, 'installationId'); + if ( + hasInstallationId && + this.data.installationId !== null && + this.data.installationId !== undefined && + typeof this.data.installationId !== 'string' && + !clearingInstallationId + ) { + throw new Parse.Error(Parse.Error.INVALID_JSON, 'installationId must be a string'); + } if (clearingInstallationId) {Also applies to: 1336-1337
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/RestWrite.js` around lines 1303 - 1307, The logic computing clearingInstallationId (using this.data.installationId and later calling .toLowerCase()) must explicitly guard against non-string installationId payloads: before any .toLowerCase() call, check typeof this.data.installationId === 'string'; if it's an object that equals { __op: 'Delete' } treat as the delete case, but for any other non-string/unsupported truthy value reject by throwing a Parse error (via the same error class used elsewhere) so malformed installationId values produce a Parse error instead of allowing a TypeError; update the code paths that reference clearingInstallationId and any subsequent .toLowerCase() usage to rely on this validated string-only branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/RestWrite.js`:
- Around line 1303-1307: The logic computing clearingInstallationId (using
this.data.installationId and later calling .toLowerCase()) must explicitly guard
against non-string installationId payloads: before any .toLowerCase() call,
check typeof this.data.installationId === 'string'; if it's an object that
equals { __op: 'Delete' } treat as the delete case, but for any other
non-string/unsupported truthy value reject by throwing a Parse error (via the
same error class used elsewhere) so malformed installationId values produce a
Parse error instead of allowing a TypeError; update the code paths that
reference clearingInstallationId and any subsequent .toLowerCase() usage to rely
on this validated string-only branch.
Prevent non-string installationId values from causing a server error by validating the type in RestWrite: remove invalid installationId on create so the existing "must specify ID" check runs, and throw Parse.Error.INVALID_JSON when installationId exists but is not a string (avoids crashes from .toLowerCase()). Add unit tests to cover create/update failures for non-string installationId inputs and ensure master key cannot clear installationId via null.
Author
|
Current behavior blocks master key editing of installationId so it follows that we should also block it from being nulled, but I do think there are circumstances in which it would be valid to clear an installationId (maintenance etc) |
| this.data.installationId.__op === 'Delete'); | ||
| if (clearingInstallationId) { | ||
| if (this.query) { | ||
| throw new Parse.Error(136, 'installationId may not be changed in this operation'); |
| }); | ||
| }); | ||
|
|
||
| it('update fails to clear installationId via Delete op', done => { |
| }); | ||
| }); | ||
|
|
||
| it('update fails to clear installationId via null', done => { |
Comment on lines
+547
to
+636
| it('update fails to clear installationId via Delete op', done => { | ||
| const installId = '12345678-abcd-abcd-abcd-123456789abc'; | ||
| const input = { | ||
| installationId: installId, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => database.adapter.find('_Installation', installationSchema, {}, {})) | ||
| .then(results => { | ||
| expect(results.length).toEqual(1); | ||
| return rest.update( | ||
| config, | ||
| auth.nobody(config), | ||
| '_Installation', | ||
| { objectId: results[0].objectId }, | ||
| { installationId: { __op: 'Delete' } } | ||
| ); | ||
| }) | ||
| .then(() => { | ||
| fail('Updating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(136); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('update fails to clear installationId via null', done => { | ||
| const installId = '12345678-abcd-abcd-abcd-123456789abc'; | ||
| const input = { | ||
| installationId: installId, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => database.adapter.find('_Installation', installationSchema, {}, {})) | ||
| .then(results => { | ||
| expect(results.length).toEqual(1); | ||
| return rest.update( | ||
| config, | ||
| auth.nobody(config), | ||
| '_Installation', | ||
| { objectId: results[0].objectId }, | ||
| { installationId: null } | ||
| ); | ||
| }) | ||
| .then(() => { | ||
| fail('Updating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(136); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('create fails when installationId is the Delete op (no real ID provided)', done => { | ||
| const input = { | ||
| installationId: { __op: 'Delete' }, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => { | ||
| fail('Creating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(135); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('create fails when installationId is null (no real ID provided)', done => { | ||
| const input = { | ||
| installationId: null, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => { | ||
| fail('Creating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(135); | ||
| done(); | ||
| }); |
| }); | ||
| }); | ||
|
|
||
| it('create fails when installationId is null (no real ID provided)', done => { |
Comment on lines
+605
to
+684
| it('create fails when installationId is the Delete op (no real ID provided)', done => { | ||
| const input = { | ||
| installationId: { __op: 'Delete' }, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => { | ||
| fail('Creating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(135); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('create fails when installationId is null (no real ID provided)', done => { | ||
| const input = { | ||
| installationId: null, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => { | ||
| fail('Creating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(135); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('create fails when installationId is a non-string object', done => { | ||
| const input = { | ||
| installationId: { foo: 'bar' }, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => { | ||
| fail('Creating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(Parse.Error.INVALID_JSON); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('update fails when installationId is a non-string object', done => { | ||
| const installId = '12345678-abcd-abcd-abcd-123456789abc'; | ||
| const input = { | ||
| installationId: installId, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => database.adapter.find('_Installation', installationSchema, {}, {})) | ||
| .then(results => { | ||
| expect(results.length).toEqual(1); | ||
| return rest.update( | ||
| config, | ||
| auth.nobody(config), | ||
| '_Installation', | ||
| { objectId: results[0].objectId }, | ||
| { installationId: { foo: 'bar' } } | ||
| ); | ||
| }) | ||
| .then(() => { | ||
| fail('Updating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(Parse.Error.INVALID_JSON); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
| }); | ||
| }); | ||
|
|
||
| it('update fails when installationId is a non-string object', done => { |
Comment on lines
+622
to
+711
| it('create fails when installationId is null (no real ID provided)', done => { | ||
| const input = { | ||
| installationId: null, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => { | ||
| fail('Creating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(135); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('create fails when installationId is a non-string object', done => { | ||
| const input = { | ||
| installationId: { foo: 'bar' }, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => { | ||
| fail('Creating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(Parse.Error.INVALID_JSON); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('update fails when installationId is a non-string object', done => { | ||
| const installId = '12345678-abcd-abcd-abcd-123456789abc'; | ||
| const input = { | ||
| installationId: installId, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => database.adapter.find('_Installation', installationSchema, {}, {})) | ||
| .then(results => { | ||
| expect(results.length).toEqual(1); | ||
| return rest.update( | ||
| config, | ||
| auth.nobody(config), | ||
| '_Installation', | ||
| { objectId: results[0].objectId }, | ||
| { installationId: { foo: 'bar' } } | ||
| ); | ||
| }) | ||
| .then(() => { | ||
| fail('Updating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(Parse.Error.INVALID_JSON); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('master key cannot clear installationId via Delete op', done => { | ||
| const installId = '12345678-abcd-abcd-abcd-123456789abc'; | ||
| const input = { | ||
| installationId: installId, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.master(config), '_Installation', input) | ||
| .then(() => database.adapter.find('_Installation', installationSchema, {}, {})) | ||
| .then(results => { | ||
| expect(results.length).toEqual(1); | ||
| return rest.update( | ||
| config, | ||
| auth.master(config), | ||
| '_Installation', | ||
| { objectId: results[0].objectId }, | ||
| { installationId: { __op: 'Delete' } } | ||
| ); | ||
| }) | ||
| .then(() => { | ||
| fail('Master key clearing of installationId should have been rejected.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(136); | ||
| done(); | ||
| }); |
Comment on lines
+656
to
+742
| it('update fails when installationId is a non-string object', done => { | ||
| const installId = '12345678-abcd-abcd-abcd-123456789abc'; | ||
| const input = { | ||
| installationId: installId, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.nobody(config), '_Installation', input) | ||
| .then(() => database.adapter.find('_Installation', installationSchema, {}, {})) | ||
| .then(results => { | ||
| expect(results.length).toEqual(1); | ||
| return rest.update( | ||
| config, | ||
| auth.nobody(config), | ||
| '_Installation', | ||
| { objectId: results[0].objectId }, | ||
| { installationId: { foo: 'bar' } } | ||
| ); | ||
| }) | ||
| .then(() => { | ||
| fail('Updating the installation should have failed.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(Parse.Error.INVALID_JSON); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('master key cannot clear installationId via Delete op', done => { | ||
| const installId = '12345678-abcd-abcd-abcd-123456789abc'; | ||
| const input = { | ||
| installationId: installId, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.master(config), '_Installation', input) | ||
| .then(() => database.adapter.find('_Installation', installationSchema, {}, {})) | ||
| .then(results => { | ||
| expect(results.length).toEqual(1); | ||
| return rest.update( | ||
| config, | ||
| auth.master(config), | ||
| '_Installation', | ||
| { objectId: results[0].objectId }, | ||
| { installationId: { __op: 'Delete' } } | ||
| ); | ||
| }) | ||
| .then(() => { | ||
| fail('Master key clearing of installationId should have been rejected.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(136); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
||
| it('master key cannot clear installationId via null', done => { | ||
| const installId = '12345678-abcd-abcd-abcd-123456789abc'; | ||
| const input = { | ||
| installationId: installId, | ||
| deviceType: 'ios', | ||
| }; | ||
| rest | ||
| .create(config, auth.master(config), '_Installation', input) | ||
| .then(() => database.adapter.find('_Installation', installationSchema, {}, {})) | ||
| .then(results => { | ||
| expect(results.length).toEqual(1); | ||
| return rest.update( | ||
| config, | ||
| auth.master(config), | ||
| '_Installation', | ||
| { objectId: results[0].objectId }, | ||
| { installationId: null } | ||
| ); | ||
| }) | ||
| .then(() => { | ||
| fail('Master key clearing of installationId via null should have been rejected.'); | ||
| done(); | ||
| }) | ||
| .catch(error => { | ||
| expect(error.code).toEqual(136); | ||
| done(); | ||
| }); | ||
| }); | ||
|
|
Comment on lines
+1322
to
+1327
| if ( | ||
| this.data.installationId !== undefined && | ||
| typeof this.data.installationId !== 'string' | ||
| ) { | ||
| throw new Parse.Error(Parse.Error.INVALID_JSON, 'installationId must be a string'); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue
RestWrite.handleInstallationruns before Parse__opoperators are processed. Two latent bugs in theinstallationIdpath:{ installationId: { __op: 'Delete' } }—this.data.installationId.toLowerCase()is called on the operator object, throws an uncaughtTypeError, server returns 500 to the client.{ installationId: null }— falls through the lowercase guard, silently nullifies the row's primary identifier and orphans it (no client SDK usingX-Parse-Installation-Idcan ever find it again).installationIdis the_Installationrow's identity — the SDK auth header (X-Parse-Installation-Id, surfaced asthis.auth.installationId) binds a non-master client request to its row. The codebase already throws Parse error 136 (installationId may not be changed in this operation) on any change ofinstallationIdbetween values. Clearing is the strongest form of changing and should be rejected for the same reason.Approach
Detect both clearing shapes at the top of
handleInstallation:this.queryset), throw Parse error 136 with the existing message — unconditional, including under master key, matching the existing change-guard at the post-fix equivalent of line 1418.delete this.data.installationIdso the existing 135'at least one ID field ... must be specified'guard fires cleanly.Setting an installationId for the first time on a row that lacks one, and creating a row with only
deviceToken, are both unaffected — the detector ignores string values, and the existing 136 guard at line 1418 only fires when both old and new are non-empty and differ.Tasks
Add changes to documentation (guides, repository pages, code comments)Add security checkAdd new Parse Error codes to Parse JS SDKSummary by CodeRabbit
Bug Fixes
Tests