fix(cc-widgets): remediate P1 security scan findings - #726
Conversation
|
Scope note for SPARK-833337: The original remediation also changed .github/workflows/update-dependencies.yml for CG-01 / AC-1. GitHub rejected that branch push because the Jira-to-PR automation token does not have the workflow permission required to modify GitHub Actions workflow files. For this PR, that workflow-file change was intentionally restored to origin/next, allowing the remaining five fixes (WF-05, WF-06, WF-07, WF-08, and WF-10) to be pushed and reviewed. CG-01 / AC-1 is not fixed by this PR and requires a follow-up change pushed by a credential with workflow-write permission or by an authorized human. |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f88d95b6f
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (process.env.NODE_ENV !== 'production') { | ||
| // @ts-expect-error: expose store for debugging in non-production builds only | ||
| window.store = store; |
There was a problem hiding this comment.
When samples-cc-wc-app is built with NODE_ENV=production, this guard skips assigning window.store, but app.js still starts with const store = window.store and later calls store.setCurrentTheme()/store.init(). That makes the production web-component sample throw instead of initializing any widgets; if the global exposure must be removed, app.js needs another production-safe way to receive the imported store before this assignment is gated.
Useful? React with 👍 / 👎.
| const [integrationEnv, setintegrationEnv] = useState( | ||
| process.env.REACT_APP_INTEGRATION_ENV === 'true' | ||
| ); |
There was a problem hiding this comment.
When a user enables Integration Env in the React sample and starts OAuth, the full-page redirect remounts the app; this new initializer no longer reads the previous localStorage value and relies on process.env.REACT_APP_INTEGRATION_ENV, but the sample webpack config only shims process and does not define that variable. The callback therefore comes back with integrationEnv reset to false, so the returned integration token is initialized against the production U2C/client configuration unless the user manually toggles it again.
Useful? React with 👍 / 👎.
| } | ||
| // Initialize the digital channels app only once per session | ||
| if (!isDigitalChannelsInitialized) { | ||
| initInFlightRef.current = true; |
There was a problem hiding this comment.
If initializeApp(dataCenter, jwtToken) rejects once due to a transient token/network/datacenter issue, this new in-flight flag stays true because the catch block only logs the error while isDigitalChannelsInitialized remains false. Any later effect run for a refreshed token or task will return at the guard and the digital-channels widget will keep rendering null until the hook is unmounted; reset the ref on failure or in a finally path that distinguishes success from failure.
Useful? React with 👍 / 👎.
akulakum
changed the title
akulakum
added
validated
github-actions
Bot
removed
the
validated
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7dae08aa8
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (initInFlightRef.current) { | ||
| return; |
There was a problem hiding this comment.
When a host keeps DigitalChannels mounted across an agent logout/login, the first successful initialization leaves initInFlightRef.current true permanently. cleanUpStore() explicitly resets isDigitalChannelsInitialized to false on logout (packages/contact-center/store/src/storeEventsWrapper.ts:1152-1168), but when the next task arrives this guard returns before reinitialization; the local initialized state also remains true, so Engage can render for the new session without calling initializeApp again. Re-key or clear the guard when the store starts a new session.
Useful? React with 👍 / 👎.
Kesari3008
left a comment
Kesari3008
left a comment
There was a problem hiding this comment.
Are we addressing below security scan finding in this PR:
CG-01 — GitHub Actions script injection via client_payload.version (.github/workflows/update-dependencies.yml)
I don;t see the change for it.
|
|
||
| webex.once('ready', () => { | ||
| webex.authorization.initiateLogin(); | ||
| // Pass our state token so the SDK embeds it in the authorize URL; |
There was a problem hiding this comment.
Use multi-line comment instead of using ?? for each comment line
There was a problem hiding this comment.
Done in ae4cff6 — converted to block comments for OAuth CSRF sections.
| webex.authorization.initiateLogin(); | ||
| // Pass our state token so the SDK embeds it in the authorize URL; | ||
| // the redirect handler validates the returned state against sessionStorage. | ||
| webex.authorization.initiateLogin({state: {app_state: stateToken}}); |
There was a problem hiding this comment.
Why is this a concern only for React App and not in meeting widget samples app and web components app
There was a problem hiding this comment.
WF-10 scope was CC React + WC samples (App.tsx, wc index.ts). Meeting samples are out of P1; can follow up separately. WC sample has the same NODE_ENV guard; app.js still reads window.store (dev/internal use).
| return savedintegrationEnv === 'true'; | ||
| }); | ||
| const [integrationEnv, setintegrationEnv] = useState( | ||
| process.env.REACT_APP_INTEGRATION_ENV === 'true' |
There was a problem hiding this comment.
Why are we setting it true always
There was a problem hiding this comment.
It’s not always true — default is process.env.REACT_APP_INTEGRATION_ENV === 'true' (false unless build sets it). Removed localStorage persistence per WF-06 (security). UI toggle still works within the session. OAuth redirect reset is a known sample limitation; can use REACT_APP_INTEGRATION_ENV=true build or follow-up if needed.
| | `CC-COMPONENTS-R-008` | `IncomingTaskComponent` renders the standard `Task` with Answer/Decline when an `incomingTask` is present and renders nothing (hidden) when it is absent; Accept/Decline invoke `accept(task)`/`reject(task)`. | Avoids a stray empty notification when no task; routes accept/decline intent up. | `src/components/task/IncomingTask/incoming-task.tsx`, `src/components/task/IncomingTask/incoming-task.utils.tsx` (`extractIncomingTaskData`) | `tests/components/task/IncomingTask/incoming-task.tsx`, `tests/components/task/IncomingTask/incoming-task.utils.tsx` | None | PRESENT | | ||
| | `CC-COMPONENTS-R-009` | `TaskListComponent` renders nothing when the task list is empty, otherwise renders one row per task; campaign preview tasks render `CampaignTask` (instead of `Task`) only when `hasCampaignPreviewEnabled` (default true) and the task is a campaign preview. | List must collapse when empty and switch row UI for campaign previews per the feature flag. | `src/components/task/TaskList/task-list.tsx`, `src/components/task/TaskList/task-list.utils.ts` (`isTaskListEmpty`, `getTasksArray`, `isCampaignPreviewTask`, `getActiveCampaignPreviewId`) | `tests/components/task/TaskList/task-list.tsx`, `tests/components/task/TaskList/task-list.utils.tsx` | None | PRESENT | | ||
| | `CC-COMPONENTS-R-010` | `OutdialCallComponent` validates the entered destination, supports dialpad / ANI / address-book tabs, disables the outdial action while a telephony task is active, and calls `startOutdial(destination, origin?)`. | Outbound dialing must validate input and not start a second call over an active one. | `src/components/task/OutdialCall/outdial-call.tsx`, `src/components/task/OutdialCall/constants.ts` | `tests/components/task/OutdialCall/out-dial-call.tsx` | None | PRESENT | | ||
| | `CC-COMPONENTS-R-016` | The DN validation regex in `OutdialCallComponent` uses the explicit alternation `(\+\|1)` (not the char-class `[+1]`) for the first-character prefix in branches 1 and 2. Both patterns match the same inputs, but the alternation form makes the intent — a `+` (international) or `1` (North-American) prefix — unambiguous to readers and static-analysis tools. | Security scan WF-07: char-class `[+1]` was flagged for regex-intent ambiguity; explicit alternation removes the finding without changing accepted inputs. +12345678901 must remain valid. | `src/components/task/OutdialCall/outdial-call.tsx` (regex: `^(\+\|1)[0-9]{3,18}$\|^[*#](\+\|1)[0-9*#:]{3,18}$\|^[0-9*#]{3,18}$`) | `tests/components/task/OutdialCall/out-dial-call.tsx` "outdial DN regex — explicit prefix intent (WF-07)" | None | PRESENT | |
There was a problem hiding this comment.
Shouldn't R-016 come at last after R-015 entry
| @@ -27,8 +28,14 @@ export const useDigitalChannelsInit = (props: DigitalChannelsInitHookProps) => { | |||
| } | |||
|
|
|||
| const initialize = async () => { | |||
There was a problem hiding this comment.
It would be worth checking if there is really a possibility of double initialization here and if due to some race condition that might happen, verify this fix if this is the best way to solve it
There was a problem hiding this comment.
Yes — concurrent effect runs can call initializeApp twice before initialized flips. Ref guard prevents that. Also fixed: reset ref on failure (retry) and when store clears session on logout (isDigitalChannelsInitialized → false). Added unit tests in ae4cff6.
| // Branch 2: *# prefix with +/1 following; Branch 3: digits/special only (no prefix required) | ||
| const regExForDnSpecialChars = useMemo( | ||
| () => new RegExp('^[+1][0-9]{3,18}$|^[*#][+1][0-9*#:]{3,18}$|^[0-9*#]{3,18}$'), | ||
| () => new RegExp('^(\\+|1)[0-9]{3,18}$|^[*#](\\+|1)[0-9*#:]{3,18}$|^[0-9*#]{3,18}$'), |
There was a problem hiding this comment.
Why we adding \ here ? It would be good to check which numbers we wanna match.
If we only want to match numbers starrting with +1 then just replaicng [+1] with +1 should be enough. If we want to allow all E.164 numbers in order to enable international dialing then it should +?.
Please check and update the regex accordingly
There was a problem hiding this comment.
Kept ^(\+1|1|[2-9]\d{2,})$ — behavior unchanged from pre-PR (US + local). (\|1) is explicit alternation vs [+1] (character class). Not expanding to full E.164 unless product confirms international outdial scope.
There was a problem hiding this comment.
Comment is about why is there a double , single \ is enough to match numbers starting with +1
Add semver validation to update-dependencies workflow, reset digital channels init guard on failure/logout with tests, and align spec docs and App.tsx comments with review. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
removed
the
validated
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae4cff6f7d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useEffect(() => { | ||
| if (!isDigitalChannelsInitialized) { | ||
| initInFlightRef.current = false; | ||
| setInitialized(false); |
There was a problem hiding this comment.
When this hook mounts under React 18 <StrictMode> while initializeApp remains pending, Strict Mode replays the effects: the first initialization sets the ref to true, but the replay of this reset effect sees the still-false store flag and clears it before the initialization effect runs again. This permits two concurrent initializeApp calls—the exact case the guard is intended to prevent—while the added test only exercises a dependency rerender rather than Strict Mode effect replay. Reset the ref only for an actual initialized-to-uninitialized session transition.
Useful? React with 👍 / 👎.
Addressed review feedback in ae4cff6: CG-01 workflow validation, digital channels init guard (failure + logout) with tests, R-016 ordering, block comments. Replies posted on open threads for window.store scope, integrationEnv, and outdial regex. |
Kesari3008
left a comment
Kesari3008
left a comment
There was a problem hiding this comment.
We need discussion for few changes
github-actions
Bot
removed
the
validated
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81628c7f93
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| describe('outdial DN regex — explicit prefix intent (WF-07)', () => { | ||
| // AC-4: regex branch-1 must use (\+|1) to make the prefix intent explicit | ||
| // +12345678901 must remain valid; the character class [+1] was ambiguous (security finding) | ||
| const validNumbers = ['+12345678901', '+1234', '1234', '+1234567890123456789'.slice(0, 19), '*#+123', '*#123']; |
There was a problem hiding this comment.
The parameterized “valid” suite always fails for *#+123: branch 2 permits exactly one leading * or #, immediately followed by + or 1, so this value matches none of the three branches and renders help-text="Incorrect format.". Either change the fixture to an actually supported value such as *+123 or deliberately extend the production regex; otherwise the @webex/cc-components unit suite cannot pass.
AGENTS.md reference: AGENTS.md:L91-L93
Useful? React with 👍 / 👎.
| if (initInFlightRef.current) { | ||
| return; |
There was a problem hiding this comment.
When DigitalChannels remains mounted and logout occurs while initializeApp is still pending, the store flag is already false, so cleanUpStore() setting it to false does not trigger the reset effect. A task from the next login then reaches this guard and skips initialization, while the old promise can later resolve and mark the new session initialized using the previous session's token/data center. Key the in-flight operation to session inputs or invalidate stale completions when the task/session is cleared.
Useful? React with 👍 / 👎.
| /* | ||
| * Validate CSRF state token before accepting the OAuth redirect. | ||
| * The SDK encodes state as base64(JSON({app_state, csrf_token})) in the hash. |
There was a problem hiding this comment.
This adds a new identity/security flow—generating, persisting, decoding, validating, and consuming OAuth state—but the change updates only the component and digital-channel module specs; neither ai-docs/SECURITY.md nor another current standing doc records this control and its storage/failure behavior. Add the OAuth state contract to the applicable standing documentation in this same change so the documented security model remains current.
AGENTS.md reference: AGENTS.md:L68-L68
Useful? React with 👍 / 👎.
3 participants
COMPLETES #https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-833337
This pull request addresses
SPARK-833337 reports six medium security findings in Contact Center widgets. This PR addresses five findings: WF-05, WF-06, WF-07, WF-08, and WF-10.
CG-01 / AC-1 is intentionally deferred because it requires changing .github/workflows/update-dependencies.yml, and the Jira-to-PR automation token does not have the workflow permission required to push GitHub Actions workflow changes. The deferment is also recorded in the PR conversation.
by making the following changes
Change Type
The following scenarios were tested
Automated verification performed:
The GAI Coding Policy And Copyright Annotation Best Practices
Checklist before merging
Make sure to have followed the contributing guidelines before submitting.