Skip to content

Commit 3169465

Browse files
authored
[TT-13779] POST form parameters are not logged for Tyk OAS APIs (#7054)
### **User description** <details open> <summary><a href="https://tyktech.atlassian.net/browse/TT-13779" title="TT-13779" target="_blank">TT-13779</a></summary> <br /> <table> <tr> <th>Summary</th> <td>POST form parameters are not logged for Tyk OAS APIs</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 Dev</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%20Commercial_candidate_rel4-2025%20ORDER%20BY%20created%20DESC" title="Commercial_candidate_rel4-2025">Commercial_candidate_rel4-2025</a>, <a href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20codilime_refined%20ORDER%20BY%20created%20DESC" title="codilime_refined">codilime_refined</a>, <a href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20customer_bug%20ORDER%20BY%20created%20DESC" title="customer_bug">customer_bug</a>, <a href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20jira_escalated%20ORDER%20BY%20created%20DESC" title="jira_escalated">jira_escalated</a></td> </tr> </table> </details> <!-- do not remove this marker as it will break jira-lint's functionality. added_by_jira_lint --> --- ## Description When a request with Content-Type "application/x-www-form-urlencoded" is processed by Tyk, the form data (message body) is not included in the analytics logs sent to the pump. The request headers, including Content-Length (which reflects the form data's length), are properly logged, but the actual form data is missing from the message body in the logs. This issue affects the completeness of analytics data for API endpoints that receive form data submissions, making it difficult for users to debug or audit these requests. ## Related Issue ## Motivation and Context ## How This Has Been Tested ## Screenshots (if appropriate) ## Types of changes <!-- What types of changes does your code introduce? Put an `x` in all the boxes that apply: --> - [x] 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. --> - [x] 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 ___ ### **Description** - Fixes deep copying of request body for outgoing requests - Adds error handling for deep copy failures - Ensures POST form parameters are logged for OAS APIs - Updates request body management in reverse proxy handler ___ ### **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>reverse_proxy.go</strong><dd><code>Add deep copy of request body and error handling in reverse proxy</code></dd></summary> <hr> gateway/reverse_proxy.go <li>Implements deepCopyBody to clone request bodies for proxying and <br>logging<br> <li> Integrates deep copy logic into WrappedServeHTTP with error handling<br> <li> Ensures both outgoing and logging requests have correct body content<br> <li> Adds debug logging and error response for deep copy failures </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/7054/files#diff-e6e07722257f7e41691e471185ad6d84fd56dc9e5459526ea32e9a5e8fa1a01b">+31/-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>
1 parent 18f9598 commit 3169465

File tree

3 files changed

+120
-4
lines changed

3 files changed

+120
-4
lines changed

gateway/reverse_proxy.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1097,6 +1097,15 @@ func (p *ReverseProxy) WrappedServeHTTP(rw http.ResponseWriter, req *http.Reques
10971097

10981098
*outreq = *req // includes shallow copies of maps, but okay
10991099
*logreq = *req
1100+
1101+
deepCopyErr := deepCopyBody(req, outreq)
1102+
if deepCopyErr != nil {
1103+
p.logger.Debug("Unable to create deep copy of request, err: ", deepCopyErr)
1104+
p.ErrorHandler.HandleError(rw, logreq, "There was a problem with reading Body of the Request.",
1105+
http.StatusInternalServerError, true)
1106+
return ProxyResponse{}
1107+
}
1108+
11001109
// remove context data from the copies
11011110
setContext(outreq, context.Background())
11021111
setContext(logreq, context.Background())
@@ -1863,6 +1872,28 @@ func nopCloseResponseBody(r *http.Response) {
18631872
copyResponse(r)
18641873
}
18651874

1875+
// Creates a deep copy of source request.Body and replaces target request.Body with it.
1876+
func deepCopyBody(source *http.Request, target *http.Request) error {
1877+
if source == nil || target == nil || source.Body == nil || source.ContentLength == -1 {
1878+
return nil
1879+
}
1880+
1881+
bodyBytes, err := io.ReadAll(source.Body)
1882+
defer func() {
1883+
source.Body.Close()
1884+
source.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))
1885+
nopCloseRequestBody(source)
1886+
}()
1887+
if err != nil {
1888+
return err
1889+
}
1890+
1891+
target.Body = io.NopCloser(bytes.NewBuffer(bodyBytes))
1892+
nopCloseRequestBody(target)
1893+
1894+
return nil
1895+
}
1896+
18661897
// IsUpgrade will return the upgrade header value and true if present for the request.
18671898
// It requires EnableWebSockets to be enabled in the gateway HTTP server config.
18681899
func (p *ReverseProxy) IsUpgrade(req *http.Request) (string, bool) {

gateway/reverse_proxy_test.go

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -377,22 +377,38 @@ func (s *Test) TestNewWrappedServeHTTP() *ReverseProxy {
377377
return s.Gw.TykNewSingleHostReverseProxy(target, spec, nil)
378378
}
379379

380+
func createReverseProxyAndServeHTTP(ts *Test, req *http.Request) (*httptest.ResponseRecorder, ProxyResponse) {
381+
proxy := ts.TestNewWrappedServeHTTP()
382+
recorder := httptest.NewRecorder()
383+
resp := proxy.WrappedServeHTTP(recorder, req, false)
384+
385+
return recorder, resp
386+
}
387+
380388
func TestWrappedServeHTTP(t *testing.T) {
381389
idleConnTimeout = 1
382390

383391
ts := StartTest(nil)
384392
defer ts.Close()
385393

386394
for i := 0; i < 10; i++ {
387-
proxy := ts.TestNewWrappedServeHTTP()
388-
recorder := httptest.NewRecorder()
389-
req, _ := http.NewRequest(http.MethodGet, "/", nil)
390-
proxy.WrappedServeHTTP(recorder, req, false)
395+
req := httptest.NewRequest(http.MethodGet, "/", nil)
396+
_, _ = createReverseProxyAndServeHTTP(ts, req)
391397
}
392398

393399
assert.Equal(t, 10, ts.Gw.ConnectionWatcher.Count())
394400
time.Sleep(time.Second * 2)
395401
assert.Equal(t, 0, ts.Gw.ConnectionWatcher.Count())
402+
403+
// Test error on deepCopyBody function
404+
mockReadCloser := createMockReadCloserWithError(errors.New("test error"))
405+
req := httptest.NewRequest(http.MethodPost, "/test", mockReadCloser)
406+
// Set any ContentLength - httptest.NewRequest sets it only for bytes.Buffer, bytes.Reader and strings.Reader
407+
req.ContentLength = 1
408+
recorder, proxyResponse := createReverseProxyAndServeHTTP(ts, req)
409+
assert.NotNil(t, proxyResponse, "error on deepCopyBody should return an empty ProxyResponse")
410+
assert.Nil(t, proxyResponse.Response, "no response should be expected on error")
411+
assert.Equal(t, http.StatusInternalServerError, recorder.Code)
396412
}
397413

398414
func TestCircuitBreaker5xxs(t *testing.T) {
@@ -863,6 +879,44 @@ func TestNopCloseResponseBody(t *testing.T) {
863879
}
864880
}
865881

882+
func TestDeepCopyBody(t *testing.T) {
883+
var src *http.Request
884+
var trg *http.Request
885+
assert.Nil(t, deepCopyBody(src, trg), "nil requests should remain nil without any error")
886+
887+
src = &http.Request{}
888+
trg = &http.Request{}
889+
assert.Nil(t, deepCopyBody(src, trg), "nil source request body should return without any error")
890+
891+
testData := []byte("testDeepCopy")
892+
src = httptest.NewRequest(http.MethodPost, "/test", bytes.NewReader(testData))
893+
src.ContentLength = -1
894+
assert.Nil(t, deepCopyBody(src, trg),
895+
"source request with ContentLength == -1 should return without any error")
896+
assert.Nil(t, trg.Body, "target request body should not be updated when ContentLength == -1")
897+
898+
src = httptest.NewRequest(http.MethodPost, "/test", bytes.NewReader(testData))
899+
assert.Nil(t, deepCopyBody(src, trg), "request with body should return without any error")
900+
assert.NotNil(t, trg.Body, "target request body should be updated")
901+
assert.True(t, src.Body != trg.Body, "target request should have different body than source request")
902+
903+
trgData, err := io.ReadAll(trg.Body)
904+
assert.Nil(t, err, "target request body should be readable")
905+
assert.Equal(t, testData, trgData, "target request body should contain the same data")
906+
907+
mockReadCloser := createMockReadCloserWithError(errors.New("test error"))
908+
src = httptest.NewRequest(http.MethodPost, "/test", mockReadCloser)
909+
// Set any ContentLength - httptest.NewRequest sets it only for bytes.Buffer, bytes.Reader and strings.Reader
910+
src.ContentLength = int64(len(testData))
911+
trg = &http.Request{}
912+
err = deepCopyBody(src, trg)
913+
assert.NotNil(t, err, "function should return an error when ReadAll fails")
914+
assert.Nil(t, trg.Body, "target request body should not be updated when ReadAll fails")
915+
assert.True(t, mockReadCloser.CloseCalled, "close function should have been called")
916+
_, ok := src.Body.(*nopCloserBuffer)
917+
assert.True(t, ok, "target request body should have been of type nopCloserBuffer")
918+
}
919+
866920
func BenchmarkGraphqlUDG(b *testing.B) {
867921
g := StartTest(func(globalConf *config.Config) {
868922
globalConf.OpenTelemetry.Enabled = true

gateway/testutil.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2124,3 +2124,34 @@ func TestHelperSSEStreamClient(tb testing.TB, ts *Test, enableWebSockets bool) e
21242124
assert.Equal(tb, i, 5)
21252125
return nil
21262126
}
2127+
2128+
// MockErrorReader is a mock io.Reader that returns an error on Read
2129+
type MockErrorReader struct {
2130+
ReturnError error
2131+
}
2132+
2133+
func (e *MockErrorReader) Read(_ []byte) (n int, err error) {
2134+
return 0, e.ReturnError
2135+
}
2136+
2137+
type MockReadCloser struct {
2138+
Reader io.Reader
2139+
CloseError error
2140+
CloseCalled bool
2141+
}
2142+
2143+
func (m *MockReadCloser) Read(p []byte) (n int, err error) {
2144+
return m.Reader.Read(p)
2145+
}
2146+
2147+
func (m *MockReadCloser) Close() error {
2148+
m.CloseCalled = true
2149+
2150+
return m.CloseError
2151+
}
2152+
2153+
func createMockReadCloserWithError(err error) *MockReadCloser {
2154+
return &MockReadCloser{
2155+
Reader: &MockErrorReader{err},
2156+
}
2157+
}

0 commit comments

Comments
 (0)