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