diff --git a/core/base-service/base.js b/core/base-service/base.js index 36201cef47f8361c162a6df87097c23f2efc6f11..8986cbf239f5cf1ec0181d07e2260c391122bf47 100644 --- a/core/base-service/base.js +++ b/core/base-service/base.js @@ -444,7 +444,7 @@ class BaseService { regex, handleRequest(cacheHeaderConfig, { queryParams, - handler: async (queryParams, match, sendBadge, request) => { + handler: async (queryParams, match, sendBadge) => { const metricHandle = metricHelper.startRequest() const namedParams = namedParamsForMatch(captureNames, match, this) @@ -473,7 +473,6 @@ class BaseService { metricHandle.noteResponseSent() }, cacheLength: this._cacheLength, - fetchLimitBytes, }) ) } diff --git a/core/base-service/got.js b/core/base-service/got.js index 3f7f766df72ab5ef645570f56864358659791000..b49591f4764b5d05a869afdecd2ef8b810ab12bc 100644 --- a/core/base-service/got.js +++ b/core/base-service/got.js @@ -94,4 +94,4 @@ function fetchFactory(fetchLimitBytes = TEN_MB) { return sendRequest.bind(sendRequest, gotWithLimit) } -export { requestOptions2GotOptions, fetchFactory } +export { requestOptions2GotOptions, fetchFactory, userAgent } diff --git a/core/base-service/legacy-request-handler.js b/core/base-service/legacy-request-handler.js index 725b1d66179e1c9df8bc1580032afa5308865dd3..2e5b7e92b9a2aaf67c691e6d77198bce3479f60e 100644 --- a/core/base-service/legacy-request-handler.js +++ b/core/base-service/legacy-request-handler.js @@ -1,12 +1,8 @@ -import request from 'request' import makeBadge from '../../badge-maker/lib/make-badge.js' import { setCacheHeaders } from './cache-headers.js' -import { Inaccessible, InvalidResponse, ShieldsRuntimeError } from './errors.js' import { makeSend } from './legacy-result-sender.js' import coalesceBadge from './coalesce-badge.js' -const userAgent = 'Shields.io/2003a' - // These query parameters are available to any badge. They are handled by // `coalesceBadge`. const globalQueryParams = new Set([ @@ -32,32 +28,12 @@ function flattenQueryParams(queryParams) { return Array.from(union).sort() } -function promisify(cachingRequest) { - return (uri, options) => - new Promise((resolve, reject) => { - cachingRequest(uri, options, (err, res, buffer) => { - if (err) { - if (err instanceof ShieldsRuntimeError) { - reject(err) - } else { - // Wrap the error in an Inaccessible so it can be identified - // by the BaseService handler. - reject(new Inaccessible({ underlyingError: err })) - } - } else { - resolve({ res, buffer }) - } - }) - }) -} - // handlerOptions can contain: // - handler: The service's request handler function // - queryParams: An array of the field names of any custom query parameters // the service uses // - cacheLength: An optional badge or category-specific cache length // (in number of seconds) to be used in preference to the default -// - fetchLimitBytes: A limit on the response size we're willing to parse // // For safety, the service must declare the query parameters it wants to use. // Only the declared parameters (and the global parameters) are provided to @@ -77,8 +53,7 @@ function handleRequest(cacheHeaderConfig, handlerOptions) { } const allowedKeys = flattenQueryParams(handlerOptions.queryParams) - const { cacheLength: serviceDefaultCacheLengthSeconds, fetchLimitBytes } = - handlerOptions + const { cacheLength: serviceDefaultCacheLengthSeconds } = handlerOptions return (queryParams, match, end, ask) => { /* @@ -139,40 +114,6 @@ function handleRequest(cacheHeaderConfig, handlerOptions) { makeSend(extension, ask.res, end)(svg) }, 25000) - function cachingRequest(uri, options, callback) { - if (typeof options === 'function' && !callback) { - callback = options - } - if (options && typeof options === 'object') { - options.uri = uri - } else if (typeof uri === 'string') { - options = { uri } - } else { - options = uri - } - options.headers = options.headers || {} - options.headers['User-Agent'] = userAgent - - let bufferLength = 0 - const r = request(options, callback) - r.on('data', chunk => { - bufferLength += chunk.length - if (bufferLength > fetchLimitBytes) { - r.abort() - r.emit( - 'error', - new InvalidResponse({ - prettyMessage: 'Maximum response size exceeded', - }) - ) - } - }) - } - - // Wrapper around `cachingRequest` that returns a promise rather than needing - // to pass a callback. - cachingRequest.asPromise = promisify(cachingRequest) - const result = handlerOptions.handler( filteredQueryParams, match, @@ -187,8 +128,7 @@ function handleRequest(cacheHeaderConfig, handlerOptions) { const svg = makeBadge(badgeData) setCacheHeadersOnResponse(ask.res, badgeData.cacheLengthSeconds) makeSend(format, ask.res, end)(svg) - }, - cachingRequest + } ) // eslint-disable-next-line promise/prefer-await-to-then if (result && result.catch) { @@ -200,4 +140,4 @@ function handleRequest(cacheHeaderConfig, handlerOptions) { } } -export { handleRequest, promisify, userAgent } +export { handleRequest } diff --git a/core/base-service/legacy-request-handler.spec.js b/core/base-service/legacy-request-handler.spec.js index 1ea498f72a2f205e98cabdfac00fa4e1d0b6ee77..2265f18e69eb39d67d23dfb584a8d287aa81230e 100644 --- a/core/base-service/legacy-request-handler.spec.js +++ b/core/base-service/legacy-request-handler.spec.js @@ -1,5 +1,4 @@ import { expect } from 'chai' -import nock from 'nock' import portfinder from 'portfinder' import Camp from '@shields_io/camp' import got from '../got-test-client.js' @@ -42,28 +41,6 @@ function createFakeHandlerWithCacheLength(cacheLengthSeconds) { } } -function fakeHandlerWithNetworkIo(queryParams, match, sendBadge, request) { - const [, someValue, format] = match - request('https://www.google.com/foo/bar', (err, res, buffer) => { - let message - if (err) { - message = err.prettyMessage - } else { - message = someValue - } - const badgeData = coalesceBadge( - queryParams, - { - label: 'testing', - message, - format, - }, - {} - ) - sendBadge(format, badgeData) - }) -} - describe('The request handler', function () { let port, baseUrl beforeEach(async function () { @@ -133,60 +110,6 @@ describe('The request handler', function () { }) }) - describe('the response size limit', function () { - beforeEach(function () { - camp.route( - /^\/testing\/([^/]+)\.(svg|png|gif|jpg|json)$/, - handleRequest(standardCacheHeaders, { - handler: fakeHandlerWithNetworkIo, - fetchLimitBytes: 100, - }) - ) - }) - - it('should not throw an error if the response <= fetchLimitBytes', async function () { - nock('https://www.google.com') - .get('/foo/bar') - .once() - .reply(200, 'x'.repeat(100)) - const { statusCode, body } = await got(`${baseUrl}/testing/123.json`, { - responseType: 'json', - }) - expect(statusCode).to.equal(200) - expect(body).to.deep.equal({ - name: 'testing', - value: '123', - label: 'testing', - message: '123', - color: 'lightgrey', - link: [], - }) - }) - - it('should throw an error if the response is > fetchLimitBytes', async function () { - nock('https://www.google.com') - .get('/foo/bar') - .once() - .reply(200, 'x'.repeat(101)) - const { statusCode, body } = await got(`${baseUrl}/testing/123.json`, { - responseType: 'json', - }) - expect(statusCode).to.equal(200) - expect(body).to.deep.equal({ - name: 'testing', - value: 'Maximum response size exceeded', - label: 'testing', - message: 'Maximum response size exceeded', - color: 'lightgrey', - link: [], - }) - }) - - afterEach(function () { - nock.cleanAll() - }) - }) - describe('caching', function () { describe('standard query parameters', function () { function register({ cacheHeaderConfig }) { diff --git a/doc/code-walkthrough.md b/doc/code-walkthrough.md index 58437a035ef185731d8d81a825c5dd6770c6fbb4..4797d7cfd104df1f8b9049448750f7371813dfae 100644 --- a/doc/code-walkthrough.md +++ b/doc/code-walkthrough.md @@ -96,7 +96,7 @@ test this kind of logic through unit tests (e.g. of `render()` and is created in a legacy helper function in [`legacy-request-handler.js`][legacy-request-handler]. This callback delegates to a callback in `BaseService.register` with four different - parameters `( queryParams, match, sendBadge, request )`, which + parameters `( queryParams, match, sendBadge )`, which then runs `BaseService.invoke`. `BaseService.invoke` instantiates the service and runs `BaseService#handle`. @@ -129,12 +129,12 @@ test this kind of logic through unit tests (e.g. of `render()` and handle unresponsive service code and the next callback is invoked: the legacy handler function. 3. The legacy handler function receives - `( queryParams, match, sendBadge, request )`. Its job is to extract data - from the regex `match` and `queryParams`, invoke `request` to fetch - whatever data it needs, and then invoke `sendBadge` with the result. + `( queryParams, match, sendBadge )`. Its job is to extract data + from the regex `match` and `queryParams`, and then invoke `sendBadge` + with the result. 4. The implementation of this function is in `BaseService.register`. It works by running `BaseService.invoke`, which instantiates the service, - injects more dependencies, and invokes `BaseService#handle` which is + injects more dependencies, and invokes `BaseService.handle` which is implemented by the service subclass. 5. The job of `handle()`, which should be implemented by each service subclass, is to return an object which partially describes a badge or diff --git a/services/github/auth/acceptor.js b/services/github/auth/acceptor.js index 80347be581ca2f31bded9d33059fda4108cb6465..b27114d76cd3d73f3c079b09ab601f2412bad340 100644 --- a/services/github/auth/acceptor.js +++ b/services/github/auth/acceptor.js @@ -1,6 +1,6 @@ import queryString from 'query-string' import request from 'request' -import { userAgent } from '../../../core/base-service/legacy-request-handler.js' +import { userAgent } from '../../../core/base-service/got.js' import log from '../../../core/server/log.js' function setRoutes({ server, authHelper, onTokenAccepted }) { diff --git a/services/github/github-api-provider.js b/services/github/github-api-provider.js index 5d13192d69b95f0b9dfd28d1865b08093a05d53b..aed1994e65c18cf93bd3f23aebf0a5a714d12d60 100644 --- a/services/github/github-api-provider.js +++ b/services/github/github-api-provider.js @@ -1,7 +1,7 @@ import Joi from 'joi' import log from '../../core/server/log.js' import { TokenPool } from '../../core/token-pooling/token-pool.js' -import { userAgent } from '../../core/base-service/legacy-request-handler.js' +import { userAgent } from '../../core/base-service/got.js' import { nonNegativeInteger } from '../validators.js' import { ImproperlyConfigured } from '../index.js' diff --git a/services/librariesio/librariesio-api-provider.js b/services/librariesio/librariesio-api-provider.js index f112e2247d93608ba7d815a781d7634d82966918..8dd9da64c92c75a4b8ad61ecfbb7cb9e7e4107d4 100644 --- a/services/librariesio/librariesio-api-provider.js +++ b/services/librariesio/librariesio-api-provider.js @@ -1,7 +1,7 @@ import { ImproperlyConfigured } from '../index.js' import log from '../../core/server/log.js' import { TokenPool } from '../../core/token-pooling/token-pool.js' -import { userAgent } from '../../core/base-service/legacy-request-handler.js' +import { userAgent } from '../../core/base-service/got.js' // Provides an interface to the Libraries.io API. export default class LibrariesIoApiProvider {