From 0e4644add7776194fe88da1b0d71de13dd1480b2 Mon Sep 17 00:00:00 2001 From: Kanad Gupta <8854718+kanadgupta@users.noreply.github.com> Date: Mon, 28 Nov 2022 18:54:57 -0600 Subject: [PATCH] feat: proxy support (#681) * test: rename test for clarity * chore: add lint rule to prevent raw node-fetch use also reformatting eslintrc and adding contrib docs on this change * feat: add proxy support * chore: fix TOC --- .eslintrc | 35 ++++++++++++++++-------- CONTRIBUTING.md | 6 +++++ README.md | 10 +++++++ __tests__/helpers/get-api-mock.ts | 9 +++++-- __tests__/lib/fetch.test.ts | 45 ++++++++++++++++++++++++++++++- src/lib/fetch.ts | 17 ++++++++++-- src/lib/getPkgVersion.ts | 1 + 7 files changed, 107 insertions(+), 16 deletions(-) diff --git a/.eslintrc b/.eslintrc index 8fceff89e..2fbbc6957 100644 --- a/.eslintrc +++ b/.eslintrc @@ -1,8 +1,5 @@ { - "extends": [ - "@readme/eslint-config", - "@readme/eslint-config/typescript" - ], + "extends": ["@readme/eslint-config", "@readme/eslint-config/typescript"], "root": true, "parserOptions": { "ecmaVersion": 2020 @@ -16,13 +13,16 @@ } ], "rules": { - "@typescript-eslint/ban-types": ["error", { - "types": { - // We specify `{}` in `CommandOptions` generics when those commands don't have their own - // options and it's cleaner for us to do that than `Record`. - "{}": false + "@typescript-eslint/ban-types": [ + "error", + { + "types": { + // We specify `{}` in `CommandOptions` generics when those commands don't have their own + // options and it's cleaner for us to do that than `Record`. + "{}": false + } } - }], + ], /** * Because our command classes have a `run` method that might not always call `this` we need to @@ -45,6 +45,19 @@ */ "no-console": "warn", - "no-restricted-syntax": "off" + "no-restricted-syntax": "off", + + "no-restricted-imports": [ + "error", + { + "paths": [ + { + "name": "node-fetch", + "importNames": ["default"], + "message": "Avoid using `node-fetch` directly and instead use the fetch wrapper located in `lib/fetch.ts`. See CONTRIBUTING.md for more information." + } + ] + } + ] } } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 2f6aa1125..bd2dca517 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -76,6 +76,12 @@ When writing command logic, avoid using `console` statements (and correspondingl +### Making `fetch` requests + +`fetch` requests are very common in this codebase. When sending `fetch` requests to the ReadMe API (i.e., [dash.readme.com](https://dash.readme.com)), make sure to use the `fetch` wrapper function located in [`src/lib/fetch.ts`](src/lib/fetch.ts). We have an ESLint rule to flag this. + +In that wrapper function, we set several important request headers and configure the proxy, if the user added one via `HTTPS_PROXY`. + ### Commit Conventions For our general commit conventions, please consult our organization contributing guidelines [here](https://github.com/readmeio/.github/blob/main/.github/CONTRIBUTING.md#commit-conventions). diff --git a/README.md b/README.md index 073c2908c..1220ece19 100644 --- a/README.md +++ b/README.md @@ -35,6 +35,7 @@ https://github.com/jonschlinkert/markdown-toc/issues/119 - [CLI Configuration](#cli-configuration) - [Setup](#setup) - [Authentication](#authentication) + - [Proxy](#proxy) - [GitHub Actions Configuration](#github-actions-configuration) - [Usage](#usage) - [Common `rdme` Options](#common-rdme-options) @@ -94,6 +95,15 @@ For usage in CI environments (GitHub Actions, CircleCI, Travis CI, etc.) or if y `rdme whoami` is also available to you to determine who is logged in, and to what project. You can clear your stored credentials with `rdme logout`. +### Proxy + +`rdme` makes API requests to the ReadMe API, which is located at [dash.readme.com](https://dash.readme.com). If you need to configure a proxy for these requests, you can do so by setting the `HTTPS_PROXY` environmental variable. + +```sh +export HTTPS_PROXY=https://proxy.example.com:5678 +rdme openapi +``` + ## GitHub Actions Configuration > **Note** diff --git a/__tests__/helpers/get-api-mock.ts b/__tests__/helpers/get-api-mock.ts index e363f6482..fc562a591 100644 --- a/__tests__/helpers/get-api-mock.ts +++ b/__tests__/helpers/get-api-mock.ts @@ -3,8 +3,13 @@ import nock from 'nock'; import { getUserAgent } from '../../src/lib/fetch'; -export default function getAPIMock(reqHeaders = {}) { - return nock(config.get('host'), { +/** + * Nock wrapper that adds required `user-agent` request header + * so it gets properly picked up by nock. + * @param proxy Optional proxy URL. Must contain trailing slash. + */ +export default function getAPIMock(reqHeaders = {}, proxy = '') { + return nock(`${proxy}${config.get('host')}`, { reqheaders: { 'User-Agent': getUserAgent(), ...reqHeaders, diff --git a/__tests__/lib/fetch.test.ts b/__tests__/lib/fetch.test.ts index 85b61a99d..633ff1dfa 100644 --- a/__tests__/lib/fetch.test.ts +++ b/__tests__/lib/fetch.test.ts @@ -87,7 +87,7 @@ describe('#fetch()', () => { mock.done(); }); - it('should support if we dont supply any other options with the request', async () => { + it('should make fetch call if no other request options are provided', async () => { const mock = getAPIMock() .get('/api/v1/doesnt-need-auth') .reply(200, function () { @@ -105,6 +105,49 @@ describe('#fetch()', () => { expect(headers['x-github-sha']).toBeUndefined(); mock.done(); }); + + describe('proxies', () => { + afterEach(() => { + delete process.env.https_proxy; + delete process.env.HTTPS_PROXY; + }); + + it('should support proxies via HTTPS_PROXY env variable', async () => { + const proxy = 'https://proxy.example.com:5678'; + + process.env.HTTPS_PROXY = proxy; + + const mock = getAPIMock({}, `${proxy}/`).get('/api/v1/proxy').reply(200); + + await fetch(`${config.get('host')}/api/v1/proxy`); + + expect(mock.isDone()).toBe(true); + }); + + it('should support proxies via https_proxy env variable', async () => { + const proxy = 'https://proxy.example.com:5678'; + + process.env.https_proxy = proxy; + + const mock = getAPIMock({}, `${proxy}/`).get('/api/v1/proxy').reply(200); + + await fetch(`${config.get('host')}/api/v1/proxy`); + + expect(mock.isDone()).toBe(true); + }); + + it('should handle trailing slash in proxy URL', async () => { + const proxy = 'https://proxy.example.com:5678/'; + + process.env.https_proxy = proxy; + + const mock = getAPIMock({}, proxy).get('/api/v1/proxy').reply(200); + + await fetch(`${config.get('host')}/api/v1/proxy`); + + expect(mock.isDone()).toBe(true); + }); + }); }); describe('#cleanHeaders()', () => { diff --git a/src/lib/fetch.ts b/src/lib/fetch.ts index 676216b14..9f5c2702d 100644 --- a/src/lib/fetch.ts +++ b/src/lib/fetch.ts @@ -1,6 +1,7 @@ import type { RequestInit, Response } from 'node-fetch'; import mime from 'mime-types'; +// eslint-disable-next-line no-restricted-imports import nodeFetch, { Headers } from 'node-fetch'; import pkg from '../../package.json'; @@ -11,6 +12,16 @@ import { debug } from './logger'; const SUCCESS_NO_CONTENT = 204; +function getProxy() { + // this is something of an industry standard env var, hence the checks for different casings + const proxy = process.env.HTTPS_PROXY || process.env.https_proxy; + if (proxy) { + // adds trailing slash + return proxy.endsWith('/') ? proxy : `${proxy}/`; + } + return ''; +} + /** * Getter function for a string to be used in the user-agent header based on the current * environment. @@ -46,9 +57,11 @@ export default function fetch(url: string, options: RequestInit = { headers: new headers.set('x-readme-source', source); - debug(`making ${(options.method || 'get').toUpperCase()} request to ${url}`); + const fullUrl = `${getProxy()}${url}`; + + debug(`making ${(options.method || 'get').toUpperCase()} request to ${fullUrl}`); - return nodeFetch(url, { + return nodeFetch(fullUrl, { ...options, headers, }); diff --git a/src/lib/getPkgVersion.ts b/src/lib/getPkgVersion.ts index e5e4bfcc4..27cbddb0d 100644 --- a/src/lib/getPkgVersion.ts +++ b/src/lib/getPkgVersion.ts @@ -1,3 +1,4 @@ +// eslint-disable-next-line no-restricted-imports import fetch from 'node-fetch'; import pkg from '../../package.json';