Conversation
0053425 to
aa6d88b
Compare
|
Heads-up on CI: the |
zeevmoney
left a comment
There was a problem hiding this comment.
Clean, behavior-equivalent refactor with a wired-in unit test — nice. Two minor notes.
[MEDIUM] The yarn.lock diff also pulls in ~43 unrelated @esbuild/* and @rollup/* entries resolved against registry.yarnpkg.com while the rest of the lockfile uses registry.npmjs.org. That's regeneration noise that mixes registry hosts and inflates the diff. Consider regenerating from main with the pinned yarn 1.22.22 so only the url-parse/@types/url-parse removals remain.
| * @returns The OPA base URL string (e.g. `http://localhost:8181/v1/data/permit/`). | ||
| */ | ||
| export function buildOpaBaseUrl(pdp: string): string { | ||
| const opaBaseUrl = new URL(pdp); |
There was a problem hiding this comment.
[LOW] Scheme-less PDP input now throws instead of returning a (bad) string
new URL('localhost:7766') throws TypeError [ERR_INVALID_URL], whereas url-parse returned a malformed string without throwing. Scheme-less PDP is unsupported either way, but the failure mode changed from silent-bad-string to a throw in the Enforcer constructor.
Suggestion: Add a @throws note to buildOpaBaseUrl (optionally a t.throws test) so callers know a scheme-less pdp throws.
There was a problem hiding this comment.
Resolved. Kyzgor added a @throws note in 7c38c64 and a t.throws test in 575f1b3. e5abd86 then changed the error from the raw TypeError to a PermitError. A pdp of localhost or //localhost:7766 now fails in new Permit() with:
PermitError: Invalid PDP URL in the "pdp" option: expected an absolute http(s) URL, e.g. "http://localhost:7766".
The JSDoc says @throws {PermitError}, and buildOpaBaseUrl: a PDP without a scheme throws a PermitError naming the pdp option covers both inputs.
localhost:7766 is still misparsed rather than rejected. URL reads localhost: as the scheme, so the function returns localhost:7766 unchanged. The JSDoc and the PR description say to pass a full URL.
There was a problem hiding this comment.
Pull request overview
Refactors OPA client base URL construction to use Node’s native WHATWG URL API instead of url-parse, and adds unit coverage to prevent regressions in the URL-building logic.
Changes:
- Replaced
url-parseusage with a small helper (buildOpaBaseUrl) based on the nativeURLAPI. - Added an AVA unit test suite covering default and edge-case PDP URL inputs.
- Removed
url-parse/@types/url-parsefrom dependencies and regenerated the lockfile; added atest:unitscript.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/enforcement/enforcer.ts |
Removes url-parse, introduces buildOpaBaseUrl(), and uses it to configure the OPA axios baseURL. |
src/tests/enforcer.spec.ts |
Adds unit tests validating the exact OPA base URL string for several PDP inputs. |
package.json |
Removes url-parse deps and adds a test:unit script to run the new test. |
yarn.lock |
Lockfile regeneration reflecting dependency removal and resulting resolution changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export function buildOpaBaseUrl(pdp: string): string { | ||
| const opaBaseUrl = new URL(pdp); | ||
| opaBaseUrl.port = '8181'; | ||
| opaBaseUrl.pathname = `${opaBaseUrl.pathname}v1/data/permit/`; | ||
| return opaBaseUrl.toString(); | ||
| } |
There was a problem hiding this comment.
Resolved in e5abd86. buildOpaBaseUrl catches the URL error and throws:
PermitError: Invalid PDP URL in the "pdp" option: expected an absolute http(s) URL, e.g. "http://localhost:7766".
The message names the option but not the value. The native TypeError message included the whole URL, credentials included, which could end up in logs. Covered by buildOpaBaseUrl: a PDP without a scheme throws a PermitError naming the pdp option and new Permit does not expose credentials from an invalid PDP URL.
One gap remains: localhost:7766 is still misparsed rather than rejected. URL accepts it with localhost: as the scheme, so no error is thrown and the OPA base URL is localhost:7766. Rejecting non-http(s) schemes would add new config validation, so it is left for a follow-up on #106. The JSDoc documents the current behavior.
| /** | ||
| * Builds the OPA client base URL from the configured PDP URL by forcing the OPA | ||
| * port (8181) and appending the OPA data path. Uses the native WHATWG `URL` | ||
| * (Node >= 10), replacing the previous `url-parse` dependency (#106). | ||
| * | ||
| * @param pdp - The configured PDP base URL (e.g. `http://localhost:7766`). | ||
| * @returns The OPA base URL string (e.g. `http://localhost:8181/v1/data/permit/`). | ||
| */ |
There was a problem hiding this comment.
Resolved in 7c38c64. The @returns tag on buildOpaBaseUrl now says a PDP path with no trailing slash is glued to the data path (/prefix -> /prefixv1/data/permit/), kept from url-parse. buildOpaBaseUrl: a path prefix preserves the existing concatenation behaviour pins both /prefix and /prefix/, so changing this behavior will fail a test.
url-parse was used only to build the OPA client base URL. Node's native WHATWG URL (available since v10; engines.node already requires >=10) does the same, so extract a buildOpaBaseUrl() helper and drop url-parse and @types/url-parse. yarn.lock is pruned of url-parse and its now-orphaned transitive dependencies (querystringify, requires-port) only; every other entry is left byte-for-byte unchanged.
Lock the exact OPA base URL produced for the default PDP, trailing-slash, explicit-port, https, and path-prefix inputs so the url-parse -> native URL refactor is proven behaviour-equivalent on valid input and any regression fails here; assert a scheme-less PDP (bare host or //host:port) throws; and assert the Enforcer wires the OPA client baseURL to buildOpaBaseUrl(pdp).
a42b43d to
575f1b3
Compare
|
Thanks — pushed an update:
Rebased on |
Resolve the test:unit conflict so the script runs main's unit suite (build/tests/unit/**) and this branch's enforcer spec. src/enforcement/enforcer.ts and yarn.lock merged without conflicts: main's dedicated PDP axios instance and PDP/OPA retry interceptors are kept, with buildOpaBaseUrl on top. The merged yarn.lock is main's lockfile without the url-parse, @types/url-parse, querystringify and requires-port entries, the same four blocks yarn removes when it regenerates the lockfile for this manifest change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main's test:unit glob (build/tests/unit/**/*.spec.js) now picks up the OPA base URL tests, so the script matches main again and both the retry and enforcer suites run from one place. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
new URL() throws a TypeError whose enumerable input property holds the whole PDP URL, so a caller logging the new Permit() failure with pino wrote any user:password in the URL to its logs. url-parse never threw here, so this path is new with the native URL switch. buildOpaBaseUrl now throws a PermitError that names the pdp option and the expected format, and does not carry the configured value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The only wiring test used the default PDP and read back the baseURL of an injected instance, so a constructor that ignored config.pdp, or an SDK-created OPA client that ignored the derived base URL, still passed. Run a useOpa check against a non-default https PDP with a port and path through both the SDK-created client and an injected opaAxiosInstance, and assert the request URL at the adapter boundary. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The spec header said the native URL refactor was proven equivalent to url-parse. That holds for canonical absolute http(s) PDP URLs only: the WHATWG parser resolves dot segments, so https://host/a/.. now yields /v1/data/permit/ instead of /a/..v1/data/permit/. Narrow the comment and pin the dot-segment behaviour with a test. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Codex <noreply@openai.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings in #122 by @Kyzgor (PER-16497, closes #106). It builds the OPA base URL with the WHATWG URL instead of url-parse, removes url-parse and @types/url-parse, and makes new Permit() throw a PermitError that names the pdp option (without its value) when pdp is empty or not an absolute URL. That throw is a breaking change for REST-only apps with an empty or invalid pdp, so it ships with this PR. Conflict resolution: kept this branch's @types/node and lockfile, and dropped the url-parse, querystringify and requires-port entries. Ported the AVA spec to Vitest as src/tests/unit/enforcement/opa-base-url.spec.ts. Co-authored-by: Kyzgor <connordgordon95@gmail.com> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PSoip6dghQ62bLQ6GBMwTA
|
Thank you for this contribution, @Kyzgor! Replacing I'm closing this PR because it has been merged into #134 and will ship from there. It goes in with #134 rather than on its own because it is a breaking change. What happened to your work:
Thanks again! |
Summary
url-parsewith the built-in WHATWGURLwhen building the OPA URL from thepdpoption.url-parseand@types/url-parse, plus their dependenciesquerystringifyandrequires-port, frompackage.jsonandyarn.lock.buildOpaBaseUrl()keeps the existing behavior: the port is set to 8181 andv1/data/permit/is appended to the PDP path.pdpvalue now throws aPermitErrorthat names the option but not the value. The nativeURLerror contained the whole URL, credentials included, and could end up in logs.useOpachecks reach the right OPA URL through both the default client and an injectedopaAxiosInstance.main(merged, not rebased); the retry support from Add opt-in HTTP retry support (REST + PDP) #120 in the enforcer is unchanged.Linear
Closes #106.
Details
OPA URL (
src/enforcement/enforcer.ts)pdphttp://localhost:7766orhttp://localhost:7766/http://localhost:8181/v1/data/permit/https://pdp.example.comhttps://pdp.example.com:8181/v1/data/permit/https://pdp.example.com:1234https://pdp.example.com:8181/v1/data/permit/http://localhost:7766/prefix/http://localhost:8181/prefix/v1/data/permit/http://localhost:7766/prefixhttp://localhost:8181/prefixv1/data/permit/(same as before; pinned by a test)https://pdp.example.com/a/..https://pdp.example.com:8181/v1/data/permit/(URLresolves dot segments; url-parse kept them)For absolute http(s) URLs in canonical form, the result is the same string url-parse produced.
Behavior change
A
pdpvalue without a scheme, such aslocalhostor//localhost:7766, now fails innew Permit()with:url-parse accepted these values silently and produced an OPA URL that couldn't be used.
localhost:7766is still parsed withlocalhost:as the scheme, so pass a full URL.Tests (
src/tests/unit/enforcer.spec.ts)tests/unit, soyarn test:unitruns them.useOpacheck posts to, with and without an injectedopaAxiosInstance.Lockfile
yarn.lockremoves only the four package blocks. A fullyarn installrewrites about 787 lines, and it does the same on unchangedmain, so the full lockfile refresh stays with PER-16497.Testing
yarn build: passesyarn lint: 0 errors (7 existing warnings)yarn test:unit: 58 passed on Node 24 and Node 22yarn test:module-imports: 9 passedyarn install --frozen-lockfile: passesNotes
PermitConnectionErrormessages and in the config debug log. The debug log is PER-16493, fixed in permitio 3.0.0: refactor SDK APIs, tests, and release validation #134.pdpwith a query string or fragment sendsuseOpachecks to the wrong path. This needs a decision on whether to support or reject such URLs.enforcer.spec.tsuses AVA. It needs porting when the Vitest migration (test: migrate AVA→Vitest with event-based waits (stacked on #131) #132, folded into permitio 3.0.0: refactor SDK APIs, tests, and release validation #134) lands.path-to-regexp,require-in-the-middle,@bitauth/libauthandpino-pretty;axiosandlodashfloors;Original change by @Kyzgor; the merge with
main, the error handling and the test commits were added by the maintainers.🤖 Generated with Claude Code
https://claude.ai/code/session_01PSoip6dghQ62bLQ6GBMwTA