Enhance filename validation #23386

cocopaw · 2025-10-16T09:32:50Z

Enhance filename validation

This PR extends [#23060] and implements the following key improvements:

Refactor isValidFileName
Enhance isValidPath to use new logic
Enhance toValidFileName to use new logic
Enhance toValidPath to use new logic
Enhance Torrent Creator to use new logic

Testing

Tested: Windows 11 + Linux Mint 22.1

Fixes: #22426 #21899 #16906 #16834 #16271 #11276 #9085 #11340 #14728 #20412 #20337 #17049 #17469 #15378 #15227 #14908 #20742

…eName

glassez · 2025-10-16T12:01:58Z

src/base/bittorrent/sessionimpl.cpp

        else
        {
-            path = Utils::Fs::toValidPath(categoryName);
+            path = Path(Utils::Fs::toValidFileName(categoryName));


Here, the category name can represent a path, so you seem to have broken it.

I couldnt fault itbwhen I was testing. I also tested subcategories. Can u give me an example scenario that would break it?

I couldnt fault itbwhen I was testing. I also tested subcategories. Can u give me an example scenario that would break it?

Did you test it with "flat" categories (i.e. when subcategories is disabled)?

I tested it with both flat categories and subcategories.

I tested it with both flat categories and subcategories.

Name like some/category/name is supposed to represent corresponding path some/category/name. Doesn't toValidFileName() replace slashes?

Ok give me a few days and ill see what I can do.

Are you ok with Path.isvalid() being moved to fs utils and reworked too ?

Are you ok with Path.isvalid() being moved to fs utils and reworked too ?

Ok.

This is just a minor housekeeping side suggestion that I noticed whilst working on fs.cpp.

Are you ok with this suggestion too?

I don't mind.

Suggestions implemented as discussed.

src/base/bittorrent/sessionimpl.cpp

src/base/utils/fs.cpp

qBittUser · 2025-10-16T13:55:40Z

1st commit title has typo: Refactor isValidName. Enahnce toValidFileName.

Should be written Enhance instead of Enahnce.

Co-authored-by: Vladimir Golovnev <[email protected]>

src/base/bittorrent/sessionimpl.cpp

src/base/bittorrent/torrentcreator.cpp

src/base/utils/fs.cpp

…Action

…atforms)

cocopaw · 2025-10-20T03:15:00Z

@Chocobo1 @glassez , ready for review.

Additional Notes:

Incorrect terminology "Relevant issues:" had been used in previous PR Block invalid file names when renaming torrent content #23060 to list the cases to close on merge. In order to address this I have checked all the relevant cases and added them via "Fixes:" here.

And outlier to this is Torrents with content names containing backslash (\) are mishandled on linux #17752 This issue was addressed via related Libtorrent PR of mine libtorrent issue 8014. The issue is in "Closed as not planned" status (incorrect) and locked to collaborators. Could someone please update it to add mention of libtorrent issue 8014 and mark it as fixed?
For the commit Remove Utils::Fs::isValidFileName from TorrentsController::renameFileAction, the functions below invoke torrent->renameFile, which now performs validation via the new Path.isValid(). The prior isValidName checks in torrentcontentmodel.cpp are therefore redundant and have been removed.

src/gui/torrentcontentmodel.cpp
..
TorrentsController::renameFileAction
TorrentsController::renameFolderAction

src/base/utils/fs.cpp

src/base/bittorrent/torrentcreator.cpp

Chocobo1 · 2025-10-26T14:36:15Z

src/webui/api/torrentscontroller.cpp

-    const QString newFileName = QFileInfo(params()[u"newPath"_s]).fileName();
-    if (!Utils::Fs::isValidName(newFileName))
-        throw APIError(APIErrorType::Conflict, tr("File name has invalid characters"));
-


Why remove it? It should be checked anyway.

Becuase it's a redundant check now. It is already checked downstream via BitTorrent::AbstractFileStorage::renameFile. Now that Path.isValid() is using the new logic we not longer need to check it via Utils::Fs::isValidName in torrentscontroller.cpp.

The error checking is also better via BitTorrent::AbstractFileStorage::renameFile as it is more descriptive (it specifies if the oldpath or newpath is invalid instead of just the whole thing)

Process Flow:

TorrentsController::renameFileAction() -> torrent->renameFile -> BitTorrent::AbstractFileStorage::renameFile -> oldPath.isValid()

void BitTorrent::AbstractFileStorage::renameFile(const Path &oldPath, const Path &newPath) { if (!oldPath.isValid()) throw RuntimeError(tr("The old path is invalid: '%1'.").arg(oldPath.toString())); if (!newPath.isValid()) throw RuntimeError(tr("The new path is invalid: '%1'.").arg(newPath.toString())); if (newPath.isAbsolute()) throw RuntimeError(tr("Absolute path isn't allowed: '%1'.").arg(newPath.toString()));

My previous comment (for reference):

For the commit "Remove Utils::Fs::isValidFileName from TorrentsController::renameFileAction", the functions below invoke "torrent->renameFile", which now performs validation via the new "Path.isValid()". The prior "isValidName" checks in torrentcontentmodel.cpp are therefore redundant and have been removed. src/gui/torrentcontentmodel.cpp .. TorrentsController::renameFileAction TorrentsController::renameFolderAction

Chocobo1 · 2025-10-26T14:36:28Z

src/webui/api/torrentscontroller.cpp

-    const QString newFolderName = QFileInfo(params()[u"newPath"_s]).fileName();
-    if (!Utils::Fs::isValidName(newFolderName))
-        throw APIError(APIErrorType::Conflict, tr("Folder name has invalid characters"));
-


Chocobo1 · 2025-10-26T14:39:41Z

src/base/path.cpp

-    const QRegularExpression regex {u"\\0"_s};
-#endif
-    return !m_pathStr.contains(regex);
+    return Utils::Fs::isValidPath(*this);


Seems to me that this change completely messed up Path class design.
I cannot understand the changes anymore. I'll stop here until things clears up.

This was agreed in the first review on this PR with glassez:

The thinking was to consolodate all file validation logic to FS Utils. This also has the benefit of allowing all related functions to share validation logic via the unamed namespace in fs.cpp.

I could just remove path.isValid() entirely from path.h and path.cpp and then change whatever was calling that to call Utils::Fs::isValidPath instead if preffered? *

* The reason I didn't do this initially is that:

It is more changes

I figured it would be good to keep the path.isValid method in path.h\path.cpp so that people can see it is available just by looking at path.h\path.cpp.

Can you expand on what you mean by "this change completely messed up Path class design" ?

cocopaw added 7 commits October 15, 2025 09:07

Refactor isValidName. Enahnce toValidFileName

d55c5fa

Improve torrent file path sanitization in TorrentCreator

4f46011

Enhance toValidPath to match toValidFileName sanitization

7b94f38

Refactor to reduce code duplication. Rename isValidName to isValidFil…

5c5d6e5

…eName

Enhance "." check and remove toValidPath

425d72e

Minor formatting changes

5c9ea79

Fix CON_1CON bug

7fbd058

glassez reviewed Oct 16, 2025

View reviewed changes

src/base/bittorrent/sessionimpl.cpp Outdated Show resolved Hide resolved

glassez reviewed Oct 16, 2025

View reviewed changes

src/base/utils/fs.cpp Outdated Show resolved Hide resolved

glassez reviewed Oct 16, 2025

View reviewed changes

src/base/utils/fs.cpp Outdated Show resolved Hide resolved

Remove *Internal from isValidFileName/toValidFileName

34b4ebb

cocopaw and others added 3 commits October 17, 2025 01:00

Reinstate toValidPath

cc22407

Update src/base/utils/fs.cpp

ccc47cd

Co-authored-by: Vladimir Golovnev <[email protected]>

Move Path::isValid() to Utils::Fs::isValidPath

2ff23a6

Chocobo1 reviewed Oct 18, 2025

View reviewed changes

cocopaw added 8 commits October 18, 2025 19:56

Rewrite Utils::Fs::isValidPath

4c6dde4

Chocobo1 review requests

ce6dba5

Rewrite Utils::Fs::toValidPath

2327d43

Remove Utils::Fs::isValidFileName from TorrentsController::renameFile…

bdd88de

…Action

Silence unused parameter warning in isDriveLetterPath (non-Windows pl…

90a1973

…atforms)

Fix Path::isValid() failing on "/"

c1fc482

Add ":" checking in OSX

af9d8ac

Use 'Q_OS_MACOS' instead of 'Q_OS_OSX' (depricated)

4371723

Chocobo1 added the Core label Oct 20, 2025

Chocobo1 reviewed Oct 26, 2025

View reviewed changes

cocopaw added 2 commits October 27, 2025 11:49

Chocobo1 review changes

b573f84

Fix CI error "isReservedCharacter not all control paths return a value"

1eec288

Uh oh!

Enhance filename validation #23386

Are you sure you want to change the base?

Enhance filename validation #23386

Uh oh!

Conversation

cocopaw commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Enhance filename validation

Testing

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qBittUser commented Oct 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cocopaw commented Oct 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cocopaw Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cocopaw commented Oct 16, 2025 •

edited

Loading

cocopaw commented Oct 20, 2025 •

edited

Loading

cocopaw Oct 27, 2025 •

edited

Loading