error if invalid chars in wallet name #20393

check if wallet name begins with one of the below chars and give error:

/ \ . ? %

Fixes https://github.com/bitcoin/bitcoin/issues/20128

PR is still in draft mode and I am not sure if this is the right approach (concept ACK). Not even sure if I have used the if statement at the right place because I tried compiling (no errors), tests passed but I was still able to create wallet name with invalid characters.

Any help in solving this problem will be appreciated. I referred to https://github.com/bitcoin/bitcoin/commit/ba1f128d6c117a63d5d904b3956551bd83405ec9 so assuming I will have to make few changes in src/wallet/wallet.cpp and write a test for it in functional/wallet_createwallet.py

Or I can close this PR if someone has a better approach, code to solve this problem and has the time to work on a new PR

What about if it begins with -? That can happen accidentally using bitcoin-cli you e.g. try something like bitcoin-cli createwallet -descriptors instead of bitcoin-cli -named createwallet -descriptors true?

Also, if you're just checking the first character, isn't regex a bit overkill?

What about waiting a few months and then you can use std::filesystem to check that std::filename::filename equals the original path, I think that would ensure it isn't traversing directories, which seems to be the goal.

What about waiting a few months and then you can use std::filesystem to check that std::filename::filename equals the original path, I think that would ensure it isn't traversing directories, which seems to be the goal.

I am okay with it and don't consider it critical if thats a better approach.

Also I am not sure why checks failed in CI but I get no errors on my local machine with same code.

What about if it begins with -? That can happen accidentally using bitcoin-cli you e.g. try something like bitcoin-cli createwallet -descriptors instead of bitcoin-cli -named createwallet -descriptors true?

Also, if you're just checking the first character, isn't regex a bit overkill?

- character is not checked and considered valid. Only / and . are checked.

I don't know any better way to do this in C++

what about string::front?

what about string::front?

This can be used IMO. Any reasons to avoid regex?

Going forward please add tests.

https://www.cplusplus.com/reference/string/string/find_first_of/

Although it would be best if there's a filesystem-specific call, since this may vary by OS... (see test/functional/feature_notifications.py:FILE_CHARS_DISALLOWED)

Any reasons to avoid regex?

Execution speed and readability of code.

Agree with @luke-jr about filesystem specific call being the best solution. And as there is anyway a plan to use C++17 for the next major release after v0.21, std::filename::filename looks like a right solution to me.

@ysangkok replaced regex with string::front @promag added tests in https://github.com/bitcoin/bitcoin/pull/20393/commits/c04c47384533b15214859fc5e2f3d4d578da2ebe although I am not sure if they are correct and doing this for the first time @luke-jr

Although it would be best if there's a filesystem-specific call, since this may vary by OS... (see test/functional/feature_notifications.py:FILE_CHARS_DISALLOWED)

Thanks for sharing the link. I checked it, did some research and thought its best to avoid dependency on checking OS for this if statement because of two reasons:

1. Some users might hide/change things so that attackers cannot identify OS
2. We don't know how many types of OS are used for running bitcoin core, their characters usage and new updates. Also lot of OS are now supporting things that were only available on other OS in past. Example: Ubuntu in Windows @kristapsk Removed regex

And as there is anyway a plan to use C++17 for the next major release after v0.21, std::filename::filename looks like a right solution to me.

Maybe. I am not the best person to comment on things related to C++

Also not sure why tests are failing in CI. Everything worked fine on my local machine and I tested with different wallet names.

Closing this PR.

Reasons:

1. Not the best solution to solve this problem. Can also break few projects using bitcoin core.

2. Better to add as an example and suggestion in docs which already got few ACKs in #20741

If anyone interested to work on similar solution in future can always open new PR and also refer to this PR.