Przeglądaj źródła

fix: CORS multi-origin, limit/offset 422, project-delete cleanup, env key persist, last-admin guard

Fszontagh 2 miesięcy temu
rodzic
commit
70023f26c3

+ 6 - 2
src/handlers/documents.cpp

@@ -40,8 +40,12 @@ void registerDocumentRoutes(ApiServer& s) {
         smartbotic::database::Client::QueryOptions opts;
         auto range = req.params.equal_range("filter");
         for (auto it = range.first; it != range.second; ++it) opts.filters.push_back(parseFilter(it->second));
-        if (req.has_param("limit"))  opts.limit  = (uint32_t)std::stoul(req.get_param_value("limit"));
-        if (req.has_param("offset")) opts.offset = (uint32_t)std::stoul(req.get_param_value("offset"));
+        auto parseUInt = [](const std::string& v, const char* name) -> uint32_t {
+            try { return (uint32_t)std::stoul(v); }
+            catch (...) { throw ApiError(ErrCode::Unprocessable, "validation", std::string(name) + " must be a non-negative integer"); }
+        };
+        if (req.has_param("limit"))  opts.limit  = parseUInt(req.get_param_value("limit"), "limit");
+        if (req.has_param("offset")) opts.offset = parseUInt(req.get_param_value("offset"), "offset");
         if (req.has_param("sort"))   opts.sortField = req.get_param_value("sort");
         if (req.has_param("desc"))   opts.sortDescending = (req.get_param_value("desc") == "true");
         auto docs = d->db.client().find(c, opts);

+ 14 - 0
src/handlers/keys.cpp

@@ -36,6 +36,14 @@ void registerKeyRoutes(ApiServer& s) {
         if (body.contains("label")) label = body["label"].get<std::string>();
         if (body.contains("projects")) projects = body["projects"].get<std::vector<std::string>>();
         if (body.contains("admin")) admin = body["admin"].get<bool>();
+        if (admin.has_value() && !*admin) {
+            auto target = d->keys.lookup(key);
+            if (target && target->admin) {
+                int adminCount = 0;
+                for (const auto& k : d->keys.list()) if (k.admin) ++adminCount;
+                if (adminCount <= 1) throw ApiError(ErrCode::Unprocessable, "last_admin", "cannot remove the last admin key");
+            }
+        }
         if (!d->keys.update(key, label, projects, admin))
             throw ApiError(ErrCode::NotFound, "not_found", "no such key");
         sendJson(res, 200, {{"updated", true}});
@@ -44,6 +52,12 @@ void registerKeyRoutes(ApiServer& s) {
     svr.Delete(R"(/api/v1/keys/([^/]+))", [d](const httplib::Request& req, httplib::Response& res) {
         requireAdmin(requireKey(*d, req));
         std::string key = req.matches[1];
+        auto target = d->keys.lookup(key);
+        if (target && target->admin) {
+            int adminCount = 0;
+            for (const auto& k : d->keys.list()) if (k.admin) ++adminCount;
+            if (adminCount <= 1) throw ApiError(ErrCode::Unprocessable, "last_admin", "cannot remove the last admin key");
+        }
         if (!d->keys.remove(key)) throw ApiError(ErrCode::NotFound, "not_found", "no such key");
         sendJson(res, 200, {{"deleted", true}});
     });

+ 5 - 0
src/handlers/projects.cpp

@@ -1,3 +1,5 @@
+#include "collection_registry.hpp"
+#include "db_gateway.hpp"
 #include "errors.hpp"
 #include "json_http.hpp"
 #include "server.hpp"
@@ -39,6 +41,9 @@ void registerProjectRoutes(ApiServer& s) {
         ApiKey k = requireKey(*d, req); requireAdmin(k);
         std::string project = req.matches[1];
         if (project == "default") throw ApiError(ErrCode::Unprocessable, "protected", "cannot drop the default project");
+        for (const auto& m : d->registry.list(project))
+            d->db.client().dropCollection(qualify(project, m.name));
+        d->db.client().dropCollection(qualify(project, "vectorapi_collections"));
         if (!d->db.dropProject(project)) throw ApiError(ErrCode::Unavailable, "db_error", "drop failed");
         sendJson(res, 200, {{"deleted", project}});
     });

+ 11 - 3
src/server.cpp

@@ -47,10 +47,18 @@ void ApiServer::registerRoutes() {
         return httplib::Server::HandlerResponse::Unhandled;
     });
 
-    svr_.set_post_routing_handler([this](const httplib::Request&, httplib::Response& res) {
+    svr_.set_post_routing_handler([this](const httplib::Request& req, httplib::Response& res) {
         auto snap = d_.settings.snapshot();
-        const std::string origin = snap->corsOrigins.empty() ? "*" : snap->corsOrigins.front();
-        res.set_header("Access-Control-Allow-Origin", origin);
+        std::string responseOrigin;
+        if (!snap->corsOrigins.empty() && snap->corsOrigins[0] == "*") {
+            responseOrigin = "*";
+        } else if (auto it = req.headers.find("Origin"); it != req.headers.end()) {
+            for (const auto& a : snap->corsOrigins) if (a == it->second) { responseOrigin = it->second; break; }
+        }
+        if (!responseOrigin.empty()) {
+            res.set_header("Access-Control-Allow-Origin", responseOrigin);
+            if (responseOrigin != "*") res.set_header("Vary", "Origin");
+        }
         res.set_header("Access-Control-Allow-Headers", "Authorization, Content-Type");
         res.set_header("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS");
     });

+ 3 - 5
src/settings_store.cpp

@@ -15,11 +15,9 @@ void SettingsStore::bootstrap(const std::string& envOpenAiKey) {
     if (auto doc = db_.client().get(coll_, docId_))
         s = Settings::fromJson(*doc);
 
-    if (s.openaiApiKey.empty() && !envOpenAiKey.empty())
-        s.openaiApiKey = envOpenAiKey;
-
-    if (!db_.client().exists(coll_, docId_))
-        db_.client().upsert(coll_, s.toJson(), docId_);
+    bool seeded = false;
+    if (s.openaiApiKey.empty() && !envOpenAiKey.empty()) { s.openaiApiKey = envOpenAiKey; seeded = true; }
+    if (seeded || !db_.client().exists(coll_, docId_)) db_.client().upsert(coll_, s.toJson(), docId_);
 
     setSnapshot(s);
 }

+ 93 - 0
tests/test_api_integration.cpp

@@ -164,3 +164,96 @@ TEST_F(ApiFixture, OpenApiDescribesProjectRoutes) {
     EXPECT_TRUE(j["paths"].contains("/api/v1/projects/{project}/collections/{name}/search"));
     EXPECT_TRUE(j["paths"].contains("/api/v1/keys"));
 }
+
+// Fix 1 regression: CORS multi-origin
+TEST_F(ApiFixture, CorsEchosAllowedOrigin) {
+    httplib::Client c("127.0.0.1", port_);
+    c.set_default_headers({{"Authorization", "Bearer " + adminKey_},
+                           {"Origin", "https://example.com"}});
+    // Seed corsOrigins with a specific origin via settings
+    svapi::Settings s = *settings_->snapshot();
+    s.corsOrigins = {"https://example.com", "https://other.com"};
+    settings_->save(s);
+    auto r = c.Get("/api/v1/projects");
+    ASSERT_TRUE(r);
+    EXPECT_EQ(r->get_header_value("Access-Control-Allow-Origin"), "https://example.com");
+    // Vary may contain multiple values; check that "Origin" appears somewhere in them
+    bool varyHasOrigin = false;
+    for (const auto& [name, val] : r->headers)
+        if (name == "Vary" && val.find("Origin") != std::string::npos) { varyHasOrigin = true; break; }
+    EXPECT_TRUE(varyHasOrigin);
+    // Restore
+    s.corsOrigins = {"*"};
+    settings_->save(s);
+}
+
+TEST_F(ApiFixture, CorsRejectsUnknownOrigin) {
+    httplib::Client c("127.0.0.1", port_);
+    c.set_default_headers({{"Authorization", "Bearer " + adminKey_},
+                           {"Origin", "https://evil.com"}});
+    svapi::Settings s = *settings_->snapshot();
+    s.corsOrigins = {"https://example.com"};
+    settings_->save(s);
+    auto r = c.Get("/api/v1/projects");
+    ASSERT_TRUE(r);
+    EXPECT_TRUE(r->get_header_value("Access-Control-Allow-Origin").empty());
+    // Restore
+    s.corsOrigins = {"*"};
+    settings_->save(s);
+}
+
+// Fix 2 regression: bad limit/offset → 422
+TEST_F(ApiFixture, BadLimitReturns422) {
+    auto c = admin();
+    std::string base = "/api/v1/projects/" + project_ + "/collections";
+    ASSERT_EQ(c.Post(base.c_str(), nlohmann::json{{"name","limitcoll"},{"kind","json"}}.dump(),"application/json")->status, 201);
+    auto r = c.Get((base + "/limitcoll/documents?limit=abc").c_str());
+    ASSERT_TRUE(r);
+    EXPECT_EQ(r->status, 422);
+    c.Delete((base + "/limitcoll").c_str());
+}
+
+// Fix 3 regression: DELETE project cleans up stale registry → recycle works
+TEST_F(ApiFixture, ProjectRecycleAfterDelete) {
+    auto c = admin();
+    std::string p2 = "svapitest_recycle";
+    db_->dropProject(p2);
+    db_->client().dropCollection(svapi::qualify(p2, "vectorapi_collections"));
+    // Create project and collection
+    ASSERT_EQ(c.Post("/api/v1/projects", nlohmann::json{{"name", p2}}.dump(), "application/json")->status, 201);
+    std::string base = "/api/v1/projects/" + p2 + "/collections";
+    ASSERT_EQ(c.Post(base.c_str(), nlohmann::json{{"name","users"},{"kind","json"}}.dump(), "application/json")->status, 201);
+    // Delete the project (should purge collections + registry)
+    ASSERT_EQ(c.Delete(("/api/v1/projects/" + p2).c_str())->status, 200);
+    // Re-create project and collection — must succeed (201, not 422)
+    ASSERT_EQ(c.Post("/api/v1/projects", nlohmann::json{{"name", p2}}.dump(), "application/json")->status, 201);
+    auto r = c.Post(base.c_str(), nlohmann::json{{"name","users"},{"kind","json"}}.dump(), "application/json");
+    ASSERT_TRUE(r);
+    EXPECT_EQ(r->status, 201);
+    // Clean up
+    c.Delete(("/api/v1/projects/" + p2).c_str());
+    db_->dropProject(p2);
+}
+
+// Fix 5 regression: cannot delete the last admin key
+TEST_F(ApiFixture, CannotDeleteLastAdminKey) {
+    auto r = admin().Delete(("/api/v1/keys/" + adminKey_).c_str());
+    ASSERT_TRUE(r);
+    EXPECT_EQ(r->status, 422);
+    // Key still works
+    auto check = admin().Get("/api/v1/projects");
+    ASSERT_TRUE(check);
+    EXPECT_EQ(check->status, 200);
+}
+
+// Fix 5 regression: cannot de-admin the last admin key
+TEST_F(ApiFixture, CannotDeAdminLastAdminKey) {
+    auto r = admin().Patch(("/api/v1/keys/" + adminKey_).c_str(),
+                           nlohmann::json{{"admin", false}}.dump(), "application/json");
+    ASSERT_TRUE(r);
+    EXPECT_EQ(r->status, 422);
+    // Key still works as admin
+    auto check = admin().Get("/api/v1/keys");
+    ASSERT_TRUE(check);
+    EXPECT_EQ(check->status, 200);
+}