CodeQL flagged 34 alerts in my SQL client. I wrote the worst one

{"c1":"b64:aHVudGVyLTIgcMOkc3N3b3Jk"} decodes to hunter-2 pässword. My own code had saved that test password as base64.

MamaSQL, my open-source SQL client for PostgreSQL, MySQL and SQLite, had 34 open GitHub code-scanning alerts, the oldest from June 26. In October I went through every one. I dismissed four with a written reason and fixed the other thirty, each with a test. This is the triage, the fixes, and the password bug I had written myself.

MamaSQL ships two ways. The desktop build is Tauri and Rust, and it keeps passwords in the Windows Credential Manager. The browser build runs from Docker: a React page plus a small Node server I call the bridge, because a browser can’t open a raw TCP connection to Postgres. Thirty-two of the 34 alerts pointed at the bridge or at the browser build’s password storage.

Five SQL injection alerts in a program that runs your SQL

CodeQL raised five js/sql-injection alerts, all rated high. Four of them (#29 to #32) sat in rawArrayRows, where the bridge hands a statement to Postgres or MySQL. The query endpoint feeds it the text from the editor:

  async query({ id, sql }) {
    const e = need(id);
    ...
    const raw = await rawArrayRows(e.engine, e.conn, sql);

That is the product. A SQL client that refused user-controlled SQL would be a text editor with a connection string. I dismissed the four as won’t fix and wrote the reason into each alert, so the next person to open one doesn’t redo the analysis:

By design: MamaSQL is a SQL client and the bridge’s query endpoint runs the SQL the user typed. … Identifier/value paths are quoted or bound.

That reason holds for the SQL, not for who gets to send it. The bridge has no login: its Origin check stops other websites from driving it through a visitor’s browser, but anything that can reach the port directly can talk to it. By default the compose file publishes the app on every interface, with a comment above it: set BIND_ADDR to 127.0.0.1 on a network you don’t trust.

The fifth, #33, looked like the same noise and turned out to be a bug. createDatabase took the name you typed and stripped every character outside [A-Za-z0-9_]. The stripping kept it from injecting anything, but asking for my-db; x created a database called mydbx, and the UI then selected the name you had typed, which didn’t exist. The new test went red against a live Postgres: status 200, and mydbx sitting in psql. Now a name either matches as typed or gets a 400:

    const safe = String(name ?? "");
    if (!/^[A-Za-z_][A-Za-z0-9_]{0,62}$/.test(safe)) {
      const err = new Error(`Invalid database name "${safe}": use letters, digits and _ (not starting with a digit).`);
      err.kind = "badRequest";

POST /api/toString answered 200

Twenty-five alerts (#2 to #26) shared one rule, js/unvalidated-dynamic-method-call: the bridge called a function picked by a name from the request. Two patterns produced them. Engine quoting was looked up as quote[engine], with the engine name the client sent, and the endpoint itself was handlers[path], a plain object indexed by the URL.

The second pattern got a test before any fix. The test posted to /api/toString, and the bridge answered 200, because the lookup had walked up to Object.prototype.toString and called it. The fix built the endpoints into a Map from the handler object. The test now asks for toString, constructor, hasOwnProperty, __proto__ and valueOf, and expects a 404 notFound for each.

quote[engine] was safer than it looked, since connect() already refused unknown engines. Nothing in the lookup itself guaranteed that, though. A dialect(engine) function now returns the Postgres or MySQL helpers from an explicit if and throws a 400 for anything else, and a test opens a connection with engine toString and checks that nothing lands in the pool.

A backslash that broke Markdown tables

Alert #1 was js/incomplete-sanitization in the result export. toMarkdown escaped | as \| but left a cell’s own backslashes alone. For a cell like a\|b, the added escape paired up with the backslash already in the text, and the pipe ended the cell. The two new tests with a backslash in the cell failed on the old code, then passed once backslashes were escaped first:

  // Backslashes first: otherwise a cell's own "\" would swallow the escape we add before "|".
  const cell = (v: unknown) =>
    v == null ? "" : String(v).replace(/\\/g, "\\\\").replace(/\|/g, "\\|").replace(/\n/g, " ");

The base64 fallback I wrote in June

Alert #28 was js/clear-text-storage-of-sensitive-data, and it was the one that mattered. The browser build saves connection passwords so it can reopen connections after a reload. Since June they had been encrypted with AES-GCM under a key the page can’t export, with that key kept in IndexedDB. The ciphertext itself went into localStorage['mamasql.secrets'], tagged enc:. This is the save path in src/ipc/http.ts before the fix:

  try {
    const key = await getKey();
    const iv = crypto.getRandomValues(new Uint8Array(12));
    ...
    s[id] = `enc:${b64(iv.buffer)}:${b64(ct)}`;
  } catch {
    s[id] = `b64:${btoa(unescape(encodeURIComponent(password)))}`;
  }

I wrote that catch on June 24, and my commit message from that day says it outright: “falls back to base64 in insecure contexts.” A page only gets crypto.subtle in a secure context, which means https or localhost. The Docker web container’s nginx listens on plain port 80, published as 5001. Open it from another machine at http://<lan-ip>:5001, and every save took the catch branch and wrote the password as base64. Meanwhile the README said secrets were encrypted with AES-GCM.

Who could read it matters here. The entry sat in that browser’s localStorage, so reading it took access to that browser profile, or script running on the MamaSQL page’s own origin. Anyone with either could decode it in one line.

The test came first. It saves hunter-2 pässword, a test value, then inspects both stores; one case removes crypto.subtle the way plain http does. Against the old code, 5 of 7 tests failed, and one failure printed what was in localStorage:

{"c1":"b64:aHVudGVyLTIgcMOkc3N3b3Jk"}

Moving passwords that were already on disk

The fix lives in a new src/ipc/secrets.ts. Each password is sealed with AES-GCM, and the ciphertext goes into a secrets store in the same IndexedDB database as the key. When the browser can’t encrypt, the password stays in memory for that page and you type it again after a reload:

async function store(id: string, password: string): Promise<void> {
  if (canEncrypt()) {
    try {
      const sealed = await seal(password);
      await request("secrets", "readwrite", (s) => s.put(sealed, id));
      memory.delete(id);
      return;
    } catch {
      /* IndexedDB/crypto refused (e.g. private mode): fall back to memory */
    }
  }
  memory.set(id, password);
}

New saves were only half of it. Every password already in someone’s localStorage would have stayed there. On first use, migrateLegacy() reads the old entry and handles each shape it could hold: enc: ciphertext moves across as it is, b64: and the older bare base64 are decoded and stored again, and then the entry is removed. Two of the seven tests cover the migration, and all seven passed after the change.

The tests run on an in-memory IndexedDB, so I also checked in a browser. The old build had left an enc: entry (53 characters) for the Demo PG connection at that origin. After loading the new build there, the legacy key was gone, IndexedDB was at version 2 with keys and secrets stores, the one saved entry was an {iv, ct} pair, and the app showed Connected · Demo PG. A reload gave the same result. That live run moved an enc: entry; the b64: path is covered by the tests only.

The desktop build was never part of this. It passes the password to secrets::set_password, which writes to the Windows Credential Manager through the keyring crate.

The scan came back with three

PRs #2 and #3 merged on the morning of October 13, I dismissed the four, and after the next CodeQL scan of main 3 of the 34 were still open. One was #34, rated critical: rust/hard-coded-cryptographic-value, in an ignored keychain test that stored the literal s3cret. The test now uses a fresh UUID each run and still passes against the Windows Credential Manager. That alert closed when the Rust scan finished.

The other two were mine to fix again. CodeQL re-located #26 onto the new dispatch line, where endpoints.get(path) still read as a request choosing what to call. The fix is the shape the rule accepts, a membership check and a type check before the call:

  const handler = endpoints.has(path) ? endpoints.get(path) : undefined;
  if (typeof handler !== "function") return sendJson(res, 404, appError("notFound", `Unknown endpoint: ${path}`));

#27, js/stack-trace-exposure, had also survived my first pass. I had stopped errMessage() from stringifying raw thrown values, but it still returned a thrown string as-is, and each reply’s kind was copied straight off the caught error. The message now comes only from an Error, else "Unexpected error", and the kind comes from a fixed list:

const ERROR_KINDS = ["badRequest", "connectionError", "notConnected", "queryError", "internal"];
function errKind(e, fallback) {
  const kind = e && typeof e === "object" ? e.kind : undefined;
  for (const known of ERROR_KINDS) if (kind === known) return known;
  return fallback;
}

This fix, like the #26 one, didn’t start from a red test. The guard test from my first pass checks that a SQL error, an unknown connection and a refused connection come back without stack frames, and it passed before that change too, so the re-scan was the check that counted. PR #4, at 42 passed and 4 skipped in Vitest on its branch, merged at 08:10 UTC, and both alerts read fixed about a minute later. The code-scanning API now returns 0 open alerts for the repository.

What plain http costs now

The browser build on plain http now forgets saved passwords on every reload, which is what it costs to stop writing them out as base64. The open question is whether the Docker image should serve https by default, so that the memory-only path stops being the normal one for anyone who opens MamaSQL from another machine.