fix: _Installation update crashes or orphans row when clearing installationId#10455
fix: _Installation update crashes or orphans row when clearing installationId#10455AdrianCurtin wants to merge 1 commit intoparse-community:alphafrom
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. |
📝 WalkthroughWalkthroughThe PR adds protection to prevent the ChangesInstallation ID Protection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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.20.0)OpenGrep fatal error (exit code 2): [00.20][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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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.
| const clearingInstallationId = | ||
| this.data.installationId === null || | ||
| (typeof this.data.installationId === 'object' && | ||
| this.data.installationId !== null && | ||
| this.data.installationId.__op === 'Delete'); |
![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
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