diff --git a/package.json b/package.json index 7106bc17..f133d69d 100644 --- a/package.json +++ b/package.json @@ -65,8 +65,7 @@ "path-to-regexp": "^6.2.1", "pino": "8.11.0", "pino-pretty": "10.2.0", - "require-in-the-middle": "^5.1.0", - "url-parse": "^1.5.10" + "require-in-the-middle": "^5.1.0" }, "devDependencies": { "@ava/typescript": "^1.1.1", @@ -76,7 +75,6 @@ "@types/express": "^4.17.9", "@types/lodash": "^4.14.166", "@types/node": "^14.14.14", - "@types/url-parse": "^1.4.11", "@typescript-eslint/eslint-plugin": "^4.0.1", "@typescript-eslint/parser": "^4.0.1", "ava": "^3.12.1", diff --git a/src/enforcement/enforcer.ts b/src/enforcement/enforcer.ts index f5218daf..ed2783bf 100644 --- a/src/enforcement/enforcer.ts +++ b/src/enforcement/enforcer.ts @@ -1,6 +1,5 @@ import axios, { AxiosInstance } from 'axios'; import { Logger } from 'pino'; -import URL from 'url-parse'; import { IPermitConfig } from '../config'; import { CheckConfig, Context, ContextStore } from '../utils/context'; @@ -120,6 +119,34 @@ export interface IEnforcer { ): Promise; } +/** + * Builds the OPA client base URL from the configured PDP URL by forcing the OPA + * port (8181) and appending the OPA data path, using the native WHATWG `URL` + * (Node >= 10) in place of the previous `url-parse` dependency (#106). + * + * @param pdp - The configured PDP base URL (e.g. `http://localhost:7766`). + * @returns The OPA base URL (e.g. `http://localhost:8181/v1/data/permit/`). A PDP + * path with no trailing slash is glued to the data path (`/prefix` -> + * `/prefixv1/data/permit/`), preserved from `url-parse` and locked by a test. + * @throws {PermitError} on input without a scheme (e.g. `localhost`); a `host:port` + * value like `localhost:7766` is misparsed, not rejected — pass a full URL. The + * error omits the configured value because it may carry credentials. + */ +export function buildOpaBaseUrl(pdp: string): string { + let opaBaseUrl: URL; + try { + opaBaseUrl = new URL(pdp); + } catch { + throw new PermitError( + 'Invalid PDP URL in the "pdp" option: expected an absolute http(s) URL, ' + + 'e.g. "http://localhost:7766".', + ); + } + opaBaseUrl.port = '8181'; + opaBaseUrl.pathname = `${opaBaseUrl.pathname}v1/data/permit/`; + return opaBaseUrl.toString(); +} + /** * The {@link Enforcer} class is responsible for performing permission checks against the PDP. * It implements the {@link IEnforcer} interface. @@ -135,9 +162,7 @@ export class Enforcer implements IEnforcer { * @param logger - The logger instance for logging. */ constructor(private config: IPermitConfig, private logger: Logger) { - const opaBaseUrl = new URL(this.config.pdp); - opaBaseUrl.set('port', '8181'); - opaBaseUrl.set('pathname', `${opaBaseUrl.pathname}v1/data/permit/`); + const opaBaseUrl = buildOpaBaseUrl(this.config.pdp); const version = process.env.npm_package_version ?? 'unknown'; // PDP gets its own dedicated axios instance so PDP-only POST retries never // apply to the shared REST API client (config.axiosInstance) — REST writes @@ -148,11 +173,11 @@ export class Enforcer implements IEnforcer { }); if (config.opaAxiosInstance) { this.opaClient = config.opaAxiosInstance; - this.opaClient.defaults.baseURL = opaBaseUrl.toString(); + this.opaClient.defaults.baseURL = opaBaseUrl; this.opaClient.defaults.headers.common['X-Permit-SDK-Version'] = `node:${version}`; } else { this.opaClient = axios.create({ - baseURL: opaBaseUrl.toString(), + baseURL: opaBaseUrl, headers: { 'X-Permit-SDK-Version': `node:${version}`, }, diff --git a/src/tests/unit/enforcer.spec.ts b/src/tests/unit/enforcer.spec.ts new file mode 100644 index 00000000..3b64a1de --- /dev/null +++ b/src/tests/unit/enforcer.spec.ts @@ -0,0 +1,111 @@ +import test from 'ava'; +import axios, { AxiosInstance, InternalAxiosRequestConfig } from 'axios'; +import pino from 'pino'; + +import { buildOpaBaseUrl } from '../../enforcement/enforcer'; +import { Permit, PermitError } from '../../index'; + +// The OPA client base URL is derived from the configured PDP URL by forcing the +// OPA port (8181) and appending the OPA data path. This was previously built +// with the `url-parse` package and is now built with the native WHATWG `URL` +// (#106). For absolute http(s) PDP URLs in canonical form the result matches what +// url-parse produced, and these assertions lock that string. WHATWG parsing also +// normalizes input that url-parse kept verbatim (dot segments, percent-encoding, +// IDN hosts); the dot-segment test below pins that intended difference. + +test('buildOpaBaseUrl: default PDP', (t) => { + t.is(buildOpaBaseUrl('http://localhost:7766'), 'http://localhost:8181/v1/data/permit/'); +}); + +test('buildOpaBaseUrl: trailing slash yields the same URL as no trailing slash', (t) => { + t.is(buildOpaBaseUrl('http://localhost:7766/'), 'http://localhost:8181/v1/data/permit/'); +}); + +test('buildOpaBaseUrl: https host without an explicit port', (t) => { + t.is(buildOpaBaseUrl('https://pdp.example.com'), 'https://pdp.example.com:8181/v1/data/permit/'); +}); + +test('buildOpaBaseUrl: an existing port is overridden with 8181', (t) => { + t.is( + buildOpaBaseUrl('https://pdp.example.com:1234'), + 'https://pdp.example.com:8181/v1/data/permit/', + ); +}); + +test('buildOpaBaseUrl: a path prefix preserves the existing concatenation behaviour', (t) => { + // Pre-existing url-parse quirk: a path with no trailing slash glues onto the data path. + t.is( + buildOpaBaseUrl('http://localhost:7766/prefix'), + 'http://localhost:8181/prefixv1/data/permit/', + ); + t.is( + buildOpaBaseUrl('http://localhost:7766/prefix/'), + 'http://localhost:8181/prefix/v1/data/permit/', + ); +}); + +test('buildOpaBaseUrl: dot segments in the PDP path are resolved before the data path', (t) => { + // url-parse kept them verbatim and produced `/a/..v1/data/permit/`. + t.is( + buildOpaBaseUrl('https://pdp.example.com/a/..'), + 'https://pdp.example.com:8181/v1/data/permit/', + ); +}); + +test('buildOpaBaseUrl: a PDP without a scheme throws a PermitError naming the pdp option', (t) => { + // Bare hosts and `//host:port` throw at construction; `localhost:7766` is misparsed, + // not rejected. + for (const pdp of ['localhost', '//localhost:7766']) { + t.throws(() => buildOpaBaseUrl(pdp), { + instanceOf: PermitError, + message: /"pdp" option.*absolute http\(s\) URL/, + }); + } +}); + +test('new Permit does not expose credentials from an invalid PDP URL', (t) => { + const error = t.throws( + () => new Permit({ token: 'test-token', pdp: 'http://pdp-user:pdp-secret@localhost:bad' }), + { instanceOf: PermitError }, + ); + // Serialize the way the SDK's pino logger would, so enumerable error fields are covered too. + t.false(JSON.stringify(pino.stdSerializers.err(error)).includes('pdp-secret')); +}); + +// A non-default scheme, port and path, so a constructor that ignores the configured +// PDP (or an OPA client that ignores the derived base URL) fails the tests below. +const CONFIGURED_PDP = 'https://pdp.example.com:1234/prefix/'; +const EXPECTED_OPA_CHECK_URL = 'https://pdp.example.com:8181/prefix/v1/data/permit/root'; + +// Reaches the SDK-created OPA client, as retry-interceptor.spec.ts does for enforcer.client. +interface PermitInternals { + enforcer: { opaClient: AxiosInstance }; +} + +// Records the URL axios would request and answers with an OPA allow decision, so the +// check runs end to end without a network call. +function captureRequestUrls(instance: AxiosInstance): string[] { + const urls: string[] = []; + instance.defaults.adapter = async (config: InternalAxiosRequestConfig) => { + urls.push(axios.getUri(config)); + return { status: 200, statusText: 'OK', headers: {}, config, data: { allow: true } }; + }; + return urls; +} + +test('a useOpa check posts to the OPA root derived from the configured PDP', async (t) => { + const permit = new Permit({ token: 'test-token', pdp: CONFIGURED_PDP }); + const urls = captureRequestUrls((permit as unknown as PermitInternals).enforcer.opaClient); + + t.true(await permit.check('user', 'read', 'document', {}, { useOpa: true })); + t.deepEqual(urls, [EXPECTED_OPA_CHECK_URL]); +}); + +test('a useOpa check through an injected opaAxiosInstance posts to the OPA root', async (t) => { + const opaAxiosInstance = axios.create(); + const urls = captureRequestUrls(opaAxiosInstance); + const permit = new Permit({ token: 'test-token', pdp: CONFIGURED_PDP, opaAxiosInstance }); + + t.true(await permit.check('user', 'read', 'document', {}, { useOpa: true })); + t.deepEqual(urls, [EXPECTED_OPA_CHECK_URL]); +}); diff --git a/yarn.lock b/yarn.lock index 4d08fd79..fe10d46d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -799,11 +799,6 @@ "@types/mime" "*" "@types/node" "*" -"@types/url-parse@^1.4.11": - version "1.4.11" - resolved "https://registry.npmjs.org/@types/url-parse/-/url-parse-1.4.11.tgz" - integrity sha512-FKvKIqRaykZtd4n47LbK/W/5fhQQ1X7cxxzG9A48h0BGN+S04NH7ervcCjM8tyR0lyGru83FAHSmw2ObgKoESg== - "@typescript-eslint/eslint-plugin@^4.0.1": version "4.33.0" resolved "https://registry.npmjs.org/@typescript-eslint/eslint-plugin/-/eslint-plugin-4.33.0.tgz" @@ -5317,11 +5312,6 @@ q@^1.5.1: resolved "https://registry.npmjs.org/q/-/q-1.5.1.tgz" integrity sha512-kV/CThkXo6xyFEZUugw/+pIOywXcDbFYgSct5cT3gqlbkBE1SJdwy6UQoZvodiWF/ckQLZyDE/Bu1M6gVu5lVw== -querystringify@^2.1.1: - version "2.2.0" - resolved "https://registry.npmjs.org/querystringify/-/querystringify-2.2.0.tgz" - integrity sha512-FIqgj2EUvTa7R50u0rGsyTftzjYmv/a3hO345bZNrqabNqjtgiDMgmo4mkUjd+nzU5oF3dClKqFIPUKybUyqoQ== - queue-microtask@^1.2.2: version "1.2.3" resolved "https://registry.npmjs.org/queue-microtask/-/queue-microtask-1.2.3.tgz" @@ -5533,11 +5523,6 @@ require-main-filename@^2.0.0: resolved "https://registry.npmjs.org/require-main-filename/-/require-main-filename-2.0.0.tgz" integrity sha512-NKN5kMDylKuldxYLSUfrbo5Tuzh4hd+2E8NPPX02mZtn1VuREQToYe/ZdlJy+J3uCpfaiGF05e7B8W0iXbQHmg== -requires-port@^1.0.0: - version "1.0.0" - resolved "https://registry.npmjs.org/requires-port/-/requires-port-1.0.0.tgz" - integrity sha512-KigOCHcocU3XODJxsu8i/j8T9tzT4adHiecwORRQ0ZZFcp7ahwXuRU1m+yuO90C5ZUyGeGfocHDI14M3L3yDAQ== - resolve-cwd@^3.0.0: version "3.0.0" resolved "https://registry.npmjs.org/resolve-cwd/-/resolve-cwd-3.0.0.tgz" @@ -6705,14 +6690,6 @@ url-parse-lax@^3.0.0: dependencies: prepend-http "^2.0.0" -url-parse@^1.5.10: - version "1.5.10" - resolved "https://registry.npmjs.org/url-parse/-/url-parse-1.5.10.tgz" - integrity sha512-WypcfiRhfeUP9vvF0j6rw0J3hrWrw6iZv3+22h6iRMJ/8z1Tj6XfLP4DsUix5MhMPnXpiHDoKyoZ/bdCkwBCiQ== - dependencies: - querystringify "^2.1.1" - requires-port "^1.0.0" - urlgrey@1.0.0: version "1.0.0" resolved "https://registry.npmjs.org/urlgrey/-/urlgrey-1.0.0.tgz"