Fix/secret handling hardening

.github/workflows/ci.yml

@@ -25,7 +25,7 @@ env:
   # images in the service Dockerfiles. Note `[golangci-lint] version` is
   # coupled too — golangci-lint only supports Go versions <= the one it was
   # built with, so bumping this without bumping that panics the linter.
-  GO_VERSION: "1.26.5"
+  GO_VERSION: "1.26.6"
 
   # Test environment variables (needed for Python tests)
   GCP_PROJECT_ID: "test-project"
@@ -313,6 +313,22 @@ jobs:
             exit 1
           fi
 
+      # Go stdlib CVEs are invisible to Dependabot: they are fixed by the
+      # toolchain, not by a dependency bump, so nothing else in CI would have
+      # caught GO-2026-5037/5039/5856. This step is the only thing watching that
+      # surface, and it is why GO_VERSION is pinned to an exact patch rather
+      # than a "1.26" floor — the pin is what the gate is asserting.
+      #
+      # govulncheck reports only vulnerabilities reachable from the call graph,
+      # so an advisory in a module we require but never call does not fail the
+      # build. That keeps it low-noise; it also means a refactor can turn a
+      # previously-silent advisory into a failure, which is the intended signal.
+      - name: Check for known Go vulnerabilities (govulncheck)
+        run: |
+          go install golang.org/x/vuln/cmd/govulncheck@latest
+          cd packages/${{ matrix.package }}
+          govulncheck ./...
+
   # ===========================================================================
   # Web/React: Test, Lint, Format, Type check, Build
   # ===========================================================================

go.work

@@ -1,4 +1,4 @@
-go 1.26.5
+go 1.26.6
 
 use (
 	./packages/apigateway

packages/apigateway/go.mod

@@ -1,6 +1,6 @@
 module github.com/andy-esch/desirelines/packages/apigateway
 
-go 1.26.5
+go 1.26.6
 
 require (
 	cloud.google.com/go/firestore v1.25.0

packages/dispatcher/go.mod

@@ -1,6 +1,6 @@
 module github.com/andy-esch/desirelines/packages/dispatcher
 
-go 1.26.5
+go 1.26.6
 
 require (
 	cloud.google.com/go/firestore v1.25.0

packages/shared/go.mod

@@ -1,6 +1,6 @@
 module github.com/andy-esch/desirelines/packages/shared
 
-go 1.26.5
+go 1.26.6
 
 require (
 	cloud.google.com/go/firestore v1.25.0

packages/shared/secrets/secrets.go

@@ -11,10 +11,24 @@ import (
 // LoadFromMount reads a secret from a file path (Infisical mount), falling back
 // to an environment variable. Returns an error if both sources are unavailable,
 // wrapping the original file-read error for debuggability.
+//
+// A present-but-empty (or whitespace-only) mount file is treated as an absent
+// secret, not a successful load of "". Returning ("", nil) meant a caller could
+// not distinguish "the secret is empty" from "the secret loaded fine", and it
+// shadowed the documented env fallback: an empty mount would win over a
+// perfectly good environment variable. It also let the process start having
+// signed things with an empty key. This restores the symmetry the env path
+// already had, where a set-but-empty variable is likewise rejected.
 func LoadFromMount(filePath, envFallback string) (string, error) {
+	var cause error
+
 	data, err := os.ReadFile(filePath) //nolint:gosec // Paths come from trusted config constants
-	if err == nil {
-		return strings.TrimSpace(string(data)), nil
+	if err != nil {
+		cause = fmt.Errorf("failed to read %s: %w", filePath, err)
+	} else if secret := strings.TrimSpace(string(data)); secret != "" {
+		return secret, nil
+	} else {
+		cause = fmt.Errorf("%s is present but empty", filePath)
 	}
 
 	if envFallback != "" {
@@ -23,5 +37,5 @@ func LoadFromMount(filePath, envFallback string) (string, error) {
 		}
 	}
 
-	return "", fmt.Errorf("secret unavailable: failed to read %s: %w", filePath, err)
+	return "", fmt.Errorf("secret unavailable: %w", cause)
 }

packages/shared/secrets/secrets_test.go

@@ -25,19 +25,55 @@ func TestLoadFromMount_FileExists(t *testing.T) {
 	}
 }
 
+// A present-but-empty mount must not be reported as a successful load. Before
+// this, LoadFromMount returned ("", nil) here, so a caller could not tell an
+// empty secret from a good one — and the apigateway would go on to sign OAuth
+// state tokens with an empty HMAC key.
 func TestLoadFromMount_FileExistsEmpty(t *testing.T) {
+	for _, tc := range []struct {
+		name     string
+		contents string
+	}{
+		{"empty", ""},
+		{"whitespace only", "   \n\t  \n"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			dir := t.TempDir()
+			path := filepath.Join(dir, "secret")
+			if err := os.WriteFile(path, []byte(tc.contents), 0o600); err != nil {
+				t.Fatal(err)
+			}
+
+			got, err := secrets.LoadFromMount(path, "")
+			if err == nil {
+				t.Fatalf("expected an error, got %q with nil error", got)
+			}
+			if got != "" {
+				t.Errorf("got %q, want empty string alongside the error", got)
+			}
+		})
+	}
+}
+
+// An empty mount must not shadow a usable environment variable. This is the
+// case the old behavior got most wrong: it returned "" successfully and the
+// documented fallback never ran.
+func TestLoadFromMount_FileExistsEmpty_FallsBackToEnv(t *testing.T) {
+	const envKey = "TEST_SECRET_EMPTY_MOUNT_FALLBACK"
+
 	dir := t.TempDir()
 	path := filepath.Join(dir, "secret")
-	if err := os.WriteFile(path, []byte(""), 0o600); err != nil {
+	if err := os.WriteFile(path, []byte("  \n"), 0o600); err != nil {
 		t.Fatal(err)
 	}
+	t.Setenv(envKey, "from-env")
 
-	got, err := secrets.LoadFromMount(path, "")
+	got, err := secrets.LoadFromMount(path, envKey)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
-	if got != "" {
-		t.Errorf("got %q, want empty string", got)
+	if got != "from-env" {
+		t.Errorf("got %q, want %q", got, "from-env")
 	}
 }
 

scripts/database/connect.sh

@@ -80,6 +80,26 @@ fi
 echo -e "${GREEN}✅ Connection string retrieved${NC}"
 echo ""
 
+# Parse the connection string so the password never reaches psql's argv, where
+# any co-tenant could read it from `ps aux` / /proc/<pid>/cmdline for the life of
+# the session. Same sed shapes as scripts/database/migrate.sh:70-72 — one parser
+# for one URI format, rather than a second dialect to keep in sync.
+# Per docs/guides/secure-scripting.md §1 ("No Secrets in Args").
+DB_USER=$(echo "$CONNECTION_STRING" | sed -E 's|^postgresql://([^:]+):.*|\1|')
+DB_PASSWORD=$(echo "$CONNECTION_STRING" | sed -E 's|^postgresql://[^:]+:(.+)@.*|\1|')
+URL_WITHOUT_CREDS=$(echo "$CONNECTION_STRING" | sed -E 's|^postgresql://.+@|postgresql://|')
+
+if [[ -z "$DB_USER" || -z "$DB_PASSWORD" || "$URL_WITHOUT_CREDS" == "$CONNECTION_STRING" ]]; then
+  echo -e "${RED}❌ Could not parse the connection string into user/password/host${NC}"
+  echo -e "${YELLOW}Expected postgresql://user:password@host/database?params${NC}"
+  exit 1
+fi
+
+# Intentionally do not echo DB_USER, DB_PASSWORD, or CONNECTION_STRING — they
+# contain credentials. Per docs/guides/secure-scripting.md ("No Echoing Secrets").
+
 # Connect via psql
 echo -e "${GREEN}🚀 Connecting to PostgreSQL...${NC}"
-psql "$CONNECTION_STRING"
+# PGPASSWORD is exported only for this psql invocation, so it is not inherited
+# by anything the session spawns (e.g. psql's \! shell escape).
+PGPASSWORD="$DB_PASSWORD" psql "$URL_WITHOUT_CREDS" --username "$DB_USER"

scripts/ops/webhook-management.sh

@@ -249,8 +249,38 @@ EOF
       "   You will stop receiving activity events until you create a new subscription."
 
     echo "Deleting webhook subscription..."
-    curl -s -X DELETE \
-      "https://www.strava.com/api/v3/push_subscriptions/$SUBSCRIPTION_ID?client_id=$CLIENT_ID&client_secret=$CLIENT_SECRET" | jq .
+    # Feed the request line over stdin, never argv, so the secret is not visible
+    # in `ps aux` / /proc/<pid>/cmdline for the life of the call.
+    # See docs/guides/secure-scripting.md §1 (No Secrets in Args).
+    #
+    # PARTIAL FIX — the secret remains in the query string, and therefore in
+    # Strava's access logs and any intermediary proxy's. That is not something
+    # this script can avoid: Strava's v3 API documents client_id and
+    # client_secret as *required query parameters* for DELETE
+    # (https://developers.strava.com/docs/webhooks/), with no request-body form.
+    # Only the local argv exposure is closed here. Rotate the client secret if
+    # you have reason to believe Strava-side logs were exposed.
+    DELETE_STATUS=0
+    DELETE_RESPONSE=$(
+      curl --config - <<EOF
+url = "https://www.strava.com/api/v3/push_subscriptions/$SUBSCRIPTION_ID?client_id=$CLIENT_ID&client_secret=$CLIENT_SECRET"
+request = "DELETE"
+silent
+show-error
+fail-with-body
+EOF
+    ) || DELETE_STATUS=$?
+    # jq runs on a captured variable rather than in a pipe: piping curl into jq
+    # would take jq's exit 0 under `set -o pipefail` semantics and report a
+    # Strava rejection as success. A successful DELETE returns an empty body.
+    if [ -n "$DELETE_RESPONSE" ] && ! printf '%s\n' "$DELETE_RESPONSE" | jq . 2>/dev/null; then
+      printf '%s\n' "$DELETE_RESPONSE"
+    fi
+    if [ "$DELETE_STATUS" -ne 0 ]; then
+      echo "❌ Strava rejected the subscription delete (curl exit $DELETE_STATUS)"
+      exit "$DELETE_STATUS"
+    fi
+    echo "✅ Subscription $SUBSCRIPTION_ID deleted. Clear INFISICAL_STRAVA_WEBHOOK_SUBSCRIPTION_ID."
     ;;
 
   *)