fix: Maintenance key IP mismatch silently downgrades to regular auth instead of rejecting#10391
Conversation
…instead of rejecting
|
🚀 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. |
📝 WalkthroughWalkthroughThe PR fixes an inconsistency where a valid maintenance key from a non-allowed IP address was silently downgraded to regular authentication instead of being rejected. The fix adds proper IP-based allowlist enforcement for maintenance keys, matching master key behavior by throwing a 403 error when the IP is not in the allowlist. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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/Middlewares.spec.js (2)
189-212: Consider consolidating with the existing test at lines 170-187.The test at lines 170-187 already covers the maintenance key IP rejection scenario with nearly identical setup. The key difference is that this new test also asserts
error.status === 403, which is valuable for verifying the HTTP status code.A few observations:
- The
reconfigureServer()call appears unnecessary sinceAppCachePutdirectly overrides the cache entry that the middleware reads from.- Consider adding the
error.statusassertion to the existing test at lines 170-187 instead of creating a new test, or remove the redundant test.If the intent is to have both tests, consider documenting why both are needed (e.g., different code paths being exercised).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/Middlewares.spec.js` around lines 189 - 212, The new test duplicates the existing maintenance-key-IP rejection scenario; remove redundancy by merging the useful assertion into the existing test: delete the new test block and add the error.status === 403 assertion and the logger.error expectation into the earlier test (which already uses AppCachePut and fakeReq headers), or if you prefer to keep both tests, add a comment explaining they exercise different code paths and remove the unnecessary reconfigureServer() call since AppCachePut already sets the cache; update the test that contains middlewares.handleParseHeaders, AppCachePut, fakeReq and logger.error to include the 403 status assertion so the behavior is covered without duplicate tests.
170-178: Minor: Consider renamingmaintenanceKeyvalue for clarity.The
maintenanceKeyconfig value is set to'masterKey'(line 174) which could cause confusion when reading the test. The new test at lines 196-202 uses'maintenanceKey'as the value, which is clearer.Proposed fix for clarity
it('should not succeed and log if the ip does not belong to maintenanceKeyIps list', async () => { const logger = require('../lib/logger').logger; spyOn(logger, 'error').and.callFake(() => {}); AppCachePut(fakeReq.body._ApplicationId, { - maintenanceKey: 'masterKey', + maintenanceKey: 'maintenanceKey', maintenanceKeyIps: ['10.0.0.0', '10.0.0.1'], }); fakeReq.ip = '10.0.0.2'; - fakeReq.headers['x-parse-maintenance-key'] = 'masterKey'; + fakeReq.headers['x-parse-maintenance-key'] = 'maintenanceKey';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/Middlewares.spec.js` around lines 170 - 178, Rename the maintenance key literal used in the test setup for clarity: when calling AppCachePut to seed the config (function AppCachePut), change the value for maintenanceKey from 'masterKey' to 'maintenanceKey' so it matches the clearer naming used elsewhere and avoid confusion with the header value; ensure fakeReq.headers['x-parse-maintenance-key'] and any assertions still reference the same value and update any related test expectations if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@spec/Middlewares.spec.js`:
- Around line 189-212: The new test duplicates the existing maintenance-key-IP
rejection scenario; remove redundancy by merging the useful assertion into the
existing test: delete the new test block and add the error.status === 403
assertion and the logger.error expectation into the earlier test (which already
uses AppCachePut and fakeReq headers), or if you prefer to keep both tests, add
a comment explaining they exercise different code paths and remove the
unnecessary reconfigureServer() call since AppCachePut already sets the cache;
update the test that contains middlewares.handleParseHeaders, AppCachePut,
fakeReq and logger.error to include the 403 status assertion so the behavior is
covered without duplicate tests.
- Around line 170-178: Rename the maintenance key literal used in the test setup
for clarity: when calling AppCachePut to seed the config (function AppCachePut),
change the value for maintenanceKey from 'masterKey' to 'maintenanceKey' so it
matches the clearer naming used elsewhere and avoid confusion with the header
value; ensure fakeReq.headers['x-parse-maintenance-key'] and any assertions
still reference the same value and update any related test expectations if
needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1cf34ebf-83ac-4a75-8898-77f0ddfa99c8
📒 Files selected for processing (2)
spec/Middlewares.spec.jssrc/middlewares.js
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10391 +/- ##
==========================================
- Coverage 92.52% 92.51% -0.02%
==========================================
Files 192 192
Lines 16620 16624 +4
Branches 229 229
==========================================
+ Hits 15377 15379 +2
- Misses 1221 1223 +2
Partials 22 22 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# [9.8.0-alpha.2](9.8.0-alpha.1...9.8.0-alpha.2) (2026-04-03) ### Bug Fixes * Maintenance key IP mismatch silently downgrades to regular auth instead of rejecting ([#10391](#10391)) ([7d8b367](7d8b367))
|
🎉 This change has been released in version 9.8.0-alpha.2 |
2 participants
Issue
Closes #10390
When a valid maintenance key is sent from a non-allowed IP address, the request is silently downgraded to regular user auth instead of being rejected with 403. This is inconsistent with master key and read-only master key behavior, which both reject with 403 on IP mismatch.
Tasks
Summary by CodeRabbit