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

Return 200 OK for all OPTIONS requests #7534

Merged
merged 7 commits into from
May 22, 2020
Merged

Conversation

clokep
Copy link
Member

@clokep clokep commented May 19, 2020

This is an attempt to fix #7313.

For workers which don't have a media listener it adds dummy endpoints which return a 200 OK for OPTIONS requests to themselves and any leaf below them. Any other requests method (POST, PUT, GET, etc.) is a 404.

For all endpoints under the root endpoint it will return a 200 OK for OPTIONS requests. Other requests will act as "normal".

I do have some questions:

  • Should this only be on workers?
  • This will now return 200 for completely bogus URLs (/_matrix/media/r0/foo/bar). Is that OK?
  • I will likely want to write some tests once some of the above are answered.

@clokep clokep requested a review from a team May 19, 2020 18:37
@clokep
Copy link
Member Author

clokep commented May 19, 2020

I requested review on this to make sure I'm on the right page. I left a few questions in the description which need to be worked through still.

@erikjohnston
Copy link
Member

(I haven't thought about this, but is a possible alternative to just reply to all options requests, no matter the path?)

@clokep
Copy link
Member Author

clokep commented May 20, 2020

@erikjohnston That could be an option (no pun intended) as well. We could probably do it via the root endpoint actually. I'll take a look at that.

@clokep
Copy link
Member Author

clokep commented May 20, 2020

@erikjohnston So looks like it IS possible, although as you said we need to think about whether that makes sense. I just pushed b390d12 although I don't expect anything to break from it.

@erikjohnston
Copy link
Member

Yeah, personally I think just returning OPTIONS everywhere is the easiest thing here (saves having to find all the places where we conditionally add resources).

@erikjohnston
Copy link
Member

  • Should this only be on workers?

In the brave new world there shouldn't be a difference between master and workers :)

  • This will now return 200 for completely bogus URLs (/_matrix/media/r0/foo/bar). Is that OK?

Yeah, it just means that webapps will have to do another round trip to get the 404. This shouldn't happen in production much, so I think its fine.

  • I will likely want to write some tests once some of the above are answered.

🎉

@clokep clokep changed the title Add a dummy listener to workers which don't have the media repo enabled. Return 200 OK for all OPTIONS requests May 22, 2020
@clokep clokep marked this pull request as ready for review May 22, 2020 13:05
@clokep clokep merged commit 4429764 into develop May 22, 2020
@clokep clokep deleted the clokep/media-repo-worker-404 branch May 22, 2020 13:30
erikjohnston added a commit that referenced this pull request May 22, 2020
phil-flex pushed a commit to phil-flex/synapse that referenced this pull request Jun 16, 2020
phil-flex pushed a commit to phil-flex/synapse that referenced this pull request Jun 16, 2020
clokep added a commit that referenced this pull request Oct 22, 2020
The handling of OPTIONS requests was consolidated in #7534, but the endpoint
specific handlers were not removed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

frontend_proxy worker should be able to handle OPTIONS for /_matrix/media requests
2 participants