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

Make OPTIONS method on MSC3916 endpoints available without auth #3431

Merged
merged 1 commit into from
Sep 22, 2024
Merged

Make OPTIONS method on MSC3916 endpoints available without auth #3431

merged 1 commit into from
Sep 22, 2024

Conversation

arenekosreal
Copy link
Contributor

@arenekosreal arenekosreal commented Sep 20, 2024

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>

@arenekosreal arenekosreal requested a review from a team as a code owner September 20, 2024 02:57
@arenekosreal arenekosreal changed the title Make OPTIONS method on MSC3916 endpoints available without authentica… Make OPTIONS method on MSC3916 endpoints available without auth Sep 20, 2024
Copy link
Contributor

@S7evinK S7evinK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 #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>
@arenekosreal
Copy link
Contributor Author

I have changed the code to check OPTIONS in MakeHTTPAPI, I must say the diff is much cleaner than the old one.

Copy link
Contributor

@S7evinK S7evinK left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're using http.StatusOK in MakeJSONAPI, so I'd keep that the same.

Copy link

codecov bot commented Sep 20, 2024

Codecov Report

Attention: Patch coverage is 0% with 4 lines in your changes missing coverage. Please review.

Project coverage is 68.36%. Comparing base (f2db7cb) to head (29028d7).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
internal/httputil/httpapi.go 0.00% 4 Missing ⚠️
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     
Flag Coverage Δ
unittests 53.35% <0.00%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@S7evinK S7evinK merged commit df770da into matrix-org:main Sep 22, 2024
19 of 20 checks passed
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.

Authenticated media requires authentication even on OPTIONS method
2 participants