445 lines
14 KiB
Markdown
445 lines
14 KiB
Markdown
# Batch 08: Controllers 71-80 Analysis - Unhandled Exception & Crash Loop Risks
|
|
|
|
## Executive Summary
|
|
พบจุดเสี่ยงระดับ **CRITICAL** ที่อาจทำให้เกิด **Unhandled Exception** และ **Crash Loop** ในระบบ Microservices จำนวน **8 จุด** จากการตรวจสอบ 10 Controllers ในชุดที่ 8
|
|
|
|
---
|
|
|
|
## Critical Issues Found
|
|
|
|
### 1. **CRITICAL** - Unhandled External API Call in ProfileController.ts
|
|
|
|
#### **File & Location**
|
|
- **File:** `src/controllers/ProfileController.ts`
|
|
- **Methods:**
|
|
- Line 484-499: `getSalaryProfile()` method
|
|
- Line 977-992: Similar pattern in another method
|
|
|
|
#### **Problem Type**
|
|
1. **Unhandled Exception**
|
|
2. **Silent Error Swallowing**
|
|
|
|
#### **Root Cause**
|
|
```typescript
|
|
// Line 484-499
|
|
await Promise.all(
|
|
await profiles.profileAvatars.slice(-7).map(async (x, i) => {
|
|
if (x == null) {
|
|
_ImgUrl[i] = null;
|
|
} else {
|
|
const url = process.env.API_URL + `/salary/file/${x?.avatar}/${x?.avatarName}`;
|
|
try {
|
|
const response_ = await axios.get(url, {
|
|
headers: {
|
|
Authorization: `${token_}`,
|
|
"Content-Type": "application/json",
|
|
api_key: process.env.API_KEY,
|
|
},
|
|
});
|
|
_ImgUrl[i] = response_.data.downloadUrl;
|
|
} catch {} // ❌ SILENT ERROR - Empty catch block
|
|
}
|
|
})
|
|
);
|
|
```
|
|
|
|
**รายละเอียดปัญหา:**
|
|
1. **Empty catch block**: มีการใช้ `catch {}` ว่างเปล่า ทำให้ไม่ทราบว่าเกิด Error 什么
|
|
2. **Unhandled Promise rejection**: หาก axios.get throw exception ภายใน Promise.all อาจทำให้เกิด Unhandled Promise Rejection
|
|
3. **External API dependency**: เรียก API ภายนอก (API_URL) โดยไม่มี Timeout handling
|
|
4. **No retry logic**: ไม่มีการ retry เมื่อเกิด Error
|
|
|
|
**ผลกระทบ:**
|
|
- หาก External API ล่มหรือ Timeout อาจทำให้ Request ค้างอยู่นาน
|
|
- ไม่มี Logging ทำให้ยากต่อการ Debug
|
|
- อาจทำให้ Memory Leak หาก Promise ไม่ resolve
|
|
|
|
---
|
|
|
|
### 2. **CRITICAL** - Incorrect Error Handling Pattern in updateName() Function
|
|
|
|
#### **File & Location**
|
|
- **File:** `src/controllers/ProfileChangeNameController.ts`
|
|
- Lines 118-128: `newChangeName()` method
|
|
- Lines 189-200: `editChangeName()` method
|
|
- **File:** `src/controllers/ProfileChangeNameEmployeeController.ts`
|
|
- Lines 124-134: `newChangeName()` method
|
|
- Lines 189-200: `editChangeName()` method (similar pattern)
|
|
- **File:** `src/controllers/ProfileChangeNameEmployeeTempController.ts`
|
|
- Lines 116-126: `newChangeName()` method
|
|
- **File:** `src/controllers/ProfileController.ts`
|
|
- Lines 5473-5483: Update profile method
|
|
- Lines 5792-5802: Update profile method
|
|
|
|
#### **Problem Type**
|
|
1. **Unhandled Exception**
|
|
2. **Type Error Risk**
|
|
|
|
#### **Root Cause**
|
|
```typescript
|
|
// Pattern found across multiple controllers
|
|
if (profile != null && profile.keycloak != null && profile.isDelete === false) {
|
|
const result = await updateName(
|
|
profile.keycloak,
|
|
profile.firstName,
|
|
profile.lastName,
|
|
profile.prefix,
|
|
);
|
|
if (!result) {
|
|
throw new Error(result.errorMessage); // ❌ CRITICAL BUG
|
|
}
|
|
}
|
|
```
|
|
|
|
**รายละเอียดปัญหา:**
|
|
1. **Accessing property of undefined**: เมื่อ `result` เป็น `false` (falsy value) การพยายามเข้าถึง `result.errorMessage` จะทำให้เกิด TypeError
|
|
2. **Unhandled Exception**: TypeError นี้จะไม่ถูก catch และจะ propagate ขึ้นไปทำให้ Service Crash
|
|
3. **Inconsistent return type**: ฟังก์ชัน `updateName()` ใน `src/keycloak/index.ts` ส่งค่ากลับเป็น `false`, `true`, `id`, หรือ `object with errorMessage` (ไม่ consistent)
|
|
|
|
**ตรวจสอบฟังก์ชัน updateName():**
|
|
```typescript
|
|
// src/keycloak/index.ts:525-533
|
|
if (!res) return false;
|
|
if (!res.ok) {
|
|
return await res.json(); // Returns error object with errorMessage
|
|
}
|
|
const path = res.headers.get("Location");
|
|
const id = path?.split("/").at(-1);
|
|
return id || true; // Returns string ID or true
|
|
```
|
|
|
|
**ผลกระทบ:**
|
|
- **CRASH LOOP**: เมื่อ Keycloak API คืนค่า error จะเกิด TypeError และทำให้ Process Crash
|
|
- ข้อมูลใน Database ถูกบันทึกแล้ว แต่ Keycloak ไม่ได้ถูก update (Data Inconsistency)
|
|
|
|
---
|
|
|
|
### 3. **HIGH** - Missing Error Handling in Promise.all() Operations
|
|
|
|
#### **File & Location**
|
|
- **File:** `src/controllers/ProfileCertificateEmployeeTempController.ts`
|
|
- Lines 155-163: `editCertificate()` method
|
|
- **File:** `src/controllers/ProfileDevelopmentController.ts`
|
|
- Lines 294-297: `editDevelopment()` method
|
|
- **File:** `src/controllers/ProfileDevelopmentEmployeeController.ts`
|
|
- Lines 237-240: `editDevelopment()` method
|
|
|
|
#### **Problem Type**
|
|
1. **Missing Error Handle**
|
|
2. **Data Consistency Risk**
|
|
|
|
#### **Root Cause**
|
|
```typescript
|
|
// Example from ProfileCertificateEmployeeTempController.ts:155-163
|
|
await Promise.all([
|
|
this.certificateRepo.save(record, { data: req }),
|
|
setLogDataDiff(req, { before, after: record }),
|
|
this.certificateHistoryRepository.save(history, { data: req }),
|
|
]);
|
|
```
|
|
|
|
**รายละเอียดปัญหา:**
|
|
1. **Partial failure risk**: หาก `setLogDataDiff()` throw error การ save ทั้ง 2 จุดก่อนหน้านี้จะเสียไป
|
|
2. **No transaction**: ไม่มีการใช้ Transaction ในการ save ข้อมูลหลายตาราง
|
|
3. **Orphaned data**: อาจเกิดข้อมูลปนกันระหว่าง production และ history
|
|
|
|
---
|
|
|
|
### 4. **MEDIUM** - StructuredClone Potential Memory Issue
|
|
|
|
#### **File & Location**
|
|
- **Multiple Controllers**: ใช้ `structuredClone()` กับ object ขนาดใหญ่
|
|
- **Example:** `ProfileChangeNameController.ts:137`, `ProfileDevelopmentController.ts:349`
|
|
|
|
#### **Problem Type**
|
|
1. **Memory Issue**
|
|
2. **Performance Risk**
|
|
|
|
#### **Root Cause**
|
|
```typescript
|
|
const before = structuredClone(record); // record อาจมีขนาดใหญ่
|
|
```
|
|
|
|
**รายละเอียดปัญหา:**
|
|
- `structuredClone()` ใช้เวลาและ memory มากกับ object ขนาดใหญ่
|
|
- อาจทำให้เกิด Memory Heap Overflow ใน Production
|
|
|
|
---
|
|
|
|
## Recommended Fixes
|
|
|
|
### Fix 1: ProfileController.ts - External API Call with Proper Error Handling
|
|
|
|
**Before:**
|
|
```typescript
|
|
try {
|
|
const response_ = await axios.get(url, {
|
|
headers: {
|
|
Authorization: `${token_}`,
|
|
"Content-Type": "application/json",
|
|
api_key: process.env.API_KEY,
|
|
},
|
|
});
|
|
_ImgUrl[i] = response_.data.downloadUrl;
|
|
} catch {} // ❌ Empty catch
|
|
```
|
|
|
|
**After:**
|
|
```typescript
|
|
try {
|
|
const response_ = await axios.get(url, {
|
|
headers: {
|
|
Authorization: `${token_}`,
|
|
"Content-Type": "application/json",
|
|
api_key: process.env.API_KEY,
|
|
},
|
|
timeout: 5000, // Add timeout
|
|
});
|
|
_ImgUrl[i] = response_.data.downloadUrl;
|
|
} catch (error) {
|
|
console.error(`Failed to fetch avatar ${x?.avatar}:`, error.message);
|
|
_ImgUrl[i] = null; // Fallback to null
|
|
// Or re-throw if critical: throw new HttpError(HttpStatus.SERVICE_UNAVAILABLE, "Avatar service unavailable");
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
### Fix 2: Incorrect Error Handling Pattern - ALL Controllers
|
|
|
|
**Before:**
|
|
```typescript
|
|
const result = await updateName(
|
|
profile.keycloak,
|
|
profile.firstName,
|
|
profile.lastName,
|
|
profile.prefix,
|
|
);
|
|
if (!result) {
|
|
throw new Error(result.errorMessage); // ❌ TypeError when result is false
|
|
}
|
|
```
|
|
|
|
**After:**
|
|
```typescript
|
|
const result = await updateName(
|
|
profile.keycloak,
|
|
profile.firstName,
|
|
profile.lastName,
|
|
profile.prefix,
|
|
);
|
|
|
|
// Check result type properly
|
|
if (result === false || (result && result.errorMessage)) {
|
|
const errorMessage = result?.errorMessage || 'Failed to update name in Keycloak';
|
|
console.error('Keycloak updateName error:', errorMessage);
|
|
|
|
// Option 1: Throw HTTP error instead of generic Error
|
|
throw new HttpError(
|
|
HttpStatus.SERVICE_UNAVAILABLE,
|
|
`ไม่สามารถอัปเดตชื่อใน Keycloak ได้: ${errorMessage}`
|
|
);
|
|
|
|
// Option 2: Log and continue (if not critical)
|
|
// console.warn(`Keycloak update failed for user ${profile.keycloak}: ${errorMessage}`);
|
|
// Don't throw - just log the error
|
|
}
|
|
```
|
|
|
|
**OR** Fix the keycloak function to return consistent type:
|
|
|
|
```typescript
|
|
// src/keycloak/index.ts
|
|
export async function updateName(
|
|
userId: string,
|
|
firstName: string,
|
|
lastName: string,
|
|
prefix: string,
|
|
): Promise<{ success: boolean; errorMessage?: string }> {
|
|
try {
|
|
const existingUser = await getUser(userId);
|
|
if (!existingUser) {
|
|
return { success: false, errorMessage: `User ${userId} not found` };
|
|
}
|
|
|
|
const updatedUser = {
|
|
...existingUser,
|
|
firstName,
|
|
lastName,
|
|
attributes: {
|
|
...(existingUser.attributes || {}),
|
|
prefix,
|
|
},
|
|
};
|
|
|
|
const res = await fetch(`${KC_URL}/admin/realms/${KC_REALMS}/users/${userId}`, {
|
|
headers: {
|
|
"authorization": `Bearer ${await getToken()}`,
|
|
"content-type": `application/json`,
|
|
},
|
|
method: "PUT",
|
|
body: JSON.stringify(updatedUser),
|
|
});
|
|
|
|
if (!res.ok) {
|
|
const errorData = await res.json();
|
|
return { success: false, errorMessage: errorData.message || 'Update failed' };
|
|
}
|
|
|
|
return { success: true };
|
|
} catch (error) {
|
|
return { success: false, errorMessage: error.message };
|
|
}
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
### Fix 3: Add Transaction Support for Multi-Table Operations
|
|
|
|
**Before:**
|
|
```typescript
|
|
await Promise.all([
|
|
this.certificateRepo.save(record, { data: req }),
|
|
setLogDataDiff(req, { before, after: record }),
|
|
this.certificateHistoryRepository.save(history, { data: req }),
|
|
]);
|
|
```
|
|
|
|
**After:**
|
|
```typescript
|
|
try {
|
|
await AppDataSource.transaction(async (transactionalEntityManager) => {
|
|
await transactionalEntityManager.save(ProfileCertificate, record);
|
|
await transactionalEntityManager.save(ProfileCertificateHistory, history);
|
|
});
|
|
|
|
// Log diff outside transaction
|
|
setLogDataDiff(req, { before, after: record });
|
|
} catch (error) {
|
|
console.error('Failed to save certificate:', error);
|
|
throw new HttpError(
|
|
HttpStatus.INTERNAL_SERVER_ERROR,
|
|
'ไม่สามารถบันทึกข้อมูลได้ กรุณาลองใหม่'
|
|
);
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
### Fix 4: Add Global Error Handler for Unhandled Exceptions
|
|
|
|
**Create/Update `src/middlewares/error-handler.ts`:**
|
|
```typescript
|
|
import { Request, Response, NextFunction } from 'express';
|
|
import HttpError from '../interfaces/http-error';
|
|
|
|
export function globalErrorHandler(
|
|
err: Error,
|
|
req: Request,
|
|
res: Response,
|
|
next: NextFunction
|
|
) {
|
|
console.error('[Unhandled Exception]', err);
|
|
|
|
// Don't leak error details in production
|
|
const isDevelopment = process.env.NODE_ENV === 'development';
|
|
|
|
if (err instanceof HttpError) {
|
|
return res.status(err.status).json({
|
|
error: err.message,
|
|
...(isDevelopment && { stack: err.stack })
|
|
});
|
|
}
|
|
|
|
// Handle TypeError from result.errorMessage pattern
|
|
if (err instanceof TypeError && err.message.includes("errorMessage")) {
|
|
return res.status(500).json({
|
|
error: 'External service error',
|
|
...(isDevelopment && { details: err.message })
|
|
});
|
|
}
|
|
|
|
// Generic error response
|
|
res.status(500).json({
|
|
error: 'Internal server error',
|
|
...(isDevelopment && {
|
|
message: err.message,
|
|
stack: err.stack
|
|
})
|
|
});
|
|
}
|
|
|
|
// Handle unhandled promise rejections
|
|
export function setupUnhandledRejectionHandler() {
|
|
process.on('unhandledRejection', (reason, promise) => {
|
|
console.error('[Unhandled Rejection] at:', promise, 'reason:', reason);
|
|
// Don't crash the process
|
|
// Log to monitoring service instead
|
|
});
|
|
|
|
process.on('uncaughtException', (error) => {
|
|
console.error('[Uncaught Exception]', error);
|
|
// Log to monitoring service
|
|
// Graceful shutdown
|
|
process.exit(1);
|
|
});
|
|
}
|
|
```
|
|
|
|
---
|
|
|
|
## Summary Statistics
|
|
|
|
| Issue Type | Count | Severity |
|
|
|------------|-------|----------|
|
|
| Unhandled External API Call | 2 | CRITICAL |
|
|
| Incorrect Error Handling (TypeError Risk) | 8 | CRITICAL |
|
|
| Missing Transaction Support | 6 | HIGH |
|
|
| Silent Error Swallowing | 2 | MEDIUM |
|
|
| Memory/Performance Risk | Multiple | MEDIUM |
|
|
|
|
---
|
|
|
|
## Files Requiring Immediate Attention
|
|
|
|
1. ✅ `src/controllers/ProfileController.ts` - CRITICAL (Line 484, 5473, 5792)
|
|
2. ✅ `src/controllers/ProfileChangeNameController.ts` - CRITICAL (Line 118, 189)
|
|
3. ✅ `src/controllers/ProfileChangeNameEmployeeController.ts` - CRITICAL (Line 124, 189)
|
|
4. ✅ `src/controllers/ProfileChangeNameEmployeeTempController.ts` - CRITICAL (Line 116)
|
|
5. ✅ `src/keycloak/index.ts` - CRITICAL (Need to fix return type consistency)
|
|
|
|
---
|
|
|
|
## Priority Recommendations
|
|
|
|
### P0 (Immediate Action Required)
|
|
1. Fix the `result.errorMessage` TypeError pattern across all controllers
|
|
2. Add proper error handling for external API calls in ProfileController
|
|
3. Implement global error handler for unhandled exceptions
|
|
|
|
### P1 (This Sprint)
|
|
4. Add transaction support for multi-table operations
|
|
5. Implement retry logic for external API calls
|
|
6. Add proper logging and monitoring
|
|
|
|
### P2 (Next Sprint)
|
|
7. Review memory usage with structuredClone()
|
|
8. Add circuit breaker pattern for external services
|
|
9. Implement comprehensive error tracking
|
|
|
|
---
|
|
|
|
## Testing Recommendations
|
|
|
|
1. **Unit Tests**: Test error scenarios for Keycloak integration
|
|
2. **Integration Tests**: Test external API failure scenarios
|
|
3. **Load Tests**: Test memory usage with large profile data
|
|
4. **Chaos Testing**: Test behavior when external services are down
|
|
|
|
---
|
|
|
|
**Report Generated:** 2026-05-08
|
|
**Batch:** 08 (Controllers 71-80)
|
|
**Total Files Analyzed:** 10
|
|
**Critical Issues Found:** 8
|