Explicitly allow imported file entries pointing to file system (#2083)

This commit is contained in:
Jonatan Kłosko 2023-07-18 02:00:11 +02:00 committed by GitHub
parent 82909f7f35
commit 5ff5e0939d
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
15 changed files with 465 additions and 29 deletions

View file

@ -376,7 +376,21 @@ defmodule Livebook.LiveMarkdown.Export do
defp notebook_stamp_metadata(notebook) do
keys = [:hub_secret_names]
put_unless_default(%{}, Map.take(notebook, keys), Map.take(Notebook.new(), keys))
metadata = put_unless_default(%{}, Map.take(notebook, keys), Map.take(Notebook.new(), keys))
# If there are any :file file entries, we want to generate a stamp
# to make sure the entries are not tampered with. We also want to
# store the information about file entries already in quarantine
if Enum.any?(notebook.file_entries, &(&1.type == :file)) do
Map.put(
metadata,
:quarantine_file_entry_names,
MapSet.to_list(notebook.quarantine_file_entry_names)
)
else
metadata
end
end
defp ensure_order(%{} = map) do

View file

@ -428,8 +428,17 @@ defmodule Livebook.LiveMarkdown.Import do
end
end
{Map.put(attrs, :file_entries, file_entries), stamp_hub_id,
messages ++ file_entry_messages}
# By default we put all :file entries in quarantine, if there
# is a valid stamp, we override this later
quarantine_file_entry_names =
for entry <- file_entries, entry.type == :file, into: MapSet.new(), do: entry.name
attrs =
attrs
|> Map.put(:file_entries, file_entries)
|> Map.put(:quarantine_file_entry_names, quarantine_file_entry_names)
{attrs, stamp_hub_id, messages ++ file_entry_messages}
_entry, {attrs, stamp_hub_id, messages} ->
{attrs, stamp_hub_id, messages}
@ -636,6 +645,9 @@ defmodule Livebook.LiveMarkdown.Import do
{:hub_secret_names, hub_secret_names}, notebook ->
%{notebook | hub_secret_names: hub_secret_names}
{:quarantine_file_entry_names, quarantine_file_entry_names}, notebook ->
%{notebook | quarantine_file_entry_names: MapSet.new(quarantine_file_entry_names)}
_entry, notebook ->
notebook
end)

View file

@ -27,6 +27,7 @@ defmodule Livebook.Notebook do
:hub_id,
:hub_secret_names,
:file_entries,
:quarantine_file_entry_names,
:teams_enabled
]
@ -48,6 +49,7 @@ defmodule Livebook.Notebook do
hub_id: String.t(),
hub_secret_names: list(String.t()),
file_entries: list(file_entry()),
quarantine_file_entry_names: MapSet.new(String.t()),
teams_enabled: boolean()
}
@ -88,6 +90,7 @@ defmodule Livebook.Notebook do
hub_id: Livebook.Hubs.Personal.id(),
hub_secret_names: [],
file_entries: [],
quarantine_file_entry_names: MapSet.new(),
teams_enabled: false
}
|> put_setup_cell(Cell.new(:code))

View file

@ -16,9 +16,9 @@ defprotocol Livebook.Runtime do
#
# to which the runtime owner is supposed to reply with
# `{:runtime_file_entry_path_reply, reply}` where `reply` is either
# `{:ok, path}` or `{:error, message}` if accessing the file rails.
# Note that `path` should be accessible within the runtime and can
# be obtained using `transfer_file/4`.
# `{:ok, path}` or `{:error, message | :forbidden}` if accessing the
# file fails. Note that `path` should be accessible within the runtime
# and can be obtained using `transfer_file/4`.
#
# Similarly the runtime can request details about the file source:
#

View file

@ -165,6 +165,9 @@ defmodule Livebook.Runtime.Evaluator.Formatter do
defp error_type(error) when is_struct(error, Kino.InterruptError),
do: {:interrupt, error.variant, error.message}
defp error_type(error) when is_struct(error, Kino.FS.ForbiddenError),
do: {:file_entry_forbidden, error.name}
defp error_type(_), do: :other
defp erlang_to_output(value) do

View file

@ -598,6 +598,14 @@ defmodule Livebook.Session do
GenServer.cast(pid, {:delete_file_entry, self(), name})
end
@doc """
Sends a file entry unquarantine request to the server.
"""
@spec allow_file_entry(pid(), String.t()) :: :ok
def allow_file_entry(pid, name) do
GenServer.cast(pid, {:allow_file_entry, self(), name})
end
@doc """
Removes cache file for the given entry file if one exists.
"""
@ -1333,6 +1341,12 @@ defmodule Livebook.Session do
{:noreply, handle_operation(state, operation)}
end
def handle_cast({:allow_file_entry, client_pid, name}, state) do
client_id = client_id(state, client_pid)
operation = {:allow_file_entry, client_id, name}
{:noreply, handle_operation(state, operation)}
end
@impl true
def handle_info({:DOWN, ref, :process, _, reason}, state)
when ref == state.runtime_monitor_ref do
@ -2449,22 +2463,35 @@ defmodule Livebook.Session do
end
defp file_entry_path(state, name, callback) do
file_entry = Enum.find(state.data.notebook.file_entries, &(&1.name == name))
case file_entry do
%{type: :attachment, name: name} ->
case fetch_file_entry(state, name) do
{:ok, %{type: :attachment, name: name}} ->
files_dir = files_dir_from_state(state)
file = FileSystem.File.resolve(files_dir, name)
file_entry_path_from_file(state, name, file, callback)
%{type: :file, name: name, file: file} ->
{:ok, %{type: :file, name: name, file: file}} ->
file_entry_path_from_file(state, name, file, callback)
%{type: :url, name: name, url: url} ->
{:ok, %{type: :url, name: name, url: url}} ->
file_entry_path_from_url(state, name, url, callback)
nil ->
callback.({:error, "no file named #{inspect(name)} exists in the notebook"})
{:error, message} ->
callback.({:error, message})
end
end
defp fetch_file_entry(state, name) do
file_entry = Enum.find(state.data.notebook.file_entries, &(&1.name == name))
cond do
file_entry == nil ->
{:error, "no file named #{inspect(name)} exists in the notebook"}
name in state.data.notebook.quarantine_file_entry_names ->
{:error, :forbidden}
true ->
{:ok, file_entry}
end
end
@ -2516,22 +2543,20 @@ defmodule Livebook.Session do
end
defp file_entry_spec(state, name) do
file_entry = Enum.find(state.data.notebook.file_entries, &(&1.name == name))
case file_entry do
%{type: :attachment, name: name} ->
case fetch_file_entry(state, name) do
{:ok, %{type: :attachment, name: name}} ->
files_dir = files_dir_from_state(state)
file = FileSystem.File.resolve(files_dir, name)
file_entry_spec_from_file(file)
%{type: :file, file: file} ->
{:ok, %{type: :file, file: file}} ->
file_entry_spec_from_file(file)
%{type: :url, url: url} ->
{:ok, %{type: :url, url: url}} ->
file_entry_spec_from_url(url)
nil ->
{:error, "no file named #{inspect(name)} exists in the notebook"}
{:error, message} ->
{:error, message}
end
end

View file

@ -222,6 +222,7 @@ defmodule Livebook.Session.Data do
| {:sync_hub_secrets, client_id()}
| {:add_file_entries, client_id(), list(Notebook.file_entry())}
| {:delete_file_entry, client_id(), String.t()}
| {:allow_file_entry, client_id(), String.t()}
| {:set_app_settings, client_id(), AppSettings.t()}
| {:set_deployed_app_slug, client_id(), String.t()}
| {:app_deactivate, client_id()}
@ -906,6 +907,7 @@ defmodule Livebook.Session.Data do
def apply_operation(data, {:add_file_entries, _client_id, file_entries}) do
data
|> with_actions()
|> unquarantine_file_entries(file_entries)
|> add_file_entries(file_entries)
|> set_dirty()
|> wrap_ok()
@ -915,6 +917,7 @@ defmodule Livebook.Session.Data do
with {:ok, file_entry} <- fetch_file_entry(data.notebook, name) do
data
|> with_actions()
|> unquarantine_file_entries([file_entry])
|> delete_file_entry(file_entry)
|> set_dirty()
|> wrap_ok()
@ -923,6 +926,18 @@ defmodule Livebook.Session.Data do
end
end
def apply_operation(data, {:allow_file_entry, _client_id, name}) do
with {:ok, file_entry} <- fetch_file_entry(data.notebook, name) do
data
|> with_actions()
|> unquarantine_file_entries([file_entry])
|> set_dirty()
|> wrap_ok()
else
_ -> :error
end
end
def apply_operation(data, {:set_app_settings, _client_id, settings}) do
data
|> with_actions()
@ -1704,6 +1719,16 @@ defmodule Livebook.Session.Data do
|> set!(notebook: %{data.notebook | file_entries: file_entries})
end
defp unquarantine_file_entries({data, _} = data_actions, file_entries) do
names = for entry <- file_entries, do: entry.name, into: MapSet.new()
data_actions
|> set!(
notebook:
Map.update!(data.notebook, :quarantine_file_entry_names, &MapSet.difference(&1, names))
)
end
defp set_section_name({data, _} = data_actions, section, name) do
data_actions
|> set!(notebook: Notebook.update_section(data.notebook, section.id, &%{&1 | name: name}))

View file

@ -295,6 +295,37 @@ defmodule LivebookWeb.Output do
"""
end
defp render_output({:error, formatted, {:file_entry_forbidden, file_entry_name}}, %{
session_id: session_id
}) do
assigns = %{
message: formatted,
file_entry_name: file_entry_name,
session_id: session_id
}
~H"""
<div class="-m-4 space-x-4 py-4">
<div
class="flex items-center justify-between border-b px-4 pb-4 mb-4"
style="color: var(--ansi-color-red);"
>
<div class="flex space-x-2 font-editor">
<.remix_icon icon="close-circle-line" />
<span>Forbidden access to file <%= inspect(@file_entry_name) %></span>
</div>
<button
class="button-base button-gray"
phx-click={JS.push("review_file_entry_access", value: %{name: @file_entry_name})}
>
Review access
</button>
</div>
<%= render_formatted_error_message(@message) %>
</div>
"""
end
defp render_output({:error, _formatted, {:interrupt, variant, message}}, %{cell_id: cell_id}) do
assigns = %{variant: variant, message: message, cell_id: cell_id}

View file

@ -3,6 +3,7 @@ defmodule LivebookWeb.SessionLive do
import LivebookWeb.UserHelpers
import LivebookWeb.SessionHelpers
import LivebookWeb.FileSystemHelpers
import Livebook.Utils, only: [format_bytes: 1]
alias Livebook.{Sessions, Session, Delta, Notebook, Runtime, LiveMarkdown}
@ -223,6 +224,7 @@ defmodule LivebookWeb.SessionLive do
id="files-list"
session={@session}
file_entries={@data_view.file_entries}
quarantine_file_entry_names={@data_view.quarantine_file_entry_names}
/>
</div>
<div data-el-secrets-list>
@ -1483,6 +1485,41 @@ defmodule LivebookWeb.SessionLive do
{:noreply, socket}
end
def handle_event("review_file_entry_access", %{"name" => name}, socket) do
file_entry = Enum.find(socket.private.data.notebook.file_entries, &(&1.name == name))
if file_entry do
on_confirm = fn socket ->
Session.allow_file_entry(socket.assigns.session.pid, file_entry.name)
socket
end
assigns = %{name: file_entry.name, file: file_entry.file}
description = ~H"""
<div>
File <span class="font-semibold"><%= @name %></span>
points to an absolute path, do you want the notebook to access it?
</div>
<div class="mt-4 flex flex-col gap-2 border border-gray-200 rounded-lg p-4">
<.labeled_text label="Path"><%= @file.path %></.labeled_text>
<.labeled_text label="File system"><%= file_system_label(@file.file_system) %></.labeled_text>
</div>
"""
{:noreply,
confirm(socket, on_confirm,
title: "Review access",
description: description,
confirm_text: "Allow access",
confirm_icon: "shield-check-line",
danger: false
)}
else
{:noreply, socket}
end
end
@impl true
def handle_info({:operation, operation}, socket) do
{:noreply, handle_operation(socket, operation)}
@ -2295,6 +2332,7 @@ defmodule LivebookWeb.SessionLive do
any_session_secrets?:
Session.Data.session_secrets(data.secrets, data.notebook.hub_id) != [],
file_entries: Enum.sort_by(data.notebook.file_entries, & &1.name),
quarantine_file_entry_names: data.notebook.quarantine_file_entry_names,
app_settings: data.notebook.app_settings,
deployed_app_slug: data.deployed_app_slug
}

View file

@ -31,11 +31,25 @@ defmodule LivebookWeb.SessionLive.FilesListComponent do
<.files_info_icon />
</div>
<div class="mt-5 flex flex-col gap-1">
<div :for={{file_entry, idx} <- Enum.with_index(@file_entries)} class="flex justify-between">
<div class="flex items-center text-gray-500">
<.remix_icon icon={file_entry_icon(file_entry.type)} class="text-lg align-middle mr-2" />
<span><%= file_entry.name %></span>
</div>
<div
:for={{file_entry, idx} <- Enum.with_index(@file_entries)}
class="flex items-center justify-between"
>
<%= if file_entry.name in @quarantine_file_entry_names do %>
<button
class="flex items-center text-yellow-bright-500 cursor-pointer tooltip top"
data-tooltip="Click to review access"
phx-click={JS.push("review_file_entry_access", value: %{name: file_entry.name})}
>
<.remix_icon icon="alert-line" class="text-lg align-middle mr-2" />
<span class="break-all"><%= file_entry.name %></span>
</button>
<% else %>
<div class="flex items-center text-gray-500">
<.remix_icon icon={file_entry_icon(file_entry.type)} class="text-lg align-middle mr-2" />
<span class="break-all"><%= file_entry.name %></span>
</div>
<% end %>
<.menu id={"file-entry-#{idx}-menu"} position={:bottom_right}>
<:toggle>
<button class="icon-button" aria-label="menu">
@ -51,7 +65,7 @@ defmodule LivebookWeb.SessionLive.FilesListComponent do
}
>
<.remix_icon icon="file-transfer-line" />
<span>Copy to files</span>
<span>Copy to files directory</span>
</button>
</.menu_item>
<.menu_item disabled={not Livebook.Session.file_entry_cacheable?(@session, file_entry)}>

View file

@ -1347,10 +1347,56 @@ defmodule Livebook.LiveMarkdown.ExportTest do
# My Notebook
"""
{document, []} = Export.notebook_to_livemd(notebook)
{document, []} = Export.notebook_to_livemd(notebook, include_stamp: false)
assert expected_document == document
end
test "stores quarantine file entry names if there are any :file file entries" do
file = Livebook.FileSystem.File.new(Livebook.FileSystem.Local.new(), p("/document.pdf"))
# All allowed
notebook = %{
Notebook.new()
| name: "My Notebook",
file_entries: [
%{type: :file, name: "document.pdf", file: file}
]
}
{document, []} = Export.notebook_to_livemd(notebook)
assert stamp_metadata(notebook, document) == %{quarantine_file_entry_names: []}
# Subset allowed
notebook = %{
Notebook.new()
| name: "My Notebook",
file_entries: [
%{type: :file, name: "document1.pdf", file: file},
%{type: :file, name: "document2.pdf", file: file}
],
quarantine_file_entry_names: MapSet.new(["document1.pdf"])
}
{document, []} = Export.notebook_to_livemd(notebook)
assert stamp_metadata(notebook, document) == %{
quarantine_file_entry_names: ["document1.pdf"]
}
end
end
defp stamp_metadata(notebook, source) do
[_, json] = Regex.run(~r/<!-- livebook:(.*) -->\n$/, source)
%{"offset" => offset, "stamp" => stamp} = Jason.decode!(json)
hub = Livebook.Hubs.fetch_hub!(notebook.hub_id)
source = binary_slice(source, 0, offset)
{:ok, metadata} = Livebook.Hubs.verify_notebook_stamp(hub, source, stamp)
metadata
end
defp spawn_widget_with_data(ref, data) do

View file

@ -1251,5 +1251,106 @@ defmodule Livebook.LiveMarkdown.ImportTest do
assert messages == ["skipping file document.pdf, since it points to an unknown file system"]
end
test "imports :file file entries with quarantine when no stamp is given" do
markdown = """
<!-- livebook:{"file_entries":[{"file":{"file_system_id":"local","path":"#{p("/document.pdf")}"},"name":"document.pdf","type":"file"}]} -->
# My Notebook
"""
{notebook, %{warnings: []}} = Import.notebook_from_livemd(markdown)
assert %Notebook{
file_entries: [
%{
type: :file,
name: "document.pdf",
file: %Livebook.FileSystem.File{
file_system: %Livebook.FileSystem.Local{},
path: p("/document.pdf")
}
}
]
} = notebook
assert notebook.quarantine_file_entry_names == MapSet.new(["document.pdf"])
end
test "imports :file file entries with quarantine when the stamp is invalid" do
file = Livebook.FileSystem.File.new(Livebook.FileSystem.Local.new(), p("/document.pdf"))
# We generate the Live Markdown programmatically, because the
# absolute path is a part of the stamp and it is different on
# Windows
{markdown, []} =
%{
Notebook.new()
| name: "My Notebook",
file_entries: [%{type: :file, name: "document.pdf", file: file}]
}
|> Livebook.LiveMarkdown.Export.notebook_to_livemd()
# Change file path in the document
markdown = String.replace(markdown, p("/document.pdf"), p("/other.pdf"))
{notebook, _} = Import.notebook_from_livemd(markdown)
assert %Notebook{
file_entries: [
%{
type: :file,
name: "document.pdf",
file: %Livebook.FileSystem.File{
file_system: %Livebook.FileSystem.Local{},
path: p("/other.pdf")
}
}
]
} = notebook
assert notebook.quarantine_file_entry_names == MapSet.new(["document.pdf"])
end
test "imports quarantine file entry names from stamp metadata" do
file = Livebook.FileSystem.File.new(Livebook.FileSystem.Local.new(), p("/document.pdf"))
{markdown, []} =
%{
Notebook.new()
| name: "My Notebook",
file_entries: [
%{type: :file, name: "document1.pdf", file: file},
%{type: :file, name: "document2.pdf", file: file}
],
quarantine_file_entry_names: MapSet.new(["document1.pdf"])
}
|> Livebook.LiveMarkdown.Export.notebook_to_livemd()
{notebook, %{warnings: []}} = Import.notebook_from_livemd(markdown)
assert %Notebook{
file_entries: [
%{
type: :file,
name: "document2.pdf",
file: %Livebook.FileSystem.File{
file_system: %Livebook.FileSystem.Local{},
path: p("/document.pdf")
}
},
%{
type: :file,
name: "document1.pdf",
file: %Livebook.FileSystem.File{
file_system: %Livebook.FileSystem.Local{},
path: p("/document.pdf")
}
}
]
} = notebook
assert notebook.quarantine_file_entry_names == MapSet.new(["document1.pdf"])
end
end
end

View file

@ -3894,6 +3894,27 @@ defmodule Livebook.Session.DataTest do
assert {:ok, %{notebook: %{file_entries: [^file_entry3, ^file_entry4, ^file_entry1]}}, []} =
Data.apply_operation(data, operation)
end
test "removes matching file entry names from quarantine" do
file = Livebook.FileSystem.File.new(Livebook.FileSystem.Local.new(), p("/image.jpg"))
file_entry = %{type: :file, name: "image.jpg", file: file}
notebook = %{
Notebook.new()
| file_entries: [file_entry],
quarantine_file_entry_names: MapSet.new(["image.jpg"])
}
data = Data.new(notebook: notebook)
file_entry = %{type: :attachment, name: "image.jpg"}
operation = {:add_file_entries, @cid, [file_entry]}
assert {:ok, %{notebook: notebook}, []} = Data.apply_operation(data, operation)
assert notebook.quarantine_file_entry_names == MapSet.new()
end
end
describe "apply_operation/2 given :delete_file_entry" do
@ -3917,6 +3938,52 @@ defmodule Livebook.Session.DataTest do
assert {:ok, %{notebook: %{file_entries: [^file_entry2]}}, []} =
Data.apply_operation(data, operation)
end
test "removes matching file entry names from quarantine" do
file = Livebook.FileSystem.File.new(Livebook.FileSystem.Local.new(), p("/image.jpg"))
file_entry = %{type: :file, name: "image.jpg", file: file}
notebook = %{
Notebook.new()
| file_entries: [file_entry],
quarantine_file_entry_names: MapSet.new(["image.jpg"])
}
data = Data.new(notebook: notebook)
operation = {:delete_file_entry, @cid, "image.jpg"}
assert {:ok, %{notebook: notebook}, []} = Data.apply_operation(data, operation)
assert notebook.quarantine_file_entry_names == MapSet.new()
end
end
describe "apply_operation/2 given :allow_file_entry" do
test "returns an error if no file entry with the given name exists" do
data = Data.new()
operation = {:allow_file_entry, @cid, "image.jpg"}
assert :error = Data.apply_operation(data, operation)
end
test "removes matching file entry names from quarantine" do
file = Livebook.FileSystem.File.new(Livebook.FileSystem.Local.new(), p("/image.jpg"))
file_entry = %{type: :file, name: "image.jpg", file: file}
notebook = %{
Notebook.new()
| file_entries: [file_entry],
quarantine_file_entry_names: MapSet.new(["image.jpg"])
}
data = Data.new(notebook: notebook)
operation = {:allow_file_entry, @cid, "image.jpg"}
assert {:ok, %{notebook: notebook}, []} = Data.apply_operation(data, operation)
assert notebook.quarantine_file_entry_names == MapSet.new()
end
end
describe "apply_operation/2 given :set_app_settings" do

View file

@ -1566,6 +1566,31 @@ defmodule Livebook.SessionTest do
{:error, ~s/no file named "image.jpg" exists in the notebook/}}
end
test "replies with error when file entry is in quarantine" do
file = Livebook.FileSystem.File.new(Livebook.FileSystem.Local.new(), p("/document.pdf"))
notebook = %{
Livebook.Notebook.new()
| file_entries: [
%{type: :file, name: "document.pdf", file: file}
],
quarantine_file_entry_names: MapSet.new(["document.pdf"])
}
session = start_session(notebook: notebook)
runtime = connected_noop_runtime(self())
Session.set_runtime(session.pid, runtime)
send(session.pid, {:runtime_file_entry_path_request, self(), "document.pdf"})
assert_receive {:runtime_file_entry_path_reply, {:error, :forbidden}}
# Spec request
send(session.pid, {:runtime_file_entry_spec_request, self(), "document.pdf"})
assert_receive {:runtime_file_entry_spec_reply, {:error, :forbidden}}
end
test "when nonexistent :attachment replies with error" do
session = start_session()

View file

@ -1771,6 +1771,38 @@ defmodule LivebookWeb.SessionLiveTest do
assert FileSystem.File.resolve(session.files_dir, "image.jpg") |> FileSystem.File.read() ==
{:ok, "content"}
end
test "allowing access to file entry in quarantine", %{conn: conn} do
file = Livebook.FileSystem.File.new(Livebook.FileSystem.Local.new(), p("/document.pdf"))
notebook = %{
Livebook.Notebook.new()
| file_entries: [
%{type: :file, name: "document.pdf", file: file}
],
quarantine_file_entry_names: MapSet.new(["document.pdf"])
}
{:ok, session} = Sessions.create_session(notebook: notebook)
{:ok, view, _} = live(conn, ~p"/sessions/#{session.id}")
assert view
|> element(~s/[data-el-files-list]/)
|> render() =~ "Click to review access"
view
|> element(~s/[data-el-files-list] button/, "document.pdf")
|> render_click()
render_confirm(view)
refute view
|> element(~s/[data-el-files-list]/)
|> render() =~ "Click to review access"
Session.close(session.pid)
end
end
describe "apps" do