diff --git a/.coderabbit.yaml b/.coderabbit.yaml index acfa54b15e..2ed0522683 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -6,6 +6,8 @@ # docs/cpp.instructions.md — C++ coding conventions # docs/web.instructions.md — Web UI coding conventions # docs/cicd.instructions.md — GitHub Actions / CI-CD conventions +# docs/hardening.instructions.md — basic rules for code hardening and robustness +# docs/securecode.instructions.md — more detailed checklists for common vulnerabilities # # NOTE: This file must be committed (tracked by git) for CodeRabbit to read # it from the repository. If it is listed in .gitignore, CodeRabbit will @@ -53,6 +55,28 @@ reviews: 5. VERIFY numerical stability / accuracy of fixed-point arithmetic. 6. CHECK for implied but weakly justified assumptions - like usermod loop() call frequency - and ask for clarification. + # ── Security hardening — firmware (trust-boundary-aware) ──────────────── + - path: "wled00/**/*.{cpp,h,hpp,ino}" + instructions: > + Apply the WLED security hardening rules from docs/hardening.instructions.md, + and consult docs/securecode.instructions.md when more details are needed for actionable recommendations. + + Trust Boundary Model — enforce input-validation and bounds-checking rules + ONLY at the first untrusted ingress point. Untrusted ingress points are: + - HTTP/JSON API request bodies and query parameters (/json/*, /win, etc.) + - WebSocket message payloads + - UDP datagrams (parsePacket() / recvfrom() and protocol wrappers for + E1.31, DDP, Art-Net, TPM2.net) + - TCP socket reads + - Serial/UART command input + - ESP-NOW raw messages input + + A value that has been validated and range-clamped at its ingress handler is + considered TRUSTED for all subsequent WLED core processing. Do NOT flag or suggest + repeated bounds/range checks or internal uses of already-sanitized data. + When it is unclear whether a value has been sanitized upstream, prefer + requesting clarification over raising a false-positive finding. + - path: "wled00/data/**" instructions: > Follow the web UI conventions documented in docs/web.instructions.md. @@ -60,8 +84,21 @@ reviews: Key rules: indent HTML and JavaScript with tabs, CSS with tabs. Files here are built into wled00/html_*.h and wled00/js_*.h by tools/cdata.js — never edit those generated headers directly. -# disabled - the below instruction has no effect -# When initially reviewing a PR, summarize good practices (top 5) and create a prioritized list of suggested improvements (focus on major ones). + + # ── Security hardening — WebUI (always an ingress/output surface) ──────── + - path: "wled00/data/**" + instructions: > + Apply the WLED web UI security rules from docs/securecode.instructions.md + (sections WEB1-WEB7). + + The Trust Boundary Model does NOT reduce scope here: the WebUI is both + an ingress point (user input, postMessage, fetched config data) and an + output/rendering surface. Always flag DOM XSS risks, unsafe + innerHTML / document.write / insertAdjacentHTML / outerHTML assignments, + postMessage handlers without origin validation, eval() / new Function(), + unsafe location.href or location.replace() assignments, and DOM insertion + from fetched or config-derived data — regardless of where the data + originates. - path: "wled00/html_*.h" instructions: > @@ -81,8 +118,33 @@ reviews: Each usermod lives in its own directory under usermods/ and is implemented as a .cpp file with a dedicated library.json file to manage dependencies. Follow the same C++ conventions as the core firmware (docs/cpp.instructions.md). -# disabled - the below instruction has no effect -# When initially reviewing a PR, summarize good practices (top 6) and create a prioritized list of suggested improvements (skip minor ones). + + # ── Security hardening — usermods (trust-boundary-aware, narrow scope) ─── + - path: "usermods/**/*.{cpp,h,hpp}" + instructions: > + For usermods, the untrusted ingress points are: + - readFromConfig(JsonObject& root) and calls to getJsonValue() + - readFromJsonState(JsonObject& obj) — JSON is parsed, but values are client-supplied + - onMqttMessage(char* topic, char* payload) — raw network strings, no core sanitization + - onEspNowMessage(uint8_t* sender, uint8_t* payload, uint8_t len) — raw radio bytes + - onUdpPacket(uint8_t* payload, size_t len) — raw UDP buffer, no core filtering + Values retrieved at these ingress points are considered trusted only after the + usermod itself has validated and range-clamped them. + + Flag ONLY downstream uses of ingress-derived values where an out-of-range or + unexpected value can cause misbehaviour that is not already guarded, for example: + - `switch` statements on an ingress-derived value with no `default` branch, + or with a missing `break` where fall-through is unintentional + - array or buffer indexing with an ingress-derived value where the index is + not clamped before use + - arithmetic with an ingress-derived value that can overflow or produce a + negative result used as a size or count + + Do NOT flag: + - getJsonValue() call sites themselves (type coercion is handled by ArduinoJson) + - Internal logic that operates on values already confirmed safe at ingress + - Repeated range checks on values that have already been clamped + - General memory-safety patterns unrelated to ingress-derived data flow - path: ".github/workflows/*.{yml,yaml}" instructions: > @@ -94,8 +156,6 @@ reviews: scoped to least privilege. Never interpolate github.event.* values directly into run: steps — pass them through an env: variable to prevent script injection. Do not use pull_request_target unless fully justified. -# disabled - the below instruction has no effect -# When initially reviewing a PR, summarize good practices (top 6) and create a prioritized list of suggested improvements. - path: "**/*.instructions.md" instructions: | @@ -113,6 +173,73 @@ reviews: 3. If new AI-facing rules were added without updating a related HUMAN_ONLY reference section, note this as a suggestion (not a required fix). + # ── Secrets / sensitive information scanning ──────────────────────────── + - path: "platformio*.ini*" + instructions: > + Scan for secrets, passwords, and other sensitive information accidentally + committed to PlatformIO configuration files (platformio.ini, + platformio_override.ini, platformio_override.ini.sample). + + Flag any of the following: + - build_flags entries that define credentials as literal values, e.g.: + -DWIFI_SSID=\"\" -DWIFI_PASS=\"\" + -DOTA_PASS=\"\" -DMQTT_PASS=\"\" + Flag only when the value is not a recognisable placeholder (see below). + - upload_flags or upload_port values that embed a password or auth token (e.g., --auth= or any URL using credential-bearing userinfo). + - Any key = pair whose key name contains "pass", "password", + "secret", "token", "key", "credential", or "auth" where the value is + a non-empty, non-placeholder literal string. + - Hardcoded IP addresses or hostnames paired with credentials in the + same environment section. + - API keys or access tokens as literal strings in any field. + + Do NOT flag: + - Values that are clearly template placeholders (e.g., YOUR_SSID, + , changeme, example_token, your_password_here). + - Values that use PlatformIO environment variable substitution (${sysenv.WIFI_PASS} or ${env:WIFI_PASS}). + - Comments that only explain what a field should contain. + - platformio_override.ini.sample entries that contain only + placeholder/example values. + + - path: "usermods/**/library.json" + instructions: > + Scan for secrets and sensitive information in usermod dependency manifests. + + Flag any of the following: + - Dependency URLs that embed credentials in the URL itself (e.g., any URL containing credential-bearing userinfo). + - Personal access tokens, OAuth tokens, or API keys as literal strings + anywhere in the file. + - Values matching well-known secret patterns: GitHub PATs (ghp_..., + github_pat_...), AWS access keys (AKIA...), or similarly structured + high-entropy tokens. + + Do NOT flag: + - Plain HTTPS or SSH URLs without embedded credentials. + - Version specifiers, semver ranges, or commit SHA references that + contain no credential prefix. + - Repository owner/name path segments (not credential material). + + - path: "usermods/**/{readme,README,Readme}.md" + instructions: > + Scan for secrets, passwords, and sensitive information in usermod + documentation files, including inside code blocks, inline code, and prose. + + Flag any of the following: + - Hardcoded Wi-Fi SSID or password values that appear to be real (non-placeholder) + strings in configuration or installation examples. + - Hardcoded OTA, AP, or MQTT passwords in code snippets or step-by-step + instructions. + - API keys, bearer tokens, or access tokens shown as literal values. + - Example platformio_override.ini snippets that contain real-looking + credential values instead of placeholders. + - Hardcoded IP addresses combined with credentials in the same example. + + Do NOT flag: + - Values that are clearly template placeholders (e.g., YOUR_SSID, + , my_secret, changeme, ****). + - Generic prose describing what a field means without supplying a value. + - Asterisk-masked values (e.g., ******, ••••••). + finishing_touches: # Docstrings | Options for generating Docstrings for your PRs/MRs. docstrings: diff --git a/AGENTS.md b/AGENTS.md index a6b5e48cd9..6b17234b4e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,7 +4,8 @@ WLED is C++ firmware for ESP32/ESP8266 microcontrollers controlling addressable with a web UI (HTML/JS/CSS). Built with PlatformIO (Arduino framework) and Node.js tooling. See also: `.github/copilot-instructions.md`, `.github/agent-build.instructions.md`, -`docs/cpp.instructions.md`, `docs/web.instructions.md`, `docs/cicd.instructions.md`. +`docs/cpp.instructions.md`, `docs/web.instructions.md`, `docs/cicd.instructions.md`, +`docs/hardening.instructions.md`, `docs/securecode.instructions.md`. Always reference these instructions - including coding guidelines in `docs/` - first and fallback to search or bash commands only when you encounter unexpected information that does not match the info here. @@ -27,7 +28,7 @@ required C headers for firmware compilation. Tests use Node.js built-in test runner (`node:test`). The single test file is `tools/cdata-test.js`. Run it with: -```sh +```bash npm test # runs all tests via `node --test` node --test tools/cdata-test.js # run just that file directly ``` @@ -41,7 +42,7 @@ target environments. Always build after code changes: `pio run -e esp32dev`. ### Recovery / Troubleshooting -```sh +```bash npm run build -- -f # force web UI rebuild rm -f wled00/html_*.h wled00/js_*.h && npm run build # clean + rebuild UI pio run --target clean # clean PlatformIO build artifacts @@ -50,7 +51,7 @@ rm -rf node_modules && npm ci # reinstall Node.js deps ## Project Structure -``` +```text wled00/ # Main firmware source (C++) data/ # Web UI source (HTML/JS/CSS) — tabs for indentation html_*.h, js_*.h # Auto-generated (NEVER edit or commit) @@ -132,6 +133,7 @@ main # Main development trunk (daily/nightly) 17.0.0-dev. Target - **Performance**: Prefer DRAM (or IRAM) for hot-path data that is *frequently* used. Prefer PSRAM for capacity-oriented buffers where slightly slower access times can be tolerated. Background Info: + - PSRAM access is up to 18× slower than DRAM on ESP32 (dual-SPI bus), 3–10× slower than DRAM on ESP32-S3/-S2 with quad-SPI bus. On ESP32-S3 with octal PSRAM (`CONFIG_SPIRAM_MODE_OCT`), the penalty is smaller (~2×) because the 8-line DTR bus can transfer 8 bits in parallel. On ESP32-P4 with hex PSRAM (`CONFIG_SPIRAM_MODE_HEX`), the 16-line bus runs at 200 MHz which brings it on-par with DRAM. - Consider that ESP32 often crashes when the largest DRAM chunk gets below 10 KB. @@ -158,10 +160,10 @@ Background Info: #### ESP32 Task Synchronization -* Use FreeRTOS mutexes, semaphores or queues when true concurrent access from multiple FreeRTOS tasks is possible, and race-conditions can lead to unexpected behaviour. -* **Avoid `portENTER_CRITICAL()` / `portEXIT_CRITICAL()`**, as these functions stall the complete system and may cause LEDs flickering. Prefer FreeRTOS mutexes, semaphores or queues. -* **Important**: Not every shared resource needs a mutex. Some synchronization is guaranteed by the overall control flow, for example when function calls are sequenced within the same loop iteration. -* Consider RAII as an alternative to mutexes or semaphores. +- Use FreeRTOS mutexes, semaphores or queues when true concurrent access from multiple FreeRTOS tasks is possible, and race-conditions can lead to unexpected behaviour. +- **Avoid `portENTER_CRITICAL()` / `portEXIT_CRITICAL()`**, as these functions stall the complete system and may cause LEDs flickering. Prefer FreeRTOS mutexes, semaphores or queues. +- **Important**: Not every shared resource needs a mutex. Some synchronization is guaranteed by the overall control flow, for example when function calls are sequenced within the same loop iteration. +- Consider RAII as an alternative to mutexes or semaphores. ## Web UI Code Style (wled00/data/) @@ -240,9 +242,16 @@ No automated linting is configured. Match existing code style in files you edit. - Provide references when making analyses or recommendations. Support factual claims with verifiable citations, references or concrete evidence; **never fabricate citations**. - **Highlight user-visible breaking changes and ripple effects** during reviews. Ask for confirmation that these were introduced intentionally. +### Security Hardening + +When writing or reviewing code in `wled00/`, `usermods/`, `wled00/data/`, or `.github/workflows/`, +consult `docs/hardening.instructions.md` (concise checklist) and `docs/securecode.instructions.md` (detailed rules with examples). +These files define WLED's threat model, trust boundary model, and WLED-specific constraints (no TLS baseline, no UDP authentication for protocol-defined +multicast/broadcast, firewall-isolated deployment assumed). + ### Attribution for AI-generated code -Using AI-generated code can hide the source of the inspiration / knowledge / sources it used. +Using AI-generated code can hide the source of the inspiration / knowledge / sources it used. - Document attribution of inspiration / knowledge / sources used in the code, e.g. link to GitHub repositories or other websites describing the principles / algorithms used. - When a larger block of code is generated by an AI tool, embed it into `// AI: below section was generated by an AI` ... `// AI: end` comments (see Comments section). @@ -251,5 +260,5 @@ Using AI-generated code can hide the source of the inspiration / knowledge / sou ### Supporting Reviews and Discussions -- **For "is it worth doing?" debates** about proposed reliability, safety, or data-integrity mechanisms (CRC checks, backups, power-loss protection): suggest a software **FMEA** (Failure Mode and Effects Analysis). +- **For "is it worth doing?" debates** about proposed reliability, safety, or data-integrity mechanisms (CRC checks, backups, power-loss protection): suggest a software **FMEA** (Failure Mode and Effects Analysis). Clarify the main feared events, enumerate failure modes, assess each mitigation's effectiveness per failure mode, note common-cause failures, and rate credibility for the typical WLED use case. diff --git a/docs/hardening.instructions.md b/docs/hardening.instructions.md new file mode 100644 index 0000000000..3f1e53e6e3 --- /dev/null +++ b/docs/hardening.instructions.md @@ -0,0 +1,58 @@ +--- +applyTo: "**/*.{cpp,h,hpp,ino,js,htm,html,css,yml,yaml}" +description: "WLED strict-mode security review: low-noise checklist." +--- + +# WLED Security Review — Low Noise Mode + +Use these code hardening rules for automated reviews with minimal false positives. + +## WLED Constraints (apply to all rules) + +- Assume firewall/DMZ/VPN deployment; focus on realistic LAN-local and supply-chain risks. +- Do **not** require TLS/HTTPS as a baseline control. +- Do **not** require authentication for standards-based UDP multicast/broadcast paths where authentication is not defined in the protocol specification. + +> **Trust boundary model**: Apply input-validation rules **only at the first untrusted ingress point** +> (HTTP/JSON API body or query string, WebSocket payload, UDP datagram, TCP read, serial command, ESP-NOW raw messages). +> Values that have been validated and range-clamped at ingress are **trusted** for internal WLED +> processing. Do not flag subsequent uses or internal copies of already-sanitized data. + +## CRITICAL Rules + +1. **No unchecked buffer copies** (`memcpy`, `memmove`, `strcpy`) in firmware paths when source buffer or size comes from an untrusted origin; prefer bounded alternatives (`strncpy`, `strlcpy`); require length validation before copying. +2. **No user-controlled format strings** in `DEBUG_PRINTF*` and similar logging APIs. +3. **Validate all untrusted external input** (HTTP/JSON/UDP/serial) before index/length/pin usage. +4. **Auth required for state-changing control endpoints where feasible** (for example HTTP/JSON); do not flag protocol-defined unauthenticated UDP multicast/broadcast channels solely for missing auth. +5. **No fail-open on parse/allocation errors** for config/state updates. +6. **No DOM XSS sinks with untrusted data** (`innerHTML`, unsafe HTML insertion). Server-side generation of JavaScript property-assignment statements (as used in WLED's printSetForm* helpers) is exempt. +7. **No dynamic code execution** (`eval`, `new Function`, string timers). +8. **No hardcoded secrets/credentials/tokens/keys** in committed files. +9. **No sensitive data in logs** (passwords, tokens, Wi-Fi secrets, auth headers). +10. **No secret exposure in workflows/log output, or in LittleFS files other than `wsec.json`**. +11. **No unsafe third-party GitHub Action pinning** (`@main`/`@master` disallowed). +12. **No untrusted expression interpolation in workflow shell commands**. + +## IMPORTANT Rules + +13. Avoid potentially unbounded string/memory operations (`strcmp`, `strchr`, `strlen`, `sprintf`) in firmware paths; prefer bounded alternatives (`strnlen`, `strncmp`, `snprintf`). +14. Check integer overflow risks in size/index arithmetic, but consider that unsigned wrap-around on small types might be intentional. +15. Reject repeated heap allocation churn in hot render/effect loops. +16. Avoid repeated `String` growth in hot paths; prefer bounded/pre-allocated buffers. +17. Ensure UI validation is mirrored by firmware-side validation. +18. Require strict origin checks for `postMessage` listeners. +19. Disallow untrusted redirect/navigation targets. +20. Prevent verbose error responses that leak internals. +21. Review new dependencies for typosquatting and known vulnerability risk. +22. Keep workflow `permissions` least-privilege. +23. Verify new `WLED_ENABLE_*` / `WLED_DISABLE_*` names are valid known flags. +24. New privileged behavior must not be enabled by insecure defaults; first-use default-credential change required where applicable. +25. OTA paths (Update.begin(), Update.write()) must verify firmware integrity (checksum/hash); TLS not required. +26. Flag xTaskCreate/xTaskCreatePinnedToCore tasks with insufficient stack for String/JSON use; flag MDNS.begin() / ArduinoOTA.setHostname() with unsanitized hostnames. +27. Flag API/config serialization that exposes Wi-Fi/AP/MQTT password fields to unauthenticated clients. +28. Treat fetched and config-derived strings as untrusted when inserting into the DOM; explicit sanitization required for HTML contexts. + +## Reviewer Output Format + +- Include severity, exact file and line, and one concrete fix direction. +- Prioritize CRITICAL findings before IMPORTANT findings. diff --git a/docs/securecode.instructions.md b/docs/securecode.instructions.md new file mode 100644 index 0000000000..7cc9593daa --- /dev/null +++ b/docs/securecode.instructions.md @@ -0,0 +1,227 @@ +--- +applyTo: "**/*.{cpp,h,hpp,ino,js,htm,html,css,yml,yaml}" +description: "WLED-focused security review guide based on OWASP Top 10 for embedded firmware and web UI." +--- + +# WLED Security Review Standards (Embedded + Web UI) + +Use this guide for AI-assisted code reviews in: +- `wled00/` +- `usermods/` +- `.github/workflows/` + +## WLED Constraints and Threat Model Assumptions + +- Assume typical deployment behind a firewall/DMZ/VPN; prioritize LAN-local and supply-chain risks. +- Do **not** require TLS/HTTPS as a baseline control for findings in this repo. +- Do **not** require authentication for standards-based UDP multicast/broadcast protocols where auth is not part of the spec. +- Do not propose mitigations that break protocol compliance just to add authentication. + +### Trust Boundary Model + +**Untrusted data** enters WLED only at the following explicit ingress points: +- HTTP/JSON API request bodies and query parameters (e.g., `/json/*`, `/win`) +- WebSocket message payloads +- UDP datagrams (`parsePacket()` / `recvfrom()` and higher-level protocol wrappers) +- TCP socket reads +- Serial/UART input used as commands +- ESP-NOW raw messages input + +**Validation and range-clamping applied at the ingress point renders data trusted** for all subsequent use within the WLED core. + +**Do not flag:** +- Repeated bounds or range checks on a value that has already been validated and clamped at its ingress handler. +- Internal WLED core logic that operates on values confirmed safe by the ingress layer. + +If it is unclear whether a value has been sanitized upstream (e.g., passed through multiple function calls without a clear annotation), prefer asking for clarification over raising a false-positive finding. + +### Locally-Stored Configuration Files (Robustness, not a primary trust boundary) + +Files read from LittleFS (`presets.json`, `cfg.json`, `ledmap.json`, `ir.json`, etc.) are written only via privileged access (`/edit`) and are considered trusted in the threat model. +However, parse them defensively (validate structure, clamp array sizes, handle missing keys gracefully) to avoid bootloops from filesystem corruption or accidental malformation. + +## Severity + +- **CRITICAL** — exploitable vulnerability; block merge. +- **IMPORTANT** — meaningful risk; fix before or with merge when practical. +- **SUGGESTION** — defense-in-depth; track for follow-up. + +## Scope (WLED-relevant) + +Prioritize: +- C++ memory safety and input validation +- Auth and access checks for state-changing HTTP/JSON APIs +- XSS and DOM safety in `wled00/data/*` +- Secrets handling (`wsec.json`) and secure logging +- Dependency and GitHub Actions supply-chain hygiene +- Fail-safe behavior on constrained devices + +De-prioritize unless explicitly introduced by a PR: +- SQL/NoSQL checks, JWT/OAuth flows, GraphQL-specific checks, generic backend framework checks not used by WLED. + +## Firmware Security (C++, OWASP A01/A04/A05/A10) + +### FW1: Unsafe buffer operations +- **Severity**: CRITICAL +- Flag `strcpy`, `sprintf`, unchecked memory access (`memcpy`, `memmove`, `memcmp`, `strcmp`, `strlen`), unchecked pointer arithmetic. +- Require explicit bounds checks and length validation. +- Prefer bounded alternatives for string operations (`strnlen`, `strncmp`, `strncpy`, `strlcpy`, `snprintf`). +- Treat a finding against FW1 as **suggestion** only when the operation is provably bounded + and both the destination capacity and copied/compared length are known safe. + and both the destination capacity and copied/compared length are known safe. + +### FW2: Format-string injection +- **Severity**: CRITICAL +- Do not pass untrusted input as a format string to `DEBUG_PRINTF*` or similar APIs. + +### FW3: Integer overflow in length and offset math +- **Severity**: IMPORTANT +- Review `count * size`, index math, narrowing casts before allocations or copies. + +### FW4: Unvalidated external input +- **Severity**: CRITICAL +- At each **untrusted ingress point** (see Trust Boundary Model above), validate and clamp values from HTTP/JSON/UDP/serial before use as lengths, indices, IDs, or pin references. +- Do not flag repeated range checks on values that have already been validated at their ingress point. +- In UDP handlers (`parsePacket()`, `read()`, and any lower-level socket wrappers), validate `packetSize` before buffer writes and clamp protocol-specific universe/channel ranges to valid limits. + +### FW5: Missing auth checks on state-changing endpoints (where auth is feasible) +- **Severity**: CRITICAL +- HTTP/JSON and other control paths that support auth must enforce configured auth policy. +- Do not flag standards-based UDP multicast/broadcast paths solely for lacking authentication when authentication is not defined in the protocol specification. + +### FW6: Fail-open behavior after parse or allocation errors +- **Severity**: IMPORTANT +- On error, reject update and preserve safe previous state. +- Explicitly check parse status (`DeserializationError error = deserializeJson(...); if (error) return/reject;`) and avoid silently applying unsafe zero/default values to safety-relevant fields (for example LED count and pin assignment). + +### FW7: Heap churn in hot paths +- **Severity**: IMPORTANT +- Avoid repeated dynamic allocation in render/effect loops; prefer pre-allocation and reuse. +- Flag allocation patterns in loop and ISR-adjacent paths that can trigger fragmentation or timing instability. + +### FW8: Unsafe use of `String` in performance-critical paths +- **Severity**: IMPORTANT +- In hot paths, avoid repeated `String` growth; reserve or use fixed buffers. +- Flag repeated `String` concatenation inside loop-heavy or ISR-adjacent code. + +### FW9: Unsafe feature flag names +- **Severity**: IMPORTANT +- Verify all new `WLED_ENABLE_*`/`WLED_DISABLE_*` names are valid known flags; typos silently alter build behavior. + +### FW10: OTA integrity verification (without TLS requirement) +- **Severity**: IMPORTANT +- OTA update flows should validate firmware integrity using the checksum/hash/signature mechanism available in the firmware/platform implementation. +- Do not require TLS/certificate pinning as a mandatory review criterion. +- In OTA paths (`Update.begin()`, `Update.write()`, and related flows), flag flashing without integrity verification. + +### FW11: FreeRTOS task stack and recursion safety +- **Severity**: IMPORTANT +- In `xTaskCreate`/`xTaskCreatePinnedToCore` tasks that process `String`/JSON-heavy data, verify stack-size sufficiency and avoid unbounded recursion. + +### FW12: mDNS and hostname sanitization +- **Severity**: IMPORTANT +- For `MDNS.begin()`, `MDNS.addService()`, and `ArduinoOTA.setHostname()`, ensure user-provided hostnames are RFC-compliant (letters/digits/hyphen, no leading/trailing hyphen) and clamped to 63 characters. + +### FW13: Outbound URL validation (no HTTPS requirement) +- **Severity**: SUGGESTION +- When using user-provided URL strings with `HTTPClient.begin()`/equivalent, validate scheme/format and constrain host targets (allowlist or equivalent policy). +- Do not require HTTPS/TLS as a baseline review rule. + +### FW14: Optional unicast UDP source filtering +- **Severity**: SUGGESTION +- For unicast UDP receive paths, prefer optional user-configurable source filtering. +- Do not require this for multicast/broadcast protocol flows. + +## Web UI Security (`wled00/data/*`, OWASP A01/A02/A05) + +### WEB1: DOM XSS through `innerHTML` +- **Severity**: CRITICAL +- Prefer `textContent`; if HTML is required, sanitize trusted content path explicitly. + +### WEB2: Dynamic code execution +- **Severity**: CRITICAL +- Reject `eval`, `new Function`, and string-based timer execution. + +### WEB3: `postMessage` without origin validation +- **Severity**: IMPORTANT +- Require strict origin allowlist checks before processing payloads. + +### WEB4: Unsafe redirects/navigation +- **Severity**: IMPORTANT +- Do not navigate directly from untrusted query/input without relative-path or allowlist checks. + +### WEB5: Client-only validation +- **Severity**: IMPORTANT +- UI validation is not sufficient; equivalent firmware-side validation is required. + +### WEB6: Direct DOM insertion from fetched/config data +- **Severity**: IMPORTANT +- Treat fetched and config-derived strings as untrusted unless proven otherwise. + +### WEB7: CSRF checks for state-changing HTTP routes (advisory) +- **Severity**: SUGGESTION +- For state-changing HTTP routes (for example `/json/state`, `/win`), prefer `Origin`/`Referer` header validation as low-cost defense-in-depth for deployments that are not directly internet-exposed. +- Treat this as advisory only, since some legitimate clients may omit these headers. + +## Secrets and Logging (OWASP A04/A09/A10) + +### SEC1: Hardcoded secrets and credentials +- **Severity**: CRITICAL +- Reject committed API keys, passwords, tokens, private keys, or test backdoors with potential security impact. + +### SEC2: Sensitive values in logs +- **Severity**: CRITICAL +- Do not log passwords, tokens, Wi-Fi keys, auth headers, or full sensitive payloads. + +### SEC3: Insecure defaults +- **Severity**: IMPORTANT +- Reject new default credentials or insecure auto-enable behavior for privileged functions. +- For setup/onboarding flows, require first-change behavior for default credentials where applicable. + +### SEC4: Overly detailed error responses +- **Severity**: IMPORTANT +- Avoid exposing stack traces or internal details to API/UI consumers. + +### SEC5: Credential exposure in API/config responses +- **Severity**: IMPORTANT +- Flag API/config serialization that exposes password-like fields (for example Wi-Fi/AP/MQTT passwords) to unauthenticated or untrusted clients. + +### SEC6: Security-relevant event logging coverage +- **Severity**: SUGGESTION +- Prefer explicit logging for auth failures, OTA attempts, config resets, and AP activation events, without logging secret values. + +## Supply Chain and CI/CD (OWASP A03/A08) + +### SC1: New dependency risk +- **Severity**: IMPORTANT +- Review new npm/pip/PlatformIO dependencies for legitimacy, pinning, and known vulnerabilities. + +### SC2: Workflow permission hardening regressions +- **Severity**: IMPORTANT +- Check for broad `permissions`, unpinned third-party actions, or unsafe secret exposure. +- Flag mutable third-party action refs (`@main`, `@master`, broad tags) where SHA pinning is expected by project policy. +- Flag overly broad permissions such as `write-all` without clear need. + +### SC3: Script injection in workflows +- **Severity**: IMPORTANT +- Avoid direct interpolation of untrusted `${{ github.event.* }}` values in `run` commands. + +## Reviewer Checklist + +- [ ] No new memory-safety hazards (bounds, overflow, unsafe copies/format strings) +- [ ] External input is validated and range-clamped at ingress points (HTTP/JSON, WebSocket, UDP, TCP, serial, ESP-NOW) +- [ ] State-changing API paths enforce auth policy +- [ ] OTA paths enforce integrity verification (without requiring TLS baseline) +- [ ] Suggested rule patterns are checked where relevant (UDP bounds, hostname sanitization, workflow pinning/permissions) +- [ ] Web UI changes avoid unsafe DOM execution/injection patterns +- [ ] No secrets added; no sensitive logging introduced +- [ ] Error handling remains fail-safe and non-leaky +- [ ] Dependency/workflow changes are supply-chain safe +- [ ] Feature-flag names are valid and not typoed + +## AI Review Behavior + +- Prefer concrete, file/line-specific findings over generic guidance. +- Prioritize **CRITICAL** and **IMPORTANT** findings. +- Skip irrelevant framework checks not used by WLED. +- If control-flow trust is unclear, ask for clarification instead of guessing.