Skip to content

security: fix audit findings — C1, H1–H4, M1–M5#998

Open
lhotea wants to merge 6 commits into
ankitpokhrel:mainfrom
lhotea:security/fix-high-medium-audit-findings
Open

security: fix audit findings — C1, H1–H4, M1–M5#998
lhotea wants to merge 6 commits into
ankitpokhrel:mainfrom
lhotea:security/fix-high-medium-audit-findings

Conversation

@lhotea

@lhotea lhotea commented May 26, 2026

Copy link
Copy Markdown

Summary

Fixes 10 findings from an internal v1.7.0 security audit: 1 Critical, 4 High, 5 Medium. No functional regressions; all existing tests pass on this branch identically to main on the development host (two pre-existing Windows-only failures in TestGetIssueRaw / TestGetPager are unchanged — not caused by this branch).

Findings addressed

ID Sev Fix
C1 Critical Scrub Authorization, Cookie, Proxy-Authorization from --debug HTTP dumps via req.Clone + REDACTED. Original request retains real headers so Do() still works.
H1 High Write config atomically (tempfile + os.Rename) with mode 0600.
H2 High Stop leaving .bkp residue containing prior credentials on jira init rerun.
H3 High Refuse mTLS client key with group/world-readable perms (POSIX only — gated on runtime.GOOS != "windows" because Unix mode bits don't reflect Windows ACLs).
H4 High Bump golang.org/x/net v0.38.0 → v0.55.0 to clear govulncheck findings (CVE-2025-22872, CVE-2025-22870, CVE-2024-45338, GO-2025-3503/3595/3942, GO-2026-4441/4918, CVE-2025-47911).
M1 Medium Sanitise terminal control sequences (OSC / DCS / APC / PM / non-SGR CSI) from server-rendered data in issue views. UTF-8-aware state machine; SGR m final byte preserved so colour output still works.
M2 Medium UX warnings for insecure: true — confirmation prompt at jira init, stderr banner on every invocation.
M3 Medium Document & enforce env-over-config token precedence in api/client.go (consult JIRA_API_TOKEN before viper-loaded api_token). README updated.
M4 Medium Drop tls.RenegotiateFreelyAsClient, default to RenegotiateNever. Redact CA/cert filesystem paths from log.Fatalf messages so they don't leak the user's home layout in stderr.
M5 Medium Bump glamour 0.9.1 → 1.0.0 (pulls goldmark 1.7.13).

Verification

  • go build ./... — clean.
  • go vet ./... — clean.
  • go test ./... — same pass/fail set as origin/main on the Windows host (TestGetIssueRaw / TestGetPager were already failing on main; reproduce by checking out main and running the same command).
  • govulncheck ./... — No vulnerabilities found. (was 7 imported + 2 module on main).
  • Manual smoke-test: built binary, confirmed Bearer canary BEARER-TEST-LEAKAGE-CANARY-XYZ does not appear in --debug output (only Authorization: REDACTED); insecure stderr banner fires when insecure: true.

Out of scope / follow-ups for maintainers

  • internal/cmd/root/root.go:checkForJiraToken only consults env / netrc / keyring — not viper-loaded YAML api_token. The M3 precedence fix in api/client.go matches the audit ask; the root.go gate quirk is documented here for a follow-up rather than altered in this PR.
  • A govulncheck + gosec CI workflow (.github/workflows/security.yml) was prepared but is omitted from this PR because the OAuth token used to push lacks the workflow scope. Adding it can be a maintainer follow-up — the dependency bump in this PR already clears the current findings.

🤖 Generated with Claude Code

Arnaud L'Hôte added 6 commits May 22, 2026 18:14
fix(security): write config atomically with 0600, no .bkp residue (H1…
3b6a00c 
…, H2)

Previously, internal/config/generator.go renamed the existing config to
<path>.bkp and recreated the file with default 0666 mode. The .bkp
file was never cleaned up and inherited the permissive mode, so token
rotation didn't actually retire the old token from disk and any local
account could read it.

Now: atomic write via tempfile + os.Rename, mode 0o600 explicitly. No
.bkp is left behind. Adds a startup permission check that warns on
non-Windows hosts when the config file is group/world-readable.

Adds tests asserting mode and absence of .bkp.
fix(security): sanitise terminal control sequences from server data (M1)
524cd58 
Issue summary, description, comment body, and user displayName fields
are returned by the Jira server as untrusted UTF-8 and were previously
rendered without filtering. A malicious account could embed OSC 8
hyperlinks (CWE-150), OSC 52 clipboard writes, or DECSET 1049 to
corrupt the user's terminal when 'jira issue view' was run.

Adds internal/view/sanitize.go with SanitizeTerminal, which strips
C0/C1 controls and any non-SGR escape sequence. All call sites that
emit server-controlled strings now route through it.

Tests cover OSC 8, OSC 52, DECSET 1049, plain SGR pass-through, and
UTF-8 preservation.
fix(security): scrub Authorization header in debug dump (C1)
2e7ab51 
httputil.DumpRequest was called with body=true after SetBasicAuth /
Authorization header was set, which leaked Bearer/Basic credentials to
stdout when --debug was used. Clone the request and replace
Authorization, Cookie, and Proxy-Authorization with REDACTED before
dumping. The original request keeps its real headers so downstream
http.Client.Do still authenticates.

prettyPrintDump now writes through an io.Writer so its output can be
captured in tests.

Adds regression tests asserting the dump contains REDACTED and not the
real Bearer token / Basic credentials, while the original request is
preserved.
chore(security): bump golang.org/x/net (H4)
1a7b13c 
Bumps golang.org/x/net from v0.38.0 to v0.55.0 to pull in fixes for known
vulnerabilities reported by govulncheck (GO-2025-3503, GO-2025-3595,
GO-2025-3942, GO-2026-4441, GO-2026-4918, CVE-2025-47911, CVE-2025-22872,
CVE-2025-22870, CVE-2024-45338).

A govulncheck/gosec CI workflow was prepared but is omitted from this PR
because the OAuth token used to push lacks the 'workflow' scope. Adding
that workflow can be a maintainer follow-up.
fix(security): mTLS hardening — key perm check, drop renegotiation, r…
f1c7857 
…edact paths (H3, M4)

- Refuse to use the mTLS client key when its file mode is group- or
  world-readable on POSIX. Skipped on Windows (mode bits don't reflect
  ACLs there).
- Remove transport.TLSClientConfig.Renegotiation =
  RenegotiateFreelyAsClient. Default RenegotiateNever is correct;
  Jira does not need renegotiation.
- Stop logging the user-supplied CA path / cert path in log.Fatalf
  output. The wrapped Go errors are still informative without the
  path concatenation.

Type-checked via go build/vet; mTLS behaviour is covered by integration
smoke testing rather than unit tests (no certs available in CI).
fix(security): insecure UX, token precedence, goldmark bump (M2, M3, M5)
79dc123 
M2: --insecure now warns loudly at config write time and requires an
explicit interactive confirmation before persisting insecure: true to
the YAML. Every subsequent command load also prints a one-line stderr
banner so the user is reminded that TLS verification is off.

M3: JIRA_API_TOKEN env var now takes precedence over the api_token key
in the YAML config. This makes credential rotation immediate instead
of requiring a config edit, and removes the operational foot-gun of a
stale on-disk token shadowing a freshly exported one. README updated
to document the new resolution order.

M5: bump glamour 0.9.1 -> 1.0.0 so the transitive goldmark dependency
moves from 1.7.8 to 1.7.13, picking up the CVE-2026-5160 fix (DoS via
crafted markdown link reference definitions).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lhotea