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
| 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
| 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
| | `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
| // 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
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.