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 14 commits into
Conversation
|
Important Review skippedToo many files! This PR contains 265 files, which is 115 over the limit of 150. To get a review, reduce the PR to 150 files or fewer by splitting it into smaller PRs or changing its base branch. 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 (265)
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
b986a7b to
fb8af58
Compare
|
coderabbit review |
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.
There was a problem hiding this comment.
Thanks. I agree that direct pass-through would create a thinner binding, but cedarling_wasm already provides that low-level API.
The goal of @janssenproject/cedarling is a stable, Web-native SDK. Its typed CedarlingOptions also supports JavaScript-specific sources such as Uint8Array archives and async loaders, which require routing through init_from_archive_bytes.
Passing CEDARLING_* values directly would expose the generated/core contract to every consumer and duplicate the existing WASM API. The official bootstrap documentation should remain the source of truth for Cedarling semantics, while this SDK maintains a curated, versioned JS interface with compatibility tests.
I therefore propose keeping the typed mapping. Consumers needing raw bootstrap access can use cedarling_wasm, or we can discuss a separate explicitly low-level API.
There was a problem hiding this comment.
OK, maybe at least we should have a method to build a config based on raw bootstrap properties; maybe for someone it will be useful.
There was a problem hiding this comment.
Ok, thank . I think this is feasible.
Ack and working on these in child PR. |
| /** | ||
| * Dispatches an unsigned authorization envelope to `authorizeUnsigned()`. | ||
| * | ||
| * @param request - Explicit unsigned authorization envelope. | ||
| * @returns The named operation's decision or an error labeled `authorize`. | ||
| */ | ||
| authorize( | ||
| request: UnsignedAuthorization, | ||
| ): Promise<AuthorizationResult<CedarlingAuthorizationError>>; | ||
|
|
||
| /** | ||
| * Dispatches a multi-issuer envelope to `authorizeMultiIssuer()`. | ||
| * | ||
| * @param request - Explicit token-validating authorization envelope. | ||
| * @returns The named operation's decision or an error labeled `authorize`. | ||
| */ | ||
| authorize( | ||
| request: MultiIssuerAuthorization, | ||
| ): Promise<AuthorizationResult<CedarlingAuthorizationError>>; | ||
|
|
||
| /** | ||
| * Dispatches a discriminated authorization union to one named operation. | ||
| * | ||
| * @param request - Unsigned or multi-issuer authorization envelope. | ||
| * @returns The selected named operation's decision with dispatcher errors. | ||
| */ | ||
| authorize( | ||
| request: AuthorizationRequest, | ||
| ): Promise<AuthorizationResult<CedarlingAuthorizationError>>; |
There was a problem hiding this comment.
Instead of having authorizeUnsigned and authorizeMultiIssuer we have 3 other method that do the same, do we really need that? KISS & YAGNI
There was a problem hiding this comment.
The three declarations are TypeScript overload signatures for one runtime dispatcher, but I agree that the dispatcher itself duplicates the two named authorization operations. Go and Python expose only unsigned and multi-issuer methods, and those names make the selected trust model explicit. I will remove the generic authorize() API and retain authorizeUnsigned() and authorizeMultiIssuer().
| * if (!closed.ok) console.error(closed.error.code); | ||
| * ``` | ||
| */ | ||
| close(): Promise<Result<void, CedarlingLifecycleError>>; |
There was a problem hiding this comment.
In WASM we have method shut_down I think better to have the same naming. To be consistent over different bindings.
There was a problem hiding this comment.
This method waits until all workers are finished
There was a problem hiding this comment.
Given the explicit intention to have less difference on public api across bindings (python, go, js,etc) goal, renaming close() to shutDown() is reasonable and still idiomatic TypeScript.
So, I will rename it to shutDown() .
|
|
||
| ```ts | ||
| const result = await client.authorizeUnsigned(request); | ||
| const { ok, allowed, denied, err } = result; |
There was a problem hiding this comment.
allowed and denied is the same bool value
There was a problem hiding this comment.
Agreed! I'll remove these flat
4ac3926 to
a663f1c
Compare
✅ Action performedReview finished.
|
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>
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>
…mples Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
…ples Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
* feat(cedarling-js)!: align public API with Cedarling bindings
Accept typed options or raw bootstrap properties through the single factory.
Expose only explicit unsigned and multi-issuer authorization methods, return canonical authorization results, and rename close to shutDown.
Update documentation, consumer verification, and contract coverage for the revised API.
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
* test(cedarling-js): strengthen SDK contract coverage
Cover raw bootstrap capabilities, retained-log expiry and drain lifecycle, and context clearing behavior.
Expand signature-enabled multi-issuer validation for expired, untrusted, tampered, and disallowed-algorithm tokens.
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
* refactor(cedarling-js): consolidate SDK internals
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
* feat(cedarling-js): harden validation and diagnostics
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
* ci(cedarling-js): harden test and release workflows
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
* docs(cedarling-js): address remaining review feedback
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
* ci(cedarling-js): harden npm bootstrap and fix doc lint warnings
- Pin npm@11.9.0 tarball with SHA-512 integrity check in build-packages.yml
- Disable lifecycle scripts during npm bootstrap
- Remove redundant "same exact" wording in docs/README.md
- Silence markdownlint MD028 false positives for GitHub alert blockquotes
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
* feat(cedarling-js): support entity references and harden value validation
- Add CedarEntityReference type and include it in CedarValue union
- Validate { __entity: { type, id } } markers in snapshotCedarValueInner
- Reject reserved cedar_entity_mapping key anywhere in Cedar values
- Return required (not type) error when action field is missing
- Assert shutDown() Result in verify-consumer.mjs ESM/CJS simulations
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
* test(cedarling-js): cover entity references, reserved keys, and missing action
- Add entity reference validation tests in common-values.test.ts
- Add cedar_entity_mapping rejection tests for nested and array cases
- Add missing action required-error test in authorize-unsigned.test.ts
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
---------
Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com>
* feat(cedarling-js): harden runtime examples Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com> * fix(cedarling-js): apply review feedback to runtime examples - unquote the single-word Lato font fallback in the shared theme - split MAIN_ARGS into individual arguments for the Electron dev server - bound issuer configuration fetches with abort timeouts - reject empty RS256 algorithm allowlists in Cedarling config loading - map signed identity failures to 401 and operational failures to 503 - filter tasks:list to outcomes the user is authorized to view - centralize Cedar identifiers and actions in shared contracts - reset permission state, trim titles, and catch logout errors in renderers - time-box Cedarling shutdown and drain pending IPC work on quit - bind the stored ID-token subject to the UserInfo token - derive cookie Secure from APP_ORIGIN behind TLS-terminating proxies - authenticate before task existence checks and announce permission checks - add tests for denied/error authorization outcomes and mutation happy paths Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com> * fix(cedarling-js): address review feedback on authorization boundary - Refactor authz-middleware.js to verify JWT via jose and derive userId from the verified token subject instead of the spoofable x-user-id header (concern from olehbozhok) - Extract checkSessionGuard as a pure exported function in ipc.ts for independent testability - Add owner-mismatch 403 test to server.test.js (alice deletes bob's task) - Add invalid signed token rejection test to server.test.js - Add policy-deny test to permissions.test.ts (decision: false) - Add signed-mode happy path test to permissions.test.ts - Add unit tests for checkSessionGuard in ipc.test.ts (no session, session match, session mismatch) Addresses olehbozhok's review comment on PR #14677. Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com> * fix(cedarling-js): close remaining review findings on shell usage and RS256 allowlist - Spawn npm without a shell on POSIX in webpack.config.renderer.dev.ts; keep shell only on Windows where the npm.cmd shim requires it - Reject an empty jwt.allowedAlgorithms array in the vercel-nextjs Cedarling config guard, matching the hono example Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com> * feat(cedarling-js): support pre-staged local package installs Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com> * feat(cedarling-js): add reproducible Docker build targets Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com> * feat(cedarling-js): orchestrate examples with Compose profiles Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com> * fix(cedarling-js): unify Cloudflare Hono workflow Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com> * refactor(cedarling-js): migrate Electron demo to electron-vite Replace the ERB and Webpack scaffold with the documented electron-vite layout while retaining the Docker-assisted local package build, external IdP, native GUI workflow, and Cedarling security boundary. Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com> * fix(cedarling-js): add missing init:true to hono-deno service hono-deno was the only compose service without init:true, unlike its sibling stacks. Without it, deno runs as PID 1 and must handle SIGTERM/zombie-reaping itself, causing slower/unclean shutdown. Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com> --------- Signed-off-by: Justin Dah-kenangnon <dah.kenangnon@gmail.com> Signed-off-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com> Co-authored-by: Oleh Bozhok <6554798+olehbozhok@users.noreply.github.com>
355336a to
b392987
Compare
) * refactor: replace assert error in favor runtime exception Signed-off-by: iromli <isman.firmansyah@gmail.com> * refactor: change RuntimeError with specific CalledProcessError Signed-off-by: iromli <isman.firmansyah@gmail.com> * chore: sync templates Signed-off-by: iromli <isman.firmansyah@gmail.com> * chore: revert to RuntimeError Signed-off-by: iromli <isman.firmansyah@gmail.com> --------- Signed-off-by: iromli <isman.firmansyah@gmail.com> Co-authored-by: Mohammad Abudayyeh <47318409+moabu@users.noreply.github.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
7 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 wasm pkg pattern.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.