fix(security): chmod 0600 on .gpg-id after write#1465
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR sets owner-only read/write permissions on created ChangesGPG-ID File Permissions Security Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/auto/util/tst_util.cpp`:
- Around line 2216-2222: Replace the bare QVERIFY() assertions that check
QFile::Permissions in the test (the local variable perms from
QFile(gpgIdFile).permissions()) with QVERIFY2() calls that include descriptive
messages so failing tests show which specific flag is wrong; for each check
(testFlag(QFile::ReadOwner), WriteOwner, ReadGroup, WriteGroup, ReadOther,
WriteOther) update the assertion to QVERIFY2(perms.testFlag(...), "expected
<flag> to be set" or "expected <flag> to be unset") so diagnostics clearly
identify which permission failed.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a911a0d-e017-416b-a7af-e14ecad8d8f0
📒 Files selected for processing (3)
src/imitatepass.cppsrc/mainwindow.cpptests/auto/util/tst_util.cpp
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Failed to clone repository into sandbox. Please try again. |
|
coverage: 28.601% (+0.2%) from 28.416% — fix/gpg-id-permissions into main |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/auto/util/tst_util.cpp`:
- Around line 2197-2215: Two assertions in the test block use bare QVERIFY(...)
and must be converted to QVERIFY2(...) with descriptive messages: replace
QVERIFY(tempDir.isValid()) (verify tempDir validity) with
QVERIFY2(tempDir.isValid(), "tempDir should be valid after creation") and
replace QVERIFY(QFile::exists(gpgIdFile)) (verify the gpg id file was written by
pass.writeGpgIdFile) with QVERIFY2(QFile::exists(gpgIdFile), "gpg id file must
exist after writeGpgIdFile"); locate these around the tempDir/gpgIdFile setup
and the call to ImitatePass::writeGpgIdFile and update the assertions to include
the provided messages.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: a73bc7e1-8458-421e-b982-fe70e052b4b3
📒 Files selected for processing (1)
tests/auto/util/tst_util.cpp
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/auto/util/tst_util.cpp`:
- Around line 2202-2213: Introduce an RAII umask guard (e.g., class UmaskGuard
with ctor that calls ::umask(newMask) and stores oldMask_ and a dtor that
restores ::umask(oldMask_); delete copy ops) and use it around the test code
that currently calls ::umask(0022) / ::umask(oldUmask): construct UmaskGuard
guard(0022) before creating UserInfo/ImitatePass and calling
ImitatePass::writeGpgIdFile(...) and remove the explicit ::umask(oldUmask)
restore; this mirrors the SshAuthSockGuard pattern used elsewhere and ensures
automatic restoration if writeGpgIdFile throws.
- Around line 2216-2222: Add assertions to verify execute bits are unset: use
the existing perms variable (from QFile(gpgIdFile).permissions()) and add
QVERIFY2(!perms.testFlag(QFile::ExeOwner), "expected ExeOwner to be unset"),
QVERIFY2(!perms.testFlag(QFile::ExeGroup), "expected ExeGroup to be unset"), and
QVERIFY2(!perms.testFlag(QFile::ExeOther), "expected ExeOther to be unset") so
the test explicitly checks that execute permissions are not set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: b60e6d37-22ad-47ce-a050-df2a903bc65e
📒 Files selected for processing (1)
tests/auto/util/tst_util.cpp
The .gpg-id file lists the GPG key IDs (often full fingerprints) that a password store / sub-tree is encrypted to. Default-umask creation leaves it world-readable (0644 with the typical 0022 umask). While the standard ~/.password-store is itself 0700 and protects the file in practice, users who relocate the store onto an NFS share, an SMB mount, a USB stick, or any FS where the parent directory's mode is more permissive lose that protection. The .gpg-id leaks both who can decrypt the store and (for full fingerprints) the long-term identity of the recipients. Lock the file to owner-only access (0600 on Unix; best-effort no-op on Windows) immediately after close. Applied to both write sites: - ImitatePass::writeGpgIdFile (used when the recipient list is edited through the Users dialog) - MainWindow::addFolder (used when "Add a .gpg-id when creating a new folder" is enabled in settings) Tests: - tst_util::writeGpgIdFileSetsOwnerOnlyPerms — forces umask 0022, calls the production writeGpgIdFile path, asserts QFile::permissions is ReadOwner|WriteOwner only. Unix-only (Qt's permission bits don't round-trip on Windows). Build clean, 114/114 util tests pass, doxygen zero warnings. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
annejan
force-pushed
the
fix/gpg-id-permissions
branch
from
2b44e73 to
2850c36
Compare
Defense-in-depth — the production writeGpgIdFile uses ReadOwner|WriteOwner exclusively, so the execute bits are always unset by construction; assert that explicitly so a future change to the permission mask can't silently leave the file executable. Reviewer nit from CodeRabbit (#1465). Skipping the sibling RAII-umask suggestion: Qt Test doesn't throw on assertion failures and the umask is restored on the next line, so the theoretical risk doesn't justify the boilerplate. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/imitatepass.cpp (1)
256-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSwitch both
.gpg-idwrite paths toQSaveFilefor atomic writes.Two functions currently use
QFilewith manualsetPermissions(), creating a window where the.gpg-idfile can be left corrupted if the process is interrupted during write/close. Follow the existing pattern inUtil::writeTemplates: useQSaveFile, callsetPermissions()beforecommit(), and check thecommit()return value. This applies to:
ImitatePass::writeGpgIdFile()(lines 256–277)MainWindow::addFolder()(lines 1487–1533, the second.gpg-idwrite path)Pattern reference from Util::writeTemplates
QSaveFile saveFile(path); if (!saveFile.open(QIODevice::WriteOnly | QIODevice::Text)) { return false; } // ... write operations ... out.flush(); if (out.status() != QTextStream::Ok) { return false; } return saveFile.commit(); // atomic: renames temp file only on success🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/imitatepass.cpp` around lines 256 - 277, Replace the QFile-based writes of .gpg-id with QSaveFile in ImitatePass::writeGpgIdFile (and the second .gpg-id write path in MainWindow::addFolder) following the Util::writeTemplates pattern: open a QSaveFile, write the key lines, flush and check QTextStream::status(), call QFile::setPermissions on the temporary save file before calling saveFile.commit(), and verify commit() returned true; if any step fails, emit the same error/return as the existing code.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/imitatepass.cpp`:
- Around line 256-277: Replace the QFile-based writes of .gpg-id with QSaveFile
in ImitatePass::writeGpgIdFile (and the second .gpg-id write path in
MainWindow::addFolder) following the Util::writeTemplates pattern: open a
QSaveFile, write the key lines, flush and check QTextStream::status(), call
QFile::setPermissions on the temporary save file before calling
saveFile.commit(), and verify commit() returned true; if any step fails, emit
the same error/return as the existing code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: ba723a88-f444-421e-954b-dfa55c07979e
📒 Files selected for processing (3)
src/imitatepass.cppsrc/mainwindow.cpptests/auto/util/tst_util.cpp
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1465 +/- ##
==========================================
+ Coverage 33.47% 33.72% +0.25%
==========================================
Files 44 44
Lines 4377 4379 +2
==========================================
+ Hits 1465 1477 +12
+ Misses 2912 2902 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
CodeRabbit's auto-fix and my follow-up exceeded the 80-column limit on three of the new QVERIFY2 calls. Reformatted to break after the condition; matches the style applied elsewhere in the file. No semantic change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary
.gpg-idlists the GPG key IDs (often full fingerprints) that a password store sub-tree is encrypted to. Default-umask creation leaves it world-readable (0644 with the typical 0022 umask).While
~/.password-storeitself is normally0700and protects the file in practice, users who relocate the store onto NFS / SMB / USB / a FS with a more permissive parent mode lose that protection. The file leaks both who can decrypt the store and (for full fingerprints) the long-term identity of the recipients.Change
Apply
QFile::setPermissions(ReadOwner | WriteOwner)(= 0600 on Unix; best-effort no-op on Windows) immediately after theclose()call. Two write sites:ImitatePass::writeGpgIdFile— recipient edits via the Users dialogMainWindow::addFolder— when "Add a .gpg-id when creating a new folder" is enabled in settingsTests
tst_util::writeGpgIdFileSetsOwnerOnlyPerms— forcesumask 0022, runsImitatePass::writeGpgIdFile, assertsQFile::permissionsisReadOwner|WriteOwneronly. Unix-only (Qt's permission bits don't round-trip through Windows ACLs).Test plan
qmake6 -r && make -j4cleantests/auto/util/tst_util— 114 passed (was 113, +1 new)doxygen Doxyfile— zero warningsclang-formatappliedls -la .gpg-idshows-rw-------Diff
3 files changed, 55 insertions (≈9 lines of production code + comments, the rest is the new test).
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests