fix(admin): son aktif admin korumasi es zamanli isteklerle asilabiliyordu - #6
Closed
ersinkoc wants to merge 1 commit into
Closed
fix(admin): son aktif admin korumasi es zamanli isteklerle asilabiliyordu
#6ersinkoc wants to merge 1 commit into
ersinkoc wants to merge 1 commit into
Conversation
…ordu Kok neden: "son aktif admin" korumasi check-then-act'ti - other_active_admin_exists() kilitsiz bir SELECT COUNT(...) yapiyor, her uc nokta (toggle_status.php ve user_collect_input() uzerinden update.php) sonra AYRI bir UPDATE calistiryordu. Tam iki aktif admin varken birbirini es zamanli pasiflestiren/dusuren iki istek de "baska admin var" gorup gecebilir ve ikisi de commit eder: sistem SIFIR aktif adminle kalir (tam kilitleme; kurtarma dogrudan DB mudahalesi ister). Kanit (round-6 proof'u, yerel MariaDB): gercek iki isci sureci - her biri kendi PDO baglantisiyla - marker dosyalariyla kesisme zorlandi; her iki guard da UPDATE'lerden once calisti, sonucta 0 aktif admin (FAIL). DuzeItSonrasi: bir isci guncelledi, digeri guard tarafindan reddedildi, 1 aktif admin kaldi (PASS). Cozum: admin/users/_validate.php'ye last_admin_atomic_guard() eklendi - cagiranin transaction'i icinde TUM aktif admin satirlari FOR UPDATE ile kilitlenir; ayni satir kumesi ayni sirada kilitlendigi icin es zamanli islemler serilesir, ikincisi bloklanir ve commit edilmis guncel durumu okuyarak reddedilir. toggle_status.php ve update.php kontrol+UPDATE'i tek transaction'a tasiyor; eski yalniz-SELECT kontrolu UX on-kontrolu olarak kaldi. Dayanikli regresyon testi tools/last_admin_race_test.php (yerel MariaDB ister; users tablosunu yedekleyip geri yukler): tek baglanti karar semasi + iki surecli zorlanmis kesisme yarisi - 7 kontrol, 0 FAIL. php tools/smoke_test.php: 58 OK / 1 FAIL (yalnizca /var/www/riskops yerlesim yolu onaylamasi); php tools/fresh_bootstrap_test.php: 13 OK / 0 FAIL. Not: dal dogrudan main uzerindendir; acik diger PR'lere bagimliligi yoktur.
ersinkoc
changed the title
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tools/last_admin_race_test.php">
<violation number="1" location="tools/last_admin_race_test.php:120">
P2: An untrusted local user can pre-create or race this predictable `/tmp` directory and its symlinks, allowing the test to overwrite files or execute a replaced worker when run with higher privileges. Use a cryptographically random temporary directory with mode `0700` and exclusive file creation.</violation>
<violation number="2" location="tools/last_admin_race_test.php:129">
P2: This test ignores the application's database configuration and hardcodes a local password. With the documented setup it can fail to connect, or the parent and workers can use different database connections, so the regression test does not test the configured application; build the parent PDO from `config/database.php` too.</violation>
<violation number="3" location="tools/last_admin_race_test.php:169">
P1: If any later database or worker operation throws, the script exits before `$restoreSnapshot()` and leaves every existing account inactive, potentially locking out all administrators. Wrap the mutating test in `try/finally` and restore the snapshot from the finally block.</violation>
</file>
<file name="admin/users/_validate.php">
<violation number="1" location="admin/users/_validate.php:69">
P1: When a target is promoted to admin after the caller’s snapshot, this early return skips the atomic check and can leave zero active admins. Lock and inspect the current target row inside the transaction instead of trusting `targetWasActiveAdmin`.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| section('1) Tek baglanti: guard karar semasi'); | ||
| /* ------------------------------------------------------------------ */ | ||
|
|
||
| $pdo->exec('UPDATE users SET status = 0'); |
There was a problem hiding this comment.
P1: If any later database or worker operation throws, the script exits before $restoreSnapshot() and leaves every existing account inactive, potentially locking out all administrators. Wrap the mutating test in try/finally and restore the snapshot from the finally block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/last_admin_race_test.php, line 169:
<comment>If any later database or worker operation throws, the script exits before `$restoreSnapshot()` and leaves every existing account inactive, potentially locking out all administrators. Wrap the mutating test in `try/finally` and restore the snapshot from the finally block.</comment>
<file context>
@@ -0,0 +1,267 @@
+section('1) Tek baglanti: guard karar semasi');
+/* ------------------------------------------------------------------ */
+
+$pdo->exec('UPDATE users SET status = 0');
+$ins = $pdo->prepare('INSERT INTO users (name, email, password, role, status) VALUES (:n, :e, :p, \'admin\', 1)');
+$ins->execute([':n' => 'Race A', ':e' => 'race-test.a@riskops.local', ':p' => password_hash('x', PASSWORD_DEFAULT)]);
</file context>
Comment on lines
+69
to
+80
| if (!$targetWasActiveAdmin) { | ||
| return true; | ||
| } | ||
| $ids = $pdo->query( | ||
| "SELECT id FROM users WHERE role = 'admin' AND status = 1 FOR UPDATE" | ||
| )->fetchAll(PDO::FETCH_COLUMN); | ||
| foreach ($ids as $id) { | ||
| if ((int)$id !== $targetUserId) { | ||
| return true; // kilide alinmis kumede baska bir aktif admin var | ||
| } | ||
| } | ||
| return false; |
There was a problem hiding this comment.
P1: When a target is promoted to admin after the caller’s snapshot, this early return skips the atomic check and can leave zero active admins. Lock and inspect the current target row inside the transaction instead of trusting targetWasActiveAdmin.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At admin/users/_validate.php, line 69:
<comment>When a target is promoted to admin after the caller’s snapshot, this early return skips the atomic check and can leave zero active admins. Lock and inspect the current target row inside the transaction instead of trusting `targetWasActiveAdmin`.</comment>
<file context>
@@ -39,6 +39,47 @@ function other_active_admin_exists(int $excludeUserId): bool
+ */
+function last_admin_atomic_guard(PDO $pdo, int $targetUserId, bool $targetWasActiveAdmin): bool
+{
+ if (!$targetWasActiveAdmin) {
+ return true;
+ }
</file context>
Suggested change
| if (!$targetWasActiveAdmin) { | |
| return true; | |
| } | |
| $ids = $pdo->query( | |
| "SELECT id FROM users WHERE role = 'admin' AND status = 1 FOR UPDATE" | |
| )->fetchAll(PDO::FETCH_COLUMN); | |
| foreach ($ids as $id) { | |
| if ((int)$id !== $targetUserId) { | |
| return true; // kilide alinmis kumede baska bir aktif admin var | |
| } | |
| } | |
| return false; | |
| $stmt = $pdo->prepare( | |
| "SELECT id, role, status FROM users | |
| WHERE id = :target OR (role = 'admin' AND status = 1) | |
| ORDER BY id FOR UPDATE" | |
| ); | |
| $stmt->execute([':target' => $targetUserId]); | |
| $rows = $stmt->fetchAll(); | |
| $targetIsActiveAdmin = false; | |
| $otherActiveAdminExists = false; | |
| foreach ($rows as $row) { | |
| $isActiveAdmin = $row['role'] === 'admin' && (int)$row['status'] === 1; | |
| if ((int)$row['id'] === $targetUserId) { | |
| $targetIsActiveAdmin = $isActiveAdmin; | |
| } elseif ($isActiveAdmin) { | |
| $otherActiveAdminExists = true; | |
| } | |
| } | |
| return !$targetIsActiveAdmin || $otherActiveAdminExists; |
|
|
||
| $tmp = sys_get_temp_dir() . '/riskops-race-' . getmypid(); | ||
| if (!is_dir($tmp)) { | ||
| mkdir($tmp, 0777, true); |
There was a problem hiding this comment.
P2: An untrusted local user can pre-create or race this predictable /tmp directory and its symlinks, allowing the test to overwrite files or execute a replaced worker when run with higher privileges. Use a cryptographically random temporary directory with mode 0700 and exclusive file creation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/last_admin_race_test.php, line 120:
<comment>An untrusted local user can pre-create or race this predictable `/tmp` directory and its symlinks, allowing the test to overwrite files or execute a replaced worker when run with higher privileges. Use a cryptographically random temporary directory with mode `0700` and exclusive file creation.</comment>
<file context>
@@ -0,0 +1,267 @@
+
+$tmp = sys_get_temp_dir() . '/riskops-race-' . getmypid();
+if (!is_dir($tmp)) {
+ mkdir($tmp, 0777, true);
+}
+$workerFile = $tmp . '/worker.php';
</file context>
|
|
||
| try { | ||
| $pdo = new PDO( | ||
| 'mysql:host=127.0.0.1;port=3306;dbname=riskops;charset=utf8mb4', |
There was a problem hiding this comment.
P2: This test ignores the application's database configuration and hardcodes a local password. With the documented setup it can fail to connect, or the parent and workers can use different database connections, so the regression test does not test the configured application; build the parent PDO from config/database.php too.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tools/last_admin_race_test.php, line 129:
<comment>This test ignores the application's database configuration and hardcodes a local password. With the documented setup it can fail to connect, or the parent and workers can use different database connections, so the regression test does not test the configured application; build the parent PDO from `config/database.php` too.</comment>
<file context>
@@ -0,0 +1,267 @@
+
+try {
+ $pdo = new PDO(
+ 'mysql:host=127.0.0.1;port=3306;dbname=riskops;charset=utf8mb4',
+ 'riskops_user',
+ 'riskops_local_dev',
</file context>
Owner
|
Teşekkürler. Dört bulguyu da doğruladım, dördü de gerçekti. Özellikle schema.sql'deki eksik users tablosu ciddiydi: belgelenmiş Düzeltmeleri main'e bağımsız olarak uyguladım (2530ae1). PR'ları merge
cubic'in son admin korumasına yaptığı P1 eleştirisini de hesaba kattım: Eklenen regresyon testleri:
#7 açık kalıyor - orada karşılığı olmayan bir test var, kapsamı Tekrar teşekkürler, başka bulgunuz olursa memnuniyetle bakarım. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hata
"Son aktif admin" koruması bir check-then-act yarışıydı: koruma, veritabanı sunucusunda kilitlenmemiş bir
SELECT COUNT(*)(other_active_admin_exists()) ve her uç noktada ondan sonra çalışan ayrı birUPDATEşeklindeydi.Tam iki aktif admin varken, birbirini eş zamanlı pasifleştiren/düşüren iki istek her ikisi de "başka bir admin var" görür, her ikisi de korumadan geçer ve her ikisi de commit eder → sistemde sıfır aktif admin kalır (tam kilitleme; kurtarma doğrudan DB müdahalesi gerektirir). Korumayı yazan yorum ("sistemde admin kalmayabilir") tam olarak bunu önlemek istiyordu.
Etkilenen yollar:
admin/users/toggle_status.php(hızlı aç/kapat)admin/users/update.php(user_collect_input()doğrulaması üzerinden rol düşürme / hesap kapatma)Kanıt (yerel MariaDB üzerinde deterministik yarış)
Proof harness'i gerçek iki işçi süreç çalıştırır — her biri kendi PDO bağlantısıyla — ve marker dosyalarıyla kesintiyi zorlar: her iki koruma da her iki UPDATE'ten önce çalışacak şekilde:
Düzeltmeden sonra aynı zorlanmış kesinti: bir işçi güncelledi, diğeri guard tarafından reddedildi, 1 aktif admin kaldı → PASS.
Çözüm
admin/users/_validate.php— yenilast_admin_atomic_guard(PDO, int, bool): çağıranın transaction'ı içinde tüm aktif admin satırlarıSELECT ... FOR UPDATEile kilitlenir. Eş zamanlı iki işlem aynı satır kümesini aynı sırada kilitlediği için serileşirler: ilki commit edene kadar ikincisi bloklanır, sonra commit edilmiş güncel durumu okur ve reddedilir (kilitli okuma, REPEATABLE READ altında bile current-read'dir).admin/users/toggle_status.phpveadmin/users/update.php— koruma + UPDATE tek transaction içinde; her iki hata yolunda rollback. Eski yalnız-SELECT kontrolu UX ön-doğrulaması olarak kalır (yarış penceresini kapatmaz, atomik olan otoriterdir).Doğrulama
tools/last_admin_race_test.php(yerel MariaDB ister;userstablosunu yedekler/geri yükler, işçi scriptini repo dışında geçici dizinde üretir): tek bağlantılı karar şeması (2 admin → izinli; tek kalan admin → reddedilir; admin olmayan hedef → etkilenmez) + iki süreçli zorlanmış kesisme yarışı — 7 OK / 0 FAIL.php -ltemiz (4 dosya).php tools/smoke_test.php: 58 OK / 1 FAIL (yalnızca/var/www/riskopsyerleşim yolu onaylaması);php tools/fresh_bootstrap_test.php: 13 OK / 0 FAIL.Kapsam çiti
Bu PR yalnızca
admin/users/_validate.php,admin/users/toggle_status.php,admin/users/update.phpvetools/last_admin_race_test.phpdosyalarını değiştirir. Dal doğrudanmainüzerindendir; açık diğer PR'lere bağımlılığı yoktur.Summary by cubic
Fixes the last-active-admin guard race so two concurrent requests can no longer both pass the "another admin exists" check and commit, leaving the system with zero active admins.
Fix
last_admin_atomic_guard()locks all active admin rows withSELECT ... FOR UPDATEinside the caller's transaction.toggle_status.phpandupdate.phpnow run the guard and the UPDATE in one transaction and roll back on both error paths.Verification
tools/last_admin_race_test.php, a deterministic two-process race test with 7 checks passing.userstable.Written for commit 2021962. Summary will update on new commits.