Skip to content

fix(proxy): #1588 accept smart-HTTP paths without .git suffix #1642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
re-vlad wants to merge 1 commit into
base: main
Choose a base branch
from
+61 −4
Open

fix(proxy): #1588 accept smart-HTTP paths without .git suffix #1642

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 19 additions & 3 deletions src/proxy/routes/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ export const processGitUrl = (url: string): GitUrlBreakdown | null => {
* into the embedded git end point and path for the git operation. */
const PROXIED_URL_PATH_REGEX = /(.+\.git)(\/.*)?/;

/** Fallback for smart-HTTP paths that omit the .git repo suffix (e.g. GitHub-style URLs). */
const SMART_HTTP_PATH_REGEX = /^(.+?)(\/(?:info\/refs|git-upload-pack|git-receive-pack)(?:\?.*)?)$/;

/** Type representing a breakdown of paths requested from the proxy server */
export type UrlPathBreakdown = { repoPath: string; gitPath: string };

Expand All @@ -85,6 +88,11 @@ export type UrlPathBreakdown = { repoPath: string; gitPath: string };
* - repoPath: /github.com/finos/git-proxy.git
* - gitPath: /info/refs?service=git-upload-pack
*
* Processing /github.com/finos/git-proxy/info/refs?service=git-upload-pack (no .git suffix)
* would produce:
* - repoPath: /github.com/finos/git-proxy.git
* - gitPath: /info/refs?service=git-upload-pack
*
* @param {string} requestPath The URL path to process.
* @return {GitUrlBreakdown | null} A breakdown of the components of the URL path.
*/
Expand All @@ -100,10 +108,18 @@ export const processUrlPath = (requestPath: string): UrlPathBreakdown | null =>
repoPath: components[1],
gitPath: components[2] ?? '/',
};
} else {
console.error(`Failed to parse proxy url path: ${requestPath}`);
return null;
}

const smartHttp = requestPath.match(SMART_HTTP_PATH_REGEX);

@fabiovincenzi fabiovincenzi Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

if (smartHttp) {
return {
repoPath: smartHttp[1] + '.git',
gitPath: smartHttp[2],
};
}

console.error(`Failed to parse proxy url path: ${requestPath}`);
return null;
};

/** Regex used to analyze repo URLs (with protocol and origin) to extract the repository name and
Expand Down
2 changes: 1 addition & 1 deletion src/proxy/routes/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ const proxyErrorHandler: ProxyOptions['proxyErrorHandler'] = (err, _res, next) =

const isPackPost = (req: Request) =>
req.method === 'POST' &&
/^(?:\/[^/]+)*\/[^/]+\.git\/(?:git-upload-pack|git-receive-pack)$/.test(req.url);
/^(?:\/[^/]+)*\/[^/]+(?:\.git)?\/(?:git-upload-pack|git-receive-pack)$/.test(req.url);

const extractRawBody = async (req: Request, res: Response, next: NextFunction) => {
if (!isPackPost(req)) {
Expand Down
7 changes: 7 additions & 0 deletions test/extractRawBody.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,13 @@ describe('isPackPost()', () => {
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', () => {

@fabiovincenzi fabiovincenzi Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

expect(isPackPost({ method: 'POST', url: '/a/b/git-upload-pack' } as Request)).toBe(true);
expect(
isPackPost({ method: 'POST', url: '/github.com/org/repo/git-receive-pack' } as Request),
).toBe(true);
});

it('returns false for other URLs', () => {
expect(isPackPost({ method: 'POST', url: '/info/refs' } as Request)).toBe(false);
});
Expand Down
15 changes: 15 additions & 0 deletions test/testProxyRoute.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,21 @@ describe('proxyFilter', () => {
expect(mockReq.body).toEqual(Buffer.from('test data'));
expect((mockReq as any).bodyRaw).toBeUndefined();
});

it('should allow GET info/refs for a .git-less smart-HTTP path without mocking processUrlPath', async () => {
mockReq.url = '/github.com/finos/git-proxy/info/refs?service=git-upload-pack';
mockReq.method = 'GET';

vi.spyOn(chain, 'executeChain').mockResolvedValue({
error: false,
blocked: false,
} as Action);

const result = await proxyFilter?.(mockReq as Request, mockRes as Response);

expect(result).toBe(true);
expect(sendMock).not.toHaveBeenCalled();
});
});

describe('Invalid requests', () => {
Expand Down
19 changes: 19 additions & 0 deletions test/testRouteFilter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,25 @@ describe('url helpers and filter functions used in the proxy', () => {
expect(processUrlPath(VERY_LONG_PATH)).toBeNull();
});

it('processUrlPath should parse smart-HTTP paths without a .git repo suffix', () => {

@fabiovincenzi fabiovincenzi Aug 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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(processUrlPath('/octocat/hello-world/info/refs?service=git-upload-pack')).toEqual({
repoPath: '/octocat/hello-world.git',
gitPath: '/info/refs?service=git-upload-pack',
});

expect(
processUrlPath('/github.com/octocat/hello-world/info/refs?service=git-upload-pack'),
).toEqual({
repoPath: '/github.com/octocat/hello-world.git',
gitPath: '/info/refs?service=git-upload-pack',
});

expect(processUrlPath('/org/owner/repo/git-upload-pack')).toEqual({
repoPath: '/org/owner/repo.git',
gitPath: '/git-upload-pack',
});
});

it('processGitUrl should return breakdown of a git URL separating out the protocol, host and repository path', () => {
expect(processGitUrl('https://somegithost.com/octocat/hello-world.git')).toEqual({
protocol: 'https://',
Expand Down
Loading