Make OPTIONS method on MSC3916 endpoints available without auth #3431
+6
−0
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
arenekosreal
changed the title
S7evinK
requested changes
Sep 20, 2024
There was a problem hiding this comment.
While this works, I'd prefer having the OPTIONS check in the MakeHTTPAPI function, i.e.
// MakeHTTPAPI adds Span metrics to the HTML Handler function
// This is used to serve HTML alongside JSON error messages
func MakeHTTPAPI(metricsName string, userAPI userapi.QueryAcccessTokenAPI, enableMetrics bool, f func(http.ResponseWriter, *http.Request), checks ...AuthAPIOption) http.Handler {
withSpan := func(w http.ResponseWriter, req *http.Request) {
if req.Method == http.MethodOptions {
util.SetCORSHeaders(w)
w.WriteHeader(http.StatusOK)
return
}
...
…tion. OPTIONS method is usually sent by browser in preflight requests, most of the time we cannot control preflight request to add auth header. Synapse will return a 204 response directly without authentication for those OPTIONS method. According to firefox's documentation, both 200 and 204 are acceptable so I think there is no need to change handler in dendrite. This closes matrix-org#3424 No need to add a test because this is just a fix and I have tested on my Cinny Web client personally. Signed-off-by: arenekosreal <17194552+arenekosreal@users.noreply.github.com>
|
I have changed the code to check |
S7evinK
approved these changes
Sep 20, 2024
| @@ -210,6 +210,12 @@ func MakeExternalAPI(metricsName string, f func(*http.Request) util.JSONResponse | |||
| // This is used to serve HTML alongside JSON error messages | |||
| func MakeHTTPAPI(metricsName string, userAPI userapi.QueryAcccessTokenAPI, enableMetrics bool, f func(http.ResponseWriter, *http.Request), checks ...AuthAPIOption) http.Handler { | |||
| withSpan := func(w http.ResponseWriter, req *http.Request) { | |||
| if req.Method == http.MethodOptions { | |||
| util.SetCORSHeaders(w) | |||
| w.WriteHeader(http.StatusOK) // Maybe http.StatusNoContent? | |||
There was a problem hiding this comment.
We're using http.StatusOK in MakeJSONAPI, so I'd keep that the same.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3431 +/- ##
==========================================
+ Coverage 68.34% 68.36% +0.02%
==========================================
Files 513 513
Lines 47026 47030 +4
==========================================
+ Hits 32138 32153 +15
+ Misses 10896 10886 -10
+ Partials 3992 3991 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
OPTIONS method is usually sent by browser in preflight requests, most of the time we cannot control preflight request to add auth header.
Synapse will return a 204 response directly without authentication for those OPTIONS method.
According to firefox's documentation, both 200 and 204 are acceptable so I think there is no need to change handler in dendrite.
This closes #3424
No need to add a test because this is just a fix and I have tested on my Cinny Web client personally.
Pull Request Checklist
Signed-off-by:
arenekosreal <17194552+arenekosreal@users.noreply.github.com>