go icon indicating copy to clipboard operation
go copied to clipboard

testing/fstest: cannot test no permission on folder

Open staticdev opened this issue 3 years ago • 7 comments

What version of Go are you using (go version)?

$ go version
go version go1.17.3 linux/amd64

Does this issue reproduce with the latest release?

Yes. Not totally sure if it is related to https://github.com/golang/go/issues/46776

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/user/.cache/go-build"
GOENV="/home/user/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/user/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/user/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17.3"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/home/user/gomodule/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build3099453485=/tmp/go-build -gno-record-gcc-switches"

What did you do?

tested.go

package x

import (
	"fmt"
	"io/fs"
	"os"
	"path/filepath"
	"strings"
)

func fileExists(fileSystem fs.FS, fileName string) (bool, error) {
	_, err := fs.Stat(fileSystem, fileName)
	if err == nil {
		return true, nil
	}
	if os.IsNotExist(err) {
		return false, fmt.Errorf("File %q does not exist.", fileName)
	}
	return false, err // test case should return here
}

func FileValid(fileSystem fs.FS, fileName string) (string, error) {
	fileExists, err := fileExists(fileSystem, fileName)
	if !fileExists || err != nil {
		return "", err
	}

	return "success", nil
}

tested_test.go

package x

import (
	"io/fs"
	"testing"
	"testing/fstest"
)

var (
	fakeFS = fstest.MapFS{
		"secret-folder":          {Mode: 0000},
		"secret-folder/readme.md": {},
	}
)

func TestFileValidPermissionDenied(t *testing.T) {
	fileName := "readme.md"
	fsys, _ := fs.Sub(fakeFS, "secret-folder")
	wantOut := ""

	out, err := FileValid(fsys, fileName)
	if out != wantOut || err == nil {
		t.Errorf("want %q, nil got %q, %v", wantOut, out, err)
	}
}

What did you expect to see?

This test should pass, returning "", and error that permission is denied.

What did you see instead?

It can Stat the file, even with no permission to the folder.

staticdev avatar Jan 24 '22 20:01 staticdev

note 0000 == 0 == unset

seankhliao avatar Jan 24 '22 20:01 seankhliao

Didn't find that in the docs. But I found a Go tutorial explaining 0000 as no permissions https://schadokar.dev/to-the-point/how-to-read-and-write-a-file-in-golang/

Does this make sense? How do I give equivalent to chmod 0000 or 000?

staticdev avatar Jan 24 '22 21:01 staticdev

CC @bcmills I think?

mknyszek avatar Jan 25 '22 15:01 mknyszek

(Not really my area, but I've been looking at io/fs today anyway.)

Looks like the fast-path for files is here: https://cs.opensource.google/go/go/+/refs/tags/go1.17.6:src/testing/fstest/mapfs.go;l=51-55;drc=d6aa162f30d63f21f5f4db75e302dbb90595bbe2

It indeed does not check parent permissions at all, although note that the fs.FS.Open method also doesn't specify detailed semantics for ModePerm bits.

bcmills avatar Jan 25 '22 16:01 bcmills

@mknyszek can I help with something else? I am new to Golang, maybe would need some guidance to have a fix PR if that is the case.

staticdev avatar Jan 27 '22 18:01 staticdev

I came across this same issue in the wild. This github issue is marked as "needs investigation", so I wrote up a simple reproducible test case based upon the initial bug report:

package x

import (
	"io/fs"
	"os"
	"testing"
	"testing/fstest"
)

func TestFileValidPermissionDenied(t *testing.T) {
	fakeFS := fstest.MapFS{
		"secret-folder":           {Mode: 0o200},
		"secret-folder/readme.md": {Data: []byte("this content should be unread"), Mode: 0o100},
	}

	out, err := fs.ReadFile(fakeFS, "secret-folder/readme.md")

	if !os.IsPermission(err) {
		t.Errorf("Expected a permission error but received %q", err)
	}

	if string(out) != "" {
		t.Errorf("want %q, got %q, %v", "", out, err)
	}
}

gwynforthewyn avatar May 06 '24 23:05 gwynforthewyn

@seankhliao @mknyszek @bcmills I'd like to take a swipe at fixing this issue. The Contribution Guide says "NeedsInvestigation: The issue is not fully understood and requires analysis to understand the root cause."

The issue here looks like what bcmills noted above: the fstest.MapFS is just a map under the hood and has no explicit checking of the pseudo-file-system's permissions.

I think the root of this issue would be fixed if we add a function to verify these permissions, something similar to:

func (fsys MapFS) checkPermissions(file *MapFile, requiredPerm fs.FileMode) bool {
    // Mock current user ID and group ID (in a real scenario, fetch this from the environment)
    currentUID := 1000 // Example: replace with actual user ID
    currentGID := 1000 // Example: replace with actual group ID

    // Check owner permissions
    if file.OwnerUID == currentUID {
        return file.Mode&fs.ModePerm&requiredPerm != 0
    }

    // Check group permissions
    if file.OwnerGID == currentGID {
        return (file.Mode>>3)&fs.ModePerm&requiredPerm != 0
    }

    // Check others permissions
    return (file.Mode>>6)&fs.ModePerm&requiredPerm != 0
}

My bitshifting isn't super great, but I think that gets us an appropriate check. Then it's a matter of wiring it in to the package.

Is there any chance I could have someone help me design the solution here and help me get this in?

Thanks

gwynforthewyn avatar Aug 27 '24 12:08 gwynforthewyn

I don't think we should do anything here, adding new checks to MapFS will potentially break too many users considering permission bits have never been needed, nor are they required.

seankhliao avatar Apr 19 '25 16:04 seankhliao