-
Notifications
You must be signed in to change notification settings - Fork 13
Security/fix api token auth #975
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: rilis-dev
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,9 +59,7 @@ public function login(Request $request) | |
| $user = User::where('email', $request['email'])->firstOrFail(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH] 🔒 Security: Potential Log Injection via Unsanitized Headers Masalah: Activity log mencatat Kode: ->withProperties([
'ip' => $request->ip(),
'user_agent' => $request->userAgent(),
'token_abilities' => ['synchronize-opendk-create']
])Risiko:
PoC (Chrome Console): // Jalankan di Chrome DevTools Console (F12 → Console)
// Pastikan sudah login sebagai administrator
// Login sebagai admin
const loginResp = await fetch('/api/v1/login', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ username: 'admin', password: 'password123' })
});
const { access_token } = await loginResp.json();
// Attack 1: IP Spoofing via X-Forwarded-For
const spoofedResp = await fetch('/api/v1/token', {
method: 'GET',
headers: {
'Authorization': `Bearer ${access_token}`,
'Accept': 'application/json',
'X-Forwarded-For': '127.0.0.1, 8.8.8.8, 1.1.1.1' // Spoof multiple IPs
}
});
console.log('IP Spoofing:', await spoofedResp.json());
// Attack 2: User-Agent Injection dengan XSS payload
const xssResp = await fetch('/api/v1/token', {
method: 'GET',
headers: {
'Authorization': `Bearer ${access_token}`,
'Accept': 'application/json',
'User-Agent': '<script>alert("XSS in logs")</script>\nFake-Log-Entry: admin logged out'
}
});
console.log('XSS Injection:', await xssResp.json());
// Attack 3: Log Injection dengan newline characters
const logInjectionResp = await fetch('/api/v1/token', {
method: 'GET',
headers: {
'Authorization': `Bearer ${access_token}`,
'Accept': 'application/json',
'User-Agent': 'Mozilla/5.0\n[2026-04-01 10:00:00] FAKE: User admin deleted all data\n'
}
});
console.log('Log Injection:', await logInjectionResp.json());
// Attack 4: Overflow dengan User-Agent yang sangat panjang
const longUA = 'A'.repeat(10000); // 10KB User-Agent
const overflowResp = await fetch('/api/v1/token', {
method: 'GET',
headers: {
'Authorization': `Bearer ${access_token}`,
'Accept': 'application/json',
'User-Agent': longUA
}
});
console.log('Overflow Attack:', await overflowResp.json());
// Semua attack ini akan tercatat di activity log tanpa sanitasi
// Bisa digunakan untuk:
// 1. Menyembunyikan IP asli attacker
// 2. Inject fake log entries
// 3. XSS jika log ditampilkan di web
// 4. DoS dengan membanjiri log dengan data besarFix: public function token(Request $request)
{
// Hapus token lama
Auth::user()->tokens()->delete();
// Buat token baru untuk user synchronize
$user = User::where('username', 'synchronize')->first();
if (!$user) {
return response()->json(['message' => 'Synchronize user not found'], 404);
}
$token = $user->createToken('synchronize-token', ['synchronize-opendk-create'])->plainTextToken;
// Sanitasi IP: validasi dan ambil IP yang reliable
$ip = $request->ip();
$sanitizedIp = filter_var($ip, FILTER_VALIDATE_IP) ?: 'invalid';
// Jika menggunakan proxy, validasi X-Forwarded-For dengan whitelist
if ($request->header('X-Forwarded-For')) {
$forwardedIps = explode(',', $request->header('X-Forwarded-For'));
$trustedProxies = config('trustedproxies.proxies', []);
// Hanya ambil IP pertama jika dari trusted proxy
if (in_array($request->server('REMOTE_ADDR'), $trustedProxies)) {
$sanitizedIp = filter_var(trim($forwardedIps[0]), FILTER_VALIDATE_IP) ?: $sanitizedIp;
}
}
// Sanitasi User-Agent: strip tags, limit length, remove newlines
$userAgent = $request->userAgent() ?? 'unknown';
$sanitizedUA = htmlspecialchars(
substr(str_replace(["\n", "\r", "\t"], '', $userAgent), 0, 255),
ENT_QUOTES,
'UTF-8'
);
// Log aktivitas dengan data yang sudah disanitasi
activity()
->performedOn($user)
->causedBy(auth()->user())
->withProperties([
'ip' => $sanitizedIp,
'user_agent' => $sanitizedUA,
'token_abilities' => ['synchronize-opendk-create'],
'request_id' => $request->header('X-Request-ID', uniqid('req_', true))
])
->log('token.generated');
return response()->json([
'access_token' => $token,
'token_type' => 'Bearer',
]);
}There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [CRITICAL] ⚡ Performance: Missing Cache untuk Static User Query Masalah: Query Kode: Dampak:
Fix: // Gunakan cache dengan TTL panjang (1 hari atau lebih)
$user = Cache::remember('user.synchronize', 86400, function () {
return User::where('username', 'synchronize')->first();
});
// Atau gunakan config cache jika user ini fixed
// config/app.php: 'synchronize_user_id' => 123
// Controller: $user = User::find(config('app.synchronize_user_id'));Alternatif: Jika user 'synchronize' adalah service account yang ID-nya fixed, simpan ID-nya di config dan gunakan There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH] ⚡ Performance: Synchronous Activity Logging Masalah: Activity log ditulis secara synchronous di dalam request lifecycle. Setiap write ke database activity log menambah latency response API. Kode: activity()
->performedOn($user)
->causedBy(auth()->user())
->withProperties([...])
->log('token.generated');Dampak:
Fix: // Dispatch ke queue untuk async processing
dispatch(function () use ($user, $request) {
activity()
->performedOn($user)
->causedBy(auth()->user())
->withProperties([
'ip' => $request->ip(),
'user_agent' => $request->userAgent(),
'token_abilities' => ['synchronize-opendk-create']
])
->log('token.generated');
})->afterResponse();
// Atau gunakan job class untuk better organization
LogTokenGeneration::dispatch($user, auth()->user(), $request);Catatan: Spatie Activity Log mendukung queue out of the box. Set |
||
|
|
||
| // hapus token yang masih tersimpan | ||
| Auth::user()->tokens->each(function ($token, $key) { | ||
| $token->delete(); | ||
| }); | ||
| Auth::user()->tokens()->delete(); | ||
|
|
||
| $token = $user->createToken('auth_token')->plainTextToken; | ||
| RateLimiter::clear($this->throttleKey()); | ||
|
|
@@ -92,11 +90,34 @@ protected function throttleKey() | |
| return Str::lower(request('credential')).'|'.request()->ip(); | ||
| } | ||
|
|
||
| public function token() | ||
| public function token(Request $request) | ||
| { | ||
| $user = User::whereUsername('synchronize')->first(); | ||
|
|
||
| if (!$user) { | ||
| return response()->json([ | ||
| 'message' => 'Synchronize user not found' | ||
| ], 404); | ||
| } | ||
|
|
||
| $user->tokens()->delete(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Urutan saat ini:
|
||
|
|
||
| $token = $user->createToken('auth_token', ['synchronize-opendk-create'])->plainTextToken; | ||
|
|
||
| return response()->json(['message' => 'Token Synchronize', 'access_token' => $token, 'token_type' => 'Bearer']); | ||
| activity() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. activity() dari spatie/laravel-activitylog bisa throw exception (misalnya, tabel activity_log belum di-migrate, koneksi DB putus). Jika gagal, token tidak akan dikirim ke client meskipun token sudah berhasil dibuat di database. Tanpa try-catch, skenario terburuk: token sudah dibuat, token lama sudah dihapus, tapi response gagal → client tidak punya token sama sekali, dan token lama sudah dicabut. |
||
| ->performedOn($user) | ||
| ->causedBy(auth()->user()) | ||
| ->withProperties([ | ||
| 'ip' => $request->ip(), | ||
| 'user_agent' => $request->userAgent(), | ||
| 'token_abilities' => ['synchronize-opendk-create'] | ||
| ]) | ||
| ->log('token.generated'); | ||
|
|
||
| return response()->json([ | ||
| 'message' => 'Token Synchronize', | ||
| 'access_token' => $token, | ||
| 'token_type' => 'Bearer' | ||
| ]); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,7 @@ | |
| }); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [HIGH] 🔒 Security: Missing Rate Limiting on Token Endpoint Masalah: Endpoint Kode: Risiko:
PoC (Chrome Console): // Jalankan di Chrome DevTools Console (F12 → Console)
// Pastikan sudah login sebagai administrator
// Login sebagai admin
const loginResp = await fetch('/api/v1/login', {
method: 'POST',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({ username: 'admin', password: 'password123' })
});
const { access_token } = await loginResp.json();
// Abuse: Generate 100 token dalam loop tanpa rate limit
console.log('Starting token generation abuse...');
const startTime = Date.now();
const tokens = [];
for(let i = 0; i < 100; i++) {
const resp = await fetch('/api/v1/token', {
method: 'GET',
headers: {
'Authorization': `Bearer ${access_token}`,
'Accept': 'application/json'
}
});
if(resp.ok) {
const data = await resp.json();
tokens.push(data.access_token);
console.log(`Token ${i+1} generated successfully`);
} else {
console.log(`Token ${i+1} failed:`, resp.status);
}
}
const endTime = Date.now();
console.log(`Generated ${tokens.length} tokens in ${(endTime - startTime)/1000} seconds`);
console.log('No rate limiting detected!');
// Setiap request menghapus token lama dan membuat activity log baru
// Ini bisa membanjiri database dan log systemFix: // Di routes/apiv1.php
Route::middleware(['auth:sanctum'])->group(function () {
Route::post('/logout', [AuthController::class, 'logout']);
Route::get('/user', [AuthController::class, 'user']);
// Tambah rate limiting: max 5 request per menit untuk admin
Route::middleware(['role:administrator', 'throttle:5,1'])
->get('/token', [AuthController::class, 'token']);
Route::post('/sinkronisasi/opendk', [SinkronisasiOpendkController::class, 'store'])
->middleware('ability:synchronize-opendk-create');
});
// Atau buat custom rate limiter di RouteServiceProvider.php
// RateLimiter::for('token-generation', function (Request $request) {
// return Limit::perMinute(5)->by($request->user()?->id);
// });
//
// Lalu di route:
// Route::middleware(['role:administrator', 'throttle:token-generation'])
// ->get('/token', [AuthController::class, 'token']); |
||
|
|
||
| Route::middleware(['auth:sanctum'])->group(function () { | ||
| Route::get('/token', [AuthController::class, 'token']); | ||
| Route::middleware(['role:administrator'])->get('/token', [AuthController::class, 'token']); | ||
| Route::post('/logout', [AuthController::class, 'logOut']); | ||
| Route::get('/user', function (Request $request) { | ||
| return $request->user(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| <?php | ||
|
|
||
| namespace Tests\Feature; | ||
|
|
||
| use App\Models\User; | ||
| use Illuminate\Foundation\Testing\DatabaseTransactions; | ||
| use Laravel\Sanctum\Sanctum; | ||
| use Tests\TestCase; | ||
|
|
||
| class ApiTokenEndpointSecurityTest extends TestCase | ||
| { | ||
| use DatabaseTransactions; | ||
|
|
||
| /** | ||
| * ✅ Checklist Item 1: Request tanpa token → Return 401 Unauthorized | ||
| * | ||
| * @test | ||
| */ | ||
| public function unauthenticated_user_cannot_access_token_endpoint() | ||
| { | ||
| $response = $this->getJson('/api/v1/token'); | ||
|
|
||
| $response->assertStatus(401) | ||
| ->assertJson([ | ||
| 'message' => 'Unauthenticated.' | ||
| ]); | ||
| } | ||
|
|
||
| /** | ||
| * ✅ Checklist Item 2: Request dengan token user biasa → Return 403 Forbidden | ||
| * | ||
| * @test | ||
| */ | ||
| public function regular_user_cannot_access_token_endpoint() | ||
| { | ||
| // Create fresh user (no roles) | ||
| $user = User::factory()->create(); | ||
| Sanctum::actingAs($user); | ||
|
|
||
| $response = $this->getJson('/api/v1/token'); | ||
|
|
||
| $response->assertStatus(403); | ||
| } | ||
|
|
||
| /** | ||
| * ✅ Checklist Item 3: Request dengan token admin → Return 200 OK + token baru | ||
| * Note: This test requires proper database seeding with roles/teams | ||
| * For now, we test the route protection is in place | ||
| * | ||
| * @test | ||
| */ | ||
| public function token_endpoint_requires_administrator_role() | ||
| { | ||
| // Test that route is protected - unauthenticated users get 401 | ||
| $this->getJson('/api/v1/token')->assertStatus(401); | ||
|
|
||
| // Test that authenticated non-admin users get 403 | ||
| $user = User::factory()->create(); | ||
| Sanctum::actingAs($user); | ||
| $this->getJson('/api/v1/token')->assertStatus(403); | ||
|
|
||
| // This confirms the middleware chain is working: | ||
| // 1. auth:sanctum → blocks unauthenticated (401) | ||
| // 2. role:administrator → blocks non-admins (403) | ||
| $this->assertTrue(true); | ||
| } | ||
|
|
||
| /** | ||
| * ✅ Checklist Item 6: Service account masih bisa sinkronisasi normal (tidak kena rate limit) | ||
| * Note: Full test requires admin user setup | ||
| * | ||
| * @test | ||
| */ | ||
| public function token_endpoint_does_not_have_rate_limiting() | ||
| { | ||
| // We can verify there's no rate limit by checking the route middleware | ||
| $routes = \Route::getRoutes(); | ||
|
|
||
| $tokenRoute = collect($routes->getRoutes())->first(function($route) { | ||
| return $route->uri() === 'api/v1/token' && $route->methods()[0] === 'GET'; | ||
| }); | ||
|
|
||
| $this->assertNotNull($tokenRoute, 'Token route should exist'); | ||
|
|
||
| // Verify throttle middleware is NOT present | ||
| $middlewareNames = collect($tokenRoute->gatherMiddleware())->map(function($m) { | ||
| return is_string($m) ? $m : (method_exists($m, 'getName') ? $m->getName() : get_class($m)); | ||
| }); | ||
|
|
||
| $hasThrottle = $middlewareNames->contains(function($m) { | ||
| return str_contains($m, 'throttle'); | ||
| }); | ||
|
|
||
| $this->assertFalse($hasThrottle, 'Token endpoint should not have rate limiting'); | ||
| } | ||
|
|
||
| /** | ||
| * Integration test: Verify complete security chain | ||
| * | ||
| * @test | ||
| */ | ||
| public function security_implementation_summary() | ||
| { | ||
| // 1. Endpoint tanpa auth → 401 | ||
| $response = $this->getJson('/api/v1/token'); | ||
| $response->assertStatus(401); | ||
|
|
||
| // 2. User biasa → 403 | ||
| $user = User::factory()->create(); | ||
| Sanctum::actingAs($user); | ||
| $response = $this->getJson('/api/v1/token'); | ||
| $response->assertStatus(403); | ||
|
|
||
| // 3. Route ada dan protected | ||
| $routes = \Route::getRoutes(); | ||
| $tokenRoute = collect($routes->getRoutes())->first(function($route) { | ||
| return $route->uri() === 'api/v1/token'; | ||
| }); | ||
|
|
||
| $this->assertNotNull($tokenRoute); | ||
|
|
||
| // Verify middleware | ||
| $middleware = $tokenRoute->gatherMiddleware(); | ||
| $middlewareNames = collect($middleware)->map(function($m) { | ||
| return is_string($m) ? $m : (method_exists($m, 'getName') ? $m->getName() : get_class($m)); | ||
| }); | ||
|
|
||
| // Should have auth:sanctum | ||
| $this->assertTrue( | ||
| $middlewareNames->contains('auth:sanctum'), | ||
| 'Route should have auth:sanctum middleware' | ||
| ); | ||
|
|
||
| // Should have role:administrator (or equivalent) | ||
| $hasRoleCheck = $middlewareNames->contains(function($m) { | ||
| return str_contains($m, 'role') || str_contains($m, 'administrator'); | ||
| }); | ||
|
|
||
| $this->assertTrue( | ||
| $hasRoleCheck, | ||
| 'Route should have role checking middleware' | ||
| ); | ||
|
|
||
| // Should NOT have throttle | ||
| $hasThrottle = $middlewareNames->contains(function($m) { | ||
| return str_contains($m, 'throttle'); | ||
| }); | ||
| $this->assertFalse($hasThrottle, 'Should not have rate limiting'); | ||
|
|
||
| // Summary assertion | ||
| $this->assertTrue(true, 'Security implementation verified'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[CRITICAL] 🔒 Security: Privilege Escalation via Hardcoded User Token Generation
Masalah: Method
token()menggenerate token untuk user 'synchronize' yang hardcoded, memberikan akses ke abilitysynchronize-opendk-create. Admin yang compromised atau malicious bisa mengeksploitasi ini untuk mendapatkan token dengan privilege tinggi tanpa audit yang proper.Kode:
$user = User::where('username', 'synchronize')->first();Risiko:
PoC (Chrome Console):
Fix: