Fix Mastodon not correctly processing HTTP Signatures with query strings

[ClearlyClaire]

Revival of #18474 (GitHub can be confusing…)

cf. https://honk.tedunangst.com/u/tedu/h/1mZMtCVQ1clC7MfBg9

When signing or verifying signatures for requests with query strings, Mastodon incorrectly builds the request-target pseudo-header. Indeed, it does not include the query string, while the HTTP Signatures draft states:

If the header field name is (request-target) then generate the header field value by concatenating the lowercased :method, an ASCII space, and the :path pseudo-headers (as specified in HTTP/2, Section 8.1.2.3). Note: For the avoidance of doubt, lowercasing only applies to the :method pseudo-header and not to the :path pseudo-header.

The :path pseudo-header is defined as:

The ":path" pseudo-header field includes the path and query parts of the target URI (the "path-absolute" production and optionally a '?' character followed by the "query" production (see Sections 3.3 and 3.4 of [RFC3986]). A request in asterisk form includes the value '*' for the ":path" pseudo-header field.

Because this is the first time in years that I have seen someone raise this issue, I think we can assume other implementations got it wrong too (or did not bother reporting it and worked around it instead). Therefore, changing to the correct version of this draft will likely cause compatibility issues. That's why for the time being, this PR only handles incoming requests in addition to the previous broken signatures.

On a side note, the drafts we are currently implementing have been superseded by more recent drafts, which we should probably move to eventually. However, I would like #15605 to be merged before working on that.

[codecov]

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (3944b12) 84.59% compared to head (833289e) 84.58%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
app/controllers/concerns/signature_verification.rb	72.72%	3 Missing ⚠️
app/lib/request.rb	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #28476      +/-   ##
==========================================
- Coverage   84.59%   84.58%   -0.02%     
==========================================
  Files        1039     1039              
  Lines       28240    28254      +14     
  Branches     4550     4556       +6     
==========================================
+ Hits        23891    23900       +9     
- Misses       3197     3202       +5     
  Partials     1152     1152              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[vmstan]

I saw where #28443 got merged. Is this PR now where it could be tested independently?

[renchap]

#28443 was a refactor of the existing tests. This PR changes the behaviour, and adds more tests for the query string cases

[vmstan]

#28443 was a refactor of the existing tests. This PR changes the behaviour, and adds more tests for the query string cases

Understood, just wanted to check it wasn't waiting/dependent on any other changes.

[vmstan]

In #18474 @nachtjasmin mentioned a change in behavior of 401s related to GoToSocial interaction, but 5 days into this patch and I've seen no change continued accumulation of dead jobs:

Mastodon::UnexpectedResponseError: https://alpha.polymaths.social/users/rl_dane/statuses/01HJNZ6SAFJ60N1ZM9M11FQY3F/replies?limit=20&only_other_accounts=true returned code 401

[ClearlyClaire]

In #18474 @nachtjasmin mentioned a change in behavior of 401s related to GoToSocial interaction, but 5 days into this patch and I've seen no change continued accumulation of dead jobs:

Mastodon::UnexpectedResponseError: https://alpha.polymaths.social/users/rl_dane/statuses/01HJNZ6SAFJ60N1ZM9M11FQY3F/replies?limit=20&only_other_accounts=true returned code 401

As written in the PR's description, this PR only handles incoming requests in addition to the previous broken signatures, it does not use correct outgoing requests, but maybe that could be changed.

[vmstan]

Understood.

[renchap]

Looks good to me, made one minor comment to make it easier when deciding when to remove the old behaviour.

[ShadowJonathan]

This PR doesn't actually make use of the new with_query_string argument for Request, still exhibiting the old behaviour.

I'll make a PR that'll deploy a fix described along the lines of superseriousbusiness/gotosocial#894 (comment)