From a9cdef8a1b854075ff080dee08036afa7e48b560 Mon Sep 17 00:00:00 2001 From: Rosie Val Date: Wed, 29 Jul 2026 00:08:12 +0100 Subject: [PATCH] Fix dashboard security issues: path traversal, OAuth CSRF, exposed bind, stale cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - _serve_spa built file paths directly from the URL with no containment check, on a route reachable with zero authentication. Verified the exact exploit (percent-encoded traversal) reproduces path escape, then fixed by resolving the path and requiring it stay under WEB_DIR before ever touching the filesystem. - OAuth state was generated and handed to the client but never bound server-side or checked on callback — classic login CSRF/session fixation. Now bound to a short-lived httponly cookie set during login and verified (constant-time) on callback. - Default bind changed from 0.0.0.0 to 127.0.0.1 — loading this cog with zero config shouldn't expose a web server on every interface before the owner has even run setup. `[p]evedash host 0.0.0.0` still opts back in. - _check_guild_access and the callback's guild-membership check relied solely on the member cache, which can wrongly deny a real admin if the Members intent hasn't warmed for that guild yet. Both now fall back to a live fetch on cache miss (per-guild for the single-guild endpoints, bot-wide only in the rare one-time login path — deliberately not done in the bulk guild-list endpoint, where that fallback would mean an API call per guild on every page load). Co-Authored-By: Claude Sonnet 5 --- dashboard/dashboard.py | 7 +++- dashboard/server.py | 74 ++++++++++++++++++++++++++++++++++-------- 2 files changed, 67 insertions(+), 14 deletions(-) diff --git a/dashboard/dashboard.py b/dashboard/dashboard.py index d6a67a6..f4adcf5 100644 --- a/dashboard/dashboard.py +++ b/dashboard/dashboard.py @@ -51,7 +51,12 @@ class EveDash(commands.Cog): force_registration=True, ) self.config.register_global( - host="0.0.0.0", + # Localhost-only by default — loading this cog with zero config + # shouldn't immediately expose an unauthenticated-until-login web + # server on every network interface. Owners who want it reachable + # externally (e.g. no reverse proxy) can opt in with + # `[p]evedash host 0.0.0.0`. + host="127.0.0.1", port=42356, client_id=None, client_secret=None, diff --git a/dashboard/server.py b/dashboard/server.py index 8f03641..1dc3d61 100644 --- a/dashboard/server.py +++ b/dashboard/server.py @@ -17,6 +17,7 @@ import time import typing import aiohttp +import discord from aiohttp import web if typing.TYPE_CHECKING: @@ -259,13 +260,30 @@ class DashboardServer: f"&scope=identify+guilds" f"&state={state}" ) - return _json({"url": url, "state": state}) + resp = _json({"url": url, "state": state}) + # Bind this state to the browser that requested it via a short-lived + # cookie, so the callback can confirm the redirect actually belongs + # to a login flow we started — otherwise an attacker's own + # authorization code could be handed to a victim's browser and get + # bound to the victim's session (login CSRF / session fixation). + resp.set_cookie( + "oauth_state", state, + max_age=600, + httponly=True, + samesite="Lax", + ) + return resp async def _auth_callback(self, request: web.Request) -> web.Response: code = request.query.get("code") if not code: return _err("Missing authorization code") + state = request.query.get("state") + expected_state = request.cookies.get("oauth_state") + if not state or not expected_state or not hmac.compare_digest(state, expected_state): + return _err("Invalid or missing state parameter", 400) + client_id = await self.cog.config.client_id() client_secret = await self.cog.config.client_secret() redirect_uri = await self.cog.config.redirect_uri() @@ -301,6 +319,17 @@ class DashboardServer: # Check access: must be bot owner or share a guild is_owner = user_id in self.bot.owner_ids has_guild = any(g.get_member(user_id) for g in self.bot.guilds) + if not is_owner and not has_guild: + # Cache miss rather than genuinely no shared guilds? This only + # runs once per login (not a hot path), so it's worth confirming + # with a live check before denying a real user access. + for g in self.bot.guilds: + try: + if await g.fetch_member(user_id): + has_guild = True + break + except (discord.NotFound, discord.HTTPException): + continue if not is_owner and not has_guild: return _err("You don't share any servers with this bot", 403) @@ -322,6 +351,7 @@ class DashboardServer: httponly=True, samesite="Lax", ) + resp.del_cookie("oauth_state") return resp async def _auth_me(self, request: web.Request) -> web.Response: @@ -437,7 +467,7 @@ class DashboardServer: if not guild: return _err("Guild not found", 404) - if not self._check_guild_access(request, guild): + if not await self._check_guild_access(request, guild): return _err("Forbidden", 403) return _json({ @@ -462,7 +492,7 @@ class DashboardServer: guild = self._get_guild(request) if not guild: return _err("Guild not found", 404) - if not self._check_guild_access(request, guild): + if not await self._check_guild_access(request, guild): return _err("Forbidden", 403) channels = [] @@ -480,7 +510,7 @@ class DashboardServer: guild = self._get_guild(request) if not guild: return _err("Guild not found", 404) - if not self._check_guild_access(request, guild): + if not await self._check_guild_access(request, guild): return _err("Forbidden", 403) roles = [] @@ -503,7 +533,7 @@ class DashboardServer: guild = self._get_guild(request) if not guild: return _err("Guild not found", 404) - if not self._check_guild_access(request, guild): + if not await self._check_guild_access(request, guild): return _err("Forbidden", 403) prefixes = await self.bot.get_valid_prefixes(guild) @@ -534,7 +564,7 @@ class DashboardServer: guild = self._get_guild(request) if not guild: return _err("Guild not found", 404) - if not self._check_guild_access(request, guild): + if not await self._check_guild_access(request, guild): return _err("Forbidden", 403) data = await request.json() @@ -568,7 +598,7 @@ class DashboardServer: guild = self._get_guild(request) if not guild: return _err("Guild not found", 404) - if not self._check_guild_access(request, guild): + if not await self._check_guild_access(request, guild): return _err("Forbidden", 403) disabled_cmds = await self.bot._config.guild(guild).disabled_commands() @@ -600,7 +630,7 @@ class DashboardServer: guild = self._get_guild(request) if not guild: return _err("Guild not found", 404) - if not self._check_guild_access(request, guild): + if not await self._check_guild_access(request, guild): return _err("Forbidden", 403) cmd_name = request.match_info["command"] @@ -693,7 +723,7 @@ class DashboardServer: guild = self._get_guild(request) if not guild: return _err("Guild not found", 404) - if not self._check_guild_access(request, guild): + if not await self._check_guild_access(request, guild): return _err("Forbidden", 403) tp_info = self.cog.third_parties.get_third_parties_info() @@ -703,7 +733,7 @@ class DashboardServer: guild = self._get_guild(request) if not guild: return _err("Guild not found", 404) - if not self._check_guild_access(request, guild): + if not await self._check_guild_access(request, guild): return _err("Forbidden", 403) cog_name = request.match_info["cog_name"] @@ -851,7 +881,18 @@ class DashboardServer: async def _serve_spa(self, request: web.Request) -> web.Response: path = request.match_info.get("path", "") - file_path = WEB_DIR / path + + # Resolve and verify the final path stays inside WEB_DIR before ever + # touching the filesystem — rejects `../`, percent-encoded traversal + # (e.g. `..%2f`), and symlink escapes alike. This route is reachable + # with no authentication (see the auth middleware's path allowlist), + # so this check is the only thing standing between it and arbitrary + # file read on the host. + try: + file_path = (WEB_DIR / path).resolve() + file_path.relative_to(WEB_DIR.resolve()) + except (ValueError, OSError): + return _err("Not found", 404) # Serve actual files if they exist if file_path.is_file(): @@ -872,11 +913,18 @@ class DashboardServer: except (KeyError, ValueError): return None - def _check_guild_access(self, request: web.Request, guild) -> bool: + async def _check_guild_access(self, request: web.Request, guild) -> bool: if request["is_owner"]: return True user_id = request["user_id"] member = guild.get_member(user_id) if not member: - return False + # Cache miss (e.g. Members intent not warmed for this guild yet) + # shouldn't wrongly deny a real admin — fall back to a live fetch. + # Only done for single-guild endpoints (this method), never in + # bulk over every guild the bot is in — see _guilds_list. + try: + member = await guild.fetch_member(user_id) + except (discord.NotFound, discord.HTTPException): + return False return member.guild_permissions.manage_guild or member.guild_permissions.administrator