diff --git a/CONTRIBUTING-SKILLS.md b/CONTRIBUTING-SKILLS.md index c2096c0..68b41d9 100644 --- a/CONTRIBUTING-SKILLS.md +++ b/CONTRIBUTING-SKILLS.md @@ -64,7 +64,7 @@ Every skill or agent must live in a plugin. Pick the one that matches your skill | Plugin | What it helps you do | Decision test | Example skills | |--------|---------------------|---------------|----------------| | **a11y** | Audit and document accessibility | Does this help me make my UI accessible? | | -| **code-review** | Review code for quality | Does this help me review code for quality? | | +| **code-review** | Review code for quality | Does this help me review code for quality? | `pf-security-scan` | | **design-audit** | Validate existing code/designs against PF standards | Does this check whether existing code or designs follow PF standards? | `pf-code-token-check`, `pf-color-scan`, `pf-figma-check` | | **design-guide** | Choose the right PF components and patterns when building | Does this help me choose the right PF components and patterns when building? | `pf-ai-guide`, `pf-figma-design-mode` | | **migration** | Upgrade PatternFly versions | Does this help me upgrade PF versions? | `pf-css-migration-scan`, `pf-react-migration-scan` | diff --git a/PLUGINS.md b/PLUGINS.md index 10393db..9fd31f6 100644 --- a/PLUGINS.md +++ b/PLUGINS.md @@ -28,6 +28,10 @@ No skills or agents yet. Code review and quality — adversarial review, security patterns +| Skill | Description | +|-------|-------------| +| `pf-security-scan` | Scan PatternFly React code for security anti-patterns — XSS via dangerouslySetInnerHTML, unsanitized user input in tooltips/labels, and insecure href patterns. | + | Agent | Description | |-------|-------------| | `pf-assist` | PatternFly development routing — maps code changes, test gaps, scaffolding needs, and design work to the right PF sub-skills. | diff --git a/README.md b/README.md index c546f68..3a3fa19 100644 --- a/README.md +++ b/README.md @@ -3,7 +3,7 @@ [![License](https://img.shields.io/github/license/patternfly/ai-helpers)](./LICENSE) [![PRs Welcome](https://img.shields.io/badge/PRs-welcome-brightgreen.svg)](./CONTRIBUTING.md) [![Plugins](https://img.shields.io/badge/plugins-8-blueviolet)](./PLUGINS.md) -[![Skills](https://img.shields.io/badge/skills-32-blue)](./PLUGINS.md) +[![Skills](https://img.shields.io/badge/skills-33-blue)](./PLUGINS.md) AI coding helpers for [PatternFly](https://www.patternfly.org/) development. This repository provides plugins and documentation to help AI tools generate accurate, best-practice PatternFly applications. diff --git a/plugins/code-review/README.md b/plugins/code-review/README.md index 251a0f8..bcf0063 100644 --- a/plugins/code-review/README.md +++ b/plugins/code-review/README.md @@ -6,6 +6,10 @@ Code review and quality — adversarial review, security patterns. ## What's Included +### Skills + +- **PF Security Scan** (`/code-review:pf-security-scan`) — Scan PatternFly React code for security anti-patterns — XSS via dangerouslySetInnerHTML, unsanitized user input in tooltips/labels, and insecure href patterns. + ### Agents - **PF Assist** (`pf-assist`) — PatternFly development routing — maps code changes, test gaps, scaffolding needs, and design work to the right PF sub-skills. diff --git a/plugins/code-review/agents/pf-assist.md b/plugins/code-review/agents/pf-assist.md index 5f84317..b2aee5e 100644 --- a/plugins/code-review/agents/pf-assist.md +++ b/plugins/code-review/agents/pf-assist.md @@ -21,6 +21,7 @@ Identify the current context from the developer's recent activity, then apply th | `/pf-figma-token-check` | Design token usage against PF token architecture | design-audit | | `/pf-figma-check` | Component usage against PF design guidelines | design-audit | | `/pf-css-migration-scan` | Legacy CSS classes from older PF versions | migration | +| `/pf-security-scan` | XSS, unsanitized user input in PF components, insecure href patterns | code-review | ### Testing — implementation done or tests needed diff --git a/plugins/code-review/skills/pf-security-scan/SKILL.md b/plugins/code-review/skills/pf-security-scan/SKILL.md new file mode 100644 index 0000000..0bc5857 --- /dev/null +++ b/plugins/code-review/skills/pf-security-scan/SKILL.md @@ -0,0 +1,149 @@ +--- +name: pf-security-scan +description: Scan PatternFly React code for security anti-patterns — XSS via dangerouslySetInnerHTML, unsanitized user input in tooltips/labels, and insecure href patterns. Use when reviewing PF code for security vulnerabilities or auditing user-controlled content in PF components. +--- + +# PF Security Scan + +Audit PatternFly React usage for security anti-patterns that turn UI components into XSS or open-redirect vectors. This skill focuses on **PF-specific hotspots** — not general app security (auth, CSRF, secrets). + +## PatternFly MCP + +If `@patternfly/patternfly-mcp` is available, use it to confirm which props accept HTML or render links for unfamiliar components. This skill defines **what to flag**; the MCP fills in API details. + +## How to run + +1. Ask the user which directory or files to scan. Default to the current working directory. +2. Limit scope to files importing from `@patternfly/react-core`, `@patternfly/react-table`, `@patternfly/react-component-groups`, or `@patternfly/react-templates`. +3. Run targeted searches (see [references/anti-patterns.md](references/anti-patterns.md) for grep patterns), then read flagged files for context. +4. Trace user-controlled data: API responses, URL params, query strings, form input, localStorage, WebSocket payloads, and props passed from parent components. +5. Report findings grouped by file with line numbers. End with a summary count. + +## Anti-patterns to detect + +### 1. XSS via `dangerouslySetInnerHTML` + +| Severity | Flag when | +|----------|-----------| +| **CRITICAL** | `dangerouslySetInnerHTML` with a value that is not a compile-time constant | +| **CRITICAL** | `__html` built from user/API data without a recognized sanitizer (DOMPurify, sanitize-html, or project-approved equivalent) | +| **WARN** | `dangerouslySetInnerHTML` anywhere — even with sanitization, confirm the sanitizer config allows only safe tags | + +Also flag PF components that accept HTML strings when fed unsanitized data: + +- `innerHTML`-style props or render callbacks that inject raw HTML +- Rich-text editors or markdown renderers whose output flows into PF `Text`, `Title`, `Alert`, or table cells without sanitization + +### 2. Unsanitized user input in tooltips, labels, and text surfaces + +User-controlled strings rendered in PF components are XSS vectors when the data can contain HTML or script payloads. + +| Severity | Component / prop | Flag when | +|----------|------------------|-----------| +| **CRITICAL** | `Tooltip` / `Popover` — `content`, `bodyContent`, `headerContent` | Value from user/API state without sanitization | +| **CRITICAL** | `Label`, `Badge`, `Chip`, `ChipGroup` — children or `text` | User-controlled value rendered directly | +| **HIGH** | `Alert` — `title`, `titleHeadingLevel`, children | Message from API/error payload without sanitization | +| **HIGH** | `Title`, `Text`, `Content` — children | User or CMS content interpolated without escaping | +| **HIGH** | `FormGroup` — `label`, `HelperText` — children | Label or helper text from user input | +| **HIGH** | `EmptyState` — `titleText`, `body` | Dynamic content from external source | +| **HIGH** | `DescriptionListDescription`, `DataListCell`, `Td` | Cell/description values from API without sanitization | +| **WARN** | `MenuItem`, `DropdownItem`, `SelectOption` — children | Display text from user data (lower risk if React text nodes, but still audit source) | + +**Safe patterns** (do not flag): + +- React text children (``) — React escapes by default +- Values passed through a project-approved sanitizer at the boundary +- Compile-time string literals + +**Unsafe patterns** (always flag): + +- `dangerouslySetInnerHTML` in or near PF text surfaces +- `innerHTML` assignment on DOM nodes inside PF component refs +- Rendering markdown/HTML from user input without sanitization pipeline + +### 3. Insecure URL / href patterns + +PF components frequently render links via `Button`, `NavItem`, `BreadcrumbItem`, `DropdownItem`, `MenuItem`, and standalone `` inside PF layouts. + +| Severity | Flag when | +|----------|-----------| +| **CRITICAL** | `href` or `to` set to `javascript:` URL (literal or constructed) | +| **CRITICAL** | `href` built from unsanitized user input without URL validation | +| **HIGH** | `href` using `data:` scheme with non-image MIME types | +| **HIGH** | Dynamic `href` from API/params with no allowlist or `URL` constructor validation | +| **WARN** | `target="_blank"` without `rel="noopener noreferrer"` on external links | +| **WARN** | `Button component="a"` with dynamic `href` — same rules as `` | + +**PF-specific link surfaces to check:** + +| Component | Props | +|-----------|-------| +| `Button` | `href`, `component="a"` | +| `NavItem` / `NavLink` | `to`, `href` | +| `BreadcrumbItem` | `to`, `href` | +| `DropdownItem` | `href`, `to` | +| `MenuItem` | `to`, `href` | +| `JumpLinksItem` | `href` | +| `ExternalLink` (if used) | `href` | + +**Safe patterns:** + +```tsx +// Allowlisted origin +const safeUrl = allowedOrigins.includes(new URL(userUrl).origin) ? userUrl : '#'; + +// React Router internal navigation only +... + +// External link with tab-nabbing protection + + +// Unvalidated redirect +... + +// String concat into href from API +... +``` + +## Report format + +For each finding: + +``` +[CRITICAL|HIGH|WARN] file/path.tsx:42 - Unsanitized API data in Tooltip content + Found: + Risk: XSS — attacker-controlled HTML in tooltip popper + Fix: Sanitize with DOMPurify before render, or render as React text child if HTML is not required +``` + +End with: + +``` +Scanned: 18 files +Critical: 2 (across 1 file) +High: 3 (across 2 files) +Warnings: 1 (across 1 file) +``` + +Group findings by anti-pattern category (XSS, unsanitized input, insecure URLs) so patterns are visible. + +## Applying fixes + +1. **Prefer React text nodes** over HTML rendering when formatting is not required. +2. **Sanitize at the boundary** — one approved sanitizer at the data ingress point, not scattered per component. +3. **Validate URLs** — parse with `new URL()`, allowlist origins or paths, reject `javascript:`, `data:`, and protocol-relative `//` from untrusted input. +4. **Add `rel="noopener noreferrer"`** to every `target="_blank"` link. +5. Do not weaken sanitizer configs to "make it work" — flag and ask the user if legitimate HTML is required. + +Only apply fixes when the remediation is unambiguous. If a finding needs a product decision (e.g., rich-text support), report it and ask. + +## Additional resources + +- Grep patterns and component checklist: [references/anti-patterns.md](references/anti-patterns.md) diff --git a/plugins/code-review/skills/pf-security-scan/references/anti-patterns.md b/plugins/code-review/skills/pf-security-scan/references/anti-patterns.md new file mode 100644 index 0000000..1c5ddcf --- /dev/null +++ b/plugins/code-review/skills/pf-security-scan/references/anti-patterns.md @@ -0,0 +1,70 @@ +# PF Security Anti-Patterns Reference + +## Scope grep commands + +Run from the project root. Adjust paths as needed. + +```bash +# Files using PatternFly React +rg "from '@patternfly/react" --glob '*.{tsx,ts,jsx,js}' + +# dangerouslySetInnerHTML (all occurrences need review) +rg "dangerouslySetInnerHTML" --glob '*.{tsx,ts,jsx,js}' + +# __html object pattern +rg "__html" --glob '*.{tsx,ts,jsx,js}' + +# javascript: URLs (literal) +rg "javascript:" --glob '*.{tsx,ts,jsx,js}' + +# data: URLs +rg "href=.*data:" --glob '*.{tsx,ts,jsx,js}' + +# target="_blank" candidates (manual review for rel attribute) +rg 'target="_blank"' --glob '*.{tsx,ts,jsx,js}' + +# Dynamic href/to bindings (manual review for validation) +rg 'href=\{' --glob '*.{tsx,ts,jsx,js}' +rg 'to=\{' --glob '*.{tsx,ts,jsx,js}' +``` + +## PF component checklist + +Components that commonly receive user-controlled strings or URLs: + +| Category | Components | Props / children | +|----------|------------|------------------| +| Tooltips / popovers | `Tooltip`, `Popover` | `content`, `bodyContent`, `headerContent` | +| Labels / chips | `Label`, `Badge`, `Chip`, `ChipGroup` | children, `text` | +| Links / navigation | `Button`, `NavItem`, `NavLink`, `BreadcrumbItem`, `DropdownItem`, `MenuItem`, `JumpLinksItem` | `href`, `to` | +| Text surfaces | `Title`, `Text`, `Content`, `Alert`, `HelperText`, `FormGroup` | children, `label`, `title` | +| Data display | `Td`, `DataListCell`, `DescriptionListDescription`, `EmptyState` | children, `dataLabel` | +| Menus / selects | `SelectOption`, `DropdownItem`, `MenuItem` | children | + +## Sanitizer recognition + +Treat these as approved sanitization when applied to the flagged value **before** render: + +- `DOMPurify.sanitize(...)` / `dompurify` +- `sanitize-html(...)` / `sanitizeHtml` +- Project-specific `sanitize*`, `cleanHtml`, `stripHtml` utilities — verify implementation if unfamiliar + +Absence of sanitization between user/API source and render target is a finding. + +## URL validation patterns + +| Pattern | Risk | Action | +|---------|------|--------| +| `href={userInput}` | Open redirect, `javascript:` injection | Validate origin/path against allowlist | +| `href={\`https://${domain}/...\`}` | Domain injection if `domain` is user-controlled | Allowlist domains | +| `href={searchParams.get('next')}` | Open redirect | Reject external URLs; allow relative paths only | +| `to={location.pathname + userInput}` | Path traversal / XSS in router | Validate path segments | +| `component="a"` + dynamic `href` | Same as `` | Apply URL rules | + +## Severity guide + +| Severity | Criteria | +|----------|----------| +| **CRITICAL** | Confirmed or highly likely exploitable XSS or `javascript:` execution path | +| **HIGH** | User-controlled content in PF render path without sanitization; unvalidated external URLs | +| **WARN** | Missing `rel` on `target="_blank"`; `dangerouslySetInnerHTML` with sanitization present but unverified; dynamic href with partial validation |