Security Panel MVP (#1660)
TODOs: - [x] Add documentation - [x] e2e tests: run security review, update knowledge, and fix issue - [x] more stringent risk rating <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Introduces a new Security mode with a Security Review panel that runs reviews, edits rules, parses findings via IPC, and supports fixing issues, with tests and prompt/runtime support. > > - **UI/Preview Panel**: > - Add `security` preview mode to `previewModeAtom` and ActionHeader (Shield button). > - New `SecurityPanel` showing findings table (sorted by severity), run review, fix issue flow, and edit `SECURITY_RULES.md` dialog. > - Wire into `PreviewPanel` content switch. > - **Hooks**: > - `useSecurityReview(appId)`: fetch latest review via IPC. > - `useStreamChat`: add `onSettled` callback to invoke refreshes after streams. > - **IPC/Main**: > - `security_handlers`: `get-latest-security-review` parses `<dyad-security-finding>` from latest assistant message. > - Register handler in `ipc_host`; expose channel in `preload`. > - `ipc_client`: add `getLatestSecurityReview(appId)`. > - `chat_stream_handlers`: detect `/security-review`, use dedicated system prompt, optionally append `SECURITY_RULES.md`, suppress Supabase-not-available note in this mode. > - **Prompts**: > - Add `SECURITY_REVIEW_SYSTEM_PROMPT` with structured finding output. > - **Supabase**: > - Enhance schema query to include `rls_enabled`, split policy `using_clause`/`with_check_clause`. > - **E2E Tests**: > - New `security_review.spec.ts` plus snapshots and fixture findings; update test helper for `security` mode and findings table snapshot. > - Fake LLM server streams security findings for `/security-review` and increases batch size. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 5022d01e22a2dd929a968eeba0da592e0aeece01. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY -->
This commit is contained in:
132
e2e-tests/fixtures/security-review/findings.md
Normal file
132
e2e-tests/fixtures/security-review/findings.md
Normal file
@@ -0,0 +1,132 @@
|
||||
OK, let's review the security.
|
||||
|
||||
Here are variations with different severity levels.
|
||||
|
||||
Purposefully putting medium on top to make sure the severity levels are sorted correctly.
|
||||
|
||||
## Medium Severity
|
||||
|
||||
<dyad-security-finding title="Unvalidated File Upload Extensions" level="medium">
|
||||
**What**: The file upload endpoint accepts any file type without validating extensions or content, only checking file size
|
||||
|
||||
**Risk**: An attacker could upload malicious files (e.g., .exe, .php) that might be executed if the server is misconfigured, or upload extremely large files to consume storage space
|
||||
|
||||
**Potential Solutions**:
|
||||
1. Implement a whitelist of allowed file extensions (e.g., `.jpg`, `.png`, `.pdf`)
|
||||
2. Validate file content type using magic numbers, not just the extension
|
||||
3. Store uploaded files outside the web root with random filenames
|
||||
4. Implement virus scanning for uploaded files using ClamAV or similar
|
||||
|
||||
**Relevant Files**: `src/api/upload.ts`
|
||||
|
||||
</dyad-security-finding>
|
||||
|
||||
<dyad-security-finding title="Missing CSRF Protection on State-Changing Operations" level="medium">
|
||||
**What**: POST, PUT, and DELETE endpoints don't implement CSRF tokens, making them vulnerable to cross-site request forgery attacks
|
||||
|
||||
**Risk**: An attacker could trick authenticated users into unknowingly performing actions like changing their email, making purchases, or deleting data by visiting a malicious website
|
||||
|
||||
**Potential Solutions**:
|
||||
1. Implement CSRF tokens using a library like `csurf` for Express
|
||||
2. Set `SameSite=Strict` or `SameSite=Lax` on session cookies
|
||||
3. Verify the `Origin` or `Referer` header for sensitive operations
|
||||
4. For API-only applications, consider using custom headers that browsers can't set cross-origin
|
||||
|
||||
**Relevant Files**: `src/middleware/auth.ts`, `src/api/*.ts`
|
||||
|
||||
</dyad-security-finding>
|
||||
|
||||
## Critical Severity
|
||||
|
||||
<dyad-security-finding title="SQL Injection in User Lookup" level="critical">
|
||||
**What**: User input flows directly into database queries without validation, allowing attackers to execute arbitrary SQL commands
|
||||
|
||||
**Risk**: An attacker could steal all customer data, delete your entire database, or take over admin accounts by manipulating the URL
|
||||
|
||||
**Potential Solutions**:
|
||||
1. Use parameterized queries: `db.query('SELECT * FROM users WHERE id = ?', [userId])`
|
||||
2. Add input validation to ensure `userId` is a number
|
||||
3. Implement an ORM like Prisma or TypeORM that prevents SQL injection by default
|
||||
|
||||
**Relevant Files**: `src/api/users.ts`
|
||||
|
||||
</dyad-security-finding>
|
||||
|
||||
<dyad-security-finding title="Hardcoded AWS Credentials in Source Code" level="critical">
|
||||
**What**: AWS access keys are stored directly in the codebase and committed to version control, exposing full cloud infrastructure access
|
||||
|
||||
**Risk**: Anyone with repository access (including former employees or compromised accounts) could spin up expensive resources, access S3 buckets with customer data, or destroy production infrastructure
|
||||
|
||||
**Potential Solutions**:
|
||||
1. Immediately rotate the exposed credentials in AWS IAM
|
||||
2. Use environment variables and add `.env` to `.gitignore`
|
||||
3. Implement AWS Secrets Manager or similar vault solution
|
||||
4. Scan git history and purge the credentials using tools like `git-filter-repo`
|
||||
|
||||
**Relevant Files**: `src/config/aws.ts`, `src/services/s3-uploader.ts`
|
||||
|
||||
</dyad-security-finding>
|
||||
|
||||
## High Severity
|
||||
|
||||
<dyad-security-finding title="Missing Authentication on Admin Endpoints" level="high">
|
||||
**What**: Administrative API endpoints can be accessed without authentication, relying only on URL obscurity
|
||||
|
||||
**Risk**: An attacker who discovers these endpoints could modify user permissions, access sensitive reports, or change system configurations without credentials
|
||||
|
||||
**Potential Solutions**:
|
||||
1. Add authentication middleware to all `/admin/*` routes
|
||||
2. Implement role-based access control (RBAC) to verify admin permissions
|
||||
3. Add audit logging for all administrative actions
|
||||
4. Consider implementing rate limiting on admin endpoints
|
||||
|
||||
**Relevant Files**: `src/api/admin/users.ts`, `src/api/admin/settings.ts`
|
||||
|
||||
</dyad-security-finding>
|
||||
|
||||
<dyad-security-finding title="JWT Secret Using Default Value" level="high">
|
||||
**What**: The application uses a hardcoded default JWT secret ("your-secret-key") for signing authentication tokens
|
||||
|
||||
**Risk**: Attackers can forge valid JWT tokens to impersonate any user, including administrators, granting them unauthorized access to user accounts and sensitive data
|
||||
|
||||
**Potential Solutions**:
|
||||
1. Generate a strong random secret: `openssl rand -base64 32`
|
||||
2. Store the secret in environment variables
|
||||
3. Rotate the JWT secret, which will invalidate all existing sessions
|
||||
4. Consider using RS256 (asymmetric) instead of HS256 for better security
|
||||
|
||||
**Relevant Files**: `src/auth/jwt.ts`
|
||||
|
||||
</dyad-security-finding>
|
||||
|
||||
## Low Severity
|
||||
|
||||
<dyad-security-finding title="Verbose Error Messages Expose Stack Traces" level="low">
|
||||
**What**: Production error responses include full stack traces and internal file paths that are sent to end users
|
||||
|
||||
**Risk**: Attackers can use this information to map your application structure, identify frameworks and versions, and find potential attack vectors more easily
|
||||
|
||||
**Potential Solutions**:
|
||||
1. Configure different error handlers for production vs development
|
||||
2. Log detailed errors server-side but send generic messages to clients
|
||||
3. Use an error handling middleware: `if (process.env.NODE_ENV === 'production') { /* hide details */ }`
|
||||
4. Implement centralized error logging with tools like Sentry
|
||||
|
||||
**Relevant Files**: `src/middleware/error-handler.ts`
|
||||
|
||||
</dyad-security-finding>
|
||||
|
||||
<dyad-security-finding title="Missing Security Headers" level="low">
|
||||
**What**: The application doesn't set recommended security headers like `X-Frame-Options`, `X-Content-Type-Options`, and `Strict-Transport-Security`
|
||||
|
||||
**Risk**: Users may be vulnerable to clickjacking attacks, MIME-type sniffing, or man-in-the-middle attacks, though exploitation requires specific conditions
|
||||
|
||||
**Potential Solutions**:
|
||||
1. Use Helmet.js middleware: `app.use(helmet())`
|
||||
2. Configure headers manually in your web server (nginx/Apache) or application
|
||||
3. Set `Content-Security-Policy` to prevent XSS attacks
|
||||
4. Enable HSTS to enforce HTTPS connections
|
||||
|
||||
**Relevant Files**: `src/app.ts`, `nginx.conf`
|
||||
|
||||
</dyad-security-finding>
|
||||
@@ -476,7 +476,9 @@ export class PageObject {
|
||||
// Preview panel
|
||||
////////////////////////////////
|
||||
|
||||
async selectPreviewMode(mode: "code" | "problems" | "preview" | "configure") {
|
||||
async selectPreviewMode(
|
||||
mode: "code" | "problems" | "preview" | "configure" | "security",
|
||||
) {
|
||||
await this.page.getByTestId(`${mode}-mode-button`).click();
|
||||
}
|
||||
|
||||
@@ -596,6 +598,12 @@ export class PageObject {
|
||||
});
|
||||
}
|
||||
|
||||
async snapshotSecurityFindingsTable() {
|
||||
await expect(
|
||||
this.page.getByTestId("security-findings-table"),
|
||||
).toMatchAriaSnapshot();
|
||||
}
|
||||
|
||||
async snapshotServerDump(
|
||||
type: "all-messages" | "last-message" | "request" = "all-messages",
|
||||
{ name = "", dumpIndex = -1 }: { name?: string; dumpIndex?: number } = {},
|
||||
|
||||
45
e2e-tests/security_review.spec.ts
Normal file
45
e2e-tests/security_review.spec.ts
Normal file
@@ -0,0 +1,45 @@
|
||||
import { test, testSkipIfWindows } from "./helpers/test_helper";
|
||||
|
||||
// Skipping because snapshotting the security findings table is not
|
||||
// consistent across platforms because different amounts of text
|
||||
// get ellipsis'd out.
|
||||
testSkipIfWindows("security review", async ({ po }) => {
|
||||
await po.setUp({ autoApprove: true });
|
||||
await po.sendPrompt("tc=1");
|
||||
|
||||
await po.selectPreviewMode("security");
|
||||
|
||||
await po.page
|
||||
.getByRole("button", { name: "Run Security Review" })
|
||||
.first()
|
||||
.click();
|
||||
await po.waitForChatCompletion();
|
||||
await po.snapshotServerDump("all-messages");
|
||||
await po.snapshotSecurityFindingsTable();
|
||||
|
||||
await po.page.getByRole("button", { name: "Fix Issue" }).first().click();
|
||||
await po.waitForChatCompletion();
|
||||
await po.snapshotMessages();
|
||||
});
|
||||
|
||||
test("security review - edit and use knowledge", async ({ po }) => {
|
||||
await po.setUp({ autoApprove: true });
|
||||
await po.sendPrompt("tc=1");
|
||||
|
||||
await po.selectPreviewMode("security");
|
||||
await po.page.getByRole("button", { name: "Edit Security Rules" }).click();
|
||||
await po.page
|
||||
.getByRole("textbox", { name: "# SECURITY_RULES.md\\n\\" })
|
||||
.click();
|
||||
await po.page
|
||||
.getByRole("textbox", { name: "# SECURITY_RULES.md\\n\\" })
|
||||
.fill("testing\nrules123");
|
||||
await po.page.getByRole("button", { name: "Save" }).click();
|
||||
|
||||
await po.page
|
||||
.getByRole("button", { name: "Run Security Review" })
|
||||
.first()
|
||||
.click();
|
||||
await po.waitForChatCompletion();
|
||||
await po.snapshotServerDump("all-messages");
|
||||
});
|
||||
File diff suppressed because it is too large
Load Diff
@@ -0,0 +1,130 @@
|
||||
- table:
|
||||
- rowgroup:
|
||||
- row "Level Issue Action":
|
||||
- cell "Level"
|
||||
- cell "Issue"
|
||||
- cell "Action"
|
||||
- rowgroup:
|
||||
- 'row "critical SQL Injection in User Lookup What: User input flows directly into database queries without validation, allowing attackers to execute arbitrary SQL commands Risk: An attac... Show more Fix Issue"':
|
||||
- cell "critical":
|
||||
- img
|
||||
- 'cell "SQL Injection in User Lookup What: User input flows directly into database queries without validation, allowing attackers to execute arbitrary SQL commands Risk: An attac... Show more"':
|
||||
- 'button "SQL Injection in User Lookup What: User input flows directly into database queries without validation, allowing attackers to execute arbitrary SQL commands Risk: An attac... Show more"':
|
||||
- paragraph:
|
||||
- strong: What
|
||||
- text: ": User input flows directly into database queries without validation, allowing attackers to execute arbitrary SQL commands"
|
||||
- paragraph:
|
||||
- strong: Risk
|
||||
- text: ": An attac..."
|
||||
- button "Show more":
|
||||
- img
|
||||
- cell "Fix Issue":
|
||||
- button "Fix Issue"
|
||||
- 'row "critical Hardcoded AWS Credentials in Source Code What: AWS access keys are stored directly in the codebase and committed to version control, exposing full cloud infrastructure access Risk: A... Show more Fix Issue"':
|
||||
- cell "critical":
|
||||
- img
|
||||
- 'cell "Hardcoded AWS Credentials in Source Code What: AWS access keys are stored directly in the codebase and committed to version control, exposing full cloud infrastructure access Risk: A... Show more"':
|
||||
- 'button "Hardcoded AWS Credentials in Source Code What: AWS access keys are stored directly in the codebase and committed to version control, exposing full cloud infrastructure access Risk: A... Show more"':
|
||||
- paragraph:
|
||||
- strong: What
|
||||
- text: ": AWS access keys are stored directly in the codebase and committed to version control, exposing full cloud infrastructure access"
|
||||
- paragraph:
|
||||
- strong: Risk
|
||||
- text: ": A..."
|
||||
- button "Show more":
|
||||
- img
|
||||
- cell "Fix Issue":
|
||||
- button "Fix Issue"
|
||||
- 'row "high Missing Authentication on Admin Endpoints What: Administrative API endpoints can be accessed without authentication, relying only on URL obscurity Risk: An attacker who discovers thes... Show more Fix Issue"':
|
||||
- cell "high":
|
||||
- img
|
||||
- 'cell "Missing Authentication on Admin Endpoints What: Administrative API endpoints can be accessed without authentication, relying only on URL obscurity Risk: An attacker who discovers thes... Show more"':
|
||||
- 'button "Missing Authentication on Admin Endpoints What: Administrative API endpoints can be accessed without authentication, relying only on URL obscurity Risk: An attacker who discovers thes... Show more"':
|
||||
- paragraph:
|
||||
- strong: What
|
||||
- text: ": Administrative API endpoints can be accessed without authentication, relying only on URL obscurity"
|
||||
- paragraph:
|
||||
- strong: Risk
|
||||
- text: ": An attacker who discovers thes..."
|
||||
- button "Show more":
|
||||
- img
|
||||
- cell "Fix Issue":
|
||||
- button "Fix Issue"
|
||||
- 'row "high JWT Secret Using Default Value What: The application uses a hardcoded default JWT secret (\"your-secret-key\") for signing authentication tokens Risk: Attackers can forge val... Show more Fix Issue"':
|
||||
- cell "high":
|
||||
- img
|
||||
- 'cell "JWT Secret Using Default Value What: The application uses a hardcoded default JWT secret (\"your-secret-key\") for signing authentication tokens Risk: Attackers can forge val... Show more"':
|
||||
- 'button "JWT Secret Using Default Value What: The application uses a hardcoded default JWT secret (\"your-secret-key\") for signing authentication tokens Risk: Attackers can forge val... Show more"':
|
||||
- paragraph:
|
||||
- strong: What
|
||||
- text: ": The application uses a hardcoded default JWT secret (\"your-secret-key\") for signing authentication tokens"
|
||||
- paragraph:
|
||||
- strong: Risk
|
||||
- text: ": Attackers can forge val..."
|
||||
- button "Show more":
|
||||
- img
|
||||
- cell "Fix Issue":
|
||||
- button "Fix Issue"
|
||||
- 'row "medium Unvalidated File Upload Extensions What: The file upload endpoint accepts any file type without validating extensions or content, only checking file size Risk: An attacker coul... Show more Fix Issue"':
|
||||
- cell "medium":
|
||||
- img
|
||||
- 'cell "Unvalidated File Upload Extensions What: The file upload endpoint accepts any file type without validating extensions or content, only checking file size Risk: An attacker coul... Show more"':
|
||||
- 'button "Unvalidated File Upload Extensions What: The file upload endpoint accepts any file type without validating extensions or content, only checking file size Risk: An attacker coul... Show more"':
|
||||
- paragraph:
|
||||
- strong: What
|
||||
- text: ": The file upload endpoint accepts any file type without validating extensions or content, only checking file size"
|
||||
- paragraph:
|
||||
- strong: Risk
|
||||
- text: ": An attacker coul..."
|
||||
- button "Show more":
|
||||
- img
|
||||
- cell "Fix Issue":
|
||||
- button "Fix Issue"
|
||||
- 'row "medium Missing CSRF Protection on State-Changing Operations What: POST, PUT, and DELETE endpoints don''t implement CSRF tokens, making them vulnerable to cross-site request forgery attacks Risk: An atta... Show more Fix Issue"':
|
||||
- cell "medium":
|
||||
- img
|
||||
- 'cell "Missing CSRF Protection on State-Changing Operations What: POST, PUT, and DELETE endpoints don''t implement CSRF tokens, making them vulnerable to cross-site request forgery attacks Risk: An atta... Show more"':
|
||||
- 'button "Missing CSRF Protection on State-Changing Operations What: POST, PUT, and DELETE endpoints don''t implement CSRF tokens, making them vulnerable to cross-site request forgery attacks Risk: An atta... Show more"':
|
||||
- paragraph:
|
||||
- strong: What
|
||||
- text: ": POST, PUT, and DELETE endpoints don't implement CSRF tokens, making them vulnerable to cross-site request forgery attacks"
|
||||
- paragraph:
|
||||
- strong: Risk
|
||||
- text: ": An atta..."
|
||||
- button "Show more":
|
||||
- img
|
||||
- cell "Fix Issue":
|
||||
- button "Fix Issue"
|
||||
- 'row "low Verbose Error Messages Expose Stack Traces What: Production error responses include full stack traces and internal file paths that are sent to end users Risk: Attackers can use this in... Show more Fix Issue"':
|
||||
- cell "low":
|
||||
- img
|
||||
- 'cell "Verbose Error Messages Expose Stack Traces What: Production error responses include full stack traces and internal file paths that are sent to end users Risk: Attackers can use this in... Show more"':
|
||||
- 'button "Verbose Error Messages Expose Stack Traces What: Production error responses include full stack traces and internal file paths that are sent to end users Risk: Attackers can use this in... Show more"':
|
||||
- paragraph:
|
||||
- strong: What
|
||||
- text: ": Production error responses include full stack traces and internal file paths that are sent to end users"
|
||||
- paragraph:
|
||||
- strong: Risk
|
||||
- text: ": Attackers can use this in..."
|
||||
- button "Show more":
|
||||
- img
|
||||
- cell "Fix Issue":
|
||||
- button "Fix Issue"
|
||||
- 'row "low Missing Security Headers What: The application doesn''t set recommended security headers like `X-Frame-Options`, `X-Content-Type-Options`, and `Strict-Transport-Security` ... Show more Fix Issue"':
|
||||
- cell "low":
|
||||
- img
|
||||
- 'cell "Missing Security Headers What: The application doesn''t set recommended security headers like `X-Frame-Options`, `X-Content-Type-Options`, and `Strict-Transport-Security` ... Show more"':
|
||||
- 'button "Missing Security Headers What: The application doesn''t set recommended security headers like `X-Frame-Options`, `X-Content-Type-Options`, and `Strict-Transport-Security` ... Show more"':
|
||||
- paragraph:
|
||||
- strong: What
|
||||
- text: ": The application doesn't set recommended security headers like"
|
||||
- code: "`X-Frame-Options`"
|
||||
- text: ","
|
||||
- code: "`X-Content-Type-Options`"
|
||||
- text: ", and"
|
||||
- code: "`Strict-Transport-Security`"
|
||||
- paragraph: ...
|
||||
- button "Show more":
|
||||
- img
|
||||
- cell "Fix Issue":
|
||||
- button "Fix Issue"
|
||||
1070
e2e-tests/snapshots/security_review.spec.ts_security-review-1.txt
Normal file
1070
e2e-tests/snapshots/security_review.spec.ts_security-review-1.txt
Normal file
File diff suppressed because it is too large
Load Diff
@@ -0,0 +1,45 @@
|
||||
- paragraph: "Please fix the following security issue in a simple and effective way:"
|
||||
- paragraph:
|
||||
- strong: SQL Injection in User Lookup
|
||||
- text: (critical severity)
|
||||
- paragraph:
|
||||
- strong: What
|
||||
- text: ": User input flows directly into database queries without validation, allowing attackers to execute arbitrary SQL commands"
|
||||
- paragraph:
|
||||
- strong: Risk
|
||||
- text: ": An attacker could steal all customer data, delete your entire database, or take over admin accounts by manipulating the URL"
|
||||
- paragraph:
|
||||
- strong: Potential Solutions
|
||||
- text: ":"
|
||||
- list:
|
||||
- listitem:
|
||||
- text: "Use parameterized queries:"
|
||||
- code: "`db.query('SELECT * FROM users WHERE id = ?', [userId])`"
|
||||
- listitem:
|
||||
- text: Add input validation to ensure
|
||||
- code: "`userId`"
|
||||
- text: is a number
|
||||
- listitem: Implement an ORM like Prisma or TypeORM that prevents SQL injection by default
|
||||
- paragraph:
|
||||
- strong: Relevant Files
|
||||
- text: ":"
|
||||
- code: "`src/api/users.ts`"
|
||||
- img
|
||||
- text: file1.txt
|
||||
- button "Edit":
|
||||
- img
|
||||
- img
|
||||
- text: file1.txt
|
||||
- paragraph: More EOM
|
||||
- button:
|
||||
- img
|
||||
- img
|
||||
- text: Approved
|
||||
- img
|
||||
- text: less than a minute ago
|
||||
- img
|
||||
- text: wrote 1 file(s)
|
||||
- button "Undo":
|
||||
- img
|
||||
- button "Retry":
|
||||
- img
|
||||
Reference in New Issue
Block a user