Answer 422 for an unknown label instead of the repository's 404

adc413e98c70 · AtlantisPleb · · parent 8e58fcf7b500

Answer 422 for an unknown label instead of the repository's 404

`POST /api/v3/repos/{owner}/{repo}/issues` answered `404 Not Found` when a
label in the request body did not exist. The controller rescued
`Ecto.NoResultsError` around the whole action. That is correct for the
repository lookup, which is why it was written, but it also caught the label,
assignee, and milestone lookups `Issues.create_issue/3` reaches further in, so
two unrelated failures left by the same door. `PATCH` on an issue shared the
rescue through the same `prepare_collections/2`.

The `404` is the answer the API gives deliberately for a repository the caller
cannot see, so a private repository's existence is not disclosed. That
ambiguity is load-bearing and stays. What it hid was the honest error: a caller
could not tell a privacy decision from a typo. One reporter read the router,
confirmed the route, checked the token, and concluded a store token could not
reach the `:agent_participation_api` pipeline. The token held `forge:write`.
The label did not exist.

A name the repository does not have now raises
`OpenAgents.Issues.UnknownReference`, carrying the request-body field and the
value, and the API answers `422` with `validation_failed` and that field in
`errors` — the way GitHub names the offending label. Assignees and milestones
shared the defect and are fixed with it.

The rescue is narrowed to what it was written for.
`OpenAgentsWeb.ControllerHelpers.lookup/1` wraps one lookup and tags the
outcome, and neither `OpenAgentsWeb.IssueController` nor
`OpenAgentsWeb.CommentController` names `Ecto.NoResultsError` any more — a
proof reads both files for it, so a lookup added later cannot rejoin the
repository's `404` without someone deciding to. The comment writes had no
second door today; they are rewritten anyway, because the rescue's width was
the defect rather than its current contents.

The privacy `404` is proven beside the new `422`: a `POST` to a private
repository the caller is not a member of still answers `not_found`, with
`errors` empty and a body naming neither the repository nor the label.

Closes #186.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DTmy4SEXrHXouw5sZbs3f4
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes
#186

Deploy story

What this commit did to the running system — joined from the forge receipt chain, the part a commit page elsewhere cannot show.

built
157 modules in 145.8 s
deployed
live · 157 modules on 3 nodes · push→live —
deployed
needs_rolling_replace · 157 modules on 0 nodes · push→live —

Changed files

  • modified INVARIANTS.md
  • modified lib/openagents/issues.ex
  • added lib/openagents/issues/unknown_reference.ex
  • modified lib/openagents_web/controllers/comment_controller.ex
  • modified lib/openagents_web/controllers/controller_helpers.ex
  • modified lib/openagents_web/controllers/issue_controller.ex
  • modified test/openagents/issues_test.exs
  • modified test/openagents/notifications_test.exs
  • modified test/openagents_web/controllers/issue_controller_test.exs

Diff

9 files changed, +490 -152

INVARIANTS.md modified +24 -1

@@ -4393,6 +4393,26 @@ Non-disclosure is preserved by construction rather than by care: a private

4393 4393
resource and an absent one both refuse with `not_found`, and no code in the
4394 4394
table distinguishes them.
4395 4395
4396
Amended 2026-08-23 (issue #186): that ambiguity is load-bearing, so nothing
4397
else may hide behind it. A `rescue Ecto.NoResultsError` wrapped around a whole
4398
controller action caught every bang lookup the action reached, not only the
4399
repository one it was written for, so `POST /api/v3/repos/{owner}/{repo}/issues`
4400
answered `404` for a label the request body named that the repository does not
4401
have — the same `404` a repository the caller cannot see answers with, which
4402
made the honest error unreachable and sent one reporter to the router and the
4403
token before the label. A lookup that may not resolve is now wrapped one at a
4404
time by `OpenAgentsWeb.ControllerHelpers.lookup/1`; neither
4405
`OpenAgentsWeb.IssueController` nor `OpenAgentsWeb.CommentController` names
4406
`Ecto.NoResultsError` at all, and a proof reads both files for it, so a lookup
4407
added later cannot rejoin the repository's `404` without someone deciding to.
4408
A label, assignee, or milestone the repository does not have raises
4409
`OpenAgents.Issues.UnknownReference` and answers `422` with `validation_failed`
4410
and the request-body field naming the value that did not resolve, the way
4411
GitHub answers. The privacy `404` is unchanged and proven beside it: a `POST`
4412
to a private repository the caller is not a member of still answers
4413
`not_found`, with `errors` empty and a body that names neither the repository
4414
nor the label.
4415
4396 4416
The published route inventory at `GET /api/v3` is derived from
4397 4417
`OpenAgentsWeb.Router.__routes__/0` through `OpenAgentsWeb.ApiRouteAuthority`,
4398 4418
never maintained beside it. Each route carries three mandatory classifications

@@ -4403,6 +4423,9 @@ answering with the envelope that answers with something else fails the build.

4403 4423
Evidence: `lib/openagents_web/api_error.ex`,
4404 4424
`lib/openagents_web/api_route_authority.ex`,
4405 4425
`lib/openagents_web/controllers/api_extension_controller.ex`,
4426
`lib/openagents_web/controllers/controller_helpers.ex`,
4427
`lib/openagents/issues/unknown_reference.ex`,
4428
`test/openagents_web/controllers/issue_controller_test.exs`,
4406 4429
`test/openagents_web/api_error_test.exs`,
4407 4430
`test/openagents_web/api_route_authority_test.exs`,
4408 4431
`test/openagents_web/controllers/api_error_contract_test.exs`, and

@@ -4444,7 +4467,7 @@ contract; the invariant prose above defines the assertion, not the filename.

4444 4467
| PROMISE-002 | `test/openagents/promise_registry_test.exs` |
4445 4468
| NOTIFY-001 | `test/openagents/notifications_test.exs`, `test/openagents_web/live/notifications_live_test.exs` |
4446 4469
4447
| FORGEAPI-001 | `test/openagents_web/controllers/api_error_contract_test.exs`, `test/openagents_web/controllers/api_extension_controller_test.exs`, `test/openagents_web/api_error_test.exs` |
4470
| FORGEAPI-001 | `test/openagents_web/controllers/api_error_contract_test.exs`, `test/openagents_web/controllers/api_extension_controller_test.exs`, `test/openagents_web/api_error_test.exs`, `test/openagents_web/controllers/issue_controller_test.exs` |
4448 4471
| DATA-001 | `test/openagents/conversations_test.exs` |
4449 4472
| DATA-002 | `test/openagents/accounts_test.exs`, `test/openagents/conversations_test.exs` |
4450 4473
| DATA-003 | `test/openagents/conversations_test.exs` |
lib/openagents/issues.ex modified +26 -4

@@ -16,7 +16,7 @@ defmodule OpenAgents.Issues do

16 16
  alias OpenAgents.Accounts.User
17 17
  alias OpenAgents.Agents.Agent
18 18
  alias OpenAgents.Analytics
19
  alias OpenAgents.Issues.{Comment, Issue, IssueDependency, TaskReferences}
19
  alias OpenAgents.Issues.{Comment, Issue, IssueDependency, TaskReferences, UnknownReference}
20 20
  alias OpenAgents.Labels
21 21
  alias OpenAgents.Labels.Label
22 22
  alias OpenAgents.Milestones

@@ -879,6 +879,19 @@ defmodule OpenAgents.Issues do

879 879
    result
880 880
  end
881 881
882
  # A name in a request body that this repository does not have is a rejected
883
  # field, not a missing resource. The bang lookups raise `Ecto.NoResultsError`,
884
  # which is also what resolving the repository raises, so leaving it unchanged
885
  # sent a typo out of the API through the same `404` a private repository
886
  # answers with. The rescue wraps one lookup and translates one failure, so a
887
  # different `Ecto.NoResultsError` raised deeper in this module keeps its own
888
  # meaning.
889
  defp resolve!(field, value, lookup) do
890
    lookup.()
891
  rescue
892
    Ecto.NoResultsError -> UnknownReference.raise!(field, value)
893
  end
894
882 895
  defp prepare_collections(attrs, repository) do
883 896
    attrs
884 897
    |> maybe_convert_milestone(repository)

@@ -892,7 +905,10 @@ defmodule OpenAgents.Issues do

892 905
893 906
  defp maybe_convert_milestone(%{"milestone" => number} = attrs, repository)
894 907
       when is_integer(number) do
895
    milestone = Milestones.get_milestone_by_number!(repository, number)
908
    milestone =
909
      resolve!(:milestone, number, fn ->
910
        Milestones.get_milestone_by_number!(repository, number)
911
      end)
896 912
897 913
    attrs
898 914
    |> Map.put("milestone", milestone_json(milestone))

@@ -911,7 +927,9 @@ defmodule OpenAgents.Issues do

911 927
    snapshots =
912 928
      Enum.map(labels, fn label ->
913 929
        name = if is_binary(label), do: label, else: label["name"] || label[:name]
914
        repository |> Labels.get_label_by_name!(name) |> label_json()
930
931
        resolve!(:labels, name, fn -> Labels.get_label_by_name!(repository, name) end)
932
        |> label_json()
915 933
      end)
916 934
917 935
    Map.put(attrs, "labels", Enum.uniq_by(snapshots, & &1["name"]))

@@ -924,7 +942,11 @@ defmodule OpenAgents.Issues do

924 942
    snapshots =
925 943
      Enum.map(assignees, fn assignee ->
926 944
        login = if is_binary(assignee), do: assignee, else: assignee["login"] || assignee[:login]
927
        repository |> Repositories.get_assignable_user_by_login!(login) |> assignee_json()
945
946
        resolve!(:assignees, login, fn ->
947
          Repositories.get_assignable_user_by_login!(repository, login)
948
        end)
949
        |> assignee_json()
928 950
      end)
929 951
930 952
    Map.put(attrs, "assignees", Enum.uniq_by(snapshots, & &1["login"]))
lib/openagents/issues/unknown_reference.ex added +44

@@ -0,0 +1,44 @@

1
defmodule OpenAgents.Issues.UnknownReference do
2
  @moduledoc """
3
  One name in a request body that the repository does not have.
4
5
  A label, an assignee, and a milestone arrive by name rather than by id, so
6
  writing an issue means resolving each of them, and each resolution can fail
7
  for a reason that has nothing to do with whether the caller may see the
8
  repository. Those lookups used to raise `Ecto.NoResultsError`, which is what
9
  the repository lookup raises too, so both failures left an API action by the
10
  same `404` — the same `404` the API returns deliberately for a repository a
11
  caller cannot see, so a client could not tell a privacy decision from a typo
12
  and the honest error was unreachable.
13
14
  This exception carries the field and the value it could not resolve, so the
15
  API answers `422` and names the offending label, login, or milestone number.
16
  """
17
18
  defexception [:field, :value, :message]
19
20
  @type t :: %__MODULE__{field: atom(), value: term(), message: String.t()}
21
22
  @doc """
23
  Raises for one name the repository does not have.
24
25
  The field is the request-body key a client sent, so the error names what the
26
  client wrote rather than the table that was read.
27
  """
28
  @spec raise!(:labels | :assignees | :milestone, term()) :: no_return()
29
  def raise!(field, value) do
30
    raise __MODULE__, field: field, value: value, message: sentence(field, value)
31
  end
32
33
  defp sentence(:labels, value),
34
    do: "#{quoted(value)} is not a label in this repository"
35
36
  defp sentence(:assignees, value),
37
    do: "#{quoted(value)} is not assignable in this repository"
38
39
  defp sentence(:milestone, value),
40
    do: "#{quoted(value)} is not a milestone in this repository"
41
42
  defp quoted(value) when is_binary(value), do: inspect(value)
43
  defp quoted(value), do: to_string(value)
44
end
lib/openagents_web/controllers/comment_controller.ex modified +78 -61

@@ -7,23 +7,25 @@ defmodule OpenAgentsWeb.CommentController do

7 7
  alias OpenAgents.Repositories
8 8
  alias OpenAgentsWeb.ApiError
9 9
10
  import OpenAgentsWeb.ControllerHelpers, only: [integer_param!: 1, lookup: 1]
11
10 12
  def index(conn, %{
11 13
        "owner" => owner,
12 14
        "repo" => repo,
13 15
        "issue_number" => issue_number
14 16
      }) do
15
    repository = Repositories.get_visible_by_path!(owner, repo, conn.assigns[:current_user])
16
17
    issue =
18
      Issues.get_issue_by_number!(
19
        repository,
20
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
21
      )
22
23
    comments = Issues.list_comments(issue)
24
    render(conn, :index, comments: comments)
25
  rescue
26
    Ecto.NoResultsError -> ApiError.not_found(conn)
17
    reader = conn.assigns[:current_user]
18
19
    with {:ok, repository} <-
20
           lookup(fn -> Repositories.get_visible_by_path!(owner, repo, reader) end),
21
         {:ok, issue} <-
22
           lookup(fn ->
23
             Issues.get_issue_by_number!(repository, integer_param!(issue_number))
24
           end) do
25
      render(conn, :index, comments: Issues.list_comments(issue))
26
    else
27
      {:error, :not_found} -> ApiError.not_found(conn)
28
    end
27 29
  end
28 30
29 31
  def create(

@@ -36,35 +38,38 @@ defmodule OpenAgentsWeb.CommentController do

36 38
      ) do
37 39
    actor = conn.assigns[:current_agent] || conn.assigns[:current_user]
38 40
39
    repository =
40
      case actor do
41
        %Agent{} -> Repositories.get_public_by_path!(owner, repo)
42
        _ -> Repositories.get_writable_by_path!(owner, repo, actor)
43
      end
44
45
    issue =
46
      Issues.get_issue_by_number!(
47
        repository,
48
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
49
      )
50
51
    if Repositories.issue_participant?(repository, actor) do
52
      case Issues.create_comment(issue, params, actor) do
53
        {:ok, %Comment{} = comment} ->
54
          conn
55
          |> put_status(:created)
56
          |> render(:show, comment: comment)
57
58
        {:error, %Ecto.Changeset{} = changeset} ->
59
          ApiError.changeset(conn, changeset)
41
    with {:ok, repository} <- lookup(fn -> write_repository(owner, repo, actor) end),
42
         {:ok, issue} <-
43
           lookup(fn ->
44
             Issues.get_issue_by_number!(repository, integer_param!(issue_number))
45
           end) do
46
      if Repositories.issue_participant?(repository, actor) do
47
        # Nothing between here and the write resolves a name, so this action has
48
        # only ever had one door. It is written this way anyway: the rescue that
49
        # was here covered the write too, and a lookup added inside it later
50
        # would have joined the repository's `404` without anyone deciding to.
51
        case Issues.create_comment(issue, params, actor) do
52
          {:ok, %Comment{} = comment} ->
53
            conn
54
            |> put_status(:created)
55
            |> render(:show, comment: comment)
56
57
          {:error, %Ecto.Changeset{} = changeset} ->
58
            ApiError.changeset(conn, changeset)
59
        end
60
      else
61
        participation_forbidden(conn, actor)
60 62
      end
61 63
    else
62
      participation_forbidden(conn, actor)
64
      {:error, :not_found} -> ApiError.not_found(conn)
63 65
    end
64
  rescue
65
    Ecto.NoResultsError -> ApiError.not_found(conn)
66 66
  end
67 67
68
  defp write_repository(owner, repo, %Agent{}), do: Repositories.get_public_by_path!(owner, repo)
69
70
  defp write_repository(owner, repo, actor),
71
    do: Repositories.get_writable_by_path!(owner, repo, actor)
72
68 73
  # The `error` key predates the envelope and a published agent client reads
69 74
  # it, so it rides beside the envelope rather than being replaced.
70 75
  defp participation_forbidden(conn, %Agent{}),

@@ -77,41 +82,53 @@ defmodule OpenAgentsWeb.CommentController do

77 82
    do: ApiError.forbidden(conn, legacy: %{"error" => "forbidden"})
78 83
79 84
  def show(conn, %{"owner" => owner, "repo" => repo, "id" => id}) do
80
    repository = Repositories.get_visible_by_path!(owner, repo, conn.assigns[:current_user])
81
    comment = Issues.get_comment!(repository, OpenAgentsWeb.ControllerHelpers.integer_param!(id))
85
    reader = conn.assigns[:current_user]
82 86
83
    render(conn, :show, comment: comment)
84
  rescue
85
    Ecto.NoResultsError -> ApiError.not_found(conn)
87
    with {:ok, repository} <-
88
           lookup(fn -> Repositories.get_visible_by_path!(owner, repo, reader) end),
89
         {:ok, comment} <-
90
           lookup(fn -> Issues.get_comment!(repository, integer_param!(id)) end) do
91
      render(conn, :show, comment: comment)
92
    else
93
      {:error, :not_found} -> ApiError.not_found(conn)
94
    end
86 95
  end
87 96
88 97
  def update(conn, %{"owner" => owner, "repo" => repo, "id" => id} = params) do
89
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
90
    comment = Issues.get_comment!(repository, OpenAgentsWeb.ControllerHelpers.integer_param!(id))
98
    user = conn.assigns.current_user
91 99
92
    case Issues.update_comment(comment, params) do
93
      {:ok, %Comment{} = comment} ->
94
        render(conn, :show, comment: comment)
100
    with {:ok, repository} <-
101
           lookup(fn -> Repositories.get_writable_by_path!(owner, repo, user) end),
102
         {:ok, comment} <-
103
           lookup(fn -> Issues.get_comment!(repository, integer_param!(id)) end) do
104
      case Issues.update_comment(comment, params) do
105
        {:ok, %Comment{} = comment} ->
106
          render(conn, :show, comment: comment)
95 107
96
      {:error, %Ecto.Changeset{} = changeset} ->
97
        ApiError.changeset(conn, changeset)
108
        {:error, %Ecto.Changeset{} = changeset} ->
109
          ApiError.changeset(conn, changeset)
110
      end
111
    else
112
      {:error, :not_found} -> ApiError.not_found(conn)
98 113
    end
99
  rescue
100
    Ecto.NoResultsError -> ApiError.not_found(conn)
101 114
  end
102 115
103 116
  def delete(conn, %{"owner" => owner, "repo" => repo, "id" => id}) do
104
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
105
    comment = Issues.get_comment!(repository, OpenAgentsWeb.ControllerHelpers.integer_param!(id))
106
107
    case Issues.delete_comment(comment) do
108
      {:ok, :ok} ->
109
        send_resp(conn, :no_content, "")
110
111
      {:error, _reason} ->
112
        ApiError.refuse(conn, "delete_failed", message: "Could not delete comment")
117
    user = conn.assigns.current_user
118
119
    with {:ok, repository} <-
120
           lookup(fn -> Repositories.get_writable_by_path!(owner, repo, user) end),
121
         {:ok, comment} <-
122
           lookup(fn -> Issues.get_comment!(repository, integer_param!(id)) end) do
123
      case Issues.delete_comment(comment) do
124
        {:ok, :ok} ->
125
          send_resp(conn, :no_content, "")
126
127
        {:error, _reason} ->
128
          ApiError.refuse(conn, "delete_failed", message: "Could not delete comment")
129
      end
130
    else
131
      {:error, :not_found} -> ApiError.not_found(conn)
113 132
    end
114
  rescue
115
    Ecto.NoResultsError -> ApiError.not_found(conn)
116 133
  end
117 134
end
lib/openagents_web/controllers/controller_helpers.ex modified +28

@@ -19,4 +19,32 @@ defmodule OpenAgentsWeb.ControllerHelpers do

19 19
  end
20 20
21 21
  def integer_param!(value) when is_integer(value), do: value
22
23
  @doc """
24
  Runs one lookup that may not resolve, and tags the outcome.
25
26
  Every bang lookup raises `Ecto.NoResultsError`, so a `rescue` wrapped around a
27
  whole controller action catches all of them at once: the repository the caller
28
  may not see, and, further in, a label a request body named. Two unrelated
29
  failures then leave by the same `404` — and because `404` is the answer the
30
  API gives deliberately for a repository a caller cannot see, the second
31
  failure is not just mislabelled, it is unreadable. A caller cannot tell a
32
  privacy decision from a typo.
33
34
  Wrapping one lookup keeps the rescue the width of the thing it was written
35
  for, so an `Ecto.NoResultsError` raised anywhere else keeps its own meaning
36
  rather than silently joining this one.
37
38
      with {:ok, repository} <- lookup(fn -> Repositories.get_visible_by_path!(owner, repo, reader) end) do
39
        ...
40
      else
41
        {:error, :not_found} -> ApiError.not_found(conn)
42
      end
43
  """
44
  @spec lookup((-> term())) :: {:ok, term()} | {:error, :not_found}
45
  def lookup(fun) when is_function(fun, 0) do
46
    {:ok, fun.()}
47
  rescue
48
    Ecto.NoResultsError -> {:error, :not_found}
49
  end
22 50
end
lib/openagents_web/controllers/issue_controller.ex modified +119 -79

@@ -6,18 +6,22 @@ defmodule OpenAgentsWeb.IssueController do

6 6
  alias OpenAgents.Issues.CompletionClaims
7 7
  alias OpenAgents.Issues.Evidence
8 8
  alias OpenAgents.Issues.Issue
9
  alias OpenAgents.Issues.UnknownReference
9 10
  alias OpenAgents.Agents.Agent
10 11
  alias OpenAgents.PullRequests
11 12
  alias OpenAgents.Repositories
12 13
  alias OpenAgentsWeb.ApiError
13 14
15
  import OpenAgentsWeb.ControllerHelpers, only: [integer_param!: 1, lookup: 1]
16
14 17
  def index(conn, %{"owner" => owner, "repo" => repo} = params) do
15 18
    reader = conn.assigns[:current_user]
16
    repository = Repositories.get_visible_by_path!(owner, repo, reader)
17 19
18
    with :ok <- validate_index_params(params),
19
         {issues, total} <-
20
           Issues.list_issues_page(repository, index_options(params, reader)) do
20
    with {:ok, repository} <-
21
           lookup(fn -> Repositories.get_visible_by_path!(owner, repo, reader) end),
22
         :ok <- validate_index_params(params) do
23
      {issues, total} = Issues.list_issues_page(repository, index_options(params, reader))
24
21 25
      conn
22 26
      |> put_extensions_header()
23 27
      |> render(:index,

@@ -37,11 +41,12 @@ defmodule OpenAgentsWeb.IssueController do

37 41
        }
38 42
      )
39 43
    else
44
      {:error, :not_found} ->
45
        not_found(conn)
46
40 47
      {:error, field, message} ->
41 48
        ApiError.validation_failed(conn, %{field => [message]})
42 49
    end
43
  rescue
44
    Ecto.NoResultsError -> not_found(conn)
45 50
  end
46 51
47 52
  @valid_states ~w(open closed all)

@@ -117,37 +122,67 @@ defmodule OpenAgentsWeb.IssueController do

117 122
  def create(conn, %{"owner" => owner, "repo" => repo} = params) do
118 123
    actor = conn.assigns[:current_agent] || conn.assigns[:current_user]
119 124
120
    repository =
121
      case actor do
122
        %Agent{} -> Repositories.get_public_by_path!(owner, repo)
123
        _ -> Repositories.get_writable_by_path!(owner, repo, actor)
124
      end
125
    case lookup(fn -> write_repository(owner, repo, actor) end) do
126
      {:error, :not_found} ->
127
        not_found(conn)
125 128
126
    if Repositories.issue_participant?(repository, actor) do
127
      case Issues.create_issue(repository, params, actor) do
128
        {:ok, %Issue{} = issue} ->
129
          conn
130
          |> put_status(:created)
131
          |> put_extensions_header()
132
          |> render(:show,
133
            issue: issue,
134
            owner: owner,
135
            repo: repo,
136
            dependencies: dependencies(issue),
137
            progress: progress(issue, actor),
138
            work: work(issue),
139
            evidence: evidence(issue),
140
            completion_claims: completion_claims(issue)
141
          )
129
      {:ok, repository} ->
130
        if Repositories.issue_participant?(repository, actor) do
131
          create_issue(conn, repository, params, actor, owner, repo)
132
        else
133
          participation_forbidden(conn, actor)
134
        end
135
    end
136
  end
142 137
143
        {:error, %Ecto.Changeset{} = changeset} ->
144
          ApiError.changeset(conn, changeset)
145
      end
146
    else
147
      participation_forbidden(conn, actor)
138
  # An agent reaches a public repository it is not a member of; a person needs
139
  # write access. Either way this is the repository lookup, and it is the only
140
  # thing `lookup/1` wraps: whatever it refuses is a `404` that discloses
141
  # nothing about whether the repository exists.
142
  defp write_repository(owner, repo, %Agent{}), do: Repositories.get_public_by_path!(owner, repo)
143
144
  defp write_repository(owner, repo, actor),
145
    do: Repositories.get_writable_by_path!(owner, repo, actor)
146
147
  defp create_issue(conn, repository, params, actor, owner, repo) do
148
    case write_issue(fn -> Issues.create_issue(repository, params, actor) end) do
149
      {:ok, %Issue{} = issue} ->
150
        conn
151
        |> put_status(:created)
152
        |> put_extensions_header()
153
        |> render(:show,
154
          issue: issue,
155
          owner: owner,
156
          repo: repo,
157
          dependencies: dependencies(issue),
158
          progress: progress(issue, actor),
159
          work: work(issue),
160
          evidence: evidence(issue),
161
          completion_claims: completion_claims(issue)
162
        )
163
164
      {:error, %Ecto.Changeset{} = changeset} ->
165
        ApiError.changeset(conn, changeset)
166
167
      {:error, %UnknownReference{} = unresolved} ->
168
        unknown_reference(conn, unresolved)
148 169
    end
170
  end
171
172
  # Writing an issue resolves a label, an assignee, and a milestone by name, and
173
  # a name this repository does not have is a rejected field rather than a
174
  # missing resource. It is caught here rather than beside the repository
175
  # lookup, so the two failures leave by different doors.
176
  defp write_issue(write) do
177
    write.()
149 178
  rescue
150
    Ecto.NoResultsError -> not_found(conn)
179
    error in UnknownReference -> {:error, error}
180
  end
181
182
  # GitHub names the offending field, and so does this: the envelope's `errors`
183
  # map carries the request-body key and the value that did not resolve.
184
  defp unknown_reference(conn, %UnknownReference{} = error) do
185
    ApiError.validation_failed(conn, %{error.field => [Exception.message(error)]})
151 186
  end
152 187
153 188
  # The `error` key predates the envelope and a published agent client reads

@@ -166,29 +201,30 @@ defmodule OpenAgentsWeb.IssueController do

166 201
        "repo" => repo,
167 202
        "issue_number" => issue_number
168 203
      }) do
169
    repository = Repositories.get_visible_by_path!(owner, repo, conn.assigns[:current_user])
204
    reader = conn.assigns[:current_user]
170 205
171
    issue =
172
      Issues.get_issue_by_number!(
173
        repository,
174
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
206
    with {:ok, repository} <-
207
           lookup(fn -> Repositories.get_visible_by_path!(owner, repo, reader) end),
208
         {:ok, issue} <-
209
           lookup(fn ->
210
             Issues.get_issue_by_number!(repository, integer_param!(issue_number))
211
           end) do
212
      conn
213
      |> put_extensions_header()
214
      |> render(:show,
215
        issue: issue,
216
        owner: owner,
217
        repo: repo,
218
        dependencies: dependencies(issue),
219
        progress: progress(issue, reader),
220
        pull_requests: PullRequests.markers_by_issue_id([issue]),
221
        work: work(issue),
222
        evidence: evidence(issue),
223
        completion_claims: completion_claims(issue)
175 224
      )
176
177
    conn
178
    |> put_extensions_header()
179
    |> render(:show,
180
      issue: issue,
181
      owner: owner,
182
      repo: repo,
183
      dependencies: dependencies(issue),
184
      progress: progress(issue, conn.assigns[:current_user]),
185
      pull_requests: PullRequests.markers_by_issue_id([issue]),
186
      work: work(issue),
187
      evidence: evidence(issue),
188
      completion_claims: completion_claims(issue)
189
    )
190
  rescue
191
    Ecto.NoResultsError -> not_found(conn)
225
    else
226
      {:error, :not_found} -> not_found(conn)
227
    end
192 228
  end
193 229
194 230
  def update(

@@ -199,34 +235,38 @@ defmodule OpenAgentsWeb.IssueController do

199 235
          "issue_number" => issue_number
200 236
        } = params
201 237
      ) do
202
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
203
204
    issue =
205
      Issues.get_issue_by_number!(
206
        repository,
207
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
208
      )
238
    user = conn.assigns.current_user
239
240
    with {:ok, repository} <-
241
           lookup(fn -> Repositories.get_writable_by_path!(owner, repo, user) end),
242
         {:ok, issue} <-
243
           lookup(fn ->
244
             Issues.get_issue_by_number!(repository, integer_param!(issue_number))
245
           end) do
246
      case write_issue(fn -> Issues.update_issue(issue, params, user) end) do
247
        {:ok, %Issue{} = issue} ->
248
          conn
249
          |> put_extensions_header()
250
          |> render(:show,
251
            issue: issue,
252
            owner: owner,
253
            repo: repo,
254
            dependencies: dependencies(issue),
255
            progress: progress(issue, user),
256
            work: work(issue),
257
            evidence: evidence(issue),
258
            completion_claims: completion_claims(issue)
259
          )
209 260
210
    case Issues.update_issue(issue, params, conn.assigns.current_user) do
211
      {:ok, %Issue{} = issue} ->
212
        conn
213
        |> put_extensions_header()
214
        |> render(:show,
215
          issue: issue,
216
          owner: owner,
217
          repo: repo,
218
          dependencies: dependencies(issue),
219
          progress: progress(issue, conn.assigns.current_user),
220
          work: work(issue),
221
          evidence: evidence(issue),
222
          completion_claims: completion_claims(issue)
223
        )
261
        {:error, %Ecto.Changeset{} = changeset} ->
262
          ApiError.changeset(conn, changeset)
224 263
225
      {:error, %Ecto.Changeset{} = changeset} ->
226
        ApiError.changeset(conn, changeset)
264
        {:error, %UnknownReference{} = unresolved} ->
265
          unknown_reference(conn, unresolved)
266
      end
267
    else
268
      {:error, :not_found} -> not_found(conn)
227 269
    end
228
  rescue
229
    Ecto.NoResultsError -> not_found(conn)
230 270
  end
231 271
232 272
  defp dependencies(%Issue{} = issue), do: Issues.dependency_graph([issue])
test/openagents/issues_test.exs modified +30 -6

@@ -4,6 +4,7 @@ defmodule OpenAgents.IssuesTest do

4 4
  alias OpenAgents.Issues
5 5
  alias OpenAgents.Issues.Comment
6 6
  alias OpenAgents.Issues.Issue
7
  alias OpenAgents.Issues.UnknownReference
7 8
8 9
  defmodule AnalyticsSink do
9 10
    def capture(event, distinct_id, properties) do

@@ -241,10 +242,19 @@ defmodule OpenAgents.IssuesTest do

241 242
             ]
242 243
    end
243 244
245
    # Not `Ecto.NoResultsError`: that is what resolving the repository raises,
246
    # and the API turns it into the `404` a repository the caller cannot see
247
    # answers with. A label the repository does not have is a rejected field,
248
    # and it says which field and which value.
244 249
    test "rejects a label outside the repository label set" do
245
      assert_raise Ecto.NoResultsError, fn ->
246
        Issues.create_issue(repository(), %{title: "labelled", labels: ["nope"]})
247
      end
250
      error =
251
        assert_raise UnknownReference, fn ->
252
          Issues.create_issue(repository(), %{title: "labelled", labels: ["nope"]})
253
        end
254
255
      assert error.field == :labels
256
      assert error.value == "nope"
257
      assert Exception.message(error) =~ "nope"
248 258
    end
249 259
250 260
    test "canonicalizes label maps from the repository row" do

@@ -301,9 +311,23 @@ defmodule OpenAgents.IssuesTest do

301 311
    end
302 312
303 313
    test "raises for an unknown milestone number" do
304
      assert_raise Ecto.NoResultsError, fn ->
305
        Issues.create_issue(repository(), %{title: "planned", milestone: 404})
306
      end
314
      error =
315
        assert_raise UnknownReference, fn ->
316
          Issues.create_issue(repository(), %{title: "planned", milestone: 404})
317
        end
318
319
      assert error.field == :milestone
320
      assert error.value == 404
321
    end
322
323
    test "raises for a login that is not assignable in this repository" do
324
      error =
325
        assert_raise UnknownReference, fn ->
326
          Issues.create_issue(repository(), %{title: "assigned", assignees: ["nobody"]})
327
        end
328
329
      assert error.field == :assignees
330
      assert error.value == "nobody"
307 331
    end
308 332
309 333
    test "accepts an explicit nil milestone" do
test/openagents/notifications_test.exs modified +6 -1

@@ -639,7 +639,12 @@ defmodule OpenAgents.NotificationsTest do

639 639
640 640
      {:ok, issue} = Issues.create_issue(repository, %{title: "internal"}, owner)
641 641
642
      assert_raise Ecto.NoResultsError, fn ->
642
      # `OpenAgents.Issues.UnknownReference` rather than `Ecto.NoResultsError`:
643
      # a login this repository cannot assign is a rejected field, and the API
644
      # answers `422` naming it rather than the `404` a repository the caller
645
      # cannot see answers with. The refusal is what matters here, and it still
646
      # happens before anything is announced.
647
      assert_raise OpenAgents.Issues.UnknownReference, fn ->
643 648
        Issues.update_issue(
644 649
          issue,
645 650
          %{"assignees" => [%{"login" => outsider.github_login}]},
test/openagents_web/controllers/issue_controller_test.exs modified +135

@@ -264,6 +264,141 @@ defmodule OpenAgentsWeb.IssueControllerTest do

264 264
    end
265 265
  end
266 266
267
  # A `404` here means one thing on purpose: this caller does not get to know
268
  # whether the repository exists. Anything else that answered `404` borrowed
269
  # that ambiguity and became unreadable — a caller could not tell a privacy
270
  # decision from a typo, so the honest error was unreachable. These tests hold
271
  # the two apart in both directions.
272
  describe "a name the repository does not have" do
273
    test "POST with an unknown label answers 422 and names the label", %{conn: conn} do
274
      conn =
275
        post(conn, ~p"/api/v3/repos/OpenAgentsInc/openagents.com/issues", %{
276
          title: "Labelled",
277
          labels: ["area:data-rights"]
278
        })
279
280
      body = json_response(conn, 422)
281
282
      assert body["code"] == "validation_failed"
283
      assert [message] = body["errors"]["labels"]
284
      assert message =~ "area:data-rights"
285
      refute Map.has_key?(body["errors"], "repository")
286
    end
287
288
    test "POST with an unknown assignee answers 422 and names the login", %{conn: conn} do
289
      conn =
290
        post(conn, ~p"/api/v3/repos/OpenAgentsInc/openagents.com/issues", %{
291
          title: "Assigned",
292
          assignees: ["not-a-member"]
293
        })
294
295
      body = json_response(conn, 422)
296
297
      assert body["code"] == "validation_failed"
298
      assert [message] = body["errors"]["assignees"]
299
      assert message =~ "not-a-member"
300
    end
301
302
    test "POST with an unknown milestone answers 422 and names the number", %{conn: conn} do
303
      conn =
304
        post(conn, ~p"/api/v3/repos/OpenAgentsInc/openagents.com/issues", %{
305
          title: "Planned",
306
          milestone: 4004
307
        })
308
309
      body = json_response(conn, 422)
310
311
      assert body["code"] == "validation_failed"
312
      assert [message] = body["errors"]["milestone"]
313
      assert message =~ "4004"
314
    end
315
316
    test "PATCH with an unknown label answers 422 too", %{conn: conn} do
317
      {:ok, issue} = Issues.create_issue(repository(), %{title: "Relabel me"})
318
319
      conn =
320
        patch(conn, ~p"/api/v3/repos/OpenAgentsInc/openagents.com/issues/#{issue.number}", %{
321
          labels: ["area:data-rights"]
322
        })
323
324
      body = json_response(conn, 422)
325
326
      assert body["code"] == "validation_failed"
327
      assert [message] = body["errors"]["labels"]
328
      assert message =~ "area:data-rights"
329
    end
330
331
    test "a repository the caller cannot see still answers 404 and discloses nothing" do
332
      private_repository = repository_fixture(%{visibility: "private"})
333
      conn = put_forge_api_token(build_conn(), "issue-create-nonmember")
334
335
      conn =
336
        post(
337
          conn,
338
          "/api/v3/repos/#{private_repository.owner}/#{private_repository.name}/issues",
339
          %{title: "Filed blind", labels: ["area:data-rights"]}
340
        )
341
342
      body = json_response(conn, 404)
343
344
      assert body["code"] == "not_found"
345
      assert body["message"] == "Not Found"
346
      assert body["errors"] == %{}
347
348
      # The label in the body is real to this request and absent from the
349
      # repository, and the answer says neither. Nothing here separates a
350
      # private repository from one that was never created.
351
      encoded = Jason.encode!(body)
352
      refute encoded =~ private_repository.name
353
      refute encoded =~ private_repository.owner
354
      refute encoded =~ "area:data-rights"
355
      refute encoded =~ "label"
356
    end
357
358
    # The width of a rescue is the thing to hold, not just today's outcome. The
359
    # rescue lives in `OpenAgentsWeb.ControllerHelpers.lookup/1` and wraps one
360
    # lookup; writing `rescue Ecto.NoResultsError` back into an action would
361
    # cover the write again, and the next bang lookup added inside it would
362
    # rejoin the repository's `404` without anyone deciding to. Neither of the
363
    # two controllers behind the `:agent_participation_api` issue and comment
364
    # writes names the exception at all.
365
    test "neither issue nor comment writes rescue Ecto.NoResultsError across an action" do
366
      for file <- [
367
            "lib/openagents_web/controllers/issue_controller.ex",
368
            "lib/openagents_web/controllers/comment_controller.ex"
369
          ] do
370
        refute File.read!(file) =~ "Ecto.NoResultsError",
371
               "#{file} rescues Ecto.NoResultsError itself. Wrap the one lookup in " <>
372
                 "OpenAgentsWeb.ControllerHelpers.lookup/1 instead, so a lookup added " <>
373
                 "later cannot leave by the repository's 404."
374
      end
375
    end
376
377
    test "the two answers are different responses, not one" do
378
      private_repository = repository_fixture(%{visibility: "private"})
379
380
      unknown_label =
381
        post(
382
          put_forge_api_token(build_conn(), "issue-create-typo", repository()),
383
          ~p"/api/v3/repos/OpenAgentsInc/openagents.com/issues",
384
          %{title: "Typo", labels: ["area:data-rights"]}
385
        )
386
387
      unreadable_repository =
388
        post(
389
          put_forge_api_token(build_conn(), "issue-create-blind"),
390
          "/api/v3/repos/#{private_repository.owner}/#{private_repository.name}/issues",
391
          %{title: "Blind", labels: ["area:data-rights"]}
392
        )
393
394
      assert unknown_label.status == 422
395
      assert unreadable_repository.status == 404
396
397
      assert json_response(unknown_label, 422)["code"] !=
398
               json_response(unreadable_repository, 404)["code"]
399
    end
400
  end
401
267 402
  describe "update" do
268 403
    test "PATCH /api/v3/repos/:owner/:repo/issues/:issue_number closes an issue", %{
269 404
      conn: conn

This page updates live while a promote is in flight · changelog