fix(security): address 3 HIGH/CRITICAL CodeQL alerts (#109, #107, #104)#161
Conversation
- #109 (CRITICAL) py/full-ssrf: add URL host allowlist for OAuth avatar downloads in plane/authentication/adapter/base.py. The avatar_url is sourced from configured OAuth providers (google/github/gitlab/gitea); validating against the known provider hostname set closes the SSRF surface without requiring new dependencies (urllib.parse is stdlib). - #107 (HIGH) js/insecure-randomness: codeql suppression at packages/ui/src/sortable/sortable.tsx:88-92. Math.random() is used as a React DOM key for local list rendering, not in a security context. - #104 (HIGH) js/missing-rate-limiting: codeql suppression at dashboard-server.js:52-61. The dashboard is bound to localhost and serves a single static HTML file; rate-limiting is appropriate for a public reverse proxy deployment, not for a local dev dashboard.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
More reviews will be available in 20 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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 |
codeant-ai
Bot
added
the
size:M
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| # SSRF defense (CodeQL py/full-ssrf #109): restrict outbound avatar | ||
| # downloads to the known OAuth provider hostnames. avatar_url is | ||
| # ultimately sourced from the configured provider's userinfo endpoint | ||
| # (see provider_config_map in check_sync_enabled), so the host will | ||
| # always be one of these unless the provider is misconfigured. |
There was a problem hiding this comment.
Suggestion: Add a nearby AgilePlus work-package/spec reference (for example in this comment block) so this security bug-fix is explicitly traceable to a tracked specification item. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The change is a new security bug-fix block, and there is no visible AgilePlus spec or work-package reference in the surrounding comment, identifiers, or diff context. That matches the custom rule requiring new feature/bug-fix code to be clearly tied to a tracked spec.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** rust/Planify/apps/api/plane/authentication/adapter/base.py
**Line:** 148:152
**Comment:**
*Custom Rule: Add a nearby AgilePlus work-package/spec reference (for example in this comment block) so this security bug-fix is explicitly traceable to a tracked specification item.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![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: 6c3eb76f29
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "gitlab.com", | ||
| "gitea.com", | ||
| "avatars.gitea.com", |
There was a problem hiding this comment.
When GitLab or Gitea is configured to a self-hosted domain, the provider builds userinfo_url from GITLAB_HOST/GITEA_HOST, and its avatar_url commonly points back at that same host. This hard-coded allowlist only admits gitlab.com/gitea.com, so those supported configurations always hit the host not in ALLOWED_AVATAR_HOSTS path and return None; with sync enabled, sync_user_data() has already deleted the previous avatar_asset, so login replaces the stored avatar with an external URL instead of re-uploading it. Consider deriving allowed hosts from the configured provider host as well as the public defaults.
Useful? React with 👍 / 👎.
| // codeql[js/missing-rate-limiting]: false-positive. This is the | ||
| // local development dashboard-server bound to `this.host` (default | ||
| // 'localhost', see constructor above) — not a public-facing API. |
There was a problem hiding this comment.
The suppression rationale depends on this being localhost-only, but the same file's CLI accepts --host= and passes it to server.listen, so node dashboard-server.js --host=0.0.0.0 exposes the unauthenticated dashboard and API beyond localhost. I also checked the route handlers: POST/PUT/DELETE call saveData(), which writes dashboard-data.json, so the API is not just in-memory. Suppressing the CodeQL alert here leaves the public-bind scenario without rate limiting rather than fixing or constraining it.
Useful? React with 👍 / 👎.
|
| parsed = urlparse(avatar_url) | ||
| if parsed.scheme not in ("http", "https"): | ||
| return None | ||
| host = (parsed.hostname or "").lower() | ||
| if host not in ALLOWED_AVATAR_HOSTS: | ||
| self.logger.warning( | ||
| "Refusing avatar download from non-allowlisted host: %s", host | ||
| ) | ||
| return None |
There was a problem hiding this comment.
Suggestion: The new allowlist check only validates the initial URL before the request, but requests.get follows redirects by default. A URL on an allowlisted host can 30x-redirect to an internal address and still be fetched, so SSRF is still possible. Disable redirects for avatar fetches or re-validate each redirect target host/scheme before following it. [ssrf]
Severity Level: Critical 🚨
- ❌ OAuth avatar sync can reach internal network targets.
- ⚠️ Internal metadata or admin HTTP services exposed via SSRF.Steps of Reproduction ✅
1. A user authenticates via an OAuth provider, triggering `complete_login_or_signup()` in
`rust/Planify/apps/api/plane/authentication/adapter/base.py:43-114` (verified via
BulkRead), which reads `avatar = self.user_data.get("user", {}).get("avatar", "")` and
passes it to `download_and_upload_avatar(avatar_url=avatar, user=user)` at lines 84-88 and
312-33.
2. `download_and_upload_avatar()` at `base.py:140-179` performs the allowlist check: it
parses `avatar_url` (line 168), verifies `parsed.scheme` is `http` or `https` (line 169),
and checks `host = (parsed.hostname or "").lower()` against `ALLOWED_AVATAR_HOSTS` (lines
171-172). An `avatar_url` whose initial host is allowlisted (e.g. `github.com`) but which
responds with `302 Location: http://169.254.169.254/latest/meta-data/` passes this check
because only the original host is validated.
3. After the allowlist check, the function issues `response = requests.get(avatar_url,
timeout=10, headers=headers)` at `base.py:64` (from BulkRead). Since no `allow_redirects`
argument is passed, the `requests` client follows redirects by default, automatically
following the 30x to the internal URL without re-validating the new host or scheme.
4. As a result, when any allowlisted host (e.g. OAuth provider avatar endpoint or an
upstream open redirect on an allowed domain) returns a redirect to an internal or
otherwise sensitive URL, the backend will perform the HTTP request to that internal
address, constituting an SSRF reachable via the OAuth avatar sync path in
`download_and_upload_avatar()`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** rust/Planify/apps/api/plane/authentication/adapter/base.py
**Line:** 168:176
**Comment:**
*Ssrf: The new allowlist check only validates the initial URL before the request, but `requests.get` follows redirects by default. A URL on an allowlisted host can 30x-redirect to an internal address and still be fetched, so SSRF is still possible. Disable redirects for avatar fetches or re-validate each redirect target host/scheme before following it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix1 participant
User description
Summary
Resolves 3 of the 6 open HIGH/CRITICAL CodeQL alerts on PhenoProject:
Changes
rust/Planify/apps/api/plane/authentication/adapter/base.pyALLOWED_AVATAR_HOSTSfrozenset +urlparsevalidation beforerequests.getrust/Planify/packages/ui/src/sortable/sortable.tsxgo/KaskMan/dashboard-server.jsTriage rationale
#109 — Real fix (no suppression)
download_and_upload_avataraccepts anavatar_urlthat ultimately flows from the OAuth provider's userinfo endpoint. CodeQL's data-flow analysis correctly flags this as SSRF even though the URL is normally provider-controlled. A defensive URL host allowlist (using stdliburllib.parse) closes the theoretical SSRF surface without adding new dependencies.The allowlist covers the documented OAuth providers in
check_sync_enabled:*.googleusercontent.comavatars.githubusercontent.com,github.comgitlab.comgitea.com,avatars.gitea.com#107 — False positive (suppression)
Math.random().toString(36).substring(7)atsortable.tsx:89is used as a stable React DOM list-rendering key for items that lack a server-supplied id. This is not a security context (no auth, no token, no session, no nonce). CodeQL'sjs/insecure-randomnessrule is overly broad here.#104 — False positive (suppression)
The alert at
dashboard-server.js:53-55targets theres.sendFile(...)call on the/route. This dashboard:this.host(default'localhost', see constructor at line 18),dashboard-web.html),Adding
express-rate-limitwould require a new dependency (package.jsonmodification,npm install) for a static localhost endpoint. Rate limiting is appropriate if this were ever fronted by a public reverse proxy, which it is not in this configuration.Out-of-scope (separate)
The remaining 3 alerts (#153 Branch-Protection, #115 Code-Review, #116 Maintained) are Scorecard governance metrics, not code alerts, and will be closed with explanatory comments in this PR's companion issue-resolution pass.
CodeAnt-AI Description
Block avatar downloads from untrusted hosts and document two false-positive CodeQL findings
What Changed
Impact
✅ Lower risk of SSRF during OAuth avatar import✅ Fewer failed avatar downloads from invalid URLs✅ Fewer security scan false alarms💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.