feat: enhance security by preventing exposure of sensitive credentials in logs and errors

cmd/vocat/main.go

@@ -462,7 +462,7 @@ func restoreConfiguredCellularData(
 		_, err = manager.SetNetwork(dataContext, entry.ID, networkRequest)
 		cancel()
 		if err != nil {
-			logger.Warn("startup cellular data recovery failed", "device_id", config.ID, "error", err)
+			logger.Warn("startup cellular data recovery failed", "device_id", config.ID)
 			continue
 		}
 		logger.Info("restored protected cellular data route", "device_id", config.ID, "interface", config.Interface)
@@ -490,7 +490,7 @@ func disableAllDeveloperCellularData(
 		_, err = manager.SetNetwork(disableContext, entry.ID, device.NetworkRequest{Enabled: false, Backend: config.DeviceBackend})
 		cancel()
 		if err != nil && ctx.Err() == nil {
-			logger.Warn("developer cleanup: stop cellular data", "device_id", config.ID, "error", err)
+			logger.Warn("developer cleanup: stop cellular data", "device_id", config.ID)
 		}
 	}
 }

internal/device/data.go

@@ -99,7 +99,14 @@ func (manager *Manager) SetNetwork(
 		if candidate.QMIControl == "" || candidate.NetworkInterface == "" {
 			return NetworkResult{}, fmt.Errorf("%w: QMI control device and network interface are required", ErrDataBackendUnavailable)
 		}
-		return setQMINetwork(ctx, candidate, request.Enabled, apn, ipVersion, request.Username, request.Password, authentication)
+		result, err := setQMINetwork(ctx, candidate, request.Enabled, apn, ipVersion, request.Username, request.Password, authentication)
+		if err != nil && (request.Username != "" || request.Password != "") {
+			// qmi-network output is outside our control and may echo values read
+			// from its temporary profile. Do not return that output when the
+			// profile contains credentials.
+			return NetworkResult{}, errors.New("authenticated QMI cellular data operation failed")
+		}
+		return result, err
 	}
 
 	client, err := manager.clientLocked(ctx, state, candidate)
@@ -108,16 +115,32 @@ func (manager *Manager) SetNetwork(
 		return NetworkResult{}, err
 	}
 	if request.Enabled {
-		commands := []string{
-			fmt.Sprintf(`AT+CGDCONT=1,"%s","%s"`, ipVersion, apn),
+		type networkCommand struct {
+			value     string
+			sensitive bool
+		}
+		commands := []networkCommand{
+			{value: fmt.Sprintf(`AT+CGDCONT=1,"%s","%s"`, ipVersion, apn)},
 		}
 		if authentication != "NONE" {
 			authCode := map[string]int{"PAP": 1, "CHAP": 2, "PAP_OR_CHAP": 3}[authentication]
-			commands = append(commands, fmt.Sprintf(`AT+CGAUTH=1,%d,"%s","%s"`, authCode, request.Username, request.Password))
+			commands = append(commands, networkCommand{
+				value:     fmt.Sprintf(`AT+CGAUTH=1,%d,"%s","%s"`, authCode, request.Username, request.Password),
+				sensitive: true,
+			})
 		}
-		commands = append(commands, "AT+CGATT=1", "AT+CGACT=1,1")
+		commands = append(commands,
+			networkCommand{value: "AT+CGATT=1"},
+			networkCommand{value: "AT+CGACT=1,1"},
+		)
 		for _, command := range commands {
-			if _, err := manager.command(ctx, client, command); err != nil {
+			var err error
+			if command.sensitive {
+				_, err = manager.sensitiveCommand(ctx, client, command.value)
+			} else {
+				_, err = manager.command(ctx, client, command.value)
+			}
+			if err != nil {
 				manager.setResult(id, state, nil, err)
 				return NetworkResult{}, err
 			}

internal/device/data_test.go

@@ -3,7 +3,10 @@ package device
 import (
 	"context"
 	"errors"
+	"strings"
 	"testing"
+
+	"vocat/internal/modem"
 )
 
 func TestSetNetworkATBackendActivatesAndDeactivatesPDP(t *testing.T) {
@@ -52,6 +55,35 @@ func TestSetNetworkATBackendAppliesPAPCredentials(t *testing.T) {
 	client.assertDone(t)
 }
 
+func TestSetNetworkDoesNotExposeAPNCredentialsInErrorsOrState(t *testing.T) {
+	const username = "private-user"
+	const password = "private-password"
+	command := `AT+CGAUTH=1,1,"` + username + `","` + password + `"`
+	client := &transcriptClient{steps: []clientStep{
+		{command: `AT+CGDCONT=1,"IPV4V6","giffgaff.com"`, response: okResponse()},
+		{command: command, err: &modem.CommandError{Command: command, Final: "ERROR"}},
+	}}
+	manager, id := newStartedTestManager(t, client)
+	_, err := manager.SetNetwork(context.Background(), id, NetworkRequest{
+		Enabled: true, APN: "giffgaff.com", IPVersion: "IPV4V6",
+		Username: username, Password: password, Authentication: "PAP",
+	})
+	if err == nil {
+		t.Fatal("SetNetwork() error = nil")
+	}
+	if strings.Contains(err.Error(), username) || strings.Contains(err.Error(), password) || strings.Contains(err.Error(), "AT+CGAUTH") {
+		t.Fatalf("SetNetwork() exposed credentials: %q", err)
+	}
+	entry, getErr := manager.Get(id)
+	if getErr != nil {
+		t.Fatal(getErr)
+	}
+	if strings.Contains(entry.LastError, username) || strings.Contains(entry.LastError, password) || strings.Contains(entry.LastError, "AT+CGAUTH") {
+		t.Fatalf("device state exposed credentials: %q", entry.LastError)
+	}
+	client.assertDone(t)
+}
+
 func TestSetNetworkRejectsUnsafeAPNBeforeOpeningModem(t *testing.T) {
 	client := &transcriptClient{}
 	manager, id := newStartedTestManager(t, client)

internal/device/esim_notifications.go

@@ -28,7 +28,7 @@ func encodePositiveInteger(value uint64) []byte {
 	}
 	encoded := make([]byte, 8)
 	for index := len(encoded) - 1; index >= 0; index-- {
-		encoded[index] = byte(value)
+		encoded[index] = byte(value & 0xff)
 		value >>= 8
 	}
 	for len(encoded) > 1 && encoded[0] == 0 {

internal/device/esim_notifications_test.go

@@ -5,6 +5,16 @@ import (
 	"testing"
 )
 
+func TestPositiveIntegerEncodingRoundTripsFullUint64Range(t *testing.T) {
+	for _, value := range []uint64{0, 1, 127, 128, 255, 256, ^uint64(0)} {
+		encoded := encodePositiveInteger(value)
+		decoded, ok := decodePositiveInteger(encoded)
+		if !ok || decoded != value {
+			t.Errorf("round trip %d: encoded=%X decoded=%d ok=%t", value, encoded, decoded, ok)
+		}
+	}
+}
+
 func testNotificationMetadata(t *testing.T, sequence byte, event []byte, address, iccid string) []byte {
 	t.Helper()
 	iccidBCD, err := encodeICCID(iccid)

internal/device/manager.go

@@ -559,3 +559,20 @@ func (manager *Manager) command(
 	}
 	return response, nil
 }
+
+// sensitiveCommand executes an AT command containing credentials or other
+// authentication material. Modem errors commonly echo the complete command,
+// so neither the returned error nor the retained device state may wrap it.
+func (manager *Manager) sensitiveCommand(
+	ctx context.Context,
+	client modem.Client,
+	command string,
+) (modem.Response, error) {
+	commandCtx, cancel := manager.withTimeout(ctx, manager.commandTimeout)
+	defer cancel()
+	response, err := client.Execute(commandCtx, command)
+	if err != nil {
+		return response, errors.New("sensitive modem command failed")
+	}
+	return response, nil
+}

internal/server/automatic_tasks.go

@@ -129,7 +129,10 @@ func (scheduler *automaticTaskScheduler) execute(run store.AutomaticTaskRun) {
 			break
 		}
 		if attempt <= task.RetryCount {
-			scheduler.server.logger.Warn("automatic task attempt failed", "task_id", task.ID, "device_id", task.DeviceID, "attempt", attempt, "error", err)
+			// A device error may contain the full AT command, including APN
+			// credentials. The persisted run retains a user-facing outcome; logs
+			// contain only non-sensitive execution metadata.
+			scheduler.server.logger.Warn("automatic task attempt failed", "task_id", task.ID, "device_id", task.DeviceID, "attempt", attempt)
 			select {
 			case <-scheduler.ctx.Done():
 				break
@@ -274,7 +277,7 @@ func (s *Server) prepareAutomaticTaskEnvironment(ctx context.Context, config *st
 	}
 	if task.TaskType != "public_ip" {
 		if _, err := s.devices.SetNetwork(ctx, physicalID, s.cardNetworkRequest(ctx, physicalID, *config, policy, false)); err != nil {
-			s.logger.Warn("automatic task could not stop unused cellular data", "device_id", config.ID, "error", err)
+			s.logger.Warn("automatic task could not stop unused cellular data", "device_id", config.ID)
 		}
 	}
 	if _, err := s.devices.SetFlight(ctx, physicalID, false); err != nil {
@@ -441,7 +444,7 @@ func (s *Server) rollbackAutomaticNetwork(deviceID, physicalID, iccid string, co
 		policy = store.CardPolicy{ICCID: iccid, APN: config.APN, IPVersion: "IPV4V6"}
 	}
 	if _, err := s.devices.SetNetwork(cleanupContext, physicalID, s.cardNetworkRequest(cleanupContext, physicalID, config, policy, false)); err != nil {
-		s.logger.Warn("stop one-shot automatic roaming data", "device_id", deviceID, "error", err)
+		s.logger.Warn("stop one-shot automatic roaming data", "device_id", deviceID)
 	}
 	config.NetworkEnabled = false
 	if err := s.store.UpsertDevice(cleanupContext, config); err != nil {

internal/server/device_api.go

@@ -1361,8 +1361,10 @@ func (s *Server) writeDeviceError(w http.ResponseWriter, err error) {
 	case errors.Is(err, context.Canceled):
 		writeError(w, http.StatusRequestTimeout, "request_canceled", "the modem request was canceled")
 	default:
-		s.logger.Warn("device operation failed", "error", err)
-		writeError(w, http.StatusBadGateway, "modem_error", err.Error())
+		// Device errors may echo an AT command. Authentication commands can
+		// contain APN credentials, so keep raw errors out of logs and responses.
+		s.logger.Warn("device operation failed")
+		writeError(w, http.StatusBadGateway, "modem_error", "the device operation failed")
 	}
 }
 

internal/server/settings_api.go

@@ -796,6 +796,10 @@ func sendEmailNotificationTest(ctx context.Context, config map[string]any) error
 	if err != nil {
 		return fmt.Errorf("%w: SMTP message rejected", errProviderRejected)
 	}
+	// Addresses are parsed as RFC mailboxes, the subject rejects control
+	// characters, and the body is MIME-base64 encoded by writePlainTextMail.
+	// CodeQL's email-injection query has no sanitizer model for these steps.
+	// codeql[go/email-injection]
 	if err := writePlainTextMail(
 		writer,
 		from,