Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't log exceptions for obviously incorrect stream tokens #18139

Merged
merged 5 commits into from
Feb 10, 2025

Conversation

erikjohnston
Copy link
Member

We log incorrect ones as we want to catch bugs where Synapse returns bad tokens. However, sometimes clients just send tokens that are e.g. empty.

We log incorrect ones as we want to catch bugs where Synapse returns bad
tokens. However, sometimes clients just send tokens that are e.g. empty.
@erikjohnston erikjohnston marked this pull request as ready for review February 6, 2025 12:28
@erikjohnston erikjohnston requested a review from a team as a code owner February 6, 2025 12:28
Copy link
Contributor

@MadLittleMods MadLittleMods left a comment

Choose a reason for hiding this comment

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

Clarified "at the exception-level" because this change is a bit confusing otherwise (we still log and raise and exception, just not at the exception-level for obviously wrong tokens).

changelog.d/18139.misc Outdated Show resolved Hide resolved
synapse/types/__init__.py Outdated Show resolved Hide resolved
Comment on lines 667 to 671
# Check it looks like a Synapse token first. We do this so that
# we don't log exceptions for obviously incorrect tokens.
if not string or string[0] not in ("s", "t", "m"):
logger.warning("Invalid token %r", string)
raise SynapseError(400, f"Invalid room stream token {string:!r}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we should think about just ignoring these obviously wrong tokens altogether or log at a much lower level.

Suggested change
# Check it looks like a Synapse token first. We do this so that
# we don't log exceptions for obviously incorrect tokens.
if not string or string[0] not in ("s", "t", "m"):
logger.warning("Invalid token %r", string)
raise SynapseError(400, f"Invalid room stream token {string:!r}")
# Check it looks like a Synapse token first. We do this so that
# we don't log exceptions for obviously incorrect tokens.
if not string or string[0] not in ("s", "t", "m"):
logger.debug("Invalid token %r", string)
raise SynapseError(400, f"Invalid room stream token {string:!r}")

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was going to suggest at least only logging the empty string case as debug. It's plausible that a non-empty token is from a future synapse (after upgrade/downgrade) or something. However, I realised we'll log these tokens anyway when we return the 400 (as we log the error string), so actually adding an extra log line here is pointless, so I've removed it entirely.

@MadLittleMods MadLittleMods changed the title Don't log exceptions for onbvious incorrect stream tokens Don't log exceptions for obviously incorrect stream tokens Feb 6, 2025
@erikjohnston erikjohnston enabled auto-merge (squash) February 10, 2025 10:31
@erikjohnston erikjohnston merged commit 4c84c9c into develop Feb 10, 2025
39 checks passed
@erikjohnston erikjohnston deleted the erikj/less_errors branch February 10, 2025 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants