fix(config): read the config file as UTF-8 rather than the platform codec - #396
Open
ppcvote wants to merge 1 commit into
Open
fix(config): read the config file as UTF-8 rather than the platform codec#396ppcvote wants to merge 1 commit into
ppcvote wants to merge 1 commit into
Conversation
…odec BdkConfigLoader.load_from_file called read_text() with no encoding, so the config was decoded with locale.getpreferredencoding(). YAML and JSON are both specified as UTF-8, so any non-ASCII value was decoded with the wrong codec on a non-UTF-8 host. The failure is silent where it matters. Measured on Windows with cp950, a config carrying a non-ASCII proxy credential: on disk : password: "sésame-café" (b'...s\xc3\xa9same-caf\xc3\xa9...') loaded : password='s矇same-caf矇' username: 'caf矇-user' No exception. The bot starts and fails later against the proxy with an error that points nowhere near the config. load_from_content on the same content returns the right values, which isolates it to the read. The repository already uses the correct pattern at service/user/user_service.py:798, so this is one argument for consistency with it. Two other implicit-encoding reads are deliberately left out of scope rather than bundled: bdk_rsa_key_config.py:59 reads a PEM key (base64, ASCII) and on_disk_datafeed_id_repository.py:56 reads an id this library writes itself. The test drives the loader in a child interpreter with PYTHONUTF8=0 LC_ALL=C so it reproduces on a UTF-8 CI host, and uses load_from_content as the oracle for what load_from_file should produce. Fixtures are written as raw bytes and their UTF-8 sequences verified in the index, so no locale can influence them. Verified by reverting the one-line change: 4 failed / 1 passed before, 5 passed after, the passing case being an ASCII control. Full suite: 558 passed before, 563 after, same 2 pre-existing failures. Closes finos#395 Signed-off-by: ppcvote <risky9763@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #395.
The defect
load_from_filedecoded the config withlocale.getpreferredencoding():YAML and JSON are both specified as UTF-8, so on a non-UTF-8 host every non-ASCII value in a config comes back wrong.
The failure is silent where it matters. Measured on Windows with cp950, on a config carrying a non-ASCII proxy credential:
password: "sésame-café"(b'...s\xc3\xa9same-caf\xc3\xa9...')load_from_filepassword='s矇same-caf矇'— no exceptionload_from_content, same bytespassword='sésame-café'Nothing errors. The bot starts, authenticates against the proxy with a password that is not the one in the file, and fails with a message that points nowhere near the config. The third row isolates it to the read rather than the parse.
A
LC_ALL=Chost raises instead, which is the loud version of the same bug; cp1252 on en-US Windows mangles it just as quietly as cp950 does.The change
One argument on line 27. The repository already uses this pattern at
service/user/user_service.py:798, so the two reads now agree.Two other implicit-encoding reads are deliberately out of scope, rather than swept in because they matched a grep:
bdk_rsa_key_config.py:59reads a PEM private key, which is base64 and therefore ASCIIon_disk_datafeed_id_repository.py:56reads an id this library wrote itselfNeither can carry non-ASCII in practice. Happy to tighten them in a separate PR if you would like the module clean under
-X warn_default_encoding, but they are a different question from this bug.The test
Two deliberate choices, because the obvious version of this test proves nothing:
It runs the loader in a child interpreter with
PYTHONUTF8=0 PYTHONCOERCECLOCALE=0 LC_ALL=C LANG=C. Without that it passes on a UTF-8 CI host whether or not the fix is present — which is why this has gone unnoticed.load_from_contentis used as the oracle. It receives already-decoded text and was always correct, so asserting the two entry points agree states the actual invariant rather than hard-coding an expected string.The fixtures are written as raw bytes, and I verified the UTF-8 sequences survive into the git index (
c3 a9foré) — a fixture silently re-encoded on the way in would quietly disarm the test for everyone else.Verified by reverting the one-line change: 4 failed / 1 passed before (both formats × both assertions), 5 passed after. The always-passing case is an ASCII config, kept as a control so a failure there means the harness broke rather than the encoding handling.
Checks
Full suite: 558 passed before, 563 after — exactly my five — with the same 2 pre-existing failures both times. Commit is DCO signed off; our FINOS CLA is on file.