From c0c118e22385cad6ded7577c7a5c92d99afa6cbb Mon Sep 17 00:00:00 2001 From: sck_0 Date: Sun, 15 Mar 2026 09:21:51 +0100 Subject: [PATCH] fix(security): harden markdown rendering and sync safety --- apps/web-app/refresh-skills-plugin.js | 30 ++++++ .../refresh-skills-plugin.security.test.js | 98 +++++++++++++++++++ apps/web-app/src/pages/Home.tsx | 2 +- apps/web-app/src/pages/SkillDetail.tsx | 3 +- .../__tests__/SkillDetail.security.test.tsx | 69 +++++++++++++ tools/scripts/generate_index.py | 11 +++ .../test_frontmatter_parsing_security.py | 25 +++++ tools/scripts/validate_skills.py | 11 +++ 8 files changed, 246 insertions(+), 3 deletions(-) create mode 100644 apps/web-app/src/__tests__/refresh-skills-plugin.security.test.js create mode 100644 apps/web-app/src/pages/__tests__/SkillDetail.security.test.tsx diff --git a/apps/web-app/refresh-skills-plugin.js b/apps/web-app/refresh-skills-plugin.js index fee80aac..ff17a99a 100644 --- a/apps/web-app/refresh-skills-plugin.js +++ b/apps/web-app/refresh-skills-plugin.js @@ -49,6 +49,21 @@ function git(cmd) { return execSync(`git ${cmd}`, { cwd: ROOT_DIR, encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] }).trim(); } +function isAllowedDevOrigin(req) { + const host = req.headers?.host; + const origin = req.headers?.origin; + + if (!host || !origin) { + return false; + } + + try { + return new URL(origin).host === host; + } catch { + return false; + } +} + /** Ensure the upstream remote exists. */ function ensureUpstream() { const remotes = git('remote'); @@ -250,6 +265,19 @@ export default function refreshSkillsPlugin() { server.middlewares.use('/api/refresh-skills', async (req, res) => { res.setHeader('Content-Type', 'application/json'); + if (req.method !== 'POST') { + res.statusCode = 405; + res.setHeader('Allow', 'POST'); + res.end(JSON.stringify({ success: false, error: 'Method not allowed' })); + return; + } + + if (!isAllowedDevOrigin(req)) { + res.statusCode = 403; + res.end(JSON.stringify({ success: false, error: 'Forbidden origin' })); + return; + } + try { let result; @@ -287,3 +315,5 @@ export default function refreshSkillsPlugin() { } }; } + +export { isAllowedDevOrigin }; diff --git a/apps/web-app/src/__tests__/refresh-skills-plugin.security.test.js b/apps/web-app/src/__tests__/refresh-skills-plugin.security.test.js new file mode 100644 index 00000000..2f3b4ae1 --- /dev/null +++ b/apps/web-app/src/__tests__/refresh-skills-plugin.security.test.js @@ -0,0 +1,98 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +const execSync = vi.fn((command) => { + if (command === 'git --version') return ''; + if (command === 'git rev-parse --git-dir') return '.git'; + if (command === 'git remote') return 'origin\nupstream\n'; + if (command === 'git rev-parse HEAD') return 'abc123'; + if (command === 'git fetch upstream main') return ''; + if (command === 'git rev-parse upstream/main') return 'abc123'; + return ''; +}); + +vi.mock('child_process', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + execSync, + default: { + ...actual, + execSync, + }, + }; +}); + +function createResponse() { + return { + statusCode: 200, + headers: {}, + body: '', + setHeader(name, value) { + this.headers[name] = value; + }, + end(payload) { + this.body = payload; + }, + }; +} + +async function loadRefreshHandler() { + const { default: refreshSkillsPlugin } = await import('../../refresh-skills-plugin.js'); + const registrations = []; + const server = { + middlewares: { + use(pathOrHandler, maybeHandler) { + if (typeof pathOrHandler === 'string') { + registrations.push({ path: pathOrHandler, handler: maybeHandler }); + return; + } + registrations.push({ path: null, handler: pathOrHandler }); + }, + }, + }; + + refreshSkillsPlugin().configureServer(server); + const registration = registrations.find((item) => item.path === '/api/refresh-skills'); + if (!registration) { + throw new Error('refresh-skills handler not registered'); + } + return registration.handler; +} + +describe('refresh-skills plugin security', () => { + beforeEach(() => { + execSync.mockClear(); + }); + + it('rejects GET requests for the sync endpoint', async () => { + const handler = await loadRefreshHandler(); + const req = { + method: 'GET', + headers: { + host: 'localhost:5173', + origin: 'http://localhost:5173', + }, + }; + const res = createResponse(); + + await handler(req, res); + + expect(res.statusCode).toBe(405); + }); + + it('rejects cross-origin POST requests for the sync endpoint', async () => { + const handler = await loadRefreshHandler(); + const req = { + method: 'POST', + headers: { + host: 'localhost:5173', + origin: 'http://evil.test', + }, + }; + const res = createResponse(); + + await handler(req, res); + + expect(res.statusCode).toBe(403); + }); +}); diff --git a/apps/web-app/src/pages/Home.tsx b/apps/web-app/src/pages/Home.tsx index 77aa40fa..9c25de7f 100644 --- a/apps/web-app/src/pages/Home.tsx +++ b/apps/web-app/src/pages/Home.tsx @@ -74,7 +74,7 @@ export function Home(): React.ReactElement { setSyncing(true); setSyncMsg(null); try { - const res = await fetch('/api/refresh-skills'); + const res = await fetch('/api/refresh-skills', { method: 'POST' }); const data = await res.json(); if (data.success) { if (data.upToDate) { diff --git a/apps/web-app/src/pages/SkillDetail.tsx b/apps/web-app/src/pages/SkillDetail.tsx index 7e7e65ee..bc08e1d2 100644 --- a/apps/web-app/src/pages/SkillDetail.tsx +++ b/apps/web-app/src/pages/SkillDetail.tsx @@ -5,7 +5,6 @@ import { SkillStarButton } from '../components/SkillStarButton'; import { useSkills } from '../context/SkillContext'; import remarkGfm from 'remark-gfm'; import rehypeHighlight from 'rehype-highlight'; -import rehypeRaw from 'rehype-raw'; // Lazy load heavy markdown component const Markdown = lazy(() => import('react-markdown')); @@ -261,7 +260,7 @@ export function SkillDetail(): React.ReactElement { }> {markdownBody} diff --git a/apps/web-app/src/pages/__tests__/SkillDetail.security.test.tsx b/apps/web-app/src/pages/__tests__/SkillDetail.security.test.tsx new file mode 100644 index 00000000..9f75dfad --- /dev/null +++ b/apps/web-app/src/pages/__tests__/SkillDetail.security.test.tsx @@ -0,0 +1,69 @@ +import { describe, it, expect, vi, beforeEach, Mock } from 'vitest'; +import { waitFor } from '@testing-library/react'; +import { SkillDetail } from '../SkillDetail'; +import { renderWithRouter } from '../../utils/testUtils'; +import { createMockSkill } from '../../factories/skill'; +import { useSkills } from '../../context/SkillContext'; + +let capturedRehypePlugins: unknown[] | undefined; + +vi.mock('../../components/SkillStarButton', () => ({ + SkillStarButton: () => , +})); + +vi.mock('../../context/SkillContext', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + useSkills: vi.fn(), + }; +}); + +vi.mock('react-markdown', () => ({ + default: ({ children, rehypePlugins }: { children: string; rehypePlugins?: unknown[] }) => { + capturedRehypePlugins = rehypePlugins; + return
{children}
; + }, +})); + +describe('SkillDetail security', () => { + beforeEach(() => { + vi.clearAllMocks(); + capturedRehypePlugins = undefined; + }); + + it('does not enable raw HTML rendering for skill markdown', async () => { + const mockSkill = createMockSkill({ + id: 'unsafe-skill', + name: 'unsafe-skill', + description: 'Skill with embedded html', + }); + + (useSkills as Mock).mockReturnValue({ + skills: [mockSkill], + stars: {}, + loading: false, + }); + + global.fetch = vi.fn().mockResolvedValue({ + ok: true, + text: async () => '# Demo\n\n', + }); + + renderWithRouter(, { + route: '/skill/unsafe-skill', + path: '/skill/:id', + useProvider: false, + }); + + await waitFor(() => { + expect(capturedRehypePlugins).toBeDefined(); + }); + + const pluginNames = (capturedRehypePlugins ?? []).map((plugin) => + typeof plugin === 'function' ? plugin.name : String(plugin), + ); + + expect(pluginNames).not.toContain('rehypeRaw'); + }); +}); diff --git a/tools/scripts/generate_index.py b/tools/scripts/generate_index.py index d6e0e8c8..b6615a88 100644 --- a/tools/scripts/generate_index.py +++ b/tools/scripts/generate_index.py @@ -3,6 +3,7 @@ import json import re import sys from collections.abc import Mapping +from datetime import date, datetime import yaml from _project_paths import find_repo_root @@ -13,6 +14,15 @@ if sys.platform == 'win32': sys.stdout = io.TextIOWrapper(sys.stdout.buffer, encoding='utf-8') sys.stderr = io.TextIOWrapper(sys.stderr.buffer, encoding='utf-8') +def normalize_yaml_value(value): + if isinstance(value, Mapping): + return {key: normalize_yaml_value(val) for key, val in value.items()} + if isinstance(value, list): + return [normalize_yaml_value(item) for item in value] + if isinstance(value, (date, datetime)): + return value.isoformat() + return value + def parse_frontmatter(content): """ Parses YAML frontmatter, sanitizing unquoted values containing @. @@ -43,6 +53,7 @@ def parse_frontmatter(content): try: parsed = yaml.safe_load(sanitized_yaml) or {} + parsed = normalize_yaml_value(parsed) if not isinstance(parsed, Mapping): print("⚠️ YAML frontmatter must be a mapping/object") return {} diff --git a/tools/scripts/tests/test_frontmatter_parsing_security.py b/tools/scripts/tests/test_frontmatter_parsing_security.py index 29b7f472..71fcb501 100644 --- a/tools/scripts/tests/test_frontmatter_parsing_security.py +++ b/tools/scripts/tests/test_frontmatter_parsing_security.py @@ -38,6 +38,31 @@ class FrontmatterParsingSecurityTests(unittest.TestCase): self.assertIsNone(metadata) self.assertTrue(any("mapping" in error.lower() for error in errors)) + def test_validate_skills_normalizes_unquoted_yaml_dates(self): + content = "---\nname: demo\ndescription: ok\ndate_added: 2026-03-15\n---\nbody\n" + metadata, errors = validate_skills.parse_frontmatter(content) + + self.assertEqual(errors, []) + self.assertEqual(metadata["date_added"], "2026-03-15") + + def test_generate_index_serializes_unquoted_yaml_dates(self): + with tempfile.TemporaryDirectory() as temp_dir: + root = Path(temp_dir) + skills_dir = root / "skills" + skill_dir = skills_dir / "demo" + output_file = root / "skills_index.json" + + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + "---\nname: demo\ndescription: ok\ndate_added: 2026-03-15\n---\nBody\n", + encoding="utf-8", + ) + + skills = generate_index.generate_index(str(skills_dir), str(output_file)) + + self.assertEqual(skills[0]["date_added"], "2026-03-15") + self.assertIn('"date_added": "2026-03-15"', output_file.read_text(encoding="utf-8")) + def test_generate_index_ignores_symlinked_skill_markdown(self): with tempfile.TemporaryDirectory() as temp_dir: root = Path(temp_dir) diff --git a/tools/scripts/validate_skills.py b/tools/scripts/validate_skills.py index 57edc0b8..69f91555 100644 --- a/tools/scripts/validate_skills.py +++ b/tools/scripts/validate_skills.py @@ -5,6 +5,7 @@ import sys import io import yaml from collections.abc import Mapping +from datetime import date, datetime from _project_paths import find_repo_root @@ -38,6 +39,15 @@ WHEN_TO_USE_PATTERNS = [ def has_when_to_use_section(content): return any(pattern.search(content) for pattern in WHEN_TO_USE_PATTERNS) +def normalize_yaml_value(value): + if isinstance(value, Mapping): + return {key: normalize_yaml_value(val) for key, val in value.items()} + if isinstance(value, list): + return [normalize_yaml_value(item) for item in value] + if isinstance(value, (date, datetime)): + return value.isoformat() + return value + def parse_frontmatter(content, rel_path=None): """ Parse frontmatter using PyYAML for robustness. @@ -51,6 +61,7 @@ def parse_frontmatter(content, rel_path=None): fm_errors = [] try: metadata = yaml.safe_load(fm_text) or {} + metadata = normalize_yaml_value(metadata) if not isinstance(metadata, Mapping): return None, ["Frontmatter must be a YAML mapping/object."]