From 6543260f0a523f1f44870381bbc36d2b7e16dc62 Mon Sep 17 00:00:00 2001 From: Jamie Magee Date: Sat, 11 Apr 2020 17:04:55 +0200 Subject: [PATCH 1/7] Remove deprecated URL API usage --- __tests__/proxy.test.ts | 35 +++++++++++++++++------------------ index.ts | 19 +++++++++---------- interfaces.ts | 3 +-- proxy.ts | 10 ++++------ 4 files changed, 31 insertions(+), 36 deletions(-) diff --git a/__tests__/proxy.test.ts b/__tests__/proxy.test.ts index f484909..e8e79f4 100644 --- a/__tests__/proxy.test.ts +++ b/__tests__/proxy.test.ts @@ -2,7 +2,6 @@ import * as http from 'http' import * as httpm from '../_out' import * as pm from '../_out/proxy' import * as proxy from 'proxy' -import * as url from 'url' let _proxyConnects: string[] let _proxyServer: http.Server @@ -39,107 +38,107 @@ describe('proxy', () => { }) it('getProxyUrl does not return proxyUrl if variables not set', () => { - let proxyUrl = pm.getProxyUrl(url.parse('https://github.com')) + let proxyUrl = pm.getProxyUrl(new URL('https://github.com')) expect(proxyUrl).toBeUndefined() }) it('getProxyUrl returns proxyUrl if https_proxy set for https url', () => { process.env['https_proxy'] = 'https://myproxysvr' - let proxyUrl = pm.getProxyUrl(url.parse('https://github.com')) + let proxyUrl = pm.getProxyUrl(new URL('https://github.com')) expect(proxyUrl).toBeDefined() }) it('getProxyUrl does not return proxyUrl if http_proxy set for https url', () => { process.env['http_proxy'] = 'https://myproxysvr' - let proxyUrl = pm.getProxyUrl(url.parse('https://github.com')) + let proxyUrl = pm.getProxyUrl(new URL('https://github.com')) expect(proxyUrl).toBeUndefined() }) it('getProxyUrl returns proxyUrl if http_proxy set for http url', () => { process.env['http_proxy'] = 'http://myproxysvr' - let proxyUrl = pm.getProxyUrl(url.parse('http://github.com')) + let proxyUrl = pm.getProxyUrl(new URL('http://github.com')) expect(proxyUrl).toBeDefined() }) it('getProxyUrl does not return proxyUrl if https_proxy set and in no_proxy list', () => { process.env['https_proxy'] = 'https://myproxysvr' process.env['no_proxy'] = 'otherserver,myserver,anotherserver:8080' - let proxyUrl = pm.getProxyUrl(url.parse('https://myserver')) + let proxyUrl = pm.getProxyUrl(new URL('https://myserver')) expect(proxyUrl).toBeUndefined() }) it('getProxyUrl returns proxyUrl if https_proxy set and not in no_proxy list', () => { process.env['https_proxy'] = 'https://myproxysvr' process.env['no_proxy'] = 'otherserver,myserver,anotherserver:8080' - let proxyUrl = pm.getProxyUrl(url.parse('https://github.com')) + let proxyUrl = pm.getProxyUrl(new URL('https://github.com')) expect(proxyUrl).toBeDefined() }) it('getProxyUrl does not return proxyUrl if http_proxy set and in no_proxy list', () => { process.env['http_proxy'] = 'http://myproxysvr' process.env['no_proxy'] = 'otherserver,myserver,anotherserver:8080' - let proxyUrl = pm.getProxyUrl(url.parse('http://myserver')) + let proxyUrl = pm.getProxyUrl(new URL('http://myserver')) expect(proxyUrl).toBeUndefined() }) it('getProxyUrl returns proxyUrl if http_proxy set and not in no_proxy list', () => { process.env['http_proxy'] = 'http://myproxysvr' process.env['no_proxy'] = 'otherserver,myserver,anotherserver:8080' - let proxyUrl = pm.getProxyUrl(url.parse('http://github.com')) + let proxyUrl = pm.getProxyUrl(new URL('http://github.com')) expect(proxyUrl).toBeDefined() }) it('checkBypass returns true if host as no_proxy list', () => { process.env['no_proxy'] = 'myserver' - let bypass = pm.checkBypass(url.parse('https://myserver')) + let bypass = pm.checkBypass(new URL('https://myserver')) expect(bypass).toBeTruthy() }) it('checkBypass returns true if host in no_proxy list', () => { process.env['no_proxy'] = 'otherserver,myserver,anotherserver:8080' - let bypass = pm.checkBypass(url.parse('https://myserver')) + let bypass = pm.checkBypass(new URL('https://myserver')) expect(bypass).toBeTruthy() }) it('checkBypass returns true if host in no_proxy list with spaces', () => { process.env['no_proxy'] = 'otherserver, myserver ,anotherserver:8080' - let bypass = pm.checkBypass(url.parse('https://myserver')) + let bypass = pm.checkBypass(new URL('https://myserver')) expect(bypass).toBeTruthy() }) it('checkBypass returns true if host in no_proxy list with port', () => { process.env['no_proxy'] = 'otherserver, myserver:8080 ,anotherserver' - let bypass = pm.checkBypass(url.parse('https://myserver:8080')) + let bypass = pm.checkBypass(new URL('https://myserver:8080')) expect(bypass).toBeTruthy() }) it('checkBypass returns true if host with port in no_proxy list without port', () => { process.env['no_proxy'] = 'otherserver, myserver ,anotherserver' - let bypass = pm.checkBypass(url.parse('https://myserver:8080')) + let bypass = pm.checkBypass(new URL('https://myserver:8080')) expect(bypass).toBeTruthy() }) it('checkBypass returns true if host in no_proxy list with default https port', () => { process.env['no_proxy'] = 'otherserver, myserver:443 ,anotherserver' - let bypass = pm.checkBypass(url.parse('https://myserver')) + let bypass = pm.checkBypass(new URL('https://myserver')) expect(bypass).toBeTruthy() }) it('checkBypass returns true if host in no_proxy list with default http port', () => { process.env['no_proxy'] = 'otherserver, myserver:80 ,anotherserver' - let bypass = pm.checkBypass(url.parse('http://myserver')) + let bypass = pm.checkBypass(new URL('http://myserver')) expect(bypass).toBeTruthy() }) it('checkBypass returns false if host not in no_proxy list', () => { process.env['no_proxy'] = 'otherserver, myserver ,anotherserver:8080' - let bypass = pm.checkBypass(url.parse('https://github.com')) + let bypass = pm.checkBypass(new URL('https://github.com')) expect(bypass).toBeFalsy() }) it('checkBypass returns false if empty no_proxy', () => { process.env['no_proxy'] = '' - let bypass = pm.checkBypass(url.parse('https://github.com')) + let bypass = pm.checkBypass(new URL('https://github.com')) expect(bypass).toBeFalsy() }) diff --git a/index.ts b/index.ts index a1f5491..a0ee081 100644 --- a/index.ts +++ b/index.ts @@ -1,4 +1,3 @@ -import url = require('url') import http = require('http') import https = require('https') import ifm = require('./interfaces') @@ -50,7 +49,7 @@ export enum MediaTypes { * @param serverUrl The server URL where the request will be sent. For example, https://api.github.com */ export function getProxyUrl(serverUrl: string): string { - let proxyUrl = pm.getProxyUrl(url.parse(serverUrl)) + let proxyUrl = pm.getProxyUrl(new URL(serverUrl)) return proxyUrl ? proxyUrl.href : '' } @@ -92,7 +91,7 @@ export class HttpClientResponse implements ifm.IHttpClientResponse { } export function isHttps(requestUrl: string) { - let parsedUrl: url.Url = url.parse(requestUrl) + let parsedUrl: URL = new URL(requestUrl) return parsedUrl.protocol === 'https:' } @@ -322,7 +321,7 @@ export class HttpClient { throw new Error('Client has already been disposed.') } - let parsedUrl = url.parse(requestUrl) + let parsedUrl = new URL(requestUrl) let info: ifm.IRequestInfo = this._prepareRequest(verb, parsedUrl, headers) // Only perform retries on reads since writes may not be idempotent. @@ -371,7 +370,7 @@ export class HttpClient { // if there's no location to redirect to, we won't break } - let parsedRedirectUrl = url.parse(redirectUrl) + let parsedRedirectUrl = new URL(redirectUrl) if ( parsedUrl.protocol == 'https:' && parsedUrl.protocol != parsedRedirectUrl.protocol && @@ -516,13 +515,13 @@ export class HttpClient { * @param serverUrl The server URL where the request will be sent. For example, https://api.github.com */ public getAgent(serverUrl: string): http.Agent { - let parsedUrl = url.parse(serverUrl) + let parsedUrl = new URL(serverUrl) return this._getAgent(parsedUrl) } private _prepareRequest( method: string, - requestUrl: url.Url, + requestUrl: URL, headers: ifm.IHeaders ): ifm.IRequestInfo { const info: ifm.IRequestInfo = {} @@ -587,9 +586,9 @@ export class HttpClient { return additionalHeaders[header] || clientHeader || _default } - private _getAgent(parsedUrl: url.Url): http.Agent { + private _getAgent(parsedUrl: URL): http.Agent { let agent - let proxyUrl: url.Url = pm.getProxyUrl(parsedUrl) + let proxyUrl: URL = pm.getProxyUrl(parsedUrl) let useProxy = proxyUrl && proxyUrl.hostname if (this._keepAlive && useProxy) { @@ -621,7 +620,7 @@ export class HttpClient { maxSockets: maxSockets, keepAlive: this._keepAlive, proxy: { - proxyAuth: proxyUrl.auth, + proxyAuth: `${proxyUrl.username}:${proxyUrl.password}`, host: proxyUrl.hostname, port: proxyUrl.port } diff --git a/interfaces.ts b/interfaces.ts index 84d9fdb..b1dabaf 100644 --- a/interfaces.ts +++ b/interfaces.ts @@ -1,5 +1,4 @@ import http = require('http') -import url = require('url') export interface IHeaders { [key: string]: any @@ -73,7 +72,7 @@ export interface IHttpClientResponse { export interface IRequestInfo { options: http.RequestOptions - parsedUrl: url.Url + parsedUrl: URL httpModule: any } diff --git a/proxy.ts b/proxy.ts index 1965cb7..bd3e83b 100644 --- a/proxy.ts +++ b/proxy.ts @@ -1,9 +1,7 @@ -import * as url from 'url' - -export function getProxyUrl(reqUrl: url.Url): url.Url | undefined { +export function getProxyUrl(reqUrl: URL): URL | undefined { let usingSsl = reqUrl.protocol === 'https:' - let proxyUrl: url.Url + let proxyUrl: URL if (checkBypass(reqUrl)) { return proxyUrl } @@ -16,13 +14,13 @@ export function getProxyUrl(reqUrl: url.Url): url.Url | undefined { } if (proxyVar) { - proxyUrl = url.parse(proxyVar) + proxyUrl = new URL(proxyVar) } return proxyUrl } -export function checkBypass(reqUrl: url.Url): boolean { +export function checkBypass(reqUrl: URL): boolean { if (!reqUrl.hostname) { return false } From fbd137758ab602b6980e3c12eaa5d48a64c07e6f Mon Sep 17 00:00:00 2001 From: Bryan MacFarlane Date: Thu, 23 Apr 2020 16:26:28 -0400 Subject: [PATCH 2/7] fix and tests --- __tests__/basics.test.ts | 44 ++++++++++++++++++++++++++++++++++++++++ index.ts | 10 +++++++++ 2 files changed, 54 insertions(+) diff --git a/__tests__/basics.test.ts b/__tests__/basics.test.ts index 20e910a..785af20 100644 --- a/__tests__/basics.test.ts +++ b/__tests__/basics.test.ts @@ -179,6 +179,50 @@ describe('basics', () => { done() }) + it('does not pass auth with diff hostname redirects', async done => { + let headers = { + "accept": "application/json", + "authorization": "shhh" + } + let res: httpm.HttpClientResponse = await _http.get( + 'https://httpbin.org/redirect-to?url=' + + encodeURIComponent('https://www.httpbin.org/get'), + headers + ) + + expect(res.message.statusCode).toBe(200) + let body: string = await res.readBody() + let obj: any = JSON.parse(body) + // httpbin "fixes" the casing + expect(obj.headers["Authorization"]).toBeUndefined() + expect(obj.headers["authorization"]).toBeUndefined() + expect(obj.url).toBe('https://www.httpbin.org/get') + + done() + }) + + it('does not pass Auth with diff hostname redirects', async done => { + let headers = { + "Accept": "application/json", + "Authorization": "shhh" + } + let res: httpm.HttpClientResponse = await _http.get( + 'https://httpbin.org/redirect-to?url=' + + encodeURIComponent('https://www.httpbin.org/get'), + headers + ) + + expect(res.message.statusCode).toBe(200) + let body: string = await res.readBody() + let obj: any = JSON.parse(body) + // httpbin "fixes" the casing + expect(obj.headers["Authorization"]).toBeUndefined() + expect(obj.headers["authorization"]).toBeUndefined() + expect(obj.url).toBe('https://www.httpbin.org/get') + + done() + }) + it('does basic head request', async done => { let res: httpm.HttpClientResponse = await _http.head( 'http://httpbin.org/get' diff --git a/index.ts b/index.ts index a1f5491..76465b0 100644 --- a/index.ts +++ b/index.ts @@ -386,6 +386,16 @@ export class HttpClient { // which will leak the open socket. await response.readBody() + // strip authorization header if redirected to a different hostname + if (parsedRedirectUrl.hostname !== parsedUrl.hostname) { + for(let header in headers){ + // header names are case insensitive + if (header.toLowerCase() === "authorization") { + delete headers[header] + } + } + } + // let's make the request with the new redirectUrl info = this._prepareRequest(verb, parsedRedirectUrl, headers) response = await this.requestRaw(info, data) From 943067fe4cc39d1edb36c04478359d56ee21adf8 Mon Sep 17 00:00:00 2001 From: Bryan MacFarlane Date: Thu, 23 Apr 2020 16:33:34 -0400 Subject: [PATCH 3/7] it's pretty now and bump version --- __tests__/basics.test.ts | 24 ++++++++++++------------ index.ts | 6 +++--- package.json | 2 +- 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/__tests__/basics.test.ts b/__tests__/basics.test.ts index 785af20..fd171db 100644 --- a/__tests__/basics.test.ts +++ b/__tests__/basics.test.ts @@ -181,47 +181,47 @@ describe('basics', () => { it('does not pass auth with diff hostname redirects', async done => { let headers = { - "accept": "application/json", - "authorization": "shhh" + accept: 'application/json', + authorization: 'shhh' } let res: httpm.HttpClientResponse = await _http.get( 'https://httpbin.org/redirect-to?url=' + encodeURIComponent('https://www.httpbin.org/get'), - headers + headers ) expect(res.message.statusCode).toBe(200) let body: string = await res.readBody() let obj: any = JSON.parse(body) // httpbin "fixes" the casing - expect(obj.headers["Authorization"]).toBeUndefined() - expect(obj.headers["authorization"]).toBeUndefined() + expect(obj.headers['Authorization']).toBeUndefined() + expect(obj.headers['authorization']).toBeUndefined() expect(obj.url).toBe('https://www.httpbin.org/get') done() }) - + it('does not pass Auth with diff hostname redirects', async done => { let headers = { - "Accept": "application/json", - "Authorization": "shhh" + Accept: 'application/json', + Authorization: 'shhh' } let res: httpm.HttpClientResponse = await _http.get( 'https://httpbin.org/redirect-to?url=' + encodeURIComponent('https://www.httpbin.org/get'), - headers + headers ) expect(res.message.statusCode).toBe(200) let body: string = await res.readBody() let obj: any = JSON.parse(body) // httpbin "fixes" the casing - expect(obj.headers["Authorization"]).toBeUndefined() - expect(obj.headers["authorization"]).toBeUndefined() + expect(obj.headers['Authorization']).toBeUndefined() + expect(obj.headers['authorization']).toBeUndefined() expect(obj.url).toBe('https://www.httpbin.org/get') done() - }) + }) it('does basic head request', async done => { let res: httpm.HttpClientResponse = await _http.head( diff --git a/index.ts b/index.ts index 76465b0..e3b9854 100644 --- a/index.ts +++ b/index.ts @@ -388,10 +388,10 @@ export class HttpClient { // strip authorization header if redirected to a different hostname if (parsedRedirectUrl.hostname !== parsedUrl.hostname) { - for(let header in headers){ + for (let header in headers) { // header names are case insensitive - if (header.toLowerCase() === "authorization") { - delete headers[header] + if (header.toLowerCase() === 'authorization') { + delete headers[header] } } } diff --git a/package.json b/package.json index 49f2e08..e57d099 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@actions/http-client", - "version": "1.0.7", + "version": "1.0.8", "description": "Actions Http Client", "main": "index.js", "scripts": { From cde0b32926ae7938c148121d11271f4cb1f5a57f Mon Sep 17 00:00:00 2001 From: Bryan MacFarlane Date: Thu, 23 Apr 2020 16:50:19 -0400 Subject: [PATCH 4/7] cr feedback --- __tests__/basics.test.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/__tests__/basics.test.ts b/__tests__/basics.test.ts index fd171db..70e9709 100644 --- a/__tests__/basics.test.ts +++ b/__tests__/basics.test.ts @@ -194,6 +194,7 @@ describe('basics', () => { let body: string = await res.readBody() let obj: any = JSON.parse(body) // httpbin "fixes" the casing + expect(obj.headers['Accept']).toBe('application/json') expect(obj.headers['Authorization']).toBeUndefined() expect(obj.headers['authorization']).toBeUndefined() expect(obj.url).toBe('https://www.httpbin.org/get') @@ -216,6 +217,7 @@ describe('basics', () => { let body: string = await res.readBody() let obj: any = JSON.parse(body) // httpbin "fixes" the casing + expect(obj.headers['Accept']).toBe('application/json') expect(obj.headers['Authorization']).toBeUndefined() expect(obj.headers['authorization']).toBeUndefined() expect(obj.url).toBe('https://www.httpbin.org/get') From 3e37a9d2928a32b465cdbd0fe9d6bc0eefe60bae Mon Sep 17 00:00:00 2001 From: Bryan MacFarlane Date: Wed, 29 Apr 2020 21:06:11 -0400 Subject: [PATCH 5/7] Update RELEASES.md --- RELEASES.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/RELEASES.md b/RELEASES.md index 79f9a58..ebee9fc 100644 --- a/RELEASES.md +++ b/RELEASES.md @@ -1,5 +1,8 @@ ## Releases +## 1.0.8 +Fixed security issue where a redirect (e.g. 302) to another domain would pass headers. The fix was to strip the authorization header if the hostname was different. More [details in PR #27](https://github.com/actions/http-client/pull/27) + ## 1.0.7 Update NPM dependencies and add 429 to the list of HttpCodes @@ -13,4 +16,4 @@ Adds \Json() helper methods for json over http scenarios. Started to add \Json() helper methods. Do not use this release for that. Use >= 1.0.5 since there was an issue with types. ## 1.0.1 to 1.0.3 -Adds proxy support. \ No newline at end of file +Adds proxy support. From 647e77eb6080da8903ab205afa490f53adb1b871 Mon Sep 17 00:00:00 2001 From: Andy McKay Date: Tue, 21 Jul 2020 08:46:10 -0700 Subject: [PATCH 6/7] Rename to work with main Updates the Action to listen to main #28. I didn't spot much else in here that we need to do yet. --- .github/workflows/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a3b9963..cbe12db 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -2,7 +2,7 @@ name: http-tests on: push: branches: - - master + - main paths-ignore: - '**.md' pull_request: From 91c9096197daef204fee1b05d44a5fa274a6a5cd Mon Sep 17 00:00:00 2001 From: Dave Hadka Date: Mon, 17 Aug 2020 22:18:54 -0500 Subject: [PATCH 7/7] Disable redirect tests due to httpbin issue --- __tests__/basics.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/__tests__/basics.test.ts b/__tests__/basics.test.ts index 70e9709..7790f4d 100644 --- a/__tests__/basics.test.ts +++ b/__tests__/basics.test.ts @@ -128,7 +128,7 @@ describe('basics', () => { }) }) - it('does basic get request with redirects', async done => { + it.skip('does basic get request with redirects', async done => { let res: httpm.HttpClientResponse = await _http.get( 'https://httpbin.org/redirect-to?url=' + encodeURIComponent('https://httpbin.org/get') @@ -140,7 +140,7 @@ describe('basics', () => { done() }) - it('does basic get request with redirects (303)', async done => { + it.skip('does basic get request with redirects (303)', async done => { let res: httpm.HttpClientResponse = await _http.get( 'https://httpbin.org/redirect-to?url=' + encodeURIComponent('https://httpbin.org/get') + @@ -164,7 +164,7 @@ describe('basics', () => { done() }) - it('does not follow redirects if disabled', async done => { + it.skip('does not follow redirects if disabled', async done => { let http: httpm.HttpClient = new httpm.HttpClient( 'typed-test-client-tests', null, @@ -179,7 +179,7 @@ describe('basics', () => { done() }) - it('does not pass auth with diff hostname redirects', async done => { + it.skip('does not pass auth with diff hostname redirects', async done => { let headers = { accept: 'application/json', authorization: 'shhh' @@ -202,7 +202,7 @@ describe('basics', () => { done() }) - it('does not pass Auth with diff hostname redirects', async done => { + it.skip('does not pass Auth with diff hostname redirects', async done => { let headers = { Accept: 'application/json', Authorization: 'shhh'