P1: Proxy checksum verification robustness (deterministic source)#38
Conversation
Reviewer's GuideSwitches proxy checksum verification to use a deterministic SHA256 sidecar file from GCS, making checksum verification mandatory and simplifying the updater logic. Sequence diagram for deterministic proxy download and checksum verificationsequenceDiagram
participant Caller
participant Updater as downloadProxy
participant GitHubAPI
participant GCSBucket
participant FileSystem as fs
participant Verifier as verifyChecksum
participant Logger
Caller->>Updater: downloadProxy(version, targetPath)
Updater->>GitHubAPI: GET /repos/.../releases/tags/{version}
GitHubAPI-->>Updater: 200 OK
Updater->>GCSBucket: GET /cloud-sql-proxy/{version}/cloud-sql-proxy.x64.exe.sha256
GCSBucket-->>Updater: checksumText
Updater->>Updater: extract SHA256 from checksumText
Updater->>Logger: info("Downloading asset from GCS...")
Updater->>GCSBucket: GET /cloud-sql-proxy/{version}/cloud-sql-proxy.x64.exe
GCSBucket-->>Updater: proxyBinary
Updater->>FileSystem: write proxyBinary to targetPath
FileSystem-->>Updater: write complete
Updater->>Logger: info("Download complete.")
Updater->>Logger: info("Verifying checksum...")
Updater->>Verifier: verifyChecksum(targetPath, expectedChecksum)
Verifier-->>Updater: isValid
alt checksum invalid
Updater->>Logger: warn("Failed to verify checksum")
Updater->>FileSystem: remove(targetPath)
FileSystem-->>Updater: removed
Updater-->>Caller: throw Error("Checksum verification failed")
else checksum valid
Updater->>Logger: info("Checksum verified.")
Updater-->>Caller: success
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
axios.get(releaseUrl)response is now unused; if the only purpose is to fail fast on an invalid tag, consider either removing the call entirely and relying on the GCS request to surface errors, or adding a short comment to clarify that the call is intentionally kept just for tag validation. - When parsing the
.sha256file, using a generic/[a-f0-9]{64}/iregex may accidentally match unrelated hashes if the format ever changes; you might want to split on whitespace and take the first token (or anchor the regex to the start of the line) to make the checksum extraction more robust to file contents.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `axios.get(releaseUrl)` response is now unused; if the only purpose is to fail fast on an invalid tag, consider either removing the call entirely and relying on the GCS request to surface errors, or adding a short comment to clarify that the call is intentionally kept just for tag validation.
- When parsing the `.sha256` file, using a generic `/[a-f0-9]{64}/i` regex may accidentally match unrelated hashes if the format ever changes; you might want to split on whitespace and take the first token (or anchor the regex to the start of the line) to make the checksum extraction more robust to file contents.
## Individual Comments
### Comment 1
<location> `src/core/updater.ts:49-54` </location>
<code_context>
- 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})`);
</code_context>
<issue_to_address>
**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.
```suggestion
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];
```
</issue_to_address>
### Comment 2
<location> `src/core/updater.ts:41` </location>
<code_context>
try {
const releaseUrl = `https://api.github.com/repos/${GITHUB_REPO}/releases/tags/${version}`;
- const response = await axios.get(releaseUrl);
+ await axios.get(releaseUrl);
// Google Cloud SQL Proxy v2 binaries are hosted on GCS
</code_context>
<issue_to_address>
**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:
```typescript
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.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| 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]; |
There was a problem hiding this comment.
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.
| 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]; |
| try { | ||
| const releaseUrl = `https://api.github.com/repos/${GITHUB_REPO}/releases/tags/${version}`; | ||
| const response = await axios.get(releaseUrl); | ||
| await axios.get(releaseUrl); |
There was a problem hiding this comment.
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}`;- Remove the now-unused
axiosimport at the top ofsrc/core/updater.ts(e.g.,import axios from 'axios';) if present. - 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.
There was a problem hiding this comment.
Pull request overview
This PR enhances the robustness of proxy checksum verification by switching from parsing checksums from GitHub release notes to fetching them from deterministic GCS .sha256 sidecar files, and makes checksum verification mandatory rather than optional.
Key changes:
- Replaces regex-based checksum extraction from release body with direct fetch from GCS
.sha256sidecar file - Makes checksum verification mandatory by removing the conditional check around verification logic
- Removes unused
escapeRegExpimport
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| const releaseUrl = `https://api.github.com/repos/${GITHUB_REPO}/releases/tags/${version}`; | ||
| const response = await axios.get(releaseUrl); | ||
| await axios.get(releaseUrl); |
There was a problem hiding this comment.
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.
| if (!checksumMatch) { | ||
| throw new Error(`Checksum file did not contain a valid SHA256 hash (${checksumUrl})`); | ||
| } | ||
| expectedChecksum = checksumMatch[0]; |
There was a problem hiding this comment.
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.
| expectedChecksum = checksumMatch[0]; | |
| expectedChecksum = checksumMatch[0].toLowerCase(); |
| // 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]; |
There was a problem hiding this comment.
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.
Closes #21
Summary:
Tests:
Rollback:
Summary by Sourcery
Switch proxy binary checksum verification to use a deterministic GCS sidecar file and enforce mandatory checksum validation during downloads.
Enhancements: