fix: restrict config file permissions to owner only#72
Conversation
Set config directory to 0o700 and config file to 0o600 to prevent other users from reading stored API tokens in multi-user environments.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f20a919d9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fs.mkdirSync(CONFIG_DIR, { recursive: true, mode: 0o700 }); | ||
| } | ||
| fs.writeFileSync(CONFIG_FILE, JSON.stringify(data, null, 2)); | ||
| fs.writeFileSync(CONFIG_FILE, JSON.stringify(data, null, 2), { mode: 0o600 }); |
There was a problem hiding this comment.
This call only enforces 0o600 when config.json is newly created; if the file already exists (the common upgrade path), writeFileSync preserves its current mode, so previously permissive files like 0644 remain readable by other users in multi-user environments. The fix needs an explicit chmod path for existing files after writing.
Useful? React with 👍 / 👎.
| function saveConfigFile(data) { | ||
| if (!fs.existsSync(CONFIG_DIR)) { | ||
| fs.mkdirSync(CONFIG_DIR, { recursive: true }); | ||
| fs.mkdirSync(CONFIG_DIR, { recursive: true, mode: 0o700 }); |
There was a problem hiding this comment.
Using mkdirSync(..., { recursive: true, mode: 0o700 }) does not correct permissions on an already-existing ~/.confluence-cli directory, so installs with older/broader directory modes (for example 0755) keep those permissions and are not fully hardened. Add an explicit chmod for the existing directory case.
Useful? React with 👍 / 👎.
pchuri
left a comment
pchuri
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @tokuhirom! Restricting config file permissions is an important security improvement.
I have one main concern:
Existing files/directories are not remediated
writeFileSync with { mode: 0o600 } and mkdirSync with { mode: 0o700 } only apply permissions when the file/directory is newly created. If a user already has config.json at 0644 or ~/.confluence-cli/ at 0755 (the common upgrade path), those permissions remain unchanged after this patch.
Suggested fix — add explicit chmodSync calls:
function saveConfigFile(data) {
if (!fs.existsSync(CONFIG_DIR)) {
fs.mkdirSync(CONFIG_DIR, { recursive: true, mode: 0o700 });
} else {
fs.chmodSync(CONFIG_DIR, 0o700);
}
fs.writeFileSync(CONFIG_FILE, JSON.stringify(data, null, 2), { mode: 0o600 });
fs.chmodSync(CONFIG_FILE, 0o600);
}This ensures that every save operation tightens permissions regardless of whether the file/directory existed before.
It would also be great to add test coverage for the chmodSync calls on existing files.
Other than that, the test refactoring (captureConfigWrite destructuring) looks clean. Nice work!
Add explicit chmodSync calls in saveConfigFile so that pre-existing ~/.confluence-cli/ directories and config.json files created before this fix (typically 0755/0644) are tightened to 0o700/0o600 on every save. Also add test coverage for chmodSync on existing files.
|
@pchuri thanks, i added the fix. |
|
Hi @tokuhirom, you mentioned that you added the fix, but I can only see the original commit (3f20a91). Could you push the follow-up commit with the |
pchuri
left a comment
pchuri
left a comment
There was a problem hiding this comment.
LGTM! The follow-up commit properly addresses the feedback — chmodSync now enforces correct permissions on existing files/directories on every save. Test coverage looks solid too.
## [1.27.1](v1.27.0...v1.27.1) (2026-03-11) ### Bug Fixes * restrict config file permissions to owner only ([#72](#72)) ([25ec54d](25ec54d))
|
🎉 This PR is included in version 1.27.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
i forgot push. thanks for merging :) |
2 participants
Description
Set config directory to 0o700 and config file to 0o600 to prevent other users from reading stored API tokens in multi-user environments.
Type of Change
Testing
Checklist
Screenshots (if applicable)
Additional Context