Repository navigation
Potential fix for code scanning alert no. 367: Missing rate limiting - #85
mmeerrkkaa wants to merge 1 commit into
Conversation
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (1)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThe Sequence diagram for rate-limited panel update applicationsequenceDiagram
actor Client
participant PanelUpdate as PanelUpdateEndpoint
participant Limiter as applyLimiter
participant Auth as authenticateUniversal
participant Authorize as authorize
participant Service as PanelUpdateService
Client->>PanelUpdate: POST /apply
PanelUpdate->>Limiter: applyLimiter
alt rate limit exceeded
Limiter-->>Client: Reject request
else request allowed
Limiter->>Auth: authenticateUniversal
Auth->>Authorize: authorize(panel:settings:edit)
Authorize->>Service: applyUpdate()
Service-->>Client: Update result
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
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="39" />
<code_context>
});
-router.post('/apply', authenticateUniversal, authorize('panel:settings:edit'), applyLimiter, async (req, res) => {
+router.post('/apply', applyLimiter, authenticateUniversal, authorize('panel:settings:edit'), async (req, res) => {
try {
const result = await PanelUpdateService.applyUpdate();
</code_context>
<issue_to_address>
**Administrators get blocked from updates**
When an unauthenticated client sends 20 POSTs from the same observed IP within five minutes before an administrator’s request, `applyLimiter` charges the IP-based quota before `authenticateUniversal` runs, so those requests exhaust the bucket and the administrator’s update request receives 429.
Run `authenticateUniversal` and `authorize` before `applyLimiter`.
Also at `backend/src/api/routes/panelUpdate.js:40`.
</issue_to_address>| }); | ||
|
|
||
| router.post('/apply', authenticateUniversal, authorize('panel:settings:edit'), applyLimiter, async (req, res) => { | ||
| router.post('/apply', applyLimiter, authenticateUniversal, authorize('panel:settings:edit'), async (req, res) => { |
There was a problem hiding this comment.
🟡 Medium · Administrators get blocked from updates
When an unauthenticated client sends 20 POSTs from the same observed IP within five minutes before an administrator’s request, applyLimiter charges the IP-based quota before authenticateUniversal runs, so those requests exhaust the bucket and the administrator’s update request receives 429.
Run authenticateUniversal and authorize before applyLimiter.
Also at backend/src/api/routes/panelUpdate.js:40.
Prompt for AI agents
In `backend/src/api/routes/panelUpdate.js` at line 39:
**Administrators get blocked from updates**
When an unauthenticated client sends 20 POSTs from the same observed IP within five minutes before an administrator’s request, `applyLimiter` charges the IP-based quota before `authenticateUniversal` runs, so those requests exhaust the bucket and the administrator’s update request receives 429.
Run `authenticateUniversal` and `authorize` before `applyLimiter`.
Also at `backend/src/api/routes/panelUpdate.js:40`.There was a problem hiding this comment.
конкретнее. Если на одном айпи есть аккаунты админа и юзера7
There was a problem hiding this comment.
🟡 Medium · Администраторы не могут получать обновления
Когда неаутентифицированный клиент отправляет 20 POST-запросов с одного и того же наблюдаемого IP-адреса в течение пяти минут до запроса администратора,
applyLimiterэто приводит к исчерпанию квоты, основанной на IP-адресеauthenticateUniversal, и, следовательно, к исчерпанию лимита запросов, а запрос на обновление от администратора получает ошибку 429.Запустите
authenticateUniversalиauthorizeперед этимapplyLimiter.Также по адресу
backend/src/api/routes/panelUpdate.js:40.Запрос для агентов ИИ
конкретнее. Если на одном айпи есть аккаунты админа и юзера7
There was a problem hiding this comment.
Да. При текущем порядке (applyLimiter → authenticateUniversal → authorize) лимитер, судя по конфигурации, учитывает IP, а не аккаунт. Поэтому если администратор и user7 находятся за одним IP, их запросы /apply используют общий лимит в 20 запросов за 5 минут:
- 20 запросов
user7исчерпают общий bucket; - следующий запрос администратора с того же IP получит
429; - это произойдёт даже если у администратора есть право
panel:settings:edit.
Перестановка authenticateUniversal перед applyLimiter не разделит лимит между администратором и user7; она лишь не позволит неаутентифицированным запросам расходовать этот IP-бюджет. Чтобы аккаунты с одним IP не блокировали друг друга, лимитер должен использовать ключ, основанный на аутентифицированном пользователе или API-ключе, а не только на IP.
There was a problem hiding this comment.
Если администратор и user7 находятся за одним IP, то в текущем варианте applyLimiter они используют один общий IP-бакет:
- первые 20
POST /applyза 5 минут — независимо от того, кто их отправил; - 21-й запрос, включая запрос администратора, получит
429; - неавторизованные запросы также расходуют этот же лимит, потому что
applyLimiterвыполняется первым.
Если поставить authenticateUniversal и authorize перед applyLimiter, запросы без авторизации или без права panel:settings:edit будут отклоняться до rate limiter и не смогут исчерпать его лимит. Но два авторизованных аккаунта с одного IP всё равно будут делить общий IP-лимит. Для раздельного лимитирования по аккаунтам потребовался бы отдельный ключ на основе идентификатора пользователя.
1 participant
Potential fix for https://github.com/blockmineJS/blockmine/security/code-scanning/367
To fix missing rate limiting without changing functionality, keep the existing
applyLimiterconfiguration but apply it beforeauthenticateUniversalandauthorize(...)in the/applyroute middleware chain.Best single fix in this file:
backend/src/api/routes/panelUpdate.js, update the/applyroute declaration soapplyLimiteris the first middleware argument after the path.Suggested fixes powered by Copilot Autofix. Review carefully before merging.
Summary by Sourcery
Bug Fixes:
Summary by CodeRabbit