zalando / skipper / 36174879562

81%

Build Type

push

github

Committed by web-flow

Commit Message

fix: io matcher drops the body beyond the max buffer size in best effort mode (#4299)

Closes #4297

## Symptom

A request body larger than `-max-matcher-buffer-size` (2MiB by default)
does not reach the backend of a `blockContent` route. Route:

```
r: * -> blockContent("Malicious Content") -> "http://backend"
```

The backend prints how many bytes it reads from the body. Through a
skipper built from master with the default buffer size:

```
POST 1048576 bytes: backend received 1048576 bytes
POST 2097152 bytes: backend received 2097152 bytes
POST 3145728 bytes: backend received 0 bytes
```

The client gets the backend's response as if the upload had been
forwarded. Depending on how the reads line up, the backend gets nothing
or only the tail of the body: with a 1024 byte buffer a 12000 byte body
arrives as 856 bytes.

This is the silent form of #2655. The `ContentLength=100003 with Body
length 0` error reported there went away when #2828 started to remove
the `Content-Length` header, but the body is still dropped.

## Cause

`io/read_stream.go`, `matcher.fill`. In best effort mode, when `pending`
grows beyond the max buffer size, the matcher inspects it and then
resets it:

```go
_, err := m.f(m.pending.Bytes())
if err != nil {
	return err
}
m.pending.Reset()
```

`ready`, the buffer that `Read` serves from, is only filled from
`pending` once the input is exhausted, so everything inspected before
that point is discarded. Only what was read after the last reset is
delivered.

## Fix

Hand the inspected data to `ready` instead of discarding it, the same
way the end of input path does.

The existing `maxBuffer` case in `TestMatcherErrorCases` expected a nil
error from reading a 17 byte body that contains the blocked string
through a 5 byte buffer. It passed because only the last two bytes,
`ss`, were delivered. With the body delivered the blocked string is
found, so the case now expects `ErrBlocked`, like its `maxBufferAbort`
twin expects `ErrMatcherBuf... (continued)

Coverage Stats

1 of 1 new or added line in 1 file covered. (100.0%)

20 existing lines in 4 files now uncovered.

27100 of 33694 relevant lines covered (80.43%)

66608.03 hits per line