Skip to content

Fix unbounded memory growth DoS vectors in ollama and opencodecommon #100

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

Closed
matdev83 wants to merge 1 commit into from
Closed

Fix unbounded memory growth DoS vectors in ollama and opencodecommon #100

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion internal/plugins/backends/ollama/plugin.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func fetchVersion(ctx context.Context, client *http.Client, nativeRoot string) (
if resp.StatusCode < 200 || resp.StatusCode > 299 {
return "", fmt.Errorf("version HTTP status %d", resp.StatusCode)
}
body, err := io.ReadAll(resp.Body)
body, err := io.ReadAll(io.LimitReader(resp.Body, 1024*1024))

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🔵 Trivial | 💤 Low value

No truncation detection when the 1 MiB limit is hit.

io.ReadAll(io.LimitReader(resp.Body, 1024*1024)) returns no error when the body is truncated at exactly 1 MiB — it looks identical to a legitimately-sized response that ends at that byte count. If a real /api/version payload were ever larger (e.g., a proxy injecting extra headers/whitespace), the truncated JSON would fail with a generic parse error rather than a clear "response too large" error, making the failure mode confusing to diagnose. Low risk here since version payloads are tiny, but worth a quick fix for clarity.

♻️ Suggested truncation-aware read 
-	body, err := io.ReadAll(io.LimitReader(resp.Body, 1024*1024))
+	const maxVersionBody = 1024 * 1024
+	body, err := io.ReadAll(io.LimitReader(resp.Body, maxVersionBody+1))
 	if err != nil {
 		return "", err
 	}
+	if len(body) > maxVersionBody {
+		return "", fmt.Errorf("version response too large (exceeds %d bytes)", maxVersionBody)
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
body, err := io.ReadAll(io.LimitReader(resp.Body, 1024*1024))
const maxVersionBody = 1024 * 1024
body, err := io.ReadAll(io.LimitReader(resp.Body, maxVersionBody+1))
if err != nil {
return "", err
}
if len(body) > maxVersionBody {
return "", fmt.Errorf("version response too large (exceeds %d bytes)", maxVersionBody)
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/plugins/backends/ollama/plugin.go` at line 141, The version-check
read in the Ollama backend does not detect when
`io.ReadAll(io.LimitReader(resp.Body, 1024*1024))` hits the 1 MiB cap, so
truncation is indistinguishable from a valid full response. Update the
`/api/version` handling in `plugin.go` to use a truncation-aware read path in
the same block where `resp.Body` is consumed, and return a clear “response too
large” error when the limit is reached instead of letting `json.Unmarshal` fail
later. Keep the fix localized around the body read and parse logic so the `body,
err` flow remains easy to find.

if err != nil {
return "", err
}
Expand Down
19 changes: 19 additions & 0 deletions internal/plugins/backends/ollama/plugin_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,22 @@
t.Fatal("expected explicitly disabled responses to not be supported")
}
}

func TestFetchVersion_OOM_Regression(t *testing.T) {

Check failure on line 142 in internal/plugins/backends/ollama/plugin_test.go

View workflow job for this annotation

GitHub Actions / qa 

Function TestFetchVersion_OOM_Regression missing the call to method parallel (paralleltest)
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
chunk := make([]byte, 1024*1024)
for i := 0; i < 50; i++ {

Check failure on line 146 in internal/plugins/backends/ollama/plugin_test.go

View workflow job for this annotation

GitHub Actions / qa 

rangeint: for loop can be modernized using range over int (modernize)
w.Write(chunk)

Check failure on line 147 in internal/plugins/backends/ollama/plugin_test.go

View workflow job for this annotation

GitHub Actions / qa 

Error return value of `w.Write` is not checked (errcheck)
}
}))
defer srv.Close()

ctx, cancel := context.WithCancel(context.Background())

Check failure on line 152 in internal/plugins/backends/ollama/plugin_test.go

View workflow job for this annotation

GitHub Actions / qa 

testingcontext: context.WithCancel can be modernized using t.Context (modernize)
defer cancel()

_, err := fetchVersion(ctx, srv.Client(), srv.URL)
if err == nil {
t.Fatalf("expected error due to large payload or invalid json, got none")
}
}
Comment on lines +142 to +159

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Test doesn't actually validate the OOM fix — it would pass identically without io.LimitReader.

The handler writes 50MB of zero-valued bytes, which is never valid JSON regardless of how much is read. fetchVersion would return an error from json.Unmarshal on an all-zero payload whether the read was bounded to 1MiB or fully unbounded — the test can't distinguish "bounded read, then invalid JSON" from "full unbounded read, then invalid JSON." This means reverting the io.LimitReader fix would not make this regression test fail, defeating its purpose as a regression guard.

To actually assert bounded memory, either:

  • Serve a payload that would successfully parse as valid JSON if fully read (e.g., {"version":"1.0.0", "padding":"...50MB of chars..."}), so the test fails specifically when reading is truncated versus when it's unbounded, or
  • Instrument/verify that only ~1MiB was actually read from the server (e.g., track bytes written vs. connection closed early), rather than only asserting err != nil.

As per path instructions, tests should be "testing observable behavior over implementation details" and provide "meaningful assertions" — the current assertion (err == nil) is too weak to catch a regression where the limit is removed.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/plugins/backends/ollama/plugin_test.go` around lines 142 - 159, The
regression test in TestFetchVersion_OOM_Regression does not prove the read is
bounded because the 50MB zero-filled body is invalid JSON either way. Update the
test to use a payload that would be valid JSON if fully read, or verify the
server only sees about 1MiB read from fetchVersion, so the test fails if the
io.LimitReader behavior is removed. Use fetchVersion, the httptest server
handler, and the test’s assertion to make the behavior observable rather than
relying on err != nil alone.

Source: Path instructions

2 changes: 1 addition & 1 deletion internal/plugins/backends/opencodecommon/catalog/loader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ func getJSON(ctx context.Context, client *http.Client, endpoint string, headers
body, _ := io.ReadAll(io.LimitReader(resp.Body, 4096))
return nil, fmt.Errorf("opencodecommon: model discovery HTTP status %d: %s", resp.StatusCode, strings.TrimSpace(string(body)))
}
body, err := io.ReadAll(resp.Body)
body, err := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024))

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Silent truncation at 10 MB on the success path with no distinguishing error.

Unlike the non-2xx error path above (line 132, which only embeds a snippet in an error string), this line bounds the actual catalog payload that gets parsed and returned to callers. If a real catalog response ever exceeds 10MB, the body is silently truncated and json.Valid(body) will fail with "invalid JSON" — giving no indication that the real cause is a size cap rather than a malformed upstream response. This makes future debugging of legitimate large-catalog growth harder to distinguish from actual upstream corruption.

♻️ Suggested truncation-aware read 
-	body, err := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024))
+	const maxCatalogBody = 10 * 1024 * 1024
+	body, err := io.ReadAll(io.LimitReader(resp.Body, maxCatalogBody+1))
 	if err != nil {
 		return nil, fmt.Errorf("opencodecommon: model discovery read: %w", err)
 	}
+	if len(body) > maxCatalogBody {
+		return nil, fmt.Errorf("opencodecommon: model discovery response too large (exceeds %d bytes)", maxCatalogBody)
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
body, err := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024))
const maxCatalogBody = 10 * 1024 * 1024
body, err := io.ReadAll(io.LimitReader(resp.Body, maxCatalogBody+1))
if err != nil {
return nil, fmt.Errorf("opencodecommon: model discovery read: %w", err)
}
if len(body) > maxCatalogBody {
return nil, fmt.Errorf("opencodecommon: model discovery response too large (exceeds %d bytes)", maxCatalogBody)
}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/plugins/backends/opencodecommon/catalog/loader.go` at line 135, The
success-path read in loader.go is silently truncating the catalog payload at 10
MB, which can make oversized but valid responses look like invalid JSON. Update
the response handling around the io.ReadAll/io.LimitReader logic in the catalog
loader so it detects when the body exceeds the cap and returns a distinct,
truncation-aware error from the catalog fetch/parse flow instead of proceeding
to json.Valid on partial data. Keep the fix localized to the loader’s
response-reading path and make the error clearly identify the size limit
condition.

if err != nil {
return nil, fmt.Errorf("opencodecommon: model discovery read: %w", err)
}
Expand Down
19 changes: 19 additions & 0 deletions internal/plugins/backends/opencodecommon/catalog/loader_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,3 +115,22 @@
t.Fatal("expected error without fallback")
}
}

func TestGetJSON_OOM_Regression(t *testing.T) {

Check failure on line 119 in internal/plugins/backends/opencodecommon/catalog/loader_test.go

View workflow job for this annotation

GitHub Actions / qa 

Function TestGetJSON_OOM_Regression missing the call to method parallel (paralleltest)
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
chunk := make([]byte, 1024*1024)
for i := 0; i < 50; i++ {

Check failure on line 123 in internal/plugins/backends/opencodecommon/catalog/loader_test.go

View workflow job for this annotation

GitHub Actions / qa 

rangeint: for loop can be modernized using range over int (modernize)
w.Write(chunk)

Check failure on line 124 in internal/plugins/backends/opencodecommon/catalog/loader_test.go

View workflow job for this annotation

GitHub Actions / qa 

Error return value of `w.Write` is not checked (errcheck)
}
}))
defer srv.Close()

ctx, cancel := context.WithCancel(context.Background())

Check failure on line 129 in internal/plugins/backends/opencodecommon/catalog/loader_test.go

View workflow job for this annotation

GitHub Actions / qa 

testingcontext: context.WithCancel can be modernized using t.Context (modernize)
defer cancel()

_, err := getJSON(ctx, srv.Client(), srv.URL, nil)
if err == nil {
t.Fatalf("expected error due to large payload or invalid json, got none")
}
}
Comment on lines +119 to +136

@coderabbitai coderabbitai Bot Jul 1, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Same weak-assertion issue as the ollama regression test — doesn't verify bounded reads.

The 50MB payload here is also all-zero bytes, which fails json.Valid regardless of whether it was read in full or truncated at 10MB. Reverting the io.LimitReader(resp.Body, 10*1024*1024) fix would not cause this test to fail, since json.Valid would still reject the fully-read, unbounded zero-byte payload. This test therefore does not actually guard against the OOM regression it's named for.

Consider serving a payload that is valid JSON when fully read (so the pre-fix code would succeed) but becomes invalid once truncated at the boundary — this way the test fails without the fix and passes with it.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/plugins/backends/opencodecommon/catalog/loader_test.go` around lines
119 - 136, The OOM regression test in getJSON is too weak because the all-zero
payload is invalid JSON even when fully read, so it does not prove reads are
bounded. Update TestGetJSON_OOM_Regression to use a payload served by
httptest.NewServer that is valid JSON when read completely but becomes invalid
if truncated at the 10MB boundary, so getJSON only passes with the
io.LimitReader(resp.Body, 10*1024*1024) safeguard. Keep the assertion in
TestGetJSON_OOM_Regression focused on distinguishing bounded from unbounded
reads, rather than merely checking for any error.

Source: Path instructions

Loading