Skip to content

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 6c521a5
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68b5e77561489500087cf9b6

@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 82.78%. Comparing base (b6dd0f1) to head (6c521a5).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1095   +/-   ##
=======================================
  Coverage   82.78%   82.78%           
=======================================
  Files          66       66           
  Lines        2783     2783           
  Branches      332      332           
=======================================
  Hits         2304     2304           
  Misses        431      431           
  Partials       48       48           

☔ 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

@kriswest
Copy link
Contributor

@andypols you should be able to update this now against main. Please check its not re-introducing any functions that are not needed - we've done some consolidation of the duplicated code in the DB. canUserApproveRejectPushRepo was recently removed from repo.ts and should not come back.

@kriswest kriswest self-requested a review August 21, 2025 10:37
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.

Pls rebase and assess against current state of main

@andypols
Copy link
Contributor Author

andypols commented Sep 1, 2025

@finos/git-proxy-maintainers I've merged with latest changes and all is left following @kriswest checking is the test. Please can you review and merge is all ok.

Thanks

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