bug: pkgHTTP.Request RespHeader() Add override previous value

[goxiaoy]

Issue description

w http.ResponseWriter, r pkgHTTP.Request
r.RespHeader().Add("Set-Cookie","a=a; Expires=Wed, 21 Oct 2023 07:28:00 GMT; Secure; HttpOnly")
r.RespHeader().Add("Set-Cookie","b=b; Expires=Wed, 21 Oct 2023 07:28:00 GMT; Secure; HttpOnly")

Cookie a is missing in the response header

Environment

• APISIX Go Plugin Runner's version: dee7fa0
• APISIX version: 2.13.0
• Go version: 1.18
• OS (cmd: uname -a): centos

Minimal test code / Steps to reproduce the issue

1. Start APISIX on docker
2. Set Plugin filter codes

Filter(conf interface{}, w http.ResponseWriter, r pkgHTTP.Request)
{
  r.RespHeader().Add("Set-Cookie","a=a; Expires=Wed, 21 Oct 2023 07:28:00 GMT; Secure; HttpOnly")
  r.RespHeader().Add("Set-Cookie","b=b; Expires=Wed, 21 Oct 2023 07:28:00 GMT; Secure; HttpOnly")
  return
}

4. Cookie a is missing in the response header

What's the actual result? (including assertion message & call stack if applicable)

Cookie b can be found, Cookie a is missing

What's the expected result?

Both cookie a and b could be found

[goxiaoy]

https://github.com/apache/apisix/blob/f624fb0a4d33e5aa674ec659e9d778da2ef9860f/apisix/plugins/ext-plugin/init.lua#L626-L635

            local len = rewrite:RespHeadersLength()
            if len > 0 then
                for i = 1, len do
                    local entry = rewrite:RespHeaders(i)
                    local name = entry:Name()
                    if exclude_resp_header[str_lower(name)] == nil then
                        core.response.set_header(name, entry:Value())
                    end
                end
            end

Codes here should take multiple header with same name into account

@rampagecong @shuaijinchao

[rampagecong]

Very good idea. I'll give it a try.

[shuaijinchao]

Very good idea. I'll give it a try.

Currently in Runner, headers are stored using map, which does not support repeated header settings, which may require us to extend the header key. If you have better ideas, you can always reply here.

cc @goxiaoy

[goxiaoy]

• From the RFC 6265

Origin servers SHOULD NOT fold multiple Set-Cookie header fields into
a single header field. The usual mechanism for folding HTTP headers
fields (i.e., as defined in [RFC2616]) might change the semantics of
the Set-Cookie header field because the %x2C (",") character is used
by Set-Cookie in a way that conflicts with such folding.

So it is a common use case for set multiple header with same name like Set-Cookie

• For the Runner,

  RespHeader is defined as standard golang net/http header, which is a map of array of string

  type Header map[string][]string
  

  And later header is serialized into HeadersVector,
  

  apisix-go-plugin-runner/internal/http/request.go

  Lines 253 to 272 in dee7fa0

  	if r.respHdr != nil {
  	respHdrs := []flatbuffers.UOffsetT{}
  	for n, arr := range r.respHdr {
  	for _, v := range arr {
  	name := builder.CreateString(n)
  	value := builder.CreateString(v)
  	A6.TextEntryStart(builder)
  	A6.TextEntryAddName(builder, name)
  	A6.TextEntryAddValue(builder, value)
  	te := A6.TextEntryEnd(builder)
  	respHdrs = append(respHdrs, te)
  	}
  	}
  	size := len(respHdrs)
  	hrc.RewriteStartRespHeadersVector(builder, size)
  	for i := size - 1; i >= 0; i-- {
  	te := respHdrs[i]
  	builder.PrependUOffsetT(te)
  	}
  	respHdrVec = builder.EndVector(size)

  So I do not think it is a bug of Runner

• For the APISIX ext-plugin

            local len = rewrite:RespHeadersLength()
            if len > 0 then
                for i = 1, len do
                    local entry = rewrite:RespHeaders(i)
                    local name = entry:Name()
                    if exclude_resp_header[str_lower(name)] == nil then
-                        core.response.set_header(name, entry:Value())
+                        core.response.add_header(name, entry:Value())
                    end
                end
            end

Might fix this bug
Should I open a new issue in APISIX?

[rampagecong]

@shuaijinchao I've tested the code above and it worked. But I'm not sure if core.response.add_header will break anything else. Could you please take a look?

[shuaijinchao]

@shuaijinchao I've tested the code above and it worked. But I'm not sure if core.response.add_header will break anything else. Could you please take a look?

You are right, after verification I found no abnormality, @spacewander what do you think?

[spacewander]

The add_header won't override the existent header. What about using a table to track headers, using set_header first and using add_header if the same header occurs again?

[rampagecong]

The add_header won't override the existent header. What about using a table to track headers, using set_header first and using add_header if the same header occurs again?
I think it works.I'll try to fix it.

[jthann]

The problem still exists, not solved

[goxiaoy]

@jthann upgrade apisix to 2.14.0 or newer