feat(cedarling-js)!: align and harden the public SDK contract - #14674
feat(cedarling-js)!: align and harden the public SDK contract#14674Dahkenangnon wants to merge 9 commits into
Conversation
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>
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>
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 skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe Cedarling JavaScript SDK now uses centralized validation, configuration, error handling, and runtime engine boundaries. Authorization uses named methods and standard ChangesCedarling JavaScript SDK
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
coderabbitai
Bot
added
area-CI
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/build-packages.yml:
- Around line 630-631: Update the npm bootstrap step around “Install npm with
trusted publishing support” to prevent lifecycle scripts from executing while
installing npm@11.9.0, preferably by disabling install scripts; retain the
pinned npm version and ensure the trusted publishing flow remains functional.
In `@jans-cedarling/bindings/cedarling_js/docs/README.md`:
- Line 391: Update the release-version description in the documentation to
replace “same exact version” with “the same version,” preserving the surrounding
wording and meaning.
In `@jans-cedarling/bindings/cedarling_js/README.md`:
- Around line 519-531: Update the adjacent security callouts in the README by
removing the blank separator lines within the blockquote structure or otherwise
restructuring the callouts to avoid MD028 warnings. Preserve both warning
messages and their intended separation while ensuring markdownlint passes.
In `@jans-cedarling/bindings/cedarling_js/scripts/verify-consumer.mjs`:
- Line 88: Update both shutdown call sites in the verifier to capture the Result
returned by client.shutDown() and assert or otherwise validate its ok status,
causing verification to fail when shutdown is unsuccessful instead of discarding
the result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fbb354bd-2c9a-4f1b-85c5-0207afbf97b2
📒 Files selected for processing (81)
.github/workflows/build-packages.yml.github/workflows/test-cedarling.ymljans-cedarling/bindings/cedarling_js/README.mdjans-cedarling/bindings/cedarling_js/docs/README.mdjans-cedarling/bindings/cedarling_js/package.jsonjans-cedarling/bindings/cedarling_js/scripts/assert-publishable.mjsjans-cedarling/bindings/cedarling_js/scripts/stage-release.mjsjans-cedarling/bindings/cedarling_js/scripts/verify-consumer.mjsjans-cedarling/bindings/cedarling_js/src/authorization/dispatch.tsjans-cedarling/bindings/cedarling_js/src/authorization/request.tsjans-cedarling/bindings/cedarling_js/src/authorization/types.tsjans-cedarling/bindings/cedarling_js/src/client/client.tsjans-cedarling/bindings/cedarling_js/src/client/types.tsjans-cedarling/bindings/cedarling_js/src/configuration/prepare.tsjans-cedarling/bindings/cedarling_js/src/configuration/types.tsjans-cedarling/bindings/cedarling_js/src/context/input.tsjans-cedarling/bindings/cedarling_js/src/context/types.tsjans-cedarling/bindings/cedarling_js/src/engine/edge.tsjans-cedarling/bindings/cedarling_js/src/engine/engine.tsjans-cedarling/bindings/cedarling_js/src/engine/factory.tsjans-cedarling/bindings/cedarling_js/src/engine/generated.tsjans-cedarling/bindings/cedarling_js/src/engine/node.tsjans-cedarling/bindings/cedarling_js/src/engine/web.tsjans-cedarling/bindings/cedarling_js/src/engine/workerd.tsjans-cedarling/bindings/cedarling_js/src/errors/errors.tsjans-cedarling/bindings/cedarling_js/src/errors/types.tsjans-cedarling/bindings/cedarling_js/src/helpers/constants.tsjans-cedarling/bindings/cedarling_js/src/helpers/records.tsjans-cedarling/bindings/cedarling_js/src/helpers/validation.tsjans-cedarling/bindings/cedarling_js/src/index.tsjans-cedarling/bindings/cedarling_js/src/issuers/input.tsjans-cedarling/bindings/cedarling_js/src/logs/normalize.tsjans-cedarling/bindings/cedarling_js/src/logs/query.tsjans-cedarling/bindings/cedarling_js/src/logs/types.tsjans-cedarling/bindings/cedarling_js/src/values/inspect.tsjans-cedarling/bindings/cedarling_js/src/values/snapshot.tsjans-cedarling/bindings/cedarling_js/src/values/types.tsjans-cedarling/bindings/cedarling_js/tests/contract/authorization.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/authorize-multi-issuer.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/authorize-unsigned.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/configuration.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/context.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/error-contract.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/issuers.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/lifecycle.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/logs.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/policy-archive.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/policy-loader.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/policy-url.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/value-validation.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/web-entry.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/web-initialization-errors.test.tsjans-cedarling/bindings/cedarling_js/tests/contract/web-tracer.test.tsjans-cedarling/bindings/cedarling_js/tests/e2e/index.tsjans-cedarling/bindings/cedarling_js/tests/e2e/signature-validation.test.tsjans-cedarling/bindings/cedarling_js/tests/e2e/stage-release.test.tsjans-cedarling/bindings/cedarling_js/tests/fixtures/browser-tracer-consumer.mjsjans-cedarling/bindings/cedarling_js/tests/fixtures/signed-issuer.tsjans-cedarling/bindings/cedarling_js/tests/tsconfig.jsonjans-cedarling/bindings/cedarling_js/tests/unit/archive-policy.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/authorize-dispatch.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/authorize-multi-issuer.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/authorize-unsigned.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/client-errors.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/common-values.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/context.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/engine-boundary.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/engine-fixture.tsjans-cedarling/bindings/cedarling_js/tests/unit/engine-initialization-fixture.tsjans-cedarling/bindings/cedarling_js/tests/unit/errors.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/helpers-validation.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/index.tsjans-cedarling/bindings/cedarling_js/tests/unit/issuers.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/lifecycle.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/loader-policy.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/logs.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/options.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/publishable-manifest.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/url-policy.test.tsjans-cedarling/bindings/cedarling_js/tests/unit/value-inspect.test.tsjans-cedarling/bindings/cedarling_js/tsconfig.json
💤 Files with no reviewable changes (4)
- jans-cedarling/bindings/cedarling_js/src/values/inspect.ts
- jans-cedarling/bindings/cedarling_js/tests/unit/authorize-dispatch.test.ts
- jans-cedarling/bindings/cedarling_js/src/authorization/dispatch.ts
- jans-cedarling/bindings/cedarling_js/src/authorization/types.ts
| - name: Install npm with trusted publishing support | ||
| run: npm install --global npm@11.9.0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow relevant section =="
if [ -f .github/workflows/build-packages.yml ]; then
sed -n '580,655p' .github/workflows/build-packages.yml | nl -ba -v580
else
echo "missing .github/workflows/build-packages.yml"
fi
echo "== npm install occurrences in workflow =="
rg -n "npm install|--global npm|permissions:|id-token|trusted publishing|npm@" .github/workflows/build-packages.yml || true
echo "== package manager / npm config files =="
git ls-files | rg 'package(-lock)?\.json$|\.npmrc$|pnpm-lock\.yaml$|yarn\.lock$|bun\.lockb?$|npm-shrinkwrap\.json$' || true
echo "== lockfile npm version/config if package-lock =="
for f in $(git ls-files | rg '(^|/)package-lock\.json$'); do
echo "-- $f --"
python3 - <<'PY'
import json, sys
p=sys.argv[1]
data=json.load(open(p))
print("lockfileVersion:", data.get("lockfileVersion"))
meta=data.get("packages",{}).get("",{})
print("root npm devDependency:", "npm" in meta.get("devDependencies",{}), meta.get("devDependencies",{}).get("npm"))
for name, spec in (data.get("packages",{}).get("",{}).get("dependencies",{})).items():
if name in ("npm",'npx-clobber','preinstall'):
print("root dependency", name, spec)
for name, v in data.get("packages",{}).items():
if name.endswith('/node_modules/npm') or name == 'node_modules/npm':
print("packages npm", name, v.get("version"), v.get("integrity", "")[:80] if v.get("integrity") else None)
PY
sed -n '1,80p' "$f"
done
echo "== npm lockfiles versions in package-lock =="
python3 - <<'PY'
import json, subprocess
files=subprocess.check_output(['git','ls-files']).splitlines()
for f in [x.decode() for x in files if x.endswith(b'-lock.json')]:
try:
data=json.load(open(f))
if isinstance(data.get('lockfileVersion'),int):
print(f, "lockfileVersion=", data.get('lockfileVersion'))
except Exception as e:
print("ERR", f, e)
PY
echo "== npm 11 docs via cache if available? =="
node - <<'JS'
try {
const npm = require('npm')
console.log('local npm require', typeof npm)
} catch (e) {
console.log('no local npm requirement available:', e.message)
}
JSRepository: JanssenProject/jans
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workflow relevant section =="
if [ -f .github/workflows/build-packages.yml ]; then
sed -n '560,660p' .github/workflows/build-packages.yml
else
echo "missing .github/workflows/build-packages.yml"
fi
echo "== npm install occurrences in workflow =="
rg -n "npm install|--global npm|permissions:|id-token|trusted publishing|npm@" .github/workflows/build-packages.yml || true
echo "== package manager / npm config files =="
git ls-files | grep -E 'package(-lock)?\.json$|\.npmrc$|pnpm-lock\.yaml$|yarn\.lock$|bun\.lockb?$|npm-shrinkwrap\.json$' || true
echo "== package-lock npm metadata =="
while read -r f; do
echo "-- $f --"
python3 - "$f" <<'PY'
import json, sys
p=sys.argv[1]
data=json.load(open(p))
print("lockfileVersion:", data.get("lockfileVersion"))
pkg=data.get("packages",{}).get("",{})
print("root devDependencies npm:", pkg.get("devDependencies", {}).get("npm"))
print("root dependencies npm:", pkg.get("dependencies", {}).get("npm"))
for name, v in data.get("packages", {}).items():
if name == "node_modules/npm" or name.endswith("/node_modules/npm"):
print("packages npm:", name, v.get("version"), (v.get("integrity") or "")[:90] or "")
PY
done < <(git ls-files | grep -E '(^|/)package-lock\.json$' || true)
echo "== repository npm CLI dependency occurrences =="
rg -n '"\s*npm\s*":\s*"[^"]+"|node_modules/npm|rush-scripts|rush-lib|rush-stack' . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' 2>/dev/null || trueRepository: JanssenProject/jans
Length of output: 6920
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== jans-cedarling package files =="
for f in jans-cedarling/bindings/cedarling_js/package.json jans-cedarling/bindings/cedarling_js/.npmrc jans-cedarling/bindings/cedarling_js/package-lock.json; do
echo "-- $f --"
if [ -f "$f" ]; then
wc -l "$f"
cat "$f"
else
echo "missing"
fi
done
echo "== repository npm CLI dependency occurrences =="
rg -n '"\s*npm\s*":\s*"[^"]+"|node_modules/npm|rush-scripts|rush-lib|rush-stack' . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' 2>/dev/null || true
echo "== npm 11 docs via docs.npmjs.com? =="
curl -fsSL --max-time 20 'https://docs.npmjs.com/packages-and-modules/configuring-the-package-manager-options/trusted-publishing#requirements' || true
echo "== npm 11.9.0 release metadata =="
curl -fsSL --max-time 20 'https://registry.npmjs.org/npm/11.9.0' | python3 - <<'PY'
import json, sys
data=json.load(sys.stdin)
version=data.get("info",{}).get("version")
engines=data.get("info",{}).get("engines",{})
bin=data.get("info",{}).get("bin",{})
print("version", version)
print("node engine", engines.get("node"))
print("bin npm", bin)
print("has npm binary", bin.get("npm"))
PY
echo "== npm 11.9.0 binary from tarball metadata =="
curl -fsSL --max-time 20 'https://registry.npmjs.org/npm/11.9.0' | python3 - <<'PY'
import tarfile, io, json, sys
data=json.load(sys.stdin)
url=data["dist"]["tarball"]
print("tarball", url)
resp=data.get("_dist",{}).get("tarball")
print("_dist tarball", resp)
PY
curl -fsSL --max-time 20 'https://registry.npmjs.org/npm/11.9.0' > /tmp/npm-11.9.0.json
python3 - <<'PY'
import json, tarfile, io, urllib.request, sys
f=open('/tmp/npm-11.9.0.json')
data=json.load(f)
url=data["dist"]["tarball"]
print("url", url)
with urllib.request.urlopen(url) as r:
t=tarfile.open(fileobj=io.BytesIO(r.read()), mode='r:gz')
members=[m for m in t.getmembers() if m.name.endswith('/') or '/' not in m.name][-10:]
for m in members:
print(m.name, m.size)
PYRepository: JanssenProject/jans
Length of output: 50375
🌐 Web query:
npm trusted publishing requirements npm 11
💡 Result:
Trusted publishing in npm allows you to publish packages from CI/CD workflows using OpenID Connect (OIDC) authentication, removing the need for long-lived npm tokens [1][2]. As of July 31, 2026, the following requirements and considerations apply when using trusted publishing with npm 11: Core Requirements - npm CLI Version: You must use npm CLI version 11.5.1 or later [1][3][2]. - Node.js Version: Node.js version 22.14.0 or higher is required [1][3]. - Account Security: Two-factor authentication (2FA) must be enabled at the npm account level [4]. - Permissions: You must have write access to the package you are configuring [4]. - Existing Package: The package must already exist on the npm registry before a trusted publisher can be configured for it [4][3]. CI/CD Configuration - Supported Providers: GitHub Actions (using GitHub-hosted runners) and GitLab CI/CD (using shared runners) are supported [1][5][2]. CircleCI is also supported [1][3]. - Workflow Permission: Your CI/CD workflow must explicitly set the id-token: write permission to allow the generation of OIDC tokens [1][6][7][8]. - Runner Type: Only cloud-hosted runners are supported; self-hosted runners are not supported [1][7]. - Exact Matching: Configuration fields (such as Organization/User, Repository, and Workflow filename) are case-sensitive and must exactly match your CI/CD provider's settings [1][7]. Note that the workflow filename should be the filename only (e.g., publish.yml), not the full path [7]. Provenance and Security - Automatic Provenance: Trusted publishing automatically generates and publishes provenance attestations by default, eliminating the need to manually include the --provenance flag [5][6][2]. - Security: This method establishes a cryptographic link between your CI/CD workflow and the npm registry, ensuring each publish is authenticated with short-lived, workflow-specific credentials [2][8]. Management - Configuration: You can manage trusted publishers via the package settings page on npmjs.com (typically found under the package's access settings) or via the npm trust command-line tool [4][6][8]. - Bulk Management: As of npm CLI v11.10.0+, you can use the npm trust command to configure trusted publishing for multiple packages in a single operation [9].
Citations:
- 1: https://docs.npmjs.com/trusted-publishers/
- 2: https://github.blog/changelog/2025-07-31-npm-trusted-publishing-with-oidc-is-generally-available/
- 3: https://circleci.com/docs/guides/deploy/deploy-to-npm-registry/index.md
- 4: https://docs.npmjs.com/cli/v11/commands/npm-trust/
- 5: https://docs.npmjs.com/generating-provenance-statements/
- 6: https://philna.sh/blog/2026/01/28/trusted-publishing-npm/
- 7: https://datafrog.io/blog/npm-trusted-publishing-github-actions-without-npm-token
- 8: https://safeguard.sh/resources/blog/npm-trusted-publishing-walkthrough-2026
- 9: https://github.blog/changelog/2026-02-18-npm-bulk-trusted-publishing-config-and-script-security-now-generally-available/
Disable lifecycle scripts on the npm bootstrap before OIDC publication.
The release job sets id-token: write before the npm install --global npm@11.9.0 bootstrap, so package metadata or lifecycle code from the public registry can run when installing the release toolchain. Use a pinned, integrity-checked npm binary or disable install scripts for this bootstrap, unless the release path requires them.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 631-631: ad-hoc installation of packages (adhoc-packages): installs a package outside of a lockfile
(adhoc-packages)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/build-packages.yml around lines 630 - 631, Update the npm
bootstrap step around “Install npm with trusted publishing support” to prevent
lifecycle scripts from executing while installing npm@11.9.0, preferably by
disabling install scripts; retain the pinned npm version and ensure the trusted
publishing flow remains functional.
Source: Linters/SAST tools
| publication. `build_cedarling_wasm` uploads its generated web `pkg/` directory | ||
| as a short-lived workflow artifact. `build_cedarling_js` depends on that job, | ||
| downloads the artifact, stages the SDK tarball, and verifies that the SDK, its | ||
| WASM dependency, and the release all use the same exact version. The job waits |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove redundant wording in the release-version description.
Replace same exact version with the same version. LanguageTool flags the current phrase as redundant.
🧰 Tools
🪛 LanguageTool
[style] ~391-~391: This phrase is redundant. Consider writing “same”.
Context: ...dependency, and the release all use the same exact version. The job waits until that WASM ...
(SAME_EXACT)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jans-cedarling/bindings/cedarling_js/docs/README.md` at line 391, Update the
release-version description in the documentation to replace “same exact version”
with “the same version,” preserving the surrounding wording and meaning.
Source: Linters/SAST tools
| > [!WARNING] | ||
| > The `authorization.dangerouslyDisableSchemaValidation` option disables Cedar schema validation for authorization requests. When enabled (`true`), `authorizeMultiIssuer()` may produce **`AUTHORIZATION_FAILED`** errors even for valid requests. This is a known incompatibility between the multi-issuer flow and schema validation bypass; it is **not safe for production** and should only be used for debugging with unsigned requests. | ||
| > Omitting `jwt.allowedAlgorithms` enables every signature algorithm supported | ||
| > by Cedarling core, including symmetric `HS256`, `HS384`, and `HS512`. | ||
| > Production applications should configure the smallest allowlist required by | ||
| > their trusted issuers and normally prefer asymmetric algorithms for remote | ||
| > OIDC issuers. | ||
|
|
||
| * Always use secure OIDC transport channels (HTTPS) to resolve remote JWKS and policy archives. | ||
| * Log level data stored in memory can contain sensitive claim attributes. Ensure `client.logs.drain()` is periodically called and written to a secure external audit ledger. | ||
| > [!WARNING] | ||
| > Setting `debug.dangerouslyExposeRawErrors: true` makes Cedarling's original, | ||
| > potentially secret-bearing failures directly accessible through | ||
| > `error.cause`. Use it only for local debugging and disable it before | ||
| > deployment. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the MD028 warnings in the security callouts.
markdownlint reports blank lines at Line [525] and Line [531] inside adjacent blockquotes. Remove or restructure those separators so the warning callouts pass the Markdown check.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 525-525: Blank line inside blockquote
(MD028, no-blanks-blockquote)
[warning] 531-531: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jans-cedarling/bindings/cedarling_js/README.md` around lines 519 - 531,
Update the adjacent security callouts in the README by removing the blank
separator lines within the blockquote structure or otherwise restructuring the
callouts to avoid MD028 warnings. Preserve both warning messages and their
intended separation while ensuring markdownlint passes.
Source: Linters/SAST tools
There was a problem hiding this comment.
This is a false positive.
The blank lines are necessary to let github properly render the warning block separately.
I'll Wrapp the callouts with <!-- markdownlint-disable MD028 --> / <!-- markdownlint-enable MD028 --> to tell markdownlint to skip the blank-line rule for that section
There was a problem hiding this comment.
@Dahkenangnon, agreed. The blank lines preserve separate GitHub warning callouts. A narrowly scoped MD028 disable/enable pair is appropriate here. I am withdrawing this finding.
✏️ Learnings added
Learnt from: Dahkenangnon
Repo: JanssenProject/jans PR: 14674
File: jans-cedarling/bindings/cedarling_js/README.md:519-531
Timestamp: 2026-08-03T10:14:39.619Z
Learning: In `jans-cedarling/bindings/cedarling_js/README.md`, blank lines between adjacent GitHub `[!WARNING]` blockquotes are intentional. They preserve separate warning-callout rendering. Use a narrowly scoped `markdownlint-disable MD028` / `markdownlint-enable MD028` pair instead of removing these separators.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
| if (!authorized.ok) throw new Error(authorized.error.code); | ||
| if (authorized.value.decision !== true) throw new Error("expected allow"); | ||
| await result.value.close(); | ||
| await result.value.shutDown(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipe
printf 'Files matching verify-consumer.mjs:\n'
fd -a 'verify-consumer\.mjs$' . || true
file="$(fd 'verify-consumer\.mjs$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
echo "--- outline ---"
ast-grep outline "$file" 2>/dev/null || true
echo "--- lines 70-115 ---"
nl -ba "$file" | sed -n '70,115p'
fi
echo "--- search shutDown/result/value ---"
rg -n "shutDown|Result\.ok|\.ok\b|shutdown" jans-cedarling/bindings/cedarling_js/scripts jans-cedarling/bindings/cedarling_js -g '*.js' -g '*.mjs' -g '*.ts' -g '*bind*' 2>/dev/null | head -n 200Repository: JanssenProject/jans
Length of output: 202
🏁 Script executed:
#!/bin/sh
set -eu
printf 'Files matching verify-consumer.mjs:\n'
fd 'verify-consumer\.mjs$' . || true
file="$(fd 'verify-consumer\.mjs$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
echo "--- outline ---"
ast-grep outline "$file" 2>/dev/null || true
echo "--- lines 70-115 ---"
nl -ba "$file" | sed -n '70,115p'
fi
echo "--- search shutDown/result/value ---"
rg -n "shutDown|Result\.ok|\.ok\b|shutdown" jans-cedarling/bindings/cedarling_js/scripts jans-cedarling/bindings/cedarling_js -g '*.js' -g '*.mjs' -g '*.ts' -g '*bind*' 2>/dev/null | head -n 200 || trueRepository: JanssenProject/jans
Length of output: 23064
🏁 Script executed:
#!/bin/sh
set -eu
file="jans-cedarling/bindings/cedarling_js/scripts/verify-consumer.mjs"
echo "--- exact verifier sections ---"
awk 'NR>=76 && NR<=110 { printf "%6d %s\n", NR, $0 }' "$file"
echo "--- client shutdown implementation ---"
awk 'NR>=350 && NR<=390 { printf "%6d %s\n", NR, $0 }' jans-cedarling/bindings/cedarling_js/src/client/client.ts
awk 'NR>=260 && NR<=280 { printf "%6d %s\n", NR, $0 }' jans-cedarling/bindings/cedarling_js/src/client/types.ts
awk 'NR>=84 && NR<=98 { printf "%6d %s\n", NR, $0 }' jans-cedarling/bindings/cedarling_js/src/client/types.ts
echo "--- result type docs ---"
awk 'NR>=120 && NR<=160 { printf "%6d %s\n", NR, $0 }' jans-cedarling/bindings/cedarling_js/src/errors/types.tsRepository: JanssenProject/jans
Length of output: 5692
Assert the shutDown() result.
client.shutDown() returns a Result; discard its ok and the verifier exits successfully even when shutdown fails. Capture the result and fail when it is not ok at both shutDown() call sites.
Proposed fix
- await result.value.shutDown();
+ const shutdownResult = await result.value.shutDown();
+ if (!shutdownResult.ok) {
+ throw new Error(`Cedarling shutdown failed: ${shutdownResult.error.code}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await result.value.shutDown(); | |
| const shutdownResult = await result.value.shutDown(); | |
| if (!shutdownResult.ok) { | |
| throw new Error(`Cedarling shutdown failed: ${shutdownResult.error.code}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jans-cedarling/bindings/cedarling_js/scripts/verify-consumer.mjs` at line 88,
Update both shutdown call sites in the verifier to capture the Result returned
by client.shutDown() and assert or otherwise validate its ok status, causing
verification to fail when shutdown is unsuccessful instead of discarding the
result.
olehbozhok
left a comment
olehbozhok
left a comment
There was a problem hiding this comment.
Fix 1: Add CedarEntityReference to CedarValue union
Priority: Low (unlikely use case)
Files: src/values/types.ts, src/values/snapshot.ts
Why: Cedar supports entity references in context values, entity attributes, and stored context data via the JSON marker { __entity: { type: "...", id: "..." } }. Rust handles this explicitly in mapper.rs:667 and entity_builder. JS does not model it — the object passes as a generic record without validation.
What to do:
1a. Add type to src/values/types.ts:
/**
* Cedar entity reference embedded in context or entity attributes.
*
* @example
* ```ts
* const ref: CedarEntityReference = {
* __entity: { type: "Jans::User", id: "alice" },
* };
* ```
*/
export interface CedarEntityReference {
readonly __entity: {
readonly type: string;
readonly id: string;
};
}Then add | CedarEntityReference to the CedarValue union:
export type CedarValue =
| CedarPrimitive
| CedarExtensionValue
| CedarEntityReference // ← add this
| readonly CedarValue[]
| { readonly [key: string]: CedarValue };1b. Handle __entity in snapshotCedarValueInner (src/values/snapshot.ts):
Before the __extn check, insert:
if (Reflect.ownKeys(value).includes("__entity")) {
return snapshotCedarEntityReference(value);
}And add the validator:
function snapshotCedarEntityReference(value: object): CedarEntityReference {
const keys = Reflect.ownKeys(value);
if (keys.length !== 1 || keys[0] !== "__entity") {
return invalidValue("Expected an entity reference with only __entity.");
}
const entityValue = (value as Record<string, unknown>).__entity;
if (!isPlainDataRecord(entityValue, false)) {
return invalidValue("Expected __entity to be a plain object.");
}
const type = entityValue.type;
const id = entityValue.id;
if (typeof type !== "string" || type.trim().length === 0) {
return invalidValue("Expected a non-empty entity type in __entity.");
}
if (typeof id !== "string" || id.trim().length === 0) {
return invalidValue("Expected a non-empty entity id in __entity.");
}
return { __entity: { type, id } };
}Expected impact: Users can now pass entity references in context and attributes with full type safety and validation.
Fix 2: Reject cedar_entity_mapping inside CedarEntity.attributes
Priority: Low
File: src/values/snapshot.ts or src/authorization/request.ts
Why: toGeneratedEntity() constructs { cedar_entity_mapping: { entity_type, id }, ...attributes } by spreading attributes. If the caller passes attributes: { cedar_entity_mapping: { ... } }, the user value overwrites the generated one. The boundary currently has no defense.
What to do:
In snapshotCedarValueInner, after the __extn / __entity checks and before snapshotObject, reject the reserved key:
// Inside snapshotCedarValueInner, after __extn / __entity checks
// Reject the reserved cedar_entity_mapping key in any Cedar object
if (Reflect.ownKeys(value).includes("cedar_entity_mapping")) {
return invalidValue(
"cedar_entity_mapping is reserved and cannot appear in Cedar values.",
);
}
return snapshotObject(value, ancestors, snapshotCedarValueInner);Alternatively, reject it only in snapshotEntity() (src/authorization/request.ts) — narrower scope, less risk of false positives, but doesn't protect context data. The global check in snapshotCedarValueInner is preferred since cedar_entity_mapping is SDK-owned everywhere.
Expected impact: User gets a clear error instead of a silently overwritten entity mapping.
Fix 3: snapshotAction(undefined) reports wrong error code
Priority: Low
File: src/authorization/request.ts
Why: When action field is completely absent from the request, field() returns undefined. Then snapshotAction(undefined) enters the object branch (typeof undefined !== "string" → false) and calls record(undefined, ["action"]) which fails with code "type" instead of "required".
What to do:
Add an early guard at the top of snapshotAction:
function snapshotAction(value: unknown): string {
if (value === undefined) {
return invalid("required", ["action"]);
}
// ... rest as before
}Expected impact: Missing action reports "required" instead of "type".
Not worth fixing
| # | What | Why not |
|---|---|---|
| 1 | CedarPrimitive.number allows float statically |
TS cannot express "safe integer". Runtime check is sufficient and documented. |
| 2 | Remove CedarContextValue/CedarContextObject aliases |
They serve as self-documenting semantic types in UnsignedAuthorizationRequest and MultiIssuerAuthorizationRequest. Zero runtime cost. |
| 3 | reason array flattening in response |
Cedar currently outputs a single flat array. If per-policy structured reasons are added in the future, this will fail the GENERATED_PROTOCOL_ERROR check — which is the right behavior for an incompatible protocol change. |
| 4 | access_count precision |
Wasm32 usize is u32 — always fits in safe integer. safeCounter() is defense-in-depth only. |
- 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>
…tion
- 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>
…ng 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>
3 participants
Prepare
Description
Target issue
Contributes to #14582.
This is a stacked PR targeting
jans-cedarling-14582and addresses the SDK-related review feedback on #14635. It does not close the target issue independently.Demo and example application changes are intentionally excluded and will be handled in a separate child PR.
Implementation Details
This PR aligns the Cedarling JavaScript SDK with the other Cedarling bindings while preserving a stable, JavaScript-native public API.
Key changes include:
createCedarling();authorizeUnsigned()andauthorizeMultiIssuer()methods;decision,requestId, and normalized diagnostics;shutDown(), including coordinated in-flight work and generated WASM resource disposal;The signed-token E2E suite uses a controlled local OpenID configuration and JWKS to verify valid RS256 tokens and reject tampered signatures, expired or untrusted claims, and disallowed algorithms.
Test and Document the changes
Validation completed:
npm run check, covering types, Node.js, Bun, Deno, Electron, workerd, Chromium, Firefox, WebKit, Vercel Edge, E2E consumers, and package staging.Please check the below before submitting your PR. The PR will not be merged if there are no commits that start with
docs:to indicate documentation changes or if the below checklist is not selected.Not selected because this PR has documentation impact and includes a dedicated
docs:commit with the required updates.Summary by CodeRabbit
New Features
Breaking Changes
close()toshutDown().Resultshape.Documentation
Tests