diff --git a/CHANGELOG.md b/CHANGELOG.md index fcd6c8109a..7bec27a1a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,9 @@ and this project adheres to [PR#4551](https://github.com/OpenFn/lightning/pull/4551) - Fix AI assistant authorization for support users on projects with support access enabled [#4571](https://github.com/OpenFn/lightning/issues/4571) +- REST API controllers now return 400 instead of 500 for malformed UUID + parameters. Extracts a shared `validate_uuid/1` helper used across all API + controllers. [#4588](https://github.com/OpenFn/lightning/issues/4588) ## [2.16.0] - 2026-03-24 diff --git a/lib/lightning_web/controllers/api/ai_assistant_controller.ex b/lib/lightning_web/controllers/api/ai_assistant_controller.ex index b6bd6ad130..57b0ea0c1f 100644 --- a/lib/lightning_web/controllers/api/ai_assistant_controller.ex +++ b/lib/lightning_web/controllers/api/ai_assistant_controller.ex @@ -9,6 +9,7 @@ defmodule LightningWeb.API.AiAssistantController do alias Lightning.Policies.Permissions alias Lightning.Projects alias Lightning.Workflows + alias LightningWeb.API.Helpers alias LightningWeb.Channels.AiAssistantJSON action_fallback LightningWeb.FallbackController @@ -75,7 +76,9 @@ defmodule LightningWeb.API.AiAssistantController do end defp get_resource("job_code", %{"job_id" => job_id}) do - {:ok, job_id} + with :ok <- Helpers.validate_uuid(job_id) do + {:ok, job_id} + end end defp get_resource("job_code", _params) do @@ -83,9 +86,11 @@ defmodule LightningWeb.API.AiAssistantController do end defp get_resource("workflow_template", %{"project_id" => project_id}) do - case Projects.get_project(project_id) do - nil -> {:error, :not_found} - project -> {:ok, project} + with :ok <- Helpers.validate_uuid(project_id) do + case Projects.get_project(project_id) do + nil -> {:error, :not_found} + project -> {:ok, project} + end end end diff --git a/lib/lightning_web/controllers/api/credential_controller.ex b/lib/lightning_web/controllers/api/credential_controller.ex index c8a12b3baf..9204dd0ee2 100644 --- a/lib/lightning_web/controllers/api/credential_controller.ex +++ b/lib/lightning_web/controllers/api/credential_controller.ex @@ -25,6 +25,7 @@ defmodule LightningWeb.API.CredentialController do alias Lightning.Policies.Permissions alias Lightning.Policies.ProjectUsers alias Lightning.Projects + alias LightningWeb.API.Helpers action_fallback LightningWeb.FallbackController @@ -61,7 +62,8 @@ defmodule LightningWeb.API.CredentialController do def index(conn, %{"project_id" => project_id}) do current_user = conn.assigns.current_resource - with project when not is_nil(project) <- Projects.get_project(project_id), + with :ok <- Helpers.validate_uuid(project_id), + project when not is_nil(project) <- Projects.get_project(project_id), :ok <- ProjectUsers |> Permissions.can( @@ -166,15 +168,15 @@ defmodule LightningWeb.API.CredentialController do def delete(conn, %{"id" => id}) do current_user = conn.assigns.current_resource - with :ok <- validate_uuid(id), + with :ok <- Helpers.validate_uuid(id), credential when not is_nil(credential) <- Credentials.get_credential(id), :ok <- validate_credential_ownership(credential, current_user), {:ok, _} <- Credentials.delete_credential(credential) do send_resp(conn, :no_content, "") else - {:error, :invalid_uuid} -> - {:error, :not_found} + {:error, :bad_request} -> + {:error, :bad_request} nil -> {:error, :not_found} @@ -187,13 +189,6 @@ defmodule LightningWeb.API.CredentialController do end end - defp validate_uuid(id) do - case Ecto.UUID.dump(to_string(id)) do - {:ok, _bin} -> :ok - :error -> {:error, :invalid_uuid} - end - end - defp validate_credential_ownership(credential, current_user) do if credential.user_id == current_user.id do :ok diff --git a/lib/lightning_web/controllers/api/helpers.ex b/lib/lightning_web/controllers/api/helpers.ex index 0c21dae60e..629007576b 100644 --- a/lib/lightning_web/controllers/api/helpers.ex +++ b/lib/lightning_web/controllers/api/helpers.ex @@ -51,4 +51,20 @@ defmodule LightningWeb.API.Helpers do } |> URI.to_string() end + + @doc """ + Validates that the given value is a well-formed UUID. + + Returns `:ok` on success or `{:error, :bad_request}` when the value + cannot be parsed as a UUID. Use this in API controllers before passing + an ID to the database layer, which would raise `Ecto.Query.CastError` + for invalid values. + """ + @spec validate_uuid(any()) :: :ok | {:error, :bad_request} + def validate_uuid(id) do + case Ecto.UUID.dump(to_string(id)) do + {:ok, _bin} -> :ok + :error -> {:error, :bad_request} + end + end end diff --git a/lib/lightning_web/controllers/api/job_controller.ex b/lib/lightning_web/controllers/api/job_controller.ex index a417753ecc..c749ae518a 100644 --- a/lib/lightning_web/controllers/api/job_controller.ex +++ b/lib/lightning_web/controllers/api/job_controller.ex @@ -24,6 +24,7 @@ defmodule LightningWeb.API.JobController do alias Lightning.Policies.Permissions alias Lightning.Policies.ProjectUsers alias Lightning.Workflows + alias LightningWeb.API.Helpers action_fallback LightningWeb.FallbackController @@ -64,7 +65,8 @@ defmodule LightningWeb.API.JobController do def index(conn, %{"project_id" => project_id} = params) do pagination_attrs = Map.take(params, ["page_size", "page"]) - with project <- Lightning.Projects.get_project(project_id), + with :ok <- Helpers.validate_uuid(project_id), + project <- Lightning.Projects.get_project(project_id), :ok <- ProjectUsers |> Permissions.can( @@ -115,7 +117,8 @@ defmodule LightningWeb.API.JobController do """ @spec show(Plug.Conn.t(), map()) :: Plug.Conn.t() def show(conn, %{"id" => id}) do - with job <- Jobs.get_job!(id), + with :ok <- Helpers.validate_uuid(id), + job <- Jobs.get_job!(id), job_with_project <- Lightning.Repo.preload(job, workflow: :project), :ok <- ProjectUsers diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index 888d20f7b4..c5813f2cda 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -20,6 +20,7 @@ defmodule LightningWeb.API.ProjectController do alias Lightning.Policies.Permissions alias Lightning.Policies.ProjectUsers alias Lightning.Projects + alias LightningWeb.API.Helpers action_fallback LightningWeb.FallbackController @@ -78,7 +79,8 @@ defmodule LightningWeb.API.ProjectController do """ @spec show(Plug.Conn.t(), map()) :: Plug.Conn.t() def show(conn, %{"id" => id}) do - with project <- Projects.get_project(id), + with :ok <- Helpers.validate_uuid(id), + project <- Projects.get_project(id), :ok <- ProjectUsers |> Permissions.can( diff --git a/lib/lightning_web/controllers/api/provisioning_controller.ex b/lib/lightning_web/controllers/api/provisioning_controller.ex index d993d4b3ac..199f638ffc 100644 --- a/lib/lightning_web/controllers/api/provisioning_controller.ex +++ b/lib/lightning_web/controllers/api/provisioning_controller.ex @@ -20,6 +20,7 @@ defmodule LightningWeb.API.ProvisioningController do alias Lightning.Projects.Provisioner alias Lightning.Workflows alias Lightning.WorkflowVersions + alias LightningWeb.API.Helpers action_fallback(LightningWeb.FallbackController) @@ -129,7 +130,8 @@ defmodule LightningWeb.API.ProvisioningController do """ @spec show(Plug.Conn.t(), map()) :: Plug.Conn.t() def show(conn, params) do - with project = %Project{} <- + with :ok <- Helpers.validate_uuid(params["id"]), + project = %Project{} <- Projects.get_project(params["id"]) || {:error, :not_found}, :ok <- Permissions.can( @@ -186,7 +188,8 @@ defmodule LightningWeb.API.ProvisioningController do """ @spec show_yaml(Plug.Conn.t(), map()) :: Plug.Conn.t() def show_yaml(conn, %{"id" => id} = params) do - with %Projects.Project{} = project <- + with :ok <- Helpers.validate_uuid(id), + %Projects.Project{} = project <- Projects.get_project(id) || {:error, :not_found}, :ok <- Permissions.can( diff --git a/lib/lightning_web/controllers/api/run_controller.ex b/lib/lightning_web/controllers/api/run_controller.ex index b04ef94a60..6bce70ea39 100644 --- a/lib/lightning_web/controllers/api/run_controller.ex +++ b/lib/lightning_web/controllers/api/run_controller.ex @@ -25,6 +25,7 @@ defmodule LightningWeb.API.RunController do alias Lightning.Policies.Permissions alias Lightning.Policies.ProjectUsers alias Lightning.Runs + alias LightningWeb.API.Helpers action_fallback LightningWeb.FallbackController @@ -72,7 +73,8 @@ defmodule LightningWeb.API.RunController do def index(conn, %{"project_id" => project_id} = params) do pagination_attrs = Map.take(params, ["page_size", "page"]) - with :ok <- + with :ok <- Helpers.validate_uuid(project_id), + :ok <- Invocation.Query.validate_datetime_params(params, [ "inserted_after", "inserted_before", @@ -140,7 +142,8 @@ defmodule LightningWeb.API.RunController do """ @spec show(Plug.Conn.t(), map()) :: Plug.Conn.t() def show(conn, %{"id" => id}) do - with run <- Runs.get(id, include: [work_order: [workflow: :project]]), + with :ok <- Helpers.validate_uuid(id), + run <- Runs.get(id, include: [work_order: [workflow: :project]]), :ok <- ProjectUsers |> Permissions.can( diff --git a/lib/lightning_web/controllers/api/work_orders_controller.ex b/lib/lightning_web/controllers/api/work_orders_controller.ex index 2d3ea6fb89..124c671fe2 100644 --- a/lib/lightning_web/controllers/api/work_orders_controller.ex +++ b/lib/lightning_web/controllers/api/work_orders_controller.ex @@ -25,6 +25,7 @@ defmodule LightningWeb.API.WorkOrdersController do alias Lightning.Policies.Permissions alias Lightning.Policies.ProjectUsers alias Lightning.WorkOrders + alias LightningWeb.API.Helpers action_fallback LightningWeb.FallbackController @@ -72,7 +73,8 @@ defmodule LightningWeb.API.WorkOrdersController do def index(conn, %{"project_id" => project_id} = params) do pagination_attrs = Map.take(params, ["page_size", "page"]) - with :ok <- + with :ok <- Helpers.validate_uuid(project_id), + :ok <- Invocation.Query.validate_datetime_params(params, [ "inserted_after", "inserted_before", @@ -140,7 +142,8 @@ defmodule LightningWeb.API.WorkOrdersController do """ @spec show(Plug.Conn.t(), map()) :: Plug.Conn.t() def show(conn, %{"id" => id}) do - with work_order <- + with :ok <- Helpers.validate_uuid(id), + work_order <- WorkOrders.get(id, include: [workflow: :project, runs: []]), :ok <- ProjectUsers diff --git a/lib/lightning_web/controllers/api/workflows_controller.ex b/lib/lightning_web/controllers/api/workflows_controller.ex index 4ff62b7b4b..0b8386d530 100644 --- a/lib/lightning_web/controllers/api/workflows_controller.ex +++ b/lib/lightning_web/controllers/api/workflows_controller.ex @@ -44,6 +44,7 @@ defmodule LightningWeb.API.WorkflowsController do alias Lightning.Workflows.Edge alias Lightning.Workflows.Presence alias Lightning.Workflows.Workflow + alias LightningWeb.API.Helpers alias LightningWeb.ChangesetJSON action_fallback LightningWeb.FallbackController @@ -451,10 +452,10 @@ defmodule LightningWeb.API.WorkflowsController do end end - defp validate_uuid(project_id) do - case Ecto.UUID.dump(to_string(project_id)) do - {:ok, _bin} -> :ok - :error -> {:error, :invalid_id, project_id} + defp validate_uuid(id) do + case Helpers.validate_uuid(id) do + :ok -> :ok + {:error, :bad_request} -> {:error, :invalid_id, id} end end diff --git a/test/lightning_web/controllers/api/credential_controller_test.exs b/test/lightning_web/controllers/api/credential_controller_test.exs index d2b41c5aed..22c73f4c82 100644 --- a/test/lightning_web/controllers/api/credential_controller_test.exs +++ b/test/lightning_web/controllers/api/credential_controller_test.exs @@ -242,6 +242,11 @@ defmodule LightningWeb.API.CredentialControllerTest do assert json_response(conn, 404) == %{"error" => "Not Found"} end + test "returns 400 for malformed project_id", %{conn: conn, user: _user} do + conn = get(conn, ~p"/api/projects/not-a-uuid/credentials") + assert json_response(conn, 400) == %{"error" => "Bad Request"} + end + test "returns empty list when project has no credentials", %{ conn: conn, user: user @@ -822,7 +827,7 @@ defmodule LightningWeb.API.CredentialControllerTest do test "handles invalid UUID format", %{conn: conn, user: _user} do conn = delete(conn, ~p"/api/credentials/invalid-uuid") - assert json_response(conn, 404) == %{"error" => "Not Found"} + assert json_response(conn, 400) == %{"error" => "Bad Request"} end end end diff --git a/test/lightning_web/controllers/api/job_controller_test.exs b/test/lightning_web/controllers/api/job_controller_test.exs index 50ae7e60a2..b55eabb18c 100644 --- a/test/lightning_web/controllers/api/job_controller_test.exs +++ b/test/lightning_web/controllers/api/job_controller_test.exs @@ -89,6 +89,20 @@ defmodule LightningWeb.API.JobControllerTest do "type" => "jobs" } end + + test "returns 400 for malformed id", %{conn: conn} do + conn = get(conn, ~p"/api/jobs/not-a-uuid") + assert json_response(conn, 400) == %{"error" => "Bad Request"} + end + end + + describe "index with invalid project_id" do + setup [:assign_bearer_for_api] + + test "returns 400 for malformed project_id", %{conn: conn} do + conn = get(conn, ~p"/api/projects/not-a-uuid/jobs") + assert json_response(conn, 400) == %{"error" => "Bad Request"} + end end defp create_job(%{project: project}) do diff --git a/test/lightning_web/controllers/api/project_controller_test.exs b/test/lightning_web/controllers/api/project_controller_test.exs index a38394797d..88dd6dbba7 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -86,6 +86,11 @@ defmodule LightningWeb.API.ProjectControllerTest do assert json_response(conn, 401) == %{"error" => "Unauthorized"} end + test "returns 400 for malformed id", %{conn: conn} do + conn = get(conn, ~p"/api/projects/not-a-uuid") + assert json_response(conn, 400) == %{"error" => "Bad Request"} + end + test "shows the project", %{conn: conn, project: project} do conn = get(conn, Routes.api_project_path(conn, :show, project)) response = json_response(conn, 200) diff --git a/test/lightning_web/controllers/api/provisioning_controller_test.exs b/test/lightning_web/controllers/api/provisioning_controller_test.exs index 5587cb4446..1051ba877e 100644 --- a/test/lightning_web/controllers/api/provisioning_controller_test.exs +++ b/test/lightning_web/controllers/api/provisioning_controller_test.exs @@ -1362,4 +1362,18 @@ defmodule LightningWeb.API.ProvisioningControllerTest do end) end) end + + describe "malformed UUID params" do + setup [:assign_bearer_for_api] + + test "returns 400 for malformed id in show", %{conn: conn} do + conn = get(conn, ~p"/api/provision/not-a-uuid") + assert json_response(conn, 400) == %{"error" => "Bad Request"} + end + + test "returns 400 for malformed id in show_yaml", %{conn: conn} do + conn = get(conn, ~p"/api/provision/yaml?id=not-a-uuid") + assert json_response(conn, 400) == %{"error" => "Bad Request"} + end + end end diff --git a/test/lightning_web/controllers/api/run_controller_test.exs b/test/lightning_web/controllers/api/run_controller_test.exs index 2beb038b45..d83a7cba55 100644 --- a/test/lightning_web/controllers/api/run_controller_test.exs +++ b/test/lightning_web/controllers/api/run_controller_test.exs @@ -688,4 +688,18 @@ defmodule LightningWeb.API.RunControllerTest do assert error_message =~ "123456" end end + + describe "malformed UUID params" do + setup [:assign_bearer_for_api] + + test "returns 400 for malformed project_id in index", %{conn: conn} do + conn = get(conn, ~p"/api/projects/not-a-uuid/runs") + assert json_response(conn, 400) == %{"error" => "Bad Request"} + end + + test "returns 400 for malformed id in show", %{conn: conn} do + conn = get(conn, ~p"/api/runs/not-a-uuid") + assert json_response(conn, 400) == %{"error" => "Bad Request"} + end + end end diff --git a/test/lightning_web/controllers/api/work_orders_controller_test.exs b/test/lightning_web/controllers/api/work_orders_controller_test.exs index b9886e0316..3748001502 100644 --- a/test/lightning_web/controllers/api/work_orders_controller_test.exs +++ b/test/lightning_web/controllers/api/work_orders_controller_test.exs @@ -446,4 +446,18 @@ defmodule LightningWeb.API.WorkOrdersControllerTest do assert error_message =~ "123456" end end + + describe "malformed UUID params" do + setup [:assign_bearer_for_api] + + test "returns 400 for malformed project_id in index", %{conn: conn} do + conn = get(conn, ~p"/api/projects/not-a-uuid/work_orders") + assert json_response(conn, 400) == %{"error" => "Bad Request"} + end + + test "returns 400 for malformed id in show", %{conn: conn} do + conn = get(conn, ~p"/api/work_orders/not-a-uuid") + assert json_response(conn, 400) == %{"error" => "Bad Request"} + end + end end