Skip to content

fix: stop dropping the bytes between the chunk boundary and the read size - #396

Open
arpitjain099 wants to merge 1 commit into
Checkmarx:masterfrom
arpitjain099:fix/chunk-peek-instead-of-read
Open

fix: stop dropping the bytes between the chunk boundary and the read size#396
arpitjain099 wants to merge 1 commit into
Checkmarx:masterfrom
arpitjain099:fix/chunk-peek-instead-of-read

Conversation

@arpitjain099

Copy link
Copy Markdown

Proposed Changes

ReadChunk reads up to size + maxPeekSize bytes out of the reader, and generateChunk then walks forward from size looking for a safe \n\n split. When it finds one it breaks and returns the prefix. The bytes that the read already consumed after that split are dropped on the floor. The next ReadChunk call starts from where the reader is now, which is past them, so that content never reaches the detectors.

Both loops (engine.go:337 for filesystem, plugins/confluence.go:370 for Confluence pages) call ReadChunk in a loop until io.EOF, so the gap is silent. Nothing errors, the scan just does not cover part of the file.

With a chunk size of 10 and a peek size of 5, scanning "0123456789\n\nSECRET\n" gives:

expected: "0123456789\n\nSECRET\n"
actual  : "0123456789\n\nRET\n"

"SEC" is gone. At the shipped defaults the window is 100KiB/25KiB, so any file over 100KiB can lose up to 25KiB per chunk, and a secret sitting in that region is not scanned.

The fix is to Peek rather than Read, and Discard exactly the number of bytes the returned chunk covers. That is what the "look-ahead" naming already implies. Both call sites build the reader with bufio.NewReaderSize(..., GetSize()+GetMaxPeekSize()), so the peek fits; bufio.ErrBufferFull is tolerated anyway, which means a caller with a smaller reader degrades to a shorter look-ahead instead of erroring.

Error behaviour is unchanged: empty input still returns io.EOF, and the unsupported-file-type check still runs on the first read with the same window of bytes.

Side note: GetPeekedBuf/PutPeekedBuf and peekedBufPool are no longer used by ReadChunk. I left them in place since they are exported and covered by tests. Happy to remove them if you would rather not carry them.

Checklist

  • I covered my changes with tests.
  • I Updated the documentation that is affected by my changes:
    • Change in the CLI arguments
    • Change in the configuration file

TestReadChunkKeepsEveryByte reads a source to EOF and compares the concatenation of every chunk against the input. It fails on master for two of its three cases and passes with this change. go test ./engine/... ./plugins/... is green, and gofmt is clean.

I submit this contribution under the Apache-2.0 license.

…size

ReadChunk consumes up to size+maxPeekSize bytes from the reader, then
generateChunk stops at the first safe "\n\n" boundary it finds past the
chunk size and returns only that prefix. Everything the read consumed after
the boundary is thrown away, and the next call starts from wherever the
reader now sits, so that content is never scanned.

Peek instead of Read and Discard exactly the bytes the chunk covers, which
is what the maxPeekSize name implies. Both call sites already size the
bufio.Reader as size+maxPeekSize, and ErrBufferFull is tolerated so a
smaller reader degrades to a short look-ahead instead of failing.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
@arpitjain099
arpitjain099 requested a review from a team as a code owner August 10, 2026 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant