Skip to content

WIP (experiment DO NOT MERGE) Add WLED secure coding AI review instruction guides #5572

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Copilot wants to merge 11 commits into
base: main
Choose a base branch
from
+251 −0
Draft

WIP (experiment DO NOT MERGE) Add WLED secure coding AI review instruction guides #5572

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
db7e419
Add WLED secure review instruction docs
Copilot May 6, 2026
9f0e0cd
Fix docs file naming to instructions pattern
Copilot May 6, 2026
466cff6
Revise security docs for WLED feasibility constraints
Copilot May 7, 2026
9351b1b
Clarify security doc wording for protocol/auth and OTA integrity
Copilot May 7, 2026
eb68806
Add missing concrete security rule coverage in securecode guide
Copilot May 7, 2026
46e2648
Clarify hostname and CSRF header wording in securecode guide
Copilot May 7, 2026
5abba3e
Refine securecode concrete patterns for UDP parse and CSRF advisory
Copilot May 7, 2026
e468f41
Update security review checklist to 26 rules
softhack007 May 7, 2026
2fd1c1e
Update security review standards and logging guidelines
softhack007 May 7, 2026
4363121
Fix formatting of secret exposure guideline
softhack007 May 7, 2026
bfdc594
rename "short instructions" to "hardening instructions"
softhack007 May 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions docs/hardening.instructions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
---
applyTo: "**/*.{cpp,h,hpp,ino,js,htm,html,css,yml,yaml}"
description: "WLED strict-mode security review: low-noise 26-rule checklist."
---

# WLED Security Review — Strict Mode (Low Noise)

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.

## CRITICAL Rules

1. **No unchecked buffer copies** (`memcpy`, `strcpy`, `sprintf`) in firmware paths.
Copy link
Copy Markdown
Member

@softhack007 softhack007 May 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include memmove(), memcmp(), strcmp(), strlen().
prefer strnlen(), strlcmp().

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 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).
Copy link
Copy Markdown
Member

@softhack007 softhack007 May 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... involving "untrusted" user inputs,

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. 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.
Copy link
Copy Markdown
Member

@softhack007 softhack007 May 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... not the legacy design policy, but indeed should be done where possible

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. 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.

Comment on lines +31 to +47
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win

Consider adding WEB6 (Direct DOM insertion) to the IMPORTANT rules.

The companion document docs/securecode.instructions.md defines WEB6 (lines 129-132) as an IMPORTANT-severity rule: "Direct DOM insertion from fetched/config data." This rule addresses the risk of treating fetched or config-derived strings as trusted when inserting into the DOM.

Since this strict-mode checklist includes other IMPORTANT-severity items (rules 13-26), omitting WEB6 appears inconsistent with the stated goal of covering all critical and important risks for low-noise automated reviews.

📋 Suggested addition

Add a new rule 27 (or renumber accordingly):

 26. Flag API/config serialization that exposes Wi-Fi/AP/MQTT password fields to unauthenticated clients.
+27. Treat fetched and config-derived strings as untrusted when inserting into the DOM; explicit sanitization required for HTML contexts.

Update line 3 and line 50 to reflect 27 total rules.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/hardening.instructions.md` around lines 31 - 47, The IMPORTANT rules
list omits the WEB6 (Direct DOM insertion) important-severity rule defined in
docs/securecode.instructions.md; add WEB6 as a new rule entry (e.g., rule 27)
with the description "Direct DOM insertion from fetched/config data" and
renumber or update the total-rules count/header references accordingly so the
checklist and securecode document are consistent; reference the WEB6 rule
identifier when editing the rules block to ensure the same wording from
securecode.instructions.md is used.

## Reviewer Output Format

- Report only findings mapped to rules 1–26.
Copy link
Copy Markdown
Member

@softhack007 softhack007 May 11, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: rephrase this rule. As it stands, this rule would suppress any other "random" security findings that might pop up during regular reviews -> not what we want to achieve.

- Include severity, exact file and line, and one concrete fix direction.
Copy link
Copy Markdown
Member

@softhack007 softhack007 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normal work? Not sure if this rule adds value.

- Prioritize CRITICAL findings before IMPORTANT findings.
199 changes: 199 additions & 0 deletions docs/securecode.instructions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,199 @@
---
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.

## 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
Copy link
Copy Markdown
Member

@softhack007 softhack007 May 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input validation => potentially untrusted input from network API's or user interface inputs.

- 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.

Copy link
Copy Markdown
Member

@softhack007 softhack007 May 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Memory safety: verify whether the calling function has already performed sufficient length checks/sanitisation on untrusted inputs; don't suggest checks that are clearly redundant.

## Firmware Security (C++, OWASP A01/A04/A05/A10)

### FW1: Unsafe buffer operations
- **Severity**: CRITICAL
- Flag `strcpy`, `sprintf`, unchecked `memcpy`, unchecked pointer arithmetic.
Copy link
Copy Markdown
Member

@softhack007 softhack007 May 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include memmove(), memcmp(), strcmp(), strlen().
prefer strnlen(), strlcmp(). Maybe strchr(), too.

- Require explicit bounds checks and length validation.

Copy link
Copy Markdown
Member

@softhack007 softhack007 May 10, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plus

  • before copying (memcpy, memmove) or comparing (memcmp()) external input data, perform a length check to avoid reading past end-of-input.

### 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.
- 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.

Copy link
Copy Markdown
Member

@softhack007 softhack007 May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@willmmiles do you think this point is feasible?
(most of the PR is wishful thinking any way 🎅 )

### 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.

Copy link
Copy Markdown
Member

@softhack007 softhack007 May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possible addition:

  • check that functions do not store "large data" (>2kB) on stack.
  • verify that recursions are bounded

### 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 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
- [ ] 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.
Loading