SonarCloud: 38 open, four worth fixing #238

closed cmc opened this on 2026-09-20 04:43 UTC · security

Discussion

cmc 2026-09-20 04:43 UTC

The SonarCloud dashboard is at 38 open issues. Triaged against the code: one is a real bug, three are accessibility changes worth making, and 34 are false positives or deliberate choices. Two rules account for 29 of the 34 and will keep re-firing on new code, so the recommendation for those is a profile change rather than 29 dismissals.

Fix

  1. Off-site redirect from the trailing-slash handler (`gosecurity:S5146`, BLOCKER, `internal/httpd/routes.go:265`). The `unmatched` handler added in #233 copies `r.URL`, trims the trailing slash from `Path`, and redirects to `u.String()`. The copy carries `Scheme` and `Host`, which `net/http` populates when the request line is in absolute form — a form RFC 7230 requires a server to accept. Reproduced against a mux with the same shape:

    "GET /cmc/ HTTP/1.1"                     -> 301 "/cmc"
    "GET http://evil.example/cmc/ HTTP/1.1"  -> 301 "http://evil.example/cmc"
    "GET //evil.example/ HTTP/1.1"           -> 307 "/evil.example/"
    "GET /cmc/?a=b HTTP/1.1"                 -> 301 "/cmc?a=b"
    

    The protocol-relative case is caught upstream by `ServeMux`'s own `cleanPath`, so this is not a recurrence of #153's item 1 by that route. The absolute-form case is. A browser only sends absolute form to a proxy, so exploitability through one directly is nil; what it reaches is anything in front that forwards the request line as it arrived.

    One line, inside the `if`, before the redirect:

    u.Scheme, u.Host, u.User = "", "", nil

    Clearing after `mux.Handler(trimmed)` rather than before keeps matching untouched; `ServeMux` matches host patterns on the `Host` header, not on `URL.Host`, so it would be safe either way. With the line in place the four cases above give `/cmc`, `/cmc`, `/evil.example/`, `/cmc?a=b`. A test belongs next to the #233 trailing-slash tests.

  2. `autofocus` on three page-level forms (`Web:S9379` ×3): `globalsearch.html:23`, `search.html:6`, `register.html:9`. Each moves focus past the heading, the result count and — on the global search page — the filter navigation, for anyone landing on the page with a screen reader or a keyboard. Nothing depends on it: the form is the first interactive thing on the page in all three, so the first Tab reaches it anyway. Remove the attribute.

Dismiss, individually

  • `gosecurity:S5144` (MAJOR, `routes.go:261`), "SSRF via unsanitized user input", on `mux.Handler(trimmed)`. `ServeMux.Handler` resolves a pattern in memory and opens nothing. The taint engine is modelling any function taking a `*http.Request` as an outbound call. False positive; it will not recur, the call site is one line.
  • `go:S2092` (MINOR, `internal/httpd/flash.go:56`), missing `Secure` on the `gitbay_next` cookie. It is `Secure: s.cfg.HTTP.TLS != "off"`, the same expression as the session cookie at `accounts.go:165` and the other two in `flash.go`, none of which are flagged. Conditional because an operator may terminate TLS in front. Same dismissal as #153's `go:S2092` ×2.
  • `go:S1313` (MINOR, `cmd/gitbay-runner/main.go:485`), hardcoded `169.254.1.2`. Pasta's `–map-host-loopback` default, which is where the instance is from inside a build container; the `buildSSH` comment above it says so. Hoisting it to a named constant would satisfy the rule and move the explanation away from the one place it is needed.
  • `docker:S6471` (MINOR, `deploy/Containerfile.ci:16`), image runs as root. Deliberate and already commented at lines 38-40: a build runs as the image's root inside its own user namespace, mapped to the unprivileged `ci-runner` on the host. A `USER` line would break jobs that install packages without changing what the build can reach on the host.

Dismiss as a group, by deactivating the rule

Both of these fire on a shape this codebase uses everywhere by choice. Dismissing them one at a time means re-dismissing on every new template and every new query — the objection `sonar-project.properties` already records against per-migration dismissals. Turn them off in the `krz` quality profile instead.

  • `Web:S6845` ×16, "tabindex should only be declared on interactive elements". Every one is `tabindex="0"` on a `<pre>`, added deliberately in !430 for WCAG 2.1.1: a `<pre>` that scrolls is a scrollable region and must be reachable by keyboard, which is what axe's `scrollable-region-focusable` requires. S6845 has no exception for it. The two rules disagree and axe is the one that matches the guideline. Sites: `account.html:174`, `adminusers.html:64`, `blame.html:22`, `build.html:15`, `builds.html:38`, `commit.html:12`, `landing.html:7`, `login.html:22,28,32`, `owner.html:131`, `registered.html:6,12`, `search.html:16`, `tree.html:53,54`.
  • `go:S2077` ×13, "dynamically formatted SQL query". #153 checked all 31 that existed then and dismissed them; these 13 are the same shape on code written since. Checked again: every one concatenates package-level constants (`repoSelect`, `mrSelect`, `buildSelect`, `snippetSelect`, `milestoneQuery`) or fragments built from literals — `scopeClause` returns one of two literal strings, `inClause` returns a `?` list sized by a slice length, `labelJoin` holds three table and column names from two package-level values, and both callers of `sharedNameRows` pass string literals. Every value is a `?` placeholder. Sites: `labels.go:45,66,106,113,143`, `repos.go:426,536`, `issues.go:284`, `mrs.go:313`, `milestones.go:147`, `snippets.go:67`, `builds.go:407`, `bookmarks.go:46`. If turning the rule off feels like too much cover, the narrower version is a store test asserting that no query string in `internal/store` reaches `Query`/`QueryRow`/`Exec` through a non-constant — that is the invariant the dismissal is actually claiming, and it is not currently checked by anything.

Keep

`Web:S9379` on `layout.html:194`, the diff line-comment textarea. That form only renders after the reader has asked for it on a specific line, and the page reloads to show it. Without JavaScript, `autofocus` is the only way focus follows the round trip; removing it would put the reader back at the top of a diff they had already navigated. Dismiss as won't fix.

Counts

16 + 4 + 13 + 1 + 1 + 1 + 1 + 1 = 38. Four fixes, 34 dismissals, 29 of which are two profile changes.

closed by cmc in commit 8877319a4c: web: no autofocus on the search and register forms

2026-09-20 05:26 UTC

referenced in commit c56ef893fb by cmc: web: a trailing-slash redirect cannot leave the site

2026-09-20 05:26 UTC

referenced in commit 1371d96586 by cmc: CHANGELOG: v1.31.0

2026-09-20 05:53 UTC