Require body for read receipts with user-agent exceptions

[rogersheu]

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.
• Pull request includes a sign off
• Code style is correct (run the linters)

Signed-off-by: Roger Sheu rogersheu92@gmail.com

Fixes #11156.

[squahtx]

Thank you for your contribution to Synapse! Before we can review the changes, could you make CI pass?
scripts-dev/lint.sh will format the code to make CI happy.

[reivilibre]

Thanks for taking this on!

I added

Fixes #11156.

to your PR description because that way it will cross-reference this PR and that issue (and automatically close the issue when this PR is merged!).

As mentioned by Sean, ./scripts-dev/lint.sh will help you with formatting the code properly and getting the checks to pass.

[reivilibre]

#11156 forgot to mention, but this should be restricted to user agents that contain Android (could use "Android" in user_agent).

I would also be tempted to collapse Element and SchildiChat together:

re.search("(Element|SchildiChat)/1.[012].*", user_agent)

Note that re.search will match anywhere in the string (this is different to re.match, which matches only at the beginning).

re.search(".*Riot.*", user_agent) can then be re.search("Riot", user_agent) instead.
But at that point, you may as well just write "Riot" in user_agent since we don't need any regex powers to do that :).

Also note that in re.search("(Element|SchildiChat)/1.[012].*", user_agent), . matches any character, so this would also match Element/110 which may not be what we were after.

Out of interest, this would be what a combined pattern looks like:
r"(Riot|(Element|SchildiChat)/1\.[012]\.).*Android"
(This also checks that it's an Android client: here's an example string from my device Element/1.2.2 (Linux; U; Android 9; ...; MatrixAndroidSDK_X 0.0.1))

(if you're not aware of this trick: I use r" so that I don't have to escape backslashes by writing \\ instead of \).

[callahad]

Ooh! Good catch on the Android part. So maybe we should wrap that whole thing and do something like:

user_agent = get_request_user_agent(request)
pattern = re.compile(r"(?:Element|SchildiChat)/1\.[012]\.")

allow_empty_body = False
if "Android" in user_agent:
    if pattern.match(user_agent) or "Riot" in user_agent:
        allow_empty_body = True

Does that look right? Should we make the pattern a module-level constant to prevent excess calls to compile?

This will exclude a tiny number of weird builds, like Element dbg/1.1.8-dev or SchildiChat[f]/1.2.2.sc44, but they're sufficiently uncommon.

[reivilibre]

That seems right to me. Perhaps we ought to have a few test cases to ensure we don't mess this up big time.

I can't remember when I read this, but iirc you're better off using re.compile once or not at all: re.match and friends have an internal cache for compiled patterns, which re.compile bypasses (or did at the time).
My personal preference is indeed to make it a module-level constant and guarantee that it's compiled only once.

[reivilibre]

SchildiChat[f]/1.2.2.sc44, but they're sufficiently uncommon.

This one makes me a little suspicious: does the [f] perhaps stand for F-Droid?

Maybe we ought to be nice and allow some wiggle room:

pattern = re.compile(r"(?:Element|SchildiChat)[^/]*/1\.[012]\.")

[callahad]

I checked again, and all the [f] and [g] SchildiChats in the logs are version 1.2.2 or later, so they can be excluded from this pattern. There was one SchildiChat.Beta[f]/1.2.0.sc42-test1 which would presumably break, but again, not worth the complexity for that one weirdo with an old test build :)

[rogersheu]

Right, I was wondering about the periods and whether those (and other various parts of the string) had to be exact too.

What's the rationale behind using re.search vs. re.compile? It looks like re.search searches for substrings by default while re.compile can match exactly as written, right?

Perhaps the weird builds could just be specifically white-listed if push comes to shove and if there aren't too many of them.

(if you're not aware of this trick: I use r" so that I don't have to escape backslashes by writing \ instead of ).

Thanks for pointing that out!

[DMRobertson]

What's the rationale behind using re.search vs. re.compile? It looks like re.search searches for substrings by default while re.compile can match exactly as written, right?

I think you might mean re.match where you say re.compile.

To use a regular expression like a.*b, the re module has to first build a "finite state machine". That machine does all the work (working out if there's a match; where it is; what the capture groups captured, ....). Creating the machine has a cost to it. After it's built, the machine can be reset and used again, without paying that build cost.

Python offers re.compile so you can build the state machine once and re-use it, and that's what @callahad proposes above.

[rogersheu]

Thanks @DMRobertson ! Makes a lot of sense. I do see now that re.search was the incorrect choice for regex function. I had worked out the difference between re.search and re.match but you explained the difference between re.search/re.match and re.compile, which is what I was looking for.

To summarize (to the extent of my knowledge), in re.compile, the re module builds the finite state machine and doesn't have to be built again (but its state can be changed when called), while each call of re.match includes the building of a finite state machine each time (like re.compile, then pattern.match), taking up runtime/memory.

I can't remember when I read this, but iirc you're better off using re.compile once or not at all: re.match and friends have an internal cache for compiled patterns, which re.compile bypasses (or did at the time).
My personal preference is indeed to make it a module-level constant and guarantee that it's compiled only once.

"at the time?" Like during recent testing? (Apologies in advance if I'm asking too many questions about this.)

[DMRobertson]

I think @reivilibre meant "at the time I read the article".

FWIW there's https://docs.python.org/3/howto/regex.html#module-level-functions which writes:

Should you use these module-level functions, or should you get the pattern and call its methods yourself? If you’re accessing a regex within a loop, pre-compiling it will save a few function calls. Outside of loops, there’s not much difference thanks to the internal cache.

https://stackoverflow.com/a/61603344/5252017 has more to say. Apparently the cache size is currently 512 patterns.

[rogersheu]

I see. That's helpful!

[rogersheu]

It would appear that the assert statement in test_read_receipt_with_empty_body(self) is not passing, which makes sense since that's what was changed in this pull request. I also see no instance of Riot/Schildi/Element set in the \rest\client\test_sync.py code, which should lead to a 400 Synapse error in body and hidden in receipts.py.

I think the fix would either be to fix the unit tests by adding in this extra information or to add in new unit tests addressing each of these possible clients.

I also ran into an issue (specifically with fcntl) running tests locally since I'm running Windows and not Linux.

[DMRobertson]

I think the fix would either be to fix the unit tests by adding in this extra information or to add in new unit tests addressing each of these possible clients.

I'd suggest:

• change test_read_receipt_with_empty_body to take an extra user_agent: str and expected_status_code: int argument
• decorate this test with @parameterized.expand to cover the different cases. Something like:

  @parameterized.expand([
      ("Android Element 1.0", 400),
      ("Android Element 1.4", 200),
      ...
  ])
  def test_read_receipt_with_empty_body(...):

  (The user agents are just examples here; I'm not sure of the specifics!)
• Pass custom_headers to make_request to set the user agent header to the test argument.
• Change the final assertEqual call to compare to expected_status_code.

I also ran into an issue (specifically with fcntl) running tests locally since I'm running Windows and not Linux.

That's unfortunate. Maybe WSL can help with this?

[DMRobertson]

Also, the newsfile check is unhappy. Should be fine if you rename 11156.misc to 11157.misc.

[reivilibre]

For testing, use a user agent of the form Element/1.2.2 (Linux; Android 6) (it's not exactly what's on the wire but close enough — just extra gunk removed). I think dmr's suggestion above to use a parameterised test is reasonable.

[rogersheu]

I think the fix would either be to fix the unit tests by adding in this extra information or to add in new unit tests addressing each of these possible clients.

I'd suggest:

• change test_read_receipt_with_empty_body to take an extra user_agent: str and expected_status_code: int argument
• decorate this test with @parameterized.expand to cover the different cases. Something like:

  @parameterized.expand([
      ("Android Element 1.0", 400),
      ("Android Element 1.4", 200),
      ...
  ])
  def test_read_receipt_with_empty_body(...):

(The user agents are just examples here; I'm not sure of the specifics!)

• Pass custom_headers to make_request to set the user agent header to the test argument.
• Change the final assertEqual call to compare to expected_status_code.

Cool, thanks for the guidance. What are the most important platforms/versions to test? I know some instances from some of our previous discussions, but I'm not fully apprised of all the version options out there.

Examples (first is from reivilibre's comment, third and fifth are from callahad's issue write-up and subsequent discussion):

• Element/1.2.2 (Linux; Android 6) (pass)
• SchildiChat/1.3.6 (Android 11) (pass)
• Element (Riot.im) (Android 9) (pass)
• Element/1.2.1 (fail)
• Element dbg/1.1.8-dev (Android) (fail)

I also ran into an issue (specifically with fcntl) running tests locally since I'm running Windows and not Linux.

That's unfortunate. Maybe WSL can help with this?

I'll look into it.

[reivilibre]

Some corrections. I also noticed your tests are failing (though this might help). Don't forget to click the circle arrows button to re-request a review when you need :)

[reivilibre]

We want to compile this only once (e.g. make it a constant outside of the function) or alternatively use re.match without pre-compiling so that it uses the re compiled pattern cache.

[reivilibre]

the header has a hyphen-minus in the name

Suggested change

	custom_headers=[("User Agent", user_agent)],
	custom_headers=[("User-Agent", user_agent)],

[rogersheu]

Is that standard header formatting or is it specific to this application? I tried sifting through the uses of custom_headers and make_request in the code but could only find a couple of instances.

[reivilibre]

It's part of the HTTP standard. I'm not sure if it's strictly case sensitive, but at the very least the hyphen-minus is required and spaces probably aren't even allowed :)

[rogersheu]

I see. I'll have to read about that then. Thanks!

[reivilibre]

It's part of the HTTP standard. I'm not sure if it's strictly case sensitive, but at the very least the hyphen-minus is required and spaces probably aren't even allowed :)

[reivilibre]

The linter wants to have a word with you.

😄

Running ./script-dev/lint.sh with your virtual environment activated will likely sort it out. Sorry that you only get the status checks after someone approves it; it's a bit painful but it's a GitHub restriction for first-time contributors :(.

[rogersheu]

I thought I'd ran the linter, but it turns out I was in the wrong directory when I ran it. Should be better now!

[reivilibre]

Thanks for your help!

[rogersheu]

Thanks for walking me through these changes, everyone!