Security: Missing authorization checks on ticket listing endpoints#2910
Security: Missing authorization checks on ticket listing endpoints#2910tomaioo wants to merge 1 commit into
Conversation
The tickets routes return issue/comment data based only on the `namespace` URL parameter and cache lookups, but do not perform any per-request authorization check (e.g., project membership or explicit RBAC verification). An authenticated user may query `/namespaces/_all/tickets` (or other namespaces) and potentially retrieve tickets for projects they should not access. Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
tomaioo
requested review from
grolu,
holgerkoser,
klocke-io and
petersutter
as code owners
|
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
gardener-prow
Bot
added
the
do-not-merge/needs-kind
|
Welcome @tomaioo! |
gardener-prow
Bot
added
the
size/S
📝 WalkthroughWalkthroughAdded authorization enforcement to ticket routes using a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/lib/routes/tickets.js (1)
57-80: Optional: avoid resolving the project twice per request.Both handlers call
authorize(req, namespace)(which resolves the project) and thengetIssues/getIssuesAndComments, each of which callsgetProjectName(namespace)again. Consider returning the resolvedprojectNamefromauthorize()(or a smallresolveAndAuthorizehelper) and passing it intogetIssues/getIssuesAndCommentsto remove the duplicated cache lookup and keep a single source of truth for the namespace → project mapping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/lib/routes/tickets.js` around lines 57 - 80, Return the resolved project name from authorize(req, namespace) (or implement a small helper like resolveAndAuthorize) and pass that projectName into getIssues and getIssuesAndComments instead of letting those functions call getProjectName(namespace) again; update authorize to return the resolved project identifier (or add resolveAndAuthorize that calls authorize then returns projectName), then change the handlers to capture that return value and supply it to getIssues(projectName) and getIssuesAndComments(projectName, name) so the project lookup/cache hit happens once per request.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/lib/routes/tickets.js`:
- Around line 42-53: authorize currently calls getProjectName(namespace) which
dereferences cache.findProjectByNamespace(namespace).metadata.name and can throw
if the project is missing; change authorize to first resolve the project (via
cache.findProjectByNamespace or the existing getProjectName helper but without
blind dereference), check for null/undefined and if missing throw
createError(403) (or 404 per policy) to avoid a TypeError, otherwise extract
project.metadata.name and call req.user.canGetProject({ name: projectName }) as
before; update any use of getProjectName inside authorize to handle the
missing-project case instead of letting it throw.
---
Nitpick comments:
In `@backend/lib/routes/tickets.js`:
- Around line 57-80: Return the resolved project name from authorize(req,
namespace) (or implement a small helper like resolveAndAuthorize) and pass that
projectName into getIssues and getIssuesAndComments instead of letting those
functions call getProjectName(namespace) again; update authorize to return the
resolved project identifier (or add resolveAndAuthorize that calls authorize
then returns projectName), then change the handlers to capture that return value
and supply it to getIssues(projectName) and getIssuesAndComments(projectName,
name) so the project lookup/cache hit happens once per request.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30f1dfc1-d6a8-4dd2-9bfc-3a2b21443b17
📒 Files selected for processing (1)
backend/lib/routes/tickets.js
| async function authorize (req, namespace) { | ||
| if (namespace === '_all') { | ||
| if (!await req.user.canListIssues()) { | ||
| throw createError(403, 'Forbidden') | ||
| } | ||
| return | ||
| } | ||
| const projectName = getProjectName(namespace) | ||
| if (!await req.user.canGetProject({ name: projectName })) { | ||
| throw createError(403, 'Forbidden') | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle missing project to avoid 500 and inconsistent auth errors.
When namespace is not _all, authorize() calls getProjectName(namespace) (line 21-25), which dereferences cache.findProjectByNamespace(namespace).metadata.name without a null-check. If the namespace does not map to a known project (unknown namespace, cache miss, or a namespace the user cannot see), this throws a TypeError and Express returns a 500. That both defeats the intent of this security fix (callers get a different error shape depending on whether the namespace exists) and leaks a probing signal.
Resolve the project first and translate a missing project into the same 403 (or a 404) consistently.
🛡️ Suggested fix
async function authorize (req, namespace) {
if (namespace === '_all') {
if (!await req.user.canListIssues()) {
throw createError(403, 'Forbidden')
}
return
}
- const projectName = getProjectName(namespace)
+ const project = cache.findProjectByNamespace(namespace)
+ if (!project) {
+ throw createError(403, 'Forbidden')
+ }
+ const projectName = project.metadata.name
if (!await req.user.canGetProject({ name: projectName })) {
throw createError(403, 'Forbidden')
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function authorize (req, namespace) { | |
| if (namespace === '_all') { | |
| if (!await req.user.canListIssues()) { | |
| throw createError(403, 'Forbidden') | |
| } | |
| return | |
| } | |
| const projectName = getProjectName(namespace) | |
| if (!await req.user.canGetProject({ name: projectName })) { | |
| throw createError(403, 'Forbidden') | |
| } | |
| } | |
| async function authorize (req, namespace) { | |
| if (namespace === '_all') { | |
| if (!await req.user.canListIssues()) { | |
| throw createError(403, 'Forbidden') | |
| } | |
| return | |
| } | |
| const project = cache.findProjectByNamespace(namespace) | |
| if (!project) { | |
| throw createError(403, 'Forbidden') | |
| } | |
| const projectName = project.metadata.name | |
| if (!await req.user.canGetProject({ name: projectName })) { | |
| throw createError(403, 'Forbidden') | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/lib/routes/tickets.js` around lines 42 - 53, authorize currently
calls getProjectName(namespace) which dereferences
cache.findProjectByNamespace(namespace).metadata.name and can throw if the
project is missing; change authorize to first resolve the project (via
cache.findProjectByNamespace or the existing getProjectName helper but without
blind dereference), check for null/undefined and if missing throw
createError(403) (or 404 per policy) to avoid a TypeError, otherwise extract
project.metadata.name and call req.user.canGetProject({ name: projectName }) as
before; update any use of getProjectName inside authorize to handle the
missing-project case instead of letting it throw.
2 participants
Summary
Security: Missing authorization checks on ticket listing endpoints
Problem
Severity:
High| File:backend/lib/routes/tickets.js:L14The tickets routes return issue/comment data based only on the
namespaceURL parameter and cache lookups, but do not perform any per-request authorization check (e.g., project membership or explicit RBAC verification). An authenticated user may query/namespaces/_all/tickets(or other namespaces) and potentially retrieve tickets for projects they should not access.Solution
Before returning tickets/comments, enforce authorization for the requesting user and namespace/project. For example, resolve namespace -> project, then call an authorization service (e.g.,
canGetProject/canListIssues) and return 403 on failure. Also explicitly forbid_allunless the user has a global admin/list permission.Changes
backend/lib/routes/tickets.js(modified)Summary by CodeRabbit