Repository navigation
Potential fix for code scanning alert no. 366: Missing rate limiting - #86
mmeerrkkaa wants to merge 2 commits into
Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (1)
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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe panel update routes now consistently invoke their existing rate limiters before authentication, including the previously unprotected Flow diagram for rate-limited panel update routesflowchart TD
Check[GET /check] --> CheckLimiter[checkLimiter]
Status[GET /status] --> StatusLimiter[checkLimiter]
Apply[POST /apply] --> ApplyLimiter[applyLimiter]
CheckLimiter --> CheckAuth[authenticateUniversal]
StatusLimiter --> StatusAuth[authenticateUniversal]
ApplyLimiter --> ApplyAuth[authenticateUniversal]
CheckAuth --> CheckHandler[PanelUpdateService.checkForUpdate]
StatusAuth --> StatusHandler[PanelUpdateService.getProgress]
ApplyAuth --> Authorize[authorize]
Authorize --> ApplyHandler[PanelUpdateService.applyUpdate]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="backend/src/api/routes/panelUpdate.js" line_range="24" />
<code_context>
});
-router.get('/check', authenticateUniversal, checkLimiter, async (req, res) => {
+router.get('/check', checkLimiter, authenticateUniversal, async (req, res) => {
try {
const fresh = req.query.fresh === '1' || req.query.fresh === 'true';
</code_context>
<issue_to_address>
**Unauthenticated traffic blocks panel users**
When unauthenticated requests share a client IP with an authorized user and exhaust a route quota, `checkLimiter` and `applyLimiter` run before authentication or authorization and count by IP, so unauthenticated requests consume the authorized caller’s quota; subsequent `/check`, `/status`, or `/apply` requests receive 429 responses.
Run each authenticated-user limiter after authentication (and authorization for `/apply`), or keep pre-authentication IP limiting in a separate bucket.
Also at `backend/src/api/routes/panelUpdate.js:25`, `backend/src/api/routes/panelUpdate.js:35-36`, `backend/src/api/routes/panelUpdate.js:39-40`.
</issue_to_address>
### Comment 2
<location path="backend/src/api/routes/panelUpdate.js" line_range="35" />
<code_context>
});
-router.get('/status', authenticateUniversal, (req, res) => {
+router.get('/status', checkLimiter, authenticateUniversal, (req, res) => {
res.json(PanelUpdateService.getProgress());
});
</code_context>
<issue_to_address>
**Update progress stops refreshing**
When an update remains active until the shared 40-request quota is exhausted, including when earlier checks used some of the quota, `checkLimiter` rejects `/status` polls with 429, and `pollStatus` ignores non-OK responses, so the dialog’s displayed progress stays stale while the update continues.
Give `/status` polling a separate, suitably sized limit or exempt it from the `/check` limiter.
Also at `backend/src/api/routes/panelUpdate.js:36`.
</issue_to_address>| }); | ||
|
|
||
| router.get('/check', authenticateUniversal, checkLimiter, async (req, res) => { | ||
| router.get('/check', checkLimiter, authenticateUniversal, async (req, res) => { |
There was a problem hiding this comment.
🟡 Medium · Unauthenticated traffic blocks panel users
When unauthenticated requests share a client IP with an authorized user and exhaust a route quota, checkLimiter and applyLimiter run before authentication or authorization and count by IP, so unauthenticated requests consume the authorized caller’s quota; subsequent /check, /status, or /apply requests receive 429 responses.
Run each authenticated-user limiter after authentication (and authorization for /apply), or keep pre-authentication IP limiting in a separate bucket.
Also at backend/src/api/routes/panelUpdate.js:25, backend/src/api/routes/panelUpdate.js:35-36, backend/src/api/routes/panelUpdate.js:39-40.
Prompt for AI agents
In `backend/src/api/routes/panelUpdate.js` at line 24:
**Unauthenticated traffic blocks panel users**
When unauthenticated requests share a client IP with an authorized user and exhaust a route quota, `checkLimiter` and `applyLimiter` run before authentication or authorization and count by IP, so unauthenticated requests consume the authorized caller’s quota; subsequent `/check`, `/status`, or `/apply` requests receive 429 responses.
Run each authenticated-user limiter after authentication (and authorization for `/apply`), or keep pre-authentication IP limiting in a separate bucket.
Also at `backend/src/api/routes/panelUpdate.js:25`, `backend/src/api/routes/panelUpdate.js:35-36`, `backend/src/api/routes/panelUpdate.js:39-40`.| }); | ||
|
|
||
| router.get('/status', authenticateUniversal, (req, res) => { | ||
| router.get('/status', checkLimiter, authenticateUniversal, (req, res) => { |
There was a problem hiding this comment.
🟠 High · Update progress stops refreshing
When an update remains active until the shared 40-request quota is exhausted, including when earlier checks used some of the quota, checkLimiter rejects /status polls with 429, and pollStatus ignores non-OK responses, so the dialog’s displayed progress stays stale while the update continues.
Give /status polling a separate, suitably sized limit or exempt it from the /check limiter.
Also at backend/src/api/routes/panelUpdate.js:36.
Prompt for AI agents
In `backend/src/api/routes/panelUpdate.js` at line 35:
**Update progress stops refreshing**
When an update remains active until the shared 40-request quota is exhausted, including when earlier checks used some of the quota, `checkLimiter` rejects `/status` polls with 429, and `pollStatus` ignores non-OK responses, so the dialog’s displayed progress stays stale while the update continues.
Give `/status` polling a separate, suitably sized limit or exempt it from the `/check` limiter.
Also at `backend/src/api/routes/panelUpdate.js:36`.
Potential fix for https://github.com/blockmineJS/blockmine/security/code-scanning/366
To fix this cleanly without changing existing functionality, attach a rate-limiting middleware to the
/statusroute, just like the other routes in this file.Best single change in this snippet:
router.get('/status', ...)to includecheckLimiter(already defined and suitable for read/check-style operations).express-rate-limitis already used and both limiters are already defined.Specific edit region:
backend/src/api/routes/panelUpdate.js/statusendpoint)This addresses all alert variants at the same location by ensuring the authenticated
/statushandler is rate-limited.Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by Sourcery
Protect panel update endpoints with consistent rate-limiting middleware while preserving their existing behavior.
Bug Fixes:
Enhancements: