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

Squah/unrevert fts changes on hotfix #7

Open
wants to merge 645 commits into
base: develop
Choose a base branch
from

Conversation

sorydima
Copy link
Owner

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Code style is correct
    (run the linters)

richvdh and others added 30 commits October 1, 2020 15:07
This has now been fixed by a different commit (73d9303).

This reverts commit b0a463f.
Cross-user `m.key_share_requests` are a relatively new `to_device` message that allows user to re-request session keys for a message from another user if they were otherwise unable to retrieve them.

Unfortunately, these have had performance concerns on matrix.org. This is a temporary patch to disable them while we investigate a better solution.
* develop:
  Don't unnecessarily start bg process in replication sending loop. (#8670)
  Don't unnecessarily start bg process while handling typing. (#8668)
Fixes:

```
builtins.TypeError: _reload_logging_config() takes 1 positional argument but 2 were given
```
Adds the redacts endpoint to workers that have the client listener.
This reverts a285fe0. Hopefully the cache is no longer required, thanks to
anoadragon453 and others added 29 commits August 22, 2022 10:47
This reverts fae708c.

We believe this to be unnecessary---other Synapse deployments do not
have this patch, and we are not aware of bridging problems as a result.

Related:

- matrix-org/matrix-appservice-irc#506
- matrix-org/synapse#4826
…lities of the underlying DB. (#11635)"

This reverts commit d902181.
…h capabilities of the underlying DB. (#11635)""

This reverts commit 7e0dd52.
@@ -410,6 +410,7 @@
request, MsisdnRequestTokenBody
)
msisdn = phone_number_to_msisdn(body.country, body.phone_number)
logger.info("Request #%s to verify ownership of %s", body.send_attempt, msisdn)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (private)
as clear text.

Copilot Autofix AI 4 months ago

To fix the problem, we should avoid logging the sensitive msisdn data directly. Instead, we can log a masked version of the phone number or omit it entirely from the logs. Masking involves replacing part of the phone number with asterisks or another character to obscure the sensitive information while still providing some context for debugging.

Steps to fix:

  1. Modify the logging statements to mask the msisdn variable.
  2. Ensure that the masking function is implemented and used consistently.
Suggested changeset 1
synapse/rest/client/account.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py
--- a/synapse/rest/client/account.py
+++ b/synapse/rest/client/account.py
@@ -412,3 +412,4 @@
         msisdn = phone_number_to_msisdn(body.country, body.phone_number)
-        logger.info("Request #%s to verify ownership of %s", body.send_attempt, msisdn)
+        masked_msisdn = msisdn[:2] + "****" + msisdn[-2:]
+        logger.info("Request #%s to verify ownership of %s", body.send_attempt, masked_msisdn)
 
@@ -442,3 +443,4 @@
 
-            logger.info("MSISDN %s is already in use by %s", msisdn, existing_user_id)
+            masked_msisdn = msisdn[:2] + "****" + msisdn[-2:]
+            logger.info("MSISDN %s is already in use by %s", masked_msisdn, existing_user_id)
             raise SynapseError(400, "MSISDN is already in use", Codes.THREEPID_IN_USE)
@@ -467,3 +469,4 @@
         )
-        logger.info("MSISDN %s: got response from identity server: %s", msisdn, ret)
+        masked_msisdn = msisdn[:2] + "****" + msisdn[-2:]
+        logger.info("MSISDN %s: got response from identity server: %s", masked_msisdn, ret)
 
EOF
@@ -412,3 +412,4 @@
msisdn = phone_number_to_msisdn(body.country, body.phone_number)
logger.info("Request #%s to verify ownership of %s", body.send_attempt, msisdn)
masked_msisdn = msisdn[:2] + "****" + msisdn[-2:]
logger.info("Request #%s to verify ownership of %s", body.send_attempt, masked_msisdn)

@@ -442,3 +443,4 @@

logger.info("MSISDN %s is already in use by %s", msisdn, existing_user_id)
masked_msisdn = msisdn[:2] + "****" + msisdn[-2:]
logger.info("MSISDN %s is already in use by %s", masked_msisdn, existing_user_id)
raise SynapseError(400, "MSISDN is already in use", Codes.THREEPID_IN_USE)
@@ -467,3 +469,4 @@
)
logger.info("MSISDN %s: got response from identity server: %s", msisdn, ret)
masked_msisdn = msisdn[:2] + "****" + msisdn[-2:]
logger.info("MSISDN %s: got response from identity server: %s", masked_msisdn, ret)

Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@@ -439,6 +440,7 @@
await self.hs.get_clock().sleep(random.randint(1, 10) / 10)
return 200, {"sid": random_string(16)}

logger.info("MSISDN %s is already in use by %s", msisdn, existing_user_id)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (private)
as clear text.

Copilot Autofix AI 4 months ago

To fix the problem, we should avoid logging the sensitive msisdn data directly. Instead, we can log a generic message or mask the sensitive parts of the phone number to ensure that it is not exposed in the logs.

  • Replace the logging statement on line 443 to avoid logging the full msisdn.
  • Introduce a utility function to mask the phone number if needed.
  • Ensure that the new logging statement provides enough information for debugging without exposing sensitive data.
Suggested changeset 1
synapse/rest/client/account.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py
--- a/synapse/rest/client/account.py
+++ b/synapse/rest/client/account.py
@@ -442,3 +442,3 @@
 
-            logger.info("MSISDN %s is already in use by %s", msisdn, existing_user_id)
+            logger.info("MSISDN is already in use by an existing user")
             raise SynapseError(400, "MSISDN is already in use", Codes.THREEPID_IN_USE)
@@ -467,3 +467,3 @@
         )
-        logger.info("MSISDN %s: got response from identity server: %s", msisdn, ret)
+        logger.info("Received response from identity server for MSISDN verification request")
 
EOF
@@ -442,3 +442,3 @@

logger.info("MSISDN %s is already in use by %s", msisdn, existing_user_id)
logger.info("MSISDN is already in use by an existing user")
raise SynapseError(400, "MSISDN is already in use", Codes.THREEPID_IN_USE)
@@ -467,3 +467,3 @@
)
logger.info("MSISDN %s: got response from identity server: %s", msisdn, ret)
logger.info("Received response from identity server for MSISDN verification request")

Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
@@ -463,6 +465,7 @@
threepid_send_requests.labels(type="msisdn", reason="add_threepid").observe(
body.send_attempt
)
logger.info("MSISDN %s: got response from identity server: %s", msisdn, ret)

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (private)
as clear text.

Copilot Autofix AI 4 months ago

To fix the problem, we should avoid logging the sensitive msisdn data directly. Instead, we can log a masked version of the phone number or omit it entirely from the log message. Masking involves replacing part of the phone number with asterisks or another character to obscure the sensitive information while still providing some context for debugging.

Steps to fix:

  1. Modify the log statement on line 468 to mask the msisdn value.
  2. Introduce a helper function to mask the phone number.
Suggested changeset 1
synapse/rest/client/account.py

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/synapse/rest/client/account.py b/synapse/rest/client/account.py
--- a/synapse/rest/client/account.py
+++ b/synapse/rest/client/account.py
@@ -467,3 +467,4 @@
         )
-        logger.info("MSISDN %s: got response from identity server: %s", msisdn, ret)
+        masked_msisdn = msisdn[:2] + "****" + msisdn[-2:]
+        logger.info("MSISDN %s: got response from identity server: %s", masked_msisdn, ret)
 
EOF
@@ -467,3 +467,4 @@
)
logger.info("MSISDN %s: got response from identity server: %s", msisdn, ret)
masked_msisdn = msisdn[:2] + "****" + msisdn[-2:]
logger.info("MSISDN %s: got response from identity server: %s", masked_msisdn, ret)

Copilot is powered by AI and may make mistakes. Always verify output.
Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
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.

8 participants