support insecure-algorithms manifest permissions flag#3967
support insecure-algorithms manifest permissions flag#3967phillipivan wants to merge 4 commits intobitfocus:mainfrom
Conversation
📝 WalkthroughWalkthroughBuilds Node.js permission arguments incrementally, extracts manifestPermissions earlier, conditionally applies Changes
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
companion/lib/Instance/NodePath.ts
Outdated
There was a problem hiding this comment.
your addition probably wants to be above this enableInspect check, so that it applies when that is enabled.
Probably needs this line split up a bit to allow different routes.
There was a problem hiding this comment.
This should be addressed now, Julian.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
companion/lib/Instance/NodePath.ts (1)
43-46: Optional:doesModuleSupportPermissionsModelsemantically gates an unrelated flag.
--openssl-legacy-provideris an OpenSSL crypto flag, independent of the Node.js permission model. Right now it can only be reached becauseinsecure-algorithmsis new enough (companion-module-base ≥ 1.15.0 > 1.12.0) that any module setting it will always pass thedoesModuleSupportPermissionsModelcheck — so there's no practical bug today.That said, if you ever want to make the intent clear and guard against future drift, you could hoist the OpenSSL push above the permissions-model gate:
✨ Optional refactor to decouple OpenSSL from the permissions model gate
const args: string[] = [] if (manifest.type === 'surface') return args -if (!doesModuleSupportPermissionsModel(moduleApiVersion)) return args const manifestPermissions = manifest.runtime.permissions || {} //@ts-expect-error until companion-module-base 1.15.0 released and included if (manifestPermissions['insecure-algorithms']) args.push('--openssl-legacy-provider') +if (!doesModuleSupportPermissionsModel(moduleApiVersion)) return argsTotally fine to leave as-is for now — just something to keep in mind if the module-base version coupling ever changes.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
companion/lib/Instance/NodePath.ts (2)
46-52: Theinsecure-algorithmshandling looks good! Small thought on the@ts-expect-error.The placement of
--openssl-legacy-providerbefore the--permissionblock is correct, and it's nice that node18 and inspect early-returns still carry this flag through.One small thought: the
@ts-expect-errorcomment mentions "until companion-module-base 1.15.0 released and included" — might be worth adding a tracking issue link or TODO tag so it's easy to find and clean up later. Totally up to you though!
63-67: Heads up: the../../..traversal from the resolved path could be fragile.The number of parent directory hops depends on where
require.resolve('@companion-module/host')lands (e.g.,dist/index.jsvsindex.js). If the package's entry point depth changes, this could resolve to an unexpectedly broad or narrow directory.Since this is dev-only, it's not critical — just something to keep in mind. You could consider resolving the
package.jsonof@companion-module/hostdirectly (e.g., viarequire.resolve('@companion-module/host/package.json')) and then usingpath.dirname()once for a more stable reference. No pressure though — totally a "nice to have"!
Combines with this PR for companion-module-base to address this issue.
//@ts-expect-errorcan be removed once PR for companion-module-base is merged and releasedSummary by CodeRabbit
Bug Fixes
New Features