harden piscsi-web and flag safe code to SonarQube

.github/workflows/cpp.yml

@@ -146,5 +146,5 @@ jobs:
             --define sonar.sources=cpp,go
             --define sonar.cfamily.gcov.reportsPath=cpp
             --define sonar.go.coverage.reportPaths=go/coverage.txt
-            --define sonar.coverage.exclusions=cpp/test/**
+            --define sonar.coverage.exclusions=cpp/test/**,go/**/*_test.go
             --define sonar.cpd.exclusions=cpp/test/**

cpp/test/test_shared.cpp

@@ -22,9 +22,8 @@ using namespace filesystem;
 
 // Inlude the process id in the temp file path so that multiple instances of the test procedures
 // could run on the same host.
-const path test_data_temp_path(temp_directory_path() /
-                               path(fmt::format("piscsi-test-{}",
-                                                getpid()))); // NOSONAR Publicly writable directory is fine here
+const path test_data_temp_path(temp_directory_path() / //NOSONAR Publicly writable directory is fine for tests
+                               path(fmt::format("piscsi-test-{}", getpid())));
 
 pair<shared_ptr<MockAbstractController>, shared_ptr<PrimaryDevice>> CreateDevice(PbDeviceType type, const string& extension)
 {

go/piscsi-web/internal/server/archive_workflows.go

@@ -184,7 +184,7 @@ func (s *Server) inspectArchive(path string, info os.FileInfo) ([]archiveMember,
 func inspectArchiveMembers(path string) ([]lsarMember, error) {
 	var lsarErr error
 	if _, err := exec.LookPath("lsar"); err == nil {
-		output, commandErr := exec.Command("lsar", "-json", "--", path).Output()
+		output, commandErr := exec.Command("lsar", "-json", "--", path).Output() //NOSONAR path protected by systemd policy
 		if commandErr == nil {
 			var result lsarResult
 			if jsonErr := json.Unmarshal(output, &result); jsonErr == nil {
@@ -289,7 +289,7 @@ func inspectWithBSDTar(path string) ([]lsarMember, error) {
 	if _, err := exec.LookPath("bsdtar"); err != nil {
 		return nil, fmt.Errorf("bsdtar is unavailable")
 	}
-	output, err := exec.Command("bsdtar", "-tf", path).Output()
+	output, err := exec.Command("bsdtar", "-tf", path).Output() //NOSONAR path protected by systemd policy
 	if err != nil {
 		return nil, err
 	}
@@ -501,7 +501,7 @@ func extractArchiveMembers(archivePath, outputDir string, members []archiveMembe
 		for _, member := range members {
 			args = append(args, regexp.QuoteMeta(member.Path))
 		}
-		return exec.Command("unar", args...).CombinedOutput()
+		return exec.Command("unar", args...).CombinedOutput() //NOSONAR path protected by systemd policy
 	}
 	if _, err := exec.LookPath("bsdtar"); err != nil {
 		return nil, fmt.Errorf("neither unar nor bsdtar is available")
@@ -510,5 +510,5 @@ func extractArchiveMembers(archivePath, outputDir string, members []archiveMembe
 	for _, member := range members {
 		args = append(args, member.Path)
 	}
-	return exec.Command("bsdtar", args...).CombinedOutput()
+	return exec.Command("bsdtar", args...).CombinedOutput() //NOSONAR path protected by systemd policy
 }

go/piscsi-web/internal/server/handlers.go

@@ -31,6 +31,12 @@ import (
 	pb "github.com/piscsi/piscsi/go/proto"
 )
 
+const (
+	contentDescriptionHeader = "Content-Description"
+	contentDispositionHeader = "Content-Disposition"
+	fileTransferDescription  = "File Transfer"
+)
+
 // serves the main control page
 func (s *Server) handleIndex(c *gin.Context) {
 	// Get base template data
@@ -537,12 +543,12 @@ type deviceParameterControl struct {
 }
 
 type deviceCatalogEntry struct {
-	Key           string
-	Name          string
-	MaxLUN        int32
-	Removable     bool
-	SupportsFile  bool
-	Parameters    []deviceParameterControl
+	Key                 string
+	Name                string
+	MaxLUN              int32
+	Removable           bool
+	SupportsFile        bool
+	Parameters          []deviceParameterControl
 	UsesNetworkTopology bool
 	NetworkProfiles     []networkTopology
 	Files               []map[string]interface{}
@@ -1361,6 +1367,15 @@ func (s *Server) uploadDestination(formValues map[string]string) (string, error)
 	}
 }
 
+func setAttachmentHeader(c *gin.Context, filename string) {
+	c.Header(contentDispositionHeader, fmt.Sprintf("attachment; filename=%s", filename))
+}
+
+func setFileDownloadHeaders(c *gin.Context, filename string) {
+	c.Header(contentDescriptionHeader, fileTransferDescription)
+	setAttachmentHeader(c, filename)
+}
+
 // handles file downloads
 func (s *Server) handleFilesDownload(c *gin.Context) {
 	filename := c.Query("file")
@@ -1395,6 +1410,22 @@ func (s *Server) handleFilesDownload(c *gin.Context) {
 		c.String(http.StatusBadRequest, "Invalid filename")
 		return
 	}
+	if source == "config" {
+		file, info, err := openRegularFileWithin(sourcePath, filename)
+		if err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				c.String(http.StatusNotFound, "File not found")
+			} else {
+				c.String(http.StatusBadRequest, "Invalid file")
+			}
+			return
+		}
+		defer file.Close()
+
+		setAttachmentHeader(c, filepath.Base(filename))
+		http.ServeContent(c.Writer, c.Request, filepath.Base(filename), info.ModTime(), file)
+		return
+	}
 
 	// Check if file exists
 	if _, err := os.Stat(realPath); os.IsNotExist(err) {
@@ -1998,27 +2029,20 @@ func (s *Server) handleFilesDownloadConfig(c *gin.Context) {
 		return
 	}
 
-	// Construct full path
-	fullPath := filepath.Join(s.config.ConfigDir, fileName)
-
-	// Verify path is within config directory
-	cleanPath := filepath.Clean(fullPath)
-	configDir := filepath.Clean(s.config.ConfigDir)
-	if !strings.HasPrefix(cleanPath, configDir) {
-		c.String(http.StatusBadRequest, "Invalid file path")
-		return
-	}
-
-	// Check if file exists
-	if _, err := os.Stat(fullPath); os.IsNotExist(err) {
-		c.String(http.StatusNotFound, "File not found: %s", fileName)
+	file, info, err := openRegularFileWithin(s.config.ConfigDir, fileName)
+	if err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			c.String(http.StatusNotFound, "File not found: %s", fileName)
+		} else {
+			c.String(http.StatusBadRequest, "Invalid file")
+		}
 		return
 	}
+	defer file.Close()
 
 	// Serve the file for download
-	c.Header("Content-Description", "File Transfer")
-	c.Header("Content-Disposition", fmt.Sprintf("attachment; filename=%s", fileName))
-	c.File(fullPath)
+	setFileDownloadHeaders(c, fileName)
+	http.ServeContent(c.Writer, c.Request, fileName, info.ModTime(), file)
 }
 
 // performs an action on a configuration file (load, delete, or send)
@@ -2087,8 +2111,7 @@ func (s *Server) handleConfigAction(c *gin.Context) {
 			return
 		}
 
-		c.Header("Content-Description", "File Transfer")
-		c.Header("Content-Disposition", fmt.Sprintf("attachment; filename=%s", fileName))
+		setFileDownloadHeaders(c, fileName)
 		c.File(fullPath)
 		return
 	}
@@ -2144,22 +2167,70 @@ func (s *Server) handleLogsLevel(c *gin.Context) {
 	})
 }
 
+const (
+	defaultSystemLogLines = 100
+	maxSystemLogLines     = 1000
+	systemLogTimeout      = 10 * time.Second
+	logsTemplate          = "logs.html"
+	systemLogsTitle       = "PiSCSI System Logs"
+	allLogsScope          = "All logs"
+)
+
+var systemLogScopes = map[string]struct{}{
+	"piscsi":           {},
+	"piscsi-web":       {},
+	"piscsi-oled":      {},
+	"piscsi-ctrlboard": {},
+}
+
+func parseSystemLogRequest(linesValue, scope string) (int, string, error) {
+	lines := defaultSystemLogLines
+	if linesValue != "" {
+		parsed, err := strconv.Atoi(linesValue)
+		if err != nil || parsed < 1 || parsed > maxSystemLogLines {
+			return 0, "", fmt.Errorf("log lines must be between 1 and %d", maxSystemLogLines)
+		}
+		lines = parsed
+	}
+
+	if scope != "" {
+		if _, ok := systemLogScopes[scope]; !ok {
+			return 0, "", fmt.Errorf("invalid log scope")
+		}
+	}
+
+	return lines, scope, nil
+}
+
 // displays system logs
 func (s *Server) handleLogsShow(c *gin.Context) {
-	lines := c.DefaultPostForm("lines", "100")
+	linesValue := c.PostForm("lines")
 	scope := c.PostForm("scope")
+	lines, scope, err := parseSystemLogRequest(linesValue, scope)
+	if err != nil {
+		s.respond(c, ResponseOptions{
+			Error:    true,
+			Message:  err.Error(),
+			Template: logsTemplate,
+			TemplateData: gin.H{
+				"Title": systemLogsTitle,
+				"Scope": allLogsScope,
+				"Lines": defaultSystemLogLines,
+			},
+		})
+		return
+	}
 
 	// Build journalctl command
-	args := []string{}
-	if lines != "" {
-		args = append(args, "-n", lines)
-	}
+	args := []string{"--no-pager", "--lines=" + strconv.Itoa(lines)}
 	if scope != "" {
-		args = append(args, "-u", scope)
+		args = append(args, "--unit="+scope)
 	}
 
 	// Execute journalctl command
-	output, err := s.runSystemCommand("journalctl", args...)
+	ctx, cancel := context.WithTimeout(c.Request.Context(), systemLogTimeout)
+	defer cancel()
+	output, err := s.runSystemCommandContext(ctx, "journalctl", args...)
 
 	logs := string(output)
 	if err != nil {
@@ -2168,26 +2239,26 @@ func (s *Server) handleLogsShow(c *gin.Context) {
 			message += ": " + details
 		}
 		data := s.getBaseTemplateData(c)
-		data["Title"] = "PiSCSI System Logs"
+		data["Title"] = systemLogsTitle
 		data["ErrorMessage"] = message
-		c.HTML(http.StatusInternalServerError, "logs.html", data)
+		c.HTML(http.StatusInternalServerError, logsTemplate, data)
 		return
 	}
 
 	// Prepare scope display text
-	scopeDisplay := "All logs"
+	scopeDisplay := allLogsScope
 	if scope != "" {
 		scopeDisplay = scope
 	}
 
 	// Render the logs template
 	data := s.getBaseTemplateData(c)
 	data["Scope"] = scopeDisplay
-	data["Lines"] = lines
+	data["Lines"] = strconv.Itoa(lines)
 	data["Logs"] = logs
-	data["Title"] = "PiSCSI System Logs"
+	data["Title"] = systemLogsTitle
 
-	c.HTML(http.StatusOK, "logs.html", data)
+	c.HTML(http.StatusOK, logsTemplate, data)
 }
 
 // restarts the system
@@ -3468,7 +3539,7 @@ func (s *Server) handleFilesCreateISO(c *gin.Context) {
 	}
 
 	args := append(append([]string{}, isoArgs...), "-o", isoPath, sourcePath)
-	output, err := exec.Command("genisoimage", args...).CombinedOutput()
+	output, err := exec.Command("genisoimage", args...).CombinedOutput() //NOSONAR path protected by systemd policy
 	if err != nil {
 		_ = os.Remove(isoPath)
 		detail := strings.TrimSpace(string(output))

go/piscsi-web/internal/server/handlers_config_download_test.go

@@ -0,0 +1,62 @@
+package server
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/gin-gonic/gin"
+	"github.com/piscsi/piscsi/go/piscsi-web/internal/config"
+)
+
+func TestConfigurationDownloadsRejectSymlinks(t *testing.T) {
+	gin.SetMode(gin.TestMode)
+	configDir := t.TempDir()
+	secretPath := filepath.Join(t.TempDir(), "secret.json")
+	if err := os.WriteFile(secretPath, []byte("secret"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(secretPath, filepath.Join(configDir, "download.json")); err != nil {
+		t.Skipf("create symbolic link: %v", err)
+	}
+
+	server := &Server{config: &config.Config{ConfigDir: configDir}}
+	configDownloadRequest := httptest.NewRequest(http.MethodPost, "/files/download_config",
+		strings.NewReader(url.Values{"file": {"download.json"}}.Encode()))
+	configDownloadRequest.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	tests := []struct {
+		name    string
+		request *http.Request
+		handler func(*gin.Context)
+	}{
+		{
+			name:    "configuration download endpoint",
+			request: configDownloadRequest,
+			handler: server.handleFilesDownloadConfig,
+		},
+		{
+			name:    "generic download endpoint with config source",
+			request: httptest.NewRequest(http.MethodGet, "/files/download_image?source=config&file=download.json", nil),
+			handler: server.handleFilesDownload,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			response := httptest.NewRecorder()
+			context, _ := gin.CreateTestContext(response)
+			context.Request = test.request
+			test.handler(context)
+			if response.Code != http.StatusBadRequest {
+				t.Fatalf("status = %d, want %d; body = %s", response.Code, http.StatusBadRequest, response.Body.String())
+			}
+			if strings.Contains(response.Body.String(), "secret") {
+				t.Fatal("download response exposed symlink target content")
+			}
+		})
+	}
+}