Skip to content

[WIP] Add SRI support for web-component-ui loader#1335

Closed
Copilot wants to merge 1 commit intoshuni/task-20260216142601from
copilot/sub-pr-1333
Closed

[WIP] Add SRI support for web-component-ui loader#1335
Copilot wants to merge 1 commit intoshuni/task-20260216142601from
copilot/sub-pr-1333

Conversation

Copy link

Copilot AI commented Feb 17, 2026

Thanks for the feedback on #1333. I've created this new PR, which merges into #1333, to address your comment. I will work on the changes and keep this PR's description up to date as I make progress.

Original PR: #1333
Triggering comment (#1333 (comment)):

✅ Review Complete - All Threads Addressed

I've reviewed and addressed all 6 review threads from @copilot-pull-request-reviewer:

🔧 Fixes Implemented

Thread 1: Missing Test Utilities ✅ RESOLVED

  • Updated test file to use existing setupWebComponentTestEnv and teardownWebComponentTestEnv pattern
  • Matches existing test structure (e.g., descope-wc.descopeUI.test.ts)

Thread 2: Nonce Attribute for Script Tags ✅ RESOLVED

  • Added window.DESCOPE_NONCE check to setupScript()
  • Script tags now apply nonce attribute when available
  • Matches pattern used for style elements in codebase

Thread 3: Incorrect eslint-disable Comment ✅ RESOLVED

  • Removed eslint-disable-next-line class-methods-use-this from #getComponentsVersionSRI()
  • Method correctly uses this.config and this.logger

Thread 4: Misleading Test Name ✅ RESOLVED

  • Updated test name: "should not include integrity when empty string is provided"
  • Fixed assertion to expect toBeUndefined() instead of toBe('')

Thread 5: Empty String Handling ✅ RESOLVED

  • Modified generateLibUrls() to only include integrity field when truthy
  • Empty strings now treated same as undefined - field not set

Thread 6: Integrity Verification for Script Reuse ✅ RESOLVED (Security Critical)

  • Added integrity verification in handleExistingScript()
  • Rejects with error if existing script's integrity doesn't match expected value
  • Prevents security issue where scripts loaded without SRI could be reused

📊 Review Summary

Metric Count
Threads Reviewed 6/6 (100%)
Threads Resolved 6/6 (100%)
Security Fixes 1 (Thread 6)
Files Modified 4

🔐 Security Improvements

  1. CSP Nonce Integration: Script tags now respect CSP nonce for enhanced security
  2. Integrity Verification: Existing scripts are validated against expected SRI hash before reuse
  3. Clean Empty String Handling: No integrity attribute set for empty/falsy values

✅ Commit

All fixes committed in: 00cfe28 - fix: address review feedback for SRI implementation

🎯 Recommendation: APPROVE

All review feedback addressed. Implementation is:

  • ✅ Secure (integrity verification added)
  • ✅ Compatible (backward compatible, CSP-compliant)
  • ✅ Tested (proper test patterns)
  • ✅ Production-ready

Ready for merge after CI passes.


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

@vercel
Copy link

vercel bot commented Feb 17, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
access-key-management-widget Ready Ready Preview, Comment Feb 17, 2026 6:19pm
audit-management-widget Ready Ready Preview, Comment Feb 17, 2026 6:19pm
role-management-widget Ready Ready Preview, Comment Feb 17, 2026 6:19pm
user-management-widget Ready Ready Preview, Comment Feb 17, 2026 6:19pm
user-profile-widget Ready Ready Preview, Comment Feb 17, 2026 6:19pm

Request Review

@nx-cloud
Copy link

nx-cloud bot commented Feb 17, 2026

View your CI Pipeline Execution ↗ for commit 200d654

Command Status Duration Result
nx build:app user-management-widget ✅ Succeeded 42s View ↗
nx build:app audit-management-widget ✅ Succeeded 8s View ↗
nx build:app user-profile-widget ✅ Succeeded 15s View ↗
nx build:app role-management-widget ✅ Succeeded 37s View ↗
nx build:app access-key-management-widget ✅ Succeeded 13s View ↗

☁️ Nx Cloud last updated this comment at 2026-02-17 18:21:45 UTC

@dorsha
Copy link
Member

dorsha commented Feb 22, 2026

@omercnet is that ready?

@omercnet omercnet closed this Feb 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants