From 7ffbef33b5110f2321dcef0cf297e1e1569e1440 Mon Sep 17 00:00:00 2001
From: Marcos Lilljedahl <marcosnils@gmail.com>
Date: Fri, 1 Apr 2022 13:00:41 -0300
Subject: [PATCH] Report error if FS is incorrectly handled on client
 filesystem read

Signed-off-by: Marcos Lilljedahl <marcosnils@gmail.com>
---
 plan/task/clientfilesystemread.go             |  9 ++++++--
 tests/plan.bats                               | 15 ++++++++++---
 .../fs/{invalid => invalid_fs_input}/test.cue |  1 +
 .../read/fs/invalid_fs_type/test.cue          | 21 +++++++++++++++++++
 4 files changed, 41 insertions(+), 5 deletions(-)
 rename tests/plan/client/filesystem/read/fs/{invalid => invalid_fs_input}/test.cue (72%)
 create mode 100644 tests/plan/client/filesystem/read/fs/invalid_fs_type/test.cue

diff --git a/plan/task/clientfilesystemread.go b/plan/task/clientfilesystemread.go
index 6ef5a32d..3dc6a5e3 100644
--- a/plan/task/clientfilesystemread.go
+++ b/plan/task/clientfilesystemread.go
@@ -26,10 +26,15 @@ func (t clientFilesystemReadTask) PreRun(_ context.Context, pctx *plancontext.Co
 		return err
 	}
 
-	if pi, err := os.Stat(path); errors.Is(err, os.ErrNotExist) {
+	isFS := plancontext.IsFSValue(v.Lookup("contents"))
+
+	switch pi, err := os.Stat(path); {
+	case errors.Is(err, os.ErrNotExist):
 		return fmt.Errorf("path %q does not exist", path)
-	} else if !pi.IsDir() && plancontext.IsFSValue(v.Lookup("contents")) {
+	case !pi.IsDir() && isFS:
 		return fmt.Errorf("path %q is not a directory", path)
+	case pi.IsDir() && !isFS:
+		return fmt.Errorf("path %q cannot be a directory", path)
 	}
 
 	if plancontext.IsFSValue(v.Lookup("contents")) {
diff --git a/tests/plan.bats b/tests/plan.bats
index 78a6cc66..69692a71 100644
--- a/tests/plan.bats
+++ b/tests/plan.bats
@@ -85,14 +85,23 @@ setup() {
 }
 
 
-@test "plan/client/filesystem/read/fs/invalid" {
-  cd "$TESTDIR/plan/client/filesystem/read/fs/invalid"
+@test "plan/client/filesystem/read/fs/invalid_fs_input" {
+  cd "$TESTDIR/plan/client/filesystem/read/fs/invalid_fs_input"
 
   run "$DAGGER" "do" -p . test
   assert_failure
   assert_output --partial 'test.txt" is not a directory'
 }
 
+
+@test "plan/client/filesystem/read/fs/invalid_fs_type" {
+  cd "$TESTDIR/plan/client/filesystem/read/fs/invalid_fs_type"
+
+  run "$DAGGER" "do" -p . test
+  assert_failure
+  assert_output --partial 'rootfs" cannot be a directory'
+}
+
 @test "plan/client/filesystem/read/fs/relative" {
   cd "$TESTDIR/plan/client/filesystem/read/fs/relative"
 
@@ -230,4 +239,4 @@ setup() {
    # Run with invalid platform
    run "$DAGGER" "do" -p./plan/platform/config_platform_failure_invalid_platform.cue verify
    assert_failure
-}
\ No newline at end of file
+}
diff --git a/tests/plan/client/filesystem/read/fs/invalid/test.cue b/tests/plan/client/filesystem/read/fs/invalid_fs_input/test.cue
similarity index 72%
rename from tests/plan/client/filesystem/read/fs/invalid/test.cue
rename to tests/plan/client/filesystem/read/fs/invalid_fs_input/test.cue
index 155203eb..e35685e8 100644
--- a/tests/plan/client/filesystem/read/fs/invalid/test.cue
+++ b/tests/plan/client/filesystem/read/fs/invalid_fs_input/test.cue
@@ -5,6 +5,7 @@ import (
 )
 
 dagger.#Plan & {
+	// Reading a file into a dagger.#FS should not be possbile
 	client: filesystem: "../rootfs/test.txt": read: contents: dagger.#FS
 	actions: test: {
 	}
diff --git a/tests/plan/client/filesystem/read/fs/invalid_fs_type/test.cue b/tests/plan/client/filesystem/read/fs/invalid_fs_type/test.cue
new file mode 100644
index 00000000..0730ed64
--- /dev/null
+++ b/tests/plan/client/filesystem/read/fs/invalid_fs_type/test.cue
@@ -0,0 +1,21 @@
+package main
+
+import (
+	"dagger.io/dagger"
+	"dagger.io/dagger/core"
+)
+
+dagger.#Plan & {
+	// Reading a directory into a non-fs should fail
+	client: filesystem: "../rootfs": read: contents: string
+	actions: {
+		image: core.#Pull & {
+			source: "alpine:3.15.0@sha256:e7d88de73db3d3fd9b2d63aa7f447a10fd0220b7cbf39803c803f2af9ba256b3"
+		}
+		test: core.#Exec & {
+			input: image.output
+			args: ["test", client.filesystem."../rootfs".read.contents]
+		}
+
+	}
+}