You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2022/03/27 13:35:47 UTC
[GitHub] [apisix-go-plugin-runner] Goxiaoy opened a new issue #74: bug: pkgHTTP.Request RespHeader() Add override previous value
Goxiaoy opened a new issue #74:
URL: https://github.com/apache/apisix-go-plugin-runner/issues/74
### 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
Workaround
Concat resp header before return
```
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")
if len(r.RespHeader().Values("Set-Cookie")) > 0 {
r.RespHeader().Set("Set-Cookie", strings.Join(r.RespHeader().Values("Set-Cookie"), "; "))
}
```
### Environment
* APISIX Go Plugin Runner's version: dee7fa0167af0ed8cdcd87d4321a43b194f2a7ed
* 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-go-plugin-runner] Goxiaoy commented on issue #74: bug: pkgHTTP.Request RespHeader() Add override previous value
Posted by GitBox <gi...@apache.org>.
Goxiaoy commented on issue #74:
URL: https://github.com/apache/apisix-go-plugin-runner/issues/74#issuecomment-1080392755
* From the [RFC 6265](https://www.rfc-editor.org/rfc/rfc6265)
> 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](https://www.rfc-editor.org/rfc/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,
https://github.com/apache/apisix-go-plugin-runner/blob/dee7fa0167af0ed8cdcd87d4321a43b194f2a7ed/internal/http/request.go#L253-L272
So I do not think it is a bug of Runner
* For the APISIX [ext-plugin](https://github.com/apache/apisix/tree/f624fb0a4d33e5aa674ec659e9d778da2ef9860f/apisix/plugins/ext-plugin)
```diff
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-go-plugin-runner] Goxiaoy commented on issue #74: bug: pkgHTTP.Request RespHeader() Add override previous value
Posted by GitBox <gi...@apache.org>.
Goxiaoy commented on issue #74:
URL: https://github.com/apache/apisix-go-plugin-runner/issues/74#issuecomment-1079964248
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-go-plugin-runner] rampagecong commented on issue #74: bug: pkgHTTP.Request RespHeader() Add override previous value
Posted by GitBox <gi...@apache.org>.
rampagecong commented on issue #74:
URL: https://github.com/apache/apisix-go-plugin-runner/issues/74#issuecomment-1080118845
Very good idea. I'll give it a try.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-go-plugin-runner] spacewander commented on issue #74: bug: pkgHTTP.Request RespHeader() Add override previous value
Posted by GitBox <gi...@apache.org>.
spacewander commented on issue #74:
URL: https://github.com/apache/apisix-go-plugin-runner/issues/74#issuecomment-1082539107
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-go-plugin-runner] rampagecong edited a comment on issue #74: bug: pkgHTTP.Request RespHeader() Add override previous value
Posted by GitBox <gi...@apache.org>.
rampagecong edited a comment on issue #74:
URL: https://github.com/apache/apisix-go-plugin-runner/issues/74#issuecomment-1082733530
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-go-plugin-runner] rampagecong commented on issue #74: bug: pkgHTTP.Request RespHeader() Add override previous value
Posted by GitBox <gi...@apache.org>.
rampagecong commented on issue #74:
URL: https://github.com/apache/apisix-go-plugin-runner/issues/74#issuecomment-1080448325
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-go-plugin-runner] rampagecong commented on issue #74: bug: pkgHTTP.Request RespHeader() Add override previous value
Posted by GitBox <gi...@apache.org>.
rampagecong commented on issue #74:
URL: https://github.com/apache/apisix-go-plugin-runner/issues/74#issuecomment-1082733530
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-go-plugin-runner] shuaijinchao commented on issue #74: bug: pkgHTTP.Request RespHeader() Add override previous value
Posted by GitBox <gi...@apache.org>.
shuaijinchao commented on issue #74:
URL: https://github.com/apache/apisix-go-plugin-runner/issues/74#issuecomment-1080362829
> 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-go-plugin-runner] shuaijinchao edited a comment on issue #74: bug: pkgHTTP.Request RespHeader() Add override previous value
Posted by GitBox <gi...@apache.org>.
shuaijinchao edited a comment on issue #74:
URL: https://github.com/apache/apisix-go-plugin-runner/issues/74#issuecomment-1081305622
> @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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [apisix-go-plugin-runner] shuaijinchao commented on issue #74: bug: pkgHTTP.Request RespHeader() Add override previous value
Posted by GitBox <gi...@apache.org>.
shuaijinchao commented on issue #74:
URL: https://github.com/apache/apisix-go-plugin-runner/issues/74#issuecomment-1081305622
> @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?
Maybe you are right, I need some time to verify.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@apisix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org