[PPSC-931] chore(ci): upgrade golangci-lint to v2.12.2 and fix goconst exclusions#218
Merged
Merged
Conversation
- Bump golangci-lint-action version in reusable-lint.yml - Add goconst.ignore-tests: true to suppress test fixture string noise - Add named constants throughout production code to satisfy stricter goconst thresholds in v2.12.x (install JSON keys, shell names, output colors, file extensions, lockfile protocol prefixes, etc.) - Add size guard to readJSONFileAsMap (CWE-770 fix) - Add #nosec/nolint/armis:ignore comments for new gosec rules: G117 (secret field marshal), G120 (multipart form in tests), G122 (filepath.Walk TOCTOU), and CWE-22/522 scanner FPs
|
| Severity | Count |
|---|---|
| 🟠 HIGH | 1 |
| 🟡 MEDIUM | 1 |
| 🔵 LOW | 1 |
Total: 3
View all 3 findings
🟠 HIGH (1)
CWE-20 - Injection (CWE-20
Location: internal/supplychain/check/npm.go:29
Improper Input Validation): The function ParseNPMLockfile gets a file path from whoever calls it and then hands that path straight to readLockfile. There is no check to make sure the path points to a legitimate lockfile or that it stays inside a safe directory. If an attacker can influence the value of path—for example, by providing a command‑line flag or by calling the function from another program—they could cause the program to read any file the process can access. This could let the attacker see the contents of files that should not be exposed, such as configuration files or source code. The code does not try to clean or validate the path before using it, so the risky operation is reachable from user input. Adding checks that the path is inside an expected location or that it has the correct file extension would reduce the risk.
type packageLockInfo struct {
Version string `json:"version"`
Resolved string `json:"resolved"`
Link bool `json:"link"`
}
// ParseNPMLockfile parses an npm package-lock.json into package entries.
// armis:ignore cwe:22 cwe:23 cwe:73 reason:local CLI reading the user's own lockfile; path is from local detection or an explicit --lockfile flag, not untrusted input crossing a trust boundary
func ParseNPMLockfile(path string) ([]PackageEntry, error) {
// armis:ignore cwe:22 cwe:23 cwe:73 reason:local CLI reading the user's own lockfile; path is from local detection or an explicit --lockfile flag, not untrusted input crossing a trust boundary
data, err := readLockfile(path)
if err != nil {
return nil, err
}
var lockfile packageLockFile
// armis:ignore cwe:770 cwe:502 reason:data is size-bounded by readLockfile and unmarshalled into a typed struct from the user's own lockfile; no untrusted-data deserialization risk
if err := json.Unmarshal(data, &lockfile); err != nil {
return nil, fmt.Errorf("parsing lockfile: %w", err)
}
CWEs: CWE-20: Improper Input Validation
🟡 MEDIUM (1)
CWE-190 - The function that determines the terminal width calls `term.GetSize` and passes the result of `os.Stderr.Fd()` after converting it to an `int`
Location: internal/output/styles.go:449
The function that determines the terminal width calls term.GetSize and passes the result of os.Stderr.Fd() after converting it to an int. os.Stderr.Fd() returns a value of type uintptr. On some platforms the size of a uintptr can be larger than the size of an int. Converting the file descriptor directly to int without checking its size could overflow, producing an incorrect (or even negative) width value. This mismatch is exactly what CWE‑190 describes: an integer overflow caused by an unchecked cast. The code does not validate the conversion, so the risky operation is present. The problem can be triggered whenever the command‑line tool runs and tries to detect the terminal size, which a user (or a script that runs the tool) can do locally. No external network input is involved, but the issue can still affect the tool’s behavior. Adding a check that the uintptr value fits into an int before casting, or using a type that matches the platform’s word size, would remove the overflow risk.
BoxWidth = 68 // Default box width for findings/summary (fallback)
MinBoxWidth = 60 // Minimum usable width
MaxBoxWidth = 120 // Cap to prevent overly wide output
BoxPadding = 4 // Margin from terminal edge
DefaultWrapWidth = 76 // Default text wrapping width
)
// TerminalWidth detects the current terminal width with fallbacks.
// Returns BoxWidth if detection fails (non-TTY, pipe, etc.)
func TerminalWidth() int {
w, _, err := term.GetSize(int(os.Stderr.Fd())) //nolint:gosec // G115: Fd() returns uintptr which fits in int on all supported platforms
if err != nil || w <= 0 {
return BoxWidth
}
// Subtract padding for visual margin
usable := w - BoxPadding
if usable < MinBoxWidth {
return MinBoxWidth
}
if usable > MaxBoxWidth {
return MaxBoxWidth
CWEs: CWE-190: Integer Overflow or Wraparound
🔵 LOW (1)
CWE-253 - In the scanning code, a pipe is used to pass the compressed repository data to the upload routine
Location: internal/scan/repo/repo.go:145
In the scanning code, a pipe is used to pass the compressed repository data to the upload routine. When the pipe writer (pw) is finished, the code calls pw.Close() but deliberately ignores any error it might return. This means that if the close operation fails (for example, because the pipe is already broken), the program will not notice the problem. As a result, the reader that expects an end‑of‑file signal might keep waiting, which could cause the upload to hang or be incomplete. The issue is that the code does not check the result of the close call, so a failure could be hidden. No user‑provided data reaches this point, and the problem does not expose secret information or allow an attacker to gain extra privileges. Adding a proper error check would make the program more reliable, but the current behavior does not create a serious security risk.
var sizeErr error
size, sizeErr = calculateFilesSize(absPath, existing)
if sizeErr != nil {
return nil, fmt.Errorf("failed to calculate files size: %w", sizeErr)
}
tarFunc = func() error {
// armis:ignore cwe:253 reason:pw.Close signals EOF to pipe reader; error not actionable in deferred cleanup
defer pw.Close() //nolint:errcheck // signals EOF to reader
return s.tarGzFiles(absPath, existing, pw)
}
} else {
// Full directory scanning mode — walk tree for both ignore patterns and directives.
ignoreMatcher, suppCfg, loadErr := LoadArmisIgnore(absPath)
if loadErr != nil {
return nil, fmt.Errorf("failed to load .armisignore: %w", loadErr)
}
suppressionConfig = suppCfg
var sizeErr error
CWEs: CWE-253: Incorrect Check of Function Return Value
Test Coverage Reporttotal: (statements) 72.1% Coverage by function |
There was a problem hiding this comment.
Pull request overview
This PR updates the repo’s linting/tooling configuration for a newer golangci-lint, adjusts goconst behavior (including excluding test/), and performs mechanical constant extraction / suppression-comment updates to satisfy the updated linter set.
Changes:
- Bump CI
golangci-lintversion and update.golangci.ymlto excludegoconstfindings undertest/. - Extract repeated literals into named constants across multiple packages (install/output/scan/supplychain).
- Update various security-suppression annotations and add/adjust file read size limits in install/editor config loading.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/testutil/mockapi.go | Extract tenant ID literal into a constant for mock API responses. |
| internal/supplychain/shell.go | Centralize shell names and pip fallback name constants. |
| internal/supplychain/shell_test.go | Update tests to use the new centralized shell constants. |
| internal/supplychain/proxy.go | Centralize npm registry time metadata keys (created/modified). |
| internal/supplychain/check/yarn.go | Reuse shared file:/link: protocol constants. |
| internal/supplychain/check/pnpm.go | Reuse shared file:/link: protocol constants. |
| internal/supplychain/check/npm.go | Reuse shared file:/link: protocol constants. |
| internal/supplychain/check/check.go | Introduce shared protocolFile/protocolLink constants for lockfile parsers. |
| internal/supplychain/check/bun.go | Reuse shared file:/link: protocol constants. |
| internal/scan/repo/repo.go | Update/augment gosec suppression commentary around os.Open in tar creation. |
| internal/scan/repo/inline.go | Extract common file extensions / keywords used in signature detection maps. |
| internal/output/styles.go | Extract repeated Tailwind hex literals into constants reused across palette entries. |
| internal/output/sarif.go | Replace "none" literal with a shared constant reference. |
| internal/output/human.go | Introduce groupByNone constant and use it for default grouping logic. |
| internal/install/native_hooks.go | Replace repeated JSON keys/values with shared constants; adjust suppression comment formatting. |
| internal/install/hooks.go | Replace repeated JSON keys/values with shared constants; tweak suppression comment metadata. |
| internal/install/editors.go | Add config read size limit constant and shared JSON key/type constants; apply size guard to JSON reads. |
| internal/install/claude.go | Replace JSON "version" key literal with shared constant. |
| internal/auth/client.go | Adjust gosec suppression rationale and add explicit nolint for credential marshaling. |
| internal/api/client_test.go | Add gosec nolint annotation for multipart parsing in tests. |
| .golangci.yml | Add goconst exclusion for test/ and set ignore-tests: true. |
| .github/workflows/reusable-lint.yml | Upgrade CI golangci-lint version pin to v2.12.2. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
25
to
28
| const ( | ||
| groupBySeverity = "severity" | ||
| groupByNone = "none" | ||
| noCWELabel = "No CWE" |
Comment on lines
454
to
458
| case model.SeverityLow, model.SeverityInfo: | ||
| return "note" | ||
| default: | ||
| return "none" | ||
| return groupByNone | ||
| } |
Comment on lines
+360
to
+364
| clean := filepath.Clean(path) | ||
| // armis:ignore cwe:22 reason:path from filepath.Join with known base dirs; filepath.Clean applied | ||
| // armis:ignore cwe:253 reason:ReadFile error handled by err == nil guard; non-critical config read | ||
| if b, err := os.ReadFile(filepath.Clean(path)); err == nil { | ||
| if info, err := os.Stat(clean); err != nil || info.Size() > maxEditorConfigSize { | ||
| return data | ||
| } |
Comment on lines
33
to
35
| if info, err := os.Stat(settingsPath); err == nil && info.Size() > maxSettingsSize { | ||
| return fmt.Errorf("settings file too large (%d bytes): %s", info.Size(), settingsPath) | ||
| } |
Comment on lines
+12
to
+15
| const ( | ||
| mcpServerName = "armis-appsec" | ||
| maxEditorConfigSize = 10 << 20 // 10 MB — matches the settings file limit in hooks.go | ||
| ) |
Comment on lines
112
to
116
| } else if info.Size() > maxSettingsSize { | ||
| return fmt.Errorf("settings file too large (%d bytes): %s", info.Size(), settingsPath) | ||
| } | ||
| // armis:ignore cwe:59 reason:settingsPath from filepath.Join(UserHomeDir, hardcoded ".claude/settings.json"); no user input | ||
| // armis:ignore cwe:59 cwe:770 reason:settingsPath from filepath.Join(UserHomeDir, hardcoded ".claude/settings.json"); size bounded by maxSettingsSize guard above | ||
| data, err := os.ReadFile(settingsPath) //nolint:gosec // G304: path constructed from UserHomeDir + hardcoded segments |
… review Resolve Copilot review comments on PR #218: - editors.go / hooks.go: size guards before os.ReadFile now reject non-regular files (devices, FIFOs). A special file can report size 0 and still stream unbounded data into ReadFile, defeating the size cap (CWE-770). Add info.Mode().IsRegular() checks. - editors_test.go: add TestRegisterSkipsOversizedConfig covering the oversized-config skip path (registration still writes a valid config). - output: introduce neutral noneValue = "none" so severityToSarifLevel references a SARIF-level name instead of borrowing groupByNone's grouping semantics. - editors.go: add armis:ignore cwe:502 at the json.Unmarshal sink (Go encoding/json into map has no gadget deserialization; local config input).
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.
Related Issue
Type of Change
Problem
The CI golangci-lint version was pinned to v2.10.1, and newer versions emit false-positive goconst findings in the
test/directory (standalone integration-test scaffolding with throwaway constants).Solution
.github/workflows/reusable-lint.yml.golangci.ymlto skip thetest/directory, which contains non-test files (e.g., mock-server.go, package main) where constant extraction adds no valuejsonKeyVersion,jsonKeyHooks,jsonKeyCommand, etc.) ininternal/install/internal/output/styles.gointernal/scan/repo/inline.gointernal/supplychain/internal/supplychain/shell.gointernal/supplychain/proxy.gointernal/install/editors.gofor config file reads to prevent unbounded resource usage (CWE-770)Testing
Reviewer Notes
All changes are mechanical constant extraction and configuration updates. No functional changes. The test/ exclusion is safe because:
test/contains integration-test scaffolding (mock-server.go, etc.), not tests matching*_test.goChecklist