add golangci-lint (errcheck/gosec/staticcheck/unused/etc.) and clear#131
Merged
Conversation
all surfaced issues Config (.golangci.yml): v2 schema, default-none, enable a curated set of high-signal linters — errcheck, gosec, govet, ineffassign, misspell, staticcheck, unconvert, unused — plus gofmt/goimports formatters. Exclusions skip noisy stdlib I/O closers, the embedded mdaas main packages, and test files for errcheck/gosec (test fixtures generate noisy hits that drown the main signal). CI: add a `golangci-lint` step after govet using the pinned official action. Fixes surfaced by the new linting: Real bugs: - pkg/handlers/smtp/certs.go SignedDNSPEM: PEM-encode error on the private key was being silently swallowed by an empty branch. Now properly returned. - pkg/xodbox/run.go waitForEvents: Notifier.Send errors were discarded. Now logged with the notifier's name. Also captures `h` by value into the goroutine to avoid loop-variable hazards. - pkg/handlers/dns/handler.go: w.WriteMsg failures now logged at Debug. decodeIP switched from ParseInt (which permitted negative values that would silently truncate on the uint32 cast) to ParseUint with a 32-bit size hint. - pkg/handlers/ftp/handler.go getAFS: MkdirAll error on the memfs is logged rather than dropped. Permissions tightened to 0750. - pkg/handlers/ssh/handler.go: io.WriteString error logged. - pkg/mdaas/build.go: MkdirAll error now returned instead of dropped; unnecessary `string(out)` removed (out is already string). Mode tightened to 0750. - pkg/mdaas/embed.go: SetupDirs error logged on init; directory perms tightened from 0755 to 0750, file perms from 0644 to 0600. Annotations (intentional, documented with `// #nosec`): - pkg/handlers/smtp/certs.go TLSConfig: deliberate weak-TLS cert generator; documented in SECURITY.md. - pkg/handlers/httpx/handler.go handleFileEvent: operator-controlled payload_dir, not request input. - pkg/handlers/httpx/payload_build.go sendFile: file is the just-built artifact path, not user input. - pkg/notifiers/webhook/notifier.go SendPost: notifier URL is operator config; posting to it is the point of the notifier. - pkg/xodbox/config.go configFromFile: --config flag value. Plus minor cleanups: unnecessary `string()` conversion in a DNS test, auto-applied goimports/staticcheck QF refactors across the tree (removing redundant embedded-field selectors, tagged switch hints left for follow-up, etc.). Tests still pass under -race.
pkg/handlers/tcp/handler.go is rewritten so each client connection
is handled by a separate handleConn method:
1. log.Fatal on net.Listen failure is replaced with a wrapped error
return. Start's signature already returns error; the previous
code unconditionally killed the process on a listen failure,
making the handler impossible to recover from a port conflict.
2. The read loop appended the entire 4096-byte scratch buffer to
the accumulated packet regardless of how many bytes were
actually read, padding every chunk with whatever stale or
zeroed contents preceded it. Reads now use the (n, err) return,
copy buf[:n] into a fresh chunk slice, and discard whatever
remains.
3. After the read loop exited the previous code unconditionally
called c.Write([]byte{0x90}) — but the loop only exits when
the peer has already closed the connection (EOF) or the read
failed. That write always failed silently. Removed.
4. The original NewEvent accepted a `packet []byte` parameter but
never propagated it to BaseEvent.RawData, so downstream
consumers always saw an empty Data() payload. Fixed: RawData
is now populated from packet, and handleConn dispatches one
DataRecv event per chunk with the actual bytes read.
Also: per-connection cleanup is now via a single defer (Close +
final Disconnect event), so the Disconnect event always fires once
even on read errors.
Tests:
- TestNewEventPropagatesPacketToRawData: pins the new RawData behaviour.
- TestStartReturnsListenError: pre-binds the test port so Start hits
the listen-error branch and verifies it returns rather than exits.
- TestHandlerStartDataAndDisconnect: writes a payload, closes the
connection, and verifies Connect, DataRecv (with correct chunk
bytes), and Disconnect all reach the dispatch channel.
Coverage: pkg/handlers/tcp 87% -> 95%. Full suite still passes
under -race; golangci-lint is clean.
.goreleaser.yaml:
- New `sboms:` section runs syft against each release archive and
emits a SPDX-JSON SBOM ({{.ArtifactName}}.spdx.json) alongside it.
- New `signs:` section runs `cosign sign-blob` against the checksum
file using keyless OIDC, producing $artifact.sig + $artifact.pem.
Because every released asset is hashed into the checksum file,
signing the checksum transitively pins every artifact.
.github/workflows/release.yml:
- Add a top-level `permissions:` block granting `id-token: write`
(required for cosign keyless OIDC), `contents: write` (release
upload), and `packages: write` (ghcr push).
- Install syft via anchore/sbom-action and cosign via the official
sigstore installer ahead of goreleaser.
- Tag the published container with both the release tag (e.g.
v1.2.3) and `:latest`, capture the build-push digest output, and
sign the image by digest with cosign keyless.
goreleaser config validates with `goreleaser check`. Full Go test
suite still passes under -race; golangci-lint is clean.
types.Handler: new Stop(ctx context.Context) error method. Each handler now owns a reference to its underlying server (or listener) and the Stop method shuts it down. Stop is safe to call before Start ever ran and is idempotent. Per-handler implementations: - pkg/handlers/tcp/handler.go: keeps the net.Listener on the Handler and Close()s it on Stop; the Accept loop unblocks with net.ErrClosed and the function returns. A `stopping` flag distinguishes graceful shutdown from a real accept failure. - pkg/handlers/dns/handler.go: switches from the package-level helper dns.HandleFunc + dns.ListenAndServe to constructing a *dns.Server with its own ServeMux. Stop calls ShutdownContext. - pkg/handlers/smtp/handler.go: stores *smtp.Server, Stop calls Shutdown. - pkg/handlers/ftp/handler.go: stores *ftpserver.FtpServer, Stop calls Stop. (ftpserverlib does not accept a context.) - pkg/handlers/ssh/handler.go: switches from the package-level ssh.ListenAndServe + ssh.Handle to constructing a *ssh.Server. Stop calls Shutdown. Also flipped all methods from value to pointer receivers (the old value receivers mutated copies — the closures inside Start only worked by accident). - pkg/handlers/httpx/handler.go: stores *http.Server, Stop calls Shutdown. Watch goroutine is now ctx-cancellable: NewHandler creates a context whose cancel func is invoked from Stop. The loop's `done := make(chan bool); <-done` block has been replaced by `<-ctx.Done()`. HTTPS path is still served by certmagic and is not graceful-shutdown-aware; tracked as follow-up. App-level wiring (pkg/xodbox/run.go): - Added App.Shutdown(): calls Stop(ctx) with a 10s timeout on every handler and unblocks waitForEvents via a stop channel. Idempotent via a recover-guarded close. - Added SIGINT/SIGTERM handling: the goroutine started by Run now installs a signal handler that calls Shutdown when an interrupt arrives, so Ctrl-C properly drains running listeners instead of exiting abruptly. - waitForEvents now selects on the stop channel as well as the event channel. Additional fixes surfaced by the refactor: - pkg/handlers/httpx/handler.go watchDir: filepath.Walk passes fi=nil when stat fails (e.g. directory removed). The previous fi.Mode().IsDir() would panic on nil; now it returns early on the err value. - watchForChanges now checks ctx.Done() before doing any work, so Stop-immediately-after-NewHandler doesn't kick off a doomed walk. Tests: - Per-handler stop_test.go: pre-start no-op behaviour and Stop unblocks Start. The FTP/SSH/SMTP unblocks-Start tests are gated on `//go:build !race` because the upstream libraries (ftpserverlib, gliderlabs/ssh, go-smtp) write their internal listener field without synchronisation, which trips -race. The pre-start no-op test runs under -race regardless. - pkg/xodbox/shutdown_test.go: App.Shutdown invokes every handler's Stop, returns even if a handler errors, and is idempotent under concurrent calls. Full suite passes under -race; golangci-lint is clean.
operational notes Each pkg/handlers/*/_index.md previously listed only the handler key and the listener default. The configuration tables were incomplete (e.g. SMTP only documented `listener`, not the cert behaviour or `AllowInsecureAuth`; HTTPX omitted `api_token` and the embedded mount point; FTP did not document the dispatched events). The pages have been rewritten to follow a consistent structure: Purpose / Behaviour / Configuration / Events / Operational notes Highlights: - DNS: documents the single-answer reflector behaviour and that the reply is always an A record regardless of the question type. - FTP: enumerates every Action emitted by the handler and notes that authentication is rejected unless Credentials is set programmatically. - SMTP: warns explicitly that AllowInsecureAuth is on, the cert is intentionally untrusted (links SECURITY.md), and that the DATA body is read but discarded. - SSH: documents that auth always returns false (no session is ever established) and that the password is logged at debug. - TCP: documents the per-chunk DataRecv events with the new RawData propagation, the Connect/Disconnect events, and that the handler never writes back. - HTTPX: split into General / TLS-ACME / MDaaS sub-tables; documents every key the code actually reads (including api_token, which was missing); records the bot-suppression threshold and the embedded mount point /ixdbxi/. Also fleshes out the webhook notifier doc (was a single sentence) with the JSON payload shape, the failure-handling matrix (2xx/3xx success, 4xx/5xx logged-only, transport errors propagated), and the filter semantics. All handler docs now also document Stop() behaviour from item #4, so operators know what shutdown semantics each handler gives them. No code changes; full test suite still passes, golangci-lint clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
all surfaced issues
Config (.golangci.yml): v2 schema, default-none, enable a curated set of high-signal linters — errcheck, gosec, govet, ineffassign, misspell, staticcheck, unconvert, unused — plus gofmt/goimports formatters. Exclusions skip noisy stdlib I/O closers, the embedded mdaas main packages, and test files for errcheck/gosec (test fixtures generate noisy hits that drown the main signal).
CI: add a
golangci-lintstep after govet using the pinned official action.Fixes surfaced by the new linting:
Real bugs:
hby value into the goroutine to avoid loop-variable hazards.string(out)removed (out is already string). Mode tightened to 0750.Annotations (intentional, documented with
// #nosec):Plus minor cleanups: unnecessary
string()conversion in a DNS test, auto-applied goimports/staticcheck QF refactors across the tree (removing redundant embedded-field selectors, tagged switch hints left for follow-up, etc.).Tests still pass under -race.