Added size limit check for sarif upload and implemented nil pointer checks for regions#1838
Added size limit check for sarif upload and implemented nil pointer checks for regions#1838
Conversation
…hecks for regions
There was a problem hiding this comment.
Pull request overview
This PR aims to harden SARIF ingestion by preventing nil-pointer panics when extracting snippet/region data and by introducing a maximum-size gate for first-party SARIF uploads.
Changes:
- Added a nil check for
PhysicalLocation.Regionwhen building snippet metadata for first-party vuln ingestion. - Added a 16 MB size limit check in the SARIF upload controller path (FirstPartyVulnScan) using JSON re-marshalling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| services/scan_service.go | Adds guarding logic around missing SARIF Region when extracting snippet coordinates/content. |
| controllers/scan_controller.go | Adds a SARIF payload size check (16 MB) before proceeding with scan processing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
services/scan_service.go
Outdated
| snippetContent = dtos.SnippetContent{ | ||
| StartLine: utils.OrDefault(loc.PhysicalLocation.Region.StartLine, 0), | ||
| EndLine: utils.OrDefault(loc.PhysicalLocation.Region.EndLine, 0), | ||
| StartColumn: utils.OrDefault(loc.PhysicalLocation.Region.StartColumn, 0), | ||
| EndColumn: utils.OrDefault(loc.PhysicalLocation.Region.EndColumn, 0), | ||
| Snippet: utils.OrDefault(loc.PhysicalLocation.Region.Snippet.Text, ""), |
There was a problem hiding this comment.
loc.PhysicalLocation.Region.Snippet is a pointer in the SARIF schema, so loc.PhysicalLocation.Region.Snippet.Text can still panic when Snippet is omitted (even if Region is non-nil). Please guard for Region.Snippet == nil (treat as empty snippet) and consider adding a regression test for a SARIF location with a region but no snippet.
| snippetContent = dtos.SnippetContent{ | |
| StartLine: utils.OrDefault(loc.PhysicalLocation.Region.StartLine, 0), | |
| EndLine: utils.OrDefault(loc.PhysicalLocation.Region.EndLine, 0), | |
| StartColumn: utils.OrDefault(loc.PhysicalLocation.Region.StartColumn, 0), | |
| EndColumn: utils.OrDefault(loc.PhysicalLocation.Region.EndColumn, 0), | |
| Snippet: utils.OrDefault(loc.PhysicalLocation.Region.Snippet.Text, ""), | |
| snippetText := "" | |
| if loc.PhysicalLocation.Region.Snippet != nil { | |
| snippetText = utils.OrDefault(loc.PhysicalLocation.Region.Snippet.Text, "") | |
| } | |
| snippetContent = dtos.SnippetContent{ | |
| StartLine: utils.OrDefault(loc.PhysicalLocation.Region.StartLine, 0), | |
| EndLine: utils.OrDefault(loc.PhysicalLocation.Region.EndLine, 0), | |
| StartColumn: utils.OrDefault(loc.PhysicalLocation.Region.StartColumn, 0), | |
| EndColumn: utils.OrDefault(loc.PhysicalLocation.Region.EndColumn, 0), | |
| Snippet: snippetText, |
controllers/scan_controller.go
Outdated
| var maxSize int64 = 16 * 1024 * 1024 //Max Upload Size 16mb | ||
| data, err := json.Marshal(&sarifScan) | ||
| if err != nil { | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, err.Error()) | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
This size check happens after ctx.Bind(&sarifScan), so the server has already accepted and parsed the full request body into memory. Additionally, json.Marshal(&sarifScan) re-allocates the payload (potentially doubling memory) and the resulting size can differ from the original upload due to omitempty/normalization. Consider enforcing the limit before binding (e.g., wrap ctx.Request().Body with http.MaxBytesReader / Echo body-limit middleware, and then Bind), and avoid re-marshalling just to compute size.
controllers/scan_controller.go
Outdated
| err = fmt.Errorf("SARIF size %d bytes exceeds limit of %d bytes", len(data), maxSize) | ||
| span.RecordError(err) | ||
| span.SetStatus(codes.Error, err.Error()) | ||
| return echo.NewHTTPError(400, "Malformed SARIF").WithInternal(err) |
There was a problem hiding this comment.
For an oversized payload, returning HTTP 400 with message "Malformed SARIF" is misleading; the SARIF may be valid but too large. Consider using HTTP 413 (Payload Too Large) and an error message that explicitly indicates the size limit was exceeded.
| return echo.NewHTTPError(400, "Malformed SARIF").WithInternal(err) | |
| return echo.NewHTTPError(413, "SARIF payload too large").WithInternal(err) |
Size limit check only prevents big sarifs from being saved to the database. The fill sized data will still reach the backend server and land in memory unless prevented by ratelimiting / size limiting through a reverse-proxy or else.