From 2d84b7d990f7e34a76d2d741d012c4fa5e35cd4f Mon Sep 17 00:00:00 2001 From: Steve Larson <9larsons@gmail.com> Date: Mon, 20 Feb 2023 09:33:11 -0600 Subject: [PATCH] Upgraded got package from v9.6.0 to v11.8.6 (#16261) Refs TryGhost/Team#2459 -upgraded got from v9.6.0 to v11.8.6 to support following redirects (and other fixes) -got v12+ requires ESM, so we do not want to upgrade further at this time -required changes to a few libraries that use externalRequests -mention discovery service tests updated to test for follow redirects --- .../core/core/server/lib/request-external.js | 27 +++---- .../core/server/services/oembed/nft-oembed.js | 3 +- ghost/core/package.json | 2 +- ghost/core/test/e2e-api/admin/oembed.test.js | 34 ++++++++- .../unit/server/lib/request-external.test.js | 11 ++- ghost/members-api/lib/services/geolocation.js | 5 +- ghost/members-api/package.json | 2 +- ghost/oembed-service/lib/oembed-service.js | 63 +++------------- .../lib/MentionDiscoveryService.js | 2 +- .../webmentions/lib/MentionSendingService.js | 8 +- .../test/MentionDiscoveryService.test.js | 24 +++--- .../test/MentionSendingService.test.js | 75 +++++++++---------- 12 files changed, 117 insertions(+), 139 deletions(-) diff --git a/ghost/core/core/server/lib/request-external.js b/ghost/core/core/server/lib/request-external.js index 18fbbf6981..a311d2c3e1 100644 --- a/ghost/core/core/server/lib/request-external.js +++ b/ghost/core/core/server/lib/request-external.js @@ -20,22 +20,32 @@ function isPrivateIp(addr) { async function errorIfHostnameResolvesToPrivateIp(options) { // allow requests through to local Ghost instance const siteUrl = new URL(config.get('url')); - const requestUrl = new URL(options.href); + const requestUrl = new URL(options.url.href); if (requestUrl.host === siteUrl.host) { return Promise.resolve(); } - const result = await dnsPromises.lookup(options.hostname); + const result = await dnsPromises.lookup(options.url.hostname); if (isPrivateIp(result.address)) { return Promise.reject(new errors.InternalServerError({ message: 'URL resolves to a non-permitted private IP block', code: 'URL_PRIVATE_INVALID', - context: options.href + context: options.url.href })); } } +async function errorIfInvalidUrl(options) { + if (!options.url.hostname || !validator.isURL(options.url.hostname)) { + throw new errors.InternalServerError({ + message: 'URL invalid.', + code: 'URL_MISSING_INVALID', + context: options.url.href + }); + } +} + // same as our normal request lib but if any request in a redirect chain resolves // to a private IP address it will be blocked before the request is made. const externalRequest = got.extend({ @@ -44,16 +54,7 @@ const externalRequest = got.extend({ }, timeout: 10000, // default is no timeout hooks: { - init: [(options) => { - if (!options.hostname || !validator.isURL(options.hostname)) { - throw new errors.InternalServerError({ - message: 'URL empty or invalid.', - code: 'URL_MISSING_INVALID', - context: options.href - }); - } - }], - beforeRequest: [errorIfHostnameResolvesToPrivateIp], + beforeRequest: [errorIfInvalidUrl,errorIfHostnameResolvesToPrivateIp], beforeRedirect: [errorIfHostnameResolvesToPrivateIp] } }); diff --git a/ghost/core/core/server/services/oembed/nft-oembed.js b/ghost/core/core/server/services/oembed/nft-oembed.js index bc2a293e72..8f1ec570f5 100644 --- a/ghost/core/core/server/services/oembed/nft-oembed.js +++ b/ghost/core/core/server/services/oembed/nft-oembed.js @@ -42,9 +42,8 @@ class NFTOEmbedProvider { headers['X-API-KEY'] = this.dependencies.config.apiKey; } const result = await externalRequest(`https://api.opensea.io/api/v1/asset/${transaction}/${asset}/?format=json`, { - json: true, headers - }); + }).json(); return { version: '1.0', type: 'nft', diff --git a/ghost/core/package.json b/ghost/core/package.json index 331de0e697..afd590dd3a 100644 --- a/ghost/core/package.json +++ b/ghost/core/package.json @@ -170,7 +170,7 @@ "fs-extra": "11.1.0", "ghost-storage-base": "1.0.0", "glob": "8.1.0", - "got": "9.6.0", + "got": "11.8.6", "gscan": "4.36.0", "human-number": "2.0.1", "image-size": "1.0.2", diff --git a/ghost/core/test/e2e-api/admin/oembed.test.js b/ghost/core/test/e2e-api/admin/oembed.test.js index 54db7dacaf..6f0f2fca48 100644 --- a/ghost/core/test/e2e-api/admin/oembed.test.js +++ b/ghost/core/test/e2e-api/admin/oembed.test.js @@ -128,6 +128,12 @@ describe('Oembed API', function () { }); it('errors when fetched url is an IP address', async function () { + // in order to follow the 302, we need to stub differently; externalRequest will block the internal IP + dnsPromises.lookup.restore(); + let dnsStub = sinon.stub(dnsPromises, 'lookup'); + dnsStub.onCall(0).returns(Promise.resolve({address: '123.123.123.123'})); + dnsStub.onCall(1).returns(Promise.resolve({address: '0.0.0.0'})); + const redirectMock = nock('http://test.com/') .get('/') .reply(302, undefined, {Location: 'http://0.0.0.0:8080'}); @@ -147,7 +153,7 @@ describe('Oembed API', function () { .expect('Cache-Control', testUtils.cacheRules.private) .expect(422); - pageMock.isDone().should.be.true(); + pageMock.isDone().should.be.false(); // we shouldn't hit this; blocked by externalRequest should.exist(res.body.errors); }); @@ -376,6 +382,15 @@ describe('Oembed API', function () { }); it('skips fetching IPv4 addresses', async function () { + dnsPromises.lookup.restore(); + sinon.stub(dnsPromises, 'lookup').callsFake(function (hostname) { + if (hostname === '192.168.0.1') { + return Promise.resolve({address: '192.168.0.1'}); + } else { + return Promise.resolve({address: '123.123.123.123'}); + } + }); + const pageMock = nock('http://test.com') .get('/') .reply(200, ''); @@ -399,6 +414,15 @@ describe('Oembed API', function () { }); it('skips fetching IPv6 addresses', async function () { + dnsPromises.lookup.restore(); + sinon.stub(dnsPromises, 'lookup').callsFake(function (hostname) { + if (hostname === '[2607:f0d0:1002:51::4]') { + return Promise.resolve({address: '192.168.0.1'}); + } else { + return Promise.resolve({address: '123.123.123.123'}); + } + }); + const pageMock = nock('http://test.com') .get('/') .reply(200, ''); @@ -422,6 +446,14 @@ describe('Oembed API', function () { }); it('skips fetching localhost', async function () { + dnsPromises.lookup.restore(); + sinon.stub(dnsPromises, 'lookup').callsFake(function (hostname) { + if (hostname === 'localhost') { + return Promise.resolve({address: '127.0.0.1'}); + } else { + return Promise.resolve({address: '123.123.123.123'}); + } + }); const pageMock = nock('http://test.com') .get('/') .reply(200, ''); diff --git a/ghost/core/test/unit/server/lib/request-external.test.js b/ghost/core/test/unit/server/lib/request-external.test.js index 8ad9818108..62a67e9861 100644 --- a/ghost/core/test/unit/server/lib/request-external.test.js +++ b/ghost/core/test/unit/server/lib/request-external.test.js @@ -263,7 +263,7 @@ describe('External Request', function () { throw new Error('Request should have rejected with invalid url message'); }, (err) => { should.exist(err); - err.message.should.be.equal('URL empty or invalid.'); + err.code.should.be.equal('ERR_INVALID_URL'); }); }); @@ -279,7 +279,8 @@ describe('External Request', function () { throw new Error('Request should have rejected with invalid url message'); }, (err) => { should.exist(err); - err.message.should.be.equal('URL empty or invalid.'); + // got v11+ throws an error instead of the external requests lib + err.message.should.be.equal('No URL protocol specified'); }); }); @@ -300,7 +301,7 @@ describe('External Request', function () { }, (err) => { requestMock.isDone().should.be.true(); should.exist(err); - err.statusMessage.should.be.equal('Not Found'); + err.response.statusMessage.should.be.equal('Not Found'); }); }); @@ -322,9 +323,7 @@ describe('External Request', function () { }, (err) => { requestMock.isDone().should.be.true(); should.exist(err); - err.statusMessage.should.be.equal('Internal Server Error'); - err.body.should.match(/something awful happened/); - err.body.should.match(/AWFUL_ERROR/); + err.response.statusMessage.should.be.equal(`Internal Server Error`); }); }); }); diff --git a/ghost/members-api/lib/services/geolocation.js b/ghost/members-api/lib/services/geolocation.js index 0f745c7ed3..72f7e907c7 100644 --- a/ghost/members-api/lib/services/geolocation.js +++ b/ghost/members-api/lib/services/geolocation.js @@ -7,9 +7,8 @@ module.exports = class GeolocationService { if (!ipAddress || (!IPV4_REGEX.test(ipAddress) && !IPV6_REGEX.test(ipAddress))) { return; } - const geojsUrl = `https://get.geojs.io/v1/ip/geo/${encodeURIComponent(ipAddress)}.json`; - const response = await got(geojsUrl, {json: true, timeout: 500}); - return response.body; + const response = await got(geojsUrl, {timeout: 500}).json(); + return response; } }; diff --git a/ghost/members-api/package.json b/ghost/members-api/package.json index 6ea79044c6..cd3e500cc6 100644 --- a/ghost/members-api/package.json +++ b/ghost/members-api/package.json @@ -41,7 +41,7 @@ "body-parser": "1.20.1", "bson-objectid": "2.0.4", "express": "4.18.2", - "got": "9.6.0", + "got": "11.8.6", "jsonwebtoken": "8.5.1", "lodash": "4.17.21", "moment": "2.29.4", diff --git a/ghost/oembed-service/lib/oembed-service.js b/ghost/oembed-service/lib/oembed-service.js index 1bd7dbd7fe..373592cc0b 100644 --- a/ghost/oembed-service/lib/oembed-service.js +++ b/ghost/oembed-service/lib/oembed-service.js @@ -4,7 +4,6 @@ const logging = require('@tryghost/logging'); const {extract, hasProvider} = require('oembed-parser'); const cheerio = require('cheerio'); const _ = require('lodash'); -const {CookieJar} = require('tough-cookie'); const charset = require('charset'); const iconv = require('iconv-lite'); @@ -68,16 +67,7 @@ class OEmbed { this.config = config; /** @type {IExternalRequest} */ - this.externalRequest = async (url, requestConfig) => { - if (this.isIpOrLocalhost(url)) { - return this.unknownProvider(url); - } - const response = await externalRequest(url, requestConfig); - if (this.isIpOrLocalhost(response.url)) { - return this.unknownProvider(url); - } - return response; - }; + this.externalRequest = externalRequest; /** @type {ICustomProvider[]} */ this.customProviders = []; @@ -117,16 +107,13 @@ class OEmbed { * @param {string} url * @param {Object} options * - * @returns {Promise<{url: string, body: any, headers: any}>} + * @returns {GotPromise} */ - async fetchPage(url, options) { - const cookieJar = new CookieJar(); + fetchPage(url, options) { return this.externalRequest( url, { - cookieJar, - method: 'GET', - timeout: 2 * 1000, + timeout: 2000, followRedirect: true, ...options }); @@ -140,7 +127,7 @@ class OEmbed { async fetchPageHtml(url) { // Fetch url and get response as binary buffer to // avoid implicit cast - const {headers, body, url: responseUrl} = await this.fetchPage( + let {headers, body, url: responseUrl} = await this.fetchPage( url, { encoding: 'binary', @@ -162,7 +149,7 @@ class OEmbed { } const decodedBody = iconv.decode( - Buffer.from(body, 'binary'), encoding); + body, encoding); return { body: decodedBody, @@ -184,12 +171,9 @@ class OEmbed { * @returns {Promise<{url: string, body: Object}>} */ async fetchPageJson(url) { - const {body, url: pageUrl} = await this.fetchPage( - url, - { - json: true - }); - + const res = await this.fetchPage(url, {responseType: 'json'}); + const body = res.body; + const pageUrl = res.url; return { body, url: pageUrl @@ -247,34 +231,6 @@ class OEmbed { }; } - /** - * @param {string} url - * @returns {boolean} - */ - isIpOrLocalhost(url) { - try { - const IPV4_REGEX = /^(?:(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)\.){3}(?:25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)$/; - const IPV6_REGEX = /:/; // fqdns will not have colons - const HTTP_REGEX = /^https?:/i; - - const siteUrl = new URL(this.config.get('url')); - const {protocol, hostname, host} = new URL(url); - - // allow requests to Ghost's own url through - if (siteUrl.host === host) { - return false; - } - - if (!HTTP_REGEX.test(protocol) || hostname === 'localhost' || IPV4_REGEX.test(hostname) || IPV6_REGEX.test(hostname)) { - return true; - } - - return false; - } catch (e) { - return true; - } - } - /** * @param {string} url * @param {string} html @@ -300,7 +256,6 @@ class OEmbed { // fetch oembed response from embedded rel="alternate" url const oembedResponse = await this.fetchPageJson(oembedUrl); - // validate the fetched json against the oembed spec to avoid // leaking non-oembed responses const body = oembedResponse.body; diff --git a/ghost/webmentions/lib/MentionDiscoveryService.js b/ghost/webmentions/lib/MentionDiscoveryService.js index e4dd4b63ee..70185c0ac8 100644 --- a/ghost/webmentions/lib/MentionDiscoveryService.js +++ b/ghost/webmentions/lib/MentionDiscoveryService.js @@ -17,7 +17,7 @@ module.exports = class MentionDiscoveryService { try { const response = await this.#externalRequest(url.href, { throwHttpErrors: false, - followRedirects: true, + followRedirect: true, maxRedirects: 10 }); if (response.statusCode === 404) { diff --git a/ghost/webmentions/lib/MentionSendingService.js b/ghost/webmentions/lib/MentionSendingService.js index c5f3f8ff76..b5a5fc105e 100644 --- a/ghost/webmentions/lib/MentionSendingService.js +++ b/ghost/webmentions/lib/MentionSendingService.js @@ -79,22 +79,24 @@ module.exports = class MentionSendingService { async send({source, target, endpoint}) { logging.info('[Webmention] Sending webmention from ' + source.href + ' to ' + target.href + ' via ' + endpoint.href); + + // default content type is application/x-www-form-encoded which is what we need for the webmentions spec const response = await this.#externalRequest.post(endpoint.href, { - body: { + form: { source: source.href, target: target.href, source_is_ghost: true }, - form: true, throwHttpErrors: false, maxRedirects: 10, followRedirect: true, - methodRewriting: false, // WARNING! this setting has a different meaning in got v12! timeout: 10000 }); + if (response.statusCode >= 200 && response.statusCode < 300) { return; } + throw new errors.BadRequestError({ message: 'Webmention sending failed with status code ' + response.statusCode, statusCode: response.statusCode diff --git a/ghost/webmentions/test/MentionDiscoveryService.test.js b/ghost/webmentions/test/MentionDiscoveryService.test.js index e819a865ce..17c336326f 100644 --- a/ghost/webmentions/test/MentionDiscoveryService.test.js +++ b/ghost/webmentions/test/MentionDiscoveryService.test.js @@ -37,22 +37,20 @@ describe('MentionDiscoveryService', function () { assert.equal(endpoint, null); }); - // TODO: need to support redirects - // it('Follows redirects', async function () { + it('Follows redirects', async function () { + let url = new URL('http://redirector.io/'); + let nextUrl = new URL('http://testpage.com/'); - // let url = new URL('http://redirector.io/'); - // let nextUrl = new URL('http://testpage.com/'); + nock(url.href) + .intercept('/', 'HEAD') + .reply(301, undefined, {location: nextUrl.href}) + .get('/') + .reply(200, 'Very cool site', {'content-type': 'text/html'}); - // let mockRedirect = nock(url.href) - // .intercept("/", "HEAD") - // .reply(301, undefined, { location: nextUrl.href }) - // .get('/') - // .reply(200, 'Very cool site', { 'content-type': 'text/html' }); + let endpoint = await service.getEndpoint(url); - // let endpoint = await service.getEndpoint(url); - - // assert(endpoint instanceof URL); - // }); + assert(endpoint instanceof URL); + }); describe('Can parse headers', function () { it('Returns null for a valid non-html site', async function () { diff --git a/ghost/webmentions/test/MentionSendingService.test.js b/ghost/webmentions/test/MentionSendingService.test.js index bf8d70441b..99d978bc9a 100644 --- a/ghost/webmentions/test/MentionSendingService.test.js +++ b/ghost/webmentions/test/MentionSendingService.test.js @@ -380,6 +380,9 @@ describe('MentionSendingService', function () { describe('send', function () { it('Can handle 202 accepted responses', async function () { + const source = new URL('https://example.com/source'); + const target = new URL('https://target.com/target'); + const endpoint = new URL('https://example.org/webmentions-test'); const scope = nock('https://example.org') .persist() .post('/webmentions-test', `source=${encodeURIComponent('https://example.com/source')}&target=${encodeURIComponent('https://target.com/target')}&source_is_ghost=true`) @@ -387,14 +390,17 @@ describe('MentionSendingService', function () { const service = new MentionSendingService({externalRequest}); await service.send({ - source: new URL('https://example.com/source'), - target: new URL('https://target.com/target'), - endpoint: new URL('https://example.org/webmentions-test') + source: source, + target: target, + endpoint: endpoint }); assert(scope.isDone()); }); it('Can handle 201 created responses', async function () { + const source = new URL('https://example.com/source'); + const target = new URL('https://target.com/target'); + const endpoint = new URL('https://example.org/webmentions-test'); const scope = nock('https://example.org') .persist() .post('/webmentions-test', `source=${encodeURIComponent('https://example.com/source')}&target=${encodeURIComponent('https://target.com/target')}&source_is_ghost=true`) @@ -402,9 +408,9 @@ describe('MentionSendingService', function () { const service = new MentionSendingService({externalRequest}); await service.send({ - source: new URL('https://example.com/source'), - target: new URL('https://target.com/target'), - endpoint: new URL('https://example.org/webmentions-test') + source: source, + target: target, + endpoint: endpoint }); assert(scope.isDone()); }); @@ -439,6 +445,28 @@ describe('MentionSendingService', function () { assert(scope.isDone()); }); + it('Can handle redirect responses', async function () { + const scope = nock('https://example.org') + .persist() + .post('/webmentions-test') + .reply(302, '', { + Location: 'https://example.org/webmentions-test-2' + }); + const scope2 = nock('https://example.org') + .persist() + .post('/webmentions-test-2') + .reply(201); + + const service = new MentionSendingService({externalRequest}); + await service.send({ + source: new URL('https://example.com'), + target: new URL('https://example.com'), + endpoint: new URL('https://example.org/webmentions-test') + }); + assert(scope.isDone()); + assert(scope2.isDone()); + }); + it('Can handle network errors', async function () { const scope = nock('https://example.org') .persist() @@ -454,41 +482,6 @@ describe('MentionSendingService', function () { assert(scope.isDone()); }); - // Redirects are currently not supported by got for POST requests! - //it('Can handle redirect responses', async function () { - // const scope = nock('https://example.org') - // .persist() - // .post('/webmentions-test') - // .reply(302, '', { - // headers: { - // Location: 'https://example.org/webmentions-test-2' - // } - // }); - // const scope2 = nock('https://example.org') - // .persist() - // .post('/webmentions-test-2') - // .reply(201); - // - // const service = new MentionSendingService({externalRequest}); - // await service.send({ - // source: new URL('https://example.com'), - // target: new URL('https://example.com'), - // endpoint: new URL('https://example.org/webmentions-test') - // }); - // assert(scope.isDone()); - // assert(scope2.isDone()); - //}); - // TODO: also check if we don't follow private IPs after redirects - - it('Does not send to private IP', async function () { - const service = new MentionSendingService({externalRequest}); - await assert.rejects(service.send({ - source: new URL('https://example.com/source'), - target: new URL('https://target.com/target'), - endpoint: new URL('http://localhost/webmentions') - }), /non-permitted private IP/); - }); - it('Does not send to private IP behind DNS', async function () { // Test that we don't make a request when a domain resolves to a private IP // domaincontrol.com -> 127.0.0.1