-
Notifications
You must be signed in to change notification settings - Fork 11
Add security review template and checklist #63
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,98 @@ | ||||||||||||||||||||||||||||||
| # ✅ /task Epic: Security Review — Routes & Files | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Create a comprehensive security review of the codebase, focusing on authentication flows, session management, and mutating routes. Use the checklists below to carefully review each of the identified files and routes, one at a time. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Output template: | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Filename: `$projectRoot/security/reports/${yyyy-mm-dd}-security-foundations-report.md` | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||
| # $projectName Security Review | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ## Executive Summary | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| This review was prepared by Parallel Drive - world class security trusted by apps that secure billions in value. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Date: $date | ||||||||||||||||||||||||||||||
| Expires: ${ date + 90 days } | ||||||||||||||||||||||||||||||
ericelliott marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||
| Source repository: $repoUrl | ||||||||||||||||||||||||||||||
| Commit hash: $commitHash | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| > ## Disclaimer | ||||||||||||||||||||||||||||||
| > This review scope is limited to the codebase at the specific commit hashes listed on the front page. Any features or changes introduced after these commits are excluded from this report. | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ## Quality Scores | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Documentation quality: ${ score }/10 | ||||||||||||||||||||||||||||||
| Test coverage and quality: ${ score }/10 | ||||||||||||||||||||||||||||||
| Critical findings: ${ count } | ||||||||||||||||||||||||||||||
| High severity findings: ${ count } | ||||||||||||||||||||||||||||||
| Medium severity findings: ${ count } | ||||||||||||||||||||||||||||||
| Low severity findings: ${ count } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ## Summary of Findings | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| $summaryOfFindings | ||||||||||||||||||||||||||||||
|
Comment on lines
+26
to
+35
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ## $route | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ${ for each file }: | ||||||||||||||||||||||||||||||
| ### $fileName | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ${ for each checklistItem } | ||||||||||||||||||||||||||||||
| ${ ✅ | ❌ | ⚠️ } $ItemName | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| ${ for each issue } | ||||||||||||||||||||||||||||||
|
Comment on lines
+39
to
+45
|
||||||||||||||||||||||||||||||
| ${ for each file }: | |
| ### $fileName | |
| ${ for each checklistItem } | |
| ${ ✅ | ❌ | ⚠️ } $ItemName | |
| ${ for each issue } | |
| [for each file in route]: | |
| ### $fileName | |
| [for each checklist item]: | |
| ${ ✅ | ❌ | ⚠️ } $ItemName | |
| [for each issue]: |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent placeholder syntax throughout the template. Lines use different formats: $date, ${score}, $summaryOfFindings, $route, $fileName, etc. For consistency and clarity in documentation templates, standardize on one format (either $variable or ${variable}).
Copilot
AI
Dec 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The template includes pseudo-code control flow syntax "${ for each file }" and "${ for each checklistItem }" (lines 39, 42, 45) that doesn't match any standard templating language. This pseudo-syntax might confuse implementers about what templating engine to use or how to implement these loops. Consider either using a specific templating language syntax (like Handlebars, Mustache, or Jinja2) or clarifying in comments that this is pseudo-code that needs to be replaced with actual templating logic.
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The closing triple quotes on line 51 should use backticks to match the opening on line 9 if this is meant to represent a Markdown code block or template. Currently line 9 uses """ and line 51 also uses """, which is not standard Markdown syntax. Consider using triple backticks (```) for consistency with Markdown conventions.
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The phrase "hangman and sidestep timing safe compare pitfalls" is unclear and grammatically awkward. Consider rephrasing for clarity, such as "mitigate timing attacks and avoid timing-safe comparison pitfalls" or "prevent timing attacks through proper hashing".
| - Verify 2FA/TOTP and backup code handling (encryption at rest, single-use, compare hashes to mitigate hangman and sidestep timing safe compare pitfalls, deletion upon use). | |
| - Verify 2FA/TOTP and backup code handling (encryption at rest, single-use, compare hashes to mitigate timing attacks and avoid timing-safe comparison pitfalls, deletion upon use). |
Copilot
AI
Jan 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop syntax for each jwt-flow { carefully run all checks in ai/rules/security/jwt-security.mdc one at a time. } uses inconsistent brace notation and unclear pseudocode. This should be clarified as instructional text rather than attempting to look like code syntax. Consider rephrasing to: "For each JWT flow detected, carefully run all checks in ai/rules/security/jwt-security.mdc one at a time."
| If JWT use is detected, for each jwt-flow { carefully run all checks in ai/rules/security/jwt-security.mdc one at a time. } | |
| If JWT use is detected, carefully run all checks in `ai/rules/security/jwt-security.mdc` for each JWT flow detected, one at a time. |
Copilot
AI
Dec 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The advice to "Never compare raw secrets with equality or crypto.timingSafeEqual" is misleading. The crypto.timingSafeEqual function is specifically designed for secure constant-time comparison of secrets and is a legitimate security best practice. While the advice to hash values before comparison is valid in certain contexts, completely discouraging the use of crypto.timingSafeEqual could lead developers to avoid a properly designed security function. Consider clarifying that crypto.timingSafeEqual is acceptable for certain use cases, or explaining more precisely when hashing is preferred over direct timing-safe comparison.
| - Timing-Safe Compare (use hashing) | |
| - Never compare raw secrets with equality or `crypto.timingSafeEqual`; avoid XOR/accumulation tricks. | |
| - Always hash both stored secret and candidate using SHA3 and compare hashes. | |
| - Applies to ALL security token compares (webhooks, reset tokens, CSRF, API keys, signatures). | |
| - If "timing safe compare" on raw value detected => CRITICAL bug report; include rationale in code comment to prevent regressions. | |
| - Timing-Safe Compare (hashing and constant-time) | |
| - Never compare secrets or secret-derived values (tokens, MACs, hashes) with plain equality (`==`/`===`); avoid XOR/accumulation or other DIY constant-time tricks. | |
| - Use a well-reviewed constant-time compare (for example, Node.js `crypto.timingSafeEqual` on equal-length `Buffer`s) when you must compare raw secrets or fixed-length secret-derived values. | |
| - Prefer storing only secret hashes or MACs (for example, using SHA-3/SHA-256, HMAC, or password hashing schemes such as bcrypt/Argon2), and compare those values using a constant-time compare. | |
| - Applies to ALL security token compares (webhooks, reset tokens, CSRF, API keys, signatures). | |
| - If a non-constant-time equality check on secrets or secret-derived values is detected => CRITICAL bug report; include rationale in code comment to prevent regressions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filename template uses
${yyyy-mm-dd}which is not a valid JavaScript date format expression. This should be clarified as a placeholder format. Consider using a clearer notation like[YYYY-MM-DD]or{date}to indicate this should be replaced with an actual date string in that format.