From 516e7a63af4156d9c25d2e79df7a58c9b2ea2f1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Macdara=20=C3=93=20Murch=C3=BA?= Date: Wed, 1 Apr 2026 09:54:16 +0800 Subject: [PATCH] Standardize UUID validation across all REST API controllers Extract a shared validate_uuid/1 function into LightningWeb.API.Helpers using the existing Ecto.UUID.dump/1 pattern. Replace private duplicate implementations in WorkflowsController and CredentialController. Apply validation to all controllers that accept UUID path or query params, so malformed UUIDs return 400 instead of raising Ecto.Query.CastError (500). Closes #4588 --- CHANGELOG.md | 3 +++ .../api/ai_assistant_controller.ex | 13 ++++++++---- .../controllers/api/credential_controller.ex | 20 +++++++++---------- lib/lightning_web/controllers/api/helpers.ex | 16 +++++++++++++++ .../controllers/api/job_controller.ex | 7 +++++-- .../controllers/api/project_controller.ex | 4 +++- .../api/provisioning_controller.ex | 7 +++++-- .../controllers/api/run_controller.ex | 7 +++++-- .../controllers/api/work_orders_controller.ex | 7 +++++-- .../controllers/api/workflows_controller.ex | 9 +++++---- .../api/credential_controller_test.exs | 7 ++++++- .../controllers/api/job_controller_test.exs | 14 +++++++++++++ .../api/project_controller_test.exs | 5 +++++ .../api/provisioning_controller_test.exs | 14 +++++++++++++ .../controllers/api/run_controller_test.exs | 14 +++++++++++++ .../api/work_orders_controller_test.exs | 14 +++++++++++++ 16 files changed, 132 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3014d74af4c..6bc04536ca9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -281,6 +281,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 c35e12cb7b6..9bf979aface 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 c8a12b3baf8..1adb91e25d2 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( @@ -72,6 +74,9 @@ defmodule LightningWeb.API.CredentialController do credentials = Credentials.list_credentials(project) render(conn, "index.json", credentials: credentials) else + {:error, :bad_request} -> + {:error, :bad_request} + nil -> {:error, :not_found} @@ -166,15 +171,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 +192,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 0c21dae60e6..629007576bd 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 6378467db60..1bbcc844a4a 100644 --- a/lib/lightning_web/controllers/api/job_controller.ex +++ b/lib/lightning_web/controllers/api/job_controller.ex @@ -54,6 +54,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 @@ -94,7 +95,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( @@ -145,7 +147,8 @@ defmodule LightningWeb.API.JobController do """ @spec show(Plug.Conn.t(), map()) :: Plug.Conn.t() def show(conn, %{"id" => id}) do - with {:ok, job} <- Jobs.get_job(id, include: [workflow: :project]), + with :ok <- Helpers.validate_uuid(id), + {:ok, job} <- Jobs.get_job(id, include: [workflow: :project]), :ok <- ProjectUsers |> Permissions.can( diff --git a/lib/lightning_web/controllers/api/project_controller.ex b/lib/lightning_web/controllers/api/project_controller.ex index 9b0edba3774..1cbae4509c4 100644 --- a/lib/lightning_web/controllers/api/project_controller.ex +++ b/lib/lightning_web/controllers/api/project_controller.ex @@ -36,6 +36,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 @@ -94,7 +95,8 @@ defmodule LightningWeb.API.ProjectController do """ @spec show(Plug.Conn.t(), map()) :: Plug.Conn.t() def show(conn, %{"id" => id}) do - with %Lightning.Projects.Project{} = project <- + with :ok <- Helpers.validate_uuid(id), + %Lightning.Projects.Project{} = project <- Projects.get_project(id), :ok <- ProjectUsers diff --git a/lib/lightning_web/controllers/api/provisioning_controller.ex b/lib/lightning_web/controllers/api/provisioning_controller.ex index d993d4b3ac3..199f638ffcf 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 7fb4346cf64..bdc83887822 100644 --- a/lib/lightning_web/controllers/api/run_controller.ex +++ b/lib/lightning_web/controllers/api/run_controller.ex @@ -56,6 +56,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 @@ -104,7 +105,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", @@ -174,7 +176,8 @@ defmodule LightningWeb.API.RunController do """ @spec show(Plug.Conn.t(), map()) :: Plug.Conn.t() def show(conn, %{"id" => id}) do - with %Lightning.Run{} = run <- + with :ok <- Helpers.validate_uuid(id), + %Lightning.Run{} = run <- Runs.get(id, include: [work_order: [workflow: :project]]), :ok <- ProjectUsers diff --git a/lib/lightning_web/controllers/api/work_orders_controller.ex b/lib/lightning_web/controllers/api/work_orders_controller.ex index 09ef28d042d..d25505452eb 100644 --- a/lib/lightning_web/controllers/api/work_orders_controller.ex +++ b/lib/lightning_web/controllers/api/work_orders_controller.ex @@ -63,6 +63,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 @@ -111,7 +112,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", @@ -181,7 +183,8 @@ defmodule LightningWeb.API.WorkOrdersController do """ @spec show(Plug.Conn.t(), map()) :: Plug.Conn.t() def show(conn, %{"id" => id}) do - with %Lightning.WorkOrder{} = work_order <- + with :ok <- Helpers.validate_uuid(id), + %Lightning.WorkOrder{} = 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 f42bca7b9d9..95d0ab65162 100644 --- a/lib/lightning_web/controllers/api/workflows_controller.ex +++ b/lib/lightning_web/controllers/api/workflows_controller.ex @@ -92,6 +92,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 @@ -499,10 +500,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 ad100c71c33..7f99d7ed048 100644 --- a/test/lightning_web/controllers/api/credential_controller_test.exs +++ b/test/lightning_web/controllers/api/credential_controller_test.exs @@ -261,6 +261,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 @@ -841,7 +846,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 e1655b01eb6..2a0b9629f3e 100644 --- a/test/lightning_web/controllers/api/job_controller_test.exs +++ b/test/lightning_web/controllers/api/job_controller_test.exs @@ -94,6 +94,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 f4e10d2b650..966729f3fe4 100644 --- a/test/lightning_web/controllers/api/project_controller_test.exs +++ b/test/lightning_web/controllers/api/project_controller_test.exs @@ -91,6 +91,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 47c8bbee57b..2219b001930 100644 --- a/test/lightning_web/controllers/api/provisioning_controller_test.exs +++ b/test/lightning_web/controllers/api/provisioning_controller_test.exs @@ -1441,4 +1441,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 3d17b1ad7fc..6bef71cd27e 100644 --- a/test/lightning_web/controllers/api/run_controller_test.exs +++ b/test/lightning_web/controllers/api/run_controller_test.exs @@ -893,4 +893,18 @@ defmodule LightningWeb.API.RunControllerTest do assert json_response(conn, 401) 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 453bd843400..9627f9a2e41 100644 --- a/test/lightning_web/controllers/api/work_orders_controller_test.exs +++ b/test/lightning_web/controllers/api/work_orders_controller_test.exs @@ -708,4 +708,18 @@ defmodule LightningWeb.API.WorkOrdersControllerTest do assert json_response(conn, 401) 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