Security and Performance: The Review Checklist You Can't Skip
Security bugs pass testing. They pass functional verification. They even pass code review and deploy to production. Then they wait quietly, undetected, until they become your 3 AM incident that escalates to C-level executives.
According to an IBM study, fixing security flaws in the development phase costs 15 times less than fixing them in production. The mathematics is clear and unambiguous: catch them in code review or pay much more later. There's no middle ground.
The Three Most Common Security Vulnerabilities
SQL Injection: Detecting It is Simple
The detection rule is straightforward: check whether external input is directly entered into SQL strings.
async function searchUsers(keyword: string) {
const result = await db.query(
`SELECT * FROM users WHERE name LIKE '%${keyword}%'`
);
return result.rows;
}
What happens if keyword is `%'; DROP TABLE users; --`? The query becomes:
SELECT * FROM users WHERE name LIKE '%%'; DROP TABLE users; --'%'
The table is deleted. All user data is lost.
The fix: Use parameterized queries. The value becomes data, never SQL:
const result = await db.query(
'SELECT * FROM users WHERE name LIKE $1',
[`%${keyword}%`]
);
The database driver ensures `keyword` is always treated as data, not executable SQL code.
XSS: Stop and Check When You See "dangerously"
function CommentDisplay({ comment }: { comment: string }) {
return (
<div dangerouslySetInnerHTML={{ __html: comment }} />
);
}
If a user stores `` in their comment, every user viewing this component has their cookies stolen and sent to an attacker.
The name "dangerouslySetInnerHTML" exists for a reason — it's a warning sign. Every time you see it, stop and check.
The fix: If you just want to display text, let React escape it automatically:
<div>{comment}</div> // React escapes HTML entities automatically
If HTML markup is truly needed, sanitize with a library like DOMPurify.
Missing Authentication: URL Security Is An Illusion
@RestController
@RequestMapping("/admin")
public class AdminController {
@GetMapping("/users")
public List<User> getAllUsers() {
return userService.findAll(); // no authentication check!
}
@DeleteMapping("/users/{id}")
public void deleteUser(@PathVariable String id) {
userService.delete(id); // anyone can delete
}
}
Just because the URL says "/admin" doesn't mean it's protected. URLs aren't security mechanisms. An unauthenticated user can call this endpoint and view all users or delete them.
Reviewers must ask: Who can call this API? Is that what you intended?
The N+1 Problem: Invisible at First, Disastrous at Scale
Code performs fine with 10 users. Becomes slow with 1,000 users. Crashes with 100,000 users. The culprit is usually the N+1 query problem — the most common performance bug:
public List<OrderSummary> getOrderSummaries() {
List<Order> orders = orderRepository.findAll(); // query 1
return orders.stream()
.map(order -> new OrderSummary(
order.getId(),
order.getUser().getName(), // query N (lazy-loads User)
order.getItems().size() // query N (lazy-loads Items)
))
.collect(Collectors.toList());
}
If there are 1,000 orders, this executes 2,001 queries. One query to fetch all orders, then one additional query for each order's user and items. In your development environment with 10 test orders? Imperceptible. In production with 100,000? The database melts.
The fix: Load related data upfront using JOIN FETCH:
@Query("SELECT o FROM Order o JOIN FETCH o.user JOIN FETCH o.items")
List<Order> findAllWithUserAndItems();
Security and Performance Review Checklist
Input-Related Security
- Are external values prevented from being inserted directly into SQL?
- Are external values prevented from rendering as HTML without escaping?
- When user input enters file paths, is path traversal prevented?
- Are numeric parameters validated for negatives, zero, and extreme values?
- Are string lengths validated?
Authentication/Authorization
- Does the new endpoint require authentication? If so, is it applied?
- Can authenticated users access only their own data? (horizontal privilege escalation)
- Are admin functions inaccessible without role checks?
- Is JWT validation performed on the server side?
Sensitive Data
- Are passwords never stored or compared in plaintext?
- Are sensitive values (passwords, tokens, credit cards) kept out of logs?
- Do API responses avoid including more information than necessary?
- Do error responses avoid exposing stack traces or internal paths?
Performance
- Is there a DB query in a loop? (N+1 problem)
- Is all data being loaded without pagination?
- Are there file size limits on uploads?
- Are there request timeouts?
- Is caching considered where appropriate?
Real-World Examples: Security Vulnerabilities in Production
History is full of security breaches that could have been caught in code review. In 2017, a healthcare company exposed 10 million patient records because an API endpoint had no authentication checks. In 2019, a financial services company lost millions to fraud because of N+1 queries that caused timeouts, allowing attackers to manipulate transaction timing. In 2020, a major retailer was breached through a forgotten XSS vulnerability that had been dormant in an admin panel for three years.
These weren't cases of complex, unknown vulnerabilities. They were all detectable in code review by someone asking the right questions: "Who can call this API?" "Are we validating external input?" "How does this scale?"
Communicating Performance Issues in Review
When you find a performance issue, be specific about the impact:
Bad comment: "This won't scale."
Good comment: "[Suggestion] This fetches all users every time. With 1 million users in production, this will query 1MB+ of data just to update one user's profile. Consider paginating or caching. Also, the loop loads each user's orders separately (N+1 problem). This would be 1,000,001 queries for 1,000 users. Consider loading orders with JOIN FETCH."
The good comment shows you've thought through the scale, the data volume, and the specific performance impact. It helps the author understand why this matters and what to fix.
Performance Impact: From Development to Production
The performance issue is insidious because it doesn't fail in development. A developer writes code that works fine with 100 test records. In production with 10 million records, it times out. By then, customers are frustrated, support tickets are piling up, and the developer is under pressure to release a fix.
The N+1 problem is particularly common:
- Development: Works fine with 10 orders (11 queries total)
- Staging: Slightly slow with 1,000 orders (1,001 queries total, ~500ms)
- Production: Unusable with 1,000,000 orders (1,000,001 queries, 45+ seconds)
This is why reviewers must understand the production scale. A solution that works for thousands of records might be a disaster for millions.
Horizontal Privilege Escalation: The Overlooked Vulnerability
One of the most overlooked security issues in code review is horizontal privilege escalation. Authentication exists (user is logged in), but authorization is missing (user accesses someone else's data):
// Vulnerable: only checks authentication, not authorization
@GetMapping("/orders/{orderId}")
public Order getOrder(@PathVariable String orderId,
@AuthenticationPrincipal UserDetails currentUser) {
return orderRepository.findById(orderId)
.orElseThrow(() -> new OrderNotFoundException(orderId));
// Doesn't verify that the order belongs to the current user!
}
A logged-in user can view ANY order if they know the ID. This is a privilege escalation bug that passes authentication but fails authorization.
The fix: Always verify ownership or role:
@GetMapping("/orders/{orderId}")
public Order getOrder(@PathVariable String orderId,
@AuthenticationPrincipal UserDetails currentUser) {
Order order = orderRepository.findById(orderId)
.orElseThrow(() -> new OrderNotFoundException(orderId));
// Verify user owns this order
if (!order.getUserId().equals(currentUser.getId())) {
throw new ForbiddenException("You don't have access to this order");
}
return order;
}
Communicating Security Issues Effectively
When you find a vulnerability, write comments with:
- What happens. "If the id value is an external input, a query like `DELETE FROM users WHERE id = 'x' OR '1'='1'` could execute."
- Why it's dangerous. "All user data would be deleted. The organization would face data loss, GDPR compliance issues, and reputational damage."
- How to fix it. "Use parameterized queries: `db.query('DELETE FROM users WHERE id = $1', [id]);`"
- Who should know. Alert relevant stakeholders if it's critical.
This teaches the author, not just commands them to fix it. Severity matters — a typo fix is different from a data loss vulnerability.
Additional Security Patterns: CORS and CSRF
Two more security vulnerabilities commonly miss review:
CORS (Cross-Origin Resource Sharing) Misconfiguration:
// Dangerous - allows anyone to access
app.use(cors({ origin: '*' }));
// Better - whitelist specific origins
app.use(cors({
origin: ['https://trusted-domain.com', 'https://app.trusted-domain.com'],
credentials: true
}));
CSRF (Cross-Site Request Forgery) Protection:
// Form should include CSRF token
<form method="POST" action="/transfer-funds">
<input type="hidden" name="_csrf" value="{{ csrfToken }}" />
<input type="number" name="amount" />
<button type="submit">Transfer</button>
</form>
// Server verifies token
app.post('/transfer-funds', (req, res) => {
if (req.body._csrf !== req.session.csrfToken) {
return res.status(403).json({ error: 'CSRF token invalid' });
}
// Process transfer
});
Security and Performance: Not Bonus Checks
Don't review security and performance "as a bonus" after feature review. These areas deserve dedicated, focused attention. According to industry data and OWASP reports, most production failures come from security flaws or performance degradation, not from logic errors or typos.
Budget the time explicitly. Spend 10-15 more minutes on security and performance review. Create a separate checklist if needed. Your ops team and your customers will thank you when the system doesn't crash or get compromised. The cost of a single security breach usually exceeds years' worth of careful review time.
Performance Red Flags in Code Review
Beyond the N+1 problem, watch for:
- Loading entire datasets: `findAll()` without pagination when working with large tables
- Repeated calculations: Same computation in a loop that could be calculated once
- Unnecessary object creation: Creating large objects in loops when one object could be reused
- Blocking calls in async code: `Thread.sleep()` or blocking I/O in async/await contexts
- Unbounded queues: Adding to queues without size limits that could cause memory exhaustion
- String concatenation in loops: `str += item` is O(n²); use StringBuilder instead
- Missing indexes: Filtering on columns without database indexes
Comments
Post a Comment