From 245aeb5ac034a83a292533ad515c03193ae35b19 Mon Sep 17 00:00:00 2001 From: Brandon Satrom Date: Tue, 14 Apr 2026 17:15:56 -0700 Subject: [PATCH 1/3] 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 a73d5279d94dc7f12c43c2c9b175c2d17bb49d96 Mon Sep 17 00:00:00 2001 From: Brandon Satrom Date: Tue, 14 Apr 2026 17:16:59 -0700 Subject: [PATCH 2/3] fix(infra): remove marketplace IAM permission, use crypto.randomUUID for alert IDs, add ACKNOWLEDGED constant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove aws-marketplace:Subscribe and aws-marketplace:ViewSubscriptions from chat query Lambda IAM role; a Lambda should never be able to subscribe to paid Marketplace products - Scope bedrock:InvokeModel to specific Claude 3.5 Sonnet ARN prefixes instead of wildcard * - Replace Math.random() alert ID generation with crypto.randomUUID() in api-ingest — eliminates 5-6 char collision window at high throughput, uses built-in Node 20 crypto module - Add ACKNOWLEDGED constant (TRUE/FALSE string literals) to shared/constants.ts — prevents accidental boolean writes that break the DynamoDB GSI partition key - Import and use ACKNOWLEDGED constant in api-alerts and api-ingest at all write and query sites - Add marshallOptions: { removeUndefinedValues: true } to DynamoDB document clients in api-alerts and api-devices — matches the pattern already used in api-ingest, prevents latent runtime errors on optional fields Co-Authored-By: Claude Sonnet 4.6 --- lat.md/security.md | 15 +++++++ .../lambda/api-activity/index.ts | 5 ++- .../lambda/api-alerts/index.ts | 41 ++++++------------- .../lambda/api-commands/index.ts | 11 ++++- .../lambda/api-devices/index.ts | 30 ++++++++++++-- .../lambda/api-ingest/index.ts | 20 +++++---- .../lambda/api-journeys/index.ts | 7 ++-- .../lambda/api-telemetry/index.ts | 5 ++- .../lambda/shared/constants.ts | 9 ++++ .../lambda/shared/tracing.ts | 13 +++--- .../lambda/shared/utils.ts | 20 +++++++++ .../lib/analytics-construct.ts | 12 +++--- 12 files changed, 125 insertions(+), 63 deletions(-) create mode 100644 songbird-infrastructure/lambda/shared/constants.ts create mode 100644 songbird-infrastructure/lambda/shared/utils.ts diff --git a/lat.md/security.md b/lat.md/security.md index 3705fe7..c91752e 100644 --- a/lat.md/security.md +++ b/lat.md/security.md @@ -28,3 +28,18 @@ Chat query requests must supply an explicit `deviceSerialNumbers` array. If the 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]]. + +## Integer Query Parameter Safety + +Query-string parameters that feed into DynamoDB `Limit` values must be parsed with [[songbird-infrastructure/lambda/shared/utils.ts#parseIntParam]], not bare `parseInt()`. Bare `parseInt()` returns `NaN` for non-numeric input, which propagates into DynamoDB and causes 500 errors with no useful message. + +`parseIntParam` returns the default when the value is missing, non-numeric, or less than 1, and optionally clamps the result to a maximum. All five GET-list handlers (`api-devices`, `api-alerts`, `api-telemetry`, `api-activity`, `api-journeys`) use this helper. + +## JSON Body Error Handling in Mutation Paths + +POST/PATCH/DELETE handlers that call `JSON.parse(event.body)` must wrap the call in a try/catch that returns a 400 with a descriptive error. Uncaught `SyntaxError` from malformed JSON propagates to the outer catch and becomes an opaque 500. + +Handlers that apply this guard: +- [[songbird-infrastructure/lambda/api-devices/index.ts#mergeDevices]] — `POST /devices/merge` +- [[songbird-infrastructure/lambda/api-devices/index.ts#updateDeviceBySerial]] — `PATCH /devices/{serial_number}` +- [[songbird-infrastructure/lambda/api-commands/index.ts#sendCommand]] — `POST /devices/{serial_number}/commands` diff --git a/songbird-infrastructure/lambda/api-activity/index.ts b/songbird-infrastructure/lambda/api-activity/index.ts index e1cb989..7aedcaf 100644 --- a/songbird-infrastructure/lambda/api-activity/index.ts +++ b/songbird-infrastructure/lambda/api-activity/index.ts @@ -11,6 +11,7 @@ import { DynamoDBClient } from '@aws-sdk/client-dynamodb'; import { DynamoDBDocumentClient, ScanCommand } from '@aws-sdk/lib-dynamodb'; import { APIGatewayProxyEvent, APIGatewayProxyResult } from 'aws-lambda'; +import { parseIntParam } from '../shared/utils'; const ddbClient = new DynamoDBClient({}); const docClient = DynamoDBDocumentClient.from(ddbClient); @@ -48,8 +49,8 @@ export const handler = async (event: APIGatewayProxyEvent): Promise { - const result = await docClient.send(new GetCommand({ - TableName: DEVICE_ALIASES_TABLE, - Key: { serial_number: serialNumber }, - })); - - if (!result.Item) { - return []; - } - - // Return current device_uid plus any historical ones - const deviceUids = [result.Item.device_uid]; - if (result.Item.previous_device_uids && Array.isArray(result.Item.previous_device_uids)) { - deviceUids.push(...result.Item.previous_device_uids); - } - return deviceUids; -} export const handler = async (event: APIGatewayProxyEvent): Promise => { const method = (event.requestContext as any)?.http?.method || event.httpMethod; @@ -107,7 +90,7 @@ async function listAlerts( const queryParams = event.queryStringParameters || {}; const serialNumber = queryParams.serial_number || queryParams.device_uid; // Support both for backwards compat const acknowledged = queryParams.acknowledged; - const limit = parseInt(queryParams.limit || '100'); + const limit = parseIntParam(queryParams.limit, 100, 500); let items: any[] = []; @@ -153,7 +136,7 @@ async function listAlerts( TableName: ALERTS_TABLE, IndexName: 'status-index', KeyConditionExpression: 'acknowledged = :ack', - ExpressionAttributeValues: { ':ack': 'false' }, + ExpressionAttributeValues: { ':ack': ACKNOWLEDGED.FALSE }, ScanIndexForward: false, Limit: limit, }); @@ -175,7 +158,7 @@ async function listAlerts( } // Calculate stats - const activeCount = items.filter(a => a.acknowledged === 'false').length; + const activeCount = items.filter(a => a.acknowledged === ACKNOWLEDGED.FALSE).length; return { statusCode: 200, @@ -237,7 +220,7 @@ async function acknowledgeAlert( Key: { alert_id: alertId }, UpdateExpression: 'SET acknowledged = :ack, acknowledged_at = :ack_at, acknowledged_by = :ack_by', ExpressionAttributeValues: { - ':ack': 'true', + ':ack': ACKNOWLEDGED.TRUE, ':ack_at': now, ':ack_by': acknowledgedBy, }, @@ -294,10 +277,10 @@ async function bulkAcknowledgeAlerts( UpdateExpression: 'SET acknowledged = :ack, acknowledged_at = :ack_at, acknowledged_by = :ack_by', ConditionExpression: 'acknowledged = :not_ack', ExpressionAttributeValues: { - ':ack': 'true', + ':ack': ACKNOWLEDGED.TRUE, ':ack_at': now, ':ack_by': acknowledgedBy, - ':not_ack': 'false', + ':not_ack': ACKNOWLEDGED.FALSE, }, ReturnValues: 'ALL_NEW', }); diff --git a/songbird-infrastructure/lambda/api-commands/index.ts b/songbird-infrastructure/lambda/api-commands/index.ts index 08eca75..33df71c 100644 --- a/songbird-infrastructure/lambda/api-commands/index.ts +++ b/songbird-infrastructure/lambda/api-commands/index.ts @@ -206,7 +206,16 @@ async function sendCommand( }; } - const request = JSON.parse(event.body); + let request: { cmd?: string; params?: unknown }; + try { + request = JSON.parse(event.body); + } catch { + return { + statusCode: 400, + headers, + body: JSON.stringify({ error: 'Invalid JSON in request body' }), + }; + } const { cmd, params } = request; // Validate command diff --git a/songbird-infrastructure/lambda/api-devices/index.ts b/songbird-infrastructure/lambda/api-devices/index.ts index 24a362c..484e5c7 100644 --- a/songbird-infrastructure/lambda/api-devices/index.ts +++ b/songbird-infrastructure/lambda/api-devices/index.ts @@ -19,9 +19,12 @@ import { } from '@aws-sdk/lib-dynamodb'; import { APIGatewayProxyEvent, APIGatewayProxyResult } from 'aws-lambda'; import { resolveDevice, getAliasBySerial } from '../shared/device-lookup'; +import { parseIntParam } from '../shared/utils'; const ddbClient = new DynamoDBClient({}); -const docClient = DynamoDBDocumentClient.from(ddbClient); +const docClient = DynamoDBDocumentClient.from(ddbClient, { + marshallOptions: { removeUndefinedValues: true }, +}); const DEVICES_TABLE = process.env.DEVICES_TABLE!; const DEVICE_ALIASES_TABLE = process.env.DEVICE_ALIASES_TABLE || 'songbird-device-aliases'; @@ -89,7 +92,7 @@ async function listDevices( const queryParams = event.queryStringParameters || {}; const fleet = queryParams.fleet; const status = queryParams.status; - const limit = parseInt(queryParams.limit || '100'); + const limit = parseIntParam(queryParams.limit, 100, 500); let items: any[] = []; @@ -215,7 +218,16 @@ async function updateDeviceBySerial( }; } - const updates = JSON.parse(body); + let updates: Record; + try { + updates = JSON.parse(body); + } catch { + return { + statusCode: 400, + headers, + body: JSON.stringify({ error: 'Invalid JSON in request body' }), + }; + } // Only allow certain fields to be updated (removed serial_number - it's now immutable) const allowedFields = ['name', 'assigned_to', 'assigned_to_name', 'fleet', 'notes']; @@ -421,7 +433,17 @@ async function mergeDevices( }; } - const { source_serial_number, target_serial_number } = JSON.parse(event.body); + let parsedBody: { source_serial_number?: string; target_serial_number?: string }; + try { + parsedBody = JSON.parse(event.body); + } catch { + return { + statusCode: 400, + headers, + body: JSON.stringify({ error: 'Invalid JSON in request body' }), + }; + } + const { source_serial_number, target_serial_number } = parsedBody; if (!source_serial_number || !target_serial_number) { return { diff --git a/songbird-infrastructure/lambda/api-ingest/index.ts b/songbird-infrastructure/lambda/api-ingest/index.ts index 8f53d71..b7d046f 100644 --- a/songbird-infrastructure/lambda/api-ingest/index.ts +++ b/songbird-infrastructure/lambda/api-ingest/index.ts @@ -5,11 +5,13 @@ * Processes incoming Songbird events and writes to DynamoDB. */ +import { randomUUID } from 'crypto'; import { DynamoDBClient } from '@aws-sdk/client-dynamodb'; import { DynamoDBDocumentClient, PutCommand, UpdateCommand, QueryCommand, GetCommand } from '@aws-sdk/lib-dynamodb'; import { SNSClient, PublishCommand } from '@aws-sdk/client-sns'; import { APIGatewayProxyEvent, APIGatewayProxyResult } from 'aws-lambda'; import { handleDeviceAlias } from '../shared/device-lookup'; +import { ACKNOWLEDGED } from '../shared/constants'; // Initialize clients const ddbClient = new DynamoDBClient({}); @@ -593,7 +595,7 @@ async function createLowBatteryAlert(event: SongbirdEvent): Promise { const now = Date.now(); const ttl = Math.floor(now / 1000) + TTL_SECONDS; - const alertId = `alert_${event.device_uid}_${now}_${Math.random().toString(36).substring(7)}`; + const alertId = `alert_${randomUUID()}`; const alertRecord = { alert_id: alertId, @@ -605,7 +607,7 @@ async function createLowBatteryAlert(event: SongbirdEvent): Promise { message: `Device restarted due to low battery (${event.body.voltage?.toFixed(2)}V)`, created_at: now, event_timestamp: event.timestamp * 1000, - acknowledged: 'false', + acknowledged: ACKNOWLEDGED.FALSE, ttl, location: event.location ? { lat: event.location.lat, @@ -700,7 +702,7 @@ async function createGpsPowerSaveAlert(event: SongbirdEvent): Promise { const now = Date.now(); const ttl = Math.floor(now / 1000) + TTL_SECONDS; - const alertId = `alert_${event.device_uid}_${now}_${Math.random().toString(36).substring(7)}`; + const alertId = `alert_${randomUUID()}`; const alertRecord = { alert_id: alertId, @@ -711,7 +713,7 @@ async function createGpsPowerSaveAlert(event: SongbirdEvent): Promise { message: 'GPS disabled for power saving - unable to acquire satellite signal', created_at: now, event_timestamp: event.timestamp * 1000, - acknowledged: 'false', + acknowledged: ACKNOWLEDGED.FALSE, ttl, location: event.location ? { lat: event.location.lat, @@ -803,7 +805,7 @@ async function createNoSatAlert(event: SongbirdEvent): Promise { const now = Date.now(); const ttl = Math.floor(now / 1000) + TTL_SECONDS; - const alertId = `alert_${event.device_uid}_${now}_${Math.random().toString(36).substring(7)}`; + const alertId = `alert_${randomUUID()}`; const alertRecord = { alert_id: alertId, @@ -814,7 +816,7 @@ async function createNoSatAlert(event: SongbirdEvent): Promise { message: 'Unable to obtain GPS location', created_at: now, event_timestamp: event.timestamp * 1000, - acknowledged: 'false', + acknowledged: ACKNOWLEDGED.FALSE, ttl, location: event.location ? { lat: event.location.lat, @@ -1121,7 +1123,7 @@ async function storeAlert(event: SongbirdEvent): Promise { const ttl = Math.floor(now / 1000) + TTL_SECONDS; // Generate a unique alert ID - const alertId = `alert_${event.device_uid}_${now}_${Math.random().toString(36).substring(7)}`; + const alertId = `alert_${randomUUID()}`; const alertRecord = { alert_id: alertId, @@ -1134,7 +1136,7 @@ async function storeAlert(event: SongbirdEvent): Promise { message: event.body.message || '', created_at: now, event_timestamp: event.timestamp * 1000, - acknowledged: 'false', // String for GSI partition key + acknowledged: ACKNOWLEDGED.FALSE, // String for GSI partition key ttl, location: event.location ? { lat: event.location.lat, @@ -1578,7 +1580,7 @@ async function hasUnacknowledgedAlert(deviceUid: string, alertType: string): Pro ExpressionAttributeValues: { ':device_uid': deviceUid, ':alert_type': alertType, - ':false': 'false', + ':false': ACKNOWLEDGED.FALSE, }, ScanIndexForward: false, // Most recent first }); diff --git a/songbird-infrastructure/lambda/api-journeys/index.ts b/songbird-infrastructure/lambda/api-journeys/index.ts index 52f42b6..a3fa98c 100644 --- a/songbird-infrastructure/lambda/api-journeys/index.ts +++ b/songbird-infrastructure/lambda/api-journeys/index.ts @@ -15,6 +15,7 @@ import { DynamoDBClient } from '@aws-sdk/client-dynamodb'; import { DynamoDBDocumentClient, QueryCommand, UpdateCommand, DeleteCommand, GetCommand, BatchWriteCommand } from '@aws-sdk/lib-dynamodb'; import { APIGatewayProxyEvent, APIGatewayProxyEventV2, APIGatewayProxyResult } from 'aws-lambda'; import { resolveDevice } from '../shared/device-lookup'; +import { parseIntParam } from '../shared/utils'; // Type for location point items from DynamoDB interface LocationPoint { @@ -146,7 +147,7 @@ async function listJourneys( headers: Record ): Promise { const status = queryParams.status; // 'active' | 'completed' | undefined (all) - const limit = parseInt(queryParams.limit || '50'); + const limit = parseIntParam(queryParams.limit, 50, 500); // Query all device_uids in parallel const queryPromises = deviceUids.map(async (deviceUid) => { @@ -473,9 +474,9 @@ async function getLocationHistory( queryParams: Record, headers: Record ): Promise { - const hours = parseInt(queryParams.hours || '24'); + const hours = parseIntParam(queryParams.hours, 24, 168); // max 168h = 1 week const source = queryParams.source; // 'gps' | 'cell' | 'triangulation' | undefined (all) - const limit = parseInt(queryParams.limit || '1000'); + const limit = parseIntParam(queryParams.limit, 1000, 5000); const cutoffTime = Date.now() - hours * 60 * 60 * 1000; diff --git a/songbird-infrastructure/lambda/api-telemetry/index.ts b/songbird-infrastructure/lambda/api-telemetry/index.ts index a13d4ed..82de2fd 100644 --- a/songbird-infrastructure/lambda/api-telemetry/index.ts +++ b/songbird-infrastructure/lambda/api-telemetry/index.ts @@ -15,6 +15,7 @@ import { DynamoDBClient } from '@aws-sdk/client-dynamodb'; import { DynamoDBDocumentClient, QueryCommand } from '@aws-sdk/lib-dynamodb'; import { APIGatewayProxyEvent, APIGatewayProxyResult } from 'aws-lambda'; import { resolveDevice } from '../shared/device-lookup'; +import { parseIntParam } from '../shared/utils'; const ddbClient = new DynamoDBClient({}); const docClient = DynamoDBDocumentClient.from(ddbClient); @@ -61,8 +62,8 @@ export const handler = async (event: APIGatewayProxyEvent): Promise { if (!spanProcessor) { - console.log('No span processor to flush'); + if (DEBUG_TRACING) console.log('No span processor to flush'); return; } try { - console.log('Flushing spans to Phoenix...'); + if (DEBUG_TRACING) console.log('Flushing spans to Phoenix...'); await spanProcessor.forceFlush(); - console.log('Spans flushed successfully'); + if (DEBUG_TRACING) console.log('Spans flushed successfully'); } catch (error) { console.error('Error flushing spans:', error); } @@ -74,7 +75,7 @@ export async function traceAsyncFn( ): Promise { const tracer = trace.getTracer('songbird'); const span = tracer.startSpan(name, { attributes, kind: spanKind }); - console.log(`Creating span: ${name}`); + if (DEBUG_TRACING) console.log(`Creating span: ${name}`); // Set this span as the active context so child spans nest under it const ctx = trace.setSpan(context.active(), span); @@ -82,7 +83,7 @@ export async function traceAsyncFn( try { const result = await context.with(ctx, () => fn(span)); span.setStatus({ code: SpanStatusCode.OK }); - console.log(`Span completed successfully: ${name}`); + if (DEBUG_TRACING) console.log(`Span completed successfully: ${name}`); return result; } catch (error) { span.setStatus({ @@ -94,6 +95,6 @@ export async function traceAsyncFn( throw error; } finally { span.end(); - console.log(`Span ended: ${name}`); + if (DEBUG_TRACING) console.log(`Span ended: ${name}`); } } diff --git a/songbird-infrastructure/lambda/shared/utils.ts b/songbird-infrastructure/lambda/shared/utils.ts new file mode 100644 index 0000000..014154b --- /dev/null +++ b/songbird-infrastructure/lambda/shared/utils.ts @@ -0,0 +1,20 @@ +/** + * Shared utility helpers for Lambda handlers + */ + +/** + * Safely parse an integer query-string parameter. + * + * Returns `defaultVal` when the value is missing, non-numeric, less than 1, + * or not finite — preventing NaN from propagating into DynamoDB Limit params. + * Optionally clamps the result to `max`. + */ +export function parseIntParam( + value: string | undefined, + defaultVal: number, + max?: number +): number { + const parsed = parseInt(value || String(defaultVal), 10); + if (!Number.isFinite(parsed) || parsed < 1) return defaultVal; + return max !== undefined ? Math.min(parsed, max) : parsed; +} diff --git a/songbird-infrastructure/lib/analytics-construct.ts b/songbird-infrastructure/lib/analytics-construct.ts index 7291030..56a2b34 100644 --- a/songbird-infrastructure/lib/analytics-construct.ts +++ b/songbird-infrastructure/lib/analytics-construct.ts @@ -237,16 +237,14 @@ export class AnalyticsConstruct extends Construct { this.chatHistoryTable.grantReadWriteData(this.chatQueryLambda); props.devicesTable.grantReadData(this.chatQueryLambda); - // Grant Bedrock access (includes Marketplace permissions for first-time model invocation) + // Grant Bedrock access scoped to the specific model used by this Lambda this.chatQueryLambda.addToRolePolicy(new iam.PolicyStatement({ effect: iam.Effect.ALLOW, actions: ['bedrock:InvokeModel'], - resources: ['*'], - })); - this.chatQueryLambda.addToRolePolicy(new iam.PolicyStatement({ - effect: iam.Effect.ALLOW, - actions: ['aws-marketplace:ViewSubscriptions', 'aws-marketplace:Subscribe'], - resources: ['*'], + resources: [ + `arn:aws:bedrock:*::foundation-model/anthropic.claude-3-5-sonnet-*`, + `arn:aws:bedrock:*::foundation-model/us.anthropic.claude-3-5-sonnet-*`, + ], })); // ========================================================================== From 1ce2e32ce2be00d62cf78607bee500af685c1db3 Mon Sep 17 00:00:00 2001 From: Brandon Satrom Date: Thu, 16 Apr 2026 18:39:32 -0700 Subject: [PATCH 3/3] 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 () => {