2026-07-31 · security · codeql · core

the scanner found the lesser flow

CodeQL raised five high alerts on spectroscope. Four were false positives. The fifth was real, and the path it pointed at was the harmless one: the dangerous caller was a query parameter it never flagged.

A static analyser is a lead generator, not an oracle. That is a description of what the tool is for, and it changes how you are supposed to answer an alert. This particular run managed to be wrong in both directions. It flagged four flows that were already contained, and on the one method where it was right it pointed at the harmless caller and missed the caller that writes.

The habit worth stealing is small. When an alert lands, do not patch the line it points at. Identify the taint source class it has found, in this case "an id that becomes a path", then go find every caller that produces one, because the scanner only saw the callers its queries model.

five alerts

The code-scanning page showed five open alerts, all high, all java/path-injection or java/xss. Alerts 1, 2 and 4 were false positives: the containment checks in that code are real, but the query does not model them as barriers. One was a direct-child parent equality check; another was a Files.walk without FOLLOW_LINKS combined with deleteIfExists, which removes a symlink rather than the thing it points at. Each dismissal carries a source-anchored argument instead of a shrug, and each of those three then had the scanner-legible guard shape added on top anyway, on the theory that an alert you have to re-argue every quarter is a cost you keep paying.

Alert 3 was real. SessionStore.readSessionEvents(id) resolved its id straight into the sessions directory with no normalize and no containment check, unlike its guarded siblings deleteSession and the controller's exportSession.

the flow it saw, and the one it did not

CodeQL reached that method through GET /api/sessions/{id}/events. That is a path variable, and the servlet container rejects %2F inside a path segment, so exploitation there is awkward. Judged on that flow alone, this is a tidy-up.

The flow it missed is worse, and it is in the same method's caller set. The websocket reads ?resume=<id>, a query parameter, which nothing pre-filters, and feeds it to loadSession, eventCount and the resume constructor. From the commit message on 3b97b2e:

the websocket's ?resume= feeds that seam with a query parameter no container pre-filters, through loadSession, eventCount AND the resume constructor, whose file every later append goes to. A ?resume=../../x could read and then append outside the store

The escalation is in the last clause. The read is bad; the resume constructor stores the resolved file as the store's append target, so every later event written in that session goes to whatever the traversal chose. A read primitive became a write primitive by being handed to a constructor.

The audience for this is bounded: /ws carries a local-origin fence from card 92, so the caller has to be on the machine. A browser page on a foreign origin is refused; a local page, or a local tool that sends no Origin at all, is not. That is not much comfort, because something running locally is precisely the attacker class the 0.3.0 hardening pass and card 74 were built around. Bounding an audience is a mitigation, not a fix.

one seam, so every caller inherits it

The correction is a single resolution point that every id-to-path conversion goes through. From spectro-core/src/main/java/dev/spectroscope/core/session/SessionStore.java:

    /**
     * Resolves a session id to its JSONL file — the ONE place an id becomes a
     * path. The id is caller input on three surfaces (REST path variable, the
     * websocket's {@code ?resume=}, the CLI's {@code --resume}), so the resolved
     * path must stay inside the store: normalized, under the sessions directory,
     * and a DIRECT child of it. Containment rather than an id-shape regex on
     * purpose — any file that really lives in the store stays addressable; the
     * strict shape check remains the HTTP edge's job.
     *
     * @param id the session id as the caller gave it — never trusted as a path
     * @return the contained session file path (it may not exist)
     * @throws IOException when the id resolves outside the sessions directory
     */
    public static Path sessionFile(String id) throws IOException {
        Path file = SESSIONS_DIR.resolve(id + ".jsonl").normalize();
        if (!file.startsWith(SESSIONS_DIR) || !SESSIONS_DIR.equals(file.getParent())) {
            throw new IOException("Not a session id: " + id);
        }
        return file;
    }

Two checks, on purpose, and the comment says why: startsWith is the guard shape scanners recognise, and parent equality is the stronger rule. Only one of them is load-bearing for correctness. The other is there so the next scanner run does not re-raise this, which is a legitimate reason to write a line of code and worth admitting rather than dressing up.

Choosing containment over a shape regex is deliberate too. A regex on the id would have been shorter, but it would make any session file that really lives in the store, yet predates the current id format, unaddressable. The strict shape check still exists. It just belongs at the HTTP edge, where rejecting odd input early is free.

The constructor now refuses before it touches anything:

        try {
            this.file = sessionFile(this.id);
        } catch (IOException outsideStore) {
            // A resume id is caller input (CLI --resume, the socket's ?resume=):
            // one that resolves outside the store is an argument error, not an
            // I/O failure — refused before anything is created or appended to.
            throw new IllegalArgumentException("Not a session id: " + id, outsideStore);
        }

Two tests went red first, readSessionEventsRefusesATraversalId and theResumeConstructorRefusesATraversalId. At the HTTP edge, /api/sessions/{id}/events picked up the same full-match SESSION_ID shape check that export and delete already had, pinned by SessionEventsShapeTest, and the export now serves through the shared seam with a modeled content type plus nosniff.

the alert that survived being fixed

There is a good coda. Alert 5, the fourth and last of the dismissals, was java/xss on the export response body. Session content is caller-shaped text, so the response was changed to stop being anything a browser would parse: application/x-ndjson, nosniff, and Content-Disposition: attachment.

CodeQL re-flagged it as alert 6, at the new line. Its java/xss query models neither a non-HTML content type nor an attachment disposition as a barrier at all, so the rewrite moved the code without moving the alert. That one was dismissed by hand, with the argument verified against the pushed code rather than the intent: the response never reaches an HTML parsing context, the id in the filename passed the full-match shape check so it carries no quote, CR or LF and cannot split a header, and the endpoint is local-origin fenced. The dismissal comment names the condition that reopens it, which is the response ever becoming renderable.

A dismissal is only honest if it says what would make it wrong. Otherwise it is just a way of turning the light off.

what the gate did not pick up

Unrelated to any alert, a javadoc break turned up on this branch. An ImageCopyController @param had drifted from body to req, which had been keeping :spectro-server:javadoc red on main.

The gate did not find it, and could not have. Its javadoc legs are :spectro-core:javadoc and :spectro-orchestrator:javadoc, two of the five Gradle modules, so spectro-server gets no javadoc coverage on any push at all. This one came out of a local ./gradlew javadoc over everything, which is the less flattering version of the story. The pipeline post argues that javadoc belongs in the gate instead of at the release. On this repo that argument currently covers two modules out of five.

The outcome is a number rather than an adjective. Zero open alerts on the code-scanning page, read back through the API instead of eyeballed on the page. The gate on the fix commit reported JUnit 1166, 0 failures and 1 skip across 173 result files, and ./gradlew javadoc ran clean over all five modules locally. The security tab picked up a SECURITY.md policy the same day. No Dependabot alerts are open either, though that boast is smaller than it sounds: twenty-four have been raised over the life of the repo, twenty-one closed by an actual dependency bump and three auto-dismissed.

the lesson

The scanner was wrong in both directions and useful anyway. It raised four flows that were already contained, which cost an afternoon of argument, and it missed the flow that could write, which no amount of staring at its output would have revealed. What it did do was name a taint source class precisely enough to be followed: an id that becomes a path.

Following that class through every caller is the part that found the real problem, and it is the part the tool cannot do for you, because the tool only knows the callers its queries model. The alert is where the work starts, not where it happens.