Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 52 additions & 25 deletions .github/workflows/pull-request.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,31 +12,6 @@ concurrency:

env:
rid: ${{ github.run_id }}-${{ github.run_number }}
PW_SANDBOX_PASSWORD: ${{ secrets.PW_SANDBOX_PASSWORD }}
PW_SENDER_EMAIL_PASSWORD: ${{ secrets.PW_SENDER_EMAIL_PASSWORD }}
PW_SANDBOX: ${{ secrets.PW_SANDBOX }}
PW_ENTRY_POINT1: ${{ secrets.PW_ENTRY_POINT1 }}
PW_ENTRY_POINT2: ${{ secrets.PW_ENTRY_POINT2 }}
PW_ENTRY_POINT3: ${{ secrets.PW_ENTRY_POINT3 }}
PW_ENTRY_POINT4: ${{ secrets.PW_ENTRY_POINT4 }}
PW_ENTRY_POINT5: ${{ secrets.PW_ENTRY_POINT5 }}
PW_ENTRY_POINT6: ${{ secrets.PW_ENTRY_POINT6 }}
PW_ENTRY_POINT7: ${{ secrets.PW_ENTRY_POINT7 }}
PW_ENTRY_POINT8: ${{ secrets.PW_ENTRY_POINT8 }}
PW_ENTRY_POINT9: ${{ secrets.PW_ENTRY_POINT9 }}
PW_ENTRY_POINT10: ${{ secrets.PW_ENTRY_POINT10 }}
PW_ENTRY_POINT11: ${{ secrets.PW_ENTRY_POINT11 }}
PW_ENTRY_POINT12: ${{ secrets.PW_ENTRY_POINT12 }}
PW_SENDER_EMAIL: ${{ secrets.PW_SENDER_EMAIL }}
PW_CHAT_URL: ${{ secrets.PW_CHAT_URL }}
PW_DIAL_NUMBER_LOGIN_USERNAME: ${{ secrets.PW_DIAL_NUMBER_LOGIN_USERNAME }}
PW_DIAL_NUMBER_LOGIN_PASSWORD: ${{ secrets.PW_DIAL_NUMBER_LOGIN_PASSWORD }}
PW_DIAL_NUMBER: ${{ secrets.PW_DIAL_NUMBER }}
PW_DIAL_NUMBER_NAME: ${{ secrets.PW_DIAL_NUMBER_NAME }}
PW_ENTRYPOINT_NAME: ${{ secrets.PW_ENTRYPOINT_NAME }}
PW_MEETING_USERID: ${{ secrets.PW_MEETING_USERID }}
PW_MEETING_PASSWORD: ${{ secrets.PW_MEETING_PASSWORD }}
PW_MEETING_DESTINATION: ${{ secrets.PW_MEETING_DESTINATION}}

jobs:
validate:
Expand Down Expand Up @@ -140,6 +115,32 @@ jobs:
concurrency:
group: e2e-meetings
cancel-in-progress: false
env:
PW_SANDBOX_PASSWORD: ${{ secrets.PW_SANDBOX_PASSWORD }}
Comment on lines +118 to +119

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Update the standing docs with the security behavior

This commit changes credential persistence, CI secret scoping, and the publish command's trust boundary without updating any module spec or standing document. That leaves the repository's security design and verification guidance out of sync with the implementation despite the mandatory same-change spec-currency rule; update the relevant standing security documentation alongside these controls.

AGENTS.md reference: AGENTS.md:L68-L68

Useful? React with 👍 / 👎.

Comment on lines +118 to +119

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Isolate secrets from checked-out PR code

When a fork PR receives both the validated and run_e2e labels, this pull_request_target job checks out the PR head while these job-level secrets are available to every subsequent step. PR-controlled install scripts, Webpack configuration, build scripts, and Playwright code can therefore read and exfiltrate all PW_* credentials; moving them from workflow scope to job scope narrows exposure but does not satisfy AC-1's requirement to keep repository secrets out of PR-head execution. Run secret-bearing E2E work from trusted base code/artifacts rather than executing the checked-out head with repository secrets.

Useful? React with 👍 / 👎.

PW_SENDER_EMAIL_PASSWORD: ${{ secrets.PW_SENDER_EMAIL_PASSWORD }}
PW_SANDBOX: ${{ secrets.PW_SANDBOX }}
PW_ENTRY_POINT1: ${{ secrets.PW_ENTRY_POINT1 }}
PW_ENTRY_POINT2: ${{ secrets.PW_ENTRY_POINT2 }}
PW_ENTRY_POINT3: ${{ secrets.PW_ENTRY_POINT3 }}
PW_ENTRY_POINT4: ${{ secrets.PW_ENTRY_POINT4 }}
PW_ENTRY_POINT5: ${{ secrets.PW_ENTRY_POINT5 }}
PW_ENTRY_POINT6: ${{ secrets.PW_ENTRY_POINT6 }}
PW_ENTRY_POINT7: ${{ secrets.PW_ENTRY_POINT7 }}
PW_ENTRY_POINT8: ${{ secrets.PW_ENTRY_POINT8 }}
PW_ENTRY_POINT9: ${{ secrets.PW_ENTRY_POINT9 }}
PW_ENTRY_POINT10: ${{ secrets.PW_ENTRY_POINT10 }}
PW_ENTRY_POINT11: ${{ secrets.PW_ENTRY_POINT11 }}
PW_ENTRY_POINT12: ${{ secrets.PW_ENTRY_POINT12 }}
PW_SENDER_EMAIL: ${{ secrets.PW_SENDER_EMAIL }}
PW_CHAT_URL: ${{ secrets.PW_CHAT_URL }}
PW_DIAL_NUMBER_LOGIN_USERNAME: ${{ secrets.PW_DIAL_NUMBER_LOGIN_USERNAME }}
PW_DIAL_NUMBER_LOGIN_PASSWORD: ${{ secrets.PW_DIAL_NUMBER_LOGIN_PASSWORD }}
PW_DIAL_NUMBER: ${{ secrets.PW_DIAL_NUMBER }}
PW_DIAL_NUMBER_NAME: ${{ secrets.PW_DIAL_NUMBER_NAME }}
PW_ENTRYPOINT_NAME: ${{ secrets.PW_ENTRYPOINT_NAME }}
PW_MEETING_USERID: ${{ secrets.PW_MEETING_USERID }}
PW_MEETING_PASSWORD: ${{ secrets.PW_MEETING_PASSWORD }}
PW_MEETING_DESTINATION: ${{ secrets.PW_MEETING_DESTINATION }}

steps:
- name: Checkout Project
Expand Down Expand Up @@ -194,6 +195,32 @@ jobs:
concurrency:
group: e2e-cc-widgets
cancel-in-progress: false
env:
PW_SANDBOX_PASSWORD: ${{ secrets.PW_SANDBOX_PASSWORD }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can have a common file from where these are exported and that can be used here in PR pipeline

PW_SENDER_EMAIL_PASSWORD: ${{ secrets.PW_SENDER_EMAIL_PASSWORD }}
PW_SANDBOX: ${{ secrets.PW_SANDBOX }}
PW_ENTRY_POINT1: ${{ secrets.PW_ENTRY_POINT1 }}
PW_ENTRY_POINT2: ${{ secrets.PW_ENTRY_POINT2 }}
PW_ENTRY_POINT3: ${{ secrets.PW_ENTRY_POINT3 }}
PW_ENTRY_POINT4: ${{ secrets.PW_ENTRY_POINT4 }}
PW_ENTRY_POINT5: ${{ secrets.PW_ENTRY_POINT5 }}
PW_ENTRY_POINT6: ${{ secrets.PW_ENTRY_POINT6 }}
PW_ENTRY_POINT7: ${{ secrets.PW_ENTRY_POINT7 }}
PW_ENTRY_POINT8: ${{ secrets.PW_ENTRY_POINT8 }}
PW_ENTRY_POINT9: ${{ secrets.PW_ENTRY_POINT9 }}
PW_ENTRY_POINT10: ${{ secrets.PW_ENTRY_POINT10 }}
PW_ENTRY_POINT11: ${{ secrets.PW_ENTRY_POINT11 }}
PW_ENTRY_POINT12: ${{ secrets.PW_ENTRY_POINT12 }}
PW_SENDER_EMAIL: ${{ secrets.PW_SENDER_EMAIL }}
PW_CHAT_URL: ${{ secrets.PW_CHAT_URL }}
PW_DIAL_NUMBER_LOGIN_USERNAME: ${{ secrets.PW_DIAL_NUMBER_LOGIN_USERNAME }}
PW_DIAL_NUMBER_LOGIN_PASSWORD: ${{ secrets.PW_DIAL_NUMBER_LOGIN_PASSWORD }}
PW_DIAL_NUMBER: ${{ secrets.PW_DIAL_NUMBER }}
PW_DIAL_NUMBER_NAME: ${{ secrets.PW_DIAL_NUMBER_NAME }}
PW_ENTRYPOINT_NAME: ${{ secrets.PW_ENTRYPOINT_NAME }}
PW_MEETING_USERID: ${{ secrets.PW_MEETING_USERID }}
PW_MEETING_PASSWORD: ${{ secrets.PW_MEETING_PASSWORD }}
PW_MEETING_DESTINATION: ${{ secrets.PW_MEETING_DESTINATION }}

steps:
- name: Checkout Project
Expand Down
23 changes: 20 additions & 3 deletions tooling/src/publish.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const {execSync} = require('child_process');
const {execFileSync} = require('child_process');
const fs = require('fs');
const path = require('path');

Expand Down Expand Up @@ -42,6 +42,17 @@ function versionAndPublish() {
);
process.exit(1);
}

// Validate branchName (used as npm dist-tag): must match npm tag naming rules.
// Reject any value containing shell metacharacters or characters outside the allowed set.
const validTagPattern = /^[A-Za-z0-9][A-Za-z0-9._-]*$/;
if (!validTagPattern.test(branchName)) {
console.error(
`Error: Invalid branchName/tag value "${branchName}". Must match /^[A-Za-z0-9][A-Za-z0-9._-]*$/`
);
process.exit(1);
return;
}
const contactCenterPath = './packages/contact-center';

try {
Expand All @@ -61,10 +72,16 @@ function versionAndPublish() {
return packageData.name;
});

// Publish the package
// Validate workspace names (npm package name pattern) and publish via no-shell execFileSync.
// Using execFileSync instead of execSync prevents shell interpretation of workspace or tag values.
const validPackageNamePattern = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/;

const publishWorkspace = (workspace) => {
if (!validPackageNamePattern.test(workspace)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we validating this in 2 places ? Validating here should take care of it right

throw new Error(`Invalid package name "${workspace}": does not match npm package name pattern`);
}
console.log(`Publishing new version for ${workspace}: ${newVersion}`);
execSync(`yarn workspace ${workspace} npm publish --tag ${branchName}`, {stdio: 'inherit'});
execFileSync('yarn', ['workspace', workspace, 'npm', 'publish', '--tag', branchName], {stdio: 'inherit'});
};

const denyList = ['@webex/test-fixtures']; // Add workspace names to exclude from publishing
Expand Down
102 changes: 82 additions & 20 deletions tooling/tests/publish.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,8 +173,8 @@ describe('versionAndPublish', () => {
mockFs.readFileSync.mockReturnValueOnce(packageJsonContent).mockReturnValueOnce(packageJsonContent2);
mockFs.writeFileSync.mockImplementation(() => {});

const mockExecSync = require('child_process').execSync;
mockExecSync.mockImplementation(() => {});
const mockExecFileSync = require('child_process').execFileSync;
mockExecFileSync.mockImplementation(() => {});

const processArgvMock = ['node', 'script.js', 'main', '1.0.1'];
process.argv = processArgvMock;
Expand All @@ -195,12 +195,18 @@ describe('versionAndPublish', () => {
'utf-8'
);

expect(mockExecSync).toHaveBeenNthCalledWith(1, 'yarn workspace @webex/cc-store npm publish --tag main', {
stdio: 'inherit',
});
expect(mockExecSync).toHaveBeenNthCalledWith(2, 'yarn workspace @webex/cc-station-login npm publish --tag main', {
stdio: 'inherit',
});
expect(mockExecFileSync).toHaveBeenNthCalledWith(
1,
'yarn',
['workspace', '@webex/cc-store', 'npm', 'publish', '--tag', 'main'],
{stdio: 'inherit'}
);
expect(mockExecFileSync).toHaveBeenNthCalledWith(
2,
'yarn',
['workspace', '@webex/cc-station-login', 'npm', 'publish', '--tag', 'main'],
{stdio: 'inherit'}
);
});

it('should not publish packages in deny list P.S we only have test-fixtures in deny list right now', () => {
Expand All @@ -221,8 +227,8 @@ describe('versionAndPublish', () => {
mockFs.readFileSync.mockReturnValueOnce(packageJsonContent).mockReturnValueOnce(packageJsonContent2);
mockFs.writeFileSync.mockImplementation(() => {});

const mockExecSync = require('child_process').execSync;
mockExecSync.mockImplementation(() => {});
const mockExecFileSync = require('child_process').execFileSync;
mockExecFileSync.mockImplementation(() => {});

const processArgvMock = ['node', 'script.js', 'main', '1.0.1'];
process.argv = processArgvMock;
Expand All @@ -243,12 +249,17 @@ describe('versionAndPublish', () => {
'utf-8'
);

expect(mockExecSync).not.toHaveBeenCalledWith('yarn workspace @webex/test-fixtures npm publish --tag main', {
stdio: 'inherit',
});
expect(mockExecSync).toHaveBeenNthCalledWith(1, 'yarn workspace @webex/cc-station-login npm publish --tag main', {
stdio: 'inherit',
});
expect(mockExecFileSync).not.toHaveBeenCalledWith(
'yarn',
['workspace', '@webex/test-fixtures', 'npm', 'publish', '--tag', 'main'],
{stdio: 'inherit'}
);
expect(mockExecFileSync).toHaveBeenNthCalledWith(
1,
'yarn',
['workspace', '@webex/cc-station-login', 'npm', 'publish', '--tag', 'main'],
{stdio: 'inherit'}
);
});

it('error occurred while reading package.json data', () => {
Expand All @@ -261,14 +272,65 @@ describe('versionAndPublish', () => {
throw new Error('Error while reading from file');
});

const mockExecSync = require('child_process').execSync;
const mockExecFileSync = require('child_process').execFileSync;

const processArgvMock = ['node', 'script.js', 'main', '1.0.1'];
process.argv = processArgvMock;

versionAndPublish();
expect(console.error).toHaveBeenCalledWith('Failed to process workspaces:', 'Error while reading from file');
expect(mockExecSync).not.toHaveBeenCalled();
expect(mockExecFileSync).not.toHaveBeenCalled();
});

it('WF-02: rejects a malicious branchName containing shell metacharacters', () => {
const packageJsonContent = JSON.stringify({
name: '@webex/cc-store',
version: '1.0.0',
});

mockFs.readdirSync.mockReturnValue([{name: 'store', isDirectory: () => true}]);
mockFs.readFileSync.mockReturnValue(packageJsonContent);
mockFs.writeFileSync.mockImplementation(() => {});

const mockExecFileSync = require('child_process').execFileSync;
mockExecFileSync.mockImplementation(() => {});

// A tag with shell metacharacters must be rejected before any publish call
process.argv = ['node', 'script.js', 'main; rm -rf /', '1.0.1'];

versionAndPublish();

expect(console.error).toHaveBeenCalledWith(
expect.stringContaining('Error')
);
expect(process.exit).toHaveBeenCalledWith(1);
expect(mockExecFileSync).not.toHaveBeenCalled();
});

it('WF-01: rejects a workspace name containing shell metacharacters', () => {
const packageJsonContent = JSON.stringify({
name: '@webex/cc-store$(malicious)',
version: '1.0.0',
});

mockFs.readdirSync.mockReturnValue([{name: 'store', isDirectory: () => true}]);
mockFs.readFileSync.mockReturnValue(packageJsonContent);
mockFs.writeFileSync.mockImplementation(() => {});

const mockExecFileSync = require('child_process').execFileSync;
mockExecFileSync.mockImplementation(() => {});

process.argv = ['node', 'script.js', 'main', '1.0.1'];

versionAndPublish();

// The malicious workspace should either be rejected (process.exit) or
// skipped without ever being passed to execFileSync
expect(mockExecFileSync).not.toHaveBeenCalledWith(
'yarn',
expect.arrayContaining(['@webex/cc-store$(malicious)']),
expect.anything()
);
});

it('should export versionAndPublish when required as a module', () => {
Expand All @@ -289,7 +351,7 @@ describe('versionAndPublish', () => {
throw new Error('Error while reading from file');
});

const mockExecSync = require('child_process').execSync;
const mockExecFileSync = require('child_process').execFileSync;

const processArgvMock = ['node', 'script.js', 'main', '1.0.1'];
process.argv = processArgvMock;
Expand All @@ -299,6 +361,6 @@ describe('versionAndPublish', () => {
versionAndPublish();

expect(console.error).toHaveBeenCalledWith('Failed to process workspaces:', 'package.json not found in store');
expect(mockExecSync).not.toHaveBeenCalled();
expect(mockExecFileSync).not.toHaveBeenCalled();
});
});
20 changes: 3 additions & 17 deletions widgets-samples/cc/samples-cc-react-app/src/App.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,6 @@ import './App.scss';
import {observer} from 'mobx-react-lite';
import EngageWidget from './EngageWidget';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Preserve the campaign-preview test control path

When playwright/suites/campaign-preview-tests.spec.ts is run against this React sample, removing the global store makes every helper in playwright/Utils/campaignPreviewUtils.ts fail at its explicit Store not found on window object guard. That suite injects campaign tasks and stubs SDK actions through window['store'], so the security change needs a replacement test-only control path or corresponding Playwright rewrite rather than only deleting the assignment.

Useful? React with 👍 / 👎.

// This is not to be included to a production app.
// Have added here for debugging purposes
window['store'] = store;

const defaultWidgets = {
stationLogin: true,
stationLoginProfile: false,
Expand Down Expand Up @@ -131,22 +127,19 @@ function App() {
};

useEffect(() => {
// Remove token persisted by older sample builds — no longer read or written (SPARK-833336)
window.localStorage.removeItem('accessToken');

if (window.location.hash) {
const urlParams = new URLSearchParams(window.location.hash.replace('#', '?'));

const accessToken = urlParams.get('access_token');

if (accessToken) {
window.localStorage.setItem('accessToken', accessToken);
setAccessToken(accessToken);
// Clear the hash from the URL to remove the token from browser history
window.history.replaceState({}, document.title, window.location.pathname + window.location.search);
}
} else {
const storedAccessToken = window.localStorage.getItem('accessToken');
if (storedAccessToken) {
setAccessToken(storedAccessToken);
}
}
}, []);

Expand Down Expand Up @@ -379,13 +372,6 @@ function App() {
});
};

// Store accessToken changes in local storage
useEffect(() => {
if (accessToken.trim() !== '') {
window.localStorage.setItem('accessToken', accessToken);
}
}, [accessToken]);

useEffect(() => {
window.localStorage.setItem('selectedWidgets', JSON.stringify(selectedWidgets));
}, [selectedWidgets]);
Expand Down
10 changes: 3 additions & 7 deletions widgets-samples/cc/samples-cc-wc-app/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
}
}

function toggleintegrationEnv() {

Check warning on line 42 in widgets-samples/cc/samples-cc-wc-app/app.js

View workflow job for this annotation

GitHub Actions / linter

'toggleintegrationEnv' is defined but never used
integrationEnv = integrationEnvElem.checked;
// Store in localStorage for persistence
localStorage.setItem('integrationEnv', JSON.stringify(integrationEnv));
Expand Down Expand Up @@ -82,12 +82,12 @@
}
}

function enableMultiLogin() {

Check warning on line 85 in widgets-samples/cc/samples-cc-wc-app/app.js

View workflow job for this annotation

GitHub Actions / linter

'enableMultiLogin' is defined but never used
if (isMultiLoginEnabled) isMultiLoginEnabled = false;
else isMultiLoginEnabled = true;
}

function toggleCampaignPreview(event) {

Check warning on line 90 in widgets-samples/cc/samples-cc-wc-app/app.js

View workflow job for this annotation

GitHub Actions / linter

'toggleCampaignPreview' is defined but never used
hasCampaignPreviewEnabled = event.target.checked;
// Sync both checkboxes
document.querySelectorAll('.campaign-preview-checkbox').forEach(function (cb) {
Expand All @@ -96,7 +96,7 @@
ccTaskList.hasCampaignPreviewEnabled = hasCampaignPreviewEnabled;
}

function doOAuthLogin() {

Check warning on line 99 in widgets-samples/cc/samples-cc-wc-app/app.js

View workflow job for this annotation

GitHub Actions / linter

'doOAuthLogin' is defined but never used
let redirectUri = `${window.location.protocol}//${window.location.host}`;

if (window.location.pathname) {
Expand Down Expand Up @@ -135,7 +135,7 @@
},
};

const webex = Webex.init(webexConfig);

Check warning on line 138 in widgets-samples/cc/samples-cc-wc-app/app.js

View workflow job for this annotation

GitHub Actions / linter

'Webex' is not defined

webex.once('ready', () => {
webex.authorization.initiateLogin();
Expand All @@ -150,6 +150,9 @@
accessTokenElem.addEventListener('keyup', updateButtonState);

window.addEventListener('load', () => {
// Remove token persisted by older sample builds — no longer read or written (SPARK-833336)
localStorage.removeItem('accessToken');

changeLoginType();
loadintegrationEnvSetting(); // Load the setting on page load
if (window.location.hash) {
Expand All @@ -158,22 +161,15 @@
const accessToken = urlParams.get('access_token');

if (accessToken) {
localStorage.setItem('accessToken', accessToken);
accessTokenElem.value = accessToken;
updateButtonState();
// Clear the hash from the URL to remove the token from browser history
window.history.replaceState({}, document.title, window.location.pathname + window.location.search);
}
} else {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we verifying if this change doesn't break anything ?

const storedAccessToken = window.localStorage.getItem('accessToken');
if (storedAccessToken) {
accessTokenElem.value = storedAccessToken;
updateButtonState();
}
}
});

function initWidgets() {

Check warning on line 172 in widgets-samples/cc/samples-cc-wc-app/app.js

View workflow job for this annotation

GitHub Actions / linter

'initWidgets' is defined but never used
const webexConfig = {
fedramp: false,
logger: {
Expand Down