Skip to content

Commit

Permalink
folder_branch_ops: disallow empty file names or links
Browse files Browse the repository at this point in the history
A user hit an issue where they created a symlink with a blank name
using SimpleFS, which then caused all sorts of rendering errors in the
GUI and `ls` problems on the command line.

Just disallow the creation of names like that, and add a SimpleFS test
for symlinks.

Also add an error stacktrace to the disallowed prefixes/names error,
for easier debugging.

Issue: HOTPOT-1276
  • Loading branch information
strib committed Nov 13, 2019
1 parent d6e629b commit ea8c9dd
Show file tree
Hide file tree
Showing 4 changed files with 56 additions and 5 deletions.
15 changes: 14 additions & 1 deletion go/kbfs/libkbfs/errors.go
Original file line number Diff line number Diff line change
Expand Up @@ -588,12 +588,25 @@ type DisallowedPrefixError struct {
prefix string
}

// Error implements the error interface for NoChainFoundError.
// Error implements the error interface for DisallowedPrefixError.
func (e DisallowedPrefixError) Error() string {
return fmt.Sprintf("Cannot create %s because it has the prefix %s",
e.name, e.prefix)
}

// DisallowedNameError indicates that the user attempted to create an
// entry using a disallowed name. It includes the plaintext name on
// purpose, for clarity in the error message.
type DisallowedNameError struct {
name string
}

// Error implements the error interface for DisallowedNameError.
func (e DisallowedNameError) Error() string {
return fmt.Sprintf("Cannot create \"%s\" because it is a disallowed name",
e.name)
}

// NameTooLongError indicates that the user tried to write a directory
// entry name that would be bigger than KBFS's supported size.
type NameTooLongError struct {
Expand Down
11 changes: 9 additions & 2 deletions go/kbfs/libkbfs/folder_branch_ops.go
Original file line number Diff line number Diff line change
Expand Up @@ -4324,10 +4324,17 @@ func checkDisallowedPrefixes(
return nil
}
}
return DisallowedPrefixError{name, prefix}
return errors.WithStack(DisallowedPrefixError{name, prefix})
}
}
return nil

// Don't allow any empty or `.` names.
switch name.Plaintext() {
case "", ".", "..":
return errors.WithStack(DisallowedNameError{name.Plaintext()})
default:
return nil
}
}

// PathType returns path type
Expand Down
2 changes: 1 addition & 1 deletion go/kbfs/libkbfs/kbfs_ops_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1455,7 +1455,7 @@ func testCreateEntryFailKBFSPrefix(t *testing.T, et data.EntryType) {
}
if err == nil {
t.Errorf("Got no expected error on create")
} else if err != expectedErr {
} else if errors.Cause(err) != expectedErr {
t.Errorf("Got unexpected error on create: %+v", err)
}
}
Expand Down
33 changes: 32 additions & 1 deletion go/kbfs/simplefs/simplefs_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"fmt"
"io/ioutil"
"os"
"path"
"path/filepath"
"sort"
"strings"
Expand Down Expand Up @@ -189,6 +190,34 @@ func TestStatNonExistent(t *testing.T) {
require.False(t, de.Writable)
}

func TestSymlink(t *testing.T) {
ctx := context.Background()
config := libkbfs.MakeTestConfigOrBust(t, "jdoe")
sfs := newSimpleFS(env.EmptyAppStateUpdater{}, config)

t.Logf("Make a file and then symlink it")
p := keybase1.NewPathWithKbfsPath(`/private/jdoe`)
target := pathAppend(p, `test1.txt`)
writeRemoteFile(ctx, t, sfs, target, []byte(`foo`))
link := pathAppend(p, `link`)
err := sfs.SimpleFSSymlink(ctx, keybase1.SimpleFSSymlinkArg{
Target: path.Base(target.String()),
Link: link,
})
require.NoError(t, err)
require.Equal(t, "foo",
string(readRemoteFile(ctx, t, sfs, link)))

// Regression for HOTPOT-1276
t.Log("Make sure a link called . will fail")
badLink := pathAppend(p, `.`)
err = sfs.SimpleFSSymlink(ctx, keybase1.SimpleFSSymlinkArg{
Target: path.Base(target.String()),
Link: badLink,
})
require.Error(t, err)
}

func TestList(t *testing.T) {
ctx := context.Background()
config := libkbfs.MakeTestConfigOrBust(t, "jdoe")
Expand Down Expand Up @@ -654,7 +683,9 @@ func readRemoteFile(ctx context.Context, t *testing.T, sfs *SimpleFS, path keyba
Size: de.Size * 2, // Check that reading past the end works.
})
require.NoError(t, err)
require.Len(t, data.Data, de.Size)
if de.DirentType != keybase1.DirentType_SYM {
require.Len(t, data.Data, de.Size)
}

// Starting the read past the end shouldn't matter either.
dataPastEnd, err := sfs.SimpleFSRead(ctx, keybase1.SimpleFSReadArg{
Expand Down

0 comments on commit ea8c9dd

Please sign in to comment.