Harden config directory creation permissions to 0700#25
Conversation
Agent-Logs-Url: https://github.com/Balaji01-4D/pgxcli/sessions/f462ef04-e39b-4665-b035-3987b6e3921b Co-authored-by: Balaji01-4D <172587380+Balaji01-4D@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Balaji01-4D/pgxcli/sessions/f462ef04-e39b-4665-b035-3987b6e3921b Co-authored-by: Balaji01-4D <172587380+Balaji01-4D@users.noreply.github.com>
0700
There was a problem hiding this comment.
Pull request overview
Security hardening to ensure user config parent directories are created with restrictive permissions when bootstrapping the default config.
Changes:
- Change
SaveConfigto create parent directories with mode0700instead ofos.ModePerm. - Add a test asserting restrictive permissions when saving into a nested config path.
- Run
go mod tidy-like dependency cleanup reflected ingo.mod/go.sum.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/config/config.go | Uses os.MkdirAll(dir, 0o700) to create private config directories. |
| internal/config/config_test.go | Adds a permissions-focused test for nested config saves. |
| go.mod | Removes/reclassifies some dependencies (unrelated to config perms). |
| go.sum | Updates checksums consistent with dependency changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestSaveConfig_CreatesDirWithRestrictivePermission(t *testing.T) { | ||
| tempDir := t.TempDir() | ||
| configPath := path.Join(tempDir, "nested", "config.toml") | ||
|
|
||
| err := SaveConfig(configPath) | ||
| assert.NoError(t, err) | ||
|
|
||
| info, err := os.Stat(path.Dir(configPath)) | ||
| assert.NoError(t, err) | ||
| assert.Equal(t, os.FileMode(0o700), info.Mode().Perm()) | ||
| } |
There was a problem hiding this comment.
This permission assertion will be unreliable on Windows (and some non-POSIX filesystems), where os.MkdirAll permission bits are ignored and os.Stat(...).Mode().Perm() is synthesized (often reporting 0777). Since CI runs go test on windows-latest, this test is likely to fail there. Consider skipping this test on Windows (e.g., via runtime.GOOS) and/or asserting only that group/other bits are not set instead of strict equality to 0700 (to be resilient to unusual umask/filesystem behavior).
There was a problem hiding this comment.
@copilot apply changes based on this feedback. skip for the windows
There was a problem hiding this comment.
Applied. The permission test now skips on Windows using runtime.GOOS == "windows" to avoid unreliable mode-bit assertions there. Commit: c41a4d3.
| require ( | ||
| github.com/BurntSushi/toml v1.6.0 | ||
| github.com/balaji01-4d/pgxspecial v0.2.1 | ||
| github.com/fatih/color v1.19.0 | ||
| github.com/google/shlex v0.0.0-20191202100458-e7afc7fbc510 | ||
| github.com/jedib0t/go-pretty/v6 v6.7.9 | ||
| github.com/jedib0t/go-prompter v0.1.0 | ||
| github.com/mattn/go-runewidth v0.0.22 | ||
| github.com/muesli/termenv v0.16.0 | ||
| github.com/olekukonko/tablewriter v1.1.4 | ||
| github.com/pganalyze/pg_query_go/v6 v6.2.2 | ||
| github.com/spf13/cobra v1.10.2 | ||
| github.com/stretchr/testify v1.11.1 |
There was a problem hiding this comment.
The PR description says the scope is limited to internal/config/config.go, but this PR also includes dependency graph changes in go.mod/go.sum (e.g., removing olekukonko/tablewriter and reclassifying some dependencies as indirect). Please either update the PR description to mention the go mod tidy/dependency cleanup, or split these dependency changes into a separate PR to keep the security hardening change focused.
There was a problem hiding this comment.
Done. I restored go.mod and go.sum to the pre-7b879c0 state so this PR stays focused on config permission hardening. Commit: 7972728.
| func TestSaveConfig_CreatesDirWithRestrictivePermission(t *testing.T) { | ||
| tempDir := t.TempDir() | ||
| configPath := path.Join(tempDir, "nested", "config.toml") | ||
|
|
||
| err := SaveConfig(configPath) | ||
| assert.NoError(t, err) | ||
|
|
||
| info, err := os.Stat(path.Dir(configPath)) | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
Using the import alias path for the path/filepath package (and then calling path.Join/path.Dir) is easy to confuse with the standard library path package, especially as this file grows. Consider importing path/filepath as filepath (or without an alias) for clarity.
There was a problem hiding this comment.
Done. I replaced the path/filepath alias with filepath and updated usages in internal/config/config_test.go for clarity. Commit: 7972728.
Agent-Logs-Url: https://github.com/Balaji01-4D/pgxcli/sessions/b0931183-9ab3-432c-a30a-e914aa3f8cb4 Co-authored-by: Balaji01-4D <172587380+Balaji01-4D@users.noreply.github.com>
Agent-Logs-Url: https://github.com/Balaji01-4D/pgxcli/sessions/f357908d-e7a4-4e2d-8299-f09bac877e53 Co-authored-by: Balaji01-4D <172587380+Balaji01-4D@users.noreply.github.com>
…8d-e7a4-4e2d-8299-f09bac877e53 Co-authored-by: Balaji01-4D <172587380+Balaji01-4D@users.noreply.github.com>
path/filepathalias in config tests for clarity