-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
add secure coding AI review 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.
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
Open
Copilot
wants to merge
25
commits into
main
Choose a base branch
from
copilot/create-ai-review-guide-wled
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+414
−8
Open
Changes from 2 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
db7e419
Add WLED secure review instruction docs
Copilot 9f0e0cd
Fix docs file naming to instructions pattern
Copilot 466cff6
Revise security docs for WLED feasibility constraints
Copilot 9351b1b
Clarify security doc wording for protocol/auth and OTA integrity
Copilot eb68806
Add missing concrete security rule coverage in securecode guide
Copilot 46e2648
Clarify hostname and CSRF header wording in securecode guide
Copilot 5abba3e
Refine securecode concrete patterns for UDP parse and CSRF advisory
Copilot e468f41
Update security review checklist to 26 rules
softhack007 2fd1c1e
Update security review standards and logging guidelines
softhack007 4363121
Fix formatting of secret exposure guideline
softhack007 bfdc594
rename "short instructions" to "hardening instructions"
softhack007 682832b
Merge branch 'main' into copilot/create-ai-review-guide-wled
softhack007 9fd05b3
updated rules
softhack007 05e175c
make hardening rules applicable based on the "Trust boundary model"
softhack007 3243167
Update docs/hardening.instructions.md
softhack007 1fef106
path instructions to flag accidentially comitted creadentials / secrets
softhack007 d1f11a8
clarification
softhack007 d299602
small fixes
softhack007 0f97aa3
rules update
softhack007 53f6817
clarification: LittleFS is not a trust boundary
softhack007 5623b95
Update .coderabbit.yaml
softhack007 1197d7b
Update .coderabbit.yaml
softhack007 d4ed132
fix initial AI slop
softhack007 9e7f133
Update .coderabbit.yaml
softhack007 ed25e5c
nitpick
softhack007 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 `<!-- HUMAN_ONLY_START --> ... <!-- HUMAN_ONLY_END -->` 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 | ||
|
softhack007 marked this conversation as resolved.
|
||
| - 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. | ||
|
|
||
|
softhack007 marked this conversation as resolved.
|
||
| ## Firmware Security (C++, OWASP A01/A04/A05/A10) | ||
|
|
||
| ### FW1: Unsafe buffer operations | ||
| - **Severity**: CRITICAL | ||
| - Flag `strcpy`, `sprintf`, unchecked `memcpy`, unchecked pointer arithmetic. | ||
|
softhack007 marked this conversation as resolved.
Outdated
|
||
| - Require explicit bounds checks and length validation. | ||
|
|
||
|
softhack007 marked this conversation as resolved.
|
||
| ### 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. | ||
|
softhack007 marked this conversation as resolved.
|
||
|
|
||
| ### 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 | ||
|
softhack007 marked this conversation as resolved.
|
||
| - [ ] 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. | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.