From 85cbf773f010b1bb2c77e51b1e994314bbf4f008 Mon Sep 17 00:00:00 2001 From: Ilja Date: Sun, 20 Mar 2022 13:32:12 +0100 Subject: [PATCH 01/11] update sweet_xml [Security] --- mix.exs | 2 +- mix.lock | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mix.exs b/mix.exs index db2f1f069..9b4a3e239 100644 --- a/mix.exs +++ b/mix.exs @@ -141,7 +141,7 @@ defp deps do {:mogrify, "~> 0.7.4"}, {:ex_aws, "~> 2.1.6"}, {:ex_aws_s3, "~> 2.0"}, - {:sweet_xml, "~> 0.6.6"}, + {:sweet_xml, "~> 0.7.2"}, {:earmark, "1.4.15"}, {:bbcode_pleroma, "~> 0.2.0"}, {:crypt, diff --git a/mix.lock b/mix.lock index 232649cd5..821c397b4 100644 --- a/mix.lock +++ b/mix.lock @@ -114,7 +114,7 @@ "remote_ip": {:git, "https://git.pleroma.social/pleroma/remote_ip.git", "b647d0deecaa3acb140854fe4bda5b7e1dc6d1c8", [ref: "b647d0deecaa3acb140854fe4bda5b7e1dc6d1c8"]}, "sleeplocks": {:hex, :sleeplocks, "1.1.1", "3d462a0639a6ef36cc75d6038b7393ae537ab394641beb59830a1b8271faeed3", [:rebar3], [], "hexpm", "84ee37aeff4d0d92b290fff986d6a95ac5eedf9b383fadfd1d88e9b84a1c02e1"}, "ssl_verify_fun": {:hex, :ssl_verify_fun, "1.1.6", "cf344f5692c82d2cd7554f5ec8fd961548d4fd09e7d22f5b62482e5aeaebd4b0", [:make, :mix, :rebar3], [], "hexpm", "bdb0d2471f453c88ff3908e7686f86f9be327d065cc1ec16fa4540197ea04680"}, - "sweet_xml": {:hex, :sweet_xml, "0.6.6", "fc3e91ec5dd7c787b6195757fbcf0abc670cee1e4172687b45183032221b66b8", [:mix], [], "hexpm", "2e1ec458f892ffa81f9f8386e3f35a1af6db7a7a37748a64478f13163a1f3573"}, + "sweet_xml": {:hex, :sweet_xml, "0.7.2", "4729f997286811fabdd8288f8474e0840a76573051062f066c4b597e76f14f9f", [:mix], [], "hexpm", "6894e68a120f454534d99045ea3325f7740ea71260bc315f82e29731d570a6e8"}, "swoosh": {:hex, :swoosh, "1.3.11", "34f79c57f19892b43bd2168de9ff5de478a721a26328ef59567aad4243e7a77b", [:mix], [{:cowboy, "~> 1.1 or ~> 2.4", [hex: :cowboy, repo: "hexpm", optional: true]}, {:finch, "~> 0.6", [hex: :finch, repo: "hexpm", optional: true]}, {:gen_smtp, "~> 0.13 or ~> 1.0", [hex: :gen_smtp, repo: "hexpm", optional: true]}, {:hackney, "~> 1.9", [hex: :hackney, repo: "hexpm", optional: true]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}, {:mail, "~> 0.2", [hex: :mail, repo: "hexpm", optional: true]}, {:mime, "~> 1.1", [hex: :mime, repo: "hexpm", optional: false]}, {:plug_cowboy, ">= 1.0.0", [hex: :plug_cowboy, repo: "hexpm", optional: true]}], "hexpm", "f1e2a048db454f9982b9cf840f75e7399dd48be31ecc2a7dc10012a803b913af"}, "syslog": {:hex, :syslog, "1.1.0", "6419a232bea84f07b56dc575225007ffe34d9fdc91abe6f1b2f254fd71d8efc2", [:rebar3], [], "hexpm", "4c6a41373c7e20587be33ef841d3de6f3beba08519809329ecc4d27b15b659e1"}, "telemetry": {:hex, :telemetry, "0.4.3", "a06428a514bdbc63293cd9a6263aad00ddeb66f608163bdec7c8995784080818", [:rebar3], [], "hexpm", "eb72b8365ffda5bed68a620d1da88525e326cb82a75ee61354fc24b844768041"}, From 4d482b765f8bebbad0d5e9e17fb923eb475313d6 Mon Sep 17 00:00:00 2001 From: Tusooa Zhu Date: Thu, 5 May 2022 18:39:34 -0400 Subject: [PATCH 02/11] Allow to skip cache in Cache plug Ref: fix-local-public --- lib/pleroma/web/plugs/cache.ex | 19 ++++++++++++------- test/pleroma/web/plugs/cache_test.exs | 18 ++++++++++++++++++ 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/lib/pleroma/web/plugs/cache.ex b/lib/pleroma/web/plugs/cache.ex index 111854859..e0467f107 100644 --- a/lib/pleroma/web/plugs/cache.ex +++ b/lib/pleroma/web/plugs/cache.ex @@ -98,14 +98,19 @@ defp cache_resp(conn, opts) do content_type = content_type(conn) conn = - unless opts[:tracking_fun] do - @cachex.put(:web_resp_cache, key, {content_type, body}, ttl: ttl) - conn - else - tracking_fun_data = Map.get(conn.assigns, :tracking_fun_data, nil) - @cachex.put(:web_resp_cache, key, {content_type, body, tracking_fun_data}, ttl: ttl) + cond do + Map.get(conn.assigns, :skip_cache, false) -> + conn - opts.tracking_fun.(conn, tracking_fun_data) + !opts[:tracking_fun] -> + @cachex.put(:web_resp_cache, key, {content_type, body}, ttl: ttl) + conn + + true -> + tracking_fun_data = Map.get(conn.assigns, :tracking_fun_data, nil) + @cachex.put(:web_resp_cache, key, {content_type, body, tracking_fun_data}, ttl: ttl) + + opts.tracking_fun.(conn, tracking_fun_data) end put_resp_header(conn, "x-cache", "MISS from Pleroma") diff --git a/test/pleroma/web/plugs/cache_test.exs b/test/pleroma/web/plugs/cache_test.exs index 0ceab6cab..4e729cafb 100644 --- a/test/pleroma/web/plugs/cache_test.exs +++ b/test/pleroma/web/plugs/cache_test.exs @@ -179,4 +179,22 @@ test "ignore non-successful responses" do |> send_resp(:im_a_teapot, "🥤") |> sent_resp() end + + test "ignores if skip_cache is assigned" do + assert @miss_resp == + conn(:get, "/") + |> assign(:skip_cache, true) + |> Cache.call(%{query_params: false, ttl: nil}) + |> put_resp_content_type("cofe/hot") + |> send_resp(:ok, "cofe") + |> sent_resp() + + assert @miss_resp == + conn(:get, "/") + |> assign(:skip_cache, true) + |> Cache.call(%{query_params: false, ttl: nil}) + |> put_resp_content_type("cofe/hot") + |> send_resp(:ok, "cofe") + |> sent_resp() + end end From fa3157df964d4f88d0fd1ce466a44333c8c7ef60 Mon Sep 17 00:00:00 2001 From: Tusooa Zhu Date: Thu, 5 May 2022 19:20:32 -0400 Subject: [PATCH 03/11] Skip cache when /objects or /activities is authenticated Ref: fix-local-public --- .../activity_pub/activity_pub_controller.ex | 11 +++++++++ lib/pleroma/web/plugs/cache.ex | 21 +++++++++------- .../activity_pub_controller_test.exs | 24 +++++++++++++++++++ 3 files changed, 47 insertions(+), 9 deletions(-) diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index 57ac40b42..d423b1139 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -84,6 +84,7 @@ def object(%{assigns: assigns} = conn, _) do user <- Map.get(assigns, :user, nil), {_, true} <- {:visible?, Visibility.visible_for_user?(object, user)} do conn + |> maybe_skip_cache(user) |> assign(:tracking_fun_data, object.id) |> set_cache_ttl_for(object) |> put_resp_content_type("application/activity+json") @@ -112,6 +113,7 @@ def activity(%{assigns: assigns} = conn, _) do user <- Map.get(assigns, :user, nil), {_, true} <- {:visible?, Visibility.visible_for_user?(activity, user)} do conn + |> maybe_skip_cache(user) |> maybe_set_tracking_data(activity) |> set_cache_ttl_for(activity) |> put_resp_content_type("application/activity+json") @@ -151,6 +153,15 @@ defp set_cache_ttl_for(conn, entity) do assign(conn, :cache_ttl, ttl) end + def maybe_skip_cache(conn, user) do + if user do + conn + |> assign(:skip_cache, true) + else + conn + end + end + # GET /relay/following def relay_following(conn, _params) do with %{halted: false} = conn <- FederatingPlug.call(conn, []) do diff --git a/lib/pleroma/web/plugs/cache.ex b/lib/pleroma/web/plugs/cache.ex index e0467f107..935b2d834 100644 --- a/lib/pleroma/web/plugs/cache.ex +++ b/lib/pleroma/web/plugs/cache.ex @@ -97,20 +97,23 @@ defp cache_resp(conn, opts) do key = cache_key(conn, opts) content_type = content_type(conn) + should_cache = not Map.get(conn.assigns, :skip_cache, false) + conn = - cond do - Map.get(conn.assigns, :skip_cache, false) -> - conn - - !opts[:tracking_fun] -> + unless opts[:tracking_fun] do + if should_cache do @cachex.put(:web_resp_cache, key, {content_type, body}, ttl: ttl) - conn + end - true -> - tracking_fun_data = Map.get(conn.assigns, :tracking_fun_data, nil) + conn + else + tracking_fun_data = Map.get(conn.assigns, :tracking_fun_data, nil) + + if should_cache do @cachex.put(:web_resp_cache, key, {content_type, body, tracking_fun_data}, ttl: ttl) + end - opts.tracking_fun.(conn, tracking_fun_data) + opts.tracking_fun.(conn, tracking_fun_data) end put_resp_header(conn, "x-cache", "MISS from Pleroma") diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index 50315e21f..511405624 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -291,6 +291,30 @@ test "it returns a json representation of the object with accept application/ld+ assert json_response(conn, 200) == ObjectView.render("object.json", %{object: note}) end + test "does not cache authenticated response", %{conn: conn} do + user = insert(:user) + reader = insert(:user) + + {:ok, post} = + CommonAPI.post(user, %{status: "test @#{reader.nickname}", visibility: "local"}) + + object = Object.normalize(post, fetch: false) + uuid = String.split(object.data["id"], "/") |> List.last() + + assert response = + conn + |> assign(:user, reader) + |> put_req_header("accept", "application/activity+json") + |> get("/objects/#{uuid}") + + json_response(response, 200) + + conn + |> put_req_header("accept", "application/activity+json") + |> get("/objects/#{uuid}") + |> json_response(404) + end + test "it returns 404 for non-public messages", %{conn: conn} do note = insert(:direct_note) uuid = String.split(note.data["id"], "/") |> List.last() From 57c486014c06715ff5cd5ad4361155d4a1776c23 Mon Sep 17 00:00:00 2001 From: "Haelwenn (lanodan) Monnier" Date: Fri, 6 May 2022 08:59:36 +0200 Subject: [PATCH 04/11] Release 2.4.3 --- CHANGELOG.md | 6 ++++++ mix.exs | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 88ad0ada9..95405bb60 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Removed +## 2.4.3 - 2022-05-06 + +### Security +- Private `/objects/` and `/activities/` leaking if cached by authenticated user +- SweetXML library DTD bomb + ## 2.4.2 - 2022-01-10 ### Fixed diff --git a/mix.exs b/mix.exs index 9b4a3e239..927f39975 100644 --- a/mix.exs +++ b/mix.exs @@ -4,7 +4,7 @@ defmodule Pleroma.Mixfile do def project do [ app: :pleroma, - version: version("2.4.2"), + version: version("2.4.3"), elixir: "~> 1.9", elixirc_paths: elixirc_paths(Mix.env()), compilers: [:phoenix, :gettext] ++ Mix.compilers(), From c62a4f1c173490ad64fdfbab0c005ca3523b6013 Mon Sep 17 00:00:00 2001 From: Tusooa Zhu Date: Fri, 19 Aug 2022 13:19:38 -0400 Subject: [PATCH 05/11] Disconnect streaming sessions when token is revoked --- .../web/mastodon_api/websocket_handler.ex | 8 ++- .../web/o_auth/token/strategy/revoke.ex | 1 + lib/pleroma/web/streamer.ex | 24 +++++++-- test/pleroma/web/streamer_test.exs | 54 +++++++++++++++++++ 4 files changed, 81 insertions(+), 6 deletions(-) diff --git a/lib/pleroma/web/mastodon_api/websocket_handler.ex b/lib/pleroma/web/mastodon_api/websocket_handler.ex index 0d1faffbd..ffbc2c4de 100644 --- a/lib/pleroma/web/mastodon_api/websocket_handler.ex +++ b/lib/pleroma/web/mastodon_api/websocket_handler.ex @@ -32,7 +32,7 @@ def init(%{qs: qs} = req, state) do req end - {:cowboy_websocket, req, %{user: user, topic: topic, count: 0, timer: nil}, + {:cowboy_websocket, req, %{user: user, topic: topic, oauth_token: oauth_token, count: 0, timer: nil}, %{idle_timeout: @timeout}} else {:error, :bad_topic} -> @@ -54,7 +54,7 @@ def websocket_init(state) do }, topic #{state.topic}" ) - Streamer.add_socket(state.topic, state.user) + Streamer.add_socket(state.topic, state.oauth_token) {:ok, %{state | timer: timer()}} end @@ -100,6 +100,10 @@ def websocket_info(:tick, state) do {:reply, :ping, %{state | timer: nil, count: 0}, :hibernate} end + def websocket_info(:close, state) do + {:stop, state} + end + # State can be `[]` only in case we terminate before switching to websocket, # we already log errors for these cases in `init/1`, so just do nothing here def terminate(_reason, _req, []), do: :ok diff --git a/lib/pleroma/web/o_auth/token/strategy/revoke.ex b/lib/pleroma/web/o_auth/token/strategy/revoke.ex index 8d6572704..03a0b91ae 100644 --- a/lib/pleroma/web/o_auth/token/strategy/revoke.ex +++ b/lib/pleroma/web/o_auth/token/strategy/revoke.ex @@ -22,5 +22,6 @@ def revoke(%App{} = app, %{"token" => token} = _attrs) do @spec revoke(Token.t()) :: {:ok, Token.t()} | {:error, Ecto.Changeset.t()} def revoke(%Token{} = token) do Repo.delete(token) + Pleroma.Web.Streamer.close_streams_by_oauth_token(token) end end diff --git a/lib/pleroma/web/streamer.ex b/lib/pleroma/web/streamer.ex index fc3bbb130..8bf70d99b 100644 --- a/lib/pleroma/web/streamer.ex +++ b/lib/pleroma/web/streamer.ex @@ -37,7 +37,7 @@ def registry, do: @registry {:ok, topic :: String.t()} | {:error, :bad_topic} | {:error, :unauthorized} def get_topic_and_add_socket(stream, user, oauth_token, params \\ %{}) do with {:ok, topic} <- get_topic(stream, user, oauth_token, params) do - add_socket(topic, user) + add_socket(topic, oauth_token) end end @@ -120,10 +120,10 @@ def get_topic(_stream, _user, _oauth_token, _params) do end @doc "Registers the process for streaming. Use `get_topic/3` to get the full authorized topic." - def add_socket(topic, user) do + def add_socket(topic, oauth_token) do if should_env_send?() do - auth? = if user, do: true - Registry.register(@registry, topic, auth?) + oauth_token_id = if oauth_token, do: oauth_token.id, else: false + Registry.register(@registry, topic, oauth_token_id) end {:ok, topic} @@ -320,6 +320,22 @@ defp thread_containment(activity, user) do end end + def close_streams_by_oauth_token(oauth_token) do + if should_env_send?() do + Registry.select( + @registry, + [ + { + {:"$1", :"$2", :"$3"}, + [{:==, :"$3", oauth_token.id}], + [:"$2"] + } + ] + ) + |> Enum.each(fn pid -> send(pid, :close) end) + end + end + # In test environement, only return true if the registry is started. # In benchmark environment, returns false. # In any other environment, always returns true. diff --git a/test/pleroma/web/streamer_test.exs b/test/pleroma/web/streamer_test.exs index b788a9138..5426467e5 100644 --- a/test/pleroma/web/streamer_test.exs +++ b/test/pleroma/web/streamer_test.exs @@ -813,4 +813,58 @@ test "it sends conversation update to the 'direct' stream when a message is dele assert last_status["id"] == to_string(create_activity.id) end end + + describe "stop streaming if token got revoked" do + test "do not revoke other tokens" do + %{user: user, token: token} = oauth_access(["read"]) + %{token: token2} = oauth_access(["read"], user: user) + %{user: user2, token: user2_token} = oauth_access(["read"]) + + post_user = insert(:user) + CommonAPI.follow(user, post_user) + CommonAPI.follow(user2, post_user) + + Streamer.get_topic_and_add_socket("user", user, token) + Streamer.get_topic_and_add_socket("user", user, token2) + Streamer.get_topic_and_add_socket("user", user2, user2_token) + + {:ok, _} = + CommonAPI.post(post_user, %{ + status: "hi" + }) + + assert_receive {:render_with_user, _, "update.json", _} + assert_receive {:render_with_user, _, "update.json", _} + assert_receive {:render_with_user, _, "update.json", _} + + Pleroma.Web.OAuth.Token.Strategy.Revoke.revoke(token) + + assert_receive :close + refute_receive :close + end + + test "revoke all streams for this token" do + %{user: user, token: token} = oauth_access(["read"]) + + post_user = insert(:user) + CommonAPI.follow(user, post_user) + + Streamer.get_topic_and_add_socket("user", user, token) + Streamer.get_topic_and_add_socket("user", user, token) + + {:ok, _} = + CommonAPI.post(post_user, %{ + status: "hi" + }) + + assert_receive {:render_with_user, _, "update.json", _} + assert_receive {:render_with_user, _, "update.json", _} + + Pleroma.Web.OAuth.Token.Strategy.Revoke.revoke(token) + + assert_receive :close + assert_receive :close + refute_receive :close + end + end end From eb42e90c4f9ca35a6dc0e84e6f87b6f4b680173c Mon Sep 17 00:00:00 2001 From: Tusooa Zhu Date: Fri, 19 Aug 2022 13:56:39 -0400 Subject: [PATCH 06/11] Use Websockex to replace websocket_client --- mix.exs | 2 +- mix.lock | 2 +- .../integration/mastodon_websocket_test.exs | 14 +++++----- test/support/websocket_client.ex | 28 +++++++++---------- 4 files changed, 22 insertions(+), 24 deletions(-) diff --git a/mix.exs b/mix.exs index 927f39975..46c9fcaa2 100644 --- a/mix.exs +++ b/mix.exs @@ -210,7 +210,7 @@ defp deps do {:excoveralls, "0.12.3", only: :test}, {:hackney, "~> 1.18.0", override: true}, {:mox, "~> 1.0", only: :test}, - {:websocket_client, git: "https://github.com/jeremyong/websocket_client.git", only: :test} + {:websockex, "~> 0.4.3", only: :test} ] ++ oauth_deps() end diff --git a/mix.lock b/mix.lock index 821c397b4..1fe713e8e 100644 --- a/mix.lock +++ b/mix.lock @@ -126,5 +126,5 @@ "unicode_util_compat": {:hex, :unicode_util_compat, "0.7.0", "bc84380c9ab48177092f43ac89e4dfa2c6d62b40b8bd132b1059ecc7232f9a78", [:rebar3], [], "hexpm", "25eee6d67df61960cf6a794239566599b09e17e668d3700247bc498638152521"}, "unsafe": {:hex, :unsafe, "1.0.1", "a27e1874f72ee49312e0a9ec2e0b27924214a05e3ddac90e91727bc76f8613d8", [:mix], [], "hexpm", "6c7729a2d214806450d29766abc2afaa7a2cbecf415be64f36a6691afebb50e5"}, "web_push_encryption": {:git, "https://github.com/lanodan/elixir-web-push-encryption.git", "026a043037a89db4da8f07560bc8f9c68bcf0cc0", [branch: "bugfix/otp-24"]}, - "websocket_client": {:git, "https://github.com/jeremyong/websocket_client.git", "9a6f65d05ebf2725d62fb19262b21f1805a59fbf", []}, + "websockex": {:hex, :websockex, "0.4.3", "92b7905769c79c6480c02daacaca2ddd49de936d912976a4d3c923723b647bf0", [:mix], [], "hexpm", "95f2e7072b85a3a4cc385602d42115b73ce0b74a9121d0d6dbbf557645ac53e4"}, } diff --git a/test/pleroma/integration/mastodon_websocket_test.exs b/test/pleroma/integration/mastodon_websocket_test.exs index 43ec57893..1e0319144 100644 --- a/test/pleroma/integration/mastodon_websocket_test.exs +++ b/test/pleroma/integration/mastodon_websocket_test.exs @@ -33,16 +33,16 @@ def start_socket(qs \\ nil, headers \\ []) do test "refuses invalid requests" do capture_log(fn -> - assert {:error, {404, _}} = start_socket() - assert {:error, {404, _}} = start_socket("?stream=ncjdk") + assert {:error, %WebSockex.RequestError{code: 404}} = start_socket() + assert {:error, %WebSockex.RequestError{code: 404}} = start_socket("?stream=ncjdk") Process.sleep(30) end) end test "requires authentication and a valid token for protected streams" do capture_log(fn -> - assert {:error, {401, _}} = start_socket("?stream=user&access_token=aaaaaaaaaaaa") - assert {:error, {401, _}} = start_socket("?stream=user") + assert {:error, %WebSockex.RequestError{code: 401}} = start_socket("?stream=user&access_token=aaaaaaaaaaaa") + assert {:error, %WebSockex.RequestError{code: 401}} = start_socket("?stream=user") Process.sleep(30) end) end @@ -102,7 +102,7 @@ test "accepts the 'user' stream", %{token: token} = _state do assert {:ok, _} = start_socket("?stream=user&access_token=#{token.token}") capture_log(fn -> - assert {:error, {401, _}} = start_socket("?stream=user") + assert {:error, %WebSockex.RequestError{code: 401}} = start_socket("?stream=user") Process.sleep(30) end) end @@ -111,7 +111,7 @@ test "accepts the 'user:notification' stream", %{token: token} = _state do assert {:ok, _} = start_socket("?stream=user:notification&access_token=#{token.token}") capture_log(fn -> - assert {:error, {401, _}} = start_socket("?stream=user:notification") + assert {:error, %WebSockex.RequestError{code: 401}} = start_socket("?stream=user:notification") Process.sleep(30) end) end @@ -120,7 +120,7 @@ test "accepts valid token on Sec-WebSocket-Protocol header", %{token: token} do assert {:ok, _} = start_socket("?stream=user", [{"Sec-WebSocket-Protocol", token.token}]) capture_log(fn -> - assert {:error, {401, _}} = + assert {:error, %WebSockex.RequestError{code: 401}} = start_socket("?stream=user", [{"Sec-WebSocket-Protocol", "I am a friend"}]) Process.sleep(30) diff --git a/test/support/websocket_client.ex b/test/support/websocket_client.ex index 34b955474..2660f6151 100644 --- a/test/support/websocket_client.ex +++ b/test/support/websocket_client.ex @@ -5,18 +5,17 @@ defmodule Pleroma.Integration.WebsocketClient do # https://github.com/phoenixframework/phoenix/blob/master/test/support/websocket_client.exs + use WebSockex + @doc """ Starts the WebSocket server for given ws URL. Received Socket.Message's are forwarded to the sender pid """ def start_link(sender, url, headers \\ []) do - :crypto.start() - :ssl.start() - - :websocket_client.start_link( - String.to_charlist(url), + WebSockex.start_link( + url, __MODULE__, - [sender], + %{ sender: sender }, extra_headers: headers ) end @@ -36,27 +35,26 @@ def send_text(server_pid, msg) do end @doc false - def init([sender], _conn_state) do - {:ok, %{sender: sender}} - end - - @doc false - def websocket_handle(frame, _conn_state, state) do + @impl true + def handle_frame(frame, state) do send(state.sender, frame) {:ok, state} end @doc false - def websocket_info({:text, msg}, _conn_state, state) do + @impl true + def handle_info({:text, msg}, state) do {:reply, {:text, msg}, state} end - def websocket_info(:close, _conn_state, _state) do + @impl true + def handle_info(:close, _state) do {:close, <<>>, "done"} end @doc false - def websocket_terminate(_reason, _conn_state, _state) do + @impl true + def terminate(_reason, _state) do :ok end end From 3522852c6196cafa63804240f52dd593e09ba694 Mon Sep 17 00:00:00 2001 From: Tusooa Zhu Date: Fri, 19 Aug 2022 14:09:42 -0400 Subject: [PATCH 07/11] Test that server will disconnect websocket upon token revocation --- .../integration/mastodon_websocket_test.exs | 18 +++++++++++++++++- test/support/websocket_client.ex | 6 ++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/test/pleroma/integration/mastodon_websocket_test.exs b/test/pleroma/integration/mastodon_websocket_test.exs index 1e0319144..adb2d7004 100644 --- a/test/pleroma/integration/mastodon_websocket_test.exs +++ b/test/pleroma/integration/mastodon_websocket_test.exs @@ -91,7 +91,7 @@ test "receives well formatted events" do {:ok, token} = OAuth.Token.exchange_token(app, auth) - %{user: user, token: token} + %{app: app, user: user, token: token} end test "accepts valid tokens", state do @@ -126,5 +126,21 @@ test "accepts valid token on Sec-WebSocket-Protocol header", %{token: token} do Process.sleep(30) end) end + + test "disconnect when token is revoked", %{app: app, user: user, token: token} do + assert {:ok, _} = start_socket("?stream=user:notification&access_token=#{token.token}") + assert {:ok, _} = start_socket("?stream=user&access_token=#{token.token}") + + {:ok, auth} = OAuth.Authorization.create_authorization(app, user) + + {:ok, token2} = OAuth.Token.exchange_token(app, auth) + assert {:ok, _} = start_socket("?stream=user&access_token=#{token2.token}") + + OAuth.Token.Strategy.Revoke.revoke(token) + + assert_receive {:close, _} + assert_receive {:close, _} + refute_receive {:close, _} + end end end diff --git a/test/support/websocket_client.ex b/test/support/websocket_client.ex index 2660f6151..abe7d5eda 100644 --- a/test/support/websocket_client.ex +++ b/test/support/websocket_client.ex @@ -41,6 +41,12 @@ def handle_frame(frame, state) do {:ok, state} end + @impl true + def handle_disconnect(conn_status, state) do + send(state.sender, {:close, conn_status}) + {:ok, state} + end + @doc false @impl true def handle_info({:text, msg}, state) do From f459c1260b43396fb7173e97e29ccef441a615ec Mon Sep 17 00:00:00 2001 From: Tusooa Zhu Date: Fri, 19 Aug 2022 14:10:07 -0400 Subject: [PATCH 08/11] Lint --- lib/pleroma/web/mastodon_api/websocket_handler.ex | 3 ++- test/pleroma/integration/mastodon_websocket_test.exs | 8 ++++++-- test/support/websocket_client.ex | 2 +- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/web/mastodon_api/websocket_handler.ex b/lib/pleroma/web/mastodon_api/websocket_handler.ex index ffbc2c4de..930e9eb29 100644 --- a/lib/pleroma/web/mastodon_api/websocket_handler.ex +++ b/lib/pleroma/web/mastodon_api/websocket_handler.ex @@ -32,7 +32,8 @@ def init(%{qs: qs} = req, state) do req end - {:cowboy_websocket, req, %{user: user, topic: topic, oauth_token: oauth_token, count: 0, timer: nil}, + {:cowboy_websocket, req, + %{user: user, topic: topic, oauth_token: oauth_token, count: 0, timer: nil}, %{idle_timeout: @timeout}} else {:error, :bad_topic} -> diff --git a/test/pleroma/integration/mastodon_websocket_test.exs b/test/pleroma/integration/mastodon_websocket_test.exs index adb2d7004..d44033842 100644 --- a/test/pleroma/integration/mastodon_websocket_test.exs +++ b/test/pleroma/integration/mastodon_websocket_test.exs @@ -41,7 +41,9 @@ test "refuses invalid requests" do test "requires authentication and a valid token for protected streams" do capture_log(fn -> - assert {:error, %WebSockex.RequestError{code: 401}} = start_socket("?stream=user&access_token=aaaaaaaaaaaa") + assert {:error, %WebSockex.RequestError{code: 401}} = + start_socket("?stream=user&access_token=aaaaaaaaaaaa") + assert {:error, %WebSockex.RequestError{code: 401}} = start_socket("?stream=user") Process.sleep(30) end) @@ -111,7 +113,9 @@ test "accepts the 'user:notification' stream", %{token: token} = _state do assert {:ok, _} = start_socket("?stream=user:notification&access_token=#{token.token}") capture_log(fn -> - assert {:error, %WebSockex.RequestError{code: 401}} = start_socket("?stream=user:notification") + assert {:error, %WebSockex.RequestError{code: 401}} = + start_socket("?stream=user:notification") + Process.sleep(30) end) end diff --git a/test/support/websocket_client.ex b/test/support/websocket_client.ex index abe7d5eda..70d331999 100644 --- a/test/support/websocket_client.ex +++ b/test/support/websocket_client.ex @@ -15,7 +15,7 @@ def start_link(sender, url, headers \\ []) do WebSockex.start_link( url, __MODULE__, - %{ sender: sender }, + %{sender: sender}, extra_headers: headers ) end From a31d6bb52c8856c71f20d49aec8948573dacba68 Mon Sep 17 00:00:00 2001 From: Tusooa Zhu Date: Fri, 19 Aug 2022 14:58:57 -0400 Subject: [PATCH 09/11] Execute session disconnect in background --- lib/pleroma/application.ex | 3 ++- lib/pleroma/web/o_auth/token/strategy/revoke.ex | 15 +++++++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/lib/pleroma/application.ex b/lib/pleroma/application.ex index 9824e0a4a..92d143665 100644 --- a/lib/pleroma/application.ex +++ b/lib/pleroma/application.ex @@ -89,7 +89,8 @@ def start(_type, _args) do Pleroma.Repo, Config.TransferTask, Pleroma.Emoji, - Pleroma.Web.Plugs.RateLimiter.Supervisor + Pleroma.Web.Plugs.RateLimiter.Supervisor, + {Task.Supervisor, name: Pleroma.TaskSupervisor} ] ++ cachex_children() ++ http_children(adapter, @mix_env) ++ diff --git a/lib/pleroma/web/o_auth/token/strategy/revoke.ex b/lib/pleroma/web/o_auth/token/strategy/revoke.ex index 03a0b91ae..de99bc137 100644 --- a/lib/pleroma/web/o_auth/token/strategy/revoke.ex +++ b/lib/pleroma/web/o_auth/token/strategy/revoke.ex @@ -21,7 +21,18 @@ def revoke(%App{} = app, %{"token" => token} = _attrs) do @doc "Revokes access token" @spec revoke(Token.t()) :: {:ok, Token.t()} | {:error, Ecto.Changeset.t()} def revoke(%Token{} = token) do - Repo.delete(token) - Pleroma.Web.Streamer.close_streams_by_oauth_token(token) + with {:ok, token} <- Repo.delete(token) do + Task.Supervisor.start_child( + Pleroma.TaskSupervisor, + Pleroma.Web.Streamer, + :close_streams_by_oauth_token, + [token], + restart: :transient + ) + + {:ok, token} + else + result -> result + end end end From 5a2c8ef4ccfbcc996fb812779730c78e2a3fbdcd Mon Sep 17 00:00:00 2001 From: Tusooa Zhu Date: Fri, 19 Aug 2022 19:58:16 -0400 Subject: [PATCH 10/11] Refactor streamer test --- test/pleroma/web/streamer_test.exs | 81 +++++++++++++++++++++++------- 1 file changed, 64 insertions(+), 17 deletions(-) diff --git a/test/pleroma/web/streamer_test.exs b/test/pleroma/web/streamer_test.exs index 5426467e5..7c4b9e288 100644 --- a/test/pleroma/web/streamer_test.exs +++ b/test/pleroma/web/streamer_test.exs @@ -815,7 +815,47 @@ test "it sends conversation update to the 'direct' stream when a message is dele end describe "stop streaming if token got revoked" do - test "do not revoke other tokens" do + setup do + child_proc = fn start, finalize -> + fn -> + start.() + + receive do + {StreamerTest, :ready} -> + assert_receive {:render_with_user, _, "update.json", _} + + receive do + {StreamerTest, :revoked} -> finalize.() + end + end + end + end + + starter = fn user, token -> + fn -> Streamer.get_topic_and_add_socket("user", user, token) end + end + + hit = fn -> assert_receive :close end + miss = fn -> refute_receive :close end + + send_all = fn tasks, thing -> Enum.each(tasks, &send(&1.pid, thing)) end + + %{ + child_proc: child_proc, + starter: starter, + hit: hit, + miss: miss, + send_all: send_all + } + end + + test "do not revoke other tokens", %{ + child_proc: child_proc, + starter: starter, + hit: hit, + miss: miss, + send_all: send_all + } do %{user: user, token: token} = oauth_access(["read"]) %{token: token2} = oauth_access(["read"], user: user) %{user: user2, token: user2_token} = oauth_access(["read"]) @@ -824,47 +864,54 @@ test "do not revoke other tokens" do CommonAPI.follow(user, post_user) CommonAPI.follow(user2, post_user) - Streamer.get_topic_and_add_socket("user", user, token) - Streamer.get_topic_and_add_socket("user", user, token2) - Streamer.get_topic_and_add_socket("user", user2, user2_token) + tasks = [ + Task.async(child_proc.(starter.(user, token), hit)), + Task.async(child_proc.(starter.(user, token2), miss)), + Task.async(child_proc.(starter.(user2, user2_token), miss)) + ] {:ok, _} = CommonAPI.post(post_user, %{ status: "hi" }) - assert_receive {:render_with_user, _, "update.json", _} - assert_receive {:render_with_user, _, "update.json", _} - assert_receive {:render_with_user, _, "update.json", _} + send_all.(tasks, {StreamerTest, :ready}) Pleroma.Web.OAuth.Token.Strategy.Revoke.revoke(token) - assert_receive :close - refute_receive :close + send_all.(tasks, {StreamerTest, :revoked}) + + Enum.each(tasks, &Task.await/1) end - test "revoke all streams for this token" do + test "revoke all streams for this token", %{ + child_proc: child_proc, + starter: starter, + hit: hit, + send_all: send_all + } do %{user: user, token: token} = oauth_access(["read"]) post_user = insert(:user) CommonAPI.follow(user, post_user) - Streamer.get_topic_and_add_socket("user", user, token) - Streamer.get_topic_and_add_socket("user", user, token) + tasks = [ + Task.async(child_proc.(starter.(user, token), hit)), + Task.async(child_proc.(starter.(user, token), hit)) + ] {:ok, _} = CommonAPI.post(post_user, %{ status: "hi" }) - assert_receive {:render_with_user, _, "update.json", _} - assert_receive {:render_with_user, _, "update.json", _} + send_all.(tasks, {StreamerTest, :ready}) Pleroma.Web.OAuth.Token.Strategy.Revoke.revoke(token) - assert_receive :close - assert_receive :close - refute_receive :close + send_all.(tasks, {StreamerTest, :revoked}) + + Enum.each(tasks, &Task.await/1) end end end From 31fd41de0cbca28cd2461e96384460596e54e9e9 Mon Sep 17 00:00:00 2001 From: Tusooa Zhu Date: Fri, 19 Aug 2022 20:29:06 -0400 Subject: [PATCH 11/11] Release 2.4.4 --- CHANGELOG.md | 5 +++++ mix.exs | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 95405bb60..bcbe3ba56 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). ### Removed +## 2.4.4 - 2022-08-19 + +### Security +- Streaming API sessions will now properly disconnect if the corresponding token is revoked + ## 2.4.3 - 2022-05-06 ### Security diff --git a/mix.exs b/mix.exs index 46c9fcaa2..0e2834fc6 100644 --- a/mix.exs +++ b/mix.exs @@ -4,7 +4,7 @@ defmodule Pleroma.Mixfile do def project do [ app: :pleroma, - version: version("2.4.3"), + version: version("2.4.4"), elixir: "~> 1.9", elixirc_paths: elixirc_paths(Mix.env()), compilers: [:phoenix, :gettext] ++ Mix.compilers(),