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

Handle missing Content-Type for remote media #11044

Closed
squahtx opened this issue Oct 11, 2021 · 6 comments · Fixed by #11200
Closed

Handle missing Content-Type for remote media #11044

squahtx opened this issue Oct 11, 2021 · 6 comments · Fixed by #11200
Assignees
Labels
S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.

Comments

@squahtx
Copy link
Contributor

squahtx commented Oct 11, 2021

Synapse expects other homeservers to always provide the Content-Type header when serving media. However, this header is not required by the spec (https://matrix.org/docs/spec/client_server/r0.6.1#id67) and Synapse raises an error when fetching media without a Content-Type header.

As a solution, we could either:

  • default to application/octet-stream when fetching media without a Content-Type header. We do this for URL previews somewhere.
  • preserve the absence of the header by recording a NULL content type in the database. No schema changes are required. Some mypy-guided code fixes are needed to ensure we handle these NULLs correctly.

Note that regardless of the solution chosen, Synapse will not generate or serve thumbnails for any remote media without a Content-Type header by default, even if the remote homeserver does have thumbnails.

https://sentry.matrix.org/sentry/synapse-matrixorg/issues/200963/

@squahtx squahtx added S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues. X-Needs-Discussion labels Oct 11, 2021
@callahad
Copy link
Contributor

callahad commented Oct 11, 2021

There's also the option of:

  • Update the spec to require Content-Type headers on media

(Though we'll still need to do one of the above proposals, in the spirit of Postel's Law)

@aaronraimist
Copy link
Contributor

default to application/octet-stream when fetching media without a Content-Type header. We do this for URL previews somewhere.

This is what matrix-org/matrix-spec-proposals#2701 says should be done

@callahad
Copy link
Contributor

callahad commented Oct 14, 2021

Conclusion from triage:

  • We will assume application/octet-stream per the HTTP/1.1 spec which reads "If a Content-Type header field is not present, the recipient MAY either assume a media type of application/octet-stream or examine the data to determine its type."

  • We will not distinguish between cases where the application/octet-stream media type was explicit and cases where it was assumed; this means we can apply that default value during ingestion of the media.

  • The database itself allows the content type type to be NULL, so we should ensure that our application code does not throw an exception when encountering NULL values from the database.

@richvdh
Copy link
Member

richvdh commented Oct 18, 2021

Synapse expects other homeservers to always provide the Content-Type header when serving media.

Which endpoint are we talking about here? Presumably GET /_matrix/media/r0/download/{serverName}/{mediaId} ?

@squahtx
Copy link
Contributor Author

squahtx commented Oct 18, 2021

Which endpoint are we talking about here? Presumably GET /_matrix/media/r0/download/{serverName}/{mediaId} ?

Yes, /download/{serverName}/{mediaId} on the remote homeserver.

@squahtx
Copy link
Contributor Author

squahtx commented Oct 21, 2021

Our /upload endpoint is also similarly non-spec compliant and may be worth fixing at the same time if it's not too much effort:

if headers.hasHeader(b"Content-Type"):
content_type_headers = headers.getRawHeaders(b"Content-Type")
assert content_type_headers # for mypy
media_type = content_type_headers[0].decode("ascii")
else:
raise SynapseError(msg="Upload request missing 'Content-Type'", code=400)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
S-Minor Blocks non-critical functionality, workarounds exist. T-Defect Bugs, crashes, hangs, security vulnerabilities, or other reported issues.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants