feat(cedarling-js): add JavaScript runtime agnostic sdk for Cedarling. - #14635
feat(cedarling-js): add JavaScript runtime agnostic sdk for Cedarling.#14635Dahkenangnon wants to merge 11 commits into
Conversation
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
|
Important Review skippedToo many files! This PR contains 249 files, which is 99 over the limit of 150. To get a review, narrow the scope: Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (249)
You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
mo-auto
added
area-documentation
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
b986a7b to
fb8af58
Compare
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
…mples Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
…ples Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
|
coderabbit review |
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
There was a problem hiding this comment.
-
There should be a section for build from source under installation:
https://github.com/JanssenProject/jans/blob/jans-cedarling-14582/jans-cedarling/bindings/cedarling_js/README.md#installation -
Why this extra
dangerouslyDisableSignatureValidationis required we already have bootstrap properties to control validation checks? Alsologging.typeis not required. -
How addition
principalandresourceattributes will be added .
https://github.com/JanssenProject/jans/blob/jans-cedarling-14582/jans-cedarling/bindings/cedarling_js/README.md#clientauthorizeunsignedrequest -
How to access complete authz-result json containing
diagnostics,reason,errors, 'request_id'. -
Describe about the test cases, how to run test cases for unsigned and multi-issuer.
-
Regarding initialization, define how the other bootstrap properties will be defined in args for initialization.
|
Thanks for the review. I’ll update the README to address all five points and clarify how the JavaScript SDK maps to Cedarling’s existing bootstrap configuration.
|
|
These above will be implement with the following rules:
EX: Building from source will be in docs/, while the fix related to the Does it sounds correct ? |
There was a problem hiding this comment.
Blocking
Signature validation is untested, and the shipped example disables it
Every contract test that runs real WASM authorization disables it:
The rationale (deterministic offline runs) is already acknowledged in the thread. The gap that remains: if a change stopped verifying signatures, no test in this suite would fail.
The demo IdP at common/idp.js:115 already generates an RSA keypair and serves JWKS, so a valid-signature case and a tampered-signature case can both run offline.
The same defect appears at runtime: common/test-config.json:10-11 sets dangerouslyDisableSignatureValidation and dangerouslyDisableStatusValidation for every example, so the demos' "signed" path accepts forged tokens. Tests don't verify, and the shipped example turns verification off — those should be fixed together.
The publish job cannot authenticate
build_cedarling_js runs npm publish at build-packages.yml:750 with no NODE_AUTH_TOKEN, no .npmrc, and no registry-url: on setup-node at :622. The sibling build_cedarling_wasm job sets it at :445.
The job therefore depends entirely on npm OIDC trusted publishing, hence npm@11.9.0 at :629. Trusted publishing must be configured against a package that already exists on npm; @janssenproject/cedarling does not. The first release will fail until someone publishes manually and configures the trusted publisher for this exact workflow and job name.
The PR description states the SDK is published "using the existing npm publishing secret." No secret is referenced in the job — either the description or the implementation needs correcting.
npm ci runs against a swapped-in file: dependency
build-packages.yml:638 runs npm version "${WASM_VERSION}" after the downloaded artifact has replaced cedarling_wasm/pkg, then :687 runs npm ci against the committed lockfile. dependencies pins file:../cedarling_wasm/pkg, and npm validates linked-package versions against the lock, so a version-bumped artifact is likely to produce "package.json and package-lock.json are not in sync."
This passes in test-cedarling.yml only because wasm-pack there builds the unbumped Cargo.toml version. The workflow_dispatch dry run noted under Release path would confirm or rule this out.
Three call sites reject where their siblings do not
These re-throw when the caught error is not a recognized SDK code:
client.ts:460in#authorizeclient.ts:583in#finishCloseclient.ts:679increateCedarling
The issuer, context, and log helpers handle the same case by wrapping. See client.ts:302-315, which uses isSdkErrorCode(...) ? error : createSdkError(...).
The doc comment indicates the throw is intentional for unexpected failures. The issue is the asymmetry: authorize, close, and createCedarling can reject while structurally identical methods never do, and the API shape leads callers to check result.ok and stop there. That produces an unhandled rejection in production.
README.md:366 states the SDK "does not throw errors on expected operational failures". Accurate as written, but "expected" is load-bearing and easy to miss. Either wrap consistently or document the rejection path.
authz-middleware.js — unguarded dereference and an ownership fallback
authz-middleware.js:88 reads diagnostics.reasons unguarded, while :72 correctly guards the same value with if (!decision && diagnostics). A deny without diagnostics throws inside the error path.
:13 sets taskOwner = task ? task.owner : userId, so a missing resource defaults ownership to the caller and any owner-based policy allows. With x-user-id defaulting to 'bob' at :10, and signature validation off per the section above, this is the file consumers are most likely to copy. It needs an explicit "intentionally insecure, do not copy" banner at minimum.
Non-blocking
The guard at assert-publishable.mjs:28 checks name === "cedarling_wasm", but the dependency is @janssenproject/cedarling_wasm, so the branch never executes. Returning false would also mark it unsafe, which inverts the apparent intent. Staging rewrites the version first, so nothing breaks today.
With allowedAlgorithms omitted, all twelve algorithms are enabled: prepare.ts:24-36, applied at prepare.ts:503. none is absent, so this is not urgent. Defaulting to symmetric HS* alongside RS/ES/PS is the standard setup for algorithm confusion. Asymmetric by default, with HS* opt-in, would be safer.
prepare.ts:21 defines UINT32_MAX as "the largest integer representable by the generated unsigned 32-bit fields", but five fields bound to Number.MAX_SAFE_INTEGER instead — CEDARLING_DATA_STORE_MAX_TTL and _DEFAULT_TTL at :437-451, CEDARLING_HTTP_MAX_RESPONSE_SIZE_BYTES at :618, and the three CEDARLING_LOCK_*_INTERVAL values. Validation accepts values the WASM boundary will reject or wrap.
httpUrl() at prepare.ts:177 rejects credentials in the URL but accepts plaintext http:. It gates policyStore.url and lock.configurationUrl, the latter alongside CEDARLING_LOCK_SSA_JWT. Requiring https: outside loopback would match the algorithm-default reasoning above.
withAuthorizationShortcuts at client.ts:80 sets denied: false on the error branch, so if (result.denied) reject() fails open while if (!result.allowed) is safe. Since the shortcuts exist to encourage terse call sites, that asymmetry belongs in the type's doc comment.
selectAuthorizationRequest in authorization/dispatch.ts reads type and request and silently ignores unknown envelope fields, while every other boundary in the SDK calls rejectUnknown.
SDK and WASM versions are permanently coupled: build-packages.yml:680 runs npm version "${WASM_VERSION}" and stage-release.mjs:110 pins exactly. A WASM patch therefore forces an SDK republish, and the published cedarling_wasm version stops matching the Rust crate. Confirming this is the intended long-term behaviour would be useful.
query.ts:94 is unreachable. Given the present === 0 check above it and the id !== undefined early return, requestId and tag cannot both be undefined at that point.
stage-release.mjs:120 spreads ...sourceManifest, so all 10 devDependencies (electron, playwright, webpack, workerd) and 24 test:* scripts land in the published package.json. Consumers do not install devDeps, so the effect is cosmetic.
test-config.json declares expectedFailures: ["authorizeMultiIssuer"] described as a "known bug", echoed by a console.log in the demo's init.js. A linked tracking issue would be better than prose in a fixture.
The new cedarling_js_tests job builds the WASM crate and installs Bun, Deno, and three Playwright browsers to run eight runtime suites, with no timeout-minutes and no persist-credentials: false on checkout. The latter is set on the new build-packages.yml job.
Release path
build-packages.yml triggers on workflow_run and workflow_dispatch only, never on pull_request. The 257 lines of publish logic added here (version coordination, manifest staging, tarball verification, npm publish) have not executed. The green checks on this PR come from test-cedarling.yml. A workflow_dispatch dry run against nightly would validate it — and would settle the npm ci and npm-authentication questions above — before anything depends on it.
Note that the new SDK publish at build-packages.yml:750 fails loudly, whereas the existing WASM publish at line 501 continues on error. Since build_cedarling_js depends on that job and then polls npm view for five minutes, a silently failed WASM publish surfaces here as a confusing timeout.
Minor: the new collect-cedarling-js-digests tag step at :1977 interpolates inputs.target_tag and workflow_run.head_branch directly into run:. This matches the existing pattern used by the other collect-* jobs in the file, so it is not a regression, but build_cedarling_js itself uses the safer env: form and the new job could match it.
CodeRabbit skipped this PR ("249 files exceed the limit of 150"). With 29,181 lines of committed lockfiles and an Electron demo carrying its own webpack configs, the PR exceeds the bots' limits. Splitting the SDK from the demos would restore that coverage.
Checked, no findings
Publish security — injection and secret handling: actions are SHA-pinned, harden-runner present, persist-credentials: false, github.repository fork guard, --provenance with OIDC, SLSA provenance job, WASM package name and version asserted before publish. No injection or token-leak paths found. (Authentication is covered separately under Blocking.)
No committed secrets. The demo IdP generates its keypair at runtime with generateKeyPairSync. The Electron demo sets contextIsolation: true, sandbox: true, nodeIntegration: false, and routes external URLs through shell.openExternal.
Error model: errors.ts:229 produces frozen Error objects, strips URL credentials, and gates retained causes through a WeakSet allowlist. One trade-off: every message becomes a fixed generic string, so the underlying Cedar and WASM detail is discarded. This may make production debugging harder than expected; a debug flag preserving detail would address it.
Input validation is the strongest part of the SDK: inspectOwnProperty avoids invoking getters, rejectUnknown catches misspelled options deterministically, and values are snapshotted before crossing the WASM boundary. Lifecycle handling — in-flight accounting, a single memoized close promise, drain-before-shutdown — is careful. scripts/verify-consumer.mjs packs, installs offline into a scratch consumer, asserts the WASM dependency hoists rather than nests, and exercises both ESM and CJS entry points.
Archive bytes are copied during validation at prepare.ts:302, so the factory does not need to copy again. The SDK lockfile is committed so npm ci resolves in CI. The Cargo workspace exclusion is harmless, though bindings/cedarling_js contains no Cargo.toml, so the entry is a no-op.
| ```ts | ||
| import { createCedarling } from "@janssenproject/cedarling"; | ||
|
|
||
| const result = await createCedarling({ |
There was a problem hiding this comment.
I think it would be better to avoid custom mapping to Bootstrap properties. Any changes in the core library would require corresponding updates in this library, which increases the maintenance burden.
I also like that every binding follows the same approach, with the official documentation serving as the single source of truth. Thanks for all the work you've put into this—it's a significant effort. That said, I think passing the values directly would be a simpler and more maintainable solution.
5 participants
Prepare
Description
Target issue
Closes #14582
Implementation Details
This PR introduces
@janssenproject/cedarling, a JavaScript and TypeScript SDK wrapping the generated@janssenproject/cedarling_wasmpackage.Key changes:
bindings/cedarling_jsfrom the parent Rust workspace because it is an npm project built on top of the WASM binding.test-cedarling.ymlbuilds the WASM package locally and runs the complete JavaScript SDK check suite.build-packages.ymlshares the generated WASM artifact, builds and validates the SDK package, coordinates release versions, and publishes it to npm using the existing npm publishing secret.No new workflow files or npm publishing credentials were introduced.
Test and Document the changes
Validation included:
npm cinpm run checkDocumentation is included in the SDK root README and
docs/README.md.Not applicable: this PR has documentation impact, and the relevant documentation has been updated.