feat(fscache): add umask for file and dir permission control

[twpayne]

Firstly, thank you very much for this very nice library!

I'm using this library for a per-user cache in chezmoi. I'd like to ensure that all files and directories created by the fscache store are only readable by the user who owns that cache.

This PR adds a fscache.WithUmask / ?umask= option to fscache that allows the application to exclude permissions from files and directories created by fscache. For example, fscache.WithUmask(0o077) will ensure that the created files and directories never have group or world read/write/execute permissions.

[bartventer]

hey thanks for putting this together and also for the kind words about the lib 👍

i approved the workflows and they did surface a couple things that probably need fixing before merge

the windows test is failing because go only really respects the owner-write bit when you call Chmod on windows. group and world bits don’t actually get enforced there. i think this probably means the option either needs to be explicitly unsupported on windows or only enabled on unix-like systems with a clear note in the docs. since it’s security related, it feels a bit risky to have it appear to work on a platform where it can’t really guarantee anything

also the lint run picked up an err shadowing issue around the f.Stat() call in set, should be a quick fix

one other small thing i noticed, invalid DSN values currently just end up as a zero mask. so a typo in ?umask= would effectively disable restrictions without any error. might be better to fail fast there and return an error for invalid or out of range values

overall though the approach looks solid on unix once those bits are addressed, and it’d be great to get this in for the chezmoi integration. really appreciate the work on it 👍

[twpayne]

Thanks for the review. I've addressed all of your comments. I've force-pushed to keep the commit history clean, but you can see the changes here: https://github.com/bartventer/httpcache/compare/c989ff9770865701932e0ac0ae63324ff8b037fb..3c3f4dc8df358a52a2faeb6f6da51318d4f52e7d.

[bartventer]

thanks, i checked the latest changes and the lint fix plus the malformed DSN handling look good

there’s still one thing on the windows side that needs addressing before merge though. WithUmask still succeeds even though the requested group and world restrictions can’t actually be enforced. skipping the windows assertion makes the tests pass, but callers can still think the option is protecting the cache when it isn’t

could you make non-zero umasks explicitly unsupported on windows and return a clear error from both the programmatic and DSN paths? the windows test should then verify that error instead of skipping the feature entirely. the package docs and example should also make the platform limitation clear

there are also two small edge cases left: ?umask= is still treated as if it wasn’t supplied, and direct calls to WithUmask don’t reject values above 0777. please add coverage for both cases as well

once those are addressed i think this should be ready to merge

[twpayne]

OK, done. Changes since last view are at https://github.com/bartventer/httpcache/compare/3c3f4dc8df358a52a2faeb6f6da51318d4f52e7d..47537fb9aa8b8538f5aa2151a58863ccc3694b60.

there are also two small edge cases left: ?umask= is still treated as if it wasn’t supplied

Fixed, but note that this required changes to the URL parsing logic that makes it different to the way other query parameters are parsed. Specifically, the only way to differentiate between the umask value being absent and the value being empty is by looking at the array of values. Other query parameters are not treated this way: they all treat an empty value as no value.

[bartventer]

the windows handling and range checks look good.

re the empty value, Get can’t tell missing from empty, but Values.Has can. pairing Has with Get would also keep the same first-value behaviour as the other query params.

i put together a small playground showing it: https://go.dev/play/p/MxopTyj7Tdu

query := u.Query()
if query.Has("umask") {
	umask, err := parseUmask(query.Get("umask"))
	if err != nil {
		return nil, err
	}
	opts = append(opts, WithUmask(umask))
}

the current loop changes what happens with repeated values. so ?umask=077&umask=000 applies both and 000 wins. i don’t think we want that to behave differently from the other params.

the package-level “Private cache files and directories” example also still needs a note that non-zero umasks aren’t supported on windows.

once those are sorted, this is good to merge.

[twpayne]

Done :)

[bartventer]

LGTM

[twpayne]

Thank you for the speedy and thorough review!

[bartventer]

thanks again for the great contribution, really appreciate it :)