Add security review template and checklist #63
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces a comprehensive security review template and checklist to guide security audits of authentication flows, session management, and mutating routes. The template provides structured guidance for conducting OWASP Top 10 vulnerability scans and other security assessments.
- Adds a markdown template for generating security review reports with executive summaries, quality scores, and findings
- Provides detailed security checklists covering authentication, OWASP Top 10 vulnerabilities, JWT security, and timing-safe comparisons
- Defines a
/securitycommand for generating comprehensive security reports
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 |
There was a problem hiding this comment.
The template uses different placeholder syntaxes inconsistently throughout. Lines 26-31 use "${ score }" and "${ count }" with curly braces and spaces, while lines 16-19 use "$date", "$repoUrl", "$commitHash" without braces, and line 35 uses "$summaryOfFindings". This inconsistency could lead to confusion when implementing the template. Consider standardizing on a single placeholder format throughout the template.
| ${ for each file }: | ||
| ### $fileName | ||
|
|
||
| ${ for each checklistItem } | ||
| ${ ✅ | ❌ | ⚠️ } $ItemName | ||
|
|
||
| ${ for each issue } | ||
| **${issue}:** ${issueDescription} | ||
|
|
||
| ${ codeSnippet } | ||
|
|
||
| ${ briefExplanation } |
There was a problem hiding this comment.
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.
|
|
||
| ## Executive Summary | ||
|
|
||
| This review was propared by by Parallel Drive - world class security trusted by apps that secure billions in value. |
There was a problem hiding this comment.
Spelling error: "propared" should be "prepared". This typo appears in the executive summary template that will be used in generated security reports.
| This review was propared by by Parallel Drive - world class security trusted by apps that secure billions in value. | |
| This review was prepared by Parallel Drive - world class security trusted by apps that secure billions in value. |
|
|
||
| ## Executive Summary | ||
|
|
||
| This review was propared by by Parallel Drive - world class security trusted by apps that secure billions in value. |
There was a problem hiding this comment.
Spelling error: There's a duplicate "by" in the text. It reads "propared by by Parallel Drive" but should only have one "by".
| This review was propared by by Parallel Drive - world class security trusted by apps that secure billions in value. | |
| This review was propared by Parallel Drive - world class security trusted by apps that secure billions in value. |
| - 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. |
There was a problem hiding this comment.
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. |
| This review was propared by by Parallel Drive - world class security trusted by apps that secure billions in value. | ||
|
|
||
| Date: $date | ||
| Expires: ${ date + 90 days } |
There was a problem hiding this comment.
Inconsistent template variable syntax. Line 17 uses JavaScript-style template literal syntax "${ date + 90 days}" which is not valid in a markdown template and won't be properly evaluated. This should either use a consistent placeholder format like "$expiryDate" matching the style used elsewhere (e.g., "$date", "$repoUrl"), or specify the expected format more clearly.
| Expires: ${ date + 90 days } | |
| Expires: $expiryDate |
No description provided.