From f602ee4ae408493162650f98a7bc986e400d2969 Mon Sep 17 00:00:00 2001 From: makan Date: Thu, 30 Jul 2026 17:13:33 +0300 Subject: [PATCH 1/5] chore: migrate golangci config to v2 and make the lint gate pass The config was still in v1 format, so golangci-lint v2 refused to load it at all: "unsupported version of the configuration". Every `make lint` and the matching pull request checklist item had been failing on load rather than reporting anything. gosimple has since folded into staticcheck and tenv into usetesting, so both entries are gone. With the config loading, 15 findings surfaced. Fixed the real ones: unused parameters renamed to _, an unchecked w.Write, and a missing package comment. The two G704 SSRF findings are the purpose of the service and carry a nosec note pointing at the checks that do gate it. redactURL now strips control characters. It returned its input unchanged when parsing failed, so it was not the sanitiser the G706 suppressions would be claiming it was; a newline in a caller-supplied URL could have forged log lines on that branch. --- .golangci.yml | 34 +++++++++++++++++++++------------- main.go | 40 ++++++++++++++++++++++++++++++++-------- main_test.go | 19 +++++++++++++++++-- 3 files changed, 70 insertions(+), 23 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index 1f8dfdb..4faadd5 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,14 +1,12 @@ +version: "2" + run: timeout: 3m linters: + # errcheck, govet, ineffassign, staticcheck and unused are enabled by + # default. gosimple was folded into staticcheck, and tenv into usetesting. enable: - - errcheck - - gosimple - - govet - - ineffassign - - staticcheck - - unused - asciicheck - bodyclose - errorlint @@ -17,13 +15,23 @@ linters: - noctx - prealloc - revive - - tenv - unconvert + - usetesting - wastedassign + settings: + errcheck: + # Closing a response body cannot fail in a way the handler can act on. + exclude-functions: + - (io.ReadCloser).Close + exclusions: + rules: + - path: _test\.go + linters: + - errcheck + - gosec + # Test requests are synthetic and never escape the process. + - noctx -issues: - exclude-rules: - - path: _test\.go - linters: - - errcheck - - gosec +formatters: + enable: + - gofmt diff --git a/main.go b/main.go index 799c610..12e8674 100644 --- a/main.go +++ b/main.go @@ -1,3 +1,8 @@ +// Command image-proxy is a stateless streaming image proxy. It fetches an +// image from an upstream URL and streams it back to the caller without +// caching, storing, or transforming it. Fetching attacker-supplied URLs is the +// point of the service, so the SSRF, size, and rate controls in this file are +// the security boundary rather than incidental hardening. package main import ( @@ -35,12 +40,23 @@ const ( var inFlight = make(chan struct{}, maxInFlight) -// redactURL strips query parameters from a URL for safe logging, keeping -// the scheme, host, and path visible. +// redactURL strips query parameters from a URL for safe logging, keeping the +// scheme, host, and path visible. Control characters are removed from whatever +// it returns: the input is caller-supplied, and a bare newline in it would let +// the caller forge log lines. url.ParseRequestURI already rejects such input on +// the request path, so this is defence in depth for the error branch below, +// which returns the string unparsed. func redactURL(rawURL string) string { - u, err := url.Parse(rawURL) + clean := strings.Map(func(r rune) rune { + if r < 0x20 || r == 0x7f { + return -1 + } + return r + }, rawURL) + + u, err := url.Parse(clean) if err != nil || u.Query().Encode() == "" { - return rawURL + return clean } u.RawQuery = "redacted" return u.String() @@ -88,7 +104,7 @@ func hostAllowed(host string) bool { // the hostname up front cannot prevent DNS rebinding, because the client // resolves the name a second time when it dials. Applies to every hop, // including redirects. -func ssrfControl(network, address string, _ syscall.RawConn) error { +func ssrfControl(_, address string, _ syscall.RawConn) error { if disableSSRFCheck { return nil } @@ -377,13 +393,13 @@ func main() { } } -func healthHandler(w http.ResponseWriter, r *http.Request) { +func healthHandler(w http.ResponseWriter, _ *http.Request) { writeSecurityHeaders(w) w.Header().Set("Content-Type", "text/plain; charset=utf-8") - w.Write([]byte("OK")) + _, _ = w.Write([]byte("OK")) } -func optionsHandler(w http.ResponseWriter, r *http.Request) { +func optionsHandler(w http.ResponseWriter, _ *http.Request) { writeCORS(w) writeSecurityHeaders(w) w.WriteHeader(http.StatusNoContent) @@ -400,6 +416,7 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { return } if len(imageURL) > maxURLLength { + // #nosec G706 -- only the length is logged, never the URL itself. log.Printf("URL too long: %d bytes", len(imageURL)) http.Error(w, "URL too long", http.StatusBadRequest) return @@ -412,6 +429,7 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { } if !hostAllowed(u.Host) { + // #nosec G706 -- redactURL strips control characters and the query string. log.Printf("Blocked host not in ALLOWED_HOSTS: url=%s", redactURL(imageURL)) http.Error(w, "Blocked: target host is not allowed", http.StatusForbidden) return @@ -419,6 +437,7 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { if !disableSSRFCheck { if blocked, reason := isBlockedHost(u.Host); blocked { + // #nosec G706 -- redactURL strips control characters and the query string. log.Printf("Blocked SSRF attempt: url=%s reason=%s", redactURL(imageURL), reason) http.Error(w, "Blocked: target host is not allowed", http.StatusForbidden) return @@ -436,6 +455,9 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { return } + // #nosec G704 -- fetching a caller-supplied URL is what this service does. + // The target has already passed hostAllowed and isBlockedHost, and the + // dialer re-checks the resolved IP in ssrfControl on every hop. originReq, err := http.NewRequestWithContext(r.Context(), http.MethodGet, imageURL, nil) if err != nil { log.Printf("Failed to create origin request: %v", err) @@ -446,6 +468,7 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { originReq.Header.Set(k, v) } + // #nosec G704 -- see above; originClient pins the SSRF check to the dial. resp, err := originClient.Do(originReq) if err != nil { status := http.StatusBadGateway @@ -489,6 +512,7 @@ func proxyHandler(w http.ResponseWriter, r *http.Request) { if err != nil { log.Printf("Error streaming response body: %v", err) } + // #nosec G706 -- redactURL strips control characters and the query string. log.Printf("%s status=%d bytes=%d duration=%s", redactURL(imageURL), resp.StatusCode, written, time.Since(start)) } diff --git a/main_test.go b/main_test.go index d754a32..86fe20e 100644 --- a/main_test.go +++ b/main_test.go @@ -105,7 +105,7 @@ func TestSecurityHeaders(t *testing.T) { "Referrer-Policy": "no-referrer", } - origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "image/svg+xml") w.Write([]byte(``)) })) @@ -214,7 +214,7 @@ func TestClientIP(t *testing.T) { } func TestOversizedContentLengthRejected(t *testing.T) { - origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + origin := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "image/png") w.Header().Set("Content-Length", "999999999") // ~1 GB, past the 50 MB cap w.WriteHeader(200) @@ -226,6 +226,21 @@ func TestOversizedContentLengthRejected(t *testing.T) { } } +func TestRedactURL(t *testing.T) { + // Query strings hold tokens and must never reach the log. + if got := redactURL("https://cdn.example/a.png?sig=secret"); got != "https://cdn.example/a.png?redacted" { + t.Errorf("query not redacted: %q", got) + } + // A newline would let a caller forge log lines. + if got := redactURL("https://cdn.example/a.png\n2026-01-01 forged entry"); strings.ContainsAny(got, "\r\n") { + t.Errorf("control characters survived: %q", got) + } + // Unparseable input still gets scrubbed rather than returned as-is. + if got := redactURL("::not a url::\x00\n"); strings.ContainsAny(got, "\r\n\x00") { + t.Errorf("control characters survived the error branch: %q", got) + } +} + func TestIsPrivateIP(t *testing.T) { blocked := []string{ "127.0.0.1", "::1", "0.0.0.0", "::", From 0bc2f3818cd19e6cad51429a7814b34084aa3325 Mon Sep 17 00:00:00 2001 From: makan Date: Thu, 30 Jul 2026 17:14:04 +0300 Subject: [PATCH 2/5] ci: add the lint gate and stop running every job twice The push trigger had been widened to every branch so the required status check would fire on pull requests. That was working around a name mismatch in the branch ruleset, which required "build / test" while the job reports as "test"; the ruleset now names the check correctly, so the trigger can go back to main and pull requests. Branch pushes were otherwise producing a duplicate run of the whole workflow, including the Docker build. golangci-lint had config but nothing ran it, even though CONTRIBUTING and the pull request template both ask for it. Add a lint job and make publish depend on it. Tests now run with -race, matching the Makefile. --- .github/workflows/build.yml | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index df1b133..e2748eb 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -2,6 +2,7 @@ name: build on: push: + branches: [main] pull_request: permissions: @@ -19,7 +20,7 @@ jobs: - run: go vet ./... - - run: go test ./... + - run: go test -race -count=1 ./... # Keeps the Dockerfile validated on pull requests, which no longer reach # the publish job. No registry credentials needed. @@ -28,12 +29,25 @@ jobs: context: . push: false + lint: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-go@v7 + with: + go-version: stable + + - uses: golangci/golangci-lint-action@v9 + with: + version: v2.12.2 + publish: # Only pushes to main publish, so pull request runs never receive a token # with packages: write — a branch PR can change this workflow and its code # runs with whatever permissions the run was granted. if: github.event_name == 'push' && github.ref == 'refs/heads/main' - needs: test + needs: [test, lint] runs-on: ubuntu-latest permissions: contents: read From b6023f5453e0049764397a2e644a7c14dfb4685a Mon Sep 17 00:00:00 2001 From: makan Date: Thu, 30 Jul 2026 17:15:10 +0300 Subject: [PATCH 3/5] build: copy all sources into the image and shrink the build context MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Dockerfile named its sources one by one, so a second .go file would have been dropped from the image with no error — the build would simply produce a binary missing that code. Copy go.mod and *.go instead, and add a .dockerignore that excludes everything else. The context was 11 MB, almost all of it the local image-proxy binary and .git, neither of which the build ever needed. Also add -trimpath so the image matches the release binaries. --- .dockerignore | 4 ++++ Dockerfile | 7 +++++-- 2 files changed, 9 insertions(+), 2 deletions(-) create mode 100644 .dockerignore diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..bf1be13 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,4 @@ +# The build context only needs the module file and the Go sources. +* +!go.mod +!*.go diff --git a/Dockerfile b/Dockerfile index ed845c4..18438b3 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,8 +1,11 @@ FROM golang:1.26-alpine AS builder WORKDIR /build -COPY go.mod main.go ./ -RUN CGO_ENABLED=0 go build -ldflags="-s -w" -o image-proxy . +# Listing sources explicitly meant a new .go file was silently left out of +# the image. .dockerignore keeps the context to go.mod and the sources. +COPY go.mod ./ +COPY *.go ./ +RUN CGO_ENABLED=0 go build -trimpath -ldflags="-s -w" -o image-proxy . # distroless static: CA certs included, runs as non-root FROM gcr.io/distroless/static:nonroot From 404a35d58ea8d9912eb903be372cc10d5b1bc913 Mon Sep 17 00:00:00 2001 From: makan Date: Thu, 30 Jul 2026 17:15:25 +0300 Subject: [PATCH 4/5] ci: match uppercase version tags in the release workflow The only tag in the repository is V1.0, and the trigger glob was 'v*', which is case sensitive. The workflow never ran for it, which is why that release carries no binaries at all. Match both cases. Also add --verify-tag so the release step fails rather than creating a tag of its own if the ref is missing, and run the tests with -race here too. --- .github/workflows/release.yml | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 9fd428c..54d38e3 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -2,7 +2,8 @@ name: release on: push: - tags: ['v*'] + # Both cases: the existing V1.0 tag would not have matched a 'v*' glob. + tags: ['v*', 'V*'] permissions: contents: read @@ -21,7 +22,7 @@ jobs: - run: go vet ./... - - run: go test ./... + - run: go test -race -count=1 ./... - name: Build run: | @@ -36,6 +37,7 @@ jobs: done cd dist && sha256sum ./*.tar.gz > checksums.txt - - run: gh release create "$GITHUB_REF_NAME" dist/* --generate-notes + # --verify-tag refuses to invent a tag if the ref is somehow absent. + - run: gh release create "$GITHUB_REF_NAME" dist/* --generate-notes --verify-tag env: GH_TOKEN: ${{ github.token }} From 6658862b3097c2a51050e8dae2c59df52a288423 Mon Sep 17 00:00:00 2001 From: makan Date: Thu, 30 Jul 2026 17:15:50 +0300 Subject: [PATCH 5/5] docs: drop the dead CodeQL badge and document the make targets codeql.yml was removed in favour of CodeQL default setup, so the badge pointed at a workflow that no longer exists and rendered as an error. Default setup has no badge endpoint; scanning results live under the Security tab. Add a Development section covering the Makefile targets, note that lint needs golangci-lint v2, and mention that the release trigger accepts an uppercase tag. --- README.md | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 8199105..2a1b84c 100644 --- a/README.md +++ b/README.md @@ -2,7 +2,6 @@ [![Go version](https://img.shields.io/badge/Go-1.25-00ADD8?logo=go)](https://go.dev) [![Build](https://github.com/schiz0x00/image-proxy/actions/workflows/build.yml/badge.svg)](https://github.com/schiz0x00/image-proxy/actions/workflows/build.yml) -[![CodeQL](https://github.com/schiz0x00/image-proxy/actions/workflows/codeql.yml/badge.svg)](https://github.com/schiz0x00/image-proxy/actions/workflows/codeql.yml) [![License](https://img.shields.io/github/license/schiz0x00/image-proxy)](LICENSE) [![Release](https://img.shields.io/github/v/release/schiz0x00/image-proxy)](https://github.com/schiz0x00/image-proxy/releases) @@ -23,7 +22,7 @@ docker run -p 8080:8080 image-proxy ## Releases -Pushing a `v*` tag builds static binaries for linux and darwin on amd64 and arm64, and attaches them plus `checksums.txt` to a GitHub release. +Pushing a `v*` or `V*` tag builds static binaries for linux and darwin on amd64 and arm64, and attaches them plus `checksums.txt` to a GitHub release. ## Behavior @@ -53,6 +52,18 @@ Pushing a `v*` tag builds static binaries for linux and darwin on amd64 and arm6 | `ALLOWED_HOSTS` | unset (any public host) | Comma-separated hostname allowlist; subdomains are included | | `TRUSTED_PROXY_HOPS` | `0` | Number of reverse proxies in front; enables `X-Forwarded-For` parsing | +## Development + +```sh +make test # go test -race +make lint # golangci-lint (config in .golangci.yml) +make vet fmt # go vet / go fmt +make coverage # writes coverage.html +make build # local binary +``` + +`make lint` needs [golangci-lint](https://golangci-lint.run) v2; the config uses the v2 schema and will not load under v1. + ## Contributing See [CONTRIBUTING.md](CONTRIBUTING.md). Bug reports and feature requests welcome via [issues](https://github.com/schiz0x00/image-proxy/issues).