[DO NOT MERGE] bug: CORS headers not returned when mw shortcircuits upsteam #1550

asoorm · 2018-03-20T10:17:01Z

In the current scenario, when the gateway does not handle CORS, and
OptionsPassthrough is set to true, if the tyk middleware shortcircuits
the response, tyk is not able to return the required upstream CORS
headers.

This fixes the issue by detecting the appropriate conditions, and
sending an OPTIONS request via loopback, then appending the full set of
CORS headers to the error response.

asoorm · 2018-03-20T10:21:57Z

PR submitted for early feedback please

asoorm · 2018-03-20T10:46:43Z

Present implementation: If a POST request is sent with a body, we get an error. But if body is empty, then it works as expected.

2018/03/20 10:44:51 http: multiple response.WriteHeader calls

Need to investigate this.

letzya · 2018-03-20T15:17:46Z

Per @asoorm have copied from the main issue:
What about other response codes? they would just be blocked?
For instance:

When rate limit exceeded
When global rate limit exceeded 429
When quota exceeded
When access denided
Access to this API has been disallowed
Key has expired, please renew 401

Sorry, haven't thoroughly checked, this is off the top of my head

dencoded · 2018-03-20T15:38:40Z

middleware.go

 			err, errCode := mw.ProcessRequest(w, r, mwConf)
 			if err != nil {

+				if errCode == http.StatusForbidden {


how would we know that this http.StatusForbidden caused by one of our middlewares, not from actual upstream?
I think if it is reply from upstream we don't need to do anything as it was resposibility of upstream to set CORS headers and they might already there.

also, how about other HTTP 4XX which can be set by other our middlewares?

Because at the point this has been injected - it's from the middleware, and not the upstream. So at this stage, the request has not yet been proxied and we are about to return an error to the user.

my understanding is that this depends on order of middlewares. it is not super clear for me that the request has not yet been proxied

dencoded · 2018-03-20T15:42:45Z

middleware.go

+	corsRes, err := corsClient.Do(corsRequest)
+	if err != nil {
+		log.WithError(err).Error("Error from upstream OPTIONS request, ignoring and returning default response")
+	}


connection leak, we should do defer corsRes.Body.Close()

dencoded · 2018-03-20T15:47:30Z

middleware.go

+	}
+
+	corsClient := &http.Client{Transport: tr}
+	corsRes, err := corsClient.Do(corsRequest)


this needs to have some kind of circuit breaker or at least time out for client to avoid waiting for upstream when it is down (if we don't it could cause bump in gateway latency)

So the circuit breaker I am not sure is required. because we are calling Tyk itself but this time, as a pre-flight request.

oh, I though we were going to send OPTIONS to upstream directly. it seems to me that calling GW itself again is an extra step (but it should work). I'd add timeout to request anyways

That's why it's called loopback. because the gw calls it's own API. The reasoning for this is because the OPTIONS request needs to go through the exact same MW chain as the original pre-flight request would have done before so that we get the same request to the same upstream endpoint.

e.g. there may be MW in place which re-writes the target.

ok, got it. according to original pre-flight request - some browsers don't send pre-flight requests for all ajax calls, so original OPTIONS one might never happened. I don't think it would cause any issues in this case, just wanted to mention it.

I am still not sure about this loopback round trip business, as looks like all we want to do is just to hit the code placed a couple of lines above here: https://github.com/TykTechnologies/tyk/pull/1550/files#diff-4c166b743d95629a9ab37eb6fe3df22aR86

why not just to prepare OPTIONS request like we do anyways and response writer (we can probably use httptest recoder) and then call h.ServeHTTP(optionsRecorder, optionsRequest) to make it go through all underlaying middle-wares down to upstream?

dencoded · 2018-03-20T15:50:54Z

middleware.go

+		TLSClientConfig: &tls.Config{InsecureSkipVerify: config.Global.HttpServerOptions.SSLInsecureSkipVerify},
+	}
+
+	corsClient := &http.Client{Transport: tr}


I'd add corsClient as field of middleware and change loopbackCORS to be method of middleware so we won't re-create http.Client within every call of loopbackCORS and benefit from keep-alived connection

dencoded · 2018-03-20T15:52:56Z

middleware.go

+		log.WithError(err).Error("Error from upstream OPTIONS request, ignoring and returning default response")
+	}
+
+	corsHeaders := map[string]string{}


might worth to cache the reply with using as key Origin received from browser to improve latency and avoid bombing upstream with OPTIONS for the same origin every time we error

dencoded · 2018-03-20T15:54:48Z

middleware.go

+
+	for k, v := range corsHeaders {
+		if v != "" {
+			w.Header().Set(k, v)


we need to make sure that response wasn't already sent to browser at this point. setting headers on http.ResponseWriter when response already sent won't take any affect

In the current scenario, when the gateway does not handle CORS, and OptionsPassthrough is set to true, if the tyk middleware shortcircuits the response, tyk is not able to return the required upstream CORS headers. This fixes the issue by detecting the appropriate conditions, and sending an OPTIONS request via loopback, then appending the full set of CORS headers to the error response.

by default, setting timeout to 2 seconds. Potential issue with becoming flooded with POST requests with failed auth - causing loopback OPTIONS requests. Need to investigate and look into further.

stale · 2020-03-25T14:38:20Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs, please add comments to this ticket if you would like it to stay open. Thank you for your contributions.

asoorm requested review from buger and lonelycode March 20, 2018 10:17

asoorm force-pushed the 1506-cors-invalid-token branch from 909ddd8 to 593debb Compare March 20, 2018 10:18

asoorm changed the title ~~bug: CORS headers not returned when mw shortcircuits upsteam~~ [DO NOT MERGE] bug: CORS headers not returned when mw shortcircuits upsteam Mar 20, 2018

asoorm force-pushed the 1506-cors-invalid-token branch from 593debb to 2711ade Compare March 20, 2018 10:24

asoorm force-pushed the 1506-cors-invalid-token branch 2 times, most recently from d0bdbe5 to 7ea4f2e Compare March 20, 2018 11:28

asoorm requested a review from dencoded March 20, 2018 15:21

dencoded reviewed Mar 20, 2018

View reviewed changes

asoorm force-pushed the 1506-cors-invalid-token branch from 7ea4f2e to 04f0c36 Compare March 20, 2018 15:45

dencoded reviewed Mar 20, 2018

View reviewed changes

asoorm added 4 commits April 3, 2018 11:06

lint: package for response helpers

a1d5099

fixing connection leak

ee25115

adding timeout to http.Client

5b16d40

by default, setting timeout to 2 seconds. Potential issue with becoming flooded with POST requests with failed auth - causing loopback OPTIONS requests. Need to investigate and look into further.

asoorm force-pushed the 1506-cors-invalid-token branch from af429ec to 5b16d40 Compare April 3, 2018 10:06

stale bot added the wontfix label Mar 25, 2020

stale bot closed this Apr 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO NOT MERGE] bug: CORS headers not returned when mw shortcircuits upsteam #1550

[DO NOT MERGE] bug: CORS headers not returned when mw shortcircuits upsteam #1550

asoorm commented Mar 20, 2018 •

edited

asoorm commented Mar 20, 2018

asoorm commented Mar 20, 2018

letzya commented Mar 20, 2018

dencoded Mar 20, 2018

asoorm Mar 20, 2018

dencoded Mar 20, 2018 •

edited

dencoded Mar 20, 2018

dencoded Mar 20, 2018

asoorm Mar 20, 2018

dencoded Mar 20, 2018

asoorm Mar 20, 2018

dencoded Mar 20, 2018

dencoded Mar 20, 2018

dencoded Mar 20, 2018

dencoded Mar 20, 2018

dencoded Mar 20, 2018 •

edited

stale bot commented Mar 25, 2020

[DO NOT MERGE] bug: CORS headers not returned when mw shortcircuits upsteam #1550

[DO NOT MERGE] bug: CORS headers not returned when mw shortcircuits upsteam #1550

Conversation

asoorm commented Mar 20, 2018 • edited

asoorm commented Mar 20, 2018

asoorm commented Mar 20, 2018

letzya commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dencoded Mar 20, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dencoded Mar 20, 2018 • edited

Choose a reason for hiding this comment

stale bot commented Mar 25, 2020

asoorm commented Mar 20, 2018 •

edited

dencoded Mar 20, 2018 •

edited

dencoded Mar 20, 2018 •

edited