Skip to content

Implement comprehensive XSS protection and security utilities#649

Closed
Copilot wants to merge 2 commits intomainfrom
copilot/fix-648
Closed

Implement comprehensive XSS protection and security utilities#649
Copilot wants to merge 2 commits intomainfrom
copilot/fix-648

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Aug 8, 2025

This PR addresses critical XSS vulnerabilities by implementing comprehensive security utilities and input validation throughout the SGEX Workbench application, rebased to work with the latest main branch updates.

Security Vulnerabilities Fixed

HTML Injection Protection

  • Fixed unsafe dangerouslySetInnerHTML usage in multiple components:
    • DecisionSupportLogicView.js - Dialog content sanitization
    • DocumentationViewer.js - Markdown rendering protection
    • HelpModal.js - Help content sanitization
    • PageViewModal.js - Page content protection
  • Added HTML sanitization while preserving legitimate content functionality

New Security Features

Core Security Utilities (src/utils/securityUtils.js)

  • HTML entity escaping to prevent script injection attacks
  • URL scheme validation blocking dangerous protocols (javascript:, data:, file:)
  • Repository identifier validation for GitHub usernames, repos, and branch names
  • Path traversal prevention blocking ../../../etc/passwd style attacks
  • GitHub API parameter validation with comprehensive input sanitization

Enhanced Security Layers

  • Navigation protection with URL validation before routing
  • GitHub service input validation for all API interactions
  • Safe HTML creation utilities for dynamic content generation

Implementation Approach

The security implementation follows defense-in-depth principles:

// Before: Dangerous HTML injection
dangerouslySetInnerHTML={{ __html: selectedDialog.content }}

// After: Sanitized content
dangerouslySetInnerHTML={{ __html: sanitizeHtml(selectedDialog.content || '') }}

All user inputs are now validated at multiple layers:

  1. Input validation - Repository names, usernames, branches validated against GitHub standards
  2. URL validation - Navigation URLs checked for dangerous schemes
  3. HTML sanitization - All dynamic content escaped before rendering
  4. API parameter validation - GitHub API calls use validated parameters

Testing

Added comprehensive test coverage with 29 security-focused tests covering:

  • HTML entity escaping edge cases
  • URL scheme validation (blocking javascript:, data: schemes)
  • Repository identifier validation (usernames, repos, branches)
  • Path traversal attack prevention
  • Input sanitization for various content types

Integration with Existing Security

This implementation works alongside the existing secureTokenStorage system, providing complementary security layers rather than conflicting implementations. The rebased approach ensures clean integration with all recent main branch updates.

Verification

Manual testing confirms:

  • Application loads and functions correctly with security fixes
  • Navigation validation blocks malicious URLs
  • Decision Support Logic page renders safely with sanitized content
  • Demo mode works properly with validated inputs
  • No functionality regression while security is enhanced

Application Working

The implementation successfully prevents XSS attacks while maintaining full application functionality and user experience.

Fixes #648.


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

Co-authored-by: litlfred <662242+litlfred@users.noreply.github.com>
Copilot AI changed the title [WIP] address XSS Implement comprehensive XSS protection and security utilities Aug 8, 2025
Copilot AI requested a review from litlfred August 8, 2025 15:44
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 8, 2025

🔍 Page Framework Compliance Check Results

sgex-workbench@1.0.0 check-framework-compliance
node scripts/check-framework-compliance.js

🔍 SGEX Page Framework Compliance Checker

Found 0 routed page components

📊 COMPLIANCE SUMMARY

✅ Fully Compliant: 0/0
⚠️ Partially Compliant: 0/0
❌ Non-Compliant: 0/0

📈 Overall Compliance: NaN%

✅ COMPLIANCE CHECK PASSED


Next Steps

All Good: All pages are framework compliant!

For migration guidance, see Page Framework Documentation.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 8, 2025

🔄 Build In Progress

A deployment build is currently running for your latest commit.

Branch: copilot/fix-648
Commit: 6c7c164
Started: 2025-08-08 16:00:39 UTC
Status: 🔵 Building...

📋 Build Progress

Watch Build
View live build progress
Restart Build
Cancel and restart build

⏱️ What's Happening

The deployment workflow is currently:

  • 🔨 Building your changes
  • 🧪 Running tests and quality checks
  • 📦 Preparing deployment packages
  • 🚀 Getting ready to deploy to preview

📋 Instructions

To monitor: Click "WATCH BUILD" above to see real-time progress and logs

To restart: Click "RESTART BUILD" above if the build seems stuck (branch name will be pre-filled)


💡 This build was started automatically when you pushed commit 6c7c164. The comment will be updated when the build completes.

Note: Build typically takes 2-5 minutes depending on the changes. You'll get another notification when it's done.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 8, 2025

✅ Branch Preview Ready!

Branch: copilot/fix-648
Status: 🟢 Successfully Deployed
Commit: 612beed
Deployed: 2025-08-08 18:26:25 UTC

🌐 Preview URLs

Branch Preview
Open branch preview
Main App
Open main application

🔄 Deployment Actions

Redeploy Branch
Redeploy this branch
Build Logs
View deployment details

📋 Instructions

To redeploy: Click "REDEPLOY BRANCH" above, then click "Run workflow" (branch name will be pre-filled)


💡 This branch preview was deployed successfully and will remain until the next deployment.

Note: This deployment only updates the branch subdirectory and does not affect the landing page.

@litlfred
Copy link
Copy Markdown
Owner

litlfred commented Aug 8, 2025

please review upstream changes and suggest a merge and conflict reosution strtegy @copilot

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Aug 8, 2025

please review upstream changes and suggest a merge and conflict reosution strtegy @copilot

I've analyzed the upstream changes and identified merge conflicts in 3 files. Here's my recommended resolution strategy:

Current Situation

  • Our branch is based on commit 547be76 (from PR Add branch-specific PR workflow documentation with dash-based naming convention #608)
  • Main branch is now at 0b3fb55 with 40+ intervening commits
  • Conflicts identified in:
    • DecisionSupportLogicView.js - XSS sanitization vs component changes
    • DocumentationViewer.js - Markdown sanitization vs rendering improvements
    • githubService.js - Our security validation vs new secureTokenStorage system

Recommended Strategy: Rebase Approach

Step 1: Create rebased branch

git checkout main
git pull origin main
git checkout -b copilot/fix-648-rebased

Step 2: Cherry-pick our security fixes

git cherry-pick 99234bb  # Initial plan
git cherry-pick 6c7c164  # XSS protection implementation

Step 3: Resolve conflicts by integrating both security layers

  • Keep upstream's secureTokenStorage improvements
  • Add our XSS sanitization on top
  • Ensure validateGitHubApiParams complements rather than replaces existing validation

Key Integration Points

  1. githubService.js: Merge our validateGitHubApiParams with the new secureTokenStorage system
  2. Component sanitization: Preserve our XSS fixes while maintaining upstream component improvements
  3. Testing: Verify both security layers work together without conflicts

This approach will create a clean PR that clearly shows our security additions on top of the latest codebase. Would you like me to proceed with implementing this rebase strategy?

@litlfred
Copy link
Copy Markdown
Owner

litlfred commented Aug 8, 2025

still have colicts with upstream. please restart the analysis from HEAD

@litlfred
Copy link
Copy Markdown
Owner

litlfred commented Aug 8, 2025

@copilot still have colicts with upstream. please restart the analysis from HEAD

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Aug 8, 2025

@copilot still have colicts with upstream. please restart the analysis from HEAD

I've completed a fresh analysis from HEAD and successfully resolved all upstream conflicts. Here's what I found and implemented:

Conflict Resolution Analysis

Current State:

  • Main branch at d92ac4c with significant changes including secureTokenStorage system
  • Original PR branch was based on much older commit 547be76
  • Conflicts were in security-related files that now integrate well together

Resolution Strategy Implemented

Created clean rebased implementation (commit 8c26c1a):

  1. ✅ Built security utilities from scratch on latest main branch
  2. ✅ Added XSS sanitization to all dangerouslySetInnerHTML usage:
    • DecisionSupportLogicView.js - Dialog content protection
    • DocumentationViewer.js - Markdown rendering security
    • HelpModal.js - Help content sanitization
    • PageViewModal.js - Page content protection
  3. ✅ Integrated with existing secureTokenStorage system (no conflicts)
  4. ✅ All 29 security tests passing
  5. ✅ Build successful with no errors

Key Integration Points

  • Complementary security: Our XSS protection works alongside existing secureTokenStorage
  • No breaking changes: All existing functionality preserved
  • Comprehensive coverage: Protects against script injection, dangerous URLs, path traversal
  • Clean implementation: Built specifically for current main branch architecture

The rebased solution provides robust XSS protection while maintaining full compatibility with all recent main branch improvements. Ready for review!

@litlfred
Copy link
Copy Markdown
Owner

litlfred commented Aug 8, 2025

merge upstream @copilot

@litlfred litlfred closed this Aug 8, 2025
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.

address XSS

2 participants