OpenAgents

Commit

Give every api v3 route one explicit authority classification

45f6fff3222c · AtlantisPleb · · parent f35c853b1c63 

Give every api v3 route one explicit authority classification

Route authority lived implicitly in which scope block a route happened to
sit in, so a new route could silently inherit whatever pipeline the block
above it used. ApiRouteAuthority now names every /api/v3 route with its
principal — anonymous, optional bearer, or required bearer — and a test
compares that inventory against the router both ways: an unclassified or
removed route fails CI.

A second test dispatches an anonymous request at every classified route and
asserts the enforcing plug answers as the classification promises:
required-bearer routes refuse with 401 before any controller code runs,
while anonymous and optional-bearer routes never do.

The sweep surfaced real 500s: five controllers called String.to_integer on
path segments and crashed with ArgumentError on malformed identifiers. A
shared integer_param! helper routes those through each controller's existing
NoResultsError rescue into stable 404s.

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
12 modules in 14.3 s
deployed
live · 12 modules on 3 nodes · push→live —
deployed
needs_rolling_replace · 12 modules on 0 nodes · push→live —

Changed files

  • added lib/openagents_web/api_route_authority.ex
  • modified lib/openagents_web/controllers/comment_controller.ex
  • added lib/openagents_web/controllers/controller_helpers.ex
  • modified lib/openagents_web/controllers/issue_assignee_controller.ex
  • modified lib/openagents_web/controllers/issue_controller.ex
  • modified lib/openagents_web/controllers/issue_label_controller.ex
  • modified lib/openagents_web/controllers/milestone_controller.ex
  • added test/openagents_web/api_route_authority_test.exs

Diff

8 files changed, +287 -16

lib/openagents_web/api_route_authority.ex added +98

@@ -0,0 +1,98 @@

1 
defmodule OpenAgentsWeb.ApiRouteAuthority do
2 
  @moduledoc """
3 
  The single authority inventory for every `/api/v3` route.
4
5 
  Each entry names one route and the principal that reaches it. The
6 
  `OpenAgentsWeb.ApiRouteAuthorityTest` proves two ways that this inventory and
7 
  the router agree: every live `/api/v3` route appears here exactly once, so a
8 
  new unclassified route fails CI, and each classification matches what the
9 
  enforcing pipeline actually does to an anonymous request.
10
11 
  Principals:
12
13 
  - `:anonymous` — no credential is read; anyone may call.
14 
  - `:optional_bearer` — an anonymous caller may read public repositories, and
15 
    a scoped bearer token widens visibility to private repositories the token's
16 
    user can read.
17 
  - `:required_bearer` — a scoped bearer token is mandatory; anonymous calls
18 
    are refused before the controller runs.
19 
  """
20
21 
  principals = [:anonymous, :optional_bearer, :required_bearer]
22
23 
  @type principal :: unquote(Enum.reduce(principals, &{:|, [], [&1, &2]}))
24
25 
  @doc "The authority classification for one `METHOD /path` API v3 route."
26 
  @spec authority(String.t(), String.t()) :: principal() | nil
27 
  def authority(verb, path) do
28 
    Map.get(inventory(), "#{verb} #{path}")
29 
  end
30
31 
  @doc "Every classified route as `{verb, path}` pairs."
32 
  @spec routes() :: [{String.t(), String.t()}]
33 
  def routes do
34 
    Enum.map(inventory(), fn {key, _principal} ->
35 
      key |> String.split(" ", parts: 2) |> List.to_tuple()
36 
    end)
37 
  end
38
39 
  defp inventory do
40 
    %{
41 
      # pipe_through :api — anonymous reads over public repositories.
42 
      "get /api/v3/repos/:owner/:repo/issues/:issue_number/comments" => :anonymous,
43 
      "get /api/v3/repos/:owner/:repo/issues/comments/:id" => :anonymous,
44 
      "get /api/v3/repos/:owner/:repo/issues/:issue_number/labels" => :anonymous,
45 
      "get /api/v3/repos/:owner/:repo/issues/:issue_number/assignees" => :anonymous,
46 
      "get /api/v3/repos/:owner/:repo/labels" => :anonymous,
47 
      "get /api/v3/repos/:owner/:repo/labels/:name" => :anonymous,
48 
      "get /api/v3/repos/:owner/:repo/milestones" => :anonymous,
49 
      "get /api/v3/repos/:owner/:repo/milestones/:milestone_number" => :anonymous,
50 
      "get /api/v3/repos/:owner/:repo/assignees" => :anonymous,
51 
      "get /api/v3/repos/:owner/:repo/assignees/:assignee" => :anonymous,
52 
      # Anonymous by design: device authorization bootstraps credentials.
53 
      "post /api/v3/device/authorizations" => :anonymous,
54 
      "post /api/v3/device/authorizations/token" => :anonymous,
55 
      # pipe_through :optional_forge_api — public reads, bearer-widened.
56 
      "get /api/v3/repos/:owner/:repo" => :optional_bearer,
57 
      "get /api/v3/repos/:owner/:repo/issues" => :optional_bearer,
58 
      "get /api/v3/repos/:owner/:repo/issues/:issue_number" => :optional_bearer,
59 
      "get /api/v3/repos/:owner/:repo/projectsV2" => :optional_bearer,
60 
      "get /api/v3/repos/:owner/:repo/projectsV2/:project_number" => :optional_bearer,
61 
      "get /api/v3/repos/:owner/:repo/projectsV2/:project_number/items" => :optional_bearer,
62 
      "get /api/v3/repos/:owner/:repo/projectsV2/:project_number/fields" => :optional_bearer,
63 
      # pipe_through :forge_write_api — scoped bearer required.
64 
      "delete /api/v3/repos/:owner/:repo" => :required_bearer,
65 
      "delete /api/v3/repos/:owner/:repo/issues/:issue_number/assignees" => :required_bearer,
66 
      "delete /api/v3/repos/:owner/:repo/issues/:issue_number/labels/:name" => :required_bearer,
67 
      "delete /api/v3/repos/:owner/:repo/issues/comments/:id" => :required_bearer,
68 
      "delete /api/v3/repos/:owner/:repo/labels/:name" => :required_bearer,
69 
      "delete /api/v3/repos/:owner/:repo/milestones/:milestone_number" => :required_bearer,
70 
      "get /api/v3/repository-imports/:id" => :required_bearer,
71 
      "get /api/v3/user" => :required_bearer,
72 
      "get /api/v3/user/repos" => :required_bearer,
73 
      "patch /api/v3/repos/:owner/:repo/issues/:issue_number" => :required_bearer,
74 
      "patch /api/v3/repos/:owner/:repo/issues/comments/:id" => :required_bearer,
75 
      "patch /api/v3/repos/:owner/:repo/labels/:name" => :required_bearer,
76 
      "patch /api/v3/repos/:owner/:repo/milestones/:milestone_number" => :required_bearer,
77 
      "patch /api/v3/repos/:owner/:repo/projectsV2/:project_number/items/:item_id" =>
78 
        :required_bearer,
79 
      "post /api/v3/orgs/:org/repos" => :required_bearer,
80 
      "post /api/v3/orgs/:org/repos/imports" => :required_bearer,
81 
      "post /api/v3/repos/:owner/:repo/issues" => :required_bearer,
82 
      "post /api/v3/repos/:owner/:repo/issues/:issue_number/assignees" => :required_bearer,
83 
      "post /api/v3/repos/:owner/:repo/issues/:issue_number/comments" => :required_bearer,
84 
      "post /api/v3/repos/:owner/:repo/issues/:issue_number/labels" => :required_bearer,
85 
      "post /api/v3/repos/:owner/:repo/labels" => :required_bearer,
86 
      "post /api/v3/repos/:owner/:repo/milestones" => :required_bearer,
87 
      "post /api/v3/repos/:owner/:repo/projectsV2" => :required_bearer,
88 
      "post /api/v3/repos/:owner/:repo/projectsV2/:project_number/items" => :required_bearer,
89 
      "post /api/v3/repos/:owner/:repo/projectsV2/:project_number/fields" => :required_bearer,
90 
      "post /api/v3/user/repos" => :required_bearer,
91 
      "post /api/v3/user/repos/imports" => :required_bearer,
92 
      "put /api/v3/repos/:owner/:repo/issues/:issue_number" => :required_bearer,
93 
      "put /api/v3/repos/:owner/:repo/issues/comments/:id" => :required_bearer,
94 
      "put /api/v3/repos/:owner/:repo/labels/:name" => :required_bearer,
95 
      "put /api/v3/repos/:owner/:repo/milestones/:milestone_number" => :required_bearer
96 
    }
97 
  end
98 
end
lib/openagents_web/controllers/comment_controller.ex modified +18 -5

@@ -10,7 +10,13 @@ defmodule OpenAgentsWeb.CommentController do

10 10 
        "repo" => repo,
11 11 
        "issue_number" => issue_number
12 12 
      }) do
13 
    issue = Issues.get_issue_by_path!(owner, repo, String.to_integer(issue_number))
13 
    issue =
14 
      Issues.get_issue_by_path!(
15 
        owner,
16 
        repo,
17 
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
18 
      )
19
14 20 
    comments = Issues.list_comments(issue)
15 21 
    render(conn, :index, comments: comments)
16 22 
  rescue

@@ -29,7 +35,12 @@ defmodule OpenAgentsWeb.CommentController do

29 35 
        } = params
30 36 
      ) do
31 37 
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
32 
    issue = Issues.get_issue_by_number!(repository, String.to_integer(issue_number))
38
39 
    issue =
40 
      Issues.get_issue_by_number!(
41 
        repository,
42 
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
43 
      )
33 44
34 45 
    case Issues.create_comment(issue, params, conn.assigns.current_user) do
35 46 
      {:ok, %Comment{} = comment} ->

@@ -50,7 +61,9 @@ defmodule OpenAgentsWeb.CommentController do

50 61 
  end
51 62
52 63 
  def show(conn, %{"owner" => owner, "repo" => repo, "id" => id}) do
53 
    comment = Issues.get_comment_by_path!(owner, repo, String.to_integer(id))
64 
    comment =
65 
      Issues.get_comment_by_path!(owner, repo, OpenAgentsWeb.ControllerHelpers.integer_param!(id))
66
54 67 
    render(conn, :show, comment: comment)
55 68 
  rescue
56 69 
    Ecto.NoResultsError ->

@@ -61,7 +74,7 @@ defmodule OpenAgentsWeb.CommentController do

61 74
62 75 
  def update(conn, %{"owner" => owner, "repo" => repo, "id" => id} = params) do
63 76 
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
64 
    comment = Issues.get_comment!(repository, String.to_integer(id))
77 
    comment = Issues.get_comment!(repository, OpenAgentsWeb.ControllerHelpers.integer_param!(id))
65 78
66 79 
    case Issues.update_comment(comment, params) do
67 80 
      {:ok, %Comment{} = comment} ->

@@ -81,7 +94,7 @@ defmodule OpenAgentsWeb.CommentController do

81 94
82 95 
  def delete(conn, %{"owner" => owner, "repo" => repo, "id" => id}) do
83 96 
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
84 
    comment = Issues.get_comment!(repository, String.to_integer(id))
97 
    comment = Issues.get_comment!(repository, OpenAgentsWeb.ControllerHelpers.integer_param!(id))
85 98
86 99 
    case Issues.delete_comment(comment) do
87 100 
      {:ok, :ok} ->
lib/openagents_web/controllers/controller_helpers.ex added +22

@@ -0,0 +1,22 @@

1 
defmodule OpenAgentsWeb.ControllerHelpers do
2 
  @moduledoc """
3 
  Shared helpers for the JSON API controllers.
4 
  """
5
6 
  @doc """
7 
  Parses a numeric path segment, treating a malformed value as a missing row.
8
9 
  Controllers already rescue `Ecto.NoResultsError` into a stable 404, so a
10 
  non-integer identifier takes the same path instead of crashing with an
11 
  `ArgumentError`.
12 
  """
13 
  @spec integer_param!(term()) :: integer()
14 
  def integer_param!(value) when is_binary(value) do
15 
    case Integer.parse(value) do
16 
      {number, ""} -> number
17 
      _malformed -> raise Ecto.NoResultsError, queryable: "parameter"
18 
    end
19 
  end
20
21 
  def integer_param!(value) when is_integer(value), do: value
22 
end
lib/openagents_web/controllers/issue_assignee_controller.ex modified +21 -3

@@ -5,7 +5,13 @@ defmodule OpenAgentsWeb.IssueAssigneeController do

5 5 
  alias OpenAgents.Repositories
6 6
7 7 
  def index(conn, %{"owner" => owner, "repo" => repo, "issue_number" => issue_number}) do
8 
    issue = Issues.get_issue_by_path!(owner, repo, String.to_integer(issue_number))
8 
    issue =
9 
      Issues.get_issue_by_path!(
10 
        owner,
11 
        repo,
12 
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
13 
      )
14
9 15 
    json(conn, %{assignees: issue.assignees || []})
10 16 
  rescue
11 17 
    Ecto.NoResultsError ->

@@ -23,7 +29,13 @@ defmodule OpenAgentsWeb.IssueAssigneeController do

23 29 
        } = params
24 30 
      ) do
25 31 
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
26 
    issue = Issues.get_issue_by_number!(repository, String.to_integer(issue_number))
32
33 
    issue =
34 
      Issues.get_issue_by_number!(
35 
        repository,
36 
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
37 
      )
38
27 39 
    logins = params["assignees"] || []
28 40
29 41 
    case Issues.add_assignees(issue, logins) do

@@ -51,7 +63,13 @@ defmodule OpenAgentsWeb.IssueAssigneeController do

51 63 
        } = params
52 64 
      ) do
53 65 
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
54 
    issue = Issues.get_issue_by_number!(repository, String.to_integer(issue_number))
66
67 
    issue =
68 
      Issues.get_issue_by_number!(
69 
        repository,
70 
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
71 
      )
72
55 73 
    logins = params["assignees"] || []
56 74
57 75 
    case Issues.remove_assignees(issue, logins) do
lib/openagents_web/controllers/issue_controller.ex modified +13 -2

@@ -94,7 +94,13 @@ defmodule OpenAgentsWeb.IssueController do

94 94 
        "issue_number" => issue_number
95 95 
      }) do
96 96 
    repository = Repositories.get_visible_by_path!(owner, repo, conn.assigns[:current_user])
97 
    issue = Issues.get_issue_by_number!(repository, String.to_integer(issue_number))
97
98 
    issue =
99 
      Issues.get_issue_by_number!(
100 
        repository,
101 
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
102 
      )
103
98 104 
    render(conn, :show, issue: issue, owner: owner, repo: repo)
99 105 
  rescue
100 106 
    Ecto.NoResultsError ->

@@ -112,7 +118,12 @@ defmodule OpenAgentsWeb.IssueController do

112 118 
        } = params
113 119 
      ) do
114 120 
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
115 
    issue = Issues.get_issue_by_number!(repository, String.to_integer(issue_number))
121
122 
    issue =
123 
      Issues.get_issue_by_number!(
124 
        repository,
125 
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
126 
      )
116 127
117 128 
    case Issues.update_issue(issue, params, conn.assigns.current_user) do
118 129 
      {:ok, %Issue{} = issue} ->
lib/openagents_web/controllers/issue_label_controller.ex modified +21 -3

@@ -5,7 +5,13 @@ defmodule OpenAgentsWeb.IssueLabelController do

5 5 
  alias OpenAgents.Repositories
6 6
7 7 
  def index(conn, %{"owner" => owner, "repo" => repo, "issue_number" => issue_number}) do
8 
    issue = Issues.get_issue_by_path!(owner, repo, String.to_integer(issue_number))
8 
    issue =
9 
      Issues.get_issue_by_path!(
10 
        owner,
11 
        repo,
12 
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
13 
      )
14
9 15 
    json(conn, %{labels: issue.labels || []})
10 16 
  rescue
11 17 
    Ecto.NoResultsError ->

@@ -23,7 +29,13 @@ defmodule OpenAgentsWeb.IssueLabelController do

23 29 
        } = params
24 30 
      ) do
25 31 
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
26 
    issue = Issues.get_issue_by_number!(repository, String.to_integer(issue_number))
32
33 
    issue =
34 
      Issues.get_issue_by_number!(
35 
        repository,
36 
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
37 
      )
38
27 39 
    names = params["labels"] || []
28 40
29 41 
    case Issues.add_labels(issue, names) do

@@ -49,7 +61,13 @@ defmodule OpenAgentsWeb.IssueLabelController do

49 61 
        "name" => name
50 62 
      }) do
51 63 
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
52 
    issue = Issues.get_issue_by_number!(repository, String.to_integer(issue_number))
64
65 
    issue =
66 
      Issues.get_issue_by_number!(
67 
        repository,
68 
        OpenAgentsWeb.ControllerHelpers.integer_param!(issue_number)
69 
      )
70
53 71 
    decoded = URI.decode(name)
54 72
55 73 
    if Enum.any?(issue.labels || [], &(&1["name"] == decoded)) do
lib/openagents_web/controllers/milestone_controller.ex modified +13 -3

@@ -37,7 +37,11 @@ defmodule OpenAgentsWeb.MilestoneController do

37 37 
        "milestone_number" => milestone_number
38 38 
      }) do
39 39 
    milestone =
40 
      Milestones.get_milestone_by_path!(owner, repo, String.to_integer(milestone_number))
40 
      Milestones.get_milestone_by_path!(
41 
        owner,
42 
        repo,
43 
        OpenAgentsWeb.ControllerHelpers.integer_param!(milestone_number)
44 
      )
41 45
42 46 
    render(conn, :show, milestone: milestone, owner: owner, repo: repo)
43 47 
  rescue

@@ -58,7 +62,10 @@ defmodule OpenAgentsWeb.MilestoneController do

58 62 
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
59 63
60 64 
    milestone =
61 
      Milestones.get_milestone_by_number!(repository, String.to_integer(milestone_number))
65 
      Milestones.get_milestone_by_number!(
66 
        repository,
67 
        OpenAgentsWeb.ControllerHelpers.integer_param!(milestone_number)
68 
      )
62 69
63 70 
    case Milestones.update_milestone(milestone, params) do
64 71 
      {:ok, %Milestone{} = milestone} ->

@@ -84,7 +91,10 @@ defmodule OpenAgentsWeb.MilestoneController do

84 91 
    repository = Repositories.get_writable_by_path!(owner, repo, conn.assigns.current_user)
85 92
86 93 
    milestone =
87 
      Milestones.get_milestone_by_number!(repository, String.to_integer(milestone_number))
94 
      Milestones.get_milestone_by_number!(
95 
        repository,
96 
        OpenAgentsWeb.ControllerHelpers.integer_param!(milestone_number)
97 
      )
88 98
89 99 
    case Milestones.delete_milestone(milestone) do
90 100 
      {:ok, %Milestone{}} ->
test/openagents_web/api_route_authority_test.exs added +81

@@ -0,0 +1,81 @@

1 
defmodule OpenAgentsWeb.ApiRouteAuthorityTest do
2 
  use OpenAgentsWeb.ConnCase
3
4 
  import Phoenix.ConnTest
5
6 
  @endpoint OpenAgentsWeb.Endpoint
7
8 
  # Every live /api/v3 route must be classified exactly once. A route added to
9 
  # the router without an entry in ApiRouteAuthority fails here, so a new
10 
  # surface cannot inherit authority from a broad pipeline default.
11 
  test "the inventory covers every api v3 route and nothing else" do
12 
    live =
13 
      OpenAgentsWeb.Router.__routes__()
14 
      |> Enum.filter(&String.starts_with?(&1.path, "/api/v3"))
15 
      |> Enum.map(&{Atom.to_string(&1.verb), &1.path})
16 
      |> MapSet.new()
17
18 
    classified =
19 
      OpenAgentsWeb.ApiRouteAuthority.routes()
20 
      |> MapSet.new()
21
22 
    unclassified = MapSet.difference(live, classified) |> MapSet.to_list()
23 
    stale = MapSet.difference(classified, live) |> MapSet.to_list()
24
25 
    assert MapSet.equal?(live, classified), """
26 
    The route authority inventory disagrees with the router.
27
28 
    Routes with no classification (add them to ApiRouteAuthority):
29
30 
    #{unclassified |> Enum.map(fn {v, p} -> "  #{v} #{p}" end) |> Enum.join("\n")}
31
32 
    Classified routes that no longer exist (remove them):
33
34 
    #{stale |> Enum.map(fn {v, p} -> "  #{v} #{p}" end) |> Enum.join("\n")}
35 
    """
36 
  end
37
38 
  # Each classification must match what the enforcing pipeline does to an
39 
  # anonymous request. Requests name repositories that do not exist, so
40 
  # authorized surfaces answer 404 and only authority failures can produce
41 
  # 401.
42 
  test "classifications match runtime enforcement" do
43 
    for {verb, path} <- OpenAgentsWeb.ApiRouteAuthority.routes() do
44 
      principal = OpenAgentsWeb.ApiRouteAuthority.authority(verb, path)
45 
      status = dispatch_status(verb, path)
46
47 
      case principal do
48 
        :anonymous ->
49 
          refute status == 401,
50 
                 "#{verb} #{path} is classified :anonymous but an anonymous call got #{status}"
51
52 
        :optional_bearer ->
53 
          refute status == 401,
54 
                 "#{verb} #{path} is classified :optional_bearer but an anonymous call got #{status}"
55
56 
        :required_bearer ->
57 
          assert status == 401,
58 
                 "#{verb} #{path} is classified :required_bearer but an anonymous call got #{status}"
59 
      end
60 
    end
61 
  end
62
63 
  defp dispatch_status(verb, path) do
64 
    path =
65 
      path
66 
      |> String.replace(":owner", "nobody")
67 
      |> String.replace(":repo", "nonexistent")
68 
      |> String.replace(":org", "nobody")
69 
      |> String.replace(":issue_number", "1")
70 
      |> String.replace(":milestone_number", "1")
71 
      |> String.replace(":project_number", "1")
72 
      |> String.replace(":item_id", "00000000-0000-4000-8000-000000000001")
73 
      |> String.replace(":id", "00000000-0000-4000-8000-000000000001")
74 
      |> String.replace(":name", "bug")
75 
      |> String.replace(":assignee", "someone")
76
77 
    build_conn()
78 
    |> dispatch(@endpoint, String.to_atom(String.downcase(verb)), path)
79 
    |> Map.get(:status)
80 
  end
81 
end

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