wallet: Disallow wallet names that are paths including .. and . elements#34544
wallet: Disallow wallet names that are paths including .. and . elements#34544achow101 wants to merge 2 commits intobitcoin:masterfrom
.. and . elements#34544Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, 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. |
|
Concept ACK |
|
https://github.com/bitcoin/bitcoin/actions/runs/21844979703/job/63075106819?pr=34544#step:12:15955: test 2026-02-10T08:24:08.355405Z TestFramework (ERROR): Unexpected exception
Traceback (most recent call last):
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\test_framework.py", line 142, in main
self.run_test()
~~~~~~~~~~~~~^^
File "D:\a\bitcoin\bitcoin/test/functional/wallet_migration.py", line 1678, in run_test
self.test_wallet_with_relative_path()
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
File "D:\a\bitcoin\bitcoin/test/functional/wallet_migration.py", line 621, in test_wallet_with_relative_path
migrate_res, wallet = self.migrate_and_get_rpc(relative_name)
~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
File "D:\a\bitcoin\bitcoin/test/functional/wallet_migration.py", line 139, in migrate_and_get_rpc
migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\coverage.py", line 50, in __call__
return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
File "D:\a\bitcoin\bitcoin\test\functional\test_framework\authproxy.py", line 156, in __call__
raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Wallet name given as a relative path cannot begin with .. or . (-4)
test 2026-02-10T08:24:08.357776Z TestFramework (DEBUG): Closing down network thread |
There was a problem hiding this comment.
Concept ACK [7c993c6]
the funtional test wallet_migration.py fails cuz the test specifically tries to migrate from a relative path using .., see line 621:
wallet_name = "relative"
absolute_path = os.path.abspath(
os.path.join(common_parent, wallet_name))
# /tmp/bitcoin_func_test_rriidks3/relative
relative_name = os.path.relpath(
absolute_path, start=self.master_node.wallets_path)
# ../../../relative
wallet = 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.
7c993c6 to
35161d1
Compare
Yes, changed the test to check for the error, and removed the other test that used a relative path like this.
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). {
"_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.",
"wallet": [
"../qt-wallet-relative"
]
}
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 |
We can write something in the release notes. I would not expect non-technical users to have wallets named like this. |
|
Agree |
|
I think this will need a rebase, to fix the failing Windows CI. |
|
|
||
| def test_invalid_wallet_names(self): | ||
| self.log.info("Test weird paths are not allowed as wallet names") | ||
| NON_NORMALIZED = ["bad/./path", "bad/../path", "/bad/./path", "/bad/../path", "../", "./", "./wallet", "../wallets/../wallets/wallet"] |
There was a problem hiding this comment.
- NON_NORMALIZED = ["bad/./path", "bad/../path", "/bad/./path", "/bad/../path", "../", "./", "./wallet", "../wallets/../wallets/wallet"]
+ NON_NORMALIZED = ["bad/./path", "bad/../path", "/bad/./path", "/bad/../path", "../", "./", "./wallet", "../wallets/../wallets/wallet", "./wallets/wallet"]There was a problem hiding this comment.
I don't think that's a useful test case as ./wallets does not exist.
There was a problem hiding this comment.
This was suggested to cover for the patterns satisfying the 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.
|
|
||
| def test_wallet_with_path(self, wallet_path): | ||
| self.log.info("Test migrating a wallet with the following path/name: %s", wallet_path) | ||
| # the wallet data is actually inside of path/that/ends/ |
There was a problem hiding this comment.
There was a problem hiding this comment.
The comment is still accurate.
There was a problem hiding this comment.
You're right that the comment is technically correct. I understood the original intent of the comment to be that when the path is "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 6a7c0d3 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.
| { | ||
| const fs::path name_path = fs::PathFromString(name); | ||
|
|
||
| // 'name' must be a normalized path, i.e. no . or .. except at the root |
There was a problem hiding this comment.
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.
w0xlt
left a comment
There was a problem hiding this comment.
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.
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.
35161d1 to
914ca19
Compare
|
The PR changes behaviour from allowing relative paths with ../. elements to rejecting them. I would suggest to consider n alternative approach, which would be to normalise the path to absolute form at the start of the function instead of rejecting it, this way users could pass whatever they want, and internally we'd always work with absolute paths. |
Wallet names including
..and.are unintuitive and can lead to various issues, see #34497This 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.