zap.Open: Invalidate relative paths and paths with ".." segments

[r-hang]

Currently, zap.Open doesn't prevent someone from explicitly doing something like zap.Open("file://../../../secret").

zap.Open already documents that paths passed to it must be absolute. Add validation to error if zap.Open is called with a relative paths that could write files outside of intended file directory hierarchy.

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

ref #1390

[codecov]

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (d27427d) 98.28% compared to head (332853b) 98.34%.

Files	Patch %	Lines
writer.go	81.25%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1397      +/-   ##
==========================================
+ Coverage   98.28%   98.34%   +0.06%     
==========================================
  Files          53       53              
  Lines        3495     3511      +16     
==========================================
+ Hits         3435     3453      +18     
+ Misses         50       48       -2     
  Partials       10       10              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[abhinav]

Can we just change the file:// sink instead?

This is how the sink is created based on scheme:

zap/sink.go

Lines 64 to 72 in 332853b

	func newSinkRegistry() *sinkRegistry {
	sr := &sinkRegistry{
	factories: make(map[string]func(*url.URL) (Sink, error)),
	openFile: os.OpenFile,
	}
	// Infallible operation: the registry is empty, so we can't have a conflict.
	_ = sr.RegisterSink(schemeFile, sr.newFileSinkFromURL)
	return sr
	}

It registers the file:// sink by default.
So when a file:// URL is given, that calls newFileSinkFromURL,
which then calls newFileSinkFromPath:

zap/sink.go

Lines 150 to 159 in 332853b

	func (sr *sinkRegistry) newFileSinkFromPath(path string) (Sink, error) {
	switch path {
	case "stdout":
	return nopCloserSink{os.Stdout}, nil
	case "stderr":
	return nopCloserSink{os.Stderr}, nil
	}
	return sr.openFile(path, os.O_WRONLY|os.O_APPEND|os.O_CREATE, 0o666)
	}

That's the function that finally calls os.OpenFile (through a function reference).
That's where we should have the validation.

[r-hang]

I just experimented and yes this location allows for the reuse of a lot more code.

[abhinav]

if filepath.IsAbs returns true, we're good. It's okay if an absolute path has /../ because that'll just modify the absolute path. /foo/bar/../baz is just /foo/baz.

[r-hang]

@abhinav, I think that means we don't need this feature then?

sr.openFile, given a rawUrl with a file scheme, always consumes an absolute path because url.Parse return object's Path field only contains an absolute path under this constraints. filepath.IsAbs will always return true here. The case where rawUrl isn't passed through url.Parse is also gated by a filepath.IsAbs.

Experimentation:
https://go.dev/play/p/w2VtVnkbIb8

Arbitrary prefix sequences of double dot segments like will be treated as if these sequences of double dot segments were not provided.

sink, _, err := zap.Open("file:///../../Users/rhang/gocode/src/x/zapopen/file") // acts like file:///Users/rhang/...

If that make sense I can close the issue after adding test to confirm this behavior?

[abhinav]

@r-hang can we confirm with a test that it's not possible to provide a relative path or one with '..' to that API?

[r-hang]

#1398 as a successor since I'm not able to reopen this one.