Skip to content

P1: Proxy checksum verification robustness (deterministic source) #38

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Kinin-Code-Offical merged 1 commit into from
Dec 22, 2025
Merged

P1: Proxy checksum verification robustness (deterministic source) #38

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 21 additions & 29 deletions src/core/updater.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import crypto from 'crypto';
import path from 'path';
import { PATHS } from '../system/paths.js';
import { logger } from './logger.js';
import { escapeRegExp } from './utils.js';

const GITHUB_REPO = 'GoogleCloudPlatform/cloud-sql-proxy';
const ASSET_NAME = 'cloud-sql-proxy.x64.exe';
Expand Down Expand Up @@ -39,23 +38,20 @@ export async function downloadProxy(version: string, targetPath: string = PATHS.

try {
const releaseUrl = `https://api.github.com/repos/${GITHUB_REPO}/releases/tags/${version}`;
const response = await axios.get(releaseUrl);
await axios.get(releaseUrl);
Copy link

@sourcery-ai sourcery-ai bot Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (performance): Re-evaluate the need for the GitHub release fetch now that the checksum is sourced from GCS only.

This call no longer uses the response and now just adds an extra network round trip per download. If it’s only meant to validate that the GitHub tag exists, consider making that explicit (e.g., distinct handling/logging for "unknown version" vs generic network errors). If that validation isn’t needed anymore, removing the call would simplify the flow and reduce latency.

Suggested implementation:

    try {
        // Google Cloud SQL Proxy v2 binaries are hosted on GCS
        downloadUrl = `https://storage.googleapis.com/cloud-sql-connectors/cloud-sql-proxy/${version}/${ASSET_NAME}`;
  1. Remove the now-unused axios import at the top of src/core/updater.ts (e.g., import axios from 'axios';) if present.
  2. If there was any error handling that specifically depended on the GitHub call (e.g., distinguishing 404 on the GitHub tag), that logic can now be removed or updated to rely solely on GCS-related errors.

Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The axios.get call at this line fetches the release information but the response is no longer used. If the purpose is to validate that the release exists on GitHub before attempting to download from GCS, consider adding a comment to clarify this intent. Otherwise, if validation is not necessary, this call could be removed to avoid an unnecessary API request.

Copilot uses AI. Check for mistakes.

// Google Cloud SQL Proxy v2 binaries are hosted on GCS
downloadUrl = `https://storage.googleapis.com/cloud-sql-connectors/cloud-sql-proxy/${version}/${ASSET_NAME}`;

// Extract checksum from release body
const { body } = response.data;
// Regex to match: | [cloud-sql-proxy.x64.exe](...) | <hash> |
const escapedAssetName = escapeRegExp(ASSET_NAME);
const checksumRegex = new RegExp(`\\| \\[${escapedAssetName}\\]\\(.*?\\) \\| ([a-f0-9]{64}) \\|`);
const match = body.match(checksumRegex);

if (match && match[1]) {
expectedChecksum = match[1];
} else {
logger.warn(`Could not extract checksum for ${ASSET_NAME} from release notes.`);
// Fetch checksum from deterministic GCS sidecar file
const checksumUrl = `${downloadUrl}.sha256`;
const checksumResponse = await axios.get(checksumUrl, { responseType: 'text' });
const checksumText = String(checksumResponse.data).trim();
const checksumMatch = checksumText.match(/[a-f0-9]{64}/i);
if (!checksumMatch) {
throw new Error(`Checksum file did not contain a valid SHA256 hash (${checksumUrl})`);
}
expectedChecksum = checksumMatch[0];
Comment on lines +49 to +54
Copy link

@sourcery-ai sourcery-ai bot Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (bug_risk): Tighten checksum regex to reduce risk of picking up an unintended hash when the file has additional content.

/[a-f0-9]{64}/i will match the first 64‑char hex run anywhere in the file. If the sidecar ever includes multiple hashes or extra content (e.g. multiple files, comments, or a header with a hash‑like token), we could verify against the wrong value. Prefer anchoring to the start of a line and capturing only the first token, e.g. /^([a-f0-9]{64})\b/m, which also supports <hash> filename formats.

Suggested change
const checksumText = String(checksumResponse.data).trim();
const checksumMatch = checksumText.match(/[a-f0-9]{64}/i);
if (!checksumMatch) {
throw new Error(`Checksum file did not contain a valid SHA256 hash (${checksumUrl})`);
}
expectedChecksum = checksumMatch[0];
const checksumText = String(checksumResponse.data).trim();
// Extract a single SHA256 hash from the start of a line (supports "<hash> filename" formats)
const checksumMatch = checksumText.match(/^([a-f0-9]{64})\b/im);
if (!checksumMatch) {
throw new Error(`Checksum file did not contain a valid SHA256 hash (${checksumUrl})`);
}
expectedChecksum = checksumMatch[1];

Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex pattern uses the case-insensitive flag (/i), but checksumMatch[0] preserves the original case from the file. SHA256 hashes are conventionally represented in lowercase hexadecimal. Consider using .toLowerCase() on the extracted checksum to normalize it, or remove the /i flag and match only lowercase hashes. The verifyChecksum function at line 29 uses hash.digest('hex') which returns lowercase, so case consistency is important for the comparison.

Suggested change
expectedChecksum = checksumMatch[0];
expectedChecksum = checksumMatch[0].toLowerCase();

Copilot uses AI. Check for mistakes.
Comment on lines +46 to +54
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new checksum fetching logic from the GCS sidecar file lacks test coverage. The codebase has tests for similar functions (e.g., selfUpdate.test.ts tests checkForUpdates and fetchSha256Sums). Consider adding tests to verify the behavior when the checksum file is missing, contains invalid format, or has uppercase/mixed-case hashes.

Copilot uses AI. Check for mistakes.

logger.info(`Downloading ${ASSET_NAME} from ${downloadUrl}...`);

Expand All @@ -78,23 +74,19 @@ export async function downloadProxy(version: string, targetPath: string = PATHS.

logger.info('Download complete.');

if (expectedChecksum) {
logger.info('Verifying checksum...');
try {
const isValid = await verifyChecksum(targetPath, expectedChecksum);

if (!isValid) {
throw new Error('Checksum verification failed');
}
logger.info('Checksum verified.');
} catch (err) {
logger.warn('Failed to verify checksum', err);
// If verification fails, we should probably remove the file
await fs.remove(targetPath);
throw err;
logger.info('Verifying checksum...');
try {
const isValid = await verifyChecksum(targetPath, expectedChecksum);

if (!isValid) {
throw new Error('Checksum verification failed');
}
} else {
logger.warn('Skipping checksum verification (checksum not found).');
logger.info('Checksum verified.');
} catch (err) {
logger.warn('Failed to verify checksum', err);
// If verification fails, we should probably remove the file
await fs.remove(targetPath);
throw err;
}

} catch (error) {
Expand Down