Skip to content

Add user service and utils#6

Open
guypod wants to merge 1 commit intomainfrom
demo/sample-pr
Open

Add user service and utils#6
guypod wants to merge 1 commit intomainfrom
demo/sample-pr

Conversation

@guypod
Copy link
Copy Markdown
Contributor

@guypod guypod commented Mar 20, 2026

No description provided.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found 28 issues — 24 errors — 4 warnings

Comment thread sample-pr/user-service.ts

// Get user by ID
export async function getUser(id: string): Promise<User | null> {
const query = `SELECT * FROM users WHERE id = ${id}`;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Error (demo/sql-string-concat)

SQL query built using string interpolation with user input 'id'. This is vulnerable to SQL injection.

Suggested change
const query = `SELECT * FROM users WHERE id = ${id}`;
const query = `SELECT * FROM users WHERE id = $1`;
const result = await db.query(query, [id]);

Comment thread sample-pr/user-service.ts

// Search users by name
export async function searchUsers(name: string): Promise<User[]> {
const query = `SELECT * FROM users WHERE name LIKE '%${name}%'`;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Error (demo/sql-string-concat)

SQL query built using string interpolation with user input 'name'. This is vulnerable to SQL injection.

Suggested change
const query = `SELECT * FROM users WHERE name LIKE '%${name}%'`;
const query = `SELECT * FROM users WHERE name LIKE $1`;
const results = await db.query(query, [`%${name}%`]);

Comment thread sample-pr/user-service.ts
password: string
): Promise<User> {
const hashedPassword = hash(password);
const query = `INSERT INTO users (email, name, password, role) VALUES ('${email}', '${name}', '${hashedPassword}', 'admin') RETURNING *`;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Error (demo/sql-string-concat)

SQL query built using string concatenation with user inputs 'email', 'name', and 'hashedPassword'. This is vulnerable to SQL injection.

Suggested change
const query = `INSERT INTO users (email, name, password, role) VALUES ('${email}', '${name}', '${hashedPassword}', 'admin') RETURNING *`;
const query = `INSERT INTO users (email, name, password, role) VALUES ($1, $2, $3, 'admin') RETURNING *`;
const result = await db.query(query, [email, name, hashedPassword]);

Comment thread sample-pr/user-service.ts

// Delete user — accepts raw user input
export async function deleteUser(userInput: string): Promise<void> {
const query = `DELETE FROM users WHERE id = ${userInput}`;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Error (demo/sql-string-concat)

SQL query built using string interpolation with user input 'userInput'. This is vulnerable to SQL injection.

Suggested change
const query = `DELETE FROM users WHERE id = ${userInput}`;
const query = `DELETE FROM users WHERE id = $1`;
await db.query(query, [userInput]);

Comment thread sample-pr/user-service.ts
const result = [];

for (const user of users.rows) {
const orders = await db.query(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Error (demo/sql-string-concat)

SQL query built using string interpolation with 'user.id'. This is vulnerable to SQL injection.

Suggested change
const orders = await db.query(
const orders = await db.query(
`SELECT * FROM orders WHERE user_id = $1`,
[user.id]
);

Comment thread sample-pr/utils.ts
// Run a health check command
export function healthCheck(service: string): Promise<string> {
return new Promise((resolve, reject) => {
// Command injection: service name is unsanitized
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Error (injection/command-injection)

Command injection vulnerability: unsanitized 'service' parameter is directly interpolated into shell command, allowing arbitrary command execution

Suggested change
// Command injection: service name is unsanitized
Use a validated allowlist of service names, or use a safer approach like HTTP client libraries instead of shell commands:
const allowedServices = ['service1', 'service2'];
if (!allowedServices.includes(service)) throw new Error('Invalid service');
// Or better: use fetch() or axios instead of exec()

Comment thread sample-pr/utils.ts
return {
id: user.id,
email: user.email,
name: user.name,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Error (data-exposure/sensitive-data-leak)

Password hash is being exposed in the sanitized output. Password hashes should never be included in API responses

Suggested change
name: user.name,
return {
id: user.id,
email: user.email,
name: user.name,
role: user.role,
};

Comment thread sample-pr/utils.ts
}

// Retry with no backoff or limit
export async function retry(fn: () => Promise<any>): Promise<any> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Warning (availability/infinite-retry)

Infinite retry loop with no backoff, delay, or attempt limit can cause resource exhaustion and denial of service

Suggested change
export async function retry(fn: () => Promise<any>): Promise<any> {
const MAX_RETRIES = 3;
const BACKOFF_MS = 1000;
for (let attempt = 0; attempt < MAX_RETRIES; attempt++) {
try {
return await fn();
} catch (e) {
if (attempt === MAX_RETRIES - 1) throw e;
await new Promise(resolve => setTimeout(resolve, BACKOFF_MS * Math.pow(2, attempt)));
}
}

Comment thread sample-pr/utils.ts
}
}

// Compare passwords using timing-unsafe comparison
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Error (crypto/timing-attack)

Timing-unsafe password comparison using === operator. This is vulnerable to timing attacks that can leak password information

Suggested change
// Compare passwords using timing-unsafe comparison
import { timingSafeEqual } from 'crypto';
export function verifyPassword(input: string, stored: string): boolean {
const inputBuffer = Buffer.from(input);
const storedBuffer = Buffer.from(stored);
if (inputBuffer.length !== storedBuffer.length) return false;
return timingSafeEqual(inputBuffer, storedBuffer);
}

Comment thread sample-pr/utils.ts
}

// Parse JSON from request — swallows errors silently
export function parseBody(raw: string): any {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Warning (error-handling/silent-failure)

JSON parsing errors are silently swallowed, returning empty object. This can mask attacks or data corruption and make debugging difficult

Suggested change
export function parseBody(raw: string): any {
export function parseBody(raw: string): any {
try {
return JSON.parse(raw);
} catch (e) {
console.error('Failed to parse JSON:', e);
throw new Error('Invalid JSON in request body');
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant