Skip to content

Conversation

kriswest
Copy link
Contributor

resolves #1163 by ensuring that requests are never forwarded on for unknown repositories:

  • Resolves issues with tests that were allowing requests to be forwarded for unknown repos
    • Stops stubs from the chain tests leaking into other tests
    • Removes stubbing from Proxy express application tests
    • Removes chain test that checks that no processors are run unless the action type is "push" or "pull"
    • Adds an integration test that checks if pull requests are forwarded to unknown repos
  • Improves handling of errors when executing the chain, ensuring that errors always block pushes
  • Changes the HTTP response code for blocked pushes to consistently use 403 instead of 200
  • Creates a default action chain that ensures that CheckRepoInAuthList is run for all requests

@kriswest kriswest requested a review from a team August 21, 2025 06:46
@kriswest kriswest added bug Something isn't working enhancement New feature or request labels Aug 21, 2025
Copy link

netlify bot commented Aug 21, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 4051bcb
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68b04058368eef000810edba

Copy link

codecov bot commented Aug 21, 2025

Codecov Report

❌ Patch coverage is 80.76923% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.70%. Comparing base (4bf2276) to head (4051bcb).
⚠️ Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
src/proxy/routes/index.ts 69.23% 2 Missing and 2 partials ⚠️
src/proxy/chain.ts 91.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1164      +/-   ##
==========================================
- Coverage   82.83%   82.70%   -0.13%     
==========================================
  Files          66       66              
  Lines        2784     2781       -3     
  Branches      334      333       -1     
==========================================
- Hits         2306     2300       -6     
- Misses        431      432       +1     
- Partials       47       49       +2     

☔ 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.

@kriswest
Copy link
Contributor Author

@finos/git-proxy-maintainers I figured out several reasons why the proxy will apparently forward requests for unknown repos:

  • stubbing in tests that wasn't needed
  • stubbing that was leaking from the chain tests because it wasn't reset - I've fixed one case, there may be others
  • no processors are run unless the action type is push or pull
  • errors thrown during the execution of the chain weren't being caught

I've tried to address each of these different issues, resulting in a new default action chain that only contains CheckRepoInAuthList (so that git-proxy will not forward requests for repos that it isn't aware of, of any type). I also switched the response code on error/block to 403 (forbidden).

@kriswest
Copy link
Contributor Author

@finos/git-proxy-maintainers I believe this and #1167 MUST both be merged before we issue another release candidate. @jescalada is there anything else that you are aware of that MUST be in the next RC? Perhaps the latest round of renovate PRs?

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.

Looks good to me! I did some experiments with removing the sinon restore/reset calls but it seems they're necessary (or our tests in testProxyRoute might need some adjustment).

@jescalada
Copy link
Contributor

@kriswest I believe this and the PR you mentioned should be enough! In fact, I was thinking about making an rc right after the multiple hosts PR got in, but I was hoping to space out the activity on the repo for SEO purposes...

We can always make new rcs until we're good to go with the v2 release. Having the community test them out for some time might be best as there have been unexpected consequences like the mongo issue that just came up.

@jescalada jescalada merged commit 24508b8 into finos:main Aug 28, 2025
14 checks passed
@kriswest kriswest deleted the 1163-dont-forward-unknown-repos branch August 28, 2025 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure that requests are not forwarded for unknown repositories
2 participants