diff options
Diffstat (limited to 'internal/admin')
| -rw-r--r-- | internal/admin/api_players.go | 18 | ||||
| -rw-r--r-- | internal/admin/api_players_path_test.go | 87 | ||||
| -rw-r--r-- | internal/admin/server.go | 6 |
3 files changed, 110 insertions, 1 deletions
diff --git a/internal/admin/api_players.go b/internal/admin/api_players.go index 66142c5..8ac0dd2 100644 --- a/internal/admin/api_players.go +++ b/internal/admin/api_players.go @@ -87,7 +87,23 @@ func (s *AdminServer) handlePlayerByID(w http.ResponseWriter, r *http.Request) { } if strings.HasPrefix(path, "account/") { - s.getAccount(w, strings.TrimPrefix(path, "account/")) + acct := strings.TrimPrefix(path, "account/") + if err := player.ValidName(acct); err != nil { + http.Error(w, `{"error":"invalid account id"}`, http.StatusBadRequest) + return + } + s.getAccount(w, acct) + return + } + + // Validate before the first join into players/characters/<name>.yaml. + // ServeMux cleans the ESCAPED path but handlers read the DECODED + // r.URL.Path, so a percent-encoded segment like %2e%2e%2f reaches this + // handler as "../" and would otherwise escape the data dir (and bypass + // the password_hash redaction in getAccount). ValidName is the same + // invariant every other name join in the codebase enforces. + if err := player.ValidName(path); err != nil { + http.Error(w, `{"error":"invalid player id"}`, http.StatusBadRequest) return } diff --git a/internal/admin/api_players_path_test.go b/internal/admin/api_players_path_test.go new file mode 100644 index 0000000..dca156e --- /dev/null +++ b/internal/admin/api_players_path_test.go @@ -0,0 +1,87 @@ +package admin + +import ( + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + + "thehouseoficarus/internal/config" + "thehouseoficarus/internal/player" +) + +// TestPlayerAPICannotEscapeData: /api/players/{name} joins the raw URL +// remainder into players/characters/<name>.yaml. ServeMux cleans the ESCAPED +// path but handlers read the DECODED r.URL.Path, so a percent-encoded segment +// (%2e%2e%2f) arrives as "../" and would otherwise read arbitrary *.yaml — +// including account files, bypassing the password_hash redaction that +// getAccount applies. The route must validate the name before the first join. +func TestPlayerAPICannotEscapeData(t *testing.T) { + root := t.TempDir() + dir := filepath.Join(root, "data") + for _, sub := range []string{"players/accounts", "players/characters"} { + if err := os.MkdirAll(filepath.Join(dir, sub), 0o700); err != nil { + t.Fatal(err) + } + } + accounts := player.NewAccountStore(dir) + if err := accounts.SaveAccount(&player.Account{Name: "auditor", Admin: true, PasswordHash: "argon2id$auditor-hash"}); err != nil { + t.Fatal(err) + } + if err := accounts.SaveAccount(&player.Account{Name: "victim", PasswordHash: "argon2id$victim-hash"}); err != nil { + t.Fatal(err) + } + victim := player.New("victim") + victim.Account = "victim" + if err := accounts.SaveCharacter(victim); err != nil { + t.Fatal(err) + } + // A canary file outside data/ that traversal could otherwise reach. + if err := os.WriteFile(filepath.Join(root, "outside.yaml"), []byte("secret: canary\n"), 0o600); err != nil { + t.Fatal(err) + } + + s, err := NewServer(config.Default(), false, accounts, nil, nil, nil, nil, dir, + nil, nil, nil, nil, filepath.Join(dir, "config.yaml"), + func(fn func()) bool { fn(); return true }) + if err != nil { + t.Fatal(err) + } + cookieRec := httptest.NewRecorder() + s.setAuthCookie(cookieRec, "auditor") + cookie := cookieRec.Result().Cookies()[0] + + get := func(target string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodGet, target, nil) + req.AddCookie(cookie) + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req) + return rec + } + + // Encoded traversal to an account file must be rejected, and the raw + // password hash must never appear in the response. + rec := get("/api/players/%2e%2e%2faccounts%2fvictim") + if rec.Code != http.StatusBadRequest { + t.Fatalf("encoded traversal: status %d body %s", rec.Code, rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "victim-hash") { + t.Fatal("encoded traversal leaked a password hash") + } + + // Traversal out of the data dir to a canary must be rejected. + rec = get("/api/players/%2e%2e%2f%2e%2e%2foutside") + if rec.Code != http.StatusBadRequest { + t.Fatalf("outside traversal: status %d body %s", rec.Code, rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "canary") { + t.Fatal("encoded traversal read a file outside the data dir") + } + + // A legitimate lookup still works. + if rec = get("/api/players/victim"); rec.Code != http.StatusOK { + t.Fatalf("legit player lookup: status %d body %s", rec.Code, rec.Body.String()) + } +} diff --git a/internal/admin/server.go b/internal/admin/server.go index 4ea22d8..c13fe96 100644 --- a/internal/admin/server.go +++ b/internal/admin/server.go @@ -389,6 +389,12 @@ func (s *AdminServer) checkAuth(r *http.Request) bool { return false } account := parts[0] + // The account name is attacker-controlled (it comes from the cookie) and + // is joined into players/accounts/<account>.yaml by isAdminAccount below, + // so reject path-like names before the first join. + if err := player.ValidName(account); err != nil { + return false + } issued, err := strconv.ParseInt(parts[1], 10, 64) if err != nil { return false |
