security: prevent cross-instance auth bypass via query/body override (CVE pending, #2435)#2549
Merged
Merged
Conversation
…2435) The auth guard in src/api/guards/auth.guard.ts validates instance ownership using req.params.instanceName. The abstract router then merged req.query (and, on /instance/create, req.body) into the instance object via Object.assign — which silently overwrote the already-authenticated instanceName. An attacker with one valid token could send: GET /chat/findMessages/MY_INSTANCE?instanceName=VICTIM_INSTANCE Auth passed for MY_INSTANCE, but dataValidate() then replaced the instance with VICTIM_INSTANCE before execute() ran — giving the caller full access to read/send messages, modify settings, and delete other tenants' instances. CWE-639: Authorization Bypass Through User-Controlled Key Fix: introduce sanitizeUntrustedInput() that strips PROTECTED_INSTANCE_FIELDS (instanceName, instanceId) from any untrusted source before merging into the instance object. Logs a warning on attempts so abuse is auditable. Closes #2435 Reported by @lighthousekeeper1212 via static analysis. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-push lint flagged whatsapp.baileys.service.ts:531 (double blank line after the stream:error 515 fix merged via #2509). Trivial prettier fix. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds input sanitization in the abstract router to prevent cross-instance authorization bypass by blocking overrides of protected instance fields from query/body, and includes a minor formatting cleanup in the WhatsApp Baileys service. Sequence diagram for sanitized instance merging in dataValidatesequenceDiagram
actor Client
participant AuthGuard
participant RouterBroker
participant sanitizeUntrustedInput
participant Controller
Client->>AuthGuard: validate(instanceName from params)
AuthGuard-->>Client: authorized
Client->>RouterBroker: dataValidate(request, body)
activate RouterBroker
RouterBroker->>RouterBroker: instance = request.params
alt request.query present
RouterBroker->>sanitizeUntrustedInput: sanitizeUntrustedInput(request.query)
sanitizeUntrustedInput-->>RouterBroker: sanitizedQuery (no instanceName, instanceId)
RouterBroker->>RouterBroker: Object.assign(instance, sanitizedQuery)
end
alt request.originalUrl includes /instance/create
RouterBroker->>sanitizeUntrustedInput: sanitizeUntrustedInput(body)
sanitizeUntrustedInput-->>RouterBroker: sanitizedBody (no instanceName, instanceId)
RouterBroker->>RouterBroker: Object.assign(instance, sanitizedBody)
end
RouterBroker->>Controller: execute(instance, ref, body)
deactivate RouterBroker
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider centralizing the
PROTECTED_INSTANCE_FIELDSin a more globally shared config or type definition so any future places that need to protect instance identifiers can reuse the same list without duplicating constants. - The
sanitizeUntrustedInputhelper currently acceptsRecord<string, any>and returns a broadRecord<string, any>; you could tighten its typing (e.g., using generics) so callers preserve stronger types while still getting the protected-field filtering.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider centralizing the `PROTECTED_INSTANCE_FIELDS` in a more globally shared config or type definition so any future places that need to protect instance identifiers can reuse the same list without duplicating constants.
- The `sanitizeUntrustedInput` helper currently accepts `Record<string, any>` and returns a broad `Record<string, any>`; you could tighten its typing (e.g., using generics) so callers preserve stronger types while still getting the protected-field filtering.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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.
Resumo
Corrige a vulnerabilidade reportada em #2435 — CWE-639: Authorization Bypass Through User-Controlled Key — que permite que qualquer dono de instância autenticado opere sobre qualquer outra instância da mesma instalação Evolution API.
Vetor de ataque
auth.guard.tsvalidareq.params.instanceName === MY_INSTANCE→ ✅ passaabstract.router.tsdataValidate()fazObject.assign(instance, req.query)→instance.instanceNameagora éVICTIM_INSTANCEexecute(instance, ...)roda contra a instância da vítimaAfetava todos os endpoints que usam
dataValidate()comparam=true(basicamente todo o app: instance, message, chat, group, integrations).Fix
Introduz
sanitizeUntrustedInput()emsrc/api/abstract/abstract.router.tsque filtraPROTECTED_INSTANCE_FIELDS = ['instanceName', 'instanceId']de qualquer fonte não-confiável antes do merge no objetoinstance. Loga warning em tentativas para auditoria.Aplicado nos dois pontos vulneráveis:
Object.assign(instance, request.query)→ sanitiza queryObject.assign(instance, body)(no fluxo/instance/create) → sanitiza bodyAuditoria adicional
Busquei outros padrões `Object.assign(.*request)` no projeto. Os outros métodos do RouterBroker (
groupNoValidate,groupValidate,inviteCodeValidate,getParticipantsValidate) atribuem no objetobody/ref(validado pelo schema), não noinstance, então não compartilham o bug.Diff
src/api/abstract/abstract.router.ts: +17 / -2 (fix do CVE)src/api/integrations/channel/whatsapp/whatsapp.baileys.service.ts: -1 (lint blank line do merge fix(baileys): prevent healthy instances from being killed after stream:error 515 #2509 que estava bloqueando pre-push)Por que não há teste
O projeto não tem suite de testes formal (ver
CLAUDE.md→ "No unit test suite currently implemented"). Em vez disso:npm run build→ success em ~3s)instance, exceto pelos 2 campos protegidos/instance/createcontinua funcionando — body do create não deveria mesmo conterinstanceName/instanceId(esses vêm da URL)Próximos passos sugeridos (follow-up, não nesta PR)
Closes #2435
Reported by @lighthousekeeper1212 via análise estática.
🤖 Generated with Claude Code
Summary by Sourcery
Prevent cross-instance authorization bypass by blocking overrides of protected instance identifiers via untrusted request data.
Bug Fixes:
Enhancements:
Chores: