From e403d74a3d23116868e47a282129bc6ea6da10c8 Mon Sep 17 00:00:00 2001 From: Hongming Wang Date: Thu, 30 Apr 2026 02:59:08 -0700 Subject: [PATCH] test(admin_test_token): pin ADMIN_TOKEN IDOR-fix (#112) gate behavior MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The admin test-token endpoint has a critical security check at admin_test_token.go:64-72 — the IDOR fix from #112 that requires an explicit ADMIN_TOKEN bearer when the env var is set. Pre-fix, the route accepted ANY bearer that matched a live org token, allowing cross-org test-token minting (and therefore cross-org workspace authentication). The current code uses subtle.ConstantTimeCompare against ADMIN_TOKEN. Test coverage was zero. The existing tests exercised the ADMIN_TOKEN-unset path (local dev / CI) but never set ADMIN_TOKEN. A regression that: - removed the os.Getenv("ADMIN_TOKEN") check - inverted the comparison - replaced ConstantTimeCompare with bytes.Equal (timing leak) - re-introduced the AdminAuth fallback that allows org tokens would not fail any test, and the breakage would re-open the IDOR that #112 closed. Adds four tests covering the gate matrix: - ADMIN_TOKEN set + no Authorization header → 401 - ADMIN_TOKEN set + wrong Authorization → 401 - ADMIN_TOKEN set + correct Authorization → 200 - ADMIN_TOKEN unset + no Authorization → 200 (gate bypassed safely) The 4-row matrix pins the gate's full truth table: any regression in either dimension (gate enabled/disabled, header correct/wrong) trips exactly one test. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../handlers/admin_test_token_test.go | 94 +++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/workspace-server/internal/handlers/admin_test_token_test.go b/workspace-server/internal/handlers/admin_test_token_test.go index 3ac72923..62d3f2b6 100644 --- a/workspace-server/internal/handlers/admin_test_token_test.go +++ b/workspace-server/internal/handlers/admin_test_token_test.go @@ -129,3 +129,97 @@ func TestAdminTestToken_HappyPath_TokenValidates(t *testing.T) { } func sqlErrNoRows() error { return sql.ErrNoRows } + +// TestAdminTestToken_AdminTokenRequired_NoHeader pins the IDOR-fix (#112): +// when ADMIN_TOKEN is set, calls without an Authorization header MUST 401. +// Pre-fix, the route accepted any bearer that matched a live org token, +// allowing cross-org test-token minting. The current code uses +// subtle.ConstantTimeCompare against ADMIN_TOKEN explicitly. This test +// pins that no-header == 401 so a regression that re-enabled the AdminAuth +// fallback would fail loudly. +func TestAdminTestToken_AdminTokenRequired_NoHeader(t *testing.T) { + setupTestDB(t) + t.Setenv("MOLECULE_ENV", "development") + t.Setenv("ADMIN_TOKEN", "the-admin-secret") + + h := NewAdminTestTokenHandler() + w, c := newTestTokenRequest("ws-1") + h.GetTestToken(c) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 with ADMIN_TOKEN set + no Authorization, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestAdminTestToken_AdminTokenRequired_WrongHeader pins that a non-matching +// bearer is rejected. Critical for #112 — an attacker presenting any other +// org's token must NOT pass. +func TestAdminTestToken_AdminTokenRequired_WrongHeader(t *testing.T) { + setupTestDB(t) + t.Setenv("MOLECULE_ENV", "development") + t.Setenv("ADMIN_TOKEN", "the-admin-secret") + + h := NewAdminTestTokenHandler() + w, c := newTestTokenRequest("ws-1") + c.Request.Header.Set("Authorization", "Bearer wrong-token") + h.GetTestToken(c) + + if w.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 with wrong Authorization, got %d: %s", w.Code, w.Body.String()) + } +} + +// TestAdminTestToken_AdminTokenRequired_CorrectHeader pins the success +// path through the ADMIN_TOKEN gate. Together with the no-header + wrong- +// header pair, this proves the gate distinguishes correct from incorrect +// rather than (e.g.) erroring on every request. +func TestAdminTestToken_AdminTokenRequired_CorrectHeader(t *testing.T) { + mock := setupTestDB(t) + t.Setenv("MOLECULE_ENV", "development") + t.Setenv("ADMIN_TOKEN", "the-admin-secret") + + mock.ExpectQuery("SELECT id FROM workspaces WHERE id ="). + WithArgs("ws-1"). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("ws-1")) + mock.ExpectExec("INSERT INTO workspace_auth_tokens"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + h := NewAdminTestTokenHandler() + w, c := newTestTokenRequest("ws-1") + c.Request.Header.Set("Authorization", "Bearer the-admin-secret") + h.GetTestToken(c) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200 with correct ADMIN_TOKEN, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock expectations not met — INSERT into workspace_auth_tokens did not run, suggesting the gate short-circuited the success path: %v", err) + } +} + +// TestAdminTestToken_AdminTokenEmpty_GateBypassedSafely pins that when +// ADMIN_TOKEN is unset (typical local-dev setup), the explicit gate is +// bypassed and the route works without an Authorization header. This is +// the same code path the existing TestAdminTestToken_EnabledViaFlagEvenInProd +// exercises, but pinned explicitly so a future refactor that conflates +// "ADMIN_TOKEN unset" with "always 401" gets caught immediately. +func TestAdminTestToken_AdminTokenEmpty_GateBypassedSafely(t *testing.T) { + mock := setupTestDB(t) + t.Setenv("MOLECULE_ENV", "development") + t.Setenv("ADMIN_TOKEN", "") + + mock.ExpectQuery("SELECT id FROM workspaces WHERE id ="). + WithArgs("ws-1"). + WillReturnRows(sqlmock.NewRows([]string{"id"}).AddRow("ws-1")) + mock.ExpectExec("INSERT INTO workspace_auth_tokens"). + WillReturnResult(sqlmock.NewResult(0, 1)) + + h := NewAdminTestTokenHandler() + w, c := newTestTokenRequest("ws-1") + // Note: NO Authorization header — the gate is unset, so this MUST work. + h.GetTestToken(c) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200 with ADMIN_TOKEN empty + no Authorization, got %d: %s", w.Code, w.Body.String()) + } +}