🛡️ Sentinel: [CRITICAL] Fix missing authentication on Zalo service#39
🛡️ Sentinel: [CRITICAL] Fix missing authentication on Zalo service#39mbsebada wants to merge 2 commits into
Conversation
Co-authored-by: mbsebada <19614248+mbsebada@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe89da6747
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!expectedApiKey) { | ||
| console.error("[Zalo Service Security] ZALO_SERVICE_API_KEY is not configured!"); | ||
| return res.status(500).json({ error: "Internal server error" }); |
There was a problem hiding this comment.
This middleware turns every non-/health request into HTTP 500 when ZALO_SERVICE_API_KEY is absent, but the current local startup path (examples/rentagent_vn/run_all.py) does not set that variable and api/routes/zalo.py defaults the forwarded key to "". In that default environment, all Zalo auth/message flows now fail immediately, which is a functional regression for the example stack rather than a graceful security hardening path. Consider wiring a shared dev key during local startup (or failing fast at process boot with explicit setup instructions) so the example remains runnable.
Useful? React with 👍 / 👎.
Co-authored-by: mbsebada <19614248+mbsebada@users.noreply.github.com>
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: The Node.js Zalo integration microservice (
examples/rentagent_vn/zalo-service/index.js) was entirely unauthenticated. This allowed any local service, or anyone able to bypass the API proxy, to call internal endpoints (such as/message/sendor/auth/cookie) directly and perform sensitive actions (like sending messages from the user's account) without proper authorization.🎯 Impact: Unauthorized internal or SSRF-exploited access to the Zalo messaging service, leading to arbitrary message sending on behalf of the integrated account.
🔧 Fix: Added authentication middleware to the Express app. It verifies an
x-api-keyheader against theZALO_SERVICE_API_KEYenvironment variable. The/healthcheck remains exempt from authentication. It usescrypto.timingSafeEqualwith matchingBufferinstances to securely compare keys and avoid timing side-channels, and fails securely (HTTP 500) if the environment variable is not configured. The Python proxy client (examples/rentagent_vn/api/routes/zalo.py) was also updated to pass this header.✅ Verification: Verified by checking code changes, linting specific changed files via
ruff check, and validating the Python test suite. Code review passed and journal updated.PR created automatically by Jules for task 5162267206061211968 started by @mbsebada