Merge pull request #84 from Abo-Omar-74/security

security: address gosec findings and improve HTTP security headers

Author: aaronbrethorst

internal/app/handlers.go

@@ -55,5 +55,7 @@ func (app *Application) healthcheckHandler(w http.ResponseWriter, r *http.Reques
 	if !ready {
 		w.WriteHeader(http.StatusInternalServerError)
 	}
-	json.NewEncoder(w).Encode(status)
+	if err := json.NewEncoder(w).Encode(status); err != nil {
+		app.Logger.Warn("failed to write healthcheck response", "error", err)
+	}
 }

internal/app/routes.go

@@ -58,7 +58,8 @@ func (app *Application) Routes(ctx context.Context) http.Handler {
 	router.HandlerFunc(http.MethodGet, "/v1/healthcheck", app.healthcheckHandler)
 	router.Handler(http.MethodGet, "/metrics", middleware.NewCachedPromHandler(ctx, prometheus.DefaultGatherer, 10*time.Second))
 
-	// Wrap router with Sentry middleware
+	// Wrap router with Sentry and securityHeaders middlewares
 	// Return wrapped httprouter instance.
-	return middleware.SentryMiddleware(router)
+	handler := middleware.SentryMiddleware(router)
+	return middleware.SecurityHeaders(handler)
 }

internal/config/backoff_time_store.go

@@ -90,6 +90,8 @@ func DoWithBackoff(ctx context.Context, client *http.Client, req *http.Request,
 }
 
 func calculateNextRetryAt(backoff time.Duration) time.Time {
+	// Adding jitter for backoff retries. Cryptographic randomness is not required.
+	// #nosec G404
 	jitter := time.Duration(rand.Float64() * float64(backoff) * JITTER_FACTOR)
 	backoff += jitter
 	if backoff > MAX_BACKOFF {

internal/config/backoff_time_store_test.go

@@ -164,7 +164,7 @@ func TestBackoffStore(t *testing.T) {
 		before, _ := store.NextRetryAt(serverID)
 		store.UpdateBackoff(serverID)
 		after, _ := store.NextRetryAt(serverID)
-		
+
 		if !after.After(before) {
 			t.Errorf("expected NextRetryAt to move forward after backoff increase, got before=%v after=%v", before, after)
 		}

internal/config/config_loader.go

@@ -9,6 +9,7 @@ import (
 	"log/slog"
 	"net/http"
 	"os"
+	"path/filepath"
 	"time"
 
 	"github.com/getsentry/sentry-go"
@@ -67,11 +68,20 @@ func refreshConfig(ctx context.Context, client *http.Client, configURL, configAu
 // LoadConfigFromFile reads a JSON configuration file from disk and unmarshals it
 // into a list of OBA server configurations (`[]models.ObaServer`).
 //
+// For security reasons, only files named `config.json` are allowed to be loaded.
+// Without this restriction, a user could supply any file path on the machine
+// (e.g., /etc/passwd), and the application would attempt to read it.
+//
 // On error, it reports issues to Sentry and returns a descriptive error.
 //
 // This function is used when the application is configured to load its server list
 // from a static file using the --config-file flag.
 func loadConfigFromFile(filePath string) ([]models.ObaServer, error) {
+	if filepath.Base(filePath) != "config.json" {
+		return nil, fmt.Errorf("invalid config file name: %s (only config.json is allowed)", filePath)
+	}
+
+	// #nosec G304 - file path validated by restricting to config.json
 	data, err := os.ReadFile(filePath)
 	if err != nil {
 		report.ReportErrorWithSentryOptions(err, report.SentryReportOptions{

internal/config/config_loader_test.go

@@ -10,6 +10,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 	"time"
@@ -29,24 +30,20 @@ func TestLoadConfigFromFile(t *testing.T) {
 		"gtfs_rt_api_key": "",
 		"gtfs_rt_api_value": ""
 		}]`
-		tmpFile, err := os.CreateTemp("", "config-*.json")
-		if err != nil {
-			t.Fatalf("Failed to create temporary file: %v", err)
-		}
-		defer os.Remove(tmpFile.Name())
 
-		if _, err := tmpFile.Write([]byte(content)); err != nil {
-			t.Fatalf("Failed to write to temporary file: %v", err)
+		dir := t.TempDir()
+		fp := filepath.Join(dir, "config.json")
+		if err := os.WriteFile(fp, []byte(content), 0o600); err != nil {
+			t.Fatalf("write config.json: %v", err)
 		}
-		tmpFile.Close()
 
-		servers, err := loadConfigFromFile(tmpFile.Name())
+		servers, err := loadConfigFromFile(fp)
 		if err != nil {
 			t.Fatalf("loadConfigFromFile failed: %v", err)
 		}
 
 		if len(servers) != 1 {
-			t.Fatalf("Expected 1 server, got %d", len(servers))
+			t.Fatalf("expected 1 server, got %d", len(servers))
 		}
 
 		expected := models.ObaServer{
@@ -62,33 +59,29 @@ func TestLoadConfigFromFile(t *testing.T) {
 		}
 
 		if servers[0] != expected {
-			t.Errorf("Expected server %+v, got %+v", expected, servers[0])
+			t.Errorf("expected %+v, got %+v", expected, servers[0])
 		}
 	})
 
 	t.Run("InvalidJSON", func(t *testing.T) {
+		dir := t.TempDir()
+		fp := filepath.Join(dir, "config.json")
 		content := `{ this is not valid JSON }`
-		tmpFile, err := os.CreateTemp("", "invalid-config-*.json")
-		if err != nil {
-			t.Fatalf("Failed to create temporary file: %v", err)
+		if err := os.WriteFile(fp, []byte(content), 0o600); err != nil {
+			t.Fatalf("write invalid config.json: %v", err)
 		}
-		defer os.Remove(tmpFile.Name())
 
-		if _, err := tmpFile.Write([]byte(content)); err != nil {
-			t.Fatalf("Failed to write to temporary file: %v", err)
-		}
-		tmpFile.Close()
-
-		_, err = loadConfigFromFile(tmpFile.Name())
-		if err == nil {
-			t.Errorf("Expected error with invalid JSON, got none")
+		if _, err := loadConfigFromFile(fp); err == nil {
+			t.Errorf("expected error with invalid JSON, got none")
 		}
 	})
 
 	t.Run("NonExistentFile", func(t *testing.T) {
-		_, err := loadConfigFromFile("non-existent-file.json")
-		if err == nil {
-			t.Errorf("Expected error for non-existent file, got none")
+		dir := t.TempDir()
+		fp := filepath.Join(dir, "config.json")
+
+		if _, err := loadConfigFromFile(fp); err == nil {
+			t.Errorf("expected error for non-existent file, got none")
 		}
 	})
 }

internal/config/test_helpers.go

@@ -10,4 +10,4 @@ type mockRoundTripper struct {
 func (m *mockRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
 	m.calls++
 	return m.handler(req)
-}
+}

internal/gtfs/gtfs_bundles.go

@@ -7,7 +7,6 @@ import (
 	"log/slog"
 	"net/http"
 	"net/url"
-	"os"
 	"strconv"
 	"sync"
 	"time"
@@ -204,34 +203,6 @@ func storeGTFSBundle(staticBundle *remoteGtfs.Static, serverID int, staticStore
 	return nil
 }
 
-// ParseGTFSFromCache reads a GTFS bundle from the cache and parses it into a gtfs.Static object.
-// It returns the parsed static data or an error if parsing fails.
-func ParseGTFSFromCache(cachePath string, serverID int) (*remoteGtfs.Static, error) {
-	fileBytes, err := os.ReadFile(cachePath)
-	if err != nil {
-		report.ReportErrorWithSentryOptions(err, report.SentryReportOptions{
-			Tags: utils.MakeMap("server_id", strconv.Itoa(serverID)),
-			ExtraContext: map[string]interface{}{
-				"cache_path": cachePath,
-			},
-		})
-		return nil, err
-	}
-
-	staticData, err := remoteGtfs.ParseStatic(fileBytes, remoteGtfs.ParseStaticOptions{})
-	if err != nil {
-		report.ReportErrorWithSentryOptions(err, report.SentryReportOptions{
-			Tags: utils.MakeMap("server_id", strconv.Itoa(serverID)),
-			ExtraContext: map[string]interface{}{
-				"cache_path": cachePath,
-			},
-		})
-		return nil, err
-	}
-
-	return staticData, nil
-}
-
 // getStopLocationsByIDs retrieves stop locations by their IDs from the GTFS cache.
 // It returns a map of stop IDs to gtfs.Stop objects.
 

internal/gtfs/gtfs_bundles_test.go

@@ -144,48 +144,6 @@ func TestGetStopLocationsByIDs(t *testing.T) {
 	})
 }
 
-func TestParseGTFSFromCache(t *testing.T) {
-	server := models.ObaServer{ID: 1, Name: "Test"}
-
-	tests := []struct {
-		name      string
-		cachePath string
-		expectErr bool
-	}{
-		{
-			name:      "Valid GTFS file",
-			cachePath: "../../testdata/gtfs.zip",
-			expectErr: false,
-		},
-		{
-			name:      "Invalid path",
-			cachePath: "../../testdata/does-not-exist.zip",
-			expectErr: true,
-		},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			staticData, err := ParseGTFSFromCache(tc.cachePath, server.ID)
-			if tc.expectErr {
-				if err == nil {
-					t.Fatal("expected error but got nil")
-				}
-				return
-			}
-			if err != nil {
-				t.Fatalf("unexpected error: %v", err)
-			}
-			if staticData == nil {
-				t.Fatal("expected staticData to be non-nil")
-			}
-			if len(staticData.Stops) == 0 {
-				t.Error("expected at least one stop in GTFS data, got 0")
-			}
-		})
-	}
-}
-
 func TestFetchAndStoreGTFSRTFeed(t *testing.T) {
 	t.Run("Success Case", func(t *testing.T) {
 		mockServer := setupGtfsServer(t, "gtfs_rt_feed_vehicles.pb")

internal/gtfs/test_helpers.go

@@ -13,13 +13,17 @@ func setupGtfsServer(t *testing.T, fixturePath string) *httptest.Server {
 
 	gtfsFixturePath := getFixturePath(t, fixturePath)
 
+	// Safe: absPath is only used in local tests and not from user input.
+	// #nosec G304
 	gtfsFileData, err := os.ReadFile(gtfsFixturePath)
 	if err != nil {
 		t.Fatalf("Failed to read GTFS-RT fixture file: %v", err)
 	}
 
 	return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.Header().Set("Content-Type", "application/octet-stream")
+		// Writing to ResponseWriter in tests, error can be safely ignored.
+		// #nosec G104
 		w.Write(gtfsFileData)
 	}))
 }
@@ -42,7 +46,8 @@ func readFixture(t *testing.T, fixturePath string) []byte {
 	if err != nil {
 		t.Fatalf("Failed to get absolute path to testdata/%s: %v", fixturePath, err)
 	}
-
+	// Safe: absPath is only used in local tests and not from user input.
+	// #nosec G304
 	data, err := os.ReadFile(absPath)
 	if err != nil {
 		t.Fatalf("Failed to read fixture file: %v", err)