Skip to content

fix: only trim trailing .git not any match#1094

Merged
jescalada merged 5 commits intofinos:mainfrom
06kellyjac:trim_dot_git_correctly
Jul 11, 2025
Merged

fix: only trim trailing .git not any match#1094
jescalada merged 5 commits intofinos:mainfrom
06kellyjac:trim_dot_git_correctly

Conversation

@06kellyjac
Copy link
Copy Markdown
Contributor

Currently .git is being replaced in repoNames but replace only catches the first match from left to right

> "hello.gitfrom.git".replace(".git", "")
'hellofrom.git'

Git repos can have .git included in the name so to correctly get the repoName without a trailing .git
it should only be removed from the back (assuming it's there).

Trailing .git being trimmed by github:
image

.git being allowed mid repo-name:
image

.git being allowed at the start of a repo name
image

This code checks the string ends with .git. If it does it returns the string up to but not including .git, otherwise it returns the whole string.

I've added a new test just for this edgecase.

  • fix: only trim trailing .git not any match
  • fix: trim ref/heads/ only from the prefix
  • test: improve visibility of throws in approve/reject push test
  • test: add approve/reject push test for .git inside name edgecase

@netlify
Copy link
Copy Markdown

netlify Bot commented Jul 8, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit 9a4032f
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/6870fa8833f44f0008128464

@06kellyjac 06kellyjac marked this pull request as ready for review July 8, 2025 11:15
@github-actions github-actions Bot added the fix label Jul 8, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 8, 2025

Codecov Report

Attention: Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.

Project coverage is 77.40%. Comparing base (15c68a3) to head (9a4032f).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
.../processors/push-action/checkUserPushPermission.ts 42.85% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1094      +/-   ##
==========================================
- Coverage   77.41%   77.40%   -0.01%     
==========================================
  Files          55       55              
  Lines        2276     2293      +17     
  Branches      255      258       +3     
==========================================
+ Hits         1762     1775      +13     
- Misses        484      488       +4     
  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.

@kriswest
Copy link
Copy Markdown
Contributor

kriswest commented Jul 8, 2025

This would be less needed if #1043 goes in soon as we make far far less use of the repoName (still used for display but little else). After that we'll use the full URL with ".git" on the end through the proxy OR _id through the API).

Currently replacing .git even in the middle of valid repo names.
This fix only trims from the back.
Do the same for the trailing .git for the ref/heads/ prefix to
be safe and clean things up.
@06kellyjac 06kellyjac force-pushed the trim_dot_git_correctly branch from e82bf0f to d268ea6 Compare July 11, 2025 11:31
@06kellyjac
Copy link
Copy Markdown
Contributor Author

Rebased

Copy link
Copy Markdown
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 after fixing merge conflicts! Although #1043 might reduce/eliminate the need for these changes, we can release this in GitProxy v1 - excellent for users who will still be using v1 for compatibility/legacy reasons. #1043 can be released with other breaking changes/security improvements in v2.

Thanks for the PR! 👍🏼

Comment thread src/proxy/processors/push-action/checkUserPushPermission.ts
Comment thread src/db/helper.ts
@jescalada jescalada merged commit 4c74a3a into finos:main Jul 11, 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.

3 participants