lint: enable gosec, validate the domain open hands to the browser - #24
Merged
Conversation
gosec was not in the enabled linter set. Turning it on surfaced 21 findings, of which one was a real (if small) weakness and the rest are false positives that now carry a written justification instead of no comment at all. The real one: `namecom open <arg>` interpolated its argument straight into a URL and handed the result to `open`/`xdg-open`/`rundll32`. Those parse a leading `-` as a flag, so `namecom open -e` was not a malformed URL, it was an argument to somebody else's program. exec.Command does not invoke a shell, so there was never metacharacter injection here — argument confusion is the whole of it, which is why it is small. But `open` is the one command that hands user input to another binary, and `namecom open "$SOME_VAR"` in a script is a plausible way to reach it. The argument is now canonicalized and run through cmdutil.ValidDomainName (the same validator the other domain commands use) before interpolation, and the domain is QueryEscaped. Escaping alone would not have been enough: QueryEscape leaves `-` untouched. URL construction moved into openTarget so the check is reachable from a test without launching a browser; the eight table cases plus a property test were verified by mutation — removing the validator fails five of them. The remaining 20 are annotated rather than suppressed wholesale: - G115 int->int32 on --years (x4): cmdutil.ValidYears already bounds the value to 1..10 a few lines above each conversion. - G115 on the page counter: bounded by LastPage, itself an int32 from the API. - G304 file reads (x5): --file and --contacts-file exist to name a file to read, the config path is this tool's own, and the update cache path is derived from os.UserCacheDir(). - G204 on token_cmd: running a shell string is the feature. A helper like `op read ...` needs quoting and pipes, and cmdline comes only from the user's own config file, never a flag, an environment variable, or an API response. Anyone who can set it can already run commands as this user; the timeout and process group are the mitigations that do apply. - G101 on a "••••••••" mask string: not a credential. - Test-only findings (G104/G115/G304/G306) are excluded by rule in .golangci.yml rather than annotated at ~9 sites, deliberately scoped to those four IDs so gosec still runs on tests for everything else. Also disables golangci-lint's default output truncation. max-same-issues defaults to 3 per distinct message, which reported 15 of the 21 findings and silently dropped six real call sites because they shared wording with others. A linter that quietly withholds findings is worse than a noisy one, and the omission is invisible unless you already suspect it.
patramsey
force-pushed
the
ci/enable-gosec
branch
from
August 2, 2026 18:39
cec6bed to
435a863
Compare
4 tasks
2 tasks
patramsey
added a commit
that referenced
this pull request
Aug 2, 2026
#24 merged without a changelog entry because CHANGELOG.md itself only arrived in #23, which landed at the same time. This backfills it. Also corrects "No user-facing behavior changes yet", which #24 made false: `namecom open` now rejects an argument that is not a plausible domain name rather than passing it through. Someone who was relying on `open` to accept an arbitrary string will notice, so it belongs under Changed as well as Security. The race fix in #25 is deliberately not listed. It is test-only bookkeeping with no user-visible effect, and Keep a Changelog is for notable changes rather than a commit log.
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.
gosecwasn't in the enabled linter set. Turning it on surfaced 21 findings — one real (if small) weakness, and 20 false positives that now carry a written justification instead of no comment at all.The real one
namecom open <arg>interpolated its argument straight into a URL and handed the result toopen/xdg-open/rundll32:Those parse a leading
-as a flag. Sonamecom open -ewasn't a malformed URL — it was an argument to somebody else's program.exec.Commanddoesn't invoke a shell, so there was never metacharacter injection here; argument confusion is the whole of it, which is why this is small. Butopenis the one command that hands user input to another binary, andnamecom open "$SOME_VAR"in a script is a plausible way to reach it.Fix: the argument is canonicalized and run through
cmdutil.ValidDomainName— the same validator the other domain commands already use — before interpolation, thenurl.QueryEscaped. Escaping alone wouldn't have closed it:QueryEscapeleaves-untouched.URL construction moved into
openTargetso the check is reachable from a test without launching a browser.The other 20, annotated not suppressed
int→int32cmdutil.ValidYearsbounds--yearsto 1..10 a few lines above each conversionLastPage, itself anint32from the API--file/--contacts-fileexist to name a file to read; the config path is this tool's own; the update cache path comes fromos.UserCacheDir()token_cmd"••••••••"mask string, not a credentialOn
token_cmdspecifically, since "we run a shell string from a config file" deserves more than a//nolint: a helper likeop read ...orpass show ...needs pipes and quoting, andcmdlinecomes only from the user's own config file — never a flag, an environment variable, or an API response. Anyone who can set it can already run commands as this user. The timeout and process group already there are the mitigations that actually apply.Test-only findings are excluded by rule in
.golangci.ymlrather than annotated at ~9 sites, scoped to those four IDs so gosec still runs on tests for everything else.Also: golangci-lint was truncating its own output
max-same-issuesdefaults to 3 per distinct message. Under the defaults this run reported 15 findings; uncapped it reports 21. The six missing ones were real call sites, dropped only because they shared wording with others — and the omission is invisible unless you already suspect it. Both caps are now disabled.Test plan
golangci-lint run— no issues, with gosec enabled and truncation offgo test -race -count=1 ./...— all packages passgo build ./... && go vet ./...cleanValidDomainNamecall fromopenTargetfails 5 of the 8 table cases (-foo,--version acme.io,"",../../etc/passwd,acme.io evil.com). The tests fail for the reason claimed, not incidentally.A
CHANGELOG.mdentry belongs with this, but that file arrives in #23 — I'll add the entry once that merges, or here on rebase if #23 lands first.