fix: return errors for malformed rows and partial credentials in DynamoDB source

Author: beekld

libs/server-sdk-dynamodb-source/src/client_factory.cpp

@@ -8,10 +8,32 @@ namespace launchdarkly::server_side::integrations::detail {
 
 namespace {
 
-bool HasExplicitCredentials(DynamoDBClientOptions const& options) {
-    return options.aws_access_key_id.has_value() ||
-           options.aws_secret_access_key.has_value() ||
-           options.aws_session_token.has_value();
+// Verifies that the credential fields in `options` form a valid combination.
+// Returns an empty optional on success, or an error string describing what's
+// wrong. The valid combinations are:
+//
+//   - none of the three set (fall through to AWS default credential chain)
+//   - access_key_id + secret_access_key (long-lived IAM keys)
+//   - access_key_id + secret_access_key + session_token (STS temporary creds)
+//
+// All other partial combinations would build a misconfigured AWS client that
+// fails opaquely at request time; catching them here surfaces the
+// misconfiguration up front.
+std::optional<std::string> ValidateCredentials(
+    DynamoDBClientOptions const& options) {
+    bool const has_key = options.aws_access_key_id.has_value();
+    bool const has_secret = options.aws_secret_access_key.has_value();
+    bool const has_token = options.aws_session_token.has_value();
+
+    if (has_key != has_secret) {
+        return "aws_access_key_id and aws_secret_access_key must both be set "
+               "or both unset";
+    }
+    if (has_token && !has_key) {
+        return "aws_session_token requires aws_access_key_id and "
+               "aws_secret_access_key";
+    }
+    return std::nullopt;
 }
 
 Aws::Client::ClientConfiguration BuildConfig(
@@ -39,14 +61,17 @@ Aws::Client::ClientConfiguration BuildConfig(
 
 }  // namespace
 
-std::shared_ptr<Aws::DynamoDB::DynamoDBClient> BuildDynamoDBClient(
-    DynamoDBClientOptions const& options) {
+tl::expected<std::shared_ptr<Aws::DynamoDB::DynamoDBClient>, std::string>
+BuildDynamoDBClient(DynamoDBClientOptions const& options) {
+    if (auto err = ValidateCredentials(options)) {
+        return tl::make_unexpected(std::move(*err));
+    }
+
     auto const config = BuildConfig(options);
 
-    if (HasExplicitCredentials(options)) {
+    if (options.aws_access_key_id) {
         Aws::Auth::AWSCredentials credentials{
-            options.aws_access_key_id.value_or(""),
-            options.aws_secret_access_key.value_or(""),
+            *options.aws_access_key_id, *options.aws_secret_access_key,
             options.aws_session_token.value_or("")};
         return std::make_shared<Aws::DynamoDB::DynamoDBClient>(credentials,
                                                                config);

libs/server-sdk-dynamodb-source/src/client_factory.hpp

@@ -4,18 +4,21 @@
 
 #include <aws/dynamodb/DynamoDBClient.h>
 
+#include <tl/expected.hpp>
+
 #include <memory>
+#include <string>
 
 namespace launchdarkly::server_side::integrations::detail {
 
 // Builds an Aws::DynamoDB::DynamoDBClient configured from the supplied
-// DynamoDBClientOptions. Caller is responsible for ensuring AwsSdkGuard::Ensure()
-// has been called first.
+// DynamoDBClientOptions, or returns an error string if the options are
+// internally inconsistent (e.g. only one of aws_access_key_id /
+// aws_secret_access_key is set).
 //
-// The shared_ptr return enables future sharing of a single client across
-// multiple DynamoDB-backed stores in the same process (e.g. data source +
-// big-segment store); today each consumer constructs its own.
-std::shared_ptr<Aws::DynamoDB::DynamoDBClient> BuildDynamoDBClient(
-    DynamoDBClientOptions const& options);
+// Caller is responsible for ensuring AwsSdkGuard::Ensure() has been called
+// first.
+tl::expected<std::shared_ptr<Aws::DynamoDB::DynamoDBClient>, std::string>
+BuildDynamoDBClient(DynamoDBClientOptions const& options);
 
 }  // namespace launchdarkly::server_side::integrations::detail

libs/server-sdk-dynamodb-source/src/dynamodb_source.cpp

@@ -32,9 +32,13 @@ DynamoDBDataSource::Create(std::string table_name,
                            DynamoDBClientOptions options) {
     try {
         detail::AwsSdkGuard::Ensure();
-        auto client = detail::BuildDynamoDBClient(options);
-        return std::unique_ptr<DynamoDBDataSource>(new DynamoDBDataSource(
-            std::move(client), std::move(table_name), std::move(prefix)));
+        auto maybe_client = detail::BuildDynamoDBClient(options);
+        if (!maybe_client) {
+            return tl::make_unexpected(std::move(maybe_client.error()));
+        }
+        return std::unique_ptr<DynamoDBDataSource>(
+            new DynamoDBDataSource(std::move(*maybe_client),
+                                   std::move(table_name), std::move(prefix)));
     } catch (std::exception const& e) {
         return tl::make_unexpected(e.what());
     }
@@ -74,7 +78,8 @@ ISerializedDataReader::GetResult DynamoDBDataSource::Get(
 
     auto const it = item.find(kItemAttribute);
     if (it == item.end()) {
-        return std::nullopt;
+        return tl::make_unexpected(
+            Error{"DynamoDB row missing expected 'item' attribute"});
     }
 
     return SerializedItemDescriptor::Present(0, it->second.GetS());
@@ -102,10 +107,14 @@ ISerializedDataReader::AllResult DynamoDBDataSource::All(
         auto const& result = outcome.GetResult();
         for (auto const& row : result.GetItems()) {
             auto const key_it = row.find(kSortKey);
-            auto const item_it = row.find(kItemAttribute);
-            if (key_it == row.end() || item_it == row.end()) {
+            if (key_it == row.end()) {
                 continue;
             }
+            auto const item_it = row.find(kItemAttribute);
+            if (item_it == row.end()) {
+                return tl::make_unexpected(
+                    Error{"DynamoDB row missing expected 'item' attribute"});
+            }
             items.emplace(
                 key_it->second.GetS(),
                 SerializedItemDescriptor::Present(0, item_it->second.GetS()));

libs/server-sdk-dynamodb-source/tests/dynamodb_source_test.cpp

@@ -420,6 +420,25 @@ TEST_F(DynamoDBTests, AllPaginatesAcrossMultiplePages) {
     ASSERT_EQ(result->size(), kFlagCount);
 }
 
+TEST_F(DynamoDBTests, GetReturnsErrorWhenRowIsMissingItemAttribute) {
+    WithPrefixedClient(prefix_, [&](auto const& client) {
+        client.PutRowWithoutItem("features", "foo");
+    });
+
+    auto const result = source->Get(FlagKind{}, "foo");
+    ASSERT_FALSE(result);
+}
+
+TEST_F(DynamoDBTests, AllReturnsErrorWhenRowIsMissingItemAttribute) {
+    WithPrefixedClient(prefix_, [&](auto const& client) {
+        client.PutFlag(Flag{"foo", 1, true});
+        client.PutRowWithoutItem("features", "bar");
+    });
+
+    auto const result = source->All(FlagKind{});
+    ASSERT_FALSE(result);
+}
+
 TEST_F(DynamoDBTests, IdentityReturnsDynamodb) {
     ASSERT_EQ(source->Identity(), "dynamodb");
 }

libs/server-sdk-dynamodb-source/tests/prefixed_dynamodb_client.hpp

@@ -5,6 +5,7 @@
 
 #include <aws/core/utils/Outcome.h>
 #include <aws/dynamodb/DynamoDBClient.h>
+#include <aws/dynamodb/DynamoDBErrors.h>
 #include <aws/dynamodb/model/AttributeDefinition.h>
 #include <aws/dynamodb/model/AttributeValue.h>
 #include <aws/dynamodb/model/CreateTableRequest.h>
@@ -79,8 +80,13 @@ class PrefixedDynamoDBClient {
         request.SetTableName(table_name);
         auto outcome = client.DeleteTable(request);
         if (!outcome.IsSuccess()) {
-            // Ignore the not-found case (test may not have created the table).
-            return;
+            // Tolerate not-found so setup can call this unconditionally.
+            if (outcome.GetError().GetErrorType() ==
+                Aws::DynamoDB::DynamoDBErrors::RESOURCE_NOT_FOUND) {
+                return;
+            }
+            FAIL() << "couldn't delete DynamoDB table " << table_name << ": "
+                   << outcome.GetError().GetMessage();
         }
     }
 
@@ -115,6 +121,13 @@ class PrefixedDynamoDBClient {
         PutRaw(Prefixed("segments"), key, ts);
     }
 
+    // Writes a row with only namespace + key, no `item` attribute. Used to
+    // simulate corrupted/malformed rows in the table.
+    void PutRowWithoutItem(std::string const& ns_suffix,
+                           std::string const& key) const {
+        PutRaw(Prefixed(ns_suffix), key, std::nullopt);
+    }
+
    private:
     std::string Prefixed(std::string const& base) const {
         if (prefix_.empty()) {