Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2d47cf6cb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…sed config - add tooling/generate-kms-caroots.js to download, verify (against pinned Cisco trust anchors), and decode the Cisco Trusted Root Store Union bundle into the caroots format - add tooling/with-kms-caroots.sh wrapper and caroots:generate script - default encryption.caroots from the WEBEX_KMS_CAROOTS env var - run browser and integration CI jobs with generated CA roots so validation is exercised against the real KMS - disable KMS cert validation in sample apps that do not ship a bundle - document the tooling and env var in the plugin README
…e, not env The previous env-var delivery was unusable: a ~362KB single environment variable exceeds the per-string exec limit (MAX_ARG_STRLEN), and reading the bundle from a file in config.js would break consuming browser app builds. - keep the shipped config a pure library: caroots defaults to undefined with no file or environment I/O - deliver generated roots to the encryption integration/browser tests via a committed test-only fixture that the wrapper populates and then restores - disable KMS cert validation in plugin-messages integration tests (they test messaging, not certificate validation) - retry transient downloads in the generator
…s, not CI The pull_request_target workflow runs the base branch's YAML, so a wrapper prefix added in the PR never executes. Move CA-roots generation into the encryption package's test:integration and test:browser scripts (which run from PR code) and revert the redundant workflow wrapper.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a53c931e98
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function download(url) { | ||
| return new Promise((resolve, reject) => { | ||
| https | ||
| .get(url, {headers: {'user-agent': 'webex-js-sdk-kms-caroots'}}, (res) => { |
There was a problem hiding this comment.
Add a timeout to CA bundle downloads
When a Cisco endpoint accepts the connection but stops sending data, this https.get() request has no timeout and therefore emits neither end nor error. Because both encryption integration scripts invoke this downloader, downloadWithRetry() never advances to another attempt and the test job can hang until the outer CI timeout; configure a request/socket timeout that destroys the request so the retry logic can run.
Useful? React with 👍 / 👎.
robstax
left a comment
There was a problem hiding this comment.
couple questions. otherwise, technically makes sense. i read up a bit on KMS, ECDH, and caroots :D :D
| @@ -0,0 +1,312 @@ | |||
| #!/usr/bin/env node | |||
There was a problem hiding this comment.
i'm a little confused by these. are consumers of the SDK expected to run these scripts? and if so, do these actually get bundled/published?
There was a problem hiding this comment.
consumers of the SDK will run these scripts and generate the files necessary for inclusion in their own configs. The JS-SDK itself will not publish this in the bundle
| before('create test user', () => | ||
| testUsers.create({count: 1}).then((users) => { | ||
| user = users[0]; | ||
| webex = new WebexCore({ |
There was a problem hiding this comment.
i think there are several other plugins that use encryption that might need to be updated like this. for example
i think these tests would reject/fail now. they didn't run in CI because of the SDK CI tooling only running changed packages, but i think these will fail. there's probably other plugins like team, board, lyra, etc that have this issue in tests
…A roots for tests Add a small, publishable @webex/kms-caroots package (openssl + node, with a webex-kms-caroots bin) that downloads, verifies against pinned Cisco anchors, and decodes the Cisco Trusted Root Store Union bundle into the encryption.caroots format, so consumers can supply roots at their own build time. Incorporate generation into the test runner: @webex/legacy-tools now calls the package before integration/browser tests and configures webex-core so the KMS certificate is validated against the real trust store for every plugin, with no per-spec wiring. Falls back to disabling validation if generation fails. Remove the earlier per-package wrapper, spec fixtures, and message-test opt-out.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a2d7ca33c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
| } finally { | ||
| if (carootsBootstrap) { | ||
| carootsBootstrap.cleanup(); |
There was a problem hiding this comment.
Keep the bootstrap until Karma completes
Karma.test() resolves immediately after calling server.start() rather than after the Karma run completes (packages/legacy/tools/src/utils/karma/karma.ts:62-63), so this finally deletes the bootstrap while Karma is still preparing/browserifying its files list. Every browser integration run that enables this new bootstrap can therefore see its first test file disappear (or run without the config mutation), preventing the intended KMS validation setup; defer cleanup to Karma's completion callback.
Useful? React with 👍 / 👎.
…strap Karma.test resolved immediately after server.start() (the completion callback, including its reject-on-failure, was dead code), so the finally block deleted the KMS bootstrap while Karma was still browserifying its files — the first test file could disappear or run without the config mutation. Resolve/reject via Karma's completion callback for single-run (only resolve immediately in watch/debug mode), and skip bootstrap cleanup in debug mode. This also restores propagation of Karma exit codes, which were previously swallowed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72a2c7f57c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| console.warn( | ||
| `KMS CA roots: generation failed (${(error as Error).message}); skipping KMS cert validation`, | ||
| ); | ||
| body = 'config.encryption.shouldValidateKMSCertificate = false;'; |
There was a problem hiding this comment.
Apply the fallback after plugin defaults
When generateKmsCaroots() fails (for example, in an offline environment or without OpenSSL), this bootstrap runs before the specs, but importing internal-plugin-encryption later registers its configuration via merge(config, options.config), whose new shouldValidateKMSCertificate: true default overwrites this false. Because no roots were generated, KMS integration tests then fail closed instead of taking the documented fallback; apply the opt-out after plugin registration or preserve an explicitly configured false while merging defaults.
Useful? React with 👍 / 👎.
Package.test now prepends a generated CA-roots bootstrap for integration runs. Stub KmsCaroots.prepareTestBootstrap so the tests do no network/file I/O, and expect the bootstrap prepended to the mocha/karma file lists. This also fixes the test-server specs, which were failing as collateral from the un-mocked generation's real I/O and unhandled rejection.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0823d5980f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| "test:integration": "node ./tooling/index.js test --integration --browser", | ||
| "test:ci:github": "node ./tooling/index.js ci --github", | ||
| "test:ci:integration": "node ./tooling/index.js ci --integration", | ||
| "caroots:generate": "webex-kms-caroots", |
There was a problem hiding this comment.
Declare the generator CLI in the root workspace
On a normal root install, yarn caroots:generate runs with only the root workspace's dependency binaries on PATH. @webex/kms-caroots is declared only by packages/legacy/tools, so its webex-kms-caroots bin is not available to this script; the new root command exits command not found unless the CLI was installed globally. Add the package as a root dependency or invoke it through its workspace.
Useful? React with 👍 / 👎.
…leanup Karma's browserify preprocessor skips dotfiles, so the injected .kms-caroots.bootstrap.js was never bundled and browser integration tests failed with 'no CA roots configured'. Rename it to a non-dotfile and use a robust webex-core config lookup. - coerce a single-value --karma-browsers option to an array - map isbot to its CommonJS entry so browserify can bundle internal-plugin-metrics - resolve Karma.test on run completion (so bootstrap cleanup runs after the run) without changing karma failure-gating behavior
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 348a17e253
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const server = new KarmaRunner.Server(parsedConfig, () => { | ||
| resolve(undefined); |
There was a problem hiding this comment.
Propagate non-zero Karma exit codes
When a single-run browser suite fails, Karma invokes this completion callback with a non-zero code, but this callback now resolves unconditionally. Consequently Package.test() and the webex-legacy-tools test command exit successfully even when Karma reports failed tests; the existing Karma test explicitly expects a rejection for callback(1). Reject on a non-zero code while retaining completion-based cleanup.
Useful? React with 👍 / 👎.
| // The generated bootstrap file, required first so it can configure the shared | ||
| // webex-core config before any WebexCore instance is constructed. Not a dotfile, | ||
| // so karma's browserify preprocessor (which skips dotfiles) still bundles it. | ||
| const BOOTSTRAP_FILENAME = 'kms-caroots.bootstrap.js'; |
There was a problem hiding this comment.
Align the bootstrap filename with its test
prepareTestBootstrap() writes kms-caroots.bootstrap.js, but the newly added KmsCaroots integration test asserts that the returned path is .kms-caroots.bootstrap.js. Once legacy-tools is built from this commit, that assertion fails on every run of its integration suite; update the expectation or use one filename consistently.
Useful? React with 👍 / 👎.
COMPLETES #< INSERT LINK TO ISSUE >
This pull request addresses
Enable validation of KMS certificate signature by default. I haven't checked in a complete CA list because we don't want to tie a particular SDK version to the latest cert list. There is tooling and documentation to help consuming applications get the latest version.
by making the following changes
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.