SPARK-843491: Remediate 8 High security findings in components - #862
SPARK-843491: Remediate 8 High security findings in components#862mkesavan13 wants to merge 1 commit into
Conversation
Jira: https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-843491 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mkesavan13
left a comment
mkesavan13
left a comment
There was a problem hiding this comment.
QA Harness — Automated Code Review
Verdict: Request changes
Findings: 9 — 0 blocking · 2 major · 6 minor · 1 nit
Pull request overview
This PR focuses on SPARK-843491: Remediate 8 High security findings in components.
Changes
- Hardens CI by switching the install step to
npm ciand adding annpm audit --audit-level=highgate, and adds unit tests for theuseActionOpenUrladaptive-card hook covering t - The tests correctly match the hook's behavior and the
isValidUrl/isBlockedHosthost-blocking logic - A minor test-robustness gap is noted inline
- Hardens dependency and URL handling as part of a security remediation: the storybook release workflow switches to
npm ciand adds annpm audit --audit-level=highgate, and a ne
Review outcome
At least one blocking or major issue was found; see the inline findings for the concrete fixes.
Reviewed changes
QA Harness reviewed 16 out of 16 changed files and generated 9 comments.
Test & CI gaps
- No test asserts the
onKeyDownhandler opens the URL on Enter/Space keypress, so the keyboard-activation branch in useActionOpenUrl.js (line 40) is uncovered. - No test asserts
onOpenUrlis called with the URL on a valid open, leaving the context callback path unverified. - src/util.test.js does not cover the
.localhostsuffix branch of the host blocklist (e.g.isValidUrl('http://api.localhost/x', ['http:'])should be false). - src/util.test.js does not cover the IPv6 link-local branch (e.g.
isValidUrl('http://[fe80::1]/x', ['http:'])should be false). - src/util.test.js does not cover the IPv6 unique-local branch (e.g.
isValidUrl('http://[fd00::1]/x', ['http:'])should be false). - No test asserts that an expired
{token, expiry}envelope is actually evicted or rejected on read — because the component has no read/eviction path, the addedexpiryfield is untested for its stated purpose. - No test covers
placeset (session/local) withttlundefined, whereDate.now() + ttl * 1000produces aNaNexpiry in the stored envelope. - No test asserts that DOMPurify actually alters output relative to markdown-it alone; add a case where sanitization is observable (dangerous attribute/tag that survives markdown-it's encoding is stripped) so removing
DOMPurify.sanitizewould fail the suite. - No test covers the
<p>-unwrap branch (lines 32-34) interacting with DOMPurify's serialized output — assert that a single-paragraph input still renders unwrapped after sanitization to guard against whitespace/normalization drift changing thehtml.length - 5check. - isBlockedHost gap cases in isValidUrl are untested: add assertions that
http://0.0.0.0/,http://[::]/,http://[::ffff:127.0.0.1]/, andhttp://localhost./are rejected (they currently pass validation), plus a positive boundary case thathttp://172.32.0.1/is accepted.
Generated by the quality-assurance harness. This review is advisory.
| div.props.onClick(); | ||
| }); | ||
|
|
||
| expect(openSpy).toHaveBeenCalledWith( |
There was a problem hiding this comment.
minor · tests
The happy-path test creates an onOpenUrl spy but never asserts it was invoked, so the hook's onOpenUrl(data.url) call on line 33 of useActionOpenUrl.js is untested — a regression that dropped that callback would still pass. Add expect(onOpenUrl).toHaveBeenCalledWith('https://example.com/page'); after the click. The added attributes onKeyDown (Enter/Space → open), role: 'link', and title are also uncovered; consider asserting at least that onKeyDown triggers window.open on Enter.
| import {deepMerge, isValidUrl} from './util'; | ||
|
|
||
| describe('isValidUrl', () => { | ||
| describe('AC-1: host allow-list hardening', () => { |
There was a problem hiding this comment.
minor · tests
The isBlockedHost logic backing isValidUrl has several SSRF-relevant branches this suite does not exercise: the .localhost suffix match (hostname.endsWith('.localhost')), the IPv6 link-local range (fe80:...), and the IPv6 unique-local range (fc00::/fd00::). Since these are the exact bypass vectors this remediation is meant to close, add cases like isValidUrl('http://foo.localhost/x', ['http:']), isValidUrl('http://[fe80::1]/x', ['http:']), and isValidUrl('http://[fd00::1]/x', ['http:']) all expecting false, so a future regression that drops one of those regexes is caught.
| node-version: 20.13.1 | ||
| - run: npm install | ||
| - run: npm ci | ||
| - run: npm audit --audit-level=high |
There was a problem hiding this comment.
minor · maintainability
npm audit --audit-level=high runs before npm run build/semantic-release, so any newly disclosed high/critical advisory anywhere in the dependency tree will fail this job and block all releases, even when unrelated to this package's code. That may be the intent as a hard gate, but consider whether release publishing should be coupled to the live advisory database (e.g. run audit in a separate non-release CI check, or scope it) so an upstream advisory doesn't unexpectedly halt shipping.
| const sessionExpiry = Date.now() + ttl * 1000; | ||
| const sessionData = JSON.stringify({token: accessToken, expiry: sessionExpiry}); | ||
|
|
||
| sessionStorage.setItem(name, sessionData); |
There was a problem hiding this comment.
major · compatibility
The session/local token is now written as JSON.stringify({token, expiry}) instead of the raw accessToken (same change at line 101 for localStorage). Any existing consumer that reads sessionStorage.getItem(name) / localStorage.getItem(name) and uses it directly as a bearer token will now receive a JSON string like {"token":"...","expiry":...} and break. This is a stored-format contract change; either keep writing the raw token, or confirm and update every reader to JSON.parse(...).token. Please document the new envelope shape for downstream callers.
| case 'session': | ||
| sessionStorage.setItem(name, accessToken); | ||
| case 'session': { | ||
| const sessionExpiry = Date.now() + ttl * 1000; |
There was a problem hiding this comment.
major · correctness
The expiry field is written but never enforced anywhere in the component (same for localExpiry at line 98). Nothing reads the stored value or evicts the token once expiry passes, so tokens still persist indefinitely — the TTL-eviction intent isn't actually realized by this write alone. Either add a read/eviction path that checks expiry and clears the item when stale, or clarify that eviction is host-owned. Also note ttl is optional (defaultProps leaves tokenStoragePolicy empty); if a place is set without ttl, Date.now() + ttl * 1000 yields NaN, producing an envelope that can never be considered expired.
| window.crypto.getRandomValues(arr); | ||
| newState = Array.from(arr).map((b) => b.toString(16).padStart(2, '0')).join(''); | ||
| } | ||
| csrfStateRef.current = newState; |
There was a problem hiding this comment.
minor · maintainability
csrfStateRef.current is assigned here but never read anywhere in the component, so the ref is effectively a write-only dead assignment. The CSRF state is sent to the auth server but the value returned on the callback is never compared against csrfStateRef.current, so this stored state provides no CSRF protection on its own. If returned-state validation is intentionally delegated to the caller, drop the unused ref; otherwise wire it into a validation check when the code/token is received.
| expect(stateParam.length).toBeGreaterThanOrEqual(32); | ||
| }); | ||
|
|
||
| test('state param is stored before auth window opens', () => { |
There was a problem hiding this comment.
nit · tests
The state param is stored before auth window opens test only re-reads the state query param from the window.open URL, which is the same thing the previous state has >=16 bytes serialized as hex test already asserts. It does not actually verify the ordering/storage claim in its name (that csrfStateRef.current is set before window.open is called), so it can never fail independently of the prior test. Either assert the ref/stored value directly, or drop this case to avoid a redundant test that reads as coverage it doesn't provide.
| const html = getHtml(renderer); | ||
|
|
||
| // No actual <img element with onerror attribute (unencoded) | ||
| expect(html).not.toMatch(/<img[^>]*onerror/i); |
There was a problem hiding this comment.
minor · tests
These two negative assertions (<img ... onerror> and <script>) pass vacuously and do not verify the DOMPurify layer they claim to test. The component constructs MarkdownIt('zero') and enables only emphasis, escape, link, list, newline, normalize, paragraph, strikethrough — the html rule is never enabled, so markdown-it already entity-encodes raw <img>/<script> into <img...> before DOMPurify.sanitize runs. As a result these assertions would pass identically against the pre-PR code with no DOMPurify at all, so they don't guard the sanitization boundary. Consider a test that fails if DOMPurify is removed — e.g. assert directly on DOMPurify.sanitize neutralizing a live HTML string, or feed input that markdown-it emits as actual markup and confirm DOMPurify strips the dangerous part.
| if (/^\[fe[89ab][0-9a-f]:[0-9a-f:]*\]$/i.test(hostname)) return true; | ||
| if (/^\[f[cd][0-9a-f]{2}:[0-9a-f:]*\]$/i.test(hostname)) return true; | ||
|
|
||
| return false; |
There was a problem hiding this comment.
minor · security
The host blocklist misses a few literal forms that resolve to local/internal targets, so isBlockedHost returns false for them:
0.0.0.0(and IPv6 unspecified[::]) —new URL('http://0.0.0.0/').hostnameis0.0.0.0, which routes to loopback on many systems but matches none of the regexes.- IPv4-mapped IPv6 —
http://[::ffff:127.0.0.1]/is normalized by the URL parser to[::ffff:7f00:1], which the[::1]/fe80/fc00patterns don't match, giving a path back to127.0.0.1. - Trailing-dot hostnames —
http://localhost./has hostnamelocalhost., which fails both=== 'localhost'andendsWith('.localhost').
Decimal/hex/octal IPv4 (e.g. http://2130706433/) are fine because the WHATWG URL parser canonicalizes them to dotted-decimal first. Impact here is limited since callers use this for browser window.open/image iconUrl rather than server-side fetches, but consider adding 0.0.0.0/[::], an IPv4-mapped-IPv6 check, and trailing-dot normalization to close the gaps.
1 participant
COMPLETES #SPARK-843491
This pull request addresses
Remediates 8 unique High-severity security findings (UF-001 through UF-008) identified in the
webex/componentsrepository via Codeguard and workflow security harness scans (scan date: 2026-07-06, commit: b4f087a). Findings span client-side trust boundaries: URL validation, OpenUrl sink, OAuth CSRF state + token storage, prototype pollution, markdown HTML sanitization, and CI supply chain.Root Cause (per finding):
isValidUrl(src/util.js) checked only the URL protocol; no host/loopback allow-list was enforced forhttp:/https:URLs.window.open(data.url, '_blank')inuseActionOpenUrl.jslacked thenoopener,noreferrerfeature string.Uint8Array(4)) and never stored before opening the auth window.secureflag;SameSite=Strictwas absent.sessionStorage/localStoragestored the raw access token; the destructuredttlvalue was never applied.deepMergeinsrc/util.jsiteratedObject.entries(src)with no guard against__proto__,constructor, orprototypekeys.markdownIt.render()output was injected viadangerouslySetInnerHTMLwith no HTML sanitizer.npm install(non-deterministic) without an audit gate before release/build steps.by making the following changes
src/util.js—isValidUrl: addsisBlockedHosthelper that blocks loopback (localhost,127.x,[::1]), link-local (169.254.x,fe80::), and private/unique-local ranges forhttp:/https:URLs;data:URIs are unaffected.deepMerge: addsFORBIDDEN_MERGE_KEYSset (__proto__,constructor,prototype) to skip those keys.src/components/adaptive-cards/hooks/useActionOpenUrl.js— Passes'noopener,noreferrer'towindow.open.src/components/SignIn/SignIn.jsx— CSRF state: 16-bytecrypto.getRandomValues→ hex string, stored incsrfStateRefbefore auth window opens. Cookie: addsSameSite=Strict. Session/local storage: stores{token, expiry}JSON envelope with TTL instead of raw token.src/components/adaptive-cards/Markdown/Markdown.jsx— WrapsmarkdownIt.render()output withDOMPurify.sanitizebeforedangerouslySetInnerHTML.package.json— Addsdompurify ^3.4.13as a runtime dependency..circleci/config.yml— Replacesnpm installwithnpm ci; addsnpm audit --audit-level=highstep gating all downstream jobs..github/workflows/npm-storybook-release.yml— Replacesnpm installwithnpm ci; addsnpm audit --audit-level=highstep gating thenpx semantic-releasestep that holdsNPM_TOKEN/GITHUB_TOKEN.ai-docs/SECURITY.md— Updates Input Validation posture to documentisValidUrlhost blocking and DOMPurify layer (spec-currency).src/components/ai-docs/components-spec.md— UpdatesSignInandMarkdownentries for CSRF state generation/storage and DOMPurify (spec-currency).New test files:
src/util.test.js,src/components/SignIn/SignIn.test.jsx,src/components/adaptive-cards/Markdown/Markdown.test.jsx,src/components/adaptive-cards/hooks/useActionOpenUrl.test.js.Change Type
The following scenarios were tested
The testing is done with the amplify link
Unit tests added covering all independently testable findings (AC-1 through AC-7, 32 targeted tests across 4 new test files).
Gate 1 (compile:
npm run build): PASSED — ESM and UMD bundles built successfully.Gate 2 (unit-test:
NODE_ENV=test npm run test): PASSED — 262 tests, 18 suites, 107 snapshots all passing.Gate 3: Not run.
Testing
npm run build) passed; Gate 2 (unit-testNODE_ENV=test npm run test) passed — 262 tests, 18 suites, 107 snapshots; Gate 3 not run.Acceptance Criteria
isValidUrlblocks loopback, link-local, and private-range hosts forhttp:/https:URLs.src/util.test.js— all passing in Gate 2Action.OpenUrlopens withnoopener,noreferrerand rejects disallowed hosts/schemes.useActionOpenUrl.test.js— all passing in Gate 2SignIn.test.jsx— all passing in Gate 2SameSite=Strictalongsidesecureflag.SignIn.test.jsx— passing in Gate 2{token, expiry}envelope usingttl; no raw unbounded token write.SignIn.test.jsx— passing in Gate 2deepMergeskips__proto__,constructor, andprototypekeys.src/util.test.js— all passing in Gate 2markdownIt.render()output is wrapped withDOMPurify.sanitizebeforedangerouslySetInnerHTML.Markdown.test.jsx— all passing in Gate 2npm ciwithnpm audit --audit-level=highas a required gate..circleci/config.ymland.github/workflows/npm-storybook-release.ymlinspected — release-blocking enforcement is CI-ownedExternal Validation Required
AC-3 — OAuth CSRF state is generated with >=16 bytes of entropy, serialized as hex/base64url, stored, and validated so mismatched or missing state on return is rejected.
external-dependencySignIn.jsx:53-112opens a cross-origin popup, pollsnewWindow.closed, and calls callergetAccessToken()with nopostMessage/location reader, so no in-repo test drives it. Validator: app team owningredirectUricompares returned state to the stored value and aborts on mismatch/absence. Uncertainty: JiraToPr cannot enforce the host-side check.statethat does not match the stored value (or carrying nostate) does not complete sign-in; a matchingstateproceeds normally.AC-8 — Both CI configurations use 'npm ci' for deterministic installs and enforce an 'npm audit --audit-level=high' gate that blocks the release on high-severity findings.
ci-pipelinenpm ci+npm audit --audit-level=highcontent is inspectable via UT-8 (.circleci/config.yml) and UT-9 (release workflow), but these are config-inspection targets, not jest unit tests, and the release-blocking guarantee only manifests when CI runs. Validator: release owners run both pipelines; a high-severity audit finding must fail before the token-holding release. Uncertainty: JiraToPr cannot run CI; validate against the existing lockfile first.Contract Discovery Warnings
AI Assistance
Checklist before merging
Jira: https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-843491