Enhance API Security#108
Conversation
|
Requesting review from @izadoesdev who has experience with the following files modified in this PR:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughAdds pre-execution SQL validation to query paths, introduces comprehensive security and CSP middlewares with rate limiting and injection detection, tightens API schemas for assistant and query requests, and updates frontend sanitization logic. Includes new security tests. No public API signatures changed besides stricter schema constraints. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant ElysiaApp as Elysia App
participant SecMW as securityMiddleware
participant Routes as Route Handler
participant CSP as cspHeaders
Client->>ElysiaApp: HTTP Request
activate ElysiaApp
ElysiaApp->>SecMW: onBeforeHandle (rate-limit + injection checks)
SecMW-->>ElysiaApp: allow or 400 SECURITY_VIOLATION
alt Allowed
ElysiaApp->>SecMW: onTransform (sanitize body)
SecMW-->>ElysiaApp: sanitized data
ElysiaApp->>Routes: handle request
Routes-->>ElysiaApp: response
ElysiaApp->>CSP: onAfterHandle (set headers)
CSP-->>ElysiaApp: headers applied
ElysiaApp-->>Client: Response
else Blocked
ElysiaApp-->>Client: 400 SECURITY_VIOLATION
end
deactivate ElysiaApp
sequenceDiagram
autonumber
participant Caller as API/Service
participant Validator as validateSQL()
participant DB as chQuery
Caller->>Validator: validateSQL(sql)
alt Invalid
Validator-->>Caller: false
Caller-->>Caller: throw Error("Invalid SQL")
else Valid
Validator-->>Caller: true
Caller->>DB: execute sql
DB-->>Caller: results
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Join our Discord community for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| /--/, // SQL comments | ||
| /\/\*/, // Multi-line comments start | ||
| /\*\//, // Multi-line comments end | ||
| /;/, // Statement terminators |
There was a problem hiding this comment.
There appears to be a logical inconsistency in the SQL validation approach. Line 20 flags all semicolons as dangerous patterns with /;/, while line 78 specifically uses semicolons to detect stacked queries with sql.split(';').
Consider either:
- Removing the generic semicolon pattern check, or
- Modifying it to detect semicolons only in suspicious contexts (e.g.,
/;\s*(DROP|DELETE|UPDATE)/i)
This would maintain the ability to detect stacked queries without incorrectly flagging all semicolons as dangerous.
| /;/, // Statement terminators | |
| /;\s*(DROP|DELETE|UPDATE|INSERT|CREATE|ALTER|TRUNCATE)/i, // Suspicious stacked queries |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
|
Thanks for pointing out the logical inconsistency in our SQL validation approach. I've implemented your suggestion by replacing the generic semicolon pattern with a more targeted one that detects suspicious stacked queries. The new regex now specifically looks for semicolons followed by potentially dangerous SQL commands like DROP, DELETE, UPDATE, INSERT, CREATE, ALTER, or TRUNCATE. This change maintains our ability to detect malicious stacked queries while avoiding false positives from harmless semicolons in legitimate SQL. |
| // Remove dangerous patterns | ||
| for (const pattern of dangerousPatterns) { | ||
| sanitized = sanitized.replace(pattern, ''); | ||
| } |
There was a problem hiding this comment.
The current approach of removing dangerous patterns with string replacement can inadvertently create new valid attack vectors through pattern concatenation. For example, removing <script and onerror= separately might not catch <scr<scriptipt onerror=alert(1)>.
Consider replacing this with a proper HTML sanitization library like DOMPurify, or implement HTML entity encoding instead of pattern removal:
// Instead of removing patterns, encode the input
sanitized = sanitized
.replace(/&/g, '&')
.replace(/</g, '<')
.replace(/>/g, '>')
.replace(/"/g, '"')
.replace(/'/g, ''');This approach is more robust against XSS attacks while preserving the original content's meaning.
| // Remove dangerous patterns | |
| for (const pattern of dangerousPatterns) { | |
| sanitized = sanitized.replace(pattern, ''); | |
| } | |
| // Encode HTML entities instead of removing patterns | |
| sanitized = sanitized.replace(/&/g, '&'); | |
| sanitized = sanitized.replace(/</g, '<'); | |
| sanitized = sanitized.replace(/>/g, '>'); | |
| sanitized = sanitized.replace(/"/g, '"'); | |
| sanitized = sanitized.replace(/'/g, '''); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
- Replace dangerous pattern removal with HTML entity encoding in sanitizeString to prevent XSS via pattern concatenation attacks - Add OR/AND condition detection to SQL validator to catch classic injection patterns like "OR 1=1" and "AND 1=1" - Maintain existing semicolon handling for stacked query detection - All security tests now passing
|
Thanks for your thorough review of our SQL validation and string sanitization code. I've implemented both of your suggested improvements:
These changes should significantly improve our security posture against both SQL injection and XSS attacks. Let me know if you'd like any further refinements. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/api/src/query/simple-builder.ts (1)
402-426: Strengthen ORDER/GROUP injection defenses with allowlisting and strict identifier checks.Keyword blacklists won’t catch crafted expressions (functions, comments, semicolons). Enforce a strict identifier policy and validate direction tokens.
Apply this refactor (adds a safe identifier helper and uses it):
@@ private buildGroupByClause(): string { const groupBy = this.request.groupBy || this.config.groupBy; if (!groupBy?.length) { return ''; } - // Security validation - only block dangerous SQL keywords - const dangerousKeywords = [ - 'DROP','DELETE','INSERT','UPDATE','CREATE','ALTER','TRUNCATE','EXEC','EXECUTE', - ]; - for (const field of groupBy) { - const upperField = field.toUpperCase(); - for (const keyword of dangerousKeywords) { - if (upperField.includes(keyword)) { - throw new Error( - `Grouping by field '${field}' contains dangerous keyword: ${keyword}` - ); - } - } - } + const safeIdentifier = (s: string) => /^[A-Za-z_][A-Za-z0-9_\.]*$/.test(s); + for (const field of groupBy) { + if (!safeIdentifier(field)) { + throw new Error(`Invalid GROUP BY identifier: '${field}'`); + } + } return ` GROUP BY ${groupBy.join(', ')}`; }And for ORDER BY:
@@ private buildOrderByClause(): string { const orderBy = this.request.orderBy || this.config.orderBy; if (!orderBy) { return ''; } - // Security validation - only block dangerous SQL keywords - const dangerousKeywords = [ - 'DROP','DELETE','INSERT','UPDATE','CREATE','ALTER','TRUNCATE','EXEC','EXECUTE', - ]; - const upperOrderBy = orderBy.toUpperCase(); - for (const keyword of dangerousKeywords) { - if (upperOrderBy.includes(keyword)) { - throw new Error( - `Ordering by field '${orderBy}' contains dangerous keyword: ${keyword}` - ); - } - } + const safeIdentifier = (s: string) => /^[A-Za-z_][A-Za-z0-9_\.]*$/.test(s); + const parts = orderBy.trim().split(/\s+/); + const field = parts[0] ?? ''; + const direction = (parts[1] ?? '').toUpperCase(); + if (!safeIdentifier(field)) { + throw new Error(`Invalid ORDER BY identifier: '${orderBy}'`); + } + if (direction && direction !== 'ASC' && direction !== 'DESC') { + throw new Error(`Invalid ORDER BY direction: '${direction}'`); + } - return ` ORDER BY ${orderBy}`; + return ` ORDER BY ${field}${direction ? ` ${direction}` : ''}`; }Optional: expose an allowlist
allowedOrderBy/allowedGroupByinSimpleQueryConfigand check membership for even tighter control.apps/api/src/agent/utils/sql-validator.ts (1)
1-29: String “includes” for keywords causes false positives (e.g., MIGRANT). Use word-boundary regexes and catch EXEC().Current checks like upperSQL.includes('GRANT') will incorrectly reject benign strings containing that substring. Switch to RegExp with word boundaries and optional whitespace for multi-word statements.
-const FORBIDDEN_SQL_KEYWORDS = [ - 'INSERT INTO', - 'UPDATE SET', - 'DELETE FROM', - 'DROP TABLE', - 'DROP DATABASE', - 'CREATE TABLE', - 'CREATE DATABASE', - 'ALTER TABLE', - 'EXEC ', - 'EXECUTE ', - 'TRUNCATE', - 'MERGE', - 'BULK', - 'RESTORE', - 'BACKUP', - 'GRANT', - 'REVOKE', - 'SHOW GRANTS', - 'SHOW USERS', - 'SYSTEM', - 'ATTACH', - 'DETACH', - 'OPTIMIZE', - 'CHECK', - 'REPAIR', - 'ANALYZE', -] as const; +const FORBIDDEN_SQL_KEYWORDS: readonly RegExp[] = [ + /\bINSERT\s+INTO\b/i, + /\bUPDATE\b/i, // Covers UPDATE ... SET + /\bDELETE\s+FROM\b/i, + /\bDROP\s+TABLE\b/i, + /\bDROP\s+DATABASE\b/i, + /\bCREATE\s+TABLE\b/i, + /\bCREATE\s+DATABASE\b/i, + /\bALTER\s+TABLE\b/i, + /\bEXEC(UTE)?\b/i, // Catches EXEC and EXECUTE, with or without space + /\bTRUNCATE\b/i, + /\bMERGE\b/i, + /\bBULK\b/i, + /\bRESTORE\b/i, + /\bBACKUP\b/i, + /\bGRANT\b/i, + /\bREVOKE\b/i, + /\bSHOW\s+GRANTS\b/i, + /\bSHOW\s+USERS\b/i, + /\bSYSTEM\b/i, + /\bATTACH\b/i, + /\bDETACH\b/i, + /\bOPTIMIZE\b/i, + /\bCHECK\b/i, + /\bREPAIR\b/i, + /\bANALYZE\b/i, +] as const;
🧹 Nitpick comments (16)
apps/api/src/index.ts (1)
16-18: Consider running CORS before security middleware to avoid blocking/altering preflight requests.Your security middleware enforces rate limits and scans headers/query params. OPTIONS preflights can be plentiful and innocuous; rate-limiting or pattern checks there may cause surprising 4xx/429s. Prefer
.use(cors(...))ahead of.use(securityMiddleware()).use(cspHeaders()), or explicitly bypass rate limiting and scanning forOPTIONSrequests in the middleware.Apply one of the following diffs:
Option A — reorder:
-const app = new Elysia() - .use(securityMiddleware()) - .use(cspHeaders()) - .use( - cors({ +const app = new Elysia() + .use( + cors({ credentials: true, origin: [ /(?:^|\.)databuddy\.cc$/, ...(process.env.NODE_ENV === 'development' ? ['http://localhost:3000'] : []), ], }) ) + .use(securityMiddleware()) + .use(cspHeaders())Option B — keep order but skip OPTIONS inside middleware:
- .onBeforeHandle(async ({ request, set }) => { + .onBeforeHandle(async ({ request, set }) => { + if (request.method === 'OPTIONS') return; // let CORS handle preflight // existing logic...apps/basket/src/utils/validation.ts (1)
56-71: Avoid slicing mid-entity if you keep HTML encoding (if you opt not to apply the minimal fix).If you choose to retain entity encoding in this function, slice before encoding or implement a streaming encoder that respects
actualMaxLengthon the encoded output without cutting entities.I can provide a safe “encode-with-limit” helper if you’d like to enforce a strict byte/char budget on the encoded string.
apps/api/src/middleware/security.ts (2)
96-122: Rate limit keying and store lifecycle can be improved (spoofable IPs, unbounded map).
x-forwarded-formay contain multiple addresses and is spoofable. Parse the left-most IP and, when possible, prefer a trusted header (e.g.,cf-connecting-ip) or the connection’s remote address.- The in-memory map can grow without bound; prune expired entries when windows roll over.
Suggested adjustments:
- const ip = request.headers.get('x-forwarded-for') || - request.headers.get('x-real-ip') || - 'unknown'; + const xff = request.headers.get('x-forwarded-for') || ''; + const firstHop = xff.split(',')[0]?.trim(); + const ip = request.headers.get('cf-connecting-ip') + || firstHop + || request.headers.get('x-real-ip') + || 'unknown'; @@ - if (!entry || now > entry.resetTime) { - rateLimitStore.set(key, { count: 1, resetTime: now + limit.window }); + if (!entry || now > entry.resetTime) { + // prune old entry to cap memory + if (entry && now > entry.resetTime) rateLimitStore.delete(key); + rateLimitStore.set(key, { count: 1, resetTime: now + limit.window });Longer-term, back this with Redis in production as your comment notes.
137-147: Query param scanning: allow benign characters but block stacked statements and comments.With tightened patterns, keep high-signal checks on
;followed by DDL/DML, inline/block comments, and UNION SELECT to reduce noise while catching classic injection vectors.I can wire your shared
validateSQL(or a lighter variant) for query params that are destined for SQL contexts, instead of scanning every param generically.apps/api/src/schemas/query-schemas.ts (3)
41-54: Constrain ParameterWithDatesSchema.name and id to safe identifier shapes.Name and id are currently unconstrained while other params use strict patterns. Align them to reduce surface for malformed inputs.
export const ParameterWithDatesSchema = t.Object({ - name: t.String(), + name: t.String({ maxLength: 100, pattern: '^[a-zA-Z_][a-zA-Z0-9_]*$' }), start_date: t.Optional(t.String()), end_date: t.Optional(t.String()), granularity: t.Optional( t.Union([ t.Literal('hourly'), t.Literal('daily'), t.Literal('hour'), t.Literal('day'), ]) ), - id: t.Optional(t.String()), + id: t.Optional(t.String({ maxLength: 100, pattern: '^[a-zA-Z0-9_-]+$' })), });
16-39: Make op/value type-safe via a discriminated union.Currently, like/gt/lt/in/notIn accept any value type. This can lead to runtime coercion bugs in builders. Consider a discriminated union keyed by op to enforce correct value types at validation time.
Example (outside selected lines for illustration):
// Alternative shape: export const FilterSchema = t.Union([ t.Object({ field: t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))), op: t.Literal('eq'), value: t.Union([t.String({ maxLength: 500 }), t.Number({ minimum: -2147483648, maximum: 2147483647 })]) }), t.Object({ field: t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))), op: t.Literal('ne'), value: t.Union([t.String({ maxLength: 500 }), t.Number({ minimum: -2147483648, maximum: 2147483647 })]) }), t.Object({ field: t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))), op: t.Literal('like'), value: t.String({ maxLength: 500 }) }), t.Object({ field: t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))), op: t.Literal('gt'), value: t.Number({ minimum: -2147483648, maximum: 2147483647 }) }), t.Object({ field: t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))), op: t.Literal('lt'), value: t.Number({ minimum: -2147483648, maximum: 2147483647 }) }), t.Object({ field: t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))), op: t.Literal('in'), value: t.Array(t.Union([t.String({ maxLength: 500 }), t.Number({ minimum: -2147483648, maximum: 2147483647 })]), { maxItems: 100 }) }), t.Object({ field: t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))), op: t.Literal('notIn'), value: t.Array(t.Union([t.String({ maxLength: 500 }), t.Number({ minimum: -2147483648, maximum: 2147483647 })]), { maxItems: 100 }) }), ]);This change improves type safety and narrows invalid combinations early.
106-145: Types: mirror the allowlist and structured orderBy in exported types.Once you wire the allowlist and structured orderBy, please update types accordingly to keep TS aligned.
Example (outside selected lines for illustration):
export type FilterField = typeof ALLOWED_FILTER_FIELDS[number]; export type FilterType = | { field: FilterField; op: 'eq' | 'ne'; value: string | number } | { field: FilterField; op: 'like'; value: string } | { field: FilterField; op: 'gt' | 'lt'; value: number } | { field: FilterField; op: 'in' | 'notIn'; value: Array<string | number> }; export type OrderByItem = { field: FilterField; direction?: 'asc' | 'desc' }; export type CompileRequestType = { /* ... */ groupBy?: FilterField[]; orderBy?: OrderByItem[]; /* ... */ }; export type DynamicQueryRequestType = { /* ... */ groupBy?: FilterField; /* ... */ };apps/api/src/agent/utils/query-executor.ts (2)
11-15: Throw a typed 400 error for validation failures.Using a generic Error can bubble up as a 500. Prefer a domain/HTTP-aware error so callers respond with 400 Bad Request.
Example (outside selected lines for illustration):
export class InvalidQueryError extends Error { status = 400 as const; code = 'ERR_INVALID_SQL' as const; constructor(message = 'SQL query validation failed') { super(message); } } // then: if (!validateSQL(sql)) { throw new InvalidQueryError('SQL query validation failed - potential security risk detected'); }
20-24: Avoid logging raw SQL in production.Even truncated, SQL can reveal schema/PII. Gate the log behind an env flag or strip it in production.
- console.info('🔍 [Query Executor] Query completed', { - timeTaken: `${queryTime}ms`, - resultCount: result.length, - sql: sql.substring(0, 100) + (sql.length > 100 ? '...' : ''), - }); + if (process.env.LOG_SQL === '1') { + console.info('🔍 [Query Executor] Query completed', { + timeTaken: `${queryTime}ms`, + resultCount: result.length, + sql: sql.substring(0, 100) + (sql.length > 100 ? '...' : ''), + }); + } else { + console.info('🔍 [Query Executor] Query completed', { + timeTaken: `${queryTime}ms`, + resultCount: result.length, + }); + }apps/api/src/middleware/security.test.ts (3)
6-37: Nice coverage of classic stacked queries and keywords. Add regression tests for false positives.Given the validator forbids keywords by substring today, add tests proving benign tokens aren’t flagged (e.g., “migrant”, “checksum”, “optimizee”). This guards against future regressions.
test('should reject malicious SQL keywords', () => { const maliciousQueries = [ "SELECT * FROM users; DROP TABLE users; --", // ... ]; for (const query of maliciousQueries) { expect(validateSQL(query)).toBe(false); } }); + + test('should not flag benign words that contain keyword substrings', () => { + const safeTokens = [ + "SELECT 'migrant' AS label", + "SELECT 'checksum' AS label", + "SELECT 'revokee' AS label", + "SELECT 'optimizee' AS label", + ]; + for (const q of safeTokens) { + expect(validateSQL(q)).toBe(true); + } + });
88-91: Add tests for trailing semicolon and semicolons inside string literals.Your stacked-query guard splits on “;”. Ensure single trailing semicolons and semicolons in strings aren’t rejected.
test('should reject overly long queries', () => { const longQuery = "SELECT * FROM users WHERE " + "a = 1 AND ".repeat(1000) + "b = 2"; expect(validateSQL(longQuery)).toBe(false); }); + + test('should allow a single trailing semicolon', () => { + expect(validateSQL('SELECT 1;')).toBe(true); + }); + + test('should allow semicolons inside string literals', () => { + expect(validateSQL("SELECT 'a;b;c' AS val")).toBe(true); + });
105-181: Deduplicate XSS detection logic with production sanitizers.The tests reimplement XSS patterns inline, which can drift from middleware/security.ts. Prefer importing a shared util or exported pattern list so tests always reflect production logic.
Example (outside selected lines for illustration):
// apps/api/src/middleware/xss-utils.ts export const XSS_PATTERNS = [ /* existing patterns */ ]; export const hasXSSPattern = (input: string) => XSS_PATTERNS.some((re) => re.test(input)); // In test: import { XSS_PATTERNS, hasXSSPattern } from './xss-utils';apps/api/src/agent/utils/sql-validator.ts (4)
30-45: Good call on narrowing the semicolon rule; consider covering additional DDL/DCL verbs.The new /;\s*(DROP|DELETE|...)/i pattern resolves earlier over-blocking. Consider extending to VIEW/FUNCTION/PROCEDURE/SCHEMA/ROLE/USER for completeness, or rely on the SELECT/WITH-only rule which already blocks them.
Possible extension (optional):
- /;\s*(DROP|DELETE|UPDATE|INSERT|CREATE|ALTER|TRUNCATE)/i, + /;\s*(DROP|DELETE|UPDATE|INSERT|CREATE|ALTER|TRUNCATE|GRANT|REVOKE)/i,
48-51: Input-type guard and length cap look good; watch the 10k limit.The early falsy/type check is solid. A 10,000-char cap is safe but may be tight for complex CTEs. If you see legitimate rejects, consider bumping to ~50k with a timeout-based safeguard at execution time.
Also applies to: 56-59
75-78: SELECT/WITH gate is effective; document EXPLAIN/EXPLAIN ANALYZE policy.Blocking EXPLAIN* is intentional here. If you plan to support explain endpoints later, document the policy or allow a feature flag.
79-85: Stacked-query detection can miscount semicolons inside string literals.Splitting on “;” naïvely will reject queries like SELECT ';' or SELECT 'a;b'. Consider a minimal tokenizer that ignores semicolons inside single/double-quoted strings.
Example (outside selected lines for illustration):
function hasMultipleStatements(sql: string): boolean { let inSingle = false, inDouble = false, prev = ''; let count = 0; for (const ch of sql) { if (ch === "'" && prev !== '\\' && !inDouble) inSingle = !inSingle; else if (ch === '"' && prev !== '\\' && !inSingle) inDouble = !inDouble; else if (ch === ';' && !inSingle && !inDouble) count++; prev = ch; } return count > 0; // treat any unquoted semicolon as stacked }Then use hasMultipleStatements(sql) instead of splitting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
apps/api/src/agent/utils/query-executor.ts(2 hunks)apps/api/src/agent/utils/sql-validator.ts(1 hunks)apps/api/src/index.ts(1 hunks)apps/api/src/middleware/security.test.ts(1 hunks)apps/api/src/middleware/security.ts(1 hunks)apps/api/src/query/simple-builder.ts(1 hunks)apps/api/src/schemas/assistant-schemas.ts(1 hunks)apps/api/src/schemas/query-schemas.ts(5 hunks)apps/basket/src/utils/validation.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-17T07:15:17.730Z
Learnt from: sbansal1999
PR: databuddy-analytics/Databuddy#92
File: apps/api/src/agent/utils/ai-client.ts:41-46
Timestamp: 2025-08-17T07:15:17.730Z
Learning: In the Databuddy codebase, AssistantRequestSchema in apps/api/src/schemas/assistant-schemas.ts validates that message roles are constrained to only 'user' | 'assistant', providing server-side validation that prevents malicious system/tool roles from being passed to the AI client. Additional role filtering in ai-client.ts is not needed due to this upstream validation.
Applied to files:
apps/api/src/schemas/assistant-schemas.ts
🔇 Additional comments (5)
apps/api/src/index.ts (1)
58-70: Verify error handling interplay so SECURITY_VIOLATION 400s aren’t wrapped into 500s.
securityMiddleware().onErrorreturns a 400 object; the app-level.onErrorhere returns a 500 Response for unhandled errors. Confirm the framework won’t double-handle and override the middleware’s 400. If needed, gate this handler to only run whenset.status < 400or when the error wasn’t handled upstream.Would you like me to generate a minimal E2E test that asserts a malicious header yields a 400 payload and is not converted to 500?
apps/api/src/query/simple-builder.ts (1)
335-346: Consider parameterizing SELECT fields from config to avoid accidental unsafe expressions.
this.config.fieldsis concatenated directly. If any field were to include a function/expression from user input, it could bypass protections. If feasible, restrictfieldsto a known-safe set or validate fields with the samesafeIdentifierused above.Would introducing
allowedSelectFields: string[]inSimpleQueryConfigbe acceptable? I can follow up with a patch across the query builder and types.apps/basket/src/utils/validation.ts (1)
191-207: validateUrl currently receives an HTML-encoded string (problematic if encoding remains).If
sanitizeStringkeeps encoding,new URL('...&...')will throw. This is another reason to keepsanitizeStringsemantics neutral and defer HTML-encoding to rendering time.After applying the minimal fix above, please run a quick check with URLs containing query params to ensure they parse as expected.
apps/api/src/schemas/assistant-schemas.ts (1)
5-8: Schema hardening looks good and aligns with upstream assumptions.Constrained IDs and message content lengths are sensible, and the roles remain limited to 'user' | 'assistant'—consistent with the earlier learning that downstream role filtering isn’t needed.
Nit: consider adding
minLength: 1towebsiteIdto exclude empty strings:- websiteId: t.String({ - maxLength: 100, - pattern: '^[a-zA-Z0-9_-]+$' - }), + websiteId: t.String({ + minLength: 1, + maxLength: 100, + pattern: '^[a-zA-Z0-9_-]+$' +}),Also applies to: 13-16, 20-21, 22-25
apps/api/src/middleware/security.ts (1)
149-159: Double-check Elysia’sonTransformreturn shape to ensure request body replacement works.Some frameworks require
returning the transformed value directly, others require mutatingbodyon context. Please confirm this usage correctly replaces the downstream handler body; otherwise, handlers may still receive the original unsanitized payload or an object{ body: ... }.I can add a minimal unit/integration test to assert a transformed body reaches a sample route. Let me know if you want me to include it in this PR.
| // Check for dangerous keyword patterns | ||
| for (const keyword of FORBIDDEN_SQL_KEYWORDS) { | ||
| if (upperSQL.includes(keyword)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update keyword scanning loop to use regex patterns.
Switch from substring includes to RegExp.test to eliminate false positives and catch EXEC() without trailing spaces.
-// Check for dangerous keyword patterns
-for (const keyword of FORBIDDEN_SQL_KEYWORDS) {
- if (upperSQL.includes(keyword)) {
- return false;
- }
-}
+// Check for dangerous keyword patterns
+for (const re of FORBIDDEN_SQL_KEYWORDS) {
+ if (re.test(sql)) {
+ return false;
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check for dangerous keyword patterns | |
| for (const keyword of FORBIDDEN_SQL_KEYWORDS) { | |
| if (upperSQL.includes(keyword)) { | |
| return false; | |
| } | |
| } | |
| // Check for dangerous keyword patterns | |
| for (const re of FORBIDDEN_SQL_KEYWORDS) { | |
| if (re.test(sql)) { | |
| return false; | |
| } | |
| } |
| /['"];?\s*(DROP|DELETE|UPDATE|INSERT|ALTER|CREATE|EXEC|EXECUTE|TRUNCATE)\b/i, | ||
| /UNION\s+(ALL\s+)?SELECT/i, | ||
| /;\s*(DROP|DELETE|UPDATE|INSERT|ALTER|CREATE)/i, | ||
|
|
There was a problem hiding this comment.
Overbroad patterns will false-positive on normal traffic (e.g., User-Agent parentheses).
Patterns like /[;&|$(){}[]]/and/[()&|!]/match common benign characters. A typical UA string contains(and)`; your header checks will block nearly all requests. Scope patterns to realistic attack sequences, not single characters, and avoid scanning UA/Referer for generic metacharacters.
Tighten the patterns and drop the single-char char-classes:
- // Command injection patterns
- /[;&|`$(){}[\]]/,
+ // Command injection patterns (operators and subshells, not generic parentheses)
+ /(?:\|\||&&|;)/,
+ /`[^`]*`/, // backtick subshell
+ /\$\([^)]+\)/, // $() subshell
+ /\$\{[^}]+\}/, // ${} expansions
@@
- // LDAP injection patterns
- /[()&|!]/,
+ // LDAP injection patterns (only if you actually process LDAP filters; otherwise remove)
+ /(?:\(|\)|\&\&?|\|\|?|\!)(?=.*=)/, // primitive heuristic; consider disabling unless neededAnd consider removing the LDAP section entirely unless the API truly embeds LDAP queries.
Also applies to: 15-37
🤖 Prompt for AI Agents
In apps/api/src/middleware/security.ts around lines 10-13 (and similarly 15-37),
the regexes are too broad and will false-positive on benign headers (e.g.,
User-Agent contains parentheses); replace single-character char-class rules with
patterns that match realistic attack sequences (e.g., SQL keywords with
surrounding delimiters or combined SQL injection patterns, semicolon plus SQL
verb, or full "UNION SELECT" sequences) and remove checks that scan
User-Agent/Referer for generic metacharacters; also remove the LDAP-specific
rules unless the service actually constructs LDAP queries, and narrow header
inspection to suspicious header values (body/params) or add allowlists so normal
UAs aren’t blocked.
| // Check for suspicious headers | ||
| const userAgent = request.headers.get('user-agent') || ''; | ||
| const referer = request.headers.get('referer') || ''; | ||
|
|
||
| if (isInjectionAttempt(userAgent) || isInjectionAttempt(referer)) { | ||
| set.status = 400; | ||
| return { | ||
| success: false, | ||
| error: 'Malicious request detected', | ||
| code: 'SECURITY_VIOLATION' | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Header scanning will break on legitimate User-Agent/Referer values.
Even with the tightened patterns, scanning full UA/Referer aggressively is noisy. Recommend limiting checks to high-signal XSS markers (<script, javascript:) and SQL keywords plus stacked queries, or skipping UA entirely.
Apply a stricter, header-specific filter:
- if (isInjectionAttempt(userAgent) || isInjectionAttempt(referer)) {
+ const headerSuspicious = [/javascript:/i, /<script/i, /;\s*(DROP|DELETE|UPDATE|INSERT|ALTER|CREATE)\b/i];
+ if (headerSuspicious.some((p) => p.test(userAgent)) ||
+ headerSuspicious.some((p) => p.test(referer))) {
set.status = 400;
return { success: false, error: 'Malicious request detected', code: 'SECURITY_VIOLATION' };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Check for suspicious headers | |
| const userAgent = request.headers.get('user-agent') || ''; | |
| const referer = request.headers.get('referer') || ''; | |
| if (isInjectionAttempt(userAgent) || isInjectionAttempt(referer)) { | |
| set.status = 400; | |
| return { | |
| success: false, | |
| error: 'Malicious request detected', | |
| code: 'SECURITY_VIOLATION' | |
| }; | |
| } | |
| // Check for suspicious headers | |
| const userAgent = request.headers.get('user-agent') || ''; | |
| const referer = request.headers.get('referer') || ''; | |
| // Only flag headers containing high-signal XSS markers or stacked SQL queries | |
| const headerSuspicious = [ | |
| /javascript:/i, | |
| /<script/i, | |
| /;\s*(DROP|DELETE|UPDATE|INSERT|ALTER|CREATE)\b/i | |
| ]; | |
| if ( | |
| headerSuspicious.some((p) => p.test(userAgent)) || | |
| headerSuspicious.some((p) => p.test(referer)) | |
| ) { | |
| set.status = 400; | |
| return { | |
| success: false, | |
| error: 'Malicious request detected', | |
| code: 'SECURITY_VIOLATION' | |
| }; | |
| } |
🤖 Prompt for AI Agents
In apps/api/src/middleware/security.ts around lines 124 to 136, the current
aggressive scanning of full User-Agent and Referer headers produces false
positives; update the logic to skip scanning User-Agent entirely (or only allow
a very small whitelist) and restrict Referer checks to high-signal indicators
only: perform a case-insensitive substring check for "<script", "javascript:",
common SQL injection patterns like "UNION SELECT", ";" followed by another SQL
keyword (stacked queries), and "OR 1=1"; if any of these specific markers are
found set set.status = 400 and return the existing security violation response.
Ensure checks are simple substring matches (case-insensitive) and do not run
regexes over the full header values to avoid breaking on legitimate UA/Referer
strings.
| export function cspHeaders() { | ||
| return new Elysia() | ||
| .onAfterHandle(({ set }) => { | ||
| set.headers = { | ||
| ...set.headers, | ||
| 'Content-Security-Policy': [ | ||
| "default-src 'self'", | ||
| "script-src 'self' 'unsafe-inline' 'unsafe-eval'", | ||
| "style-src 'self' 'unsafe-inline'", | ||
| "img-src 'self' data: https:", | ||
| "font-src 'self' data:", | ||
| "connect-src 'self'", | ||
| "frame-src 'none'", | ||
| "object-src 'none'", | ||
| "media-src 'self'", | ||
| "worker-src 'self'", | ||
| "child-src 'none'", | ||
| "form-action 'self'", | ||
| "frame-ancestors 'none'", | ||
| "base-uri 'self'", | ||
| "upgrade-insecure-requests" | ||
| ].join('; '), | ||
| 'X-Content-Type-Options': 'nosniff', | ||
| 'X-Frame-Options': 'DENY', | ||
| 'X-XSS-Protection': '1; mode=block', | ||
| 'Referrer-Policy': 'strict-origin-when-cross-origin', | ||
| 'Permissions-Policy': [ | ||
| 'accelerometer=()', | ||
| 'camera=()', | ||
| 'geolocation=()', | ||
| 'gyroscope=()', | ||
| 'magnetometer=()', | ||
| 'microphone=()', | ||
| 'payment=()', | ||
| 'usb=()' | ||
| ].join(', ') | ||
| }; | ||
| }); | ||
| } No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
CSP is permissive ('unsafe-inline'/'unsafe-eval') and is applied to API responses.
- If any browser consumes these responses as documents (unlikely), this CSP weakens XSS defenses. For an API, CSP is generally unnecessary. If you keep it, tighten for production and keep permissive directives for development only.
Environment-gate the policy:
- 'Content-Security-Policy': [
- "default-src 'self'",
- "script-src 'self' 'unsafe-inline' 'unsafe-eval'",
- "style-src 'self' 'unsafe-inline'",
+ const isProd = process.env.NODE_ENV === 'production';
+ 'Content-Security-Policy': [
+ "default-src 'self'",
+ isProd
+ ? "script-src 'self'"
+ : "script-src 'self' 'unsafe-inline' 'unsafe-eval'",
+ isProd
+ ? "style-src 'self'"
+ : "style-src 'self' 'unsafe-inline'",
"img-src 'self' data: https:",
"font-src 'self' data:",
"connect-src 'self'",
"frame-src 'none'",
"object-src 'none'",
"media-src 'self'",
"worker-src 'self'",
"child-src 'none'",
"form-action 'self'",
"frame-ancestors 'none'",
"base-uri 'self'",
"upgrade-insecure-requests"
].join('; '),
- 'X-XSS-Protection': '1; mode=block',
+ // Optional/legacy: consider dropping X-XSS-Protection
+ // 'X-XSS-Protection': '1; mode=block',Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/api/src/middleware/security.ts around lines 175 to 213, the CSP
middleware currently applies a permissive policy (includes 'unsafe-inline' and
'unsafe-eval') to all API responses; change it to only apply a tightened CSP in
production and avoid applying CSP to non-document/API responses: detect NODE_ENV
(or an equivalent runtime flag) and return a no-op middleware in non-production,
remove 'unsafe-inline' and 'unsafe-eval' from the production policy, and ensure
the middleware only sets CSP for responses with a document/html content-type (or
other client-consumed document routes) so API JSON responses are not affected;
if needed, provide a separate relaxed policy behind an explicit DEV flag for
local development.
|
|
||
| // Additional validation of the final SQL | ||
| if (!this.isValidSQL(sql)) { | ||
| throw new Error('Generated SQL failed security validation'); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid validator drift: reuse the central SQL validator instead of a local copy.
There’s a dedicated validator in apps/api/src/agent/utils/sql-validator.ts. Duplicating regexes here risks divergence and gaps over time.
Apply this refactor:
@@
-import { chQuery } from '@databuddy/db';
+import { chQuery } from '@databuddy/db';
+import { validateSQL } from '../agent/utils/sql-validator';
@@
- // Additional validation of the final SQL
- if (!this.isValidSQL(sql)) {
+ // Additional validation of the final SQL
+ if (!validateSQL(sql)) {
throw new Error('Generated SQL failed security validation');
}
@@
- private isValidSQL(sql: string): boolean {
- // Basic SQL injection pattern detection
- const dangerousPatterns = [
- /;\s*(?:DROP|DELETE|UPDATE|INSERT|ALTER|CREATE|EXEC|EXECUTE)\b/i,
- /UNION\s+SELECT/i,
- /--[\s\S]*$/m,
- /\/\*[\s\S]*?\*\//g,
- /\bINTO\s+OUTFILE\b/i,
- /\bLOAD_FILE\b/i,
- ];
-
- return !dangerousPatterns.some(pattern => pattern.test(sql));
- }
+ // Validation now delegated to the shared validatorAlso applies to: 479-491
🤖 Prompt for AI Agents
In apps/api/src/query/simple-builder.ts around lines 469-474 and 479-491, the
code performs local SQL validation via this.isValidSQL which duplicates regex
logic; replace that with the central validator exported from
apps/api/src/agent/utils/sql-validator.ts by importing the validator function
(e.g., validateSQL or isSafeSQL) at the top of the file and calling it in place
of this.isValidSQL; remove or deprecate the local validator helper to avoid
duplication and ensure both validation sites use the same implementation, and
update error messages to include the central validator's result if it returns
details.
| // Security validation for field names | ||
| const ALLOWED_FILTER_FIELDS = [ | ||
| 'path', 'referrer', 'device_type', 'country', 'region', 'city', | ||
| 'browser_name', 'browser_version', 'os_name', 'os_version', | ||
| 'utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content', | ||
| 'event_name', 'title', 'language', 'screen_resolution' | ||
| ] as const; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce the allowlist for filterable fields (currently defined but unused).
You declare ALLOWED_FILTER_FIELDS but still accept any identifier-shaped field. Wire the allowlist into FilterSchema to reduce attack surface and prevent accidental querying of non-indexed/unsupported columns.
Apply this diff to bind the schema to the allowlist:
export const FilterSchema = t.Object({
- field: t.String({
- minLength: 1,
- maxLength: 50,
- pattern: '^[a-zA-Z_][a-zA-Z0-9_]*$'
- }),
+ field: t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))),
op: t.Enum({Also applies to: 16-22
🤖 Prompt for AI Agents
In apps/api/src/schemas/query-schemas.ts around lines 8-15 (and also apply same
change to lines 16-22), ALLOWED_FILTER_FIELDS is declared but not enforced in
the FilterSchema so any identifier-shaped field is currently accepted; update
FilterSchema to restrict the field key to the ALLOWED_FILTER_FIELDS by deriving
a zod enum (or union of literal types) from ALLOWED_FILTER_FIELDS and using that
enum for the key type instead of a generic identifier/string, ensuring the
schema only accepts those allowed field names and applying the same enforcement
wherever filter keys are validated.
| groupBy: t.Optional(t.String({ maxLength: 100, pattern: '^[a-zA-Z_][a-zA-Z0-9_]*$' })), | ||
| startDate: t.Optional(t.String({ maxLength: 50 })), | ||
| endDate: t.Optional(t.String({ maxLength: 50 })), | ||
| timeZone: t.Optional(t.String({ maxLength: 100 })), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Restrict DynamicQueryRequestSchema.groupBy to allowed fields.
Free-form identifiers here can drift from your supported column set and complicate downstream SQL construction. Reuse the same allowlist you just defined.
- groupBy: t.Optional(t.String({ maxLength: 100, pattern: '^[a-zA-Z_][a-zA-Z0-9_]*$' })),
+ groupBy: t.Optional(t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f])))),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| groupBy: t.Optional(t.String({ maxLength: 100, pattern: '^[a-zA-Z_][a-zA-Z0-9_]*$' })), | |
| startDate: t.Optional(t.String({ maxLength: 50 })), | |
| endDate: t.Optional(t.String({ maxLength: 50 })), | |
| timeZone: t.Optional(t.String({ maxLength: 100 })), | |
| }); | |
| groupBy: t.Optional(t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f])))), | |
| startDate: t.Optional(t.String({ maxLength: 50 })), | |
| endDate: t.Optional(t.String({ maxLength: 50 })), | |
| timeZone: t.Optional(t.String({ maxLength: 100 })), | |
| }); |
🤖 Prompt for AI Agents
In apps/api/src/schemas/query-schemas.ts around lines 73 to 77, the groupBy
field currently accepts any identifier string which can diverge from supported
columns; replace its free-form string type with the same allowlist/enum you
defined earlier (use that array to build a union/enum type) so groupBy is
Optional of only those allowed field names, remove the generic identifier
pattern constraint, and update any related types/exports so downstream SQL
construction only ever receives validated, allowed column names.
| groupBy: t.Optional(t.Array(t.String({ | ||
| maxLength: 100, | ||
| pattern: '^[a-zA-Z_][a-zA-Z0-9_]*$' | ||
| }), { maxItems: 20 })), | ||
| orderBy: t.Optional(t.String({ | ||
| maxLength: 200, | ||
| pattern: '^[a-zA-Z_][a-zA-Z0-9_, ]*[a-zA-Z0-9_]$' | ||
| })), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Model CompileRequestSchema.groupBy as an allowlisted array, not arbitrary identifiers.
Tightening groupBy across compile-time requests aligns with the filter-field hardening and avoids unexpected columns.
- groupBy: t.Optional(t.Array(t.String({
- maxLength: 100,
- pattern: '^[a-zA-Z_][a-zA-Z0-9_]*$'
- }), { maxItems: 20 })),
+ groupBy: t.Optional(
+ t.Array(
+ t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))),
+ { maxItems: 20 }
+ )
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| groupBy: t.Optional(t.Array(t.String({ | |
| maxLength: 100, | |
| pattern: '^[a-zA-Z_][a-zA-Z0-9_]*$' | |
| }), { maxItems: 20 })), | |
| orderBy: t.Optional(t.String({ | |
| maxLength: 200, | |
| pattern: '^[a-zA-Z_][a-zA-Z0-9_, ]*[a-zA-Z0-9_]$' | |
| })), | |
| groupBy: t.Optional( | |
| t.Array( | |
| t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))), | |
| { maxItems: 20 } | |
| ) | |
| ), |
| orderBy: t.Optional(t.String({ | ||
| maxLength: 200, | ||
| pattern: '^[a-zA-Z_][a-zA-Z0-9_, ]*[a-zA-Z0-9_]$' | ||
| })), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace free-form orderBy string with a structured, validated shape.
The current regex allows spaces and commas anywhere, which can lead to brittle parsing and subtle bugs. Prefer a typed array of { field, direction }, with field allowlisted.
- orderBy: t.Optional(t.String({
- maxLength: 200,
- pattern: '^[a-zA-Z_][a-zA-Z0-9_, ]*[a-zA-Z0-9_]$'
- })),
+ orderBy: t.Optional(
+ t.Array(
+ t.Object({
+ field: t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))),
+ direction: t.Optional(t.Enum({ asc: 'asc', desc: 'desc' }))
+ }),
+ { maxItems: 10 }
+ )
+ ),If changing the public request shape is not feasible right now, consider at least validating tokens server-side by splitting on commas and enforcing [field, optional direction] against the same allowlist before SQL generation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| orderBy: t.Optional(t.String({ | |
| maxLength: 200, | |
| pattern: '^[a-zA-Z_][a-zA-Z0-9_, ]*[a-zA-Z0-9_]$' | |
| })), | |
| orderBy: t.Optional( | |
| t.Array( | |
| t.Object({ | |
| field: t.Enum(Object.fromEntries(ALLOWED_FILTER_FIELDS.map((f) => [f, f]))), | |
| direction: t.Optional(t.Enum({ asc: 'asc', desc: 'desc' })) | |
| }), | |
| { maxItems: 10 } | |
| ) | |
| ), |
🤖 Prompt for AI Agents
In apps/api/src/schemas/query-schemas.ts around lines 98 to 101, replace the
free-form orderBy string with a validated structured shape: change the schema to
accept an array of objects { field: string, direction?: 'asc'|'desc' } where
field is validated against an allowlist of sortable column names and direction
is an enum defaulting to 'asc'; ensure the schema enforces max length for field
names and rejects unknown fields. If changing the public shape is not feasible,
add server-side validation for the existing string by splitting on commas,
trimming tokens, parsing each token into [field, optional direction], validating
field against the same allowlist and direction against {'asc','desc'} and reject
or sanitize any invalid tokens before SQL generation.
| // Encode HTML entities to prevent XSS attacks | ||
| let sanitized = input.trim(); | ||
| sanitized = sanitized.replace(/&/g, '&'); | ||
| sanitized = sanitized.replace(/</g, '<'); | ||
| sanitized = sanitized.replace(/>/g, '>'); | ||
| sanitized = sanitized.replace(/"/g, '"'); | ||
| sanitized = sanitized.replace(/'/g, '''); | ||
|
|
||
| return sanitized |
There was a problem hiding this comment.
HTML-encoding inside sanitizeString breaks URL validation and other non-HTML consumers.
validateUrl() passes the input through sanitizeString() and then constructs new URL(...). Encoding & to & (and quotes) turns many valid URLs invalid. This also alters session IDs, headers, etc., in unexpected ways.
Replace entity-encoding with a neutral sanitizer here, and perform HTML escaping at render time (or in a separate helper). Minimal fix:
- // Encode HTML entities to prevent XSS attacks
- let sanitized = input.trim();
- sanitized = sanitized.replace(/&/g, '&');
- sanitized = sanitized.replace(/</g, '<');
- sanitized = sanitized.replace(/>/g, '>');
- sanitized = sanitized.replace(/"/g, '"');
- sanitized = sanitized.replace(/'/g, ''');
-
- return sanitized
+ const sanitized = input.trim();
+ return sanitized
.slice(0, actualMaxLength)
.split('')
.filter((char) => {
const code = char.charCodeAt(0);
return !(
code <= 8 ||
code === 11 ||
code === 12 ||
(code >= 14 && code <= 31) ||
code === 127
);
})
.join('')
.replace(/\s+/g, ' ');If you need a safe HTML rendering helper, add alongside:
export function escapeHtml(str: string): string {
return str
.replace(/&/g, '&')
.replace(/</g, '<')
.replace(/>/g, '>')
.replace(/"/g, '"')
.replace(/'/g, ''');
}
|
Thanks for the feedback! I've addressed the issues with the SQL validation approach and sanitization methods as suggested:
These changes significantly improve security by tightening validation patterns while reducing false positives on legitimate inputs. |
3 participants
Description
Implement comprehensive security measures to prevent SQL injection and other potential vulnerabilities in the API
Changes
query-executor.tssql-validator.tswith robust injection preventionSummary by CodeRabbit
New Features
Security
Improvements
Tests