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..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); @@ -71,6 +79,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())); } /**