Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix a bug where Synapse fails to start if a signing key file contains an empty line. #13738

Merged
merged 2 commits into from
Sep 8, 2022
Merged

Fix a bug where Synapse fails to start if a signing key file contains an empty line. #13738

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70accba
Don't choke on empty signing key lines
reivilibre Sep 7, 2022
016b14e
Newsfile
reivilibre Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/13738.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a bug where Synapse fails to start if a signing key file contains an empty line.
13 changes: 12 additions & 1 deletion synapse/config/key.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,18 @@ def read_signing_keys(self, signing_key_path: str, name: str) -> List[SigningKey

signing_keys = self.read_file(signing_key_path, name)
try:
return read_signing_keys(signing_keys.splitlines(True))
loaded_signing_keys = read_signing_keys(
[
signing_key_line
for signing_key_line in signing_keys.splitlines(keepends=False)
if signing_key_line.strip()
]
)

if not loaded_signing_keys:
raise ConfigError(f"No signing keys in file {signing_key_path}")

return loaded_signing_keys
Comment on lines -220 to +231
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why we had keepends=True before? Seems like something we don't want to have in any case.

Nit: could use a generator expression, but given that a) we've already loaded the whole file into memory and b) this is going to be a tiny file, it's not worth the effort. I probably shouldn't have even written this sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered a generator expression but I don't necessarily trust the library to not change in such a way that makes it so the generator gets consumed multiple times — this kind of thing has bitten before, so for this tiny file I thought it would be better to just be a bit defensive.

+1 to wondering why keepends=True before. .split() knocks them out in any case, but I find it weird

except Exception as e:
raise ConfigError("Error reading %s: %s" % (name, str(e)))

Expand Down