Skip to content

Conversation

fabiovincenzi
Copy link
Contributor

@fabiovincenzi fabiovincenzi commented Sep 15, 2025

Fixes #1181 by implementing proper Git protocol error handling for GET /info/refs requests, replacing confusing error messages when cloning unauthorized repositories.

The issue was a Git protocol mismatch in error handling:

  1. GET /info/refs requests (clone operations) require a specific error packet format (https://git-scm.com/docs/gitprotocol-pack)
  2. Previous implementation used POST-oriented sideband format (\x02 + tab formatting) for all requests
  3. Git client received malformed upload-pack data → parsing failed → generic "hash algorithm" error
Screenshot 2025-09-15 at 16 02 13

Copy link

netlify bot commented Sep 15, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

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

@github-actions github-actions bot added the fix label Sep 15, 2025
Copy link

codecov bot commented Sep 15, 2025

Codecov Report

❌ Patch coverage is 88.88889% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.86%. Comparing base (6ee5f0b) to head (fadf820).
⚠️ Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
src/proxy/routes/index.ts 88.88% 3 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1204      +/-   ##
==========================================
+ Coverage   83.72%   83.86%   +0.13%     
==========================================
  Files          67       67              
  Lines        2888     2906      +18     
  Branches      366      366              
==========================================
+ Hits         2418     2437      +19     
+ Misses        410      409       -1     
  Partials       60       60              

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

Great work figuring this out! I've dug out some more info on when requests to /info/refs occur, to inform comments and think you should make a function name more specific - but otherwise very happy to see this solved.

fabiovincenzi and others added 3 commits September 15, 2025 16:43
Co-authored-by: Kris West <kristopher.west@natwest.com>
Signed-off-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com>
Co-authored-by: Kris West <kristopher.west@natwest.com>
Signed-off-by: Fabio Vincenzi <93596376+fabiovincenzi@users.noreply.github.com>
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.

I just realized the catch statement down on line 103 of src/proxy/routes/index.ts should probably do the same check and use the response format - that handles uncaught errors thrown while running the chain. Not critical but more complete?

@fabiovincenzi
Copy link
Contributor Author

I just realized the catch statement down on line 103 of src/proxy/routes/index.ts should probably do the same check and use the response format - that handles uncaught errors thrown while running the chain. Not critical but more complete?

You are right! @kriswest

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.

One thing to simplify, otherwise LGTM!

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.

Tested the original flow and works fine, LGTM after fixing up Kris' comments! 🚀

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.

Happy with this.

@kriswest kriswest merged commit a15adf1 into finos:main Sep 19, 2025
14 checks passed
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.

Incorrect error message on cloning unauthorized repo
3 participants