From 974a27329ef45ef7995898ae21e2778dc999564e Mon Sep 17 00:00:00 2001 From: Aaron Kimbrell Date: Sun, 27 Sep 2026 08:51:53 -0500 Subject: [PATCH] fix(dashboard): DataTables queries count as reads for read-only API keys POST /api/tables/... only reads, so read-only keys may use it (and the API docs list it for them). Keys limited to some addresses can't open the WebSocket, whose address isn't checked, nor keys whose allowed paths leave out /ws. Refusals are audited without an account target. Co-Authored-By: Claude Opus 5.5 --- dDashboardServer/auth/ApiKeyService.cpp | 3 +++ dDashboardServer/auth/ApiKeyService.h | 3 ++- dDashboardServer/routes/APIRoutes.cpp | 3 +-- dDashboardServer/routes/ApiKeyRoutes.cpp | 2 +- dDashboardServer/routes/RouteUtils.cpp | 16 ++++++++++------ dDashboardServer/routes/RouteUtils.h | 5 +++-- 6 files changed, 20 insertions(+), 12 deletions(-) diff --git a/dDashboardServer/auth/ApiKeyService.cpp b/dDashboardServer/auth/ApiKeyService.cpp index 758e2eb6b..e7268b30d 100644 --- a/dDashboardServer/auth/ApiKeyService.cpp +++ b/dDashboardServer/auth/ApiKeyService.cpp @@ -206,6 +206,9 @@ namespace ApiKeyService { std::lock_guard lock(state.mutex); const auto& cached = Lookup(state, DashboardAuthService::Sha256Hex(token)); if (!cached.key) return std::nullopt; + // The WebSocket doesn't tell us the address, so a key limited to some addresses can't use it; one limited to + // some paths only if /ws is one of them + if (!cached.allowedIps.empty() || !ApiKeys::PathAllowed(cached.allowedPaths, "/ws")) return std::nullopt; const auto owner = CheckKeyAndOwner(*cached.key); if (!owner) return std::nullopt; return Verified{ cached.key->accountId, owner->username, owner->gmLevel, diff --git a/dDashboardServer/auth/ApiKeyService.h b/dDashboardServer/auth/ApiKeyService.h index 2a2e4ff6a..472126924 100644 --- a/dDashboardServer/auth/ApiKeyService.h +++ b/dDashboardServer/auth/ApiKeyService.h @@ -44,7 +44,8 @@ namespace ApiKeyService { }; eResult Authenticate(const std::string& token, HTTPContext& context, HTTPReply& reply); - // For WebSocket connections: the owner and scope, without counting against the limits + // For WebSocket connections: the owner and scope, without counting against the limits. Keys limited to some + // addresses can't be used there (the address isn't known), nor keys whose allowed paths leave out /ws. struct Verified { uint32_t accountId{}; std::string username; diff --git a/dDashboardServer/routes/APIRoutes.cpp b/dDashboardServer/routes/APIRoutes.cpp index ac3a9e535..508cc61ac 100644 --- a/dDashboardServer/routes/APIRoutes.cpp +++ b/dDashboardServer/routes/APIRoutes.cpp @@ -109,14 +109,13 @@ namespace { if (!context.apiKey) return true; const auto& key = *context.apiKey; if (ApiKeyService::SessionOnlyPath(doc.path)) return false; - if (key.readOnly && doc.method != "GET") return false; + if (key.readOnly && !doc.reads) return false; return doc.permission.empty() ? (doc.minGmLevel <= 0 || key.allPermissions) : key.Has(doc.permission); } // Register a DataTables endpoint. The fetcher returns the DB layer's JSON string. Access: a GM level or a Perm. template void TableRoute(const std::string& path, const Access& access, const std::string& description, TableFetcher fetcher) { - ReadRoutes reads; // the query is a POST body, but it only reads Route(eHTTPMethod::POST, path, access, description, [fetcher = std::move(fetcher)](HTTPReply& reply, const HTTPContext& context) { const auto request = ParseDataTablesRequest(context.body); const auto body = ParseBody(context); diff --git a/dDashboardServer/routes/ApiKeyRoutes.cpp b/dDashboardServer/routes/ApiKeyRoutes.cpp index 1f153e413..d2ee652a9 100644 --- a/dDashboardServer/routes/ApiKeyRoutes.cpp +++ b/dDashboardServer/routes/ApiKeyRoutes.cpp @@ -279,7 +279,7 @@ namespace ApiKeyRoutes { // Refusals of keys (their scope, read-only, addresses, paths, limits) go to the audit log, a minute apart at most ApiKeyService::SetDeniedHook([](const HTTPContext& context, const std::string& reason) { - Audit(context, "api_key_denied", reason, AuditTarget::Account(context.accountId)); + Audit(context, "api_key_denied", reason); }); RequireAuthMiddleware::SetApiKeyDeniedHook([](const HTTPContext& context, const std::string& reason) { ApiKeyService::NoteDenied(context, reason); }); ApiKeyService::SetClientAddress([](const HTTPContext& context) { return ClientAddress(context); }); diff --git a/dDashboardServer/routes/RouteUtils.cpp b/dDashboardServer/routes/RouteUtils.cpp index 64a5daf72..f5f613240 100644 --- a/dDashboardServer/routes/RouteUtils.cpp +++ b/dDashboardServer/routes/RouteUtils.cpp @@ -35,8 +35,12 @@ namespace RouteUtils { std::vector g_RouteDocs; int g_ReadRoutes = 0; - std::shared_ptr MakeRequireAuth(std::shared_ptr middleware) { - if (g_ReadRoutes > 0) middleware->SetReadsOnly(); + bool Reads(eHTTPMethod method, const std::string& path) { + return method == eHTTPMethod::GET || g_ReadRoutes > 0 || (method == eHTTPMethod::POST && path.starts_with("/api/tables/")); + } + + std::shared_ptr MakeRequireAuth(std::shared_ptr middleware, eHTTPMethod method, const std::string& path) { + if (Reads(method, path)) middleware->SetReadsOnly(); return middleware; } } @@ -63,16 +67,16 @@ namespace RouteUtils { void Route(eHTTPMethod method, const std::string& path, int16_t minGmLevel, const std::string& description, Handler handler) { std::vector middleware; - if (minGmLevel >= 0) middleware.push_back(MakeRequireAuth(std::make_shared(static_cast(minGmLevel)))); - g_RouteDocs.push_back({ std::string(magic_enum::enum_name(method)), path, minGmLevel, description, "" }); + if (minGmLevel >= 0) middleware.push_back(MakeRequireAuth(std::make_shared(static_cast(minGmLevel)), method, path)); + g_RouteDocs.push_back({ std::string(magic_enum::enum_name(method)), path, minGmLevel, description, "", Reads(method, path) }); Register(method, path, std::move(middleware), std::move(handler)); } void Route(eHTTPMethod method, const std::string& path, const Perm& permission, const std::string& description, Handler handler) { if (!Permissions::Find(permission.key)) LOG("Route %s uses unknown permission %s; nobody can use it", path.c_str(), permission.key.c_str()); std::vector middleware; - middleware.push_back(MakeRequireAuth(std::make_shared(std::function([key = permission.key] { return Permissions::Level(key); }), permission.key))); - g_RouteDocs.push_back({ std::string(magic_enum::enum_name(method)), path, Permissions::Level(permission.key), description, permission.key }); + middleware.push_back(MakeRequireAuth(std::make_shared(std::function([key = permission.key] { return Permissions::Level(key); }), permission.key), method, path)); + g_RouteDocs.push_back({ std::string(magic_enum::enum_name(method)), path, Permissions::Level(permission.key), description, permission.key, Reads(method, path) }); Register(method, path, std::move(middleware), std::move(handler)); } diff --git a/dDashboardServer/routes/RouteUtils.h b/dDashboardServer/routes/RouteUtils.h index a13ac5f25..f0d242e09 100644 --- a/dDashboardServer/routes/RouteUtils.h +++ b/dDashboardServer/routes/RouteUtils.h @@ -243,6 +243,7 @@ namespace RouteUtils { int16_t minGmLevel; std::string description; std::string permission; // set for routes guarded by a permission; minGmLevel is then its level at the time + bool reads{}; // makes no changes (a GET, or a POST that only reads): read-only API keys may use it }; // A named permission (Permissions.h) guarding a route; its GM level can be changed while the server runs @@ -259,8 +260,8 @@ namespace RouteUtils { void Route(eHTTPMethod method, const std::string& path, int16_t minGmLevel, const std::string& description, Handler handler); void Route(eHTTPMethod method, const std::string& path, const Perm& permission, const std::string& description, Handler handler); - // Routes registered while one of these lives only read, even POSTs (DataTables and lookups send their query as a - // body), so read-only API keys may use them + // Routes registered while one of these lives only read, even POSTs (lookups that send their query as a body), so + // read-only API keys may use them. POST /api/tables/... (DataTables) always counts as a read. struct ReadRoutes { ReadRoutes(); ~ReadRoutes();