WIP (experiment DO NOT MERGE) Add WLED secure coding AI review instruction guides#5572
WIP (experiment DO NOT MERGE) Add WLED secure coding AI review instruction guides#5572
Conversation
Agent-Logs-Url: https://github.com/wled/WLED/sessions/82b5d757-7791-4aa3-8f77-96b25e52b6d5 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
Agent-Logs-Url: https://github.com/wled/WLED/sessions/dc60e89a-6f5e-4441-8b4d-a70d61c1bf5c Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
softhack007
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds two WLED-focused security review instruction documents: a concise strict-mode checklist ( ChangesWLED security instruction documents
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
enhancement
discussion
AI
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai please look for similar secure coding guidelines/checklists in github projects that have similarities with the WLED architecture and build tooling. Additionally, look up the original OWASP "top 10".
|
This comment was marked as outdated.
This comment was marked as outdated.
|
@coderabbitai some expectations stated in #5572 (comment) are technically over-the-top (currently unfeasible due to very limited flash space). Also you can assume that WLED will be operated behind a firewall (DMZ style, wireguard, or VPN), which reduces some risks that system exposed to the "open internet" have to deal with. Technical constraints include:
Please revise your proposal accordingly. |
|
Tip For best results, initiate chat on the files or code changes.
Revised Prioritized Rule ProposalChanges from previous version (summary)
🔴 P1 — CRITICAL (revised)1. Default / Hardcoded Credentials [A07/A05]
🟠 P2 — IMPORTANT (revised)2. OTA Firmware Integrity [A08] — hash-only, no TLS requirement
3. Credential/Password Exposure via API Responses [A02]
4. UDP Input Length/Bounds Checking [A03] — no auth requirement
5. FreeRTOS Task Stack Overflows [A03 / embedded]
6. mDNS / Hostname Injection [A03]
🟡 P3 — SUGGESTIONS (revised)7. Outbound URL Validation (SSRF / misconfiguration) [A10] — no HTTPS requirement
8. CSRF via Origin Header [A01/A04]
9. Configurable UDP Source Filter (optional, not mandatory) [A01]
10–12. Existing rules — Clarifications (unchanged in priority)
Final Revised Priority Table
The three rules that were 🔴 P1 in the previous proposal — SSRF with HTTPS, OTA with TLS, and UDP authentication — have been either downgraded or stripped of the infeasible requirements, keeping only the parts that are actionable within WLED's flash and protocol constraints. |
|
@copilot please adjust this PR according to suggestions made in #5572 (comment) |
Agent-Logs-Url: https://github.com/wled/WLED/sessions/73d38e00-6610-47e7-a7de-4a926fe0cc68 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
Agent-Logs-Url: https://github.com/wled/WLED/sessions/73d38e00-6610-47e7-a7de-4a926fe0cc68 Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
Updated as requested in commit |
|
@copilot #5572 (comment) also suggests new rules and concrete patterns to be checked. Please verify each suggested rule for coverage in the existing instructions (docs/securecode.instructions.md) and add rules when they are not covered yet. |
Agent-Logs-Url: https://github.com/wled/WLED/sessions/e367885f-c511-44ca-8807-67d5e031d15c Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
Agent-Logs-Url: https://github.com/wled/WLED/sessions/e367885f-c511-44ca-8807-67d5e031d15c Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
Agent-Logs-Url: https://github.com/wled/WLED/sessions/e367885f-c511-44ca-8807-67d5e031d15c Co-authored-by: softhack007 <91616163+softhack007@users.noreply.github.com>
Done in |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
aligns with coderabbit suggestions
Clarified security review instructions and enhanced severity descriptions for hardcoded secrets and credential exposure.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12858
✅ All 13 Revised Rules — Now Fully Integrated
Previously Flagged Gaps — Both Now Resolved
One Remaining Minor Nit
This is cosmetic only and has no effect on review behavior. Summary: The integration is complete. All 13 rules from the revised proposal are present and correctly scoped in both files. The short checklist has been correctly extended to 26 rules with the header description updated to match. No substantive gaps remain. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/secure-short.instructions.md (1)
31-46: 💤 Low valueIMPORTANT rules are sound; consider splitting rule 25 for clarity.
The 14 IMPORTANT rules provide good coverage of secondary security concerns. Rule 24 correctly requires OTA integrity without TLS (matching constraints).
Optional refinement: Rule 25 (line 45) combines two distinct embedded concerns: FreeRTOS stack safety and mDNS/hostname sanitization. If the 26-rule compact format is later relaxed, splitting this into two rules would improve clarity and specificity.
🤖 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/secure-short.instructions.md` around lines 31 - 46, Split the combined Rule 25 into two separate rules for clarity: one rule that flags xTaskCreate and xTaskCreatePinnedToCore calls that allocate insufficient stack for use of String/JSON (explicitly mention checking stack size against worst-case String/JSON usage), and a second rule that flags MDNS.begin() and ArduinoOTA.setHostname() calls with unsanitized hostnames (explicitly require hostname validation/sanitization and allowed-character checks); update references in the list so the new rules replace the single Rule 25 entry and keep numbering consistent.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@docs/secure-short.instructions.md`:
- Around line 31-46: Split the combined Rule 25 into two separate rules for
clarity: one rule that flags xTaskCreate and xTaskCreatePinnedToCore calls that
allocate insufficient stack for use of String/JSON (explicitly mention checking
stack size against worst-case String/JSON usage), and a second rule that flags
MDNS.begin() and ArduinoOTA.setHostname() calls with unsanitized hostnames
(explicitly require hostname validation/sanitization and allowed-character
checks); update references in the list so the new rules replace the single Rule
25 entry and keep numbering consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 83d69bdb-b4d9-4ec2-b333-37539ff5905b
📒 Files selected for processing (1)
docs/secure-short.instructions.md
| - 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. | ||
|
|
There was a problem hiding this comment.
@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. | ||
|
|
There was a problem hiding this comment.
possible addition:
- check that functions do not store "large data" (>2kB) on stack.
- verify that recursions are bounded
plus minor fixes
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/securecode.instructions.md (1)
179-179: 💤 Low valueMinor: Correct GitHub capitalization.
The official platform name is "GitHub" (capital H).
Suggested fix
-- Avoid direct interpolation of untrusted `${{ github.event.* }}` values in `run` commands. +- Avoid direct interpolation of untrusted `${{ GitHub.event.* }}` values in `run` commands.Note: If the intent is to reference the literal GitHub Actions expression syntax
github.event.*, the lowercase form is technically correct for the expression context itself. In that case, you could clarify:-- Avoid direct interpolation of untrusted `${{ github.event.* }}` values in `run` commands. +- Avoid direct interpolation of untrusted GitHub Actions context values (e.g., `${{ github.event.* }}`) in `run` commands.🤖 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/securecode.instructions.md` at line 179, Update the sentence that currently reads "Avoid direct interpolation of untrusted `${{ github.event.* }}` values in `run` commands." to use the correct platform capitalization "GitHub" (i.e., "Avoid direct interpolation of untrusted `${{ github.event.* }}` values in GitHub Actions `run` commands."), while preserving the literal expression `github.event.*` in lowercase if you intend to show the expression syntax.docs/hardening.instructions.md (2)
27-27: 💤 Low valueClarify scope of Rule 10 with improved structure.
Rule 10 combines two distinct concerns—workflow/log output and LittleFS file storage—in a single sentence. The "or" construction makes it slightly ambiguous whether both clauses apply to "secret exposure" or if they're alternatives.
Suggested rewording for clarity
-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** (except `wsec.json`).Or split into two sub-points if the distinction is important:
-10. **No secret exposure in workflows/log output, or in LittleFS files other than `wsec.json`**. +10. **No secret exposure**: reject secrets in workflow/log output and in LittleFS files other than `wsec.json`.🤖 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` at line 27, The Rule 10 sentence is ambiguous because it joins workflow/log output and LittleFS file storage with "or"; update the text for clarity by either (a) rephrasing the single sentence to explicitly state both must not contain secrets (e.g., "No secret exposure in workflows or log output, and no secret storage in LittleFS files except wsec.json"), or (b) split into two sub-points under Rule 10—one forbidding secrets in workflows/log output and a second forbidding secrets in LittleFS files other than `wsec.json`—so the scope of each restriction is unambiguous.
2-2: 💤 Low valueConsider removing CSS from the applyTo glob.
The applyTo glob includes
cssfiles, but none of the 26 hardening rules are meaningfully applicable to CSS. CSS files don't contain buffer operations, format strings, auth logic, or other rule targets. Consider removingcssfrom the glob to avoid unnecessary processing.Suggested adjustment
-applyTo: "**/*.{cpp,h,hpp,ino,js,htm,html,css,yml,yaml}" +applyTo: "**/*.{cpp,h,hpp,ino,js,htm,html,yml,yaml}"🤖 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` at line 2, The applyTo glob currently includes CSS via the line applyTo: "**/*.{cpp,h,hpp,ino,js,htm,html,css,yml,yaml}"; remove "css" from that brace list so it reads applyTo: "**/*.{cpp,h,hpp,ino,js,htm,html,yml,yaml}" (or equivalent) to avoid running the 26 hardening rules against CSS files; if you later need CSS-specific checks, create a separate rule set or glob for CSS instead.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@docs/hardening.instructions.md`:
- Line 27: The Rule 10 sentence is ambiguous because it joins workflow/log
output and LittleFS file storage with "or"; update the text for clarity by
either (a) rephrasing the single sentence to explicitly state both must not
contain secrets (e.g., "No secret exposure in workflows or log output, and no
secret storage in LittleFS files except wsec.json"), or (b) split into two
sub-points under Rule 10—one forbidding secrets in workflows/log output and a
second forbidding secrets in LittleFS files other than `wsec.json`—so the scope
of each restriction is unambiguous.
- Line 2: The applyTo glob currently includes CSS via the line applyTo:
"**/*.{cpp,h,hpp,ino,js,htm,html,css,yml,yaml}"; remove "css" from that brace
list so it reads applyTo: "**/*.{cpp,h,hpp,ino,js,htm,html,yml,yaml}" (or
equivalent) to avoid running the 26 hardening rules against CSS files; if you
later need CSS-specific checks, create a separate rule set or glob for CSS
instead.
In `@docs/securecode.instructions.md`:
- Line 179: Update the sentence that currently reads "Avoid direct interpolation
of untrusted `${{ github.event.* }}` values in `run` commands." to use the
correct platform capitalization "GitHub" (i.e., "Avoid direct interpolation of
untrusted `${{ github.event.* }}` values in GitHub Actions `run` commands."),
while preserving the literal expression `github.event.*` in lowercase if you
intend to show the expression syntax.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9d476561-594e-456a-824d-0070923f2bc7
📒 Files selected for processing (2)
docs/hardening.instructions.mddocs/securecode.instructions.md
| - **Severity**: CRITICAL | ||
| - Flag `strcpy`, `sprintf`, unchecked `memcpy`, unchecked pointer arithmetic. | ||
| - Require explicit bounds checks and length validation. | ||
|
|
There was a problem hiding this comment.
plus
- before copying (memcpy, memmove) or comparing (memcmp()) external input data, perform a length check to avoid reading past end-of-input.
|
|
||
| ### FW1: Unsafe buffer operations | ||
| - **Severity**: CRITICAL | ||
| - Flag `strcpy`, `sprintf`, unchecked `memcpy`, unchecked pointer arithmetic. |
There was a problem hiding this comment.
include memmove(), memcmp(), strcmp(), strlen().
prefer strnlen(), strlcmp().
|
|
||
| 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. | ||
|
|
There was a problem hiding this comment.
memory safety: avoid suggesting redundant checks that create unnecessary complexity; verify if a buffer was already length-cheched / sanitized by the calling function.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@docs/hardening.instructions.md`:
- Around line 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b017687c-a0e9-4f2f-9323-1889897f0bcb
📒 Files selected for processing (2)
docs/hardening.instructions.mddocs/securecode.instructions.md
| ## 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. 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. | ||
|
|
There was a problem hiding this comment.
🛠️ 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.
this PR is an experiment for creating a security-aware review checklist (for coderabbit PR reviews) that should be relevant for WLED.
The list is based on the OWASP "top 10" from https://github.com/github/awesome-copilot/blob/main/instructions/security-and-owasp.instructions.md
DO NOT MERGE yet!!
This PR now includes:
docs/*.instructions.mdnaming pattern:docs/securecode.instructions.mddocs/secure-short.instructions.mddocs/securecode.instructions.md, including concrete pattern guidance for:Update.*pathsOrigin/Refereradvisory checks for non-internet-exposed deploymentsSummary by CodeRabbit
Summary by CodeRabbit
Release Notes