From 5df5458f1d057ca8ba9f9e4b0296687e90dbbd6b Mon Sep 17 00:00:00 2001
From: Martin Pitt <mpitt@redhat.com>
Date: Mon, 12 Feb 2024 15:32:19 +0100
Subject: [PATCH] bridge: Fix fsreplace1 crash for error cases

Catch errors from `rename()` and translate them into an `access-denied`
(e.g. the specified `path` is a directory) or `not-found` (directory is
deleted underneath us) channel error instead of crashing the bridge.

Likewise, intercept `FileNotFoundError` when trying to create a temp
file in a nonexisiting directory. That previously returned
`internal-error`, but it's really not. We don't have a really good
exisiting problem code, but `access-denied` is closest.
---
 src/cockpit/channels/filesystem.py | 17 ++++++++++++++---
 test/pytest/test_bridge.py         | 15 +++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/src/cockpit/channels/filesystem.py b/src/cockpit/channels/filesystem.py
index 953ccdbd43b8..9df461b27b14 100644
--- a/src/cockpit/channels/filesystem.py
+++ b/src/cockpit/channels/filesystem.py
@@ -181,6 +181,9 @@ def do_data(self, data):
                     continue
                 except PermissionError as exc:
                     raise ChannelError('access-denied') from exc
+                except FileNotFoundError as exc:
+                    # directory of path does not exist
+                    raise ChannelError('not-found') from exc
                 except OSError as exc:
                     raise ChannelError('internal-error', message=str(exc)) from exc
             else:
@@ -212,10 +215,18 @@ def do_done(self):
 
             try:
                 os.rename(self._temppath, self._path)
-            except OSError:
-                # ensure to not leave the temp file behind
+            # ensure to not leave the temp file behind
+            except FileNotFoundError as exc:
                 self.unlink_temppath()
-                raise
+                raise ChannelError('not-found', message=str(exc)) from exc
+            except IsADirectoryError as exc:
+                self.unlink_temppath()
+                # not ideal, but the closest code we have
+                raise ChannelError('access-denied', message=str(exc)) from exc
+            except OSError as exc:
+                self.unlink_temppath()
+                raise ChannelError('internal-error', message=str(exc)) from exc
+
             self._tempfile.close()
             self._tempfile = None
 
diff --git a/test/pytest/test_bridge.py b/test/pytest/test_bridge.py
index 03f86b1cec61..3f03bf8327b0 100644
--- a/test/pytest/test_bridge.py
+++ b/test/pytest/test_bridge.py
@@ -513,6 +513,21 @@ async def test_fsreplace1(transport: MockTransport, tmp_path: Path) -> None:
     assert not myfile.exists()
 
 
+@pytest.mark.asyncio
+async def test_fsreplace1_error(transport: MockTransport, tmp_path: Path) -> None:
+    # trying to write a directory
+    ch = await transport.check_open('fsreplace1', path=str(tmp_path))
+    transport.send_data(ch, b'not me')
+    transport.send_done(ch)
+    await transport.assert_msg('', command='close', channel=ch, problem='access-denied')
+
+    # nonexisting directory
+    ch = await transport.check_open('fsreplace1', path='/non/existing/file')
+    transport.send_data(ch, b'not me')
+    transport.send_done(ch)
+    await transport.assert_msg('', command='close', channel=ch, problem='not-found')
+
+
 @pytest.mark.asyncio
 @pytest.mark.parametrize('channeltype', CHANNEL_TYPES)
 async def test_channel(bridge: Bridge, transport: MockTransport, channeltype, tmp_path: Path) -> None: