Skip to content

Commit 969f937

Browse files
edsonmichaquesebkehr
authored and
Tit Petric
committed
[TT-13440] correctly sync multi-value response headers with coprocess middleware (#6883)
### **User description** <details open> <summary><a href="https://tyktech.atlassian.net/browse/TT-13440" title="TT-13440" target="_blank">TT-13440</a></summary> <br /> <table> <tr> <th>Summary</th> <td>[Community PR] Multi-value response headers are lost after sync with coprocess middleware</td> </tr> <tr> <th>Type</th> <td> <img alt="Bug" src="https://tyktech.atlassian.net/rest/api/2/universal_avatar/view/type/issuetype/avatar/10303?size=medium" /> Bug </td> </tr> <tr> <th>Status</th> <td>In Code Review</td> </tr> <tr> <th>Points</th> <td>N/A</td> </tr> <tr> <th>Labels</th> <td><a href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20community_bug%20ORDER%20BY%20created%20DESC" title="community_bug">community_bug</a>, <a href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20github%20ORDER%20BY%20created%20DESC" title="github">github</a></td> </tr> </table> </details> <!-- do not remove this marker as it will break jira-lint's functionality. added_by_jira_lint --> --- …ewares <!-- Provide a general summary of your changes in the Title above --> ## Description When synchronizing single- and multi-valued header representations (of coprocess-based middleware responses) the list of values for any multi-valued header is currently replaced by a list containing only the value given by its single-value representation effectively dropping all but the first value. Instead synchronization should affect/replace only the first value and retain possibly remaining values. <!-- Describe your changes in detail --> ## Related Issue https://tyktech.atlassian.net/browse/TT-13440 #6411 <!-- This project only accepts pull requests related to open issues. --> <!-- If suggesting a new feature or change, please discuss it in an issue first. --> <!-- If fixing a bug, there should be an issue describing it with steps to reproduce. --> <!-- OSS: Please link to the issue here. Tyk: please create/link the JIRA ticket. --> ## Motivation and Context <!-- Why is this change required? What problem does it solve? --> We like to employ Tyk Gateway with a coprocess-based response middleware attached to an upstream possibly responding with multiple _Set-Cookie_ headers. We also require our middleware to modify other headers like _Location_. As is due to synchronization only the first _Set-Cookie_ header passes our middleware. ## How This Has Been Tested This PR adds a [test](https://github.com/TykTechnologies/tyk/pull/6883/files#diff-c0fdc6fa7ea0ffd782deb36e7843d48aefff201af73e79a2c742027cf4557f5f) which should fail without proposed [fix](https://github.com/TykTechnologies/tyk/pull/6883/files#diff-9cbfe628982b2afb94d1e9a5200fc9a4fdc00cb58fe65d1090a3725e4e4c5953). <!-- Please describe in detail how you tested your changes --> <!-- Include details of your testing environment, and the tests --> <!-- you ran to see how your change affects other areas of the code, etc. --> <!-- This information is helpful for reviewers and QA. --> ## Screenshots (if appropriate) ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality) ## Checklist <!-- Go over all the following points, and put an `x` in all the boxes that apply --> <!-- If there are no documentation updates required, mark the item as checked. --> <!-- Raise up any additional concerns not covered by the checklist. --> - [ ] I ensured that the documentation is up to date - [ ] I explained why this PR updates go.mod in detail with reasoning why it's required - [ ] I would like a code coverage CI quality gate exception and have explained why ___ ### **PR Type** Bug fix, Tests ___ ### **Description** - Fixed synchronization of multi-value headers in coprocess middleware. - Modified `syncHeadersAndMultiValueHeaders` to retain additional header values. - Added a new test case to validate multi-value header retention. - Ensured existing functionality remains unaffected with updated logic. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>coprocess.go</strong><dd><code>Update multi-value header synchronization logic</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> gateway/coprocess.go <li>Enhanced synchronization logic for multi-value headers.<br> <li> Ensured first value in multi-value headers matches single-value <br>headers.<br> <li> Preserved additional values in multi-value headers during updates. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6883/files#diff-9cbfe628982b2afb94d1e9a5200fc9a4fdc00cb58fe65d1090a3725e4e4c5953">+7/-1</a>&nbsp; &nbsp; &nbsp; </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>coprocess_test.go</strong><dd><code>Add tests for multi-value header synchronization</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary> <hr> gateway/coprocess_test.go <li>Added test cases for multi-value header synchronization.<br> <li> Covered scenarios with empty and multiple Set-Cookie headers.<br> <li> Verified preservation of additional values in multi-value headers. </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6883/files#diff-c0fdc6fa7ea0ffd782deb36e7843d48aefff201af73e79a2c742027cf4557f5f">+54/-0</a>&nbsp; &nbsp; </td> </tr> </table></td></tr></tr></tbody></table> ___ > <details> <summary> Need help?</summary><li>Type <code>/help how to ...</code> in the comments thread for any questions about PR-Agent usage.</li><li>Check out the <a href="https://qodo-merge-docs.qodo.ai/usage-guide/">documentation</a> for more information.</li></details> --------- Co-authored-by: Sebastian Kehr <kehr@iat.uni-leipzig.de>
1 parent da49250 commit 969f937

File tree

2 files changed

+61
-1
lines changed

2 files changed

+61
-1
lines changed

gateway/coprocess.go

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -611,6 +611,7 @@ func (h *CustomMiddlewareResponseHook) HandleResponse(rw http.ResponseWriter, re
611611
// syncHeadersAndMultiValueHeaders synchronizes the content of 'headers' and 'multiValueHeaders'.
612612
// If a key is updated or added in 'headers', the corresponding key in 'multiValueHeaders' is also updated or added.
613613
// If a key is removed from 'headers', the corresponding key in 'multiValueHeaders' is also removed.
614+
// If multiValuesHeaders contains a key with multiple values and the same key is present in headers, the first value in multiValuesHeaders is updated with the value from headers, while the remaining values remain unchanged.
614615
func syncHeadersAndMultiValueHeaders(headers map[string]string, multiValueHeaders []*coprocess.Header) []*coprocess.Header {
615616
updatedMultiValueHeaders := []*coprocess.Header{}
616617

@@ -619,7 +620,12 @@ func syncHeadersAndMultiValueHeaders(headers map[string]string, multiValueHeader
619620
for _, header := range multiValueHeaders {
620621
if header.Key == k {
621622
found = true
622-
header.Values = []string{v}
623+
624+
// if the key is present in multiValueHeaders, update the first value with the value from headers
625+
if len(header.Values) > 0 {
626+
header.Values[0] = v
627+
}
628+
623629
break
624630
}
625631
}

gateway/coprocess_test.go

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,60 @@ func TestSyncHeadersAndMultiValueHeaders(t *testing.T) {
214214
},
215215
},
216216
},
217+
{
218+
name: "keeping multivalue headers",
219+
headers: map[string]string{
220+
"Header": "newValue1",
221+
},
222+
initialMultiValueHeaders: []*coprocess.Header{
223+
{
224+
Key: "Header",
225+
Values: []string{"oldValue1", "value2"},
226+
},
227+
},
228+
expectedMultiValueHeaders: []*coprocess.Header{
229+
{
230+
Key: "Header",
231+
Values: []string{"newValue1", "value2"},
232+
},
233+
},
234+
},
235+
{
236+
name: "empty multi value headers",
237+
headers: map[string]string{
238+
"Header": "newValue1",
239+
},
240+
initialMultiValueHeaders: []*coprocess.Header{},
241+
expectedMultiValueHeaders: []*coprocess.Header{
242+
{Key: "Header", Values: []string{"newValue1"}},
243+
},
244+
},
245+
{
246+
name: "multiple Set-Cookie headers",
247+
headers: map[string]string{
248+
"Set-Cookie": "session=abc123; Path=/",
249+
},
250+
initialMultiValueHeaders: []*coprocess.Header{
251+
{
252+
Key: "Set-Cookie",
253+
Values: []string{
254+
"session=dce123; Path=/",
255+
"user=john; Path=/",
256+
"theme=dark; Path=/",
257+
},
258+
},
259+
},
260+
expectedMultiValueHeaders: []*coprocess.Header{
261+
{
262+
Key: "Set-Cookie",
263+
Values: []string{
264+
"session=abc123; Path=/",
265+
"user=john; Path=/",
266+
"theme=dark; Path=/",
267+
},
268+
},
269+
},
270+
},
217271
}
218272

219273
for _, tc := range testCases {

0 commit comments

Comments
 (0)