From 9f66eaef5a253294def52978f75134b74ff66d59 Mon Sep 17 00:00:00 2001 From: Thomas Cooper Date: Tue, 23 Jan 2024 15:53:27 -0500 Subject: [PATCH 1/3] feat: filter non-git HTTP requests being proxied Fixes #410 --- src/proxy/routes/index.js | 60 ++++++++++++++++++++++++++++++++++++++- test/testRouteFilter.js | 43 ++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 test/testRouteFilter.js diff --git a/src/proxy/routes/index.js b/src/proxy/routes/index.js index 3ecd263af..dd8218b3c 100644 --- a/src/proxy/routes/index.js +++ b/src/proxy/routes/index.js @@ -5,6 +5,48 @@ const proxy = require('express-http-proxy'); const router = express.Router(); const chain = require('../chain'); +/** + * Check whether an HTTP request matches the list of permitted User-Agent strings. + * @param {string} agent the User-Agent string from the http request + * @return {boolean} whether the User-Agent is allowed + */ +const allowedUserAgent = (agent) => { + return agent.startsWith('git/'); +}; + +/** + * As part of content negotiation, the client sends an Accept header to indicate + * the media types it understands. This function checks whether the media type + * is allowed. + * @param {string} contentType Received media type from the request Accept header + * @return {boolean} Whether the media type is allowed + */ +const allowedContentType = (contentType) => { + return contentType.startsWith('application/x-git-'); +}; + +/** + * Test whether the request URL is allowed for proxying. + * @param {string} url the request URL (path) + * @return {boolean} whether the URL is allowed + */ +const allowedUrl = (url) => { + const safePaths = [ + '/info/refs?service=git-upload-pack', + '/info/refs?service=git-receive-pack', + '/git-upload-pack', + '/git-receive-pack', + ]; + const parts = url.split('/'); + if ( + (parts.length === 4 || parts.length === 5) && + Boolean(safePaths.find((path) => url.endsWith(path))) + ) { + return true; + } + return false; +}; + router.use( '/', proxy('https://github.com', { @@ -12,6 +54,16 @@ router.use( try { console.log(req.url); console.log('recieved'); + if ( + !( + allowedUserAgent(req.headers['user-agent']) && + allowedUrl(req.url) && + allowedContentType(req.headers['content-type']) + ) + ) { + res.status(400).send('Invalid request received'); + return false; + } if (req.body && req.body.length) { req.rawBody = req.body.toString('utf8'); } @@ -73,4 +125,10 @@ const handleMessage = async (message) => { return packetMessage; }; -module.exports = { router, handleMessage }; +module.exports = { + router, + handleMessage, + allowedContentType, + allowedUrl, + allowedUserAgent, +}; diff --git a/test/testRouteFilter.js b/test/testRouteFilter.js new file mode 100644 index 000000000..7fcfc5c63 --- /dev/null +++ b/test/testRouteFilter.js @@ -0,0 +1,43 @@ +/* eslint-disable max-len */ +const chai = require('chai'); +const allowedContentType = require('../src/proxy/routes').allowedContentType; +const allowedUrl = require('../src/proxy/routes').allowedUrl; +const allowedUserAgent = require('../src/proxy/routes').allowedUserAgent; + +chai.should(); + +const expect = chai.expect; + +describe('url filters for proxying ', function () { + it('allowedUrls should return true for safe URLs', function () { + expect( + allowedUrl('/octocat/hello-world.git/info/refs?service=git-upload-pack'), + ).true; + expect(allowedUrl('/octocat/hello-world.git/git-upload-pack')).true; + expect(allowedUrl('/octocat/hello-world/info/refs?service=git-upload-pack')) + .true; + expect(allowedUrl('/octocat/hello-world/git-upload-pack')).true; + }); + + it('allowedUrls should return false for unsafe URLs', function () { + expect(allowedUrl('/octocat/hello-world.git/foo/bar/baz')).false; + }); + + it('allowedUserAgent should return true for safe user-agent', function () { + expect(allowedUserAgent('git/2.40.0')).true; + }); + + it('allowedUserAgent should return false for other user-agents', function () { + expect(allowedUserAgent('Mozilla/5.0')).true; + }); + + it('allowedContentType should return true for safe content-type', function () { + expect(allowedContentType('application/x-git-upload-pack-request')).true; + expect(allowedContentType('application/x-git-receive-pack-request')).true; + }); + + it('allowedContentType should return false for other content-type', function () { + expect(allowedContentType('application/json')).false; + expect(allowedContentType('text/html')).true; + }); +}); From a243f7c37ba52385361df64794a9e67bc8d791c4 Mon Sep 17 00:00:00 2001 From: Thomas Cooper Date: Wed, 24 Jan 2024 16:27:57 -0500 Subject: [PATCH 2/3] feat: refactor to one validation function - Accept is only set for certain git actions so refactored into one validation function - pass routes correctly to express (fix TypeError on startup) --- src/proxy/index.js | 4 +- src/proxy/routes/index.js | 78 ++++++++++++++++---------------------- test/testRouteFilter.js | 79 ++++++++++++++++++++++++++------------- 3 files changed, 87 insertions(+), 74 deletions(-) diff --git a/src/proxy/index.js b/src/proxy/index.js index cce669618..b22801039 100644 --- a/src/proxy/index.js +++ b/src/proxy/index.js @@ -1,7 +1,7 @@ /* eslint-disable max-len */ const proxyApp = require('express')(); const bodyParser = require('body-parser'); -const routes = require('./routes').router; +const router = require('./routes').router; const config = require('../config'); const db = require('../db'); const { GIT_PROXY_SERVER_PORT: proxyHttpPort } = require('../config/env').Vars; @@ -14,7 +14,7 @@ const options = { // Setup the proxy middleware proxyApp.use(bodyParser.raw(options)); -proxyApp.use('/', routes); +proxyApp.use('/', router); const start = async () => { // Check to see if the default repos are in the repo list diff --git a/src/proxy/routes/index.js b/src/proxy/routes/index.js index dd8218b3c..116e20c5f 100644 --- a/src/proxy/routes/index.js +++ b/src/proxy/routes/index.js @@ -6,43 +6,38 @@ const router = express.Router(); const chain = require('../chain'); /** - * Check whether an HTTP request matches the list of permitted User-Agent strings. - * @param {string} agent the User-Agent string from the http request - * @return {boolean} whether the User-Agent is allowed + * Check whether an HTTP request is a Git request. It is a Git request if it + * matches one of the following: + * - the URL path is a well-known Git endpoint + * - the User-Agent starts with "git/" + * - the Accept header starts with "application/x-git-" (for specific paths) + * @param {express.Request} req Raw HTTP request + * @return {boolean} Whether the request is a Git request */ -const allowedUserAgent = (agent) => { - return agent.startsWith('git/'); -}; - -/** - * As part of content negotiation, the client sends an Accept header to indicate - * the media types it understands. This function checks whether the media type - * is allowed. - * @param {string} contentType Received media type from the request Accept header - * @return {boolean} Whether the media type is allowed - */ -const allowedContentType = (contentType) => { - return contentType.startsWith('application/x-git-'); -}; - -/** - * Test whether the request URL is allowed for proxying. - * @param {string} url the request URL (path) - * @return {boolean} whether the URL is allowed - */ -const allowedUrl = (url) => { - const safePaths = [ - '/info/refs?service=git-upload-pack', - '/info/refs?service=git-receive-pack', - '/git-upload-pack', - '/git-receive-pack', - ]; +const validGitRequest = (req) => { + const { 'user-agent': agent, accept } = req.headers; + const { url } = req; const parts = url.split('/'); + // only valid GitHub URLs are supported. + // url = '/{owner}/{repo}.git/{git-path}' + // url.split('/') = ['', '{owner}', '{repo}.git', '{git-path}'] + if (parts.length !== 4 && parts.length !== 5) { + return false; + } + parts.splice(1, 2); // remove the {owner} and {repo} from the array + const gitPath = parts.join('/'); if ( - (parts.length === 4 || parts.length === 5) && - Boolean(safePaths.find((path) => url.endsWith(path))) + gitPath === '/info/refs?service=git-upload-pack' || + gitPath === '/info/refs?service=git-receive-pack' ) { - return true; + // https://www.git-scm.com/docs/http-protocol#_discovering_references + // We can only filter based on User-Agent since the Accept header is not + // sent in this request + return agent.startsWith('git/'); + } + if (gitPath === '/git-upload-pack' || gitPath === '/git-receive-pack') { + // https://www.git-scm.com/docs/http-protocol#_uploading_data + return agent.startsWith('git/') && accept.startsWith('application/x-git-'); } return false; }; @@ -52,15 +47,10 @@ router.use( proxy('https://github.com', { filter: async function (req, res) { try { - console.log(req.url); - console.log('recieved'); - if ( - !( - allowedUserAgent(req.headers['user-agent']) && - allowedUrl(req.url) && - allowedContentType(req.headers['content-type']) - ) - ) { + console.log('request url: ', req.url); + console.log('host: ', req.headers.host); + console.log('user-agent: ', req.headers['user-agent']); + if (!validGitRequest(req)) { res.status(400).send('Invalid request received'); return false; } @@ -128,7 +118,5 @@ const handleMessage = async (message) => { module.exports = { router, handleMessage, - allowedContentType, - allowedUrl, - allowedUserAgent, + validGitRequest, }; diff --git a/test/testRouteFilter.js b/test/testRouteFilter.js index 7fcfc5c63..a635f4b18 100644 --- a/test/testRouteFilter.js +++ b/test/testRouteFilter.js @@ -1,43 +1,68 @@ /* eslint-disable max-len */ const chai = require('chai'); -const allowedContentType = require('../src/proxy/routes').allowedContentType; -const allowedUrl = require('../src/proxy/routes').allowedUrl; -const allowedUserAgent = require('../src/proxy/routes').allowedUserAgent; +const validGitRequest = require('../src/proxy/routes').validGitRequest; chai.should(); const expect = chai.expect; describe('url filters for proxying ', function () { - it('allowedUrls should return true for safe URLs', function () { - expect( - allowedUrl('/octocat/hello-world.git/info/refs?service=git-upload-pack'), - ).true; - expect(allowedUrl('/octocat/hello-world.git/git-upload-pack')).true; - expect(allowedUrl('/octocat/hello-world/info/refs?service=git-upload-pack')) - .true; - expect(allowedUrl('/octocat/hello-world/git-upload-pack')).true; + it('validGitRequest should return true for safe requests on expected URLs', function () { + [ + '/octocat/hello-world.git/info/refs?service=git-upload-pack', + '/octocat/hello-world.git/info/refs?service=git-receive-pack', + '/octocat/hello-world.git/git-upload-pack', + '/octocat/hello-world.git/git-receive-pack', + ] + .map((url) => { + return { + headers: { + 'user-agent': 'git/2.30.0', + accept: 'application/x-git-upload-pack-request', + }, + url: url, + }; + }) + .forEach((req) => { + expect(validGitRequest(req)).true; + }); }); - it('allowedUrls should return false for unsafe URLs', function () { - expect(allowedUrl('/octocat/hello-world.git/foo/bar/baz')).false; + it('validGitRequest should return false for unsafe URLs', function () { + const req = { + headers: { + 'user-agent': 'Mozilla/5.0', + accept: '*/*', + }, + url: '/', + }; + expect(validGitRequest(req)).false; }); - it('allowedUserAgent should return true for safe user-agent', function () { - expect(allowedUserAgent('git/2.40.0')).true; + it('validGitRequest should return false for other user-agents', function () { + const req = { + headers: { + 'user-agent': 'Mozilla/5.0', + accept: '*/*', + }, + url: '/octocat/hello-world', + }; + expect(validGitRequest(req)).false; }); - it('allowedUserAgent should return false for other user-agents', function () { - expect(allowedUserAgent('Mozilla/5.0')).true; - }); - - it('allowedContentType should return true for safe content-type', function () { - expect(allowedContentType('application/x-git-upload-pack-request')).true; - expect(allowedContentType('application/x-git-receive-pack-request')).true; - }); - - it('allowedContentType should return false for other content-type', function () { - expect(allowedContentType('application/json')).false; - expect(allowedContentType('text/html')).true; + it('validGitRequest should return false for unexpected content-type on certain URLs', function () { + ['application/json', 'text/html', '*/*'] + .map((accept) => { + return { + headers: { + 'user-agent': 'git/2.30.0', + accept: accept, + }, + url: '/octocat/hello-world.git/git-upload-pack', + }; + }) + .forEach((req) => { + expect(validGitRequest(req)).false; + }); }); }); From 30922744ef0df7bb30255bd9a668297e69cdd8a9 Mon Sep 17 00:00:00 2001 From: Thomas Cooper Date: Fri, 26 Jan 2024 12:19:06 -0500 Subject: [PATCH 3/3] feat: separate url stripping into its own function - separate out the HTTP git filtering logic from the URL parsing, which includes stripping the GitHub repo owner/name. --- src/proxy/routes/index.js | 45 ++++++++++------ test/testRouteFilter.js | 106 ++++++++++++++++++++++---------------- 2 files changed, 91 insertions(+), 60 deletions(-) diff --git a/src/proxy/routes/index.js b/src/proxy/routes/index.js index 116e20c5f..cc9bf46c7 100644 --- a/src/proxy/routes/index.js +++ b/src/proxy/routes/index.js @@ -6,36 +6,45 @@ const router = express.Router(); const chain = require('../chain'); /** - * Check whether an HTTP request is a Git request. It is a Git request if it - * matches one of the following: - * - the URL path is a well-known Git endpoint - * - the User-Agent starts with "git/" - * - the Accept header starts with "application/x-git-" (for specific paths) - * @param {express.Request} req Raw HTTP request - * @return {boolean} Whether the request is a Git request + * For a given Git HTTP request destined for a GitHub repo, + * remove the GitHub specific components of the URL. + * @param {string} url URL path of the request + * @return {string} Modified path which removes the {owner}/{repo} parts */ -const validGitRequest = (req) => { - const { 'user-agent': agent, accept } = req.headers; - const { url } = req; +const stripGitHubFromGitPath = (url) => { const parts = url.split('/'); - // only valid GitHub URLs are supported. // url = '/{owner}/{repo}.git/{git-path}' // url.split('/') = ['', '{owner}', '{repo}.git', '{git-path}'] if (parts.length !== 4 && parts.length !== 5) { - return false; + console.error('unexpected url received: ', url); + return undefined; } parts.splice(1, 2); // remove the {owner} and {repo} from the array - const gitPath = parts.join('/'); + return parts.join('/'); +}; + +/** + * Check whether an HTTP request has the expected properties of a + * Git HTTP request. The URL is expected to be "sanitized", stripped of + * specific paths such as the GitHub {owner}/{repo}.git parts. + * @param {string} url Sanitized URL which only includes the path specific to git + * @param {*} headers Request headers (TODO: Fix JSDoc linting and refer to node:http.IncomingHttpHeaders) + * @return {boolean} If true, this is a valid and expected git request. Otherwise, false. + */ +const validGitRequest = (url, headers) => { + const { 'user-agent': agent, accept } = headers; if ( - gitPath === '/info/refs?service=git-upload-pack' || - gitPath === '/info/refs?service=git-receive-pack' + [ + '/info/refs?service=git-upload-pack', + '/info/refs?service=git-receive-pack', + ].includes(url) ) { // https://www.git-scm.com/docs/http-protocol#_discovering_references // We can only filter based on User-Agent since the Accept header is not // sent in this request return agent.startsWith('git/'); } - if (gitPath === '/git-upload-pack' || gitPath === '/git-receive-pack') { + if (['/git-upload-pack', '/git-receive-pack'].includes(url)) { // https://www.git-scm.com/docs/http-protocol#_uploading_data return agent.startsWith('git/') && accept.startsWith('application/x-git-'); } @@ -50,7 +59,8 @@ router.use( console.log('request url: ', req.url); console.log('host: ', req.headers.host); console.log('user-agent: ', req.headers['user-agent']); - if (!validGitRequest(req)) { + const gitPath = stripGitHubFromGitPath(req.url); + if (gitPath === undefined || !validGitRequest(gitPath, req.headers)) { res.status(400).send('Invalid request received'); return false; } @@ -119,4 +129,5 @@ module.exports = { router, handleMessage, validGitRequest, + stripGitHubFromGitPath, }; diff --git a/test/testRouteFilter.js b/test/testRouteFilter.js index a635f4b18..8a0262d13 100644 --- a/test/testRouteFilter.js +++ b/test/testRouteFilter.js @@ -1,68 +1,88 @@ /* eslint-disable max-len */ const chai = require('chai'); const validGitRequest = require('../src/proxy/routes').validGitRequest; +const stripGitHubFromGitPath = + require('../src/proxy/routes').stripGitHubFromGitPath; chai.should(); const expect = chai.expect; describe('url filters for proxying ', function () { + it('stripGitHubFromGitPath should return the sanitized URL with owner & repo removed', function () { + expect( + stripGitHubFromGitPath( + '/octocat/hello-world.git/info/refs?service=git-upload-pack', + ), + ).eq('/info/refs?service=git-upload-pack'); + }); + + it('stripGitHubFromGitPath should return undefined if the url', function () { + expect(stripGitHubFromGitPath('/octocat/hello-world')).undefined; + }); + it('validGitRequest should return true for safe requests on expected URLs', function () { [ - '/octocat/hello-world.git/info/refs?service=git-upload-pack', - '/octocat/hello-world.git/info/refs?service=git-receive-pack', - '/octocat/hello-world.git/git-upload-pack', - '/octocat/hello-world.git/git-receive-pack', - ] - .map((url) => { - return { - headers: { - 'user-agent': 'git/2.30.0', - accept: 'application/x-git-upload-pack-request', - }, - url: url, - }; - }) - .forEach((req) => { - expect(validGitRequest(req)).true; - }); + '/info/refs?service=git-upload-pack', + '/info/refs?service=git-receive-pack', + '/git-upload-pack', + '/git-receive-pack', + ].forEach((url) => { + expect( + validGitRequest(url, { + 'user-agent': 'git/2.30.0', + accept: 'application/x-git-upload-pack-request', + }), + ).true; + }); }); it('validGitRequest should return false for unsafe URLs', function () { - const req = { - headers: { + ['/', '/foo'].forEach((url) => { + expect( + validGitRequest(url, { + 'user-agent': 'git/2.30.0', + accept: 'application/x-git-upload-pack-request', + }), + ).false; + }); + }); + + it('validGitRequest should return false for a browser request', function () { + expect( + validGitRequest('/', { 'user-agent': 'Mozilla/5.0', accept: '*/*', - }, - url: '/', - }; - expect(validGitRequest(req)).false; + }), + ).false; }); - it('validGitRequest should return false for other user-agents', function () { - const req = { - headers: { + it('validGitRequest should return false for unexpected combinations of headers & URLs', function () { + // expected Accept=application/x-git-upload-pack + expect( + validGitRequest('/git-upload-pack', { + 'user-agent': 'git/2.30.0', + accept: '*/*', + }), + ).false; + + // expected User-Agent=git/* + expect( + validGitRequest('/info/refs?service=git-upload-pack', { 'user-agent': 'Mozilla/5.0', accept: '*/*', - }, - url: '/octocat/hello-world', - }; - expect(validGitRequest(req)).false; + }), + ).false; }); it('validGitRequest should return false for unexpected content-type on certain URLs', function () { - ['application/json', 'text/html', '*/*'] - .map((accept) => { - return { - headers: { - 'user-agent': 'git/2.30.0', - accept: accept, - }, - url: '/octocat/hello-world.git/git-upload-pack', - }; - }) - .forEach((req) => { - expect(validGitRequest(req)).false; - }); + ['application/json', 'text/html', '*/*'].map((accept) => { + expect( + validGitRequest('/git-upload-pack', { + 'user-agent': 'git/2.30.0', + accept: accept, + }), + ).false; + }); }); });