PMM-15129 Fix gosec Go linter issues (p2)#5640
Draft
maxkondr wants to merge 5 commits into
Draft
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5640 +/- ##
==========================================
+ Coverage 43.59% 44.32% +0.73%
==========================================
Files 415 415
Lines 43134 42953 -181
==========================================
+ Hits 18804 19041 +237
+ Misses 22454 22022 -432
- Partials 1876 1890 +14
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses gosec-reported issues across PMM components by adding overflow/underflow protections for integer conversions, tightening HTTP server hardening (e.g., Slowloris mitigation), improving error handling for I/O/close paths, and extending tests for edge cases in QAN and managed services.
Changes:
- Added range checks (and/or explicit lint suppressions) around potentially unsafe integer conversions and sensitive-field JSON marshaling.
- Hardened HTTP servers and handlers (e.g.,
ReadHeaderTimeout, improved write/close error handling). - Expanded unit tests for pagination/validation and overflow scenarios (sparklines, PITR parsing, templates, checks, backup logs).
Reviewed changes
Copilot reviewed 30 out of 30 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| vmproxy/proxy/proxy.go | Logs response write failures instead of ignoring them. |
| qan-api2/models/reporter.go | Adds validation and overflow protection for sparkline timeframe calculations. |
| qan-api2/models/reporter_test.go | Adds validation/overflow tests for reporter sparklines logic. |
| qan-api2/models/metrics.go | Adds validation and overflow protection for metrics sparklines timeframe calculations. |
| qan-api2/models/metrics_test.go | Adds sparklines tests for invalid ranges, small ranges, and overflow. |
| qan-api2/main.go | Improves shutdown logging and handler write error handling for debug endpoint. |
| managed/utils/validators/alerting_rules.go | Handles temp file close/remove errors more explicitly. |
| managed/utils/encryption/helpers.go | Cleans custom encryption key path from env var. |
| managed/utils/encryption/encryption.go | Cleans encryption path and documents gosec suppression for admin-controlled path read. |
| managed/services/user/user.go | Adds range checks/logging around uint32 conversions for API response fields. |
| managed/services/server/server.go | Uses bounded parsing for dummy gRPC code injection path and returns proper gRPC errors. |
| managed/services/server/server_test.go | Adds tests for Version dummy behaviors (panic + grpc code parsing cases). |
| managed/services/server/logs.go | Improves close error handling via errors.Join and documents zip-copy gosec suppression rationale. |
| managed/services/management/checks.go | Adds pagination math changes and severity range warnings for advisor check results. |
| managed/services/management/checks_test.go | Adds tests for pagination behavior, empty results, and severity overflow scenario; expands advisor listing tests. |
| managed/services/management/backup/restore_service.go | Skips out-of-range chunk IDs before casting to uint32. |
| managed/services/management/backup/restore_service_test.go | Adds tests ensuring invalid chunk IDs are skipped; removes test DB logger. |
| managed/services/management/backup/backup_service.go | Skips out-of-range chunk IDs before casting to uint32. |
| managed/services/management/backup/backup_service_test.go | Adds tests ensuring invalid chunk IDs are skipped; removes test DB logger. |
| managed/services/backup/pitr_timerange_service.go | Hardens PITR timestamp/index parsing against overflow/underflow. |
| managed/services/backup/pitr_timerange_service_test.go | Adds PITR parsing tests for overflow/underflow scenarios. |
| managed/services/alerting/service.go | Clamps total counts to int32 range and warns on severity range before enum cast. |
| managed/services/alerting/service_test.go | Adds tests for severity overflow/underflow and template pagination. |
| managed/services/agents/service_info_broker.go | Clamps PostgreSQL database count before casting to int32. |
| managed/services/agents/actions.go | Documents gosec suppression for placeholders count conversion. |
| managed/models/role_helpers.go | Adds warning and documents gosec suppression for role ID uint32 conversion. |
| managed/models/encryption_helpers.go | Adds documented gosec suppressions around JSON marshal of sensitive-field structs. |
| managed/models/artifact_helpers.go | Adjusts metadata trimming logic in a way intended to address overflow concerns. |
| managed/cmd/pmm-managed/main.go | Adds ReadHeaderTimeout and improves debug handler write error handling. |
| managed/cmd/pmm-encryption-rotation/main.go | Logs DB close errors instead of ignoring them. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| agent.PostgreSQLOptions.DatabaseCount = int32(databaseCount - excludedDatabaseCount) | ||
|
|
||
| dbCount := min(max(databaseCount-excludedDatabaseCount, 0), math.MaxInt32) | ||
| agent.PostgreSQLOptions.DatabaseCount = int32(dbCount) |
There was a problem hiding this comment.
🚫 [golangci] reported by reviewdog 🐶
G115: integer overflow conversion int -> int32 (gosec)
Comment on lines
345
to
350
| if req.PageIndex != nil { | ||
| pageIndex = int(*req.PageIndex) | ||
| if pageIndex < 0 { | ||
| return nil, errors.New("page index must be non-negative") | ||
| } | ||
| } |
Comment on lines
351
to
356
| if req.PageSize != nil { | ||
| pageSize = int(*req.PageSize) | ||
| if pageSize < 0 { | ||
| return nil, errors.New("page size must be non-negative") | ||
| } | ||
| } |
Comment on lines
+127
to
+133
| var pageIndex, pageSize int | ||
| if req.PageIndex != nil { | ||
| pageIndex = int(*req.PageIndex) | ||
| if pageIndex < 0 { | ||
| return nil, errors.New("page index must be non-negative") | ||
| } | ||
| } |
Comment on lines
+134
to
+139
| if req.PageSize != nil { | ||
| pageSize = int(*req.PageSize) | ||
| if pageSize < 0 { | ||
| return nil, errors.New("page size must be non-negative") | ||
| } | ||
| } |
Comment on lines
+168
to
+172
| code, err := strconv.ParseUint(strings.TrimPrefix(req.Dummy, "grpccode-"), 10, 32) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, status.Errorf(codes.InvalidArgument, "invalid gRPC code: %v", err) | ||
| } | ||
| grpcCode := codes.Code(code) | ||
| grpcCode := codes.Code(uint32(code)) |
Comment on lines
+329
to
+331
| if periodStartToSec <= periodStartFromSec { | ||
| return nil, errReporterPeriodStartToLessStartFrom | ||
| } |
| n = uint32(len(s.MetadataList)) | ||
| } | ||
| s.MetadataList = s.MetadataList[n:] | ||
| idx := min(int(n), len(s.MetadataList)) |
Comment on lines
+374
to
+382
| t.Parallel() | ||
| // periodTo <= periodFrom should return (nil, error) early | ||
| res, err := r.SelectSparklines(context.Background(), "val", 1000, 1000, nil, nil, "group", "col", false) | ||
| require.ErrorIs(t, err, errReporterPeriodStartToLessStartFrom) | ||
| require.Nil(t, res) | ||
|
|
||
| res, err = r.SelectSparklines(context.Background(), "val", 2000, 1000, nil, nil, "group", "col", false) | ||
| require.ErrorIs(t, err, errReporterPeriodStartToLessStartFrom) | ||
| require.Nil(t, res) |
Comment on lines
+226
to
+227
| a1, _ := models.CreateArtifact(q, models.CreateArtifactParams{Name: "a1", Vendor: "V", LocationID: locationID1, ServiceID: serviceID1, DataModel: models.PhysicalDataModel, Status: models.PendingBackupStatus, Mode: models.Snapshot}) | ||
| a2, _ := models.CreateArtifact(q, models.CreateArtifactParams{Name: "a2", Vendor: "V", LocationID: locationID1, ServiceID: serviceID1, DataModel: models.PhysicalDataModel, Status: models.PendingBackupStatus, Mode: models.Snapshot}) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket number: PMM-15129
This pull request introduces a series of safety improvements and bug fixes across the codebase, primarily focused on preventing integer overflows, improving error handling, and enhancing security and server robustness. The main themes are overflow protection for integer conversions, improved error handling/logging, and explicit documentation for intentional security lint suppressions. Additionally, test coverage is increased for edge cases.
Overflow protection and input validation:
Added checks across multiple services (alerting, backup, roles, agents, artifact helpers) to ensure values assigned to
int32/uint32fields do not overflow or underflow, with warnings or early returns when out-of-range values are detected. This includes handling for role IDs, template severities, database counts, log chunk IDs, and artifact metadata. [1] [2] [3] [4] [5] [6] [7]Updated parsing logic for PITR (Point-In-Time Recovery) timestamps and artifact metadata to handle overflows/underflows safely, returning
nilor skipping invalid records. [1] [2] [3]Server robustness and security:
Set
ReadHeaderTimeoutfor HTTP servers to mitigate Slowloris attacks, preventing malicious clients from holding connections open indefinitely. [1] [2] [3]Improved error handling for resource closure and HTTP response writing, logging errors instead of ignoring them. [1] [2]
Security linting and documentation:
//nolint:goseccomments with explanations to intentional uses ofjson.Marshalon structs with sensitive fields, clarifying that these cases are safe in context. [1] [2] [3] [4] [5]Testing and code quality:
Enhanced test coverage for overflow/underflow scenarios and pagination logic, especially for alerting templates and PITR timestamp parsing. [1] [2]
Minor improvements such as updating test template names and cleaning up test database usage. [1] [2]
These changes collectively improve the reliability, security, and maintainability of the codebase.