From 837095e4cd8899e80e774970619db40a23937f31 Mon Sep 17 00:00:00 2001 From: Alejandro Rizzo Date: Mon, 9 Nov 2026 16:34:59 +0000 Subject: [PATCH 1/2] fix: pin AES-GCM auth tag length and apply Codacy lint fixes decryptToken accepted a truncated GCM tag (down to 4 bytes) because createDecipheriv was called without authTagLength, so a prefix of a valid tag still verified. Require exactly 16 bytes, which is what encryptToken has always written, so existing credential files keep working. Also apply the safe subset of ESLint autofixes surfaced while tuning the repo's Codacy configuration (unused imports, nullish defaults, typed catch params), add .codacy.yaml so Cloud ESLint stops failing to parse .changeset/changelog.test.ts and skips test files, and drop the legacy .codacy/codacy.yaml from the old Codacy CLI. Co-Authored-By: Claude Opus 5.5 --- .changeset/gcm-auth-tag-length.md | 5 +++++ .codacy.yaml | 17 +++++++++++++++++ .codacy/codacy.yaml | 8 -------- AGENTS.md | 2 +- src/commands/finding.ts | 1 - src/commands/findings.ts | 2 +- src/commands/issue.ts | 2 +- src/commands/login.ts | 2 +- src/commands/pull-request.ts | 13 ++++++------- src/commands/repository.ts | 3 +-- src/utils/auth.ts | 2 +- src/utils/credentials.test.ts | 9 +++++++++ src/utils/credentials.ts | 20 ++++++++++++++++---- src/utils/diff.ts | 2 +- src/utils/formatting.ts | 12 +++++------- src/utils/git-remote.ts | 2 +- src/utils/import-config.ts | 8 ++++---- src/utils/prompt.ts | 2 +- src/utils/reanalyze-wait.ts | 4 ++-- src/utils/repo-tree.ts | 2 +- 20 files changed, 74 insertions(+), 44 deletions(-) create mode 100644 .changeset/gcm-auth-tag-length.md create mode 100644 .codacy.yaml delete mode 100644 .codacy/codacy.yaml diff --git a/.changeset/gcm-auth-tag-length.md b/.changeset/gcm-auth-tag-length.md new file mode 100644 index 0000000..53186fc --- /dev/null +++ b/.changeset/gcm-auth-tag-length.md @@ -0,0 +1,5 @@ +--- +"@codacy/codacy-cloud-cli": patch +--- + +Stored credentials now require the full 16-byte AES-GCM authentication tag. Previously a truncated tag (as short as 4 bytes) still verified, which weakened the integrity check on `~/.codacy/credentials`; existing credential files keep working because the CLI has always written full-length tags. Also tidies a handful of lint findings (unused imports, `??` over `||` for nullish defaults, typed `catch` parameters) and adds a `.codacy.yaml` so Codacy skips test files for ESLint and generated Markdown for markdownlint. diff --git a/.codacy.yaml b/.codacy.yaml new file mode 100644 index 0000000..06ea0e7 --- /dev/null +++ b/.codacy.yaml @@ -0,0 +1,17 @@ +--- +exclude_paths: + # Already ignored in the Codacy UI; repeated here so the file and the UI agree + - "src/commands/issues.ts" +engines: + eslint-9: + exclude_paths: + # Mock-heavy tests: unbound-method on expect(svc.method), `as any`, no-unsafe-* + - "**/*.test.ts" + - "src/test-support.ts" + # changesets helper (.cjs) — ESLint has no Node globals configured for it + - ".changeset/**" + markdownlint: + exclude_paths: + - "CHANGELOG.md" + - "CODE_OF_CONDUCT.md" + - "SPECS/commands/tools-and-patterns.md" diff --git a/.codacy/codacy.yaml b/.codacy/codacy.yaml deleted file mode 100644 index 6a8dadd..0000000 --- a/.codacy/codacy.yaml +++ /dev/null @@ -1,8 +0,0 @@ -runtimes: - - node@22.2.0 - - python@3.11.11 -tools: - - eslint@8.57.0 - - lizard@1.17.19 - - semgrep@1.78.0 - - trivy@0.59.1 diff --git a/AGENTS.md b/AGENTS.md index 4d8dfd3..293303f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -249,6 +249,6 @@ When completing work, agents **must** update relevant documentation: ## Useful Context -- Codacy API docs: https://api.codacy.com/api/api-docs +- Codacy API docs: - The CLI targets Codacy Cloud (app.codacy.com), not self-hosted instances - Provider shortcodes used in commands: `gh` (GitHub), `gl` (GitLab), `bb` (Bitbucket) diff --git a/src/commands/finding.ts b/src/commands/finding.ts index 058c7e0..127ade6 100644 --- a/src/commands/finding.ts +++ b/src/commands/finding.ts @@ -17,7 +17,6 @@ import { } from "../utils/formatting"; import { sanitizeText } from "../utils/sanitize"; import { SecurityService } from "../api/client/services/SecurityService"; -import { IgnoreSRMItemBody } from "../api/client/models/IgnoreSRMItemBody"; import { AnalysisService } from "../api/client/services/AnalysisService"; import { ToolsService } from "../api/client/services/ToolsService"; import { FileService } from "../api/client/services/FileService"; diff --git a/src/commands/findings.ts b/src/commands/findings.ts index 1a1d4be..b1b6192 100644 --- a/src/commands/findings.ts +++ b/src/commands/findings.ts @@ -236,7 +236,7 @@ export function parseImageFilter( const refTag = name.slice(colon + 1); name = name.slice(0, colon); if (!name || !refTag) { - throw new Error(`--image expects name or name:tag (got "${image!.trim()}").`); + throw new Error(`--image expects name or name:tag (got "${image?.trim()}").`); } if (tag && tag !== refTag) { throw new Error( diff --git a/src/commands/issue.ts b/src/commands/issue.ts index 1f2dc11..c8549b3 100644 --- a/src/commands/issue.ts +++ b/src/commands/issue.ts @@ -97,7 +97,7 @@ Examples: encodeURIComponent(issue.filePath), startLine, endLine, - ).catch((e) => { + ).catch((e: unknown) => { console.error(ansis.red(`File path: ${sanitizeText(issue.filePath)}`)); console.error(ansis.red(`Error fetching file content: ${e}`)); return null; diff --git a/src/commands/login.ts b/src/commands/login.ts index 912aa09..b176396 100644 --- a/src/commands/login.ts +++ b/src/commands/login.ts @@ -17,7 +17,7 @@ import { /** Reads the account token from `--token`, or prompts for it interactively. */ const acquireToken = async (tokenOption?: string): Promise => { if (tokenOption) { - const token = String(tokenOption).trim(); + const token = tokenOption.trim(); if (!token) throw new Error("Token cannot be empty."); return token; } diff --git a/src/commands/pull-request.ts b/src/commands/pull-request.ts index ce77589..c13937e 100644 --- a/src/commands/pull-request.ts +++ b/src/commands/pull-request.ts @@ -18,7 +18,6 @@ import { truncate, buildGateStatus, formatStandards, - colorByGate, colorSeverity, formatDelta, formatPrCoverage, @@ -207,8 +206,8 @@ function printAbout( function buildGateHints(pr: PullRequestWithAnalysis): Record { const hints: Record = {}; const reasons: AnalysisResultReason[] = [ - ...(pr.quality?.resultReasons || []), - ...(pr.coverage?.resultReasons || []), + ...(pr.quality?.resultReasons ?? []), + ...(pr.coverage?.resultReasons ?? []), ]; for (const r of reasons) { const key = gateKeyFromReason(r.gate); @@ -325,7 +324,7 @@ function printFilesList(files: FileDeltaAnalysis[]): void { q.deltaFixedIssues > 0 || q.deltaComplexity || q.deltaClonesCount)) || - (c && c.deltaCoverage) + (c?.deltaCoverage) ); }); @@ -573,7 +572,7 @@ function printAnnotatedDiff( if (!issuesByFile.has(fp)) issuesByFile.set(fp, new Map()); const m = issuesByFile.get(fp)!; if (!m.has(ln)) m.set(ln, []); - m.get(ln)!.push(tagged); + m.get(ln)?.push(tagged); } const { files } = parseDiff(diffText, true); @@ -1267,7 +1266,7 @@ Examples: pullRequest: prData, newIssues: newIssues.data, potentialIssues: potentialIssues.data, - files: filesData.data || [], + files: filesData.data ?? [], }, [ // About "pullRequest.pullRequest.repository", @@ -1325,7 +1324,7 @@ Examples: printDiffCoverageSummary(fileCoverageList); // Files - printFilesList(filesData.data || []); + printFilesList(filesData.data ?? []); printPaginationWarning( filesData.pagination, "Not all files are shown.", diff --git a/src/commands/repository.ts b/src/commands/repository.ts index 000e93d..eeb054a 100644 --- a/src/commands/repository.ts +++ b/src/commands/repository.ts @@ -25,7 +25,6 @@ import { truncate, buildGateStatus, formatStandards, - colorByGate, formatDelta, formatPrCoverage, formatRepoCoverageDetail, @@ -540,7 +539,7 @@ Examples: organization, repository, ), - fetchIfAccountToken(auth, noPullRequests(), () => + fetchIfAccountToken(auth, noPullRequests(), async () => AnalysisService.listRepositoryPullRequests( provider, organization, diff --git a/src/utils/auth.ts b/src/utils/auth.ts index 1182bb6..fc551c0 100644 --- a/src/utils/auth.ts +++ b/src/utils/auth.ts @@ -236,7 +236,7 @@ export function resolveAccountAuth(command: Command, because: string): AccountAu * hang off a non-whitelisted endpoint — see the pull request table in * `commands/repository.ts`. */ -export function fetchIfAccountToken( +export async function fetchIfAccountToken( auth: RemoteAuth, fallback: F, fetch: () => Promise, diff --git a/src/utils/credentials.test.ts b/src/utils/credentials.test.ts index b14515e..3941f47 100644 --- a/src/utils/credentials.test.ts +++ b/src/utils/credentials.test.ts @@ -71,6 +71,15 @@ describe("credentials", () => { expect(() => decryptToken(JSON.stringify(parsed))).toThrow(); }); + it("should reject a truncated auth tag", () => { + // A prefix of a valid GCM tag still verifies unless the tag length is + // pinned, so this would decrypt successfully without authTagLength. + const encrypted = encryptToken("my-secret"); + const parsed = JSON.parse(encrypted); + parsed.authTag = parsed.authTag.slice(0, 8); + expect(() => decryptToken(JSON.stringify(parsed))).toThrow(); + }); + it("should throw on invalid JSON", () => { expect(() => decryptToken("not-json")).toThrow(); }); diff --git a/src/utils/credentials.ts b/src/utils/credentials.ts index 14aae0b..7a3e881 100644 --- a/src/utils/credentials.ts +++ b/src/utils/credentials.ts @@ -10,6 +10,10 @@ const ALGORITHM = "aes-256-gcm"; const KEY_LENGTH = 32; const IV_LENGTH = 16; const SALT_LENGTH = 32; +// GCM tags can legally be truncated (down to 4 bytes), and a truncated prefix of +// a valid tag still verifies unless the expected length is pinned. We always +// write the full 16-byte tag, so require exactly that on read. +const AUTH_TAG_LENGTH = 16; const PBKDF2_ITERATIONS = 100_000; const PBKDF2_DIGEST = "sha512"; @@ -40,7 +44,9 @@ export function encryptToken(token: string): string { const iv = crypto.randomBytes(IV_LENGTH); const key = deriveKey(getMachineKey(), salt); - const cipher = crypto.createCipheriv(ALGORITHM, key, iv); + const cipher = crypto.createCipheriv(ALGORITHM, key, iv, { + authTagLength: AUTH_TAG_LENGTH, + }); const encrypted = Buffer.concat([ cipher.update(token, "utf8"), cipher.final(), @@ -66,7 +72,13 @@ export function decryptToken(payloadJson: string): string { const encrypted = Buffer.from(payload.encrypted, "hex"); const key = deriveKey(getMachineKey(), salt); - const decipher = crypto.createDecipheriv(ALGORITHM, key, iv); + if (authTag.length !== AUTH_TAG_LENGTH) { + throw new Error("Invalid authentication tag length"); + } + + const decipher = crypto.createDecipheriv(ALGORITHM, key, iv, { + authTagLength: AUTH_TAG_LENGTH, + }); decipher.setAuthTag(authTag); const decrypted = Buffer.concat([ @@ -123,7 +135,7 @@ export function deleteCredentials(): boolean { } } -export function promptForToken(prompt: string): Promise { +export async function promptForToken(prompt: string): Promise { return new Promise((resolve, reject) => { if (!process.stdin.isTTY) { reject(new Error("Interactive input required. Use --token instead.")); @@ -138,7 +150,7 @@ export function promptForToken(prompt: string): Promise { let token = ""; const onData = (char: string) => { - const chunk = char.toString(); + const chunk = char; for (const c of chunk) { if (c === "\n" || c === "\r") { diff --git a/src/utils/diff.ts b/src/utils/diff.ts index fe6c087..d8104f6 100644 --- a/src/utils/diff.ts +++ b/src/utils/diff.ts @@ -1,4 +1,4 @@ -import gitDiffParser, { File } from "gitdiff-parser"; +import gitDiffParser from "gitdiff-parser"; import { sortBy } from "lodash"; // TODO: remove this when the API has the correct diff for commits coming from Git diff --git a/src/utils/formatting.ts b/src/utils/formatting.ts index f8533a9..477412a 100644 --- a/src/utils/formatting.ts +++ b/src/utils/formatting.ts @@ -550,7 +550,7 @@ export function formatRepoCoverageCell( function hasCoverageValue(coverage: Coverage | undefined): boolean { return ( coverage?.coveragePercentage !== undefined && - coverage?.coveragePercentage !== null + coverage.coveragePercentage !== null ); } @@ -703,8 +703,8 @@ export function formatDelta( export function buildGateStatus(pr: PullRequestWithAnalysis): GateStatusMap { const status: GateStatusMap = {}; const reasons: AnalysisResultReason[] = [ - ...(pr.quality?.resultReasons || []), - ...(pr.coverage?.resultReasons || []), + ...(pr.quality?.resultReasons ?? []), + ...(pr.coverage?.resultReasons ?? []), ]; for (const r of reasons) { const gate = r.gate.toLowerCase(); @@ -1249,9 +1249,7 @@ export async function resolveToolUuids( continue; } - if (!allTools) { - allTools = await fetchTools(); - } + allTools ??= await fetchTools(); const lower = input.toLowerCase(); @@ -1274,7 +1272,7 @@ export async function resolveToolUuids( return ( t.name.toLowerCase().includes(lower) || t.shortName.toLowerCase().includes(lower) || - (t.prefix && t.prefix.toLowerCase().includes(lower)) + (t.prefix?.toLowerCase().includes(lower)) ); }); diff --git a/src/utils/git-remote.ts b/src/utils/git-remote.ts index 0db802a..35b81c6 100644 --- a/src/utils/git-remote.ts +++ b/src/utils/git-remote.ts @@ -21,7 +21,7 @@ function stripUserinfo(host: string): string { } export function parseGitRemoteUrl(url: string): RepoContext | null { - const match = url.match(SSH_REGEX) || url.match(HTTPS_REGEX); + const match = (SSH_REGEX.exec(url)) ?? (HTTPS_REGEX.exec(url)); if (!match) return null; const [, rawHost, org, repo] = match; diff --git a/src/utils/import-config.ts b/src/utils/import-config.ts index 02daa24..5ba7655 100644 --- a/src/utils/import-config.ts +++ b/src/utils/import-config.ts @@ -98,7 +98,7 @@ export function resolveToolId( // Match by prefix (strip trailing _ before comparing) const byPrefix = allTools.find( - (t) => t.prefix && t.prefix.replace(/_$/, "").toLowerCase() === id, + (t) => t.prefix?.replace(/_$/, "").toLowerCase() === id, ); if (byPrefix) return byPrefix; @@ -125,7 +125,7 @@ export async function getLocalSupportedToolIds(): Promise { const info = JSON.parse(stdout); if (!info.tools || !Array.isArray(info.tools)) return null; return info.tools - .filter((t: any) => t && t.supported && typeof t.id === "string") + .filter((t: any) => t?.supported && typeof t.id === "string") .map((t: any) => t.id as string); } catch { return null; @@ -193,12 +193,12 @@ export function buildImportPreview( // Tools in the config that need enabling (currently disabled or not present) const toolsToEnable = resolved.filter( - (r) => !r.repoTool || !r.repoTool.settings.isEnabled, + (r) => !r.repoTool?.settings.isEnabled, ); // Tools in the config that are already enabled (need reconfiguration) const toolsToReconfigure = resolved.filter( - (r) => r.repoTool && r.repoTool.settings.isEnabled, + (r) => r.repoTool?.settings.isEnabled, ); const { toolsToDisable, cloudOnlyTools } = splitEnabledNotInConfig( diff --git a/src/utils/prompt.ts b/src/utils/prompt.ts index 9623f69..ddf4e5a 100644 --- a/src/utils/prompt.ts +++ b/src/utils/prompt.ts @@ -17,7 +17,7 @@ import * as readline from "readline"; * to answer must abort rather than delete by accident. `-y` is how a caller * says yes ahead of time. */ -export function confirmAction(message: string): Promise { +export async function confirmAction(message: string): Promise { return new Promise((resolve) => { if (!process.stdin.isTTY) { resolve(false); diff --git a/src/utils/reanalyze-wait.ts b/src/utils/reanalyze-wait.ts index 3ad4ae5..cd7a3a1 100644 --- a/src/utils/reanalyze-wait.ts +++ b/src/utils/reanalyze-wait.ts @@ -33,8 +33,8 @@ export const PATTERN_LIMIT = 20; * resolver (`vi.spyOn(timers, "sleep").mockResolvedValue()`). */ export const timers = { - sleep(ms: number): Promise { - return new Promise((resolve) => setTimeout(resolve, ms)); + async sleep(ms: number): Promise { + return new Promise((resolve) => void setTimeout(resolve, ms)); }, }; diff --git a/src/utils/repo-tree.ts b/src/utils/repo-tree.ts index d0011ca..b162355 100644 --- a/src/utils/repo-tree.ts +++ b/src/utils/repo-tree.ts @@ -74,7 +74,7 @@ export async function mapWithConcurrency( } }; const size = Math.min(limit, items.length); - await Promise.all(Array.from({ length: size }, () => worker())); + await Promise.all(Array.from({ length: size }, async () => worker())); } /** From 503f2d34ac9c09eaa8bea12bcee270cc7306ec7d Mon Sep 17 00:00:00 2001 From: Alejandro Rizzo Date: Mon, 9 Nov 2026 16:37:29 +0000 Subject: [PATCH 2/2] test(credentials): assert encryptToken writes a full 16-byte auth tag Addresses the Codacy AI review test suggestion on #67. Co-Authored-By: Claude Opus 5.5 --- src/utils/credentials.test.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/utils/credentials.test.ts b/src/utils/credentials.test.ts index 3941f47..759baf0 100644 --- a/src/utils/credentials.test.ts +++ b/src/utils/credentials.test.ts @@ -54,6 +54,14 @@ describe("credentials", () => { expect(parsed).toHaveProperty("encrypted"); }); + it("should write a full 16-byte auth tag that decryption accepts", () => { + const encrypted = encryptToken("test-token"); + const parsed = JSON.parse(encrypted); + // decryptToken rejects anything shorter, so the writer must emit 16 bytes + expect(Buffer.from(parsed.authTag, "hex")).toHaveLength(16); + expect(decryptToken(encrypted)).toBe("test-token"); + }); + it("should produce different ciphertexts for the same token (random salt/iv)", () => { const token = "same-token"; const a = encryptToken(token);