From db7e419c9c30e185723fca790c802f16625c757e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 May 2026 23:10:12 +0000 Subject: [PATCH 01/11] Add WLED secure review instruction docs Agent-Logs-Url: https://github.com/wled/WLED/sessions/82b5d757-7791-4aa3-8f77-96b25e52b6d5 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com> --- docs/secure-short.instrustions.md | 44 +++++++++ docs/securecode.instrustions.md | 147 ++++++++++++++++++++++++++++++ 2 files changed, 191 insertions(+) create mode 100644 docs/secure-short.instrustions.md create mode 100644 docs/securecode.instrustions.md diff --git a/docs/secure-short.instrustions.md b/docs/secure-short.instrustions.md new file mode 100644 index 0000000000..41932194bf --- /dev/null +++ b/docs/secure-short.instrustions.md @@ -0,0 +1,44 @@ +--- +applyTo: "**/*.{cpp,h,hpp,ino,js,htm,html,css,yml,yaml}" +description: "WLED strict-mode security review: low-noise 24-rule checklist." +--- + +# WLED Security Review — Strict Mode (Low Noise) + +Use these 24 rules for automated reviews with minimal false positives. + +## CRITICAL Rules + +1. **No unchecked buffer copies** (`memcpy`, `strcpy`, `sprintf`) in firmware paths. +2. **No user-controlled format strings** in `DEBUG_PRINTF*` and similar logging APIs. +3. **Validate all external input** (HTTP/JSON/UDP/serial) before index/length/pin usage. +4. **Auth required for every state-changing endpoint**. +5. **No fail-open on parse/allocation errors** for config/state updates. +6. **No DOM XSS sinks with untrusted data** (`innerHTML`, unsafe HTML insertion). +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**. +11. **No unsafe third-party GitHub Action pinning** (`@main`/`@master` disallowed). +12. **No untrusted expression interpolation in workflow shell commands**. + +## IMPORTANT Rules + +13. Check integer overflow risks in size/index arithmetic. +14. Reject repeated heap allocation churn in hot render/effect loops. +15. Avoid repeated `String` growth in hot paths; prefer bounded/pre-allocated buffers. +16. Ensure UI validation is mirrored by firmware-side validation. +17. Require strict origin checks for `postMessage` listeners. +18. Disallow untrusted redirect/navigation targets. +19. Prevent verbose error responses that leak internals. +20. Review new dependencies for typosquatting and known vulnerability risk. +21. Keep workflow `permissions` least-privilege. +22. Verify new `WLED_ENABLE_*` / `WLED_DISABLE_*` names are valid known flags. +23. Ensure new privileged behavior is not enabled by insecure defaults. +24. Preserve safe behavior under malformed inputs and low-memory conditions. + +## Reviewer Output Format + +- Report only findings mapped to rules 1–24. +- Include severity, exact file and line, and one concrete fix direction. +- Prioritize CRITICAL findings before IMPORTANT findings. diff --git a/docs/securecode.instrustions.md b/docs/securecode.instrustions.md new file mode 100644 index 0000000000..d4c9e2b9fc --- /dev/null +++ b/docs/securecode.instrustions.md @@ -0,0 +1,147 @@ +--- +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: +- `/home/runner/work/WLED/WLED/wled00/` +- `/home/runner/work/WLED/WLED/usermods/` +- `/home/runner/work/WLED/WLED/.github/workflows/` + +Ignore sections wrapped in ` ... ` in repo docs when applying review criteria. + +## 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 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 `memcpy`, unchecked pointer arithmetic. +- Require explicit bounds checks and length validation. + +### 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 +- Validate and clamp external values from HTTP/JSON/UDP/serial before use as lengths, indices, IDs, or pin references. + +### FW5: Missing auth checks on state-changing endpoints +- **Severity**: CRITICAL +- Any endpoint that changes device state/config must enforce configured auth policy. + +### FW6: Fail-open behavior after parse or allocation errors +- **Severity**: IMPORTANT +- On error, reject update and preserve safe previous state. + +### FW7: Heap churn in hot paths +- **Severity**: IMPORTANT +- Avoid repeated dynamic allocation in render/effect loops; prefer pre-allocation and reuse. + +### FW8: Unsafe use of `String` in performance-critical paths +- **Severity**: IMPORTANT +- In hot paths, avoid repeated `String` growth; reserve or use fixed buffers. + +### FW9: Unsafe feature flag names +- **Severity**: IMPORTANT +- Verify all new `WLED_ENABLE_*`/`WLED_DISABLE_*` names are valid known flags; typos silently alter build behavior. + +## 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. + +## 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. + +### 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. + +### SEC4: Overly detailed error responses +- **Severity**: IMPORTANT +- Avoid exposing stack traces or internal details to API/UI consumers. + +## 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. + +### 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 before use +- [ ] State-changing API paths enforce auth policy +- [ ] 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. From 9f0e0cd70c5c3f2bc792fd5ac8725c4026f320c3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 6 May 2026 23:12:40 +0000 Subject: [PATCH 02/11] Fix docs file naming to instructions pattern Agent-Logs-Url: https://github.com/wled/WLED/sessions/dc60e89a-6f5e-4441-8b4d-a70d61c1bf5c Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com> --- ...{secure-short.instrustions.md => secure-short.instructions.md} | 0 docs/{securecode.instrustions.md => securecode.instructions.md} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename docs/{secure-short.instrustions.md => secure-short.instructions.md} (100%) rename docs/{securecode.instrustions.md => securecode.instructions.md} (100%) diff --git a/docs/secure-short.instrustions.md b/docs/secure-short.instructions.md similarity index 100% rename from docs/secure-short.instrustions.md rename to docs/secure-short.instructions.md diff --git a/docs/securecode.instrustions.md b/docs/securecode.instructions.md similarity index 100% rename from docs/securecode.instrustions.md rename to docs/securecode.instructions.md From 466cff692cefbfa0fd162380ebfed9034be1956e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 7 May 2026 00:05:10 +0000 Subject: [PATCH 03/11] Revise security docs for WLED feasibility constraints Agent-Logs-Url: https://github.com/wled/WLED/sessions/73d38e00-6610-47e7-a7de-4a926fe0cc68 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com> --- docs/secure-short.instructions.md | 10 ++++++++-- docs/securecode.instructions.md | 17 +++++++++++++++-- 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/docs/secure-short.instructions.md b/docs/secure-short.instructions.md index 41932194bf..e117a91a24 100644 --- a/docs/secure-short.instructions.md +++ b/docs/secure-short.instructions.md @@ -7,12 +7,18 @@ description: "WLED strict-mode security review: low-noise 24-rule checklist." Use these 24 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 channels where protocol specs do not define auth. + ## CRITICAL Rules 1. **No unchecked buffer copies** (`memcpy`, `strcpy`, `sprintf`) in firmware paths. 2. **No user-controlled format strings** in `DEBUG_PRINTF*` and similar logging APIs. 3. **Validate all external input** (HTTP/JSON/UDP/serial) before index/length/pin usage. -4. **Auth required for every state-changing endpoint**. +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). 7. **No dynamic code execution** (`eval`, `new Function`, string timers). @@ -35,7 +41,7 @@ Use these 24 rules for automated reviews with minimal false positives. 21. Keep workflow `permissions` least-privilege. 22. Verify new `WLED_ENABLE_*` / `WLED_DISABLE_*` names are valid known flags. 23. Ensure new privileged behavior is not enabled by insecure defaults. -24. Preserve safe behavior under malformed inputs and low-memory conditions. +24. Preserve safe behavior under malformed inputs and low-memory conditions; OTA update paths should verify firmware integrity (checksum/hash), without requiring TLS. ## Reviewer Output Format diff --git a/docs/securecode.instructions.md b/docs/securecode.instructions.md index d4c9e2b9fc..24b8ea532e 100644 --- a/docs/securecode.instructions.md +++ b/docs/securecode.instructions.md @@ -12,6 +12,13 @@ Use this guide for AI-assisted code reviews in: Ignore sections wrapped in ` ... ` in repo docs when applying review criteria. +## 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. + ## Severity - **CRITICAL** — exploitable vulnerability; block merge. @@ -50,9 +57,10 @@ De-prioritize unless explicitly introduced by a PR: - **Severity**: CRITICAL - Validate and clamp external values from HTTP/JSON/UDP/serial before use as lengths, indices, IDs, or pin references. -### FW5: Missing auth checks on state-changing endpoints +### FW5: Missing auth checks on state-changing endpoints (where auth is feasible) - **Severity**: CRITICAL -- Any endpoint that changes device state/config must enforce configured auth policy. +- 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 protocol specs do not provide it. ### FW6: Fail-open behavior after parse or allocation errors - **Severity**: IMPORTANT @@ -70,6 +78,11 @@ De-prioritize unless explicitly introduced by a PR: - **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 (checksum/hash/signature mechanism used by the implementation). +- Do not require TLS/certificate pinning as a mandatory review criterion. + ## Web UI Security (`wled00/data/*`, OWASP A01/A02/A05) ### WEB1: DOM XSS through `innerHTML` From 9351b1b28afd07eb72dadc93fa6611887c45bdea Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 7 May 2026 00:05:41 +0000 Subject: [PATCH 04/11] Clarify security doc wording for protocol/auth and OTA integrity Agent-Logs-Url: https://github.com/wled/WLED/sessions/73d38e00-6610-47e7-a7de-4a926fe0cc68 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com> --- docs/secure-short.instructions.md | 2 +- docs/securecode.instructions.md | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/secure-short.instructions.md b/docs/secure-short.instructions.md index e117a91a24..99bebc470f 100644 --- a/docs/secure-short.instructions.md +++ b/docs/secure-short.instructions.md @@ -11,7 +11,7 @@ Use these 24 rules for automated reviews with minimal false positives. - 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 channels where protocol specs do not define auth. +- Do **not** require authentication for standards-based UDP multicast/broadcast paths where authentication is not defined in the protocol specification. ## CRITICAL Rules diff --git a/docs/securecode.instructions.md b/docs/securecode.instructions.md index 24b8ea532e..b392210232 100644 --- a/docs/securecode.instructions.md +++ b/docs/securecode.instructions.md @@ -60,7 +60,7 @@ De-prioritize unless explicitly introduced by a PR: ### 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 protocol specs do not provide it. +- 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 @@ -80,7 +80,7 @@ De-prioritize unless explicitly introduced by a PR: ### FW10: OTA integrity verification (without TLS requirement) - **Severity**: IMPORTANT -- OTA update flows should validate firmware integrity (checksum/hash/signature mechanism used by the implementation). +- 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. ## Web UI Security (`wled00/data/*`, OWASP A01/A02/A05) From eb68806f19351785dcbae1627d1d226eb5516245 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 7 May 2026 00:13:43 +0000 Subject: [PATCH 05/11] Add missing concrete security rule coverage in securecode guide Agent-Logs-Url: https://github.com/wled/WLED/sessions/e367885f-c511-44ca-8807-67d5e031d15c Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com> --- docs/securecode.instructions.md | 40 +++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/docs/securecode.instructions.md b/docs/securecode.instructions.md index b392210232..b12fdc9a34 100644 --- a/docs/securecode.instructions.md +++ b/docs/securecode.instructions.md @@ -56,6 +56,7 @@ De-prioritize unless explicitly introduced by a PR: ### FW4: Unvalidated external input - **Severity**: CRITICAL - Validate and clamp external values from HTTP/JSON/UDP/serial before use as lengths, indices, IDs, or pin references. +- In UDP handlers (`parsePacket()`, `recvfrom()`), 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 @@ -65,14 +66,17 @@ De-prioritize unless explicitly introduced by a PR: ### FW6: Fail-open behavior after parse or allocation errors - **Severity**: IMPORTANT - On error, reject update and preserve safe previous state. +- Explicitly check parse status (`deserializeJson()`/equivalent) 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 @@ -82,6 +86,25 @@ De-prioritize unless explicitly introduced by a PR: - **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 restricted to RFC-compliant characters (`[a-zA-Z0-9-]`) 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) @@ -109,6 +132,10 @@ De-prioritize unless explicitly introduced by a PR: - **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` validation as low-cost defense-in-depth in firewall-isolated deployments. + ## Secrets and Logging (OWASP A04/A09/A10) ### SEC1: Hardcoded secrets and credentials @@ -122,11 +149,20 @@ De-prioritize unless explicitly introduced by a PR: ### 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 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 @@ -136,6 +172,8 @@ De-prioritize unless explicitly introduced by a PR: ### 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 @@ -146,6 +184,8 @@ De-prioritize unless explicitly introduced by a PR: - [ ] No new memory-safety hazards (bounds, overflow, unsafe copies/format strings) - [ ] External input is validated and range-clamped before use - [ ] 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 From 46e2648c93d71acce50b35fb27fd20f3e06df326 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 7 May 2026 00:14:19 +0000 Subject: [PATCH 06/11] Clarify hostname and CSRF header wording in securecode guide Agent-Logs-Url: https://github.com/wled/WLED/sessions/e367885f-c511-44ca-8807-67d5e031d15c Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com> --- docs/securecode.instructions.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/securecode.instructions.md b/docs/securecode.instructions.md index b12fdc9a34..0ef11d78d7 100644 --- a/docs/securecode.instructions.md +++ b/docs/securecode.instructions.md @@ -94,7 +94,7 @@ De-prioritize unless explicitly introduced by a PR: ### FW12: mDNS and hostname sanitization - **Severity**: IMPORTANT -- For `MDNS.begin()`, `MDNS.addService()`, and `ArduinoOTA.setHostname()`, ensure user-provided hostnames are restricted to RFC-compliant characters (`[a-zA-Z0-9-]`) and clamped to 63 characters. +- 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 @@ -134,7 +134,7 @@ De-prioritize unless explicitly introduced by a PR: ### WEB7: CSRF checks for state-changing HTTP routes (advisory) - **Severity**: SUGGESTION -- For state-changing HTTP routes (for example `/json/state`, `/win`), prefer `Origin`/`Referer` validation as low-cost defense-in-depth in firewall-isolated deployments. +- For state-changing HTTP routes (for example `/json/state`, `/win`), prefer `Origin`/`Referer` header validation as low-cost defense-in-depth in firewall-isolated deployments. ## Secrets and Logging (OWASP A04/A09/A10) From 5abba3ec1eefa9caa10eed3299b3b14b31766c80 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 7 May 2026 00:14:56 +0000 Subject: [PATCH 07/11] Refine securecode concrete patterns for UDP parse and CSRF advisory Agent-Logs-Url: https://github.com/wled/WLED/sessions/e367885f-c511-44ca-8807-67d5e031d15c Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com> --- docs/securecode.instructions.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/securecode.instructions.md b/docs/securecode.instructions.md index 0ef11d78d7..0e7c474aba 100644 --- a/docs/securecode.instructions.md +++ b/docs/securecode.instructions.md @@ -56,7 +56,7 @@ De-prioritize unless explicitly introduced by a PR: ### FW4: Unvalidated external input - **Severity**: CRITICAL - Validate and clamp external values from HTTP/JSON/UDP/serial before use as lengths, indices, IDs, or pin references. -- In UDP handlers (`parsePacket()`, `recvfrom()`), validate `packetSize` before buffer writes and clamp protocol-specific universe/channel ranges to valid limits. +- 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 @@ -66,7 +66,7 @@ De-prioritize unless explicitly introduced by a PR: ### FW6: Fail-open behavior after parse or allocation errors - **Severity**: IMPORTANT - On error, reject update and preserve safe previous state. -- Explicitly check parse status (`deserializeJson()`/equivalent) and avoid silently applying unsafe zero/default values to safety-relevant fields (for example LED count and pin assignment). +- 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 @@ -134,7 +134,8 @@ De-prioritize unless explicitly introduced by a PR: ### 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 in firewall-isolated deployments. +- 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) From e468f411e0af231f8325edc69e6fd1ca8af6692b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frank=20M=C3=B6hle?= <91616163+softhack007@users.noreply.github.com> Date: Thu, 7 May 2026 02:32:18 +0200 Subject: [PATCH 08/11] Update security review checklist to 26 rules aligns with coderabbit suggestions --- docs/secure-short.instructions.md | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/docs/secure-short.instructions.md b/docs/secure-short.instructions.md index 99bebc470f..1179c00e9c 100644 --- a/docs/secure-short.instructions.md +++ b/docs/secure-short.instructions.md @@ -1,11 +1,11 @@ --- applyTo: "**/*.{cpp,h,hpp,ino,js,htm,html,css,yml,yaml}" -description: "WLED strict-mode security review: low-noise 24-rule checklist." +description: "WLED strict-mode security review: low-noise 26-rule checklist." --- # WLED Security Review — Strict Mode (Low Noise) -Use these 24 rules for automated reviews with minimal false positives. +Use these compact rules for automated reviews with minimal false positives. ## WLED Constraints (apply to all rules) @@ -24,7 +24,7 @@ Use these 24 rules for automated reviews with minimal false positives. 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**. +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**. @@ -40,11 +40,13 @@ Use these 24 rules for automated reviews with minimal false positives. 20. Review new dependencies for typosquatting and known vulnerability risk. 21. Keep workflow `permissions` least-privilege. 22. Verify new `WLED_ENABLE_*` / `WLED_DISABLE_*` names are valid known flags. -23. Ensure new privileged behavior is not enabled by insecure defaults. -24. Preserve safe behavior under malformed inputs and low-memory conditions; OTA update paths should verify firmware integrity (checksum/hash), without requiring TLS. +23. New privileged behavior must not be enabled by insecure defaults; first-use default-credential change required where applicable. +24. OTA paths (Update.begin(), Update.write()) must verify firmware integrity (checksum/hash); TLS not required. +25. Flag xTaskCreate/xTaskCreatePinnedToCore tasks with insufficient stack for String/JSON use; flag MDNS.begin() / ArduinoOTA.setHostname() with unsanitized hostnames. +26. Flag API/config serialization that exposes Wi-Fi/AP/MQTT password fields to unauthenticated clients. ## Reviewer Output Format -- Report only findings mapped to rules 1–24. +- Report only findings mapped to rules 1–26. - Include severity, exact file and line, and one concrete fix direction. - Prioritize CRITICAL findings before IMPORTANT findings. From 2fd1c1e91c8a1bbe90824579331554d9be0462d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frank=20M=C3=B6hle?= <91616163+softhack007@users.noreply.github.com> Date: Thu, 7 May 2026 02:39:16 +0200 Subject: [PATCH 09/11] Update security review standards and logging guidelines Clarified security review instructions and enhanced severity descriptions for hardcoded secrets and credential exposure. --- docs/securecode.instructions.md | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/docs/securecode.instructions.md b/docs/securecode.instructions.md index 0e7c474aba..5749be2b8d 100644 --- a/docs/securecode.instructions.md +++ b/docs/securecode.instructions.md @@ -6,9 +6,9 @@ description: "WLED-focused security review guide based on OWASP Top 10 for embed # WLED Security Review Standards (Embedded + Web UI) Use this guide for AI-assisted code reviews in: -- `/home/runner/work/WLED/WLED/wled00/` -- `/home/runner/work/WLED/WLED/usermods/` -- `/home/runner/work/WLED/WLED/.github/workflows/` +- `wled00/` +- `usermods/` +- `.github/workflows/` Ignore sections wrapped in ` ... ` in repo docs when applying review criteria. @@ -31,7 +31,7 @@ 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 and secure logging +- Secrets handling (`wsec.json`) and secure logging - Dependency and GitHub Actions supply-chain hygiene - Fail-safe behavior on constrained devices @@ -141,7 +141,7 @@ De-prioritize unless explicitly introduced by a PR: ### SEC1: Hardcoded secrets and credentials - **Severity**: CRITICAL -- Reject committed API keys, passwords, tokens, private keys, or test backdoors. +- Reject committed API keys, passwords, tokens, private keys, or test backdoors with potential security impact. ### SEC2: Sensitive values in logs - **Severity**: CRITICAL @@ -158,7 +158,7 @@ De-prioritize unless explicitly introduced by a PR: ### 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 clients. +- 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 From 43631214925c9772aeb00800de248fcb8ed1fe57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Frank=20M=C3=B6hle?= <91616163+softhack007@users.noreply.github.com> Date: Thu, 7 May 2026 02:42:57 +0200 Subject: [PATCH 10/11] Fix formatting of secret exposure guideline --- docs/secure-short.instructions.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/secure-short.instructions.md b/docs/secure-short.instructions.md index 1179c00e9c..e9c90fa14e 100644 --- a/docs/secure-short.instructions.md +++ b/docs/secure-short.instructions.md @@ -24,7 +24,7 @@ Use these compact rules for automated reviews with minimal false positives. 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``**. +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**. From bfdc59425cc253bc706ebe45a27d38daecbb46e3 Mon Sep 17 00:00:00 2001 From: Frank <91616163+softhack007@users.noreply.github.com> Date: Thu, 7 May 2026 15:38:22 +0200 Subject: [PATCH 11/11] rename "short instructions" to "hardening instructions" plus minor fixes --- .../{secure-short.instructions.md => hardening.instructions.md} | 2 +- docs/securecode.instructions.md | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) rename docs/{secure-short.instructions.md => hardening.instructions.md} (97%) diff --git a/docs/secure-short.instructions.md b/docs/hardening.instructions.md similarity index 97% rename from docs/secure-short.instructions.md rename to docs/hardening.instructions.md index e9c90fa14e..45d82566dc 100644 --- a/docs/secure-short.instructions.md +++ b/docs/hardening.instructions.md @@ -5,7 +5,7 @@ description: "WLED strict-mode security review: low-noise 26-rule checklist." # WLED Security Review — Strict Mode (Low Noise) -Use these compact rules for automated reviews with minimal false positives. +Use these code hardening rules for automated reviews with minimal false positives. ## WLED Constraints (apply to all rules) diff --git a/docs/securecode.instructions.md b/docs/securecode.instructions.md index 5749be2b8d..e6d0522520 100644 --- a/docs/securecode.instructions.md +++ b/docs/securecode.instructions.md @@ -10,8 +10,6 @@ Use this guide for AI-assisted code reviews in: - `usermods/` - `.github/workflows/` -Ignore sections wrapped in ` ... ` in repo docs when applying review criteria. - ## WLED Constraints and Threat Model Assumptions - Assume typical deployment behind a firewall/DMZ/VPN; prioritize LAN-local and supply-chain risks.