Skip to content

Commit

Permalink
zap.Open: Invalidate relative path roots
Browse files Browse the repository at this point in the history
Add validation to ensure file schema paths passed to zap.Open are
absolute since this is already documented.

Curently, file schema URIs with relative roots e.g. "file://../"
are parsed to "" by url.Parse and lead to errors when opening a "" path.
Additional validation makes this problem easier to correct.

Tests are also added to demonstrate that double dot segements within
file schema URIs passed to zap.Open remaining within the specified file
hierarchy.

This change addresses https://cwe.mitre.org/data/definitions/23.html

ref #1390

This PR succeeds #1397
  • Loading branch information
r-hang committed Dec 19, 2023
1 parent d27427d commit ba41f97
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 0 deletions.
5 changes: 5 additions & 0 deletions sink.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ func (sr *sinkRegistry) newSink(rawURL string) (Sink, error) {
if err != nil {
return nil, fmt.Errorf("can't parse %q as a URL: %v", rawURL, err)
}

if u.Scheme == schemeFile && !filepath.IsAbs(u.Path) {
return nil, fmt.Errorf("file URI %q attempts a relative path", rawURL)
}

if u.Scheme == "" {
u.Scheme = schemeFile
}
Expand Down
86 changes: 86 additions & 0 deletions writer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,92 @@ func TestOpenOtherErrors(t *testing.T) {
}
}

func TestOpenRelativeValidated(t *testing.T) {
tests := []struct {
msg string
paths []string
wantErr string
}{
{
msg: "invalid relative path root",
paths: []string{
"file:../some/path",
},
// url.Parse's Path for this path value is "" which would result
// in a file not found error if not validated.
wantErr: `open sink "file:../some/path": file URI "file:../some/path" attempts a relative path`,
},
{
msg: "invalid double dot as the host element",
paths: []string{
"file://../some/path",
},
wantErr: `open sink "file://../some/path": file URLs must leave host empty or use localhost: got file://../some/path`,
},
}

for _, tt := range tests {
t.Run(tt.msg, func(t *testing.T) {
_, _, err := Open(tt.paths...)
assert.EqualError(t, err, tt.wantErr)
})
}
}

func TestOpenDotSegmentsSanitized(t *testing.T) {
tempName := filepath.Join(t.TempDir(), "test.log")
assert.False(t, fileExists(tempName))
require.True(t, filepath.IsAbs(tempName), "Expected absolute temp file path.")

tests := []struct {
msg string
paths []string
toWrite []byte
wantFileContents string
}{
{
msg: "no hostname one double dot segment",
paths: []string{"file:/.." + tempName},
toWrite: []byte("a"),
wantFileContents: "a",
},
{
msg: "no hostname two double dot segments",
paths: []string{"file:/../.." + tempName},
toWrite: []byte("b"),
wantFileContents: "ab",
},
{
msg: "empty host name one double dot segment",
paths: []string{"file:///.." + tempName},
toWrite: []byte("c"),
wantFileContents: "abc",
},
{
msg: "empty hostname two double dot segments",
paths: []string{"file:///../.." + tempName},
toWrite: []byte("d"),
wantFileContents: "abcd",
},
}

for _, tt := range tests {
t.Run(tt.msg, func(t *testing.T) {
ws, cleanup, err := Open(tt.paths...)
require.NoError(t, err)
defer cleanup()

_, err = ws.Write(tt.toWrite)
require.NoError(t, err)

b, err := os.ReadFile(tempName)
require.NoError(t, err)

assert.Equal(t, string(b), tt.wantFileContents)
})
}
}

type testWriter struct {
expected string
t testing.TB
Expand Down

0 comments on commit ba41f97

Please sign in to comment.