feat(install-dynamic-plugins): port from Python to TypeScript/Node.js#4574
Open
gustavolira wants to merge 8 commits into
Open
feat(install-dynamic-plugins): port from Python to TypeScript/Node.js#4574gustavolira wants to merge 8 commits into
gustavolira wants to merge 8 commits into
Conversation
Code Review by Qodo
1.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Review Summary by QodoPort install-dynamic-plugins from Python to TypeScript/Node.js with performance improvements
WalkthroughsDescription• **Complete rewrite**: Replaces Python-based install-dynamic-plugins.py with a TypeScript/Node.js
implementation (18 modules, 105 Jest tests)
• **Performance improvements**: Incorporates parallel OCI downloads, shared image cache, and
streaming SHA verification from the install-dynamic-plugins-fast.py POC
• **New package structure**: scripts/install-dynamic-plugins/ with TypeScript sources, bundled to
single dist/install-dynamic-plugins.cjs (~412 KB)
• **Runtime contract unchanged**: Same dynamic-plugins.yaml input schema, same output format, same
lock-file behavior, same {{inherit}} semantics
• **Resource-conscious concurrency**: Respects cgroup CPU limits with default workers `max(1,
min(floor(cpus/2), 6)), configurable via DYNAMIC_PLUGINS_WORKERS`
• **Memory-efficient**: Streaming tar extraction and SHA verification; typical 10-plugin run uses
20–80 MB peak RSS
• **Security parity**: Path traversal prevention, zip-bomb detection, symlink validation, device
file rejection, SRI integrity verification, registry fallback
• **CI updated**: Replaced pytest with npm run tsc && npm test plus bundle freshness
verification
• **Container image**: Updated Containerfile to copy .cjs bundle instead of .py; wrapper shell
script execs node instead of python
• **Deleted**: install-dynamic-plugins.py (1288 LOC), test_install-dynamic-plugins.py (3065
LOC), pytest.ini
• **Documentation**: Comprehensive README, updated user docs and CI references
Diagramflowchart LR
A["Python Script<br/>install-dynamic-plugins.py"] -->|"Replaced by"| B["TypeScript/Node.js<br/>18 modules + 105 tests"]
B -->|"Bundled to"| C["dist/install-dynamic-plugins.cjs<br/>~412 KB"]
C -->|"Copied in"| D["Container Image<br/>Containerfile"]
E["Parallel OCI<br/>Downloads"] -->|"Incorporated"| B
F["Shared Image<br/>Cache"] -->|"Incorporated"| B
G["Streaming SHA<br/>Verification"] -->|"Incorporated"| B
H["pytest"] -->|"Replaced by"| I["npm test<br/>Jest 105 tests"]
File Changes1. scripts/install-dynamic-plugins/src/index.ts
|
Contributor
|
The container image build workflow finished with status: |
Contributor
|
my browser cannot even load the 11,5k-line file 😅 |
Member
Author
@rostalan No need to review that file, it's the auto-generated esbuild bundle of src/. Just pushed a commit marking it as linguist-generated so GitHub collapses it in the diff. Only src/ and tests/ need review; CI verifies the bundle is up-to-date on every push. |
Contributor
|
The container image build workflow finished with status: |
gustavolira
added a commit
to gustavolira/rhdh
that referenced
this pull request
Apr 13, 2026
… feedback Addresses review feedback on PR redhat-developer#4574: - Extract shared helpers into `src/util.ts`: `fileExists`, `isInside`, `isPlainObject`, `isAllowedEntryType`, `markAsFresh`. Removes 4x duplication across `index.ts`, `catalog-index.ts`, `installer-oci.ts`, `merger.ts`, and `tar-extract.ts`. - Unify `catalog-index.ts` tar filter with `tar-extract.ts`: oversized entries now throw `InstallException` instead of being silently dropped; `OldFile` and `ContiguousFile` are accepted (were previously excluded by mistake). Uses the `isAllowedEntryType` helper. - Extract `markAsFresh(installed, pluginPath)` helper used by both installers to drop stale hash entries after a successful install. - `installer-npm.ts`: use `npm pack --json` instead of parsing the last line of text stdout (warnings on stdout would shift the filename). Also simplify the integrity-check flow — one gate that throws on missing hash, then one verify call (was two overlapping conditionals). - Split `Plugin` → `PluginSpec` (YAML schema) + `Plugin` (internal, with `_level`/`plugin_hash`/`version`). Makes it explicit which fields originate from user YAML vs runtime state. - `lock-file.ts`: add `DYNAMIC_PLUGINS_LOCK_TIMEOUT_MS` (default 10 min) so a stale lock from a `kill -9`'d process no longer wedges the init container forever. New test covers the timeout path. - Drop the broken `lint:check` script — it had `|| true` silencing every lint error and there is no ESLint config in the package. - `README.md`: remove stale reference to non-existent `cli.ts`, document the new lock-timeout env var, mention `util.ts`. Tests: 115 (was 114). Bundle rebuilt (413.4 KB).
Contributor
|
The container image build workflow finished with status: |
Contributor
|
The container image build workflow finished with status: |
gustavolira
added a commit
to gustavolira/rhdh
that referenced
this pull request
Apr 13, 2026
Resolves the 21 issues flagged by SonarQube on PR redhat-developer#4574: Prototype pollution (CodeQL, merger.ts) - `deepMerge` now assigns via `Object.defineProperty` (bypasses the `__proto__` setter on `Object.prototype`) in addition to the existing `FORBIDDEN_KEYS` guard. CodeQL recognizes this pattern. Redundant type assertions - `index.ts:180`: drop `pc as Record<string, unknown>` — use the `isPlainObject` type guard already imported from `util.ts`. - `installer-npm.ts:37`, `installer-oci.ts:35`: replace `(plugin.pluginConfig ?? {}) as Record<string, unknown>` with a typed local variable. - `installer-oci.ts:41,51,71,78`: drop `as string` casts by restructuring the `isAlreadyInstalled` helper with proper `undefined` checks. - `merger.ts:136-140`: replace `.slice(-1)[0] as string` with `.at(-1) ?? ''`. - `merger.ts:215`: `ReadonlyArray<keyof Plugin | string>` collapses to `ReadonlyArray<string>`. Cognitive complexity reductions - `installOciPlugin` (17 → ~10): extract `resolvePullPolicy` and `isAlreadyInstalled` helpers. - `mergeOciPlugin` (20 → ~12): extract `resolveInherit`. - `npmPluginKey` (16 → ~7): extract `tryParseAlias`, `isGitLikeSpec`, `stripRefSuffix`. - `ociPluginKey`: extract `autoDetectPluginPath`. Modern JS / readability (es2015-es2022) - `integrity.ts`: `charCodeAt` → `codePointAt` (es2015). - `oci-key.ts`: use `String.raw` for the regex pieces containing `\s`, `\d`, `\]`, `\\` instead of escaped string literals (es2015). - `oci-key.ts:escape`: `.replace(/.../g, ...)` → `.replaceAll(...)` (es2021). - `plugin-hash.ts`: pass an explicit code-point comparator to `sort` so deterministic-hash behavior is spelled out. `localeCompare` is NOT used — it varies per-locale and would break hash stability. All 115 tests still pass. Bundle rebuilt (415.1 KB).
Contributor
|
The container image build workflow finished with status: |
Member
Author
|
/review |
PR Reviewer Guide 🔍(Review updated until commit ca0c4c6)Warning
Here are some key observations to aid the review process:
|
Contributor
|
The container image build workflow finished with status: |
gustavolira
added a commit
to gustavolira/rhdh
that referenced
this pull request
Apr 13, 2026
… feedback Addresses review feedback on PR redhat-developer#4574: - Extract shared helpers into `src/util.ts`: `fileExists`, `isInside`, `isPlainObject`, `isAllowedEntryType`, `markAsFresh`. Removes 4x duplication across `index.ts`, `catalog-index.ts`, `installer-oci.ts`, `merger.ts`, and `tar-extract.ts`. - Unify `catalog-index.ts` tar filter with `tar-extract.ts`: oversized entries now throw `InstallException` instead of being silently dropped; `OldFile` and `ContiguousFile` are accepted (were previously excluded by mistake). Uses the `isAllowedEntryType` helper. - Extract `markAsFresh(installed, pluginPath)` helper used by both installers to drop stale hash entries after a successful install. - `installer-npm.ts`: use `npm pack --json` instead of parsing the last line of text stdout (warnings on stdout would shift the filename). Also simplify the integrity-check flow — one gate that throws on missing hash, then one verify call (was two overlapping conditionals). - Split `Plugin` → `PluginSpec` (YAML schema) + `Plugin` (internal, with `_level`/`plugin_hash`/`version`). Makes it explicit which fields originate from user YAML vs runtime state. - `lock-file.ts`: add `DYNAMIC_PLUGINS_LOCK_TIMEOUT_MS` (default 10 min) so a stale lock from a `kill -9`'d process no longer wedges the init container forever. New test covers the timeout path. - Drop the broken `lint:check` script — it had `|| true` silencing every lint error and there is no ESLint config in the package. - `README.md`: remove stale reference to non-existent `cli.ts`, document the new lock-timeout env var, mention `util.ts`. Tests: 115 (was 114). Bundle rebuilt (413.4 KB).
gustavolira
force-pushed
the
feat/install-dynamic-plugins-ts
branch
from
b76ee6a to
dd84f75
Compare
gustavolira
added a commit
to gustavolira/rhdh
that referenced
this pull request
Apr 13, 2026
Resolves the 21 issues flagged by SonarQube on PR redhat-developer#4574: Prototype pollution (CodeQL, merger.ts) - `deepMerge` now assigns via `Object.defineProperty` (bypasses the `__proto__` setter on `Object.prototype`) in addition to the existing `FORBIDDEN_KEYS` guard. CodeQL recognizes this pattern. Redundant type assertions - `index.ts:180`: drop `pc as Record<string, unknown>` — use the `isPlainObject` type guard already imported from `util.ts`. - `installer-npm.ts:37`, `installer-oci.ts:35`: replace `(plugin.pluginConfig ?? {}) as Record<string, unknown>` with a typed local variable. - `installer-oci.ts:41,51,71,78`: drop `as string` casts by restructuring the `isAlreadyInstalled` helper with proper `undefined` checks. - `merger.ts:136-140`: replace `.slice(-1)[0] as string` with `.at(-1) ?? ''`. - `merger.ts:215`: `ReadonlyArray<keyof Plugin | string>` collapses to `ReadonlyArray<string>`. Cognitive complexity reductions - `installOciPlugin` (17 → ~10): extract `resolvePullPolicy` and `isAlreadyInstalled` helpers. - `mergeOciPlugin` (20 → ~12): extract `resolveInherit`. - `npmPluginKey` (16 → ~7): extract `tryParseAlias`, `isGitLikeSpec`, `stripRefSuffix`. - `ociPluginKey`: extract `autoDetectPluginPath`. Modern JS / readability (es2015-es2022) - `integrity.ts`: `charCodeAt` → `codePointAt` (es2015). - `oci-key.ts`: use `String.raw` for the regex pieces containing `\s`, `\d`, `\]`, `\\` instead of escaped string literals (es2015). - `oci-key.ts:escape`: `.replace(/.../g, ...)` → `.replaceAll(...)` (es2021). - `plugin-hash.ts`: pass an explicit code-point comparator to `sort` so deterministic-hash behavior is spelled out. `localeCompare` is NOT used — it varies per-locale and would break hash stability. All 115 tests still pass. Bundle rebuilt (415.1 KB).
Contributor
|
The container image build workflow finished with status: |
Member
Author
|
/review |
|
Persistent review updated to latest commit ee984d5 |
jonkoops
approved these changes
Apr 28, 2026
Contributor
|
This PR is stale because it has been open 7 days with no activity. Remove stale label or comment or this will be closed in 21 days. |
… the TS implementation Brings the TS port back in sync with three commits that landed on main while this PR was open: - redhat-developer#4576 (OCI disable pre-merge): pre-compute the set of OCI registries that will be effectively disabled after merging, then filter them out of every plugin list before the merge calls skopeo. Avoids wasted remote fetches for plugins the operator already disabled at a higher level. New helpers in src/merger.ts (preMergeOciDisabledState, filterDisabledOciPlugins) and src/oci-key.ts (tryParseOciRegistryAndPath). loadAllPlugins() in src/index.ts now reads every include file up front so the pre-merge pass can run before mergePlugin() touches the OCI cache. - redhat-developer#4666 (MAX_ENTRY_SIZE bump): default from 20MB to 40MB. - redhat-developer#4655 (EXTRA_CATALOG_INDEX_IMAGES): comma-separated catalog index images, each extracted into <CATALOG_ENTITIES_EXTRACT_DIR>/extra/<name>/ catalog-entities. New helpers in src/catalog-index.ts (extractCatalogIndexLayers refactored out of extractCatalogIndex, extractExtraCatalogIndex, parseExtraCatalogIndexImages, imageRefToSubdirectory). New maybeExtractExtraCatalogIndexes() in src/index.ts wires it into runInstaller after the primary catalog index. Tests: 27 new cases under __tests__/merger-pre-merge.test.ts (15) and __tests__/extra-catalog-index.test.ts (12) covering the level-override matrix, ambiguous-pathless detection, same-level duplicates, invalid OCI strings, extensions/marketplace fallback, missing-entities warning, and the duplicate-subdir overwrite warning ordering. Existing tar-extract.test.ts + types.test.ts updated for the new 40MB default. All 155 vitest cases pass. Bundle rebuilt.
gustavolira
force-pushed
the
feat/install-dynamic-plugins-ts
branch
from
8fad1af to
4a2a8cb
Compare
|
New changes are detected. LGTM label has been removed. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4574 +/- ##
==========================================
- Coverage 53.65% 52.86% -0.80%
==========================================
Files 121 109 -12
Lines 2350 2132 -218
Branches 539 536 -3
==========================================
- Hits 1261 1127 -134
+ Misses 1084 1004 -80
+ Partials 5 1 -4
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
…deQL The shared Set.has() lookup wasn't recognised by CodeQL's js/prototype-polluting-function rule as an exhaustive sanitizer, even though Object.defineProperty already bypasses the __proto__ setter. Replace it with the inlined string-literal pattern CodeQL accepts. Closes CodeQL alert redhat-developer#116 on PR redhat-developer#4574 (safeSet at merger.ts:36).
Contributor
|
The container image build workflow finished with status: |
Member
Author
|
/review |
|
Persistent review updated to latest commit ca0c4c6 |
…-developer#4574 - catalog-index.ts: invert the eq===-1 branch so the positive case comes first (Sonar S3923 'Unexpected negated condition'). - index.ts: drop five redundant type assertions on PluginSpec / Plugin / IncludePluginList — TS already narrows correctly thanks to PluginSpec being a structural subset of Plugin and Array of being assignable to ReadonlyArray. - merger.ts: * Split preMergeOciDisabledState into top-level helpers (processOciEntry, recordEntryState, recordRegistryPath, validateAmbiguousPathless, effectiveRegistryDisabled, computeDisabledRegistries) so the orchestrator stays well under the cognitive-complexity ceiling. * Replace the nested ternary in the explicit-paths sort with String#localeCompare. * Replace 'bucket && bucket.size === 1' with optional-chain 'bucket?.size === 1'. * Drop a stray NUL byte in the entry-key template that crept in via an early Edit and was making the file impossible to diff in a few tools. 155 vitest cases still pass; bundle rebuilt.
Contributor
|
The container image build workflow finished with status: |
…dir names Qodo's persistent review flagged that the explicit 'name=<ref>' form of EXTRA_CATALOG_INDEX_IMAGES does not validate 'name', so an operator could accidentally (or maliciously) extract an extra catalog index outside of '<entitiesDir>/extra/' by passing names containing '..' or path separators. The auto-derived form is already safe because imageRefToSubdirectory strips '/', ':', and '@'. Add isSafeSubdirectoryName() that rejects empty names, '.', '..', and any name containing '/' or '\\'. Parsing logs a warning and skips the entry instead of crashing the installer, matching the existing 'empty image reference' handling. 6 new vitest cases cover the rejected forms and document that the check is character-based (so URL-encoded separators like '%2F' are accepted verbatim rather than decoded). Bundle rebuilt (225.6 kB).
Contributor
|
The container image build workflow finished with status: |
Targeted cleanups identified in the in-conversation review. No behaviour
changes outside of the two defense-in-depth guards listed below.
Correctness/security:
- installer-npm.ts: validate that the filename emitted by 'npm pack
--json' is flat (no '/', no '\\', no leading '..') before joining it
to 'destination'. A registry returning '../evil.tgz' would otherwise
let extraction escape.
- installer-oci.ts: replace 'pkg.split("!")' with an indexOf-based
splitOciPackage helper so plugin paths containing a literal '!' are no
longer silently truncated. Applied to both the install path and the
Always-policy digest comparison.
- catalog-index.ts: re-validate 'subdirectory' inside
extractExtraCatalogIndex itself instead of trusting all callers to go
through parseExtraCatalogIndexImages. Five new vitest cases cover the
rejected forms.
- index.ts: drop the existsSyncSafe wrapper and use Node's existsSync
directly (the wrapper was a literal reimplementation that also
conflated 'doesn't exist' with 'no read permission').
- image-cache.ts: surface a typed InstallException with the image ref
when 'io.backstage.dynamic-packages' is unparseable, instead of
letting JSON.parse crash the install.
Maintainability:
- types.ts: new effectivePullPolicy() helper centralises the ':latest!'
fallback that was duplicated between installer-oci.ts and
definitelyNoOp in index.ts.
- merger.ts: copyPluginFields now uses a for-of loop with safeSet
(same prototype-pollution guard as deepMerge) instead of building an
intermediate object via Object.fromEntries. effectiveRegistryDisabled
drops the 'as string' cast via destructuring + undefined check.
IncludePluginList downgraded from exported to internal.
- tar-extract.ts: comment the POSIX assumption around the
pluginPathBoundary check; the trailing slash is unambiguous now.
- index.ts: destructure process.argv to remove the 'as string' cast.
- installer-npm.ts: extract isNpmPackJsonEntry() type guard so the
install path stays linear.
- concurrency.ts: replace the magic-number worker caps (6/3) with
MAX_OCI_WORKERS / MAX_NPM_WORKERS named constants.
- catalog-index.ts: simplify isSafeSubdirectoryName (the second clause
was unreachable once '/' and '\\' were rejected upfront).
166 vitest cases (was 161; +5 new defense-in-depth tests). Bundle
rebuilt (226.0 kB).
Contributor
|
The container image build workflow finished with status: |
- Generic 'safeSet<T extends object>' removes the awkward 'dst as unknown as Record<string, unknown>' double-cast in copyPluginFields. The Object.defineProperty signature already accepts any object, so the constraint widening is enough. - Drop the redundant 'as unknown' on parseYaml's return in mergePluginsFromFile — yaml v2's parse() is already typed unknown. No behaviour changes. 166 vitest cases still pass.
Contributor
|
The container image build workflow finished with status: |
The 'Upload test results to Codecov' step at the monorepo level looks at $RUNNER_TEMP/test-results/ for JUnit reports. Because this PR doesn't touch any yarn workspace, 'yarn run test --affected' skips all workspace tests and the directory ends up empty — codecov[bot] then posts a 'JUnit XML file not found' warning on the PR. Run vitest with the junit reporter so it writes its own report into $RUNNER_TEMP/test-results/install-dynamic-plugins.junit.xml; the existing test-results-action then finds it (alongside any workspace reports) and the warning stops firing. No new upload step needed — test-results-action already searches that directory. Verified locally: 'JUNIT report written to /tmp/.../junit.xml' (166 testcases serialized).
Contributor
|
The container image build workflow finished with status: |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Replaces the Python-based
install-dynamic-plugins.pyinit-container script with a TypeScript/Node.js implementation, incorporating all improvements from theinstall-dynamic-plugins-fast.pyPOC (#4523): parallel OCI downloads, shared image cache, streaming SHA verification.Motivation: The Python script was a longstanding duplication target for export overlays (see rhdh-plugin-export-overlays#2231). A Node.js implementation removes the Python dependency from the init-container runtime, enables reuse across RHDH core and overlays, and adopts the faster parallel architecture natively.
What changed
scripts/install-dynamic-plugins/— 18 TypeScript modules + 9 Jest test files (105 tests)dist/install-dynamic-plugins.cjs(~412 KB), committed and verified fresh in CI (same pattern as.yarn/releases/yarn-*.cjs).cjsbundle instead of.py; wrapper shell script execsnodeinstead ofpythonnpm run tsc && npm test+ bundle freshness check (replacespytest)install-dynamic-plugins.py(1288 LOC),test_install-dynamic-plugins.py(3065 LOC),pytest.iniKey design decisions
Runtime contract is unchanged
Same
dynamic-plugins.yamlinput schema, sameapp-config.dynamic-plugins.yamloutput, samedynamic-plugin-config.hash/dynamic-plugin-image.hashfiles, same lock-file behaviour, same{{inherit}}semantics and OCI path auto-detection.Resource-conscious concurrency
availableParallelism()respects cgroup CPU limits (init containers often get 0.5 CPU)max(1, min(floor(cpus/2), 6))— cap avoids exhausting registry/networkDYNAMIC_PLUGINS_WORKERS=<n>Memory: streaming everywhere
node-tarstreams extraction — no full-archive read into RAMnode:cryptopipeline for SHA integrity — chunks through the hashSecurity parity with Python
.., absolute)tar-extract.tsMAX_ENTRY_SIZE)tar-extract.ts,catalog-index.tstar-extract.tstar-extract.tspackage/prefix enforced for NPM tarballstar-extract.tssha256/sha384/sha512)integrity.tsregistry.access.redhat.com/rhdh→quay.io/rhdhimage-resolver.tsTest plan
npm run tscpasses (strict mode,noUncheckedIndexedAccess)npm test— 105 Jest tests pass (npm-key, oci-key, integrity, tar-extract, merger, concurrency, lock-file, image-resolver, plugin-hash)npm run buildproduces freshdist/install-dynamic-plugins.cjs.cjs(CI will verify)fast.pybaseline (~2:42 for full catalog)Compatibility
skopeois still installed for OCI inspectioninstall-dynamic-plugins.shwrapper contract unchanged (./install-dynamic-plugins.sh /dynamic-plugins-root)Related