Skip to content

server_send_error() sends every error through server_send_ok(), so failures return HTTP 200 #2065

Description

@JBailes

Found during exploratory testing. Confirmed in source on testing; not fixed, because changing status codes is a contract change that needs an owner's call — existing clients may depend on the current behaviour.

What

Handler-level errors on /v1 come back with a success status code:

memory/store valid content    -> HTTP 200  {"status":"error","message":"failed to store memory"}
memory/store empty string     -> HTTP 200  {"status":"error","message":"failed to store memory"}
memory/store missing content  -> HTTP 200  {"status":"error","message":"missing key or content"}

The framework itself gets this right — it is only the handler path that does not:

malformed JSON body           -> HTTP 400  {"error":"invalid JSON body"}
unknown endpoint              -> HTTP 404  {"error":"not found"}

So a client checking HTTP status — including anything using curl -f, most HTTP libraries' raise_for_status(), and monitoring probes — reads a failed write as a success.

Why

src/server/server.c:

int server_send_error(server_conn_t *conn, const char *message, const char *request_id)
{
   cJSON *resp = cJSON_CreateObject();
   cJSON_AddStringToObject(resp, "status", "error");
   cJSON_AddStringToObject(resp, "message", message);
   if (request_id)
      cJSON_AddStringToObject(resp, "request_id", request_id);
   return server_send_ok(conn, resp);      // <-- the error goes out via the OK path
}

The shared error helper delivers through server_send_ok. Every caller inherits it.

handle_memory_store in src/server/server_state.c reaches it both ways:

if (jo_need_str(req, "key", &key) < 0 || jo_need_str(req, "content", &content) < 0)
   return server_send_error(conn, "missing key or content", NULL);
...
else
   resp = jo_err("failed to store memory");
return send_and_free(conn, resp);

Scope

Small: 11 jo_err call sites in total, 7 of them under src/server/ (4 in server_state.c, 3 in server_audit_replay_routes.c), plus the shared server_send_error helper itself.

Why it is worth attention

This is the same defect class as several others found this week — a confident success reported over something that did not happen — but at the protocol level, where it silently defeats every generic client-side error check rather than just misleading a human reading output.

Note on the second envelope shape

Errors here use {"status":"error","message":...} while the framework paths use {"error":...}. Two shapes for the same concept is a smaller problem than the status code, but worth deciding together if this is picked up.

Not proposed here

Whether to switch these to 4xx/5xx, and which codes, is a compatibility decision. Flagging it with the root cause pinned rather than guessing at the contract.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions