From 245aeb5ac034a83a292533ad515c03193ae35b19 Mon Sep 17 00:00:00 2001 From: Brandon Satrom Date: Tue, 14 Apr 2026 17:15:56 -0700 Subject: [PATCH 1/2] fix(infra): SQL injection via parameterized queries, admin check on feedback endpoint, restrict device auth Co-Authored-By: Claude Sonnet 4.6 --- lat.md/lat.md | 3 + lat.md/security.md | 30 +++ songbird-firmware/src/audio/SongbirdAudio.cpp | 6 +- .../src/commands/SongbirdEnv.cpp | 191 +++++++++--------- songbird-firmware/src/core/SongbirdState.cpp | 110 ++++++++-- songbird-firmware/src/rtos/SongbirdSync.cpp | 12 +- songbird-firmware/src/rtos/SongbirdSync.h | 3 +- songbird-firmware/src/rtos/SongbirdTasks.cpp | 13 +- .../lambda/analytics/chat-query.ts | 42 ++-- .../lambda/analytics/feedback.ts | 42 ++-- .../lambda/shared/rag-retrieval.ts | 20 +- 11 files changed, 306 insertions(+), 166 deletions(-) create mode 100644 lat.md/lat.md create mode 100644 lat.md/security.md diff --git a/lat.md/lat.md b/lat.md/lat.md new file mode 100644 index 0000000..8eb0df5 --- /dev/null +++ b/lat.md/lat.md @@ -0,0 +1,3 @@ +This directory defines the high-level concepts, business logic, and architecture of this project using markdown. It is managed by [lat.md](https://www.npmjs.com/package/lat.md) — a tool that anchors source code to these definitions. Install the `lat` command with `npm i -g lat.md` and run `lat --help`. + +- [[security]] — Security invariants: parameterized SQL, admin authorization on the feedback endpoint, device serial number scoping. diff --git a/lat.md/security.md b/lat.md/security.md new file mode 100644 index 0000000..3705fe7 --- /dev/null +++ b/lat.md/security.md @@ -0,0 +1,30 @@ +# Security and Data Access + +Security invariants for the Songbird backend Lambda layer — parameterized SQL, authorization checks, and device-scoped data access. + +## Parameterized SQL Queries + +All Aurora queries use RDS Data API `parameters` — never string interpolation or manual quote-escaping. This applies to every `ExecuteStatementCommand` call across the analytics layer. + +The two Lambda functions that write to `analytics.rag_documents` must follow this rule: + +- [[songbird-infrastructure/lambda/shared/rag-retrieval.ts#retrieveRelevantContext]] — vector similarity search passes the embedding and title exclusion list as named parameters (`:embedding`, `:limit`, `:p0`…`:pN`). +- [[songbird-infrastructure/lambda/analytics/feedback.ts#indexPositiveFeedback]] — DELETE and INSERT for the upsert pattern use `:title`, `:content`, `:embedding`, `:metadata` parameters. + +String values that were previously escaped with `.replace(/'/g, "''")` must not return. Parameterized queries eliminate that class of bug entirely. + +## Admin Authorization on Feedback Endpoint + +The `GET /analytics/feedback` route returns all users' query history (questions, generated SQL, usernames). It must verify the caller belongs to the Cognito `Admin` group before returning any data. + +The check reads `cognito:groups` from the JWT claims injected by API Gateway's JWT authorizer. A missing or non-Admin group claim returns 403. This is a defense-in-depth check — the endpoint may also be restricted at the API Gateway level, but the Lambda must not rely solely on that. + +See [[songbird-infrastructure/lambda/analytics/feedback.ts#handler]]. + +## Device Serial Number Authorization + +Chat query requests must supply an explicit `deviceSerialNumbers` array. If the array is absent or empty, the handler returns 403 immediately. + +The previous behavior — falling back to `SELECT DISTINCT serial_number FROM analytics.devices` — granted unrestricted data access to any caller who omitted the field. That fallback has been removed. + +See [[songbird-infrastructure/lambda/analytics/chat-query.ts#handler]]. diff --git a/songbird-firmware/src/audio/SongbirdAudio.cpp b/songbird-firmware/src/audio/SongbirdAudio.cpp index 2f519f6..d283931 100644 --- a/songbird-firmware/src/audio/SongbirdAudio.cpp +++ b/songbird-firmware/src/audio/SongbirdAudio.cpp @@ -196,7 +196,11 @@ void audioPlayMelody(const Melody* melody, uint8_t volume) { // Small gap between notes (unless it's a rest) if (melody->notes[i] != NOTE_REST && i < melody->length - 1) { - vTaskDelay(pdMS_TO_TICKS(TONE_GAP_MS)); + if (useRtosPrimitives()) { + vTaskDelay(pdMS_TO_TICKS(TONE_GAP_MS)); + } else { + delay(TONE_GAP_MS); // Arduino delay — safe before scheduler starts + } } } } diff --git a/songbird-firmware/src/commands/SongbirdEnv.cpp b/songbird-firmware/src/commands/SongbirdEnv.cpp index 6243818..26fb080 100644 --- a/songbird-firmware/src/commands/SongbirdEnv.cpp +++ b/songbird-firmware/src/commands/SongbirdEnv.cpp @@ -495,163 +495,168 @@ void envLogConfigChanges(const SongbirdConfig* oldConfig, const SongbirdConfig* return; } - Serial.println("[Env] Configuration changed from Notehub:"); + #ifdef DEBUG_MODE + DEBUG_SERIAL.println("[Env] Configuration changed from Notehub:"); // Mode if (oldConfig->mode != newConfig->mode) { - Serial.print(" mode: "); - Serial.print(envGetModeName(oldConfig->mode)); - Serial.print(" -> "); - Serial.println(envGetModeName(newConfig->mode)); + DEBUG_SERIAL.print(" mode: "); + DEBUG_SERIAL.print(envGetModeName(oldConfig->mode)); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(envGetModeName(newConfig->mode)); } // Timing if (oldConfig->gpsIntervalMin != newConfig->gpsIntervalMin) { - Serial.print(" gps_interval_min: "); - Serial.print(oldConfig->gpsIntervalMin); - Serial.print(" -> "); - Serial.println(newConfig->gpsIntervalMin); + DEBUG_SERIAL.print(" gps_interval_min: "); + DEBUG_SERIAL.print(oldConfig->gpsIntervalMin); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->gpsIntervalMin); } if (oldConfig->syncIntervalMin != newConfig->syncIntervalMin) { - Serial.print(" sync_interval_min: "); - Serial.print(oldConfig->syncIntervalMin); - Serial.print(" -> "); - Serial.println(newConfig->syncIntervalMin); + DEBUG_SERIAL.print(" sync_interval_min: "); + DEBUG_SERIAL.print(oldConfig->syncIntervalMin); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->syncIntervalMin); } if (oldConfig->heartbeatHours != newConfig->heartbeatHours) { - Serial.print(" heartbeat_hours: "); - Serial.print(oldConfig->heartbeatHours); - Serial.print(" -> "); - Serial.println(newConfig->heartbeatHours); + DEBUG_SERIAL.print(" heartbeat_hours: "); + DEBUG_SERIAL.print(oldConfig->heartbeatHours); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->heartbeatHours); } // Temperature alerts if (oldConfig->tempAlertHighC != newConfig->tempAlertHighC) { - Serial.print(" temp_alert_high_c: "); - Serial.print(oldConfig->tempAlertHighC); - Serial.print(" -> "); - Serial.println(newConfig->tempAlertHighC); + DEBUG_SERIAL.print(" temp_alert_high_c: "); + DEBUG_SERIAL.print(oldConfig->tempAlertHighC); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->tempAlertHighC); } if (oldConfig->tempAlertLowC != newConfig->tempAlertLowC) { - Serial.print(" temp_alert_low_c: "); - Serial.print(oldConfig->tempAlertLowC); - Serial.print(" -> "); - Serial.println(newConfig->tempAlertLowC); + DEBUG_SERIAL.print(" temp_alert_low_c: "); + DEBUG_SERIAL.print(oldConfig->tempAlertLowC); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->tempAlertLowC); } // Humidity alerts if (oldConfig->humidityAlertHigh != newConfig->humidityAlertHigh) { - Serial.print(" humidity_alert_high: "); - Serial.print(oldConfig->humidityAlertHigh); - Serial.print(" -> "); - Serial.println(newConfig->humidityAlertHigh); + DEBUG_SERIAL.print(" humidity_alert_high: "); + DEBUG_SERIAL.print(oldConfig->humidityAlertHigh); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->humidityAlertHigh); } if (oldConfig->humidityAlertLow != newConfig->humidityAlertLow) { - Serial.print(" humidity_alert_low: "); - Serial.print(oldConfig->humidityAlertLow); - Serial.print(" -> "); - Serial.println(newConfig->humidityAlertLow); + DEBUG_SERIAL.print(" humidity_alert_low: "); + DEBUG_SERIAL.print(oldConfig->humidityAlertLow); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->humidityAlertLow); } // Pressure and voltage alerts if (oldConfig->pressureAlertDelta != newConfig->pressureAlertDelta) { - Serial.print(" pressure_alert_delta: "); - Serial.print(oldConfig->pressureAlertDelta); - Serial.print(" -> "); - Serial.println(newConfig->pressureAlertDelta); + DEBUG_SERIAL.print(" pressure_alert_delta: "); + DEBUG_SERIAL.print(oldConfig->pressureAlertDelta); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->pressureAlertDelta); } if (oldConfig->voltageAlertLow != newConfig->voltageAlertLow) { - Serial.print(" voltage_alert_low: "); - Serial.print(oldConfig->voltageAlertLow); - Serial.print(" -> "); - Serial.println(newConfig->voltageAlertLow); + DEBUG_SERIAL.print(" voltage_alert_low: "); + DEBUG_SERIAL.print(oldConfig->voltageAlertLow); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->voltageAlertLow); } // Motion if (oldConfig->motionSensitivity != newConfig->motionSensitivity) { - Serial.print(" motion_sensitivity: "); - Serial.print(getSensitivityName(oldConfig->motionSensitivity)); - Serial.print(" -> "); - Serial.println(getSensitivityName(newConfig->motionSensitivity)); + DEBUG_SERIAL.print(" motion_sensitivity: "); + DEBUG_SERIAL.print(getSensitivityName(oldConfig->motionSensitivity)); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(getSensitivityName(newConfig->motionSensitivity)); } if (oldConfig->motionWakeEnabled != newConfig->motionWakeEnabled) { - Serial.print(" motion_wake_enabled: "); - Serial.print(oldConfig->motionWakeEnabled ? "true" : "false"); - Serial.print(" -> "); - Serial.println(newConfig->motionWakeEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" motion_wake_enabled: "); + DEBUG_SERIAL.print(oldConfig->motionWakeEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->motionWakeEnabled ? "true" : "false"); } // Audio if (oldConfig->audioEnabled != newConfig->audioEnabled) { - Serial.print(" audio_enabled: "); - Serial.print(oldConfig->audioEnabled ? "true" : "false"); - Serial.print(" -> "); - Serial.println(newConfig->audioEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" audio_enabled: "); + DEBUG_SERIAL.print(oldConfig->audioEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->audioEnabled ? "true" : "false"); } if (oldConfig->audioVolume != newConfig->audioVolume) { - Serial.print(" audio_volume: "); - Serial.print(oldConfig->audioVolume); - Serial.print(" -> "); - Serial.println(newConfig->audioVolume); + DEBUG_SERIAL.print(" audio_volume: "); + DEBUG_SERIAL.print(oldConfig->audioVolume); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->audioVolume); } if (oldConfig->audioAlertsOnly != newConfig->audioAlertsOnly) { - Serial.print(" audio_alerts_only: "); - Serial.print(oldConfig->audioAlertsOnly ? "true" : "false"); - Serial.print(" -> "); - Serial.println(newConfig->audioAlertsOnly ? "true" : "false"); + DEBUG_SERIAL.print(" audio_alerts_only: "); + DEBUG_SERIAL.print(oldConfig->audioAlertsOnly ? "true" : "false"); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->audioAlertsOnly ? "true" : "false"); } // Commands if (oldConfig->cmdWakeEnabled != newConfig->cmdWakeEnabled) { - Serial.print(" cmd_wake_enabled: "); - Serial.print(oldConfig->cmdWakeEnabled ? "true" : "false"); - Serial.print(" -> "); - Serial.println(newConfig->cmdWakeEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" cmd_wake_enabled: "); + DEBUG_SERIAL.print(oldConfig->cmdWakeEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->cmdWakeEnabled ? "true" : "false"); } if (oldConfig->cmdAckEnabled != newConfig->cmdAckEnabled) { - Serial.print(" cmd_ack_enabled: "); - Serial.print(oldConfig->cmdAckEnabled ? "true" : "false"); - Serial.print(" -> "); - Serial.println(newConfig->cmdAckEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" cmd_ack_enabled: "); + DEBUG_SERIAL.print(oldConfig->cmdAckEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->cmdAckEnabled ? "true" : "false"); } if (oldConfig->locateDurationSec != newConfig->locateDurationSec) { - Serial.print(" locate_duration_sec: "); - Serial.print(oldConfig->locateDurationSec); - Serial.print(" -> "); - Serial.println(newConfig->locateDurationSec); + DEBUG_SERIAL.print(" locate_duration_sec: "); + DEBUG_SERIAL.print(oldConfig->locateDurationSec); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->locateDurationSec); } // Misc if (oldConfig->ledEnabled != newConfig->ledEnabled) { - Serial.print(" led_enabled: "); - Serial.print(oldConfig->ledEnabled ? "true" : "false"); - Serial.print(" -> "); - Serial.println(newConfig->ledEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" led_enabled: "); + DEBUG_SERIAL.print(oldConfig->ledEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->ledEnabled ? "true" : "false"); } if (oldConfig->debugMode != newConfig->debugMode) { - Serial.print(" debug_mode: "); - Serial.print(oldConfig->debugMode ? "true" : "false"); - Serial.print(" -> "); - Serial.println(newConfig->debugMode ? "true" : "false"); + DEBUG_SERIAL.print(" debug_mode: "); + DEBUG_SERIAL.print(oldConfig->debugMode ? "true" : "false"); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->debugMode ? "true" : "false"); } // GPS Power Management if (oldConfig->gpsPowerSaveEnabled != newConfig->gpsPowerSaveEnabled) { - Serial.print(" gps_power_save_enabled: "); - Serial.print(oldConfig->gpsPowerSaveEnabled ? "true" : "false"); - Serial.print(" -> "); - Serial.println(newConfig->gpsPowerSaveEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" gps_power_save_enabled: "); + DEBUG_SERIAL.print(oldConfig->gpsPowerSaveEnabled ? "true" : "false"); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->gpsPowerSaveEnabled ? "true" : "false"); } if (oldConfig->gpsSignalTimeoutMin != newConfig->gpsSignalTimeoutMin) { - Serial.print(" gps_signal_timeout_min: "); - Serial.print(oldConfig->gpsSignalTimeoutMin); - Serial.print(" -> "); - Serial.println(newConfig->gpsSignalTimeoutMin); + DEBUG_SERIAL.print(" gps_signal_timeout_min: "); + DEBUG_SERIAL.print(oldConfig->gpsSignalTimeoutMin); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->gpsSignalTimeoutMin); } if (oldConfig->gpsRetryIntervalMin != newConfig->gpsRetryIntervalMin) { - Serial.print(" gps_retry_interval_min: "); - Serial.print(oldConfig->gpsRetryIntervalMin); - Serial.print(" -> "); - Serial.println(newConfig->gpsRetryIntervalMin); + DEBUG_SERIAL.print(" gps_retry_interval_min: "); + DEBUG_SERIAL.print(oldConfig->gpsRetryIntervalMin); + DEBUG_SERIAL.print(" -> "); + DEBUG_SERIAL.println(newConfig->gpsRetryIntervalMin); } + #else + (void)oldConfig; + (void)newConfig; + #endif } diff --git a/songbird-firmware/src/core/SongbirdState.cpp b/songbird-firmware/src/core/SongbirdState.cpp index 0bb3973..ac5adb3 100644 --- a/songbird-firmware/src/core/SongbirdState.cpp +++ b/songbird-firmware/src/core/SongbirdState.cpp @@ -9,6 +9,7 @@ #include "SongbirdState.h" #include "SongbirdNotecard.h" #include +#include // ============================================================================= // Module State @@ -189,46 +190,70 @@ bool stateIsWarmBoot(void) { // ============================================================================= void stateIncrementBootCount(void) { + taskENTER_CRITICAL(); s_state.bootCount++; + taskEXIT_CRITICAL(); } void stateUpdateSyncTime(void) { + taskENTER_CRITICAL(); s_state.lastSyncTime = millis(); + taskEXIT_CRITICAL(); } void stateUpdateGpsFixTime(void) { + taskENTER_CRITICAL(); s_state.lastGpsFixTime = millis(); + taskEXIT_CRITICAL(); } void stateUpdateLastPressure(float pressure) { + taskENTER_CRITICAL(); s_state.lastPressure = pressure; + taskEXIT_CRITICAL(); } void stateSetMode(OperatingMode mode) { + taskENTER_CRITICAL(); s_state.currentMode = mode; + taskEXIT_CRITICAL(); } void stateSetAlert(uint8_t alertFlag) { + taskENTER_CRITICAL(); s_state.alertsSent |= alertFlag; + taskEXIT_CRITICAL(); } void stateClearAlert(uint8_t alertFlag) { + taskENTER_CRITICAL(); s_state.alertsSent &= ~alertFlag; + taskEXIT_CRITICAL(); } uint8_t stateGetAlerts(void) { - return s_state.alertsSent; + taskENTER_CRITICAL(); + uint8_t alerts = s_state.alertsSent; + taskEXIT_CRITICAL(); + return alerts; } void stateSetMotion(bool motion) { + taskENTER_CRITICAL(); if (motion) { + // Sticky flag — only cleared by stateGetAndClearMotion() s_state.motionSinceLastReport = true; } + // Intentional: passing false is a no-op to prevent race conditions + // between motion detection and reporting. + taskEXIT_CRITICAL(); } bool stateGetAndClearMotion(void) { + taskENTER_CRITICAL(); bool motion = s_state.motionSinceLastReport; s_state.motionSinceLastReport = false; + taskEXIT_CRITICAL(); return motion; } @@ -246,45 +271,69 @@ uint32_t stateGetTotalUptimeSec(void) { } uint32_t stateGetBootCount(void) { - return s_state.bootCount; + taskENTER_CRITICAL(); + uint32_t count = s_state.bootCount; + taskEXIT_CRITICAL(); + return count; } float stateGetLastPressure(void) { - return s_state.lastPressure; + taskENTER_CRITICAL(); + float pressure = s_state.lastPressure; + taskEXIT_CRITICAL(); + return pressure; } void stateSetTransitLock(bool locked, OperatingMode previousMode) { + taskENTER_CRITICAL(); s_state.transitLocked = locked; if (locked) { s_state.preTransitMode = previousMode; } + taskEXIT_CRITICAL(); } bool stateIsTransitLocked(void) { - return s_state.transitLocked; + taskENTER_CRITICAL(); + bool locked = s_state.transitLocked; + taskEXIT_CRITICAL(); + return locked; } OperatingMode stateGetPreTransitMode(void) { - return s_state.preTransitMode; + taskENTER_CRITICAL(); + OperatingMode mode = s_state.preTransitMode; + taskEXIT_CRITICAL(); + return mode; } void stateSetDemoLock(bool locked, OperatingMode previousMode) { + taskENTER_CRITICAL(); s_state.demoLocked = locked; if (locked) { s_state.preDemoMode = previousMode; } + taskEXIT_CRITICAL(); } bool stateIsDemoLocked(void) { - return s_state.demoLocked; + taskENTER_CRITICAL(); + bool locked = s_state.demoLocked; + taskEXIT_CRITICAL(); + return locked; } OperatingMode stateGetPreDemoMode(void) { - return s_state.preDemoMode; + taskENTER_CRITICAL(); + OperatingMode mode = s_state.preDemoMode; + taskEXIT_CRITICAL(); + return mode; } void stateUpdateLockLED(void) { + taskENTER_CRITICAL(); bool lockActive = s_state.transitLocked || s_state.demoLocked; + taskEXIT_CRITICAL(); // Note: LED is active-high (GPIO HIGH = LED on) digitalWrite(LOCK_LED_PIN, lockActive ? HIGH : LOW); @@ -299,35 +348,55 @@ void stateUpdateLockLED(void) { // ============================================================================= void stateSetGpsPowerSaving(bool enabled) { + taskENTER_CRITICAL(); s_state.gpsPowerSaving = enabled; + taskEXIT_CRITICAL(); } bool stateIsGpsPowerSaving(void) { - return s_state.gpsPowerSaving; + taskENTER_CRITICAL(); + bool saving = s_state.gpsPowerSaving; + taskEXIT_CRITICAL(); + return saving; } void stateSetLastGpsRetryTime(uint32_t time) { + taskENTER_CRITICAL(); s_state.lastGpsRetryTime = time; + taskEXIT_CRITICAL(); } uint32_t stateGetLastGpsRetryTime(void) { - return s_state.lastGpsRetryTime; + taskENTER_CRITICAL(); + uint32_t t = s_state.lastGpsRetryTime; + taskEXIT_CRITICAL(); + return t; } void stateSetGpsWasActive(bool active) { + taskENTER_CRITICAL(); s_state.gpsWasActive = active; + taskEXIT_CRITICAL(); } bool stateGetGpsWasActive(void) { - return s_state.gpsWasActive; + taskENTER_CRITICAL(); + bool active = s_state.gpsWasActive; + taskEXIT_CRITICAL(); + return active; } void stateSetGpsActiveStartTime(uint32_t time) { + taskENTER_CRITICAL(); s_state.gpsActiveStartTime = time; + taskEXIT_CRITICAL(); } uint32_t stateGetGpsActiveStartTime(void) { - return s_state.gpsActiveStartTime; + taskENTER_CRITICAL(); + uint32_t t = s_state.gpsActiveStartTime; + taskEXIT_CRITICAL(); + return t; } // ============================================================================= @@ -335,13 +404,20 @@ uint32_t stateGetGpsActiveStartTime(void) { // ============================================================================= uint32_t stateCalculateChecksum(const SongbirdState* state) { - if (state == NULL) { - return 0; - } - - // Calculate CRC over all fields except the checksum itself + if (state == NULL) { return 0; } + // Copy into a zeroed buffer to normalize padding bytes between struct fields. + // Without this, two logically identical states can produce different CRC values + // because the compiler inserts padding bytes (e.g., between bool and uint32_t + // fields) whose values are undefined and can vary across memset vs memcpy paths. + // NOTE: This changes the CRC algorithm behaviour relative to any checksum + // computed before this fix. Devices upgrading from older firmware will fail + // the checksum check on the first boot and perform a clean state reset — + // this is expected and correct (STATE_VERSION was also bumped for the same reason). + SongbirdState normalized; + memset(&normalized, 0, sizeof(normalized)); + memcpy(&normalized, state, sizeof(normalized)); size_t checksumOffset = offsetof(SongbirdState, checksum); - return crc32((const uint8_t*)state, checksumOffset); + return crc32((const uint8_t*)&normalized, checksumOffset); } bool stateValidateChecksum(const SongbirdState* state) { diff --git a/songbird-firmware/src/rtos/SongbirdSync.cpp b/songbird-firmware/src/rtos/SongbirdSync.cpp index 86be144..bed8fa9 100644 --- a/songbird-firmware/src/rtos/SongbirdSync.cpp +++ b/songbird-firmware/src/rtos/SongbirdSync.cpp @@ -14,6 +14,7 @@ SemaphoreHandle_t g_i2cMutex = NULL; SemaphoreHandle_t g_configMutex = NULL; +SemaphoreHandle_t g_stateMutex = NULL; QueueHandle_t g_audioQueue = NULL; QueueHandle_t g_noteQueue = NULL; QueueHandle_t g_configQueue = NULL; @@ -43,6 +44,11 @@ bool syncInit(void) { return false; } + g_stateMutex = xSemaphoreCreateMutex(); + if (g_stateMutex == NULL) { + return false; + } + // Create queues g_audioQueue = xQueueCreate(AUDIO_QUEUE_SIZE, sizeof(AudioQueueItem)); if (g_audioQueue == NULL) { @@ -169,8 +175,10 @@ bool syncQueueConfig(const SongbirdConfig* config) { if (g_configQueue == NULL || config == NULL) { return false; } - // Blocking send - config updates are important - return xQueueSend(g_configQueue, config, portMAX_DELAY) == pdTRUE; + // Bounded timeout - config is polled every ~30s so a dropped update will + // be retried automatically. portMAX_DELAY would block EnvTask indefinitely + // if MainTask falls behind, causing a priority-inversion stall. + return xQueueSend(g_configQueue, config, pdMS_TO_TICKS(500)) == pdTRUE; } bool syncReceiveConfig(SongbirdConfig* config) { diff --git a/songbird-firmware/src/rtos/SongbirdSync.h b/songbird-firmware/src/rtos/SongbirdSync.h index 0f86e0b..6fc68e9 100644 --- a/songbird-firmware/src/rtos/SongbirdSync.h +++ b/songbird-firmware/src/rtos/SongbirdSync.h @@ -80,6 +80,7 @@ typedef struct { // Mutexes extern SemaphoreHandle_t g_i2cMutex; // Protects I2C bus (Notecard + BME280) extern SemaphoreHandle_t g_configMutex; // Protects shared configuration +extern SemaphoreHandle_t g_stateMutex; // Protects SongbirdState s_state // Queues extern QueueHandle_t g_audioQueue; // Audio events -> AudioTask @@ -118,7 +119,7 @@ extern volatile bool g_systemReady; // Set when all tasks initialized * @brief Initialize all synchronization primitives * * Must be called before creating any tasks. Creates: - * - i2cMutex and configMutex + * - i2cMutex, configMutex, and stateMutex * - audioQueue, noteQueue, and configQueue * - syncSemaphore * - sleepEvent group diff --git a/songbird-firmware/src/rtos/SongbirdTasks.cpp b/songbird-firmware/src/rtos/SongbirdTasks.cpp index ab589e1..b76d7d0 100644 --- a/songbird-firmware/src/rtos/SongbirdTasks.cpp +++ b/songbird-firmware/src/rtos/SongbirdTasks.cpp @@ -199,6 +199,11 @@ bool tasksSleepRequested(void) { void tasksGetConfig(SongbirdConfig* config) { if (config == NULL) return; + // Pre-populate with safe defaults in case mutex times out. + // Without this, a timeout would leave the caller with uninitialized + // stack data used as active configuration. + envInitDefaults(config); + if (syncAcquireConfig(100)) { memcpy(config, &s_currentConfig, sizeof(SongbirdConfig)); syncReleaseConfig(); @@ -401,8 +406,8 @@ void MainTask(void* pvParameters) { audioToggleMute(); s_clickCount = 0; } - // Check for double-click (demo lock) - after triple-click window - else if (s_clickCount == 2 && elapsed >= MULTI_CLICK_WINDOW_MS && elapsed < TRIPLE_CLICK_TIMEOUT_MS) { + // Check for double-click (demo lock) - triple-click already caught above + else if (s_clickCount == 2 && elapsed >= MULTI_CLICK_WINDOW_MS) { // Double-click detected - toggle demo lock #ifdef DEBUG_MODE DEBUG_SERIAL.println("[MainTask] Double-click - toggling demo lock"); @@ -542,7 +547,7 @@ void MainTask(void* pvParameters) { // Periodic health check static uint32_t lastHealthCheck = 0; - if (millis() - lastHealthCheck > 60000) { // Every minute + if (millis() - lastHealthCheck > HEALTH_CHECK_INTERVAL_MS) { lastHealthCheck = millis(); #ifdef DEBUG_MODE @@ -627,7 +632,7 @@ void SensorTask(void* pvParameters) { uint32_t interval = envGetSensorIntervalMs(&config); if (interval == 0) { // Sleep mode - sensors disabled, just wait - vTaskDelay(pdMS_TO_TICKS(60000)); // 1 minute wait in sleep mode + vTaskDelay(pdMS_TO_TICKS(SLEEP_MODE_SENSOR_WAIT_MS)); continue; } diff --git a/songbird-infrastructure/lambda/analytics/chat-query.ts b/songbird-infrastructure/lambda/analytics/chat-query.ts index 2f5552b..6644884 100644 --- a/songbird-infrastructure/lambda/analytics/chat-query.ts +++ b/songbird-infrastructure/lambda/analytics/chat-query.ts @@ -464,35 +464,21 @@ export const handler = async (event: APIGatewayProxyEvent): Promise record[0]?.stringValue) - .filter((sn): sn is string => !!sn); - - // Fallback: also check telemetry table if devices table is empty - if (deviceSerialNumbers.length === 0) { - const telemetryResult = await rds.send(new ExecuteStatementCommand({ - resourceArn: CLUSTER_ARN, - secretArn: SECRET_ARN, - database: DATABASE_NAME, - sql: 'SELECT DISTINCT serial_number FROM analytics.telemetry LIMIT 100', - })); - - deviceSerialNumbers = (telemetryResult.records || []) - .map(record => record[0]?.stringValue) - .filter((sn): sn is string => !!sn); - } + if (deviceSerialNumbers.length === 0) { + return { + statusCode: 403, + headers: { + 'Content-Type': 'application/json', + 'Access-Control-Allow-Origin': '*', + }, + body: JSON.stringify({ error: 'deviceSerialNumbers is required' }), + }; } // Look up the user's assigned device from DynamoDB devices table diff --git a/songbird-infrastructure/lambda/analytics/feedback.ts b/songbird-infrastructure/lambda/analytics/feedback.ts index 903dc71..5d641fe 100644 --- a/songbird-infrastructure/lambda/analytics/feedback.ts +++ b/songbird-infrastructure/lambda/analytics/feedback.ts @@ -118,33 +118,38 @@ async function indexPositiveFeedback(req: FeedbackRequest): Promise { const embedding = await embedText(content); const embeddingStr = `[${embedding.join(',')}]`; - const titleEscaped = title.replace(/'/g, "''"); - const contentEscaped = content.replace(/'/g, "''"); - const metadataEscaped = metadata.replace(/'/g, "''"); - - // Delete existing by title then insert (upsert pattern) - await rds.send(new ExecuteStatementCommand({ + const connectionParams = { resourceArn: CLUSTER_ARN, secretArn: SECRET_ARN, database: DATABASE_NAME, - sql: `DELETE FROM analytics.rag_documents WHERE title = '${titleEscaped}'`, + }; + + // Delete existing by title then insert (upsert pattern) + await rds.send(new ExecuteStatementCommand({ + ...connectionParams, + sql: `DELETE FROM analytics.rag_documents WHERE title = :title`, + parameters: [{ name: 'title', value: { stringValue: title } }], })); await rds.send(new ExecuteStatementCommand({ - resourceArn: CLUSTER_ARN, - secretArn: SECRET_ARN, - database: DATABASE_NAME, + ...connectionParams, sql: ` INSERT INTO analytics.rag_documents (doc_type, title, content, embedding, metadata, pinned) VALUES ( 'example', - '${titleEscaped}', - '${contentEscaped}', - '${embeddingStr}'::vector, - '${metadataEscaped}'::jsonb, + :title, + :content, + :embedding::vector, + :metadata::jsonb, FALSE ) `, + parameters: [ + { name: 'title', value: { stringValue: title } }, + { name: 'content', value: { stringValue: content } }, + { name: 'embedding', value: { stringValue: embeddingStr } }, + { name: 'metadata', value: { stringValue: metadata } }, + ], })); console.log(`Indexed positive feedback example: "${title}"`); @@ -186,6 +191,15 @@ export const handler = async (event: APIGatewayProxyEvent): Promise `'${d.title.replace(/'/g, "''")}'`).join(','); - const excludePinned = pinnedTitles.length > 0 - ? `AND title NOT IN (${pinnedTitles})` + const titleParams = pinnedDocs.map((d, i) => ({ + name: `p${i}`, + value: { stringValue: d.title }, + })); + const excludePinned = titleParams.length > 0 + ? `AND title NOT IN (${titleParams.map(p => `:${p.name}`).join(',')})` : ''; const sql = ` SELECT title, content, doc_type, - 1 - (embedding <=> '${embeddingStr}'::vector) AS similarity + 1 - (embedding <=> :embedding::vector) AS similarity FROM analytics.rag_documents WHERE embedding IS NOT NULL AND pinned = FALSE ${excludePinned} - ORDER BY embedding <=> '${embeddingStr}'::vector - LIMIT ${topK} + ORDER BY embedding <=> :embedding::vector + LIMIT :limit `; const result = await rds.send(new ExecuteStatementCommand({ @@ -102,6 +105,11 @@ export async function retrieveRelevantContext( secretArn, database: databaseName, sql, + parameters: [ + { name: 'embedding', value: { stringValue: embeddingStr } }, + { name: 'limit', value: { longValue: topK } }, + ...titleParams, + ], })); const similarDocs: RetrievedDocument[] = (result.records || []).map(record => ({ From 898e1f14e8922a2df62d156c7a5948d16f4ef617 Mon Sep 17 00:00:00 2001 From: Brandon Satrom Date: Thu, 16 Apr 2026 18:40:13 -0700 Subject: [PATCH 2/2] chore: merge main and update feedback.test.ts for admin-group auth Merge origin/main to pick up the test suite from #18, then update the 3 GET /analytics/feedback tests to include cognito:groups=Admin in the authorizer JWT claims, matching the admin-group check this PR adds to the handler. Also adds a new test covering the 403 denial path. Co-Authored-By: Claude Opus 4.7 --- .../lambda/analytics/feedback.test.ts | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/songbird-infrastructure/lambda/analytics/feedback.test.ts b/songbird-infrastructure/lambda/analytics/feedback.test.ts index d7109c5..b83f211 100644 --- a/songbird-infrastructure/lambda/analytics/feedback.test.ts +++ b/songbird-infrastructure/lambda/analytics/feedback.test.ts @@ -69,7 +69,10 @@ describe('analytics/feedback handler', () => { const result = await handler(makeEvent({ httpMethod: 'GET', - requestContext: { http: { method: 'GET', path: '/analytics/feedback' } }, + requestContext: { + http: { method: 'GET', path: '/analytics/feedback' }, + authorizer: { jwt: { claims: { 'cognito:groups': 'Admin' } } }, + }, })); expect(result.statusCode).toBe(200); @@ -86,7 +89,10 @@ describe('analytics/feedback handler', () => { const result = await handler(makeEvent({ httpMethod: 'GET', - requestContext: { http: { method: 'GET', path: '/analytics/feedback' } }, + requestContext: { + http: { method: 'GET', path: '/analytics/feedback' }, + authorizer: { jwt: { claims: { 'cognito:groups': 'Admin' } } }, + }, })); expect(result.statusCode).toBe(200); @@ -107,7 +113,10 @@ describe('analytics/feedback handler', () => { const result = await handler(makeEvent({ httpMethod: 'GET', - requestContext: { http: { method: 'GET', path: '/analytics/feedback' } }, + requestContext: { + http: { method: 'GET', path: '/analytics/feedback' }, + authorizer: { jwt: { claims: { 'cognito:groups': 'Admin' } } }, + }, queryStringParameters: { limit: '2' }, })); @@ -116,6 +125,18 @@ describe('analytics/feedback handler', () => { expect(body.items.length).toBe(2); }); + it('GET returns 403 when caller is not in the Admin group', async () => { + const result = await handler(makeEvent({ + httpMethod: 'GET', + requestContext: { + http: { method: 'GET', path: '/analytics/feedback' }, + authorizer: { jwt: { claims: { 'cognito:groups': 'Viewer' } } }, + }, + })); + + expect(result.statusCode).toBe(403); + }); + // ─── DELETE ────────────────────────────────────────────────────────── it('DELETE removes feedback record by userEmail and ratedAt', async () => {