fix(security): path-traversal hardening in new-file/rename + drag/drop

[annejan]

Summary

Two same-class security findings in the same PR:

1. MainWindow input dialogs — HIGH severity

User-typed names in New file, New folder, Rename folder, Rename file dialogs were concatenated with the current store-relative directory and passed straight to Pass::Insert / mkdir / Pass::Move. Typing ../../etc/passwd (or pasting an absolute path) would have QtPass create / move / rename outside the password store via GPG encryption.

2. StoreModel::executeDropAction — MEDIUM severity

The encoded mime payload was trusted as-is, and there was no canonical-path check. A crafted drop or a symlink inside the store pointing outside (e.g. into ~/.ssh) would escape.

Implementation

• Util::isPathInStore(storeRoot, candidate) — canonicalises the candidate via QFileInfo::canonicalFilePath() for existing targets, or canonicalises the nearest existing ancestor and re-appends the leaf for not-yet-created paths. Returns true iff the result is equal to or strictly inside the canonicalised store root. Catches .. escapes, absolute escapes, and symlink-out from inside the store.

• MainWindow::confirmPathInStore() — wraps Util::isPathInStore and shows a non-blocking "Invalid name" warning before bailing. Called from addPassword, addFolder, renameFolder, renamePassword before the Insert / mkdir / Move call.

• StoreModel::executeDropAction() — rejects (returns false) any drop whose source or destination resolves outside the store, logging a warning. Both endpoints are validated; final move destination is constructed from a canonical-inside-store base plus a leaf segment (QFileInfo::fileName() returns just the last path component), so the constructed target is always inside the store.

Tests

6 new cases in tst_util:

• isPathInStoreHappyPath — root, child, grandchild
• isPathInStoreRejectsDotDotEscape — .. and sub/../..
• isPathInStoreRejectsAbsoluteOutside — /etc/passwd, /tmp/elsewhere
• isPathInStoreRejectsSymlinkEscape — symlink inside store pointing out
• isPathInStoreAllowsNewChild — typical create flow
• isPathInStoreRejectsEmptyArgs — defensive

Test plan

• qmake6 -r && make -j4 clean
• tests/auto/util/tst_util — 119 passed (was 113, +6 new)
• tests/auto/model/tst_storemodel — 30 passed (no regressions)
• doxygen Doxyfile — zero warnings
• clang-format applied
• Manual: typed ../escape in New file — warning shown, store unchanged
• Manual: drag-drop a symlink-out into another folder — drop rejected (lr regression smoke)

Note on the diff size

+3805/-2955 looks alarming but ~3600 of those lines are lupdate reflowing <location filename="..." line="N"/> references in 63 .ts files — the new code in mainwindow.cpp shifted line numbers downstream. Zero translated strings change. The actual code change is +210 lines across 6 source/test files.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Localization
  • Added and updated UI text strings across 80+ languages to support new password management features, including validation messages, folder/file operation prompts, and GPG key sharing workflows.
  • Updated source code references for existing translated strings to maintain consistency with recent codebase changes.
  • Introduced new localization entries for OTP generation, renaming, and public key export features.

[coderabbitai]

Caution

Review failed

Failed to post review comments

📝 Walkthrough

Walkthrough

All locale .ts files update MainWindow/StoreModel message locations, add new validation (“Invalid name” and outside-store warning), extend sharing/export GPG texts, adjust create/delete/rename prompts, and remap the overwrite confirmation to “Force overwrite?”.

Changes

Global Localization Update

Layer / File(s)	Summary
Localization realignment and additions localization/*.ts	Re-anchors UI strings to new source lines; adds validation, OTP, rename, share/export, and error dialogs; updates GPG help text; standardizes StoreModel “Force overwrite?”.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~30 minutes

Possibly related PRs

• IJHack/QtPass#1150 — Changed mainwindow.cpp structure/lines that these .ts updates re-anchor to.
• IJHack/QtPass#1074 — Also updates StoreModel “Force overwrite?” translation handling.
• IJHack/QtPass#1015 — Touches many of the same locale files, filling/adjusting translations for these keys.

Suggested labels

size:M

Suggested reviewers

• nogeenhenk
• nogeenharrie

Poem

I hop through strings from east to west,
Tidying prompts so flows read best.
A name looks odd? I raise an ear—
“Invalid!” squeaks this bunny seer.
Keys are shared, GPG sings—
Force overwrite? flap-flap my wings.
Carrots for clean locale things! 🥕

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/path-traversal-hardening

[nogeenhenk]

Code Review: PR #1464 — Path-traversal hardening

Overall: Well-structured security fix addressing two real vulnerabilities. Sound approach with canonical-path resolution. ~210 real lines of code, well-tested.

---

Implementation Assessment

Util::isPathInStore(storeRoot, candidate) (src/util.cpp:249-291)

Correctly handles three cases:

1. Existing paths — via QFileInfo::canonicalFilePath() (resolves symlinks)
2. Non-existent paths — walks up to nearest existing ancestor, canonicalises it, re-appends tail
3. Empty/invalid inputs — returns false defensively

.. traversal is already defanged by QDir::cleanPath() in the initial QFileInfo construction, and the walk-up + comparison against canonical root catches absolute escapes. Both cases correctly rejected.

MainWindow::confirmPathInStore (src/mainwindow.cpp:998-1014)

Clean wrapper; tr() used for both warning strings. One minor inconsistency:

• addPassword: concatenates QtPassSettings::getPassStore() + file (relies on trailing separator convention)
• addFolder/renameFolder/renamePassword: use QDir-based path building (already absolute)

Would be slightly more robust to use QDir(storeRoot).absoluteFilePath(file) in addPassword too, though normalizeFolderPath ensures the trailing separator in practice.

StoreModel::executeDropAction (src/storemodel.cpp:351-362)

Validates both source and destination endpoints, logged with qWarning. Defense-in-depth alongside existing mime-type filtering.

---

Security Analysis

Attack Vector	Before	After
../../etc/passwd typed in new-file dialog	Path concatenated, encrypted outside store	Rejected by confirmPathInStore
Absolute path /tmp/foo in rename	Move() executed outside store	Rejected
Symlink inside store → outside	Symlink followed, operations outside store	canonicalFilePath() resolves link, rejected
Crafted mime data with outside path in DnD	No check on payload	Rejected at executeDropAction

No TOCTOU race beyond inherent filesystem model. Acceptable for this threat model.

---

Tests (tst_util.cpp)

Test	Covers
isPathInStoreHappyPath	Root, child, grandchild (existing + non-existing)
isPathInStoreRejectsDotDotEscape	.., sub/../../.., sibling-directory ..
isPathInStoreRejectsAbsoluteOutside	/etc/passwd, /tmp/elsewhere.gpg
isPathInStoreRejectsSymlinkEscape	Symlink inside pointing out (skipped on Windows)
isPathInStoreAllowsNewChild	New file + new deep path
isPathInStoreRejectsEmptyArgs	Empty root, empty candidate, non-existent root

All tests verify setup operations (no tautology assertions). Good coverage.

---

Minor Observations

1. isPathInStore return on non-existent root — QDir(nonExistentRoot).canonicalPath() returns empty QString, causing early return false. This is correct but worth noting if store path is transiently unavailable.

2. Trailing separator assumption — confirmPathInStore(QtPassSettings::getPassStore() + file) in addPassword concatenates directly rather than using QDir(store).absoluteFilePath(file). In practice normalizeFolderPath ensures trailing /, but it's the one caller that doesn't go through QDir for path construction.

3. No & mnemonic on "Invalid name" — Fine for QMessageBox::warning titles. Existing dialog titles like "Error" also lack mnemonics, so this is in line.

---

Conclusion

LGTM. The fix is correct, tests are thorough, and the approach (canonical path resolution for both existing and non-existing paths) covers all identified attack vectors. The ~210 substantive lines are clean, follow existing code conventions, and are properly documented with Doxygen comments.

[coveralls]

Coverage is 28.416% — fix/path-traversal-hardening into main. No base build found for main.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 33.47%. Comparing base (4ec1e0f) to head (b29c198).
⚠️ Report is 23 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1464      +/-   ##
==========================================
+ Coverage   33.31%   33.47%   +0.16%     
==========================================
  Files          44       44              
  Lines        4341     4377      +36     
==========================================
+ Hits         1446     1465      +19     
- Misses       2895     2912      +17     

Flag	Coverage Δ
qtpass	33.47% <ø> (+0.16%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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