Skip to content

fix: prevent DOS when checking an unknown repo #1095

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

andypols
Copy link
Contributor

@andypols andypols commented Jul 8, 2025

Summary

This PR fixes a potential denial-of-service (DoS) vulnerability:

When pushing to an unknown repository, the MongoDB implementation throws a TypeError due to attempting to access properties on a null object:

console.log(repo.users.canPush);
// TypeError: Cannot read properties of null (reading 'users')

Root Cause

The file-based database implementation correctly checks for the existence of a repository before accessing its fields. However, the MongoDB implementation does not.

Specifically, checkUserPushPermission calls isUserPushAllowed, which assumes the repository exists. If the repository is not found, accessing its properties throws a TypeError and stops the service.

Fix

This PR addresses the issue by:

  • Adding a guard clause in the MongoDB implementation of isUserPushAllowed to handle missing repositories safely.

  • Adds a unit test to verify behaviour when the repository does not exist.

Copy link

netlify bot commented Jul 8, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 33cf19f
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68737a0dfde66d0008aa15f6

@github-actions github-actions bot added the fix label Jul 8, 2025
Copy link
Contributor

@jescalada jescalada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍🏼

I looked around the mongo handlers and it seems there aren't any other similar bugs.

Copy link

codecov bot commented Jul 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.58%. Comparing base (4956b73) to head (33cf19f).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1095      +/-   ##
==========================================
- Coverage   77.40%   75.58%   -1.83%     
==========================================
  Files          55       57       +2     
  Lines        2293     2392      +99     
  Branches      258      271      +13     
==========================================
+ Hits         1775     1808      +33     
- Misses        488      554      +66     
  Partials       30       30              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will conflict with #1043 and the maintainers will need to decide which goes first with the other to resolve conflicts. However, I'd be happy to pick up the new tests from this and include in #1043 (by applying them to the src/db/index rather than src/db/mongo

}
resolve(repo.users.canPush.includes(user) || repo.users.canAuthorise.includes(user));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
resolve(repo.users.canPush.includes(user) || repo.users.canAuthorise.includes(user));
resolve(repo.users?.canPush.includes(user) || repo.users?.canAuthorise.includes(user));

The DB clients should make sure that the users element exists (although I think I had to make sure that was happening in #1043, after stumbling over a case where it did not). Doesn't hurt to be defensive here.

@@ -79,19 +79,19 @@ export const deleteRepo = async (name: string) => {
export const isUserPushAllowed = async (name: string, user: string) => {
name = name.toLowerCase();
user = user.toLowerCase();
console.log(`checking if user ${user} can push to ${name}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that #1043 moves this functions into src/db/index.ts so that it is not duplicated between the clients.

@@ -0,0 +1,52 @@
const chai = require('chai');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be orphaned when #1043 merges as the function under test moves to the src/db/index and applies over the underlying db client configured.

There's some, but not total, overlap between these tests and #1043 -these are tighter unit tests on isUserPushAllowed and are worth having.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants