Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Fix unsafe hotserving behaviour for non-multimedia uploads. #15680

Merged
merged 23 commits into from
Jun 15, 2023
Merged

Fix unsafe hotserving behaviour for non-multimedia uploads. #15680

merged 23 commits into from
Jun 15, 2023

Conversation

joshqou
Copy link
Contributor

@joshqou joshqou commented May 29, 2023

Ensures that the Media API only returns a disposition type of inline for media, return attachment for everything else. NVT#1548992

Signed-off-by: Josh Qou jqou@icloud.com

@joshqou joshqou requested a review from a team as a code owner May 29, 2023 03:22
@joshqou
Copy link
Contributor Author

joshqou commented May 30, 2023

Patch works but synapse has some strict tests that are failing, can't find where the tests referencing perl are so not much I can do.

@clokep
Copy link
Member

clokep commented May 31, 2023

Patch works but synapse has some strict tests that are failing, can't find where the tests referencing perl are so not much I can do.

It looks like linting is failing, you should be able to run black (or scripts-dev/lint.sh) and it should fix it for you.

@joshqou
Copy link
Contributor Author

joshqou commented May 31, 2023

@clokep The actual fails relate to the media api tests, it's just the change I tried making that's causing the lint fail. I'm not too familiar with poetry so I'd appreciate some help from a more experienced synapse contributor with fixing the tests so this can get merged in.

@joshqou joshqou mentioned this pull request Jun 1, 2023
Closed
@joshqou
Copy link
Contributor Author

joshqou commented Jun 1, 2023

Currently blocked due to matrix-org/sytest/#1352.

@joshqou joshqou mentioned this pull request Jun 1, 2023
Merged
clokep
clokep reviewed Jun 1, 2023
View reviewed changes
synapse/media/_base.py Outdated Show resolved Hide resolved
clokep
clokep reviewed Jun 1, 2023
View reviewed changes
synapse/media/_base.py Outdated Show resolved Hide resolved
@joshqou @clokep
Update synapse/media/_base.py
af6e331 
Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>
clokep
clokep reviewed Jun 1, 2023
View reviewed changes
changelog.d/15680.bugfix Outdated Show resolved Hide resolved
@clokep clokep requested a review from a team June 4, 2023 11:41
@erikjohnston
Copy link
Member

Does this also force downloading of things like PDFs? I know that users do appreciate the ability to click on the PDF and have it open in a browser tab.

@joshqou
Copy link
Contributor Author

joshqou commented Jun 8, 2023 •
edited
Loading

@erikjohnston Yes. Adding a mimetype whitelist would fix that, however would also complicate the PR. The current media tests only really make sense for a single disposition type, so supporting both inline and attachment would require most of the tests to be rewritten or changed.

@reivilibre
Copy link
Contributor

Does this also force downloading of things like PDFs? I know that users do appreciate the ability to click on the PDF and have it open in a browser tab.

I tested this in Firefox and Element Web.

What I observed:

  • both before and after this change, if you click on a PDF attachment (in an unencrypted room) in Element Web, the PDF is downloaded and opened in a browser tab (using default settings for PDF handling in Firefox) — the downloaded copy is what is opened (via file:/// URI).
  • before this change, accessing a PDF file directly https://syn1:8448/_matrix/media/v3/download/syn1/DCwMApqkfYSGtJSmpiJgXkiw would open it inline
  • after this change, accessing a PDF file directly as above does the same as what Element Web does: the PDF is downloaded and Firefox opens the PDF in a new tab via file:///.

What's crucially notable is that Element Web was unaffected.
I am ignoring the IRC bridging concern because I'm not familiar, but I guess that would hit the difference noted in points 2 and 3.

Given the security benefit, I don't see anything that should put us off here.

@reivilibre
Copy link
Contributor

The only things I can see blocking us from taking this right now:

Other than that I think it would be fine?

@joshqou
Copy link
Contributor Author

joshqou commented Jun 13, 2023

@reivilibre Made PRs for dendrite and complement.

@reivilibre reivilibre enabled auto-merge (squash) June 15, 2023 11:29
@reivilibre reivilibre disabled auto-merge June 15, 2023 13:23
@reivilibre reivilibre merged commit d939120 into matrix-org:develop Jun 15, 2023
@anoadragon453 anoadragon453 mentioned this pull request Jun 16, 2023
Merged
@Danieloni1 Danieloni1 mentioned this pull request Jun 19, 2023
Merged
This was referenced Jul 5, 2023
Closed
Closed
@reivilibre reivilibre mentioned this pull request Jul 21, 2023
Closed
yingziwu added a commit to yingziwu/synapse that referenced this pull request Aug 21, 2023
@yingziwu
Merge tag 'v1.87.0' into develop
635426a 
Please note that this will be the last release of Synapse that is compatible with
Python 3.7 and earlier.
This is due to Python 3.7 now having reached End of Life; see our [deprecation policy](https://matrix-org.github.io/synapse/v1.87/deprecation_policy.html)
for more details.

- Pin `pydantic` to `^1.7.4` to avoid backwards-incompatible API changes from the 2.0.0 release.
  Resolves matrix-org#15858.
  Contributed by @PaarthShah. ([\matrix-org#15862](matrix-org#15862))

- Split out 2022 changes from the changelog so the rendered version in GitHub doesn't timeout as much. ([\matrix-org#15846](matrix-org#15846))

- Improve `/messages` response time by avoiding backfill when we already have messages to return. ([\matrix-org#15737](matrix-org#15737))
- Add spam checker module API for logins. ([\matrix-org#15838](matrix-org#15838))

- Fix a long-standing bug where media files were served in an unsafe manner. Contributed by @joshqou. ([\matrix-org#15680](matrix-org#15680))
- Avoid invalidating a cache that was just prefilled. ([\matrix-org#15758](matrix-org#15758))
- Fix requesting multiple keys at once over federation, related to [MSC3983](matrix-org/matrix-spec-proposals#3983). ([\matrix-org#15770](matrix-org#15770))
- Fix joining rooms through aliases where the alias server isn't a real homeserver. Contributed by @tulir @ Beeper. ([\matrix-org#15776](matrix-org#15776))
- Fix a bug in push rules handling leading to an invalid (per spec) `is_user_mention` rule sent to clients. Also fix wrong rule names for `is_user_mention` and `is_room_mention`. ([\matrix-org#15781](matrix-org#15781))
- Fix a bug introduced in 1.57.0 where the wrong table would be locked on updating database rows when using SQLite as the database backend. ([\matrix-org#15788](matrix-org#15788))
- Fix Sytest environmental variable evaluation in CI. ([\matrix-org#15804](matrix-org#15804))
- Fix forgotten rooms missing from initial sync after rejoining them. Contributed by Nico from Famedly. ([\matrix-org#15815](matrix-org#15815))
- Fix sqlite `user_filters` upgrade introduced in v1.86.0. ([\matrix-org#15817](matrix-org#15817))

- Document `looping_call()` functionality that will wait for the given function to finish before scheduling another. ([\matrix-org#15772](matrix-org#15772))
- Fix a typo in the [Admin API](https://matrix-org.github.io/synapse/latest/usage/administration/admin_api/index.html). ([\matrix-org#15805](matrix-org#15805))
- Fix typo in MSC number in faster remote room join architecture doc. ([\matrix-org#15812](matrix-org#15812))

- Remove experimental [MSC2716](matrix-org/matrix-spec-proposals#2716) implementation to incrementally import history into existing rooms. ([\matrix-org#15748](matrix-org#15748))

- Replace `EventContext` fields `prev_group` and `delta_ids` with field `state_group_deltas`. ([\matrix-org#15233](matrix-org#15233))
- Regularly try to send transactions to other servers after they failed instead of waiting for a new event to be available before trying. ([\matrix-org#15743](matrix-org#15743))
- Fix requesting multiple keys at once over federation, related to [MSC3983](matrix-org/matrix-spec-proposals#3983). ([\matrix-org#15755](matrix-org#15755))
- Allow for the configuration of max request retries and min/max retry delays in the matrix federation client. ([\matrix-org#15783](matrix-org#15783))
- Switch from `matrix://` to `matrix-federation://` scheme for internal Synapse routing of outbound federation traffic. ([\matrix-org#15806](matrix-org#15806))
- Fix harmless exceptions being printed when running the port DB script. ([\matrix-org#15814](matrix-org#15814))

* Bump attrs from 22.2.0 to 23.1.0. ([\matrix-org#15801](matrix-org#15801))
* Bump cryptography from 40.0.2 to 41.0.1. ([\matrix-org#15800](matrix-org#15800))
* Bump ijson from 3.2.0.post0 to 3.2.1. ([\matrix-org#15802](matrix-org#15802))
* Bump phonenumbers from 8.13.13 to 8.13.14. ([\matrix-org#15798](matrix-org#15798))
* Bump ruff from 0.0.265 to 0.0.272. ([\matrix-org#15799](matrix-org#15799))
* Bump ruff from 0.0.272 to 0.0.275. ([\matrix-org#15833](matrix-org#15833))
* Bump serde_json from 1.0.96 to 1.0.97. ([\matrix-org#15797](matrix-org#15797))
* Bump serde_json from 1.0.97 to 1.0.99. ([\matrix-org#15832](matrix-org#15832))
* Bump towncrier from 22.12.0 to 23.6.0. ([\matrix-org#15831](matrix-org#15831))
* Bump types-opentracing from 2.4.10.4 to 2.4.10.5. ([\matrix-org#15830](matrix-org#15830))
* Bump types-setuptools from 67.8.0.0 to 68.0.0.0. ([\matrix-org#15835](matrix-org#15835))
@clokep clokep mentioned this pull request Oct 6, 2023
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@clokep clokep clokep left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joshqou @clokep @erikjohnston @reivilibre