golangci-lint upgrade to v2.11.4, related fixes#549
Merged
Conversation
The branch updates `.golangci.yaml` and is being linted locally with `golangci-lint` 2.11.4, but CI was still pinned to 2.9.0. That leaves local and CI checks evaluating different rule sets and can hide or invent failures depending on where the lint runs. Update the workflow pin to 2.11.4 so the GitHub Action fetches the same linter version used locally. Keeping the versions aligned makes the branch's lint results reproducible across environments.
Recent `golangci-lint` releases enable the `noctx` checks that now flag plain `httptest.NewRequest` calls in our tests and shared test helpers. Those requests were still valid, but they no longer match the context- aware API the linter expects. Switch the affected tests and helper code to `httptest.NewRequestWithContext`, reusing each test's existing context where possible and falling back to `context.Background()` in the small helper that has no test context available. This keeps the request setup behavior the same while satisfying the stricter lint rule.
The packager writes `.mod`, `.zip`, and `.info` artifacts under a caller-provided output directory. Those writes were assembled with `filepath.Join` and then passed straight to file creation helpers, which is exactly the pattern `gosec` now flags for path traversal. Open the version output directory as an `os.Root` and perform the file writes through that root instead. This keeps the artifact generation behavior the same, but confines every write to the intended output subtree even if a filename is malformed or hostile.
Collaborator
|
Hrmm, yeah not sure about that health check one ... did you look at the diff? We add >200 LOCs just related to health checks, not including tests. What are the consequences if we just disable the |
`gosec` flagged the `riverui -healthcheck=...` code path because it constructs an outbound HTTP request using deployment configuration. That probe is an operator-invoked check of the running River UI server's own health endpoint, and we don't treat steering it via env as a meaningful security boundary in this context. Keep the existing behavior and add a narrow `//nolint:gosec` suppression on the request construction and execution lines with an explanation of why the warning is not actionable here.
Contributor
Author
|
@brandur yeah, agreed, gross and totally unnecessary 🤖 I removed it altogether in favor of lint ignore comments |
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
2 participants
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.
This repo hadn't had its golangci-lint version updated to the latest. It turns out a few things broke when I ran with that locally:
node_modules, so I had to exclude that.noctxchecks want you to avoid context-freehttptest.NewRequestcalls, so those were changed tohttptest.NewRequestWithContextto leverage thet.Context()as appropriate.gosecflagged caller-provided output paths in packager as a risk. I could have skipped this given its input is constrained by workflows, but it seemed easy enough to accept the fix instead.gosecflagged the code path behindriverui -healthcheck=..., which is an operator-invoked probe of the running server's HTTP health endpoint. I left the behavior alone and added a narrow//nolint:gosecsuppression on that specific request path with an explanation of why the warning isn't actionable here.