.. and . elements
#34544
Wallet names including .. and . are unintuitive and can lead to various issues, see #34497
This disallows creating or loading wallets that have any path elements that are .. or ., including any present in an absolute path. This does not disallow relative paths altogether but rather limits them to only being subdirectories of the wallets directory.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | davidgumberg, arejula27 |
| Approach ACK | w0xlt |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
https://github.com/bitcoin/bitcoin/actions/runs/21844979703/job/63075106819?pr=34544#step:12:15955:
0 test 2026-02-10T08:24:08.355405Z TestFramework (ERROR): Unexpected exception
1 Traceback (most recent call last):
2 File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 142, in main
3 self.run_test()
4 ~~~~~~~~~~~~~^^
5 File "D:\a\bitcoin\bitcoin/test/functional/wallet_migration.py", line 1678, in run_test
6 self.test_wallet_with_relative_path()
7 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
8 File "D:\a\bitcoin\bitcoin/test/functional/wallet_migration.py", line 621, in test_wallet_with_relative_path
9 migrate_res, wallet = self.migrate_and_get_rpc(relative_name)
10 ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
11 File "D:\a\bitcoin\bitcoin/test/functional/wallet_migration.py", line 139, in migrate_and_get_rpc
12 migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
13 File "D:\a\bitcoin\bitcoin\test\functional\test_framework\coverage.py", line 50, in __call__
14 return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
15 File "D:\a\bitcoin\bitcoin\test\functional\test_framework\authproxy.py", line 156, in __call__
16 raise JSONRPCException(response['error'], status)
17 test_framework.authproxy.JSONRPCException: Wallet name given as a relative path cannot begin with .. or . (-4)
18 test 2026-02-10T08:24:08.357776Z TestFramework (DEBUG): Closing down network thread
2922+ // 'name' must be a normalized path, i.e. no . or .. except at the root
2923+ if (name_path != name_path.lexically_normal()) {
2924+ return util::Error{Untranslated("Wallet name given as a path must be normalized")};
2925+ }
2926+
2927+ // 'name' cannot begin with . or ..
This comment is not correct, as I was available to do
0$➜ bitcoin (34544) % bitcoin-cli -regtest createwallet ".my_pb_wall" 20:11
1{
2 "name": ".my_pb_wall"
3}
./ and ../
Concept ACK [7c993c6036088990af9cfca12bb7cf33120cf33c]
the funtional test wallet_migration.py fails cuz the test specifically tries to migrate from a relative path using .., see line 621:
0wallet_name = "relative"
1absolute_path = os.path.abspath(
2 os.path.join(common_parent, wallet_name))
3# /tmp/bitcoin_func_test_rriidks3/relative
4relative_name = os.path.relpath(
5 absolute_path, start=self.master_node.wallets_path)
6# ../../../relative
7
8wallet = self.create_legacy_wallet(relative_name)
This is done because the nodes use different wallet directories (/tmp/bitcoin_func_test_kfajzcqh/node0/regtest/wallets and /tmp/bitcoin_func_test_kfajzcqh/node1/regtest/wallets), which causes GetWalletPath to fail, as it relies on ../../ to reach the old node’s wallet directory.
To fix this, migrations should only happen from a common ancestor directory to avoid ... However, I suggest using absolute paths for migration and deprecate this test case. Instead, the test should verify that attempting to migrate using a relative path returns an error (e.g., “migration from a relative path is not allowed”).
This change will cause users who load wallets using relative paths to fail. For the loadwallet RPC, it may be useful to improve the error message, for example:
Wallet file verification failed. Wallet names given as relative paths cannot begin with .. or . since v31. Please use an absolute path instead. Your wallet data has not been lost.
Wallet names that are also paths that contain . and .. are unintuitive
and can result in unexpected behavior, particularly in migration.
Therefore we should disallow users from specifying wallet names that
contain . and .. as path elements.
However, I suggest using absolute paths for migration and deprecate this test case. Instead, the test should verify that attempting to migrate using a relative path returns an error (e.g., “migration from a relative path is not allowed”).
Yes, changed the test to check for the error, and removed the other test that used a relative path like this.
it may be useful to improve the error message
I included the suggestion to use an absolute path.
When using Bitcoin-Qt, it fails during initialisation, shows an error, and then crashes. Relaunching the program results in the same error, making it impossible to use (and impossible to manually unload the wallet using the GUI).
To resolve the issue, users can open DATADIR/settings.json and remove the wallet entry:
0{
1 "_warning_": "This file is automatically generated and updated by Bitcoin Core. Please do not edit this file while the node is running, as any changes might be ignored or overwritten.",
2 "wallet": [
3 "../qt-wallet-relative"
4 ]
5}
For non-technical users, this can be difficult to troubleshoot. It would be nice to update the doc somewhere so they can read how to fix the crash loop or automatically remove all wallets that start with .. or ./ from the auto load configuration.
For non-technical users, this can be difficult to troubleshoot. It would be nice to update the doc somewhere so they can read how to fix the crash loop or automatically remove all wallets that start with .. or ./ from the auto load configuration.
We can write something in the release notes. I would not expect non-technical users to have wallets named like this.
405@@ -404,6 +406,15 @@ def wallet_file(name):
406 self.nodes[0].unloadwallet(wallet)
407 self.nodes[1].loadwallet(wallet)
408
409+ def test_invalid_wallet_names(self):
410+ self.log.info("Test weird paths are not allowed as wallet names")
411+ NON_NORMALIZED = ["bad/./path", "bad/../path", "/bad/./path", "/bad/../path", "../", "./", "./wallet", "../wallets/../wallets/wallet"]
0- NON_NORMALIZED = ["bad/./path", "bad/../path", "/bad/./path", "/bad/../path", "../", "./", "./wallet", "../wallets/../wallets/wallet"]
1+ NON_NORMALIZED = ["bad/./path", "bad/../path", "/bad/./path", "/bad/../path", "../", "./", "./wallet", "../wallets/../wallets/wallet", "./wallets/wallet"]
./wallets does not exist.
lexically_normal check. It can be just as easily be replaced with ./bad/path, a pattern that is not explicitly mentioned in the existing test cases.
.., but that specific case no longer exists and the comment probably should have been cleaned up already in 6a7c0d3f874942a460bf5b13a107a2b21eb2e572, so it’s not really specific to this PR.
"path/that/ends/in/..", it actually resolves to path/that/ends/. That’s true, but since the specific test case where the path ends with .. has been removed in 6a7c0d3f874942a460bf5b13a107a2b21eb2e572 and this function is now generic, having only this comment left is a bit confusing when reading the code. In any case, this is just my preference and not meant to be blocking.
2916@@ -2917,17 +2917,29 @@ bool CWallet::EraseAddressReceiveRequest(WalletBatch& batch, const CTxDestinatio
2917
2918 static util::Result<fs::path> GetWalletPath(const std::string& name)
2919 {
2920+ const fs::path name_path = fs::PathFromString(name);
2921+
2922+ // 'name' must be a normalized path, i.e. no . or .. except at the root
i.e. no . or .. except at the root
This gives the impression that ./bad/path would be be allowed but it is not as the tests show.
Perharps a graceful fallback can be added to handle the issue mentioned here: #34544 (comment) (which could also occur outside the GUI).
Otherwise, it looks good to me.