diff --git a/.changeset/fix-scm-url-path-traversal.md b/.changeset/fix-scm-url-path-traversal.md new file mode 100644 index 0000000000..e2b6ddab0f --- /dev/null +++ b/.changeset/fix-scm-url-path-traversal.md @@ -0,0 +1,5 @@ +--- +'@backstage/integration': patch +--- + +Fixed a security vulnerability where path traversal sequences in SCM URLs could be used to access unintended API endpoints using server-side integration credentials. diff --git a/packages/integration/src/bitbucketCloud/core.ts b/packages/integration/src/bitbucketCloud/core.ts index 9beb3bd748..b1f95ad2d0 100644 --- a/packages/integration/src/bitbucketCloud/core.ts +++ b/packages/integration/src/bitbucketCloud/core.ts @@ -16,6 +16,7 @@ import fetch from 'cross-fetch'; import parseGitUrl from 'git-url-parse'; +import { parseGitUrlSafe } from '../helpers'; import { BitbucketCloudIntegrationConfig } from './config'; import { DateTime } from 'luxon'; @@ -165,7 +166,7 @@ export async function getBitbucketCloudDownloadUrl( ref, protocol, resource, - } = parseGitUrl(url); + } = parseGitUrlSafe(url); let branch = ref; if (!branch) { @@ -193,7 +194,7 @@ export function getBitbucketCloudFileFetchUrl( config: BitbucketCloudIntegrationConfig, ): string { try { - const { owner, name, ref, filepathtype, filepath } = parseGitUrl(url); + const { owner, name, ref, filepathtype, filepath } = parseGitUrlSafe(url); if (!owner || !name || (filepathtype !== 'src' && filepathtype !== 'raw')) { throw new Error('Invalid Bitbucket Cloud URL or file path'); } diff --git a/packages/integration/src/bitbucketServer/core.ts b/packages/integration/src/bitbucketServer/core.ts index 8741b8f0e7..5d042d5f02 100644 --- a/packages/integration/src/bitbucketServer/core.ts +++ b/packages/integration/src/bitbucketServer/core.ts @@ -16,6 +16,7 @@ import fetch from 'cross-fetch'; import parseGitUrl from 'git-url-parse'; +import { parseGitUrlSafe } from '../helpers'; import { BitbucketServerIntegrationConfig } from './config'; /** @@ -74,7 +75,12 @@ export async function getBitbucketServerDownloadUrl( url: string, config: BitbucketServerIntegrationConfig, ): Promise { - const { name: repoName, owner: project, ref, filepath } = parseGitUrl(url); + const { + name: repoName, + owner: project, + ref, + filepath, + } = parseGitUrlSafe(url); let branch = ref; if (!branch) { @@ -108,7 +114,7 @@ export function getBitbucketServerFileFetchUrl( config: BitbucketServerIntegrationConfig, ): string { try { - const { owner, name, ref, filepathtype, filepath } = parseGitUrl(url); + const { owner, name, ref, filepathtype, filepath } = parseGitUrlSafe(url); if ( !owner || !name || diff --git a/packages/integration/src/github/core.test.ts b/packages/integration/src/github/core.test.ts index 8e9507b239..d129f57838 100644 --- a/packages/integration/src/github/core.test.ts +++ b/packages/integration/src/github/core.test.ts @@ -154,5 +154,49 @@ describe('github core', () => { ), ).toEqual('https://ghe.mycompany.net/raw/a/b/branchname/path/to/c.yaml'); }); + + it('rejects URLs with encoded path traversal sequences', () => { + const config: GithubIntegrationConfig = { + host: 'github.com', + apiBaseUrl: 'https://api.github.com', + }; + expect(() => + getGithubFileFetchUrl( + 'https://github.com/octocat/Hello-World/blob/main/%2e%2e%2f%2e%2e%2f%2e%2e%2f%2e%2e%2fuser/repos', + config, + tokenCredentials, + ), + ).toThrow(/path traversal/); + }); + + it('rejects URLs with literal path traversal in filepath', () => { + const config: GithubIntegrationConfig = { + host: 'github.com', + apiBaseUrl: 'https://api.github.com', + }; + // Literal ../ is normalized by the URL constructor before git-url-parse + // sees it, so it fails with the existing validation instead + expect(() => + getGithubFileFetchUrl( + 'https://github.com/octocat/Hello-World/blob/main/../../user/repos', + config, + tokenCredentials, + ), + ).toThrow(/Incorrect URL/); + }); + + it('rejects raw endpoint URLs with path traversal', () => { + const config: GithubIntegrationConfig = { + host: 'github.com', + rawBaseUrl: 'https://raw.githubusercontent.com', + }; + expect(() => + getGithubFileFetchUrl( + 'https://github.com/octocat/Hello-World/blob/main/%2e%2e%2f%2e%2e%2fuser/repos', + config, + tokenCredentials, + ), + ).toThrow(/path traversal/); + }); }); }); diff --git a/packages/integration/src/github/core.ts b/packages/integration/src/github/core.ts index d1f583b2fb..9599338497 100644 --- a/packages/integration/src/github/core.ts +++ b/packages/integration/src/github/core.ts @@ -14,8 +14,8 @@ * limitations under the License. */ -import parseGitUrl from 'git-url-parse'; import { GithubIntegrationConfig } from './config'; +import { parseGitUrlSafe } from '../helpers'; import { GithubCredentials } from './types'; /** @@ -39,7 +39,7 @@ export function getGithubFileFetchUrl( credentials: GithubCredentials, ): string { try { - const { owner, name, ref, filepathtype, filepath } = parseGitUrl(url); + const { owner, name, ref, filepathtype, filepath } = parseGitUrlSafe(url); if ( !owner || !name || diff --git a/packages/integration/src/helpers.test.ts b/packages/integration/src/helpers.test.ts index f1dd2e7ac8..b12236b031 100644 --- a/packages/integration/src/helpers.test.ts +++ b/packages/integration/src/helpers.test.ts @@ -19,6 +19,7 @@ import { basicIntegrations, defaultScmResolveUrl, isValidHost, + parseGitUrlSafe, } from './helpers'; describe('basicIntegrations', () => { @@ -317,3 +318,99 @@ describe('defaultScmResolveUrl', () => { ).toBe('https://b.com/b.yaml'); }); }); + +describe('parseGitUrlSafe', () => { + it('parses a valid GitHub blob URL', () => { + const result = parseGitUrlSafe( + 'https://github.com/owner/repo/blob/main/path/to/file.yaml', + ); + expect(result.owner).toBe('owner'); + expect(result.name).toBe('repo'); + expect(result.ref).toBe('main'); + expect(result.filepath).toBe('path/to/file.yaml'); + }); + + it('rejects URLs with encoded path traversal in filepath', () => { + expect(() => + parseGitUrlSafe( + 'https://github.com/octocat/Hello-World/blob/main/%2e%2e%2f%2e%2e%2f%2e%2e%2f%2e%2e%2fuser/repos', + ), + ).toThrow('path traversal'); + }); + + it('rejects URLs with double-encoded path traversal in filepath', () => { + expect(() => + parseGitUrlSafe( + 'https://github.com/octocat/Hello-World/blob/main/foo%2f%2e%2e%2fbar%2f%2e%2e%2f%2e%2e%2fsecret', + ), + ).toThrow('path traversal'); + }); + + it('rejects URLs with uppercase %2E encoding in filepath', () => { + expect(() => + parseGitUrlSafe( + 'https://github.com/octocat/Hello-World/blob/main/%2E%2E%2F%2E%2E%2Fuser/repos', + ), + ).toThrow('path traversal'); + }); + + it('rejects URLs with mixed-case percent encoding', () => { + expect(() => + parseGitUrlSafe( + 'https://github.com/octocat/Hello-World/blob/main/%2E%2e%2Fuser/repos', + ), + ).toThrow('path traversal'); + }); + + it('rejects URLs where git-url-parse leaves percent-encoded traversal segments', () => { + expect(() => + parseGitUrlSafe( + 'https://github.com/octocat/Hello-World/blob/main/%252e%252e%252fuser/repos', + ), + ).toThrow('path traversal'); + }); + + it('rejects URLs with triple-encoded path traversal', () => { + expect(() => + parseGitUrlSafe( + 'https://github.com/octocat/Hello-World/blob/main/%25252e%25252e%25252fuser/repos', + ), + ).toThrow('path traversal'); + }); + + it('rejects URLs with literal .. in the middle of the filepath', () => { + // URL normalization resolves foo/../bar to just bar, so the filepath + // won't contain traversal. But we verify parseGitUrlSafe still handles + // the resolved path safely. + const result = parseGitUrlSafe( + 'https://github.com/owner/repo/blob/main/foo/../bar', + ); + expect(result.filepath).toBe('bar'); + }); + + it('handles literal ../ that gets normalized away by URL parsing', () => { + // Literal ../../../ gets resolved by the URL constructor before + // git-url-parse sees it. This mangles owner/name but the filepath + // is empty, so parseGitUrlSafe doesn't reject it. Downstream + // functions reject these via their own validation. + const result = parseGitUrlSafe( + 'https://github.com/owner/repo/blob/main/../../../user/repos', + ); + expect(result.filepath).toBe(''); + expect(result.owner).not.toBe('owner'); + }); + + it('allows filenames that contain dots but are not traversal', () => { + const result = parseGitUrlSafe( + 'https://github.com/owner/repo/blob/main/path/to/some..file.yaml', + ); + expect(result.filepath).toBe('path/to/some..file.yaml'); + }); + + it('allows URLs without a filepath', () => { + const result = parseGitUrlSafe('https://github.com/owner/repo'); + expect(result.owner).toBe('owner'); + expect(result.name).toBe('repo'); + expect(result.filepath).toBe(''); + }); +}); diff --git a/packages/integration/src/helpers.ts b/packages/integration/src/helpers.ts index f56d360325..9a7012d9ce 100644 --- a/packages/integration/src/helpers.ts +++ b/packages/integration/src/helpers.ts @@ -18,6 +18,38 @@ import parseGitUrl from 'git-url-parse'; import { trimEnd } from 'lodash'; import { ScmIntegration, ScmIntegrationsGroup } from './types'; +/** + * Wraps git-url-parse and rejects URLs whose filepath contains path traversal + * segments. Without this check, a URL like + * `https://github.com/o/r/blob/main/%2e%2e%2f%2e%2e%2fuser/repos` would be + * decoded to `../../user/repos` and could escape the expected API path when + * interpolated into provider API URLs. + */ +export function parseGitUrlSafe(url: string) { + const parsed = parseGitUrl(url); + if (parsed.filepath) { + let decoded = parsed.filepath; + let previous; + do { + previous = decoded; + try { + decoded = decodeURIComponent(decoded); + } catch { + break; + } + } while (decoded !== previous); + + if ( + decoded.split('/').some(segment => segment === '..' || segment === '.') + ) { + throw new Error( + 'Invalid SCM URL: path traversal is not allowed in the URL', + ); + } + } + return parsed; +} + /** Checks whether the given argument is a valid URL hostname */ export function isValidHost(host: string): boolean { const check = new URL('http://example.com'); @@ -84,7 +116,7 @@ export function defaultScmResolveUrl(options: { if (url.startsWith('/')) { // If it is an absolute path, move relative to the repo root - const { href, filepath } = parseGitUrl(base); + const { href, filepath } = parseGitUrlSafe(base); updated = new URL(href);