fix(proxy): #1588 accept smart-HTTP paths without .git suffix - #1642
fix(proxy): #1588 accept smart-HTTP paths without .git suffix#1642re-vlad wants to merge 1 commit into
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
re-vlad
added
bug
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1642 +/- ##
==========================================
+ Coverage 85.87% 85.89% +0.01%
==========================================
Files 84 84
Lines 8109 8116 +7
Branches 1376 1379 +3
==========================================
+ Hits 6964 6971 +7
Misses 1117 1117
Partials 28 28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return null; | ||
| } | ||
|
|
||
| const smartHttp = requestPath.match(SMART_HTTP_PATH_REGEX); |
There was a problem hiding this comment.
The fallback is only reached when PROXIED_URL_PATH_REGEX fails, but that regex is unanchored, and String.match doesn't have to consume the whole string, so it matches almost any path containing .git and silently drops the rest. That includes paths where .git sits inside the repo name:
/github.com/finos/.github/info/refs?service=git-upload-pack
consumed: /github.com/finos/.git
dropped: hub/info/refs?service=git-upload-pack
→ repoPath "/github.com/finos/.git", gitPath "/"
validGitRequest("/") then rejects it, so the new fallback is dead code for these paths and the user still gets Invalid request received. .github and .github.io show up in most orgs. They work fine today with an explicit .git in the remote, so this isn't a regression, but it's exactly the git-less form this PR is meant to enable.
Fix is to swap the order: the operation suffix is a protocol guarantee. Match the anchored /info/refs, /git-upload-pack or /git-receive-pack suffix against the end of the path, take the prefix as the repo path, and append .git only when it isn't already there, the same normalisation you're doing now, just reached first.
Ran it over the full matrix: fixes .github, user.github.io and the query-string case (…&foo=.git, which currently swallows the whole path into repoPath), and leaves every currently-working input identical. Worth anchoring PROXIED_URL_PATH_REGEX while you're in here too, byte-identical on everything that works today, and it kills a whole class of silent partial matches.
| expect(processUrlPath(VERY_LONG_PATH)).toBeNull(); | ||
| }); | ||
|
|
||
| it('processUrlPath should parse smart-HTTP paths without a .git repo suffix', () => { |
There was a problem hiding this comment.
Could you add the cases that pin the ordering down? Every input here has a repo name with no .git in it, which is why the problem isn't visible:
- /github.com/finos/.github/info/refs?service=git-upload-pack → /github.com/finos/.github.git
- /github.com/user/user.github.io/git-receive-pack → /github.com/user/user.github.io.git
- /github.com/finos/git-proxy (no git operation) → still null
The last one guards the Action constructor path, which relies on processUrlPath returning null rather than inventing a repo when there's nothing to split on.
| expect(isPackPost({ method: 'POST', url: '/a.git/git-upload-pack' } as Request)).toBe(true); | ||
| }); | ||
|
|
||
| it('returns true for git-upload-pack POST without a .git repo suffix', () => { |
There was a problem hiding this comment.
Could you add the negative that fixes the new boundary? isPackPost({ method: 'POST', url: '/git-upload-pack' }) must stay false, a single segment isn't a repo. It holds today, but nothing here would catch a future simplification of this regex that opens it up.
2 participants
Description
Git smart-HTTP clients may send repository paths without a
.gitsuffix (e.g.GET /owner/repo/info/refs?service=git-upload-pack). GitHub accepts this form; Git Proxy previously did not.processUrlPathonly matched paths containing.git((.+\.git)(\/.*)?). For.git-less smart-HTTP URLs it returnednull, so -proxyFilterrejected the request withInvalid request receivedbefore the plugin chain ran.This PR fixes that in two places (both are required for a full clone/fetch):
processUrlPath- after the existing.gitregex, add a smart-HTTP suffix fallback for paths ending in/info/refs,/git-upload-pack, or/git-receive-pack. NormaliserepoPathto end with.gitinternally soparseActionandgetRepoByUrlcontinue to work unchanged.isPackPost- make.gitoptional in the POST pack URL regex soPOST /owner/repo/git-upload-packis recognised andextractRawBodyruns. Without this, discovery could succeed but clone/fetch would still fail on the second HTTP request.Complementary to, not a substitute for: v1->v2 DB URL migration (#1535 / #1543), which normalises stored
repos.url. This PR fixes inbound wire paths clients send at runtime.Related Issue
Resolves #1588
Checklist
General
Documentation
N/A — behaviour fix aligned with GitHub/git client conventions; no new user-facing configuration.
Configuration
config.schema.json) was modified:npm run generate-config-types)npm run gen-schema-doc)N/A —
config.schema.jsonnot modified.Tests
npm test)npm run lintandnpm run format:check)npm run check-types)Local verification:
npm test(1141 passed),npm run lint, Prettier on changed files.Test plan
processUrlPath('/octocat/hello-world/info/refs?service=git-upload-pack')→repoPath: /octocat/hello-world.gitprocessUrlPath('/github.com/octocat/hello-world/info/refs?service=git-upload-pack')→ proxied host path with.gitnormalisedprocessUrlPath('/org/owner/repo/git-upload-pack')→ pack path parsed.gitunchanged (/octocat/hello-world.git/info/refs?...,/octocat/hello-world.git,/octocat/hello-world.git/)isPackPosttrue forPOST /a/b/git-upload-packandPOST /a/b.git/git-upload-packproxyFilterallowsGET /github.com/finos/git-proxy/info/refs?service=git-upload-packwithout mockingprocessUrlPathcurl -H "User-Agent: git/2.43.0" "http://<proxy>/owner/repo/info/refs?service=git-upload-pack"against authorised repo (both with and without.gitin path)