merge #44 into cyphar/filepath-securejoin:main

Aleksa Sarai (2):
  gha: add GOARCH=386 build check
  mkdirall: switch to os.FileMode argument

LGTMs: cyphar kolyshkin

Author: cyphar

.github/workflows/ci.yml

@@ -19,6 +19,8 @@ jobs:
           - windows-latest
           - ubuntu-latest
           - macos-latest
+        go-arch:
+          - amd64
         go-version:
           - "1.18"
           - "1.19"
@@ -27,13 +29,19 @@ jobs:
           - "1.22"
           - "1.23"
           - "^1"
+        include:
+          - os: ubuntu-latest
+            go-arch: "386"
+            go-version: "^1"
     runs-on: ${{ matrix.os }}
     steps:
       - uses: actions/checkout@v4
       - uses: actions/setup-go@v5
         with:
           go-version: ${{ matrix.go-version }}
           check-latest: true
+      - name: set GOOARCH
+        run: echo "GOARCH=${{ matrix.go-arch }}" >>"$GITHUB_ENV"
       - name: go build check
         run: go build ./...
       - name: go test build check

CHANGELOG.md

@@ -22,6 +22,20 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
   necessarily a breaking API change (though we expect no real users to be
   affected by it).
 
+- `MkdirAll` and `MkdirHandle` now take an `os.FileMode`-style mode argument
+  instead of a raw `unix.S_*`-style mode argument, which may cause compile-time
+  type errors depending on how you use `filepath-securejoin`. For most users,
+  there will be no change in behaviour aside from the type change (as the
+  bottom `0o777` bits are the same in both formats, and most users are probably
+  only using those bits).
+
+  However, if you were using `unix.S_ISVTX` to set the sticky bit with
+  `MkdirAll(Handle)` you will need to switch to `os.ModeSticky` otherwise you
+  will get a runtime error with this update. In addition, the error message you
+  will get from passing `unix.S_ISUID` and `unix.S_ISGID` will be different as
+  they are treated as invalid bits now (note that previously passing said bits
+  was also an error).
+
 ## [0.3.6] - 2024-12-17 ##
 
 ### Compatibility ###

mkdir_linux.go

@@ -21,6 +21,33 @@ var (
 	errPossibleAttack = errors.New("possible attack detected")
 )
 
+// modePermExt is like os.ModePerm except that it also includes the set[ug]id
+// and sticky bits.
+const modePermExt = os.ModePerm | os.ModeSetuid | os.ModeSetgid | os.ModeSticky
+
+//nolint:cyclop // this function needs to handle a lot of cases
+func toUnixMode(mode os.FileMode) (uint32, error) {
+	sysMode := uint32(mode.Perm())
+	if mode&os.ModeSetuid != 0 {
+		sysMode |= unix.S_ISUID
+	}
+	if mode&os.ModeSetgid != 0 {
+		sysMode |= unix.S_ISGID
+	}
+	if mode&os.ModeSticky != 0 {
+		sysMode |= unix.S_ISVTX
+	}
+	// We don't allow file type bits.
+	if mode&os.ModeType != 0 {
+		return 0, fmt.Errorf("%w %+.3o (%s): type bits not permitted", errInvalidMode, mode, mode)
+	}
+	// We don't allow other unknown modes.
+	if mode&^modePermExt != 0 || sysMode&unix.S_IFMT != 0 {
+		return 0, fmt.Errorf("%w %+.3o (%s): unknown mode bits", errInvalidMode, mode, mode)
+	}
+	return sysMode, nil
+}
+
 // MkdirAllHandle is equivalent to [MkdirAll], except that it is safer to use
 // in two respects:
 //
@@ -39,17 +66,17 @@ var (
 // a brand new lookup of unsafePath (such as with [SecureJoin] or openat2) after
 // doing [MkdirAll]. If you intend to open the directory after creating it, you
 // should use MkdirAllHandle.
-func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err error) {
-	// Make sure there are no os.FileMode bits set.
-	if mode&^0o7777 != 0 {
-		return nil, fmt.Errorf("%w for mkdir 0o%.3o", errInvalidMode, mode)
+func MkdirAllHandle(root *os.File, unsafePath string, mode os.FileMode) (_ *os.File, Err error) {
+	unixMode, err := toUnixMode(mode)
+	if err != nil {
+		return nil, err
 	}
 	// On Linux, mkdirat(2) (and os.Mkdir) silently ignore the suid and sgid
 	// bits. We could also silently ignore them but since we have very few
 	// users it seems more prudent to return an error so users notice that
 	// these bits will not be set.
-	if mode&^0o1777 != 0 {
-		return nil, fmt.Errorf("%w for mkdir 0o%.3o: suid and sgid are ignored by mkdir", errInvalidMode, mode)
+	if unixMode&^0o1777 != 0 {
+		return nil, fmt.Errorf("%w for mkdir %+.3o: suid and sgid are ignored by mkdir", errInvalidMode, mode)
 	}
 
 	// Try to open as much of the path as possible.
@@ -104,9 +131,6 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 		return nil, fmt.Errorf("%w: yet-to-be-created path %q contains '..' components", unix.ENOENT, remainingPath)
 	}
 
-	// Make sure the mode doesn't have any type bits.
-	mode &^= unix.S_IFMT
-
 	// Create the remaining components.
 	for _, part := range remainingParts {
 		switch part {
@@ -123,7 +147,7 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 		// directory at the same time as us. In that case, just continue on as
 		// if we created it (if the created inode is not a directory, the
 		// following open call will fail).
-		if err := unix.Mkdirat(int(currentDir.Fd()), part, uint32(mode)); err != nil && !errors.Is(err, unix.EEXIST) {
+		if err := unix.Mkdirat(int(currentDir.Fd()), part, unixMode); err != nil && !errors.Is(err, unix.EEXIST) {
 			err = &os.PathError{Op: "mkdirat", Path: currentDir.Name() + "/" + part, Err: err}
 			// Make the error a bit nicer if the directory is dead.
 			if deadErr := isDeadInode(currentDir); deadErr != nil {
@@ -196,10 +220,7 @@ func MkdirAllHandle(root *os.File, unsafePath string, mode int) (_ *os.File, Err
 // If you plan to open the directory after you have created it or want to use
 // an open directory handle as the root, you should use [MkdirAllHandle] instead.
 // This function is a wrapper around [MkdirAllHandle].
-//
-// NOTE: The mode argument must be set the unix mode bits (unix.S_I...), not
-// the Go generic mode bits ([os.FileMode]...).
-func MkdirAll(root, unsafePath string, mode int) error {
+func MkdirAll(root, unsafePath string, mode os.FileMode) error {
 	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 	if err != nil {
 		return err

mkdir_linux_test.go

@@ -20,14 +20,14 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-type mkdirAllFunc func(t *testing.T, root, unsafePath string, mode int) error
+type mkdirAllFunc func(t *testing.T, root, unsafePath string, mode os.FileMode) error
 
-var mkdirAll_MkdirAll mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode int) error {
+var mkdirAll_MkdirAll mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode os.FileMode) error {
 	// We can't check expectedPath here.
 	return MkdirAll(root, unsafePath, mode)
 }
 
-var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode int) error {
+var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath string, mode os.FileMode) error {
 	// Same logic as MkdirAll.
 	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 	if err != nil {
@@ -55,7 +55,7 @@ var mkdirAll_MkdirAllHandle mkdirAllFunc = func(t *testing.T, root, unsafePath s
 	return nil
 }
 
-func checkMkdirAll(t *testing.T, mkdirAll mkdirAllFunc, root, unsafePath string, mode, expectedMode int, expectedErr error) {
+func checkMkdirAll(t *testing.T, mkdirAll mkdirAllFunc, root, unsafePath string, mode os.FileMode, expectedMode int, expectedErr error) {
 	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 	require.NoError(t, err)
 	defer rootDir.Close()
@@ -248,31 +248,36 @@ func TestMkdirAllHandle_AsRoot(t *testing.T) {
 
 func testMkdirAll_InvalidMode(t *testing.T, mkdirAll mkdirAllFunc) {
 	for _, test := range []struct {
-		mode        int
+		mode        os.FileMode
 		expectedErr error
 	}{
-		// os.FileMode bits are invalid.
-		{int(os.ModeDir | 0o777), errInvalidMode},
-		{int(os.ModeSticky | 0o777), errInvalidMode},
-		{int(os.ModeIrregular | 0o777), errInvalidMode},
+		// unix.S_IS* bits are invalid.
+		{unix.S_ISUID | 0o777, errInvalidMode},
+		{unix.S_ISGID | 0o777, errInvalidMode},
+		{unix.S_ISVTX | 0o777, errInvalidMode},
+		{unix.S_ISUID | unix.S_ISGID | unix.S_ISVTX | 0o777, errInvalidMode},
 		// unix.S_IFMT bits are also invalid.
 		{unix.S_IFDIR | 0o777, errInvalidMode},
 		{unix.S_IFREG | 0o777, errInvalidMode},
 		{unix.S_IFIFO | 0o777, errInvalidMode},
+		// os.FileType bits are also invalid.
+		{os.ModeDir | 0o777, errInvalidMode},
+		{os.ModeNamedPipe | 0o777, errInvalidMode},
+		{os.ModeIrregular | 0o777, errInvalidMode},
 		// suid/sgid bits are silently ignored by mkdirat and so we return an
 		// error explicitly.
-		{unix.S_ISUID | 0o777, errInvalidMode},
-		{unix.S_ISGID | 0o777, errInvalidMode},
-		{unix.S_ISUID | unix.S_ISGID | unix.S_ISVTX | 0o777, errInvalidMode},
+		{os.ModeSetuid | 0o777, errInvalidMode},
+		{os.ModeSetgid | 0o777, errInvalidMode},
+		{os.ModeSetuid | os.ModeSetgid | os.ModeSticky | 0o777, errInvalidMode},
 		// Proper sticky bit should work.
-		{unix.S_ISVTX | 0o777, nil},
+		{os.ModeSticky | 0o777, nil},
 		// Regular mode bits.
 		{0o777, nil},
 		{0o711, nil},
 	} {
 		root := t.TempDir()
 		err := mkdirAll(t, root, "a/b/c", test.mode)
-		assert.ErrorIsf(t, err, test.expectedErr, "mkdirall 0o%.3o", test.mode)
+		assert.ErrorIsf(t, err, test.expectedErr, "mkdirall %+.3o (%s)", test.mode, test.mode)
 	}
 }
 
@@ -295,7 +300,7 @@ func newRacingMkdirMeta() *racingMkdirMeta {
 	}
 }
 
-func (m *racingMkdirMeta) checkMkdirAllHandle_Racing(t *testing.T, root, unsafePath string, mode int, allowedErrs []error) {
+func (m *racingMkdirMeta) checkMkdirAllHandle_Racing(t *testing.T, root, unsafePath string, mode os.FileMode, allowedErrs []error) {
 	rootDir, err := os.OpenFile(root, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0)
 	require.NoError(t, err, "open root")
 	defer rootDir.Close()