Skip to content

fix(proxy): preserve original Git pack POST streams before validation#1060

Merged
JamieSlome merged 12 commits intofinos:mainfrom
fabiovincenzi:clone-fix
Jul 4, 2025
Merged

fix(proxy): preserve original Git pack POST streams before validation#1060
JamieSlome merged 12 commits intofinos:mainfrom
fabiovincenzi:clone-fix

Conversation

@fabiovincenzi
Copy link
Contributor

This PR ensures that Git git-upload-pack and git-receive-pack POST requests are handled without changing their original packet:

  • Remove bodyParser.raw() in src/proxy/index.ts so that the proxy sees the unmodified stream.
  • Add a teeAndValidate middleware in src/proxy/routes/index.ts which:
    1. Duplicates the incoming request stream via PassThrough pipes.
    2. Uses one copy to extract the raw body and run our chain.
    3. Forwards the untouched original stream to GitHub, preserving headers and framing.

Closes #1037.

@netlify
Copy link

netlify bot commented Jun 25, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 37d9464
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68679087dbc5b90008881f67

@github-actions github-actions bot added the fix label Jun 25, 2025
@codecov
Copy link

codecov bot commented Jun 25, 2025

Codecov Report

Attention: Patch coverage is 93.87755% with 3 lines in your changes missing coverage. Please review.

Project coverage is 76.87%. Comparing base (75fb0e6) to head (37d9464).
Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
src/proxy/routes/index.ts 93.87% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1060      +/-   ##
==========================================
+ Coverage   76.78%   76.87%   +0.09%     
==========================================
  Files          55       55              
  Lines        2261     2266       +5     
  Branches      251      252       +1     
==========================================
+ Hits         1736     1742       +6     
+ Misses        495      494       -1     
  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

@coopernetes coopernetes left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @fabiovincenzi ! LGTM 👍

@coopernetes coopernetes enabled auto-merge June 27, 2025 03:28
@JamieSlome
Copy link
Member

@fabiovincenzi - can we resolve the merge conflict?

auto-merge was automatically disabled June 30, 2025 09:29

Head branch was pushed to by a user without write access

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 into the E2E CI issue. It seems that some of the latest changes in the CI might have broken the Cypress test that relies on test-repo being added in the unit tests.

I noticed that GitProxy adds the finos/git-proxy repo by default, so checking for that repo would likely fix the issue:

-     const cloneURL = 'http://localhost:8000/finos/test-repo.git';
+     const cloneURL = 'http://localhost:8000/finos/git-proxy.git';
      const tooltipQuery = 'div[role="tooltip"]';

      cy
        // tooltip isn't open to start with
        .get(tooltipQuery)
        .should('not.exist');

      cy
-       // find the entry for finos/test-repo
-       .get('a[href="/dashboard/repo/test-repo"]')
+       // find the entry for finos/git-proxy
+       .get('a[href="/dashboard/repo/git-proxy"]')
        // take it's parent row
        .closest('tr')
        // find the nearby span containing Code we can click to open the tooltip
        .find('span')
        .contains('Code')
        .should('exist')
        .click();

@jescalada
Copy link
Contributor

I also noticed that all the old unit tests are being ignored for some odd reason... Wonder if resolving the merge conflicts will fix this?

@fabiovincenzi
Copy link
Contributor Author

@jescalada Thanks for catching that – I realized I accidentally left a .only on the new tests, which caused only those to run and skipped the older ones. I also fixed the repo tests.

Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

LGTM! 🍰

@JamieSlome JamieSlome merged commit f96a8c4 into finos:main Jul 4, 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.

fetches and clones through git-proxy fail intermittently

4 participants