The Proxy filter lost all response headers set by other filters(eg: CORSAdaptor) before it #713

jthann · 2022-07-23T00:09:45Z

Describe the bug
In Easegress v2.0.0，the Proxy filter will lost all response headers set by other filters. For example if using CORSAdaptor before Proxy filter，for simple cors request，it will set Vary Access-Control-Allow-Origin Access-Control-Allow-Credentials headers. All these headers will lost after Proxy filter handling. This will cause simple cors request cannot work properly when api and web page from different domain.

To Reproduce
Using following configuration under Easegress v2.0.0：(echo-backend service providing 9095 port is from Easegress example
frontend just use ResponseBuilder to build a html page containing javascript that send Cors request to api domain)

Make sure adding following configuration to/etc/hosts file:

127.0.0.1 frontend.work api.backend.work

kind: HTTPServer
name: http
port: 80
https: false
rules:
  - host: frontend.work
    paths:
      - PathPrefix: /page
        backend: frontend
  - host: api.backend.work
    paths:
      - pathPrefix: /api
        backend: echo-backend
---
name: frontend
kind: Pipeline
flow:
  - filter: respBuilder
  - filter: rsp
filters:
  - name: respBuilder
    kind: ResponseBuilder
    template: |
      statusCode: 200
  - name: rsp
    kind: ResponseAdaptor
    body: >+
      <!DOCTYPE html>
      <html>   
      <body>
      <h1>This is Frontend Page!</h1>
      <script>
          fetch('http://api.backend.work/api', {
              mode: "cors",
              method: "POST",
              credentials: "include",
              cache: 'no-cache',              
          }).then(data => console.log(data))
            .catch((error) => {console.log('Error', error)});
      </script>
      </body>
      </html>
---
name: echo-backend
kind: Pipeline
flow:
  - filter: cors-filter
    jumpIf: { preflighted: END }
  - filter: proxy
filters:
  - name: cors-filter
    kind: CORSAdaptor
    supportCORSRequest: true
    allowedOrigins: ["http://frontend.work"]
    allowCredentials: true
    allowedMethods: ["GET","POST"]
  - name: proxy
    kind: Proxy
    pools:
      - servers:
          - url: http://127.0.0.1:9095
        loadBalance:
          policy: roundRobin

Open up chrome web browser and access http://frontend.work/，after sending request to http://api.backend.work/api by javascript in html，the browser will report error: CORS error MissingAllowOriginHeader

Expected behavior
The Proxy filter should reserve all response headers set by other filters configuring before it.

The text was updated successfully, but these errors were encountered:

easegress-io#713

jthann · 2022-07-27T22:58:20Z

@suchen-sci @localvar The solution does not solve the response headers loss problem completely. It only handle the CORS response headers. But there are also other headers loss problem, let me clarify: in our business scene, we have a GlobalFilter before Proxy filter. In this GlobalFilter we need to judge if a JWT token in user cookie is nearly expired, If it is, we need to refresh user cookies by setting multi Set-Cookie response headers. After Proxy filters handling, the Set-Cookie response headers will be lost. So I think we should have a common solution for all response headers as following code，not just for special headers.

The current solution code block

	if r, _ := spCtx.GetOutputResponse().(*httpprot.Response); r != nil {
		copyCORSHeaders(resp.HTTPHeader(), r.HTTPHeader())
                //omit some lines
	}

maybe should change as following

	if r, _ := spCtx.GetOutputResponse().(*httpprot.Response); r != nil {
		for k, v := range r.HTTPHeader() {
			for _, vv := range v {   
				if strings.HasPrefix(k, "Access-Control-Allow-") {
					resp.Header().Set(k, vv)
				} else {
					resp.Header().Add(k, vv)
				}
			}
		}
	}

Note that two nested for loop is to handle same Header names like multi Set-Cookie，the if judgement strings.HasPrefix(k, "Access-Control-Allow-") is to handle some cases in which the backend services like many spring boot service also handle the CORS request and will return multi CORS headers to client causing web browsers report errors.

localvar · 2022-07-28T01:45:11Z

@jthann , got the idea. but we still need to consider these items:

for the CORS headers, I think it is better to keep the behavior of the current copyCORSHeaders.
for other headers, is it correct to always call Add?
when copying a header, we need to keep the order of the values, that's, the values of previous request first.

any comments?

suchen-sci · 2022-07-28T02:26:33Z

Maybe there are two cases here:

proxy response replaces previous response.
there is no previous response, just some headers.

In first case, I think we should not copy the headers. In second case, we should copy the response. Since we provide multiple namespaces, so in practice, first case should not exist. User should use different namespaces for different responses. So we only need to take care about second case, where we should simply copy all headers.

Any idea here?

localvar · 2022-07-28T02:37:49Z

@suchen-sci agree, but we still need to consider the 3 issues I mentioned before, simply copying all headers may be incorrect.

suchen-sci · 2022-07-28T02:53:30Z

@localvar my naive opinions about the 3 issues are:

Agree. since CORS filter is our built-in filter, i think current copyCORSHeaders is cool.
I think always call Add is ok, because if a header is not supposed to add to response, it should be put in different namespace response.
i think maybe put values of previous request first is better. If customer don't want this behavior, they can put their custom filter after the proxy filter in pipeline flow.

localvar · 2022-07-28T03:09:43Z

@suchen-sci for the 2nd, I think Set is desired in some cases.

And notice another issue in the CORSAdaptor, we should not always create a new response, if there's already one, we should reuse it.

jthann · 2022-07-28T03:30:57Z

@localvar For the 2nd issue，always call Add for other headers is ok except for CORS headers. From browser perspective，duplicate CORS headers will report error，other headers is ok even duplicated.
By the way，is there any way or docs to help me understand v2.0.0 design of different namespaces for different requests or responses?

localvar · 2022-07-28T03:48:47Z

@jthann @suchen-sci for example: at first, header X-Foo contains value hello, and we get a new value world, if we use Set, it will only contain world, while if we use Add, it will contain both hello and world, I think this makes some applications behave differently.

and you can find a example of multiple namespaces at: #539 (comment)

jthann · 2022-07-28T13:46:16Z

@localvar @suchen-sci I just investigate some other gateways and do analysis about how they handle this problem.
There are three of these:Spring Cloud Gateway、Spring Cloud Netflix Zuul、APISIX(OpenResty based on Nginx)

First Spring Cloud Gateway just add response headers including headers set by filters and those returned from upstream backend service. Except that it's default behavior removing some headers as following in HEADERS_REMOVED_ON_REQUEST from source code

   /**
    * Headers to remove as the result of applying the filter.
    */
   public static final Set<String> HEADERS_REMOVED_ON_REQUEST = new HashSet<>(
   		Arrays.asList("connection", "keep-alive", "transfer-encoding", "te",
   				"trailer", "proxy-authorization", "proxy-authenticate",
   				"x-application-context", "upgrade"
   		// these two are not listed in
   		// https://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-14#section-7.1.3
   		// "proxy-connection",
   		// "content-length",
   		));

It also provides the interface: org.springframework.cloud.gateway.filter.headers.HttpHeadersFilter.
User can implement this interface and register to determine which headers to ignore from backend upstream service.

Second Spring Cloud Netflix Zuul has a default builtin behavior that will ignore these headers from upstream backend service, including connection、content-length、server、transfer-encoding、x-application-context from org.springframework.cloud.netflix.zuul.filters.ProxyRequestHelper.isIncludedHeader. Also it provides
the configuration item zuul.ignored-headers, user can configure to decide which headers to ignore from upstream backend service.

Third APISIX(OpenResty based on Nginx) which also provide CORS plugin，but unlike other gateways
this plugin implements callback function header_filter which is executed after response returned from backend upstream service. In this function using set to make sure CORS headers will not duplicate. For APISIX like CORS plugin, user can implement plugin to handle which headers should return to client. By the way, for Nginx its default behavior is just add all response headers from upstream backend service.

So based on above analysis, the conclusion is that there is not a default solution that will meet all requirements.
Easegress can have a default implementation to make sure no response headers lost, after that it should have some
configuration item in proxy filter to support that user can determine which headers should be ignored and not return to clients.

localvar · 2022-07-29T00:22:17Z

Thanks for the detailed investigation, we will follow your proposal to fix the issue:

Easegress can have a default implementation to make sure no response headers lost, after that it should have some
configuration item in proxy filter to support that user can determine which headers should be ignored and not return to clients.

localvar · 2022-07-29T09:40:12Z

hi @jthann , my new PR (#727) keeps all the headers of the existing response, largely following the go reverse proxy example at: https://github.com/golang/go/blob/95b68e1e02fa713719f02f6c59fb1532bd05e824/src/net/http/httputil/reverseproxy.go

but I haven't added a configuration to let users determine headers they want to discard, I'd like to wait until some users ask for it, I think this could help us to understand the requirement better.

jthann · 2022-07-30T01:05:59Z

@localvar Yeah，the solution is okay，Easegress as a platform should solve the most common user requirements and provide a solid foundation for custom development easily.

In addition to response headers，there are also request headers cases，for example in microservice authentication，we don't expect request Authorization headers to be proxied to backend，including other sensitive headers like Cookie、 X-Frame-Options etc，because all this sensitive headers should be digested in gateway layer.

So under this case，Easegress may provide a configuration item to support which sensitive headers to be ignored and not transferred to backend services.

localvar · 2022-08-01T00:42:37Z

@jthann , ignore request/response headers are already supported by using a RequestAdaptor or ResponseAdaptor.

jthann added a commit to jthann/easegress that referenced this issue Jul 23, 2022

fix Proxy filter missing response headers bug

1476e97

easegress-io#713

jthann mentioned this issue Jul 23, 2022

fix Proxy filter missing response headers bug #714

Closed

localvar assigned suchen-sci Jul 25, 2022

jthann mentioned this issue Jul 25, 2022

Update pool.go #721

Closed

xxx7xxxx closed this as completed in 9d8544a Jul 27, 2022

localvar reopened this Jul 28, 2022

xxx7xxxx closed this as completed in 4e01442 Aug 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The Proxy filter lost all response headers set by other filters(eg: CORSAdaptor) before it #713

The Proxy filter lost all response headers set by other filters(eg: CORSAdaptor) before it #713

jthann commented Jul 23, 2022

jthann commented Jul 27, 2022 •

edited

Loading

localvar commented Jul 28, 2022

suchen-sci commented Jul 28, 2022

localvar commented Jul 28, 2022

suchen-sci commented Jul 28, 2022

localvar commented Jul 28, 2022

jthann commented Jul 28, 2022

localvar commented Jul 28, 2022 •

edited

Loading

jthann commented Jul 28, 2022 •

edited

Loading

localvar commented Jul 29, 2022

localvar commented Jul 29, 2022

jthann commented Jul 30, 2022 •

edited

Loading

localvar commented Aug 1, 2022

The Proxy filter lost all response headers set by other filters(eg: CORSAdaptor) before it #713

The Proxy filter lost all response headers set by other filters(eg: CORSAdaptor) before it #713

Comments

jthann commented Jul 23, 2022

jthann commented Jul 27, 2022 • edited Loading

localvar commented Jul 28, 2022

suchen-sci commented Jul 28, 2022

localvar commented Jul 28, 2022

suchen-sci commented Jul 28, 2022

localvar commented Jul 28, 2022

jthann commented Jul 28, 2022

localvar commented Jul 28, 2022 • edited Loading

jthann commented Jul 28, 2022 • edited Loading

localvar commented Jul 29, 2022

localvar commented Jul 29, 2022

jthann commented Jul 30, 2022 • edited Loading

localvar commented Aug 1, 2022

jthann commented Jul 27, 2022 •

edited

Loading

localvar commented Jul 28, 2022 •

edited

Loading

jthann commented Jul 28, 2022 •

edited

Loading

jthann commented Jul 30, 2022 •

edited

Loading