Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add StringWriter interface to ResponseWriter #921

Closed
wants to merge 5 commits into from
Closed

add StringWriter interface to ResponseWriter #921

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a0bcdb2
add StringWriter interface to ResponseWriter
romainmenke Nov 20, 2023
3ef48cb
add tests
romainmenke Nov 20, 2023
16a9792
docs
romainmenke Nov 20, 2023
8abb514
Update http/interceptor.go
romainmenke Nov 20, 2023
74ca0a5
apply suggestions from code review
romainmenke Nov 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 32 additions & 18 deletions http/interceptor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,11 +104,25 @@ func (i *rwInterceptor) Write(b []byte) (int, error) {
return i.w.Write(b)
}

// WriteString buffers the response body until the request body limit is reach or an
// interruption is triggered, this buffer is later used to analyse the body in
// the response processor.
// If the body isn't accessible or the mime type isn't processable, the response
// body is being written to the delegate response writer directly.
func (i *rwInterceptor) WriteString(s string) (n int, err error) {
return i.Write([]byte(s))
}

func (i *rwInterceptor) Header() http.Header {
return i.w.Header()
}

var _ http.ResponseWriter = (*rwInterceptor)(nil)
type stringResponseWriter interface {
romainmenke marked this conversation as resolved.
Show resolved Hide resolved
http.ResponseWriter
io.StringWriter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mind pointing at an example of another similar code that implements io.StringWriter? This doesn't seem so bad, but I'm wondering when a caller wouldn't just call Write([]byte(s)) themselves without inspecting the interfaces.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to a thing but now I only find old references.
Like these :

I suspect that WriteString on ResponseWriter is no longer common and that Write([]bytes(s)) is the idiomatic way.

We still have a bunch of tests to make sure that all middleware we add implements the needed interfaces. But it has been a while (±6 years) since I revisited those tests.

e.g. CloseNotifier is also unimplemented here but that is documented as deprecated in http.

So, yeah, this is ok to close for me :)
It can always be revisited if there is an actual need for this and not because of some legacy requirement.

Thank you for the super speedy review 🙇

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for the investigation @romainmenke! Yeah let's go ahead and close for now then, if we find a strong use case for it we can reintroduce it.

}

var _ stringResponseWriter = (*rwInterceptor)(nil)

// wrap wraps the interceptor into a response writer that also preserves
// the http interfaces implemented by the original response writer to avoid
Expand Down Expand Up @@ -175,103 +189,103 @@ func wrap(w http.ResponseWriter, r *http.Request, tx types.Transaction) (
switch {
case !isHijacker && !isPusher && !isFlusher && !isReader:
return struct {
http.ResponseWriter
stringResponseWriter
}{i}, responseProcessor
case !isHijacker && !isPusher && !isFlusher && isReader:
return struct {
http.ResponseWriter
stringResponseWriter
io.ReaderFrom
}{i, reader}, responseProcessor
case !isHijacker && !isPusher && isFlusher && !isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Flusher
}{i, flusher}, responseProcessor
case !isHijacker && !isPusher && isFlusher && isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Flusher
io.ReaderFrom
}{i, flusher, reader}, responseProcessor
case !isHijacker && isPusher && !isFlusher && !isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Pusher
}{i, pusher}, responseProcessor
case !isHijacker && isPusher && !isFlusher && isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Pusher
io.ReaderFrom
}{i, pusher, reader}, responseProcessor
case !isHijacker && isPusher && isFlusher && !isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Pusher
http.Flusher
}{i, pusher, flusher}, responseProcessor
case !isHijacker && isPusher && isFlusher && isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Pusher
http.Flusher
io.ReaderFrom
}{i, pusher, flusher, reader}, responseProcessor
case isHijacker && !isPusher && !isFlusher && !isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Hijacker
}{i, hijacker}, responseProcessor
case isHijacker && !isPusher && !isFlusher && isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Hijacker
io.ReaderFrom
}{i, hijacker, reader}, responseProcessor
case isHijacker && !isPusher && isFlusher && !isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Hijacker
http.Flusher
}{i, hijacker, flusher}, responseProcessor
case isHijacker && !isPusher && isFlusher && isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Hijacker
http.Flusher
io.ReaderFrom
}{i, hijacker, flusher, reader}, responseProcessor
case isHijacker && isPusher && !isFlusher && !isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Hijacker
http.Pusher
}{i, hijacker, pusher}, responseProcessor
case isHijacker && isPusher && !isFlusher && isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Hijacker
http.Pusher
io.ReaderFrom
}{i, hijacker, pusher, reader}, responseProcessor
case isHijacker && isPusher && isFlusher && !isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Hijacker
http.Pusher
http.Flusher
}{i, hijacker, pusher, flusher}, responseProcessor
case isHijacker && isPusher && isFlusher && isReader:
return struct {
http.ResponseWriter
stringResponseWriter
http.Hijacker
http.Pusher
http.Flusher
io.ReaderFrom
}{i, hijacker, pusher, flusher, reader}, responseProcessor
default:
return struct {
http.ResponseWriter
stringResponseWriter
}{i}, responseProcessor
}
}
30 changes: 30 additions & 0 deletions http/interceptor_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
package http

import (
"io"
"net/http"
"net/http/httptest"
"testing"
Expand Down Expand Up @@ -44,3 +45,32 @@ func TestWriteHeader(t *testing.T) {
t.Errorf("unexpected status code, want %d, have %d", want, have)
}
}

func TestWriteString(t *testing.T) {
waf, err := coraza.NewWAF(coraza.NewWAFConfig())
if err != nil {
t.Fatal(err)
}

tx := waf.NewTransaction()
req, _ := http.NewRequest("GET", "", nil)
res := httptest.NewRecorder()
rw, responseProcessor := wrap(res, req, tx)
n, err := rw.(io.StringWriter).WriteString("hello")
if err != nil {
t.Fatal(err)
}

if n != 5 {
t.Errorf("unexpected number of bytes written, want 5, have %d", n)
}

err = responseProcessor(tx, req)
if err != nil {
t.Errorf("unexpected error: %v", err)
}

if want, have := "hello", res.Body.String(); want != have {
t.Errorf("unexpected response body, want %s, have %s", want, have)
}
}