Feature/web gui skill scanner#62
Conversation
Made-with: Cursor
Made-with: Cursor
vineethsai7
left a comment
There was a problem hiding this comment.
Code Review — Security & Code Quality
Security Findings
1. CRITICAL: llm_api_key accepted as Form field / request body instead of Header
The llm_api_key is accepted via Form(...) on the upload endpoints (/scan-upload-html, /scan-upload-markdown) and as a Pydantic body field on ScanRequest. API keys in request bodies/form data are more likely to be logged by proxies, WAFs, and access logs than when sent via headers.
The existing pattern for vt_api_key and aidefense_api_key correctly uses Header(None, alias="X-...-Key"). The llm_api_key should follow the same convention.
Files: skill_scanner/api/router.py lines 199-201, 729-731, 917-919
2. HIGH: Missing Subresource Integrity (SRI) on external CDN script
<script src="https://cdn.jsdelivr.net/npm/jszip@3.10.1/dist/jszip.min.js"></script>External scripts must use SRI hashes to prevent supply chain attacks. If the CDN is compromised, arbitrary JS would execute in the user's browser.
File: skill_scanner/api/frontend/index.html line 626
3. MEDIUM: innerHTML used with file names — DOM XSS sink
The setStatus function uses innerHTML to render status messages containing file names (e.g., file.name). While low risk for local files, this is a DOM XSS sink pattern. Should use textContent or escape HTML entities before interpolation.
File: skill_scanner/api/frontend/index.html lines 656-657, 711
4. MEDIUM: No Content-Security-Policy header or meta tag
The static HTML page and API responses don't set a CSP header. For a tool that renders untrusted HTML reports inside an iframe (via srcdoc), a CSP provides defense-in-depth against injected scripts.
File: skill_scanner/api/frontend/index.html
Code Quality Findings
5. HIGH: ~300 lines of duplicated upload/scan/extract logic
The three upload endpoints (/scan-upload, /scan-upload-html, /scan-upload-markdown) each independently implement the complete ZIP upload, validation, extraction, and scan pipeline (~150 lines each of nearly identical code). Should extract a shared helper.
File: skill_scanner/api/router.py
6. MEDIUM: /scan-html duplicates /scan endpoint logic
The /scan-html endpoint is a near-copy of /scan, differing only in the final rendering step. The scan + meta-analysis logic should be factored into a shared helper.
7. MEDIUM: Nested asyncio.run() inside run_in_executor is fragile
In scan_uploaded_skill_html and scan_uploaded_skill_markdown, meta-analysis creates a new event loop inside a thread inside the existing event loop. This can be simplified since the endpoint is already async.
8. LOW: Imports inside function bodies; _frontend_dir path computation is non-obvious
Multiple endpoints have import asyncio, import stat, import zipfile inside function bodies. Also Path(__file__).with_suffix("").parent / "frontend" could be simplified to Path(__file__).parent / "frontend".
Security: - Bundle JSZip locally to eliminate external CDN calls - Tighten CSP to default-src 'none' with explicit allowlist - Remove allow-same-origin from iframe sandbox - Add Referrer-Policy meta tag and server-side security headers (X-Content-Type-Options, X-Frame-Options, Cache-Control, Permissions-Policy) - Fix double-encoding bug in status/error display UI/UX: - Add light/dark mode with system preference detection and persistence - Add summary cards showing scan findings by severity - Add live scan timer and in-session scan history - Improve error handling with styled banner and retry button - Add ARIA roles, labels, and focus indicators for accessibility Packaging: - Move fastapi/uvicorn/python-multipart to optional [web] extras group - Guard API imports so core package works without web dependencies - Update error messages to guide users to install [web] extra Code quality: - Deduplicate shared helpers in API router
- Bundle JSZip locally (skill_scanner/api/frontend/js/jszip.min.js), remove CDN - Add Content-Security-Policy meta tag in frontend index.html - Extract _extract_uploaded_zip() for shared ZIP upload/validate/extract logic - Extract _run_scan_with_meta() for shared scan + meta-analysis; use in /scan, /scan-html, /scan-upload-html, /scan-upload-markdown - Replace nested asyncio.run() in upload handlers with direct await - Move asyncio, concurrent.futures, stat, zipfile to top-level imports in router - Use Path(__file__).parent for frontend dir in api.py
Pull Request
Description
Summary:
This PR adds a lightweight local web UI for Skill Scanner, mounted under
/uion the existing FastAPI server.skill_scanner/apiand exposes it at/ui.SKILL.md) and run a scan using the existing analyzers.HTMLReporterto embed interactive reports, and adds an option to download both HTML and Markdown reports.Implementation notes
skill_scanner/api/frontend/index.htmland talks to:POST /scan-upload-html(HTML report)POST /scan-upload-markdown(Markdown report)POST /scan-htmlfor scanning an on-disk skill directory.skill_scanner/api/api.pymounts the frontend at/uiusingStaticFiles.router.pywires the new endpoints to the existingSkillScannerand reporters without changing CLI behavior.Motivation:
This makes it much easier for users to quickly inspect scan results locally without having to construct CLI commands, and provides a more discoverable way to tweak scan options and view rich HTML reports.
Type of Change
Related Issues
Closes #[issue number]
Fixes #[issue number]
Related to #[issue number]
Changes Made
Change 1: Add local web UI mounted at
/uifrontenddirectory fromskill_scanner/apiviaStaticFilesinapi.py.index.htmlprovides a dark-themed UI with drag‑and‑drop upload, “Choose ZIP…” and “Choose folder…” actions, and an embedded iframe for interactive HTML reports.Change 2: Extend API router to support UI workflows
ScanRequestand_build_analyzersto accept optionalllm_model,llm_base_url, andllm_api_key, wiring them through tobuild_analyzers./scan-htmlto scan an existing skill directory and return an HTML report usingHTMLReporter./scan-upload-html(HTML) and/scan-upload-markdown(Markdown) endpoints that reuse the existingSkillScannerpipeline and meta‑analyzer, streaming results back as reports for the UI.Change 3: Improve report rendering & UX
HTMLReportertable CSS to use fixed layout and word‑wrapping so long paths don’t require horizontal scrolling.Testing
Test Coverage
Manual Testing
Describe manual testing performed:
# Commands run for testing skill-scanner-apiOpen
http://localhost:8000/ui/in your local browser.Results:
/ui, drop a ZIP or choose a folder to scan, see an embedded HTML report, configure LLM options when needed, and download both HTML and Markdown reports directly from the UI.Checklist
Code Quality
Documentation
Security
Testing
uv run pre-commit run --all-filesuv run python evals/runners/benchmark_runner.pyPerformance Impact
Screenshots (if applicable)
Additional Notes
Any additional information reviewers should know.
Reviewer Checklist
For reviewers: