Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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/19094.misc
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Use cheaper random string function in logcontext utilities.
8 changes: 4 additions & 4 deletions synapse/logging/context.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@
from twisted.python.threadpool import ThreadPool

from synapse.logging.loggers import ExplicitlyConfiguredLogger
from synapse.util.stringutils import random_string
from synapse.util.stringutils import pseudo_random_string

if TYPE_CHECKING:
from synapse.logging.scopecontextmanager import _LogContextScope
Expand Down Expand Up @@ -657,7 +657,7 @@ def __init__(
self, new_context: LoggingContextOrSentinel = SENTINEL_CONTEXT
) -> None:
self._new_context = new_context
self._instance_id = random_string(5)
self._instance_id = pseudo_random_string(5)

def __enter__(self) -> None:
logcontext_debug_logger.debug(
Expand Down Expand Up @@ -859,7 +859,7 @@ def run_in_background(
Note that the returned Deferred does not follow the synapse logcontext
rules.
"""
instance_id = random_string(5)
instance_id = pseudo_random_string(5)
calling_context = current_context()
logcontext_debug_logger.debug(
"run_in_background(%s): called with logcontext=%s", instance_id, calling_context
Expand Down Expand Up @@ -1012,7 +1012,7 @@ def make_deferred_yieldable(deferred: "defer.Deferred[T]") -> "defer.Deferred[T]
restores the old context once the awaitable completes (execution passes from the
reactor back to the code).
"""
instance_id = random_string(5)
instance_id = pseudo_random_string(5)
logcontext_debug_logger.debug(
"make_deferred_yieldable(%s): called with logcontext=%s",
instance_id,
Expand Down
17 changes: 17 additions & 0 deletions synapse/util/stringutils.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#
#
import itertools
import random
import re
import secrets
import string
Expand Down Expand Up @@ -56,6 +57,10 @@ def random_string(length: int) -> str:
"""Generate a cryptographically secure string of random letters.

Drawn from the characters: `a-z` and `A-Z`

Because this is generated from cryptographic sources, it takes a notable amount of
effort to generate (computationally expensive). If you don't need cryptographic
security, consider using `pseudo_random_string` for better performance.
"""
return "".join(secrets.choice(string.ascii_letters) for _ in range(length))

Expand All @@ -68,6 +73,18 @@ def random_string_with_symbols(length: int) -> str:
return "".join(secrets.choice(_string_with_symbols) for _ in range(length))


def pseudo_random_string(length: int) -> str:
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I also considered naming this random_string_insecure_fast

🤷

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think we should call this insecure_random_string. Being pseudo-random doesn't necessarily imply insecure, that's what CSPRNGs are for, after all.

It's maybe not a huge deal, but given the sheer hazard of using the wrong type of random in the wrong place, I much prefer the clear and simple insecure label, because it brings your attention to an important (negative) caveat. In theory that could raise some alarm bells at a critical time during review.

On the other hand, I don't think it's important to say fast — if someone consciously thinks about the speed at PR review time, they can look it up. (But I'm also not against calling it 'fast', to be fair!)

Copy link
Copy Markdown
Contributor Author

@MadLittleMods MadLittleMods Oct 30, 2025

Choose a reason for hiding this comment

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

Originally, I didn't even consider that a simple, innocuous-sounding random_string(...) utility would have a noticeable impact in the app when using it in the logcontext code. There is no "cryptographically"/"crypt" hint about it from the outside that I would normally think about needing in a secure context.

Adding fast at-least sparks an idea that the other variation could be slow and better consideration on which one to choose.

I'll go with random_string_insecure_fast (suffix) so it appears more readily and obviously next to random_string in typeahead.

"""
Generate a string of random letters (insecure, fast). This is a more performant but
insecure version of `random_string`.

WARNING: Not for security or cryptographic uses. Use `random_string` instead.

Drawn from the characters: `a-z` and `A-Z`
"""
return "".join(random.choice(string.ascii_letters) for _ in range(length))


def is_ascii(s: bytes) -> bool:
try:
s.decode("ascii").encode("ascii")
Expand Down
Loading