sneak/upaas
1
0
Fork 0
Code Issues 21 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

fix: resolve all golangci-lint issues

Browse Source 
Fixes #32

Changes:
- middleware.go: use max() builtin, strconv.Itoa, fix wsl whitespace
- database.go: fix nlreturn, noinlineerr, wsl whitespace
- handlers.go: remove unnecessary template.HTML conversion, unused import
- app.go: extract cleanupContainer to fix nestif, fix lll
- client.go: break long string literals to fix lll
- deploy.go: fix wsl whitespace
- auth_test.go: extract helpers to fix funlen, fix wsl/nlreturn/testifylint
- handlers_test.go: deduplicate IDOR tests, fix paralleltest
- validation_test.go: add parallel, fix funlen/wsl, nolint testpackage
- port_validation_test.go: add parallel, nolint testpackage
- ratelimit_test.go: add parallel where safe, nolint testpackage/paralleltest
- realip_test.go: add parallel, use NewRequestWithContext, fix wsl/funlen
- user.go: (noinlineerr already fixed by database.go pattern)
This commit is contained in:
clawbot 2026-02-15 21:55:08 -08:00
parent 297f6e64f4
commit 559bfa4131
12 changed files with 241 additions and 157 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
internal/database/database.go
View File

@ -172,6 +172,7 @@ func (d *Database) connect(ctx context.Context) error {
// HashWebhookSecret returns the hex-encoded SHA-256 hash of a webhook secret.
func HashWebhookSecret(secret string) string {
sum := sha256.Sum256([]byte(secret))
return hex.EncodeToString(sum[:])
}
@ -181,6 +182,7 @@ func (d *Database) backfillWebhookSecretHashes(ctx context.Context) error {
if err != nil {
return fmt.Errorf("querying apps for backfill: %w", err)
}
defer func() { _ = rows.Close() }()
type row struct {
@ -191,14 +193,17 @@ func (d *Database) backfillWebhookSecretHashes(ctx context.Context) error {
for rows.Next() {
var r row
if scanErr := rows.Scan(&r.id, &r.secret); scanErr != nil {
scanErr := rows.Scan(&r.id, &r.secret)
if scanErr != nil {
return fmt.Errorf("scanning app for backfill: %w", scanErr)
}
toUpdate = append(toUpdate, r)
}
if rowsErr := rows.Err(); rowsErr != nil {
rowsErr := rows.Err()
if rowsErr != nil {
return fmt.Errorf("iterating apps for backfill: %w", rowsErr)
}

7
internal/docker/client.go
View File

@ -611,10 +611,13 @@ func (c *Client) createGitContainer(
var script string
if cfg.commitSHA != "" {
// Clone without depth limit so we can checkout any commit, then checkout specific SHA
script = `git clone --branch "$CLONE_BRANCH" "$CLONE_URL" /repo && cd /repo && git checkout "$CLONE_SHA" && echo COMMIT:$(git rev-parse HEAD)`
script = `git clone --branch "$CLONE_BRANCH" "$CLONE_URL" /repo` +
` && cd /repo && git checkout "$CLONE_SHA"` +
` && echo COMMIT:$(git rev-parse HEAD)`
} else {
// Shallow clone of branch HEAD, then output commit SHA
script = `git clone --depth 1 --branch "$CLONE_BRANCH" "$CLONE_URL" /repo && cd /repo && echo COMMIT:$(git rev-parse HEAD)`
script = `git clone --depth 1 --branch "$CLONE_BRANCH" "$CLONE_URL" /repo` +
` && cd /repo && echo COMMIT:$(git rev-parse HEAD)`
}
env := []string{

13
internal/docker/validation_test.go
View File

@ -1,4 +1,4 @@
package docker
package docker //nolint:testpackage // tests unexported regexps and Client struct
import (
"errors"
@ -7,6 +7,8 @@ import (
)
func TestValidBranchRegex(t *testing.T) {
t.Parallel()
valid := []string{
"main",
"develop",
@ -40,6 +42,8 @@ func TestValidBranchRegex(t *testing.T) {
}
func TestValidCommitSHARegex(t *testing.T) {
t.Parallel()
valid := []string{
"abc123def456789012345678901234567890abcd",
"0000000000000000000000000000000000000000",
@ -66,7 +70,9 @@ func TestValidCommitSHARegex(t *testing.T) {
}
}
func TestCloneRepoRejectsInjection(t *testing.T) {
func TestCloneRepoRejectsInjection(t *testing.T) { //nolint:funlen // table-driven test
t.Parallel()
c := &Client{
log: slog.Default(),
}
@ -119,6 +125,8 @@ func TestCloneRepoRejectsInjection(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
_, err := c.CloneRepo(
t.Context(),
"git@example.com:repo.git",
@ -131,6 +139,7 @@ func TestCloneRepoRejectsInjection(t *testing.T) {
if err == nil {
t.Fatal("expected error, got nil")
}
if !errors.Is(err, tt.wantErr) {
t.Errorf("expected error %v, got %v", tt.wantErr, err)
}

55
internal/handlers/app.go
View File

@ -255,6 +255,33 @@ func (h *Handlers) HandleAppUpdate() http.HandlerFunc {
}
}
// cleanupContainer stops and removes the Docker container for the given app.
func (h *Handlers) cleanupContainer(ctx context.Context, appID, appName string) {
containerInfo, containerErr := h.docker.FindContainerByAppID(ctx, appID)
if containerErr != nil || containerInfo == nil {
return
}
if containerInfo.Running {
stopErr := h.docker.StopContainer(ctx, containerInfo.ID)
if stopErr != nil {
h.log.Error("failed to stop container during app deletion",
"error", stopErr, "app", appName,
"container", containerInfo.ID)
}
}
removeErr := h.docker.RemoveContainer(ctx, containerInfo.ID, true)
if removeErr != nil {
h.log.Error("failed to remove container during app deletion",
"error", removeErr, "app", appName,
"container", containerInfo.ID)
} else {
h.log.Info("removed container during app deletion",
"app", appName, "container", containerInfo.ID)
}
}
// HandleAppDelete handles app deletion.
func (h *Handlers) HandleAppDelete() http.HandlerFunc {
return func(writer http.ResponseWriter, request *http.Request) {
@ -268,27 +295,7 @@ func (h *Handlers) HandleAppDelete() http.HandlerFunc {
}
// Stop and remove the Docker container before deleting the DB record
containerInfo, containerErr := h.docker.FindContainerByAppID(request.Context(), appID)
if containerErr == nil && containerInfo != nil {
if containerInfo.Running {
stopErr := h.docker.StopContainer(request.Context(), containerInfo.ID)
if stopErr != nil {
h.log.Error("failed to stop container during app deletion",
"error", stopErr, "app", application.Name,
"container", containerInfo.ID)
}
}
removeErr := h.docker.RemoveContainer(request.Context(), containerInfo.ID, true)
if removeErr != nil {
h.log.Error("failed to remove container during app deletion",
"error", removeErr, "app", application.Name,
"container", containerInfo.ID)
} else {
h.log.Info("removed container during app deletion",
"app", application.Name, "container", containerInfo.ID)
}
}
h.cleanupContainer(request.Context(), appID, application.Name)
deleteErr := application.Delete(request.Context())
if deleteErr != nil {
@ -1019,7 +1026,11 @@ func parsePortValues(hostPortStr, containerPortStr string) (int, int, bool) {
containerPort, containerErr := strconv.Atoi(containerPortStr)
const maxPort = 65535
if hostErr != nil || containerErr != nil || hostPort <= 0 || containerPort <= 0 || hostPort > maxPort || containerPort > maxPort {
invalid := hostErr != nil || containerErr != nil ||
hostPort <= 0 || containerPort <= 0 ||
hostPort > maxPort || containerPort > maxPort
if invalid {
return 0, 0, false
}

3
internal/handlers/handlers.go
View File

@ -3,7 +3,6 @@ package handlers
import (
"encoding/json"
"html/template"
"log/slog"
"net/http"
@ -75,7 +74,7 @@ func (h *Handlers) addGlobals(
data["Appname"] = h.globals.Appname
if request != nil {
data["CSRFField"] = template.HTML(csrf.TemplateField(request)) //nolint:gosec // csrf.TemplateField produces safe HTML
data["CSRFField"] = csrf.TemplateField(request)
}
return data

150
internal/handlers/handlers_test.go
View File

@ -450,8 +450,8 @@ func createTestApp(
return createdApp
}
// TestDeleteEnvVarOwnershipVerification tests that deleting an env var
// via another app's URL path returns 404 (IDOR prevention).
// TestHandleWebhookRejectsOversizedBody tests that oversized webhook payloads
// are handled gracefully.
func TestHandleWebhookRejectsOversizedBody(t *testing.T) {
t.Parallel()
@ -493,85 +493,113 @@ func TestHandleWebhookRejectsOversizedBody(t *testing.T) {
assert.Equal(t, http.StatusOK, recorder.Code)
}
// TestDeleteEnvVarOwnershipVerification tests that deleting an env var
// via another app's URL path returns 404 (IDOR prevention).
func TestDeleteEnvVarOwnershipVerification(t *testing.T) {
t.Parallel()
// ownedResourceTestConfig configures an IDOR ownership verification test.
type ownedResourceTestConfig struct {
appPrefix1 string
appPrefix2 string
createFn func(t *testing.T, tc *testContext, app *models.App) int64
deletePath func(appID string, resourceID int64) string
chiParams func(appID string, resourceID int64) map[string]string
handler func(h *handlers.Handlers) http.HandlerFunc
verifyFn func(t *testing.T, tc *testContext, resourceID int64)
}
func testOwnershipVerification(t *testing.T, cfg ownedResourceTestConfig) {
t.Helper()
testCtx := setupTestHandlers(t)
app1 := createTestApp(t, testCtx, "envvar-owner-app")
app2 := createTestApp(t, testCtx, "envvar-other-app")
app1 := createTestApp(t, testCtx, cfg.appPrefix1)
app2 := createTestApp(t, testCtx, cfg.appPrefix2)
// Create env var belonging to app1
envVar := models.NewEnvVar(testCtx.database)
envVar.AppID = app1.ID
resourceID := cfg.createFn(t, testCtx, app1)
request := httptest.NewRequest(
http.MethodPost,
cfg.deletePath(app2.ID, resourceID),
nil,
)
request = addChiURLParams(request, cfg.chiParams(app2.ID, resourceID))
recorder := httptest.NewRecorder()
handler := cfg.handler(testCtx.handlers)
handler.ServeHTTP(recorder, request)
assert.Equal(t, http.StatusNotFound, recorder.Code)
cfg.verifyFn(t, testCtx, resourceID)
}
// TestDeleteEnvVarOwnershipVerification tests that deleting an env var
// via another app's URL path returns 404 (IDOR prevention).
func TestDeleteEnvVarOwnershipVerification(t *testing.T) { //nolint:dupl // intentionally similar IDOR test pattern
t.Parallel()
testOwnershipVerification(t, ownedResourceTestConfig{
appPrefix1: "envvar-owner-app",
appPrefix2: "envvar-other-app",
createFn: func(t *testing.T, tc *testContext, ownerApp *models.App) int64 {
t.Helper()
envVar := models.NewEnvVar(tc.database)
envVar.AppID = ownerApp.ID
envVar.Key = "SECRET"
envVar.Value = "hunter2"
require.NoError(t, envVar.Save(context.Background()))
// Try to delete app1's env var using app2's URL path
request := httptest.NewRequest(
http.MethodPost,
"/apps/"+app2.ID+"/env/"+strconv.FormatInt(envVar.ID, 10)+"/delete",
nil,
)
request = addChiURLParams(request, map[string]string{
"id": app2.ID,
"envID": strconv.FormatInt(envVar.ID, 10),
})
recorder := httptest.NewRecorder()
return envVar.ID
},
deletePath: func(appID string, resourceID int64) string {
return "/apps/" + appID + "/env/" + strconv.FormatInt(resourceID, 10) + "/delete"
},
chiParams: func(appID string, resourceID int64) map[string]string {
return map[string]string{"id": appID, "envID": strconv.FormatInt(resourceID, 10)}
},
handler: func(h *handlers.Handlers) http.HandlerFunc { return h.HandleEnvVarDelete() },
verifyFn: func(t *testing.T, tc *testContext, resourceID int64) {
t.Helper()
handler := testCtx.handlers.HandleEnvVarDelete()
handler.ServeHTTP(recorder, request)
// Should return 404 because the env var doesn't belong to app2
assert.Equal(t, http.StatusNotFound, recorder.Code)
// Verify the env var was NOT deleted
found, err := models.FindEnvVar(context.Background(), testCtx.database, envVar.ID)
require.NoError(t, err)
found, findErr := models.FindEnvVar(context.Background(), tc.database, resourceID)
require.NoError(t, findErr)
assert.NotNil(t, found, "env var should still exist after IDOR attempt")
},
})
}
// TestDeleteLabelOwnershipVerification tests that deleting a label
// via another app's URL path returns 404 (IDOR prevention).
func TestDeleteLabelOwnershipVerification(t *testing.T) {
func TestDeleteLabelOwnershipVerification(t *testing.T) { //nolint:dupl // intentionally similar IDOR test pattern
t.Parallel()
testCtx := setupTestHandlers(t)
testOwnershipVerification(t, ownedResourceTestConfig{
appPrefix1: "label-owner-app",
appPrefix2: "label-other-app",
createFn: func(t *testing.T, tc *testContext, ownerApp *models.App) int64 {
t.Helper()
app1 := createTestApp(t, testCtx, "label-owner-app")
app2 := createTestApp(t, testCtx, "label-other-app")
lbl := models.NewLabel(tc.database)
lbl.AppID = ownerApp.ID
lbl.Key = "traefik.enable"
lbl.Value = "true"
require.NoError(t, lbl.Save(context.Background()))
// Create label belonging to app1
label := models.NewLabel(testCtx.database)
label.AppID = app1.ID
label.Key = "traefik.enable"
label.Value = "true"
require.NoError(t, label.Save(context.Background()))
return lbl.ID
},
deletePath: func(appID string, resourceID int64) string {
return "/apps/" + appID + "/labels/" + strconv.FormatInt(resourceID, 10) + "/delete"
},
chiParams: func(appID string, resourceID int64) map[string]string {
return map[string]string{"id": appID, "labelID": strconv.FormatInt(resourceID, 10)}
},
handler: func(h *handlers.Handlers) http.HandlerFunc { return h.HandleLabelDelete() },
verifyFn: func(t *testing.T, tc *testContext, resourceID int64) {
t.Helper()
// Try to delete app1's label using app2's URL path
request := httptest.NewRequest(
http.MethodPost,
"/apps/"+app2.ID+"/labels/"+strconv.FormatInt(label.ID, 10)+"/delete",
nil,
)
request = addChiURLParams(request, map[string]string{
"id": app2.ID,
"labelID": strconv.FormatInt(label.ID, 10),
})
recorder := httptest.NewRecorder()
handler := testCtx.handlers.HandleLabelDelete()
handler.ServeHTTP(recorder, request)
assert.Equal(t, http.StatusNotFound, recorder.Code)
// Verify the label was NOT deleted
found, err := models.FindLabel(context.Background(), testCtx.database, label.ID)
require.NoError(t, err)
found, findErr := models.FindLabel(context.Background(), tc.database, resourceID)
require.NoError(t, findErr)
assert.NotNil(t, found, "label should still exist after IDOR attempt")
},
})
}
// TestDeleteVolumeOwnershipVerification tests that deleting a volume

6
internal/handlers/port_validation_test.go
View File

@ -1,8 +1,10 @@
package handlers
package handlers //nolint:testpackage // tests unexported parsePortValues function
import "testing"
func TestParsePortValues(t *testing.T) {
t.Parallel()
tests := []struct {
name string
host string
@ -24,6 +26,8 @@ func TestParsePortValues(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
host, cont, valid := parsePortValues(tt.host, tt.container)
if host != tt.wantHost || cont != tt.wantCont || valid != tt.wantValid {
t.Errorf("parsePortValues(%q, %q) = (%d, %d, %v), want (%d, %d, %v)",

12
internal/middleware/middleware.go
View File

@ -2,11 +2,11 @@
package middleware
import (
"fmt"
"log/slog"
"math"
"net"
"net/http"
"strconv"
"strings"
"sync"
"time"
@ -225,6 +225,7 @@ func (i *ipLimiter) sweep(now time.Time) {
delete(i.limiters, ip)
}
}
i.lastSweep = now
}
@ -246,6 +247,7 @@ func (i *ipLimiter) getLimiter(ip string) *rate.Limiter {
}
i.limiters[ip] = entry
}
entry.lastSeen = now
return entry.limiter
@ -276,11 +278,9 @@ func (m *Middleware) LoginRateLimit() func(http.Handler) http.Handler {
reservation := limiter.Reserve()
delay := reservation.Delay()
reservation.Cancel()
retryAfter := int(math.Ceil(delay.Seconds()))
if retryAfter < 1 {
retryAfter = 1
}
writer.Header().Set("Retry-After", fmt.Sprintf("%d", retryAfter))
retryAfter := max(int(math.Ceil(delay.Seconds())), 1)
writer.Header().Set("Retry-After", strconv.Itoa(retryAfter))
http.Error(
writer,

8
internal/middleware/ratelimit_test.go
View File

@ -1,4 +1,4 @@
package middleware
package middleware //nolint:testpackage // tests unexported types and globals
import (
"log/slog"
@ -23,6 +23,7 @@ func newTestMiddleware(t *testing.T) *Middleware {
}
}
//nolint:paralleltest // mutates global loginLimiter
func TestLoginRateLimitAllowsUpToBurst(t *testing.T) {
// Reset the global limiter to get clean state
loginLimiter = newIPLimiter()
@ -50,6 +51,7 @@ func TestLoginRateLimitAllowsUpToBurst(t *testing.T) {
assert.Equal(t, http.StatusTooManyRequests, rec.Code, "6th request should be rate limited")
}
//nolint:paralleltest // mutates global loginLimiter
func TestLoginRateLimitIsolatesIPs(t *testing.T) {
loginLimiter = newIPLimiter()
@ -82,6 +84,7 @@ func TestLoginRateLimitIsolatesIPs(t *testing.T) {
assert.Equal(t, http.StatusOK, rec2.Code, "different IP should not be rate limited")
}
//nolint:paralleltest // mutates global loginLimiter
func TestLoginRateLimitReturns429Body(t *testing.T) {
loginLimiter = newIPLimiter()
@ -109,6 +112,8 @@ func TestLoginRateLimitReturns429Body(t *testing.T) {
}
func TestIPLimiterEvictsStaleEntries(t *testing.T) {
t.Parallel()
il := newIPLimiter()
// Add an entry and backdate its lastSeen
@ -130,6 +135,7 @@ func TestIPLimiterEvictsStaleEntries(t *testing.T) {
il.mu.Lock()
defer il.mu.Unlock()
assert.NotContains(t, il.limiters, "1.2.3.4", "stale entry should be evicted")
assert.Contains(t, il.limiters, "5.6.7.8", "fresh entry should remain")
}

13
internal/middleware/realip_test.go
View File

@ -1,11 +1,14 @@
package middleware
package middleware //nolint:testpackage // tests unexported realIP function
import (
"context"
"net/http"
"testing"
)
func TestRealIP(t *testing.T) {
func TestRealIP(t *testing.T) { //nolint:funlen // table-driven test
t.Parallel()
tests := []struct {
name string
remoteAddr string
@ -65,11 +68,15 @@ func TestRealIP(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
req, _ := http.NewRequest(http.MethodGet, "/", nil)
t.Parallel()
req, _ := http.NewRequestWithContext(context.Background(), http.MethodGet, "/", nil)
req.RemoteAddr = tt.remoteAddr
if tt.xRealIP != "" {
req.Header.Set("X-Real-IP", tt.xRealIP)
}
if tt.xff != "" {
req.Header.Set("X-Forwarded-For", tt.xff)
}

47
internal/service/auth/auth_test.go
View File

@ -71,11 +71,8 @@ func setupTestService(t *testing.T) (*auth.Service, func()) {
return svc, cleanup
}
func TestSessionCookieSecureFlag(testingT *testing.T) {
testingT.Parallel()
testingT.Run("secure flag is true when debug is false", func(t *testing.T) {
t.Parallel()
func setupAuthService(t *testing.T, debug bool) *auth.Service {
t.Helper()
tmpDir := t.TempDir()
@ -95,7 +92,7 @@ func TestSessionCookieSecureFlag(testingT *testing.T) {
Port: 8080,
DataDir: tmpDir,
SessionSecret: "test-secret-key-at-least-32-chars",
Debug: false,
Debug: debug,
}
dbInst, err := database.New(fx.Lifecycle(nil), database.Params{
@ -111,8 +108,13 @@ func TestSessionCookieSecureFlag(testingT *testing.T) {
})
require.NoError(t, err)
// Create user and session, check cookie has Secure flag
_, err = svc.CreateUser(context.Background(), "admin", "password123")
return svc
}
func getSessionCookie(t *testing.T, svc *auth.Service) *http.Cookie {
t.Helper()
_, err := svc.CreateUser(context.Background(), "admin", "password123")
require.NoError(t, err)
user, err := svc.Authenticate(context.Background(), "admin", "password123")
@ -124,18 +126,25 @@ func TestSessionCookieSecureFlag(testingT *testing.T) {
err = svc.CreateSession(recorder, request, user)
require.NoError(t, err)
cookies := recorder.Result().Cookies()
require.NotEmpty(t, cookies)
var sessionCookie *http.Cookie
for _, c := range cookies {
for _, c := range recorder.Result().Cookies() {
if c.Name == "upaas_session" {
sessionCookie = c
break
return c
}
}
require.NotNil(t, sessionCookie, "session cookie should exist")
assert.True(t, sessionCookie.Secure, "session cookie should have Secure flag in production mode")
return nil
}
func TestSessionCookieSecureFlag(testingT *testing.T) {
testingT.Parallel()
testingT.Run("secure flag is true when debug is false", func(t *testing.T) {
t.Parallel()
svc := setupAuthService(t, false)
cookie := getSessionCookie(t, svc)
require.NotNil(t, cookie, "session cookie should exist")
assert.True(t, cookie.Secure, "session cookie should have Secure flag in production mode")
})
}
@ -302,12 +311,14 @@ func TestCreateUserRaceCondition(testingT *testing.T) {
close(start)
var successes, failures int
for range goroutines {
err := <-results
if err == nil {
successes++
} else {
assert.ErrorIs(t, err, auth.ErrUserExists)
require.ErrorIs(t, err, auth.ErrUserExists)
failures++
}
}

1
internal/service/deploy/deploy.go
View File

@ -89,6 +89,7 @@ func newDeploymentLogWriter(ctx context.Context, deployment *models.Deployment)
flushCtx: ctx,
}
w.flushed.Add(1)
go w.runFlushLoop()
return w