Dashboard Rewrite PR

[ShadowLp174]

PR draft for dashboard rewrite commits

In general, to fix inefficient regular expressions you remove ambiguous nested repetitions (like (...+)* or ([...]*[...])*) or refactor the pattern so that the engine does not need to try many overlapping ways to match the same substring. Another robust option is to avoid complex hand-written validators entirely and instead use built-in or well-tested parsing APIs.

For this case, the simplest safe change that preserves intended functionality is to stop manually validating URL structure with a complex regex and instead leverage the browser’s URL constructor: if new URL(str) succeeds, the string is a syntactically valid URL (optionally requiring http: or https:). This avoids the problematic regex entirely, eliminating the risk of exponential backtracking while keeping the same “is this a URL?” behavior from the caller’s point of view.

Concretely, in dashboard/static/js/SearchInput.js, replace the entire #isValidUrl method body (lines 45–52) with a try/catch that calls new URL(str), checks for http:/https:, and returns a boolean. No external dependencies or new imports are needed, and no other code changes are required because #isValidUrl’s signature and return type stay the same.

---

[ShadowLp174]

Will fix later, probably

To fix this, add CSRF protection middleware to the Express app and enforce it for state-changing routes that rely on cookies/sessions, including /api/login and /api/login/verify. A common approach is to use the lusca library’s csrf middleware as recommended, which integrates well with cookieParser and express-session.

Concretely, in dashboard/index.js:

1. Add an import for lusca near the top of the file.
2. After app.use(cookieParser()); and app.use(ses);, add app.use(lusca.csrf()); to ensure all subsequent POST/PUT/DELETE (and optionally other) routes require a valid CSRF token.
3. For any routes that serve HTML (or JSON) to browser clients needing to submit POST requests, ensure they can obtain the CSRF token. The standard pattern is to expose req.csrfToken() via locals or JSON. However, in this snippet we only see API endpoints and cannot safely change their contract, so the minimal security-improving fix here is to enable the middleware globally; if the client currently does not send CSRF tokens, calls to protected routes will begin failing with a 403 until the client is updated to include them. That is an expected, but intentional, behavior change to add security.

The smallest change to address the CodeQL alerts without altering the internal logic of /api/login and /api/login/verify is therefore:

• Add const lusca = require('lusca'); at the top.
• Add app.use(lusca.csrf()); after app.use(ses);.

---

To fix this, the Secure flag must be set on the session cookie itself via the cookie option of express-session, not as a top-level option. The best fix is to replace the current session({...}) configuration so that it defines a cookie object containing secure, and, ideally, other safe defaults like httpOnly and sameSite, while preserving existing behavior elsewhere.

Concretely, in dashboard/index.js, lines 52–57 where const ses = session({ ... }) is defined should be updated. We will remove the top-level secure property and instead add a cookie property: cookie: { secure: !!remix.config.ssl.useSSL, httpOnly: true, sameSite: 'lax' }. This keeps the existing intention—only mark cookies as secure when SSL is enabled—while actually applying the setting to the transmitted cookie. No additional imports are required, and no other parts of the file need to change because the variable ses is still the same session middleware.

To fix this, introduce a rate-limiting middleware (using a well-known library such as express-rate-limit) and apply it before the authorization middleware at line 66. This will cap the number of requests a single client (IP) can make per time window, mitigating denial-of-service by repeated authorization attempts.

Concretely, in dashboard/index.js:

1. Add an import for express-rate-limit near the other require calls.
2. After creating the Express app and before the JSON/urlencoded/static/session middlewares, configure a limiter, e.g., 100 requests per 15 minutes per IP (you can adjust as needed).
3. Apply the limiter with app.use(limiter); so that it covers all requests, including those that hit the async (req, _res, next) authorization middleware at line 66.

This approach does not alter existing business logic; it just introduces an additional middleware in the standard Express pipeline.

In general, the fix is to introduce a rate‑limiting middleware and apply it to sensitive/expensive routes, especially authentication/authorization endpoints. In an Express app, this is commonly done using express-rate-limit, which tracks requests per client (usually by IP) within a time window and rejects or slows requests that exceed the configured limit.

For this code, the best minimal‑impact fix is:

• Add a require('express-rate-limit') import near the top of dashboard/index.js.
• Define a specific limiter for auth‑related routes (e.g., /api/login and /api/login/verify), with a conservative but reasonable configuration such as a small number of requests per IP per minute.
• Apply that limiter as middleware to those routes only, so that the rest of the application’s behavior is unchanged.

Concretely:

• At the top of dashboard/index.js, after the existing require statements, add:
  const rateLimit = require("express-rate-limit");

• In the initialization section (around where this.commandsRaw is set and routes are defined), before the /api/login route is registered, define an authLimiter:

  const authLimiter = rateLimit({
    windowMs: 15 * 60 * 1000, // 15 minutes
    max: 100, // limit each IP to 100 requests per window
    standardHeaders: true,
    legacyHeaders: false
  });

• Modify the /api/login and /api/login/verify handlers to include authLimiter as middleware:
  app.post("/api/login", authLimiter, async (req, res) => { ... });
app.post("/api/login/verify", authLimiter, async (req, res) => { ... });

These changes are fully local to dashboard/index.js, preserve the existing logic inside the handlers, and introduce robust rate limiting on the sensitive endpoints.

In general, the problem is that Utils.uid() relies on Math.random(), which is not cryptographically secure. To fix this, we should replace the Math.random()-based portion with randomness from a cryptographically secure PRNG. On Node.js, that means using the crypto module (for example, crypto.randomBytes or crypto.randomUUID). The goal is to keep the overall behavior (a reasonably short, uppercased string ID that can be generated anywhere Utils.uid() is used) while removing dependence on Math.random().

The best targeted fix is to:

1. Import Node’s built-in crypto module in src/Utils.mjs.
2. Rewrite Utils.uid() so that it uses crypto.randomBytes to generate the random component instead of Math.random(). We can still prefix with the timestamp (as before) to preserve the “Date + random” structure, but the random part should come from secure bytes converted to base36 (or hex) and then uppercased. This keeps the type and general format of the ID similar while making it unpredictable.
3. Leave all call sites (like this.uid = Utils.uid(); in src/CommandHandler.mjs) unchanged, since the function signature and return type remain a string.

Concretely, in src/Utils.mjs we add import crypto from "node:crypto"; (or import { randomBytes } from "node:crypto";) at the top, and change uid() to something like:

static uid() {
  const timePart = Date.now().toString(36);
  const randomPart = crypto.randomBytes(8).toString("hex"); // or base64url-like processing if desired
  return (timePart + randomPart).toUpperCase();
}

This does not change how uid() is used anywhere else and removes the insecure Math.random() call entirely.

In general, to fix insecure randomness, replace uses of Math.random() (or similar weak PRNGs) in any code that generates identifiers, tokens, or secrets that might be security-sensitive with a cryptographically secure RNG. In Node.js, this typically means using the crypto module (crypto.randomBytes, or crypto.randomInt / UUIDs, etc.).

For this codebase, the single best fix is to update Utils.uid() in src/Utils.mjs to no longer use Math.random(), but instead derive the random portion from crypto.randomBytes. We can keep the overall structure (date-based prefix plus random suffix, uppercased base‑36 string) so we don’t change the type, format, or rough length of uid values, only how unpredictable they are. A reasonable implementation:

• Import Node’s built-in crypto module in src/Utils.mjs.
• In Utils.uid(), keep the timestamp part as is (new Date().valueOf().toString(36)), but replace Math.random().toString(36).substr(2) with something like crypto.randomBytes(16).toString("base64url") or crypto.randomBytes(8).toString("hex"), converted to upper case to maintain the original behavior. To preserve the original “letters+digits” style, we can use hex or base64url; either is fine for uniqueness and security. The exact alphabet is unlikely to be security-relevant, but we will keep the ID as an uppercase string with no whitespace.

Concretely:

• Edit src/Utils.mjs:

  • Add import crypto from "node:crypto"; (or import * as crypto from "node:crypto";) at the top alongside existing imports (there are none currently, so we just add it).
  • Replace the body of static uid() so that it uses crypto.randomBytes instead of Math.random(). For example:

  static uid() {
    const timePart = new Date().valueOf().toString(36);
    const randomPart = crypto.randomBytes(16).toString("hex");
    return (timePart + randomPart).toUpperCase();
  }

This change keeps Utils.uid() returning an uppercase string identifier and does not require any changes in src/CommandHandler.mjs, because it continues to call Utils.uid() in the same way.