feature: process external plugin security audit - #840
Conversation
…into story/771-plugin-value-resolvers
|
Important Review skippedToo many files! This PR contains 274 files, which is 174 over the limit of 100. 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: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (274)
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 |
Includes changes from #830.
Closes generiekzaakafhandelcomponent/gzac-issues#821
Hardening & fixes for the external plugin system
Follow-up to
story/771-plugin-value-resolvers, addressing the findings of a full audit of theexternal plugin/app system (backend
external-pluginmodule, Node.js plugin host + SDK, and theAngular integration).
Security — token & proxy paths
window.location.originand hard-requires same-origin plus the API base-path prefix before attaching the downscoped user token. An empty endpoint allowlist means deny-all, andbundleUrlmust be http(s) before it is trusted./dataroute requires an existing configuration with the admin-grantedfrontend_datacapability and a valid user token, introspected against GZAC via a newGET /api/v1/external-plugin/user-token/introspectendpoint and bound to the exact configuration. Fail-closed (401/403/503), with a 60s positive cache and a per-configuration rate limit — no anonymous Wasm execution remains.GET configuration/{id}no longer returns decryptedx-secretproperties: secrets are masked on read, and absent secret fields on update keep the stored ciphertext. RabbitMQ credentials are redacted (amqp://***@…) in all host REST responses.Authorizationheader ingzac_api, and the host enforces the granted-endpoint allowlist on everygzac_apicall — GZAC sendsgrantedEndpointsin each config push so that enforcement is active.host-originsendpoint feeds CSP construction at bootstrap, so non-admin users no longer get CSP-blocked iframes (previously sourced from an ADMIN-only endpoint). The user-token mint response carries the granted endpoints, which all three surfaces feed into the iframe allowlist.SHA-256(secret|"service")vsSHA-256(secret|"user")) instead of one shared key distinguished only by a claim.Security — grants, denylist & host hardening
GETthe introspection endpoint.parentOriginoption) and queues data-bearing messages instead of posting to"*"; messages from other origins are ignored.Stability & correctness
gzac_apicallback fetch also has a timeout.actionFailed/hook-rejection paths (unreachable host → clear 503) instead of raw 500s; silently swallowed push/delete failures are logged.@SchedulerLock(the job class isopenso ShedLock's CGLIB proxy can subclass it), preventing duplicate polling and config re-pushes in multi-node deployments.getCurrentUserTeams()short-circuits for the system account again (regression from giving the system user a username).EXTERNAL_PLUGINtab contentKeys are validated up front (400 instead of 500/aborted import) via one shared parser with tolerant event listeners, and re-imports publish update events so tab type changes clean up stale side rows instead of falsely blocking configuration deletion.catchError+stopSaving, JSON-editor fallback on bundle-resolve failures). Token re-minting lives in one sharedExternalPluginSessionServicewith retry/backoff instead of three silent copies. The menu resolver matches the required Admin item by stable ID (no more duplicate empty Admin groups after a rename),IncludeFunctionpersists as an enum name instead of an ordinal, and/plugin-pagesroutes are registered in the gzac/valtimo apps, not just dev.tab_key156→256 typo,logicalFilePath/xsd alignment, master changelog include order, andON DELETE CASCADEon the granted-endpoint/event FKs. Note: unreleased changesets were edited in place, so local dev databases need a reset.frontend_datawas added to the GZACExternalPluginCapabilityenum (+ en/nl descriptions), so plugins declaring it in their manifest can actually be configured under the strict grant check.Quality, UX & cleanup
tab--external-pluginclass (no longer clips the widgets tab);::ng-deepscoped to:host. Plugin-management polling shares one 5s stream instead of three independent timers with deep-equal band-aids; the edit-modal subscription leak and stale-title carryover are fixed; the version-mismatch warning interpolates the actual versions.ngModel→ reactive FormControl,$-prefix naming corrected, drag listeners cleaned up inngOnDestroy, blocking startup initializers parallelized.accessToken, andStiSpike*test artifacts renamed. Host code deduplicated: genericcallExport, sharedguardHostCallcapability guard, shared abstract token filter, reuse ofProcessDefinitionUsageMetaResolver. The SDK pack tool stampssdkVersioninto manifests, validated on upload.aws/s3-resourceandcase-opensearch, and both modules added to the coverage-test classpath. MenuConfiguration gained a payload size limit, a shared singleton constant, and a security-asserting IT. Duplicate@valtimo-plugins/freemarkerkey removed fromrelease-plugins.json.Docs & tests
plugin-host/scratch/toplugin-host/docs/and was fully rewritten to describe the as-built system (strict grants, token introspection, the full/datagate chain);app/README.md,TESTING.md, and the SDK README are aligned.SystemPrincipalbehavior, the external-plugin tab service, and host-side timeout/zip-slip/replay/header-strip/allowlist/data-gating paths (~60+ new tests across the stack).Verification
:backend:external-plugin,:backend:plugin,:backend:case,:backend:keycloak-iam,:backend:admin-settingstest suites pass (incl. the MenuConfiguration IT on PostgreSQL).tsc --noEmitclean.@valtimo/pluginand@valtimo/componentsspecs pass.Known trade-offs (accepted by design)
/datarate limit and introspection cache are per-replica in-memory; host entities have no optimistic locking (ShedLock covers the practical race).