fix(bootstrap): reject on non-2xx tarball response and handle zlib errors - #356
fix(bootstrap): reject on non-2xx tarball response and handle zlib errors#356cs-raj wants to merge 5 commits into
Conversation
…rors streamRelease now throws GithubError for HTTP 4xx/5xx responses instead of silently piping the error body (e.g. "404: Not Found") into the zlib decompressor. This was the root cause of the Z_DATA_ERROR crash when the cli-use branch was absent from a repo. extract now attaches an error handler directly on the zlib.createUnzip() stream. Node's pipe() does not forward stream errors, so without this listener a zlib failure emitted an unhandled error event and crashed the process rather than rejecting the Promise cleanly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
Moving cliux.loader() (spinner stop) out of finally and into catch before cliux.error() prevents the spinner's carriage-return from wiping the error line. Success path stops the spinner inline after getLatest resolves. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
Replace the generic cliux.error+rethrow pattern with a single clean Error throw so oclif prints one message. Message names both the repo and the missing cli-use branch so the developer knows exactly what to check on GitHub. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
There was a problem hiding this comment.
Pull request overview
This PR hardens the contentstack-bootstrap plugin’s GitHub tarball download + extraction path so csdx cm:bootstrap fails gracefully (rejects promises) instead of crashing on invalid gzip data returned from failed GitHub responses.
Changes:
- Add HTTP status validation to
streamRelease()so error responses aren’t treated as tarball streams. - Attach an
errorhandler to the unzip stream inextract()and add unit tests covering these failure modes. - Update bootstrap error messaging for missing/unavailable app downloads.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/contentstack-bootstrap/test/github.test.js | Adds unit tests for streamRelease() status handling and extract() invalid gzip rejection. |
| packages/contentstack-bootstrap/src/bootstrap/index.ts | Adjusts loader lifecycle and maps GitHub 404s to a user-facing “app unavailable” error. |
| packages/contentstack-bootstrap/src/bootstrap/github/client.ts | Adds response status checking before returning the tarball stream; adds unzip error handling. |
| packages/contentstack-bootstrap/messages/index.json | Introduces a new user-facing message for app download unavailability. |
| .talismanrc | Updates checksums / ignore entries (incl. newly added test file). |
Suppressed comments (1)
packages/contentstack-bootstrap/src/bootstrap/github/client.ts:93
extract()now listens forunziperrors, but the sourcestreamcan still emit anerrorevent with no listener (Node treats that as an unhandled exception). Attach an error handler to the input stream so network/IO failures reject the Promise instead of crashing.
return new Promise((resolve, reject) => {
const unzip = zlib.createUnzip();
unzip.on('error', reject);
stream
.pipe(unzip)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Add stream.on('error', reject) to handle network/IO failures on the
source stream, not just zlib decompression errors
- Use distinct error message for non-404 HTTP failures (5xx, 403, etc.)
so users aren't told "repo not found" when it's a server/auth error
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
packages/contentstack-bootstrap/test/github.test.js:2
github.test.jsnow requiressinon, butpackages/contentstack-bootstrap/package.jsondoes not declare it indevDependencies. This makes the test suite depend on workspace hoisting (e.g.shamefully-hoist) and can break if hoisting settings change.
const sinon = require('sinon');
packages/contentstack-bootstrap/test/github.test.js:93
- Avoid using a token-like literal in tests if it triggers secret-scanner false positives. Using a clearly dummy value also makes it easier to remove the
.talismanrcallowlist entry for this file.
const client = new GitHubClient(GitHubClient.parsePath('contentstack/private-repo'), true, 'my-token');
await client.streamRelease(client.gitTarBallUrl);
const callOptions = httpStub.options.firstCall.args[0];
expect(callOptions.headers).to.deep.equal({ Authorization: 'token my-token' });
| - filename: packages/contentstack-bootstrap/test/github.test.js | ||
| checksum: b7badfcd3bbad0cb876364542bba26cdfd854f1b138be2896b5f84c219767040 |
There was a problem hiding this comment.
The false positive is unavoidable here — the test asserts that the exact string 'Authorization' is used as the header key, which is the correct and required HTTP header name. Removing or obfuscating it would reduce test fidelity. The talismanrc entry is scoped to this specific file with a checksum, so any future edits to the file will force a new checksum update and re-review. Risk is low and accepted.
| "CLI_BOOTSTRAP_GITHUB_ACCESS_NOT_FOUND": "No Github access token found", | ||
| "CLI_BOOTSTRAP_START_CLONE_APP": "Cloning the selected app", | ||
| "CLI_BOOTSTRAP_REPO_NOT_FOUND": "Unable to find a repo for \"%s\"", | ||
| "CLI_BOOTSTRAP_APP_UNAVAILABLE": "Unable to download \"%s\": branch \"cli-use\" not found. Ensure the branch exists on the GitHub repository.", |
There was a problem hiding this comment.
Fixed in 28f9ef3. Widened the message to: "Unable to download '%s': the repository or branch 'cli-use' was not found. Ensure both exist on GitHub." — accurate for both a missing branch and a missing/inaccessible repo.
| if (response.status >= 400) { | ||
| const message = response.status === 404 | ||
| ? messageHandler.parse('CLI_BOOTSTRAP_REPO_NOT_FOUND', `${this.repo.user}/${this.repo.name}`) | ||
| : messageHandler.parse('CLI_BOOTSTRAP_GITHUB_SERVER_ERROR', `${this.repo.user}/${this.repo.name}`, response.status); | ||
| throw new GithubError(message, response.status); |
There was a problem hiding this comment.
Fixed in 28f9ef3. Changed the check to response.status < 200 || response.status >= 400 so any non-2xx response — including unexpected 3xx that might slip past axios's redirect handling — is rejected rather than treated as a valid tarball stream.
- Widen CLI_BOOTSTRAP_APP_UNAVAILABLE to cover both repo and branch missing, not just branch, since GitHub returns 404 for both cases - Change status check from >= 400 to < 200 || >= 400 so unexpected non-2xx responses (e.g. stray 3xx) are also rejected as invalid Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🔒 Security Scan Results
⏱️ SLA Breach Summary
✅ BUILD PASSED - All security checks passed |
|
|
||
| const response = await HttpClient.create().options(options).get(url); | ||
|
|
||
| if (response.status < 200 || response.status >= 400) { |
| try { | ||
| await this.ghClient.getLatest(this.cloneDirectory); | ||
| cliux.loader(); | ||
| } catch (error) { | ||
| if (error instanceof GithubError) { | ||
| if (error.status === 404) { | ||
| cliux.error(messageHandler.parse('CLI_BOOTSTRAP_REPO_NOT_FOUND', this.appConfig.source)); | ||
| } | ||
| cliux.loader(); | ||
| if (error instanceof GithubError && error.status === 404) { | ||
| throw new Error(messageHandler.parse('CLI_BOOTSTRAP_APP_UNAVAILABLE', this.appConfig.source)); | ||
| } |
| it('should return the response stream when status is 200', async () => { | ||
| const mockStream = new Readable({ read() {} }); | ||
| const httpStub = { get: sandbox.stub().resolves({ status: 200, data: mockStream }), options: sandbox.stub().returnsThis() }; | ||
| sandbox.stub(HttpClient, 'create').returns(httpStub); | ||
|
|
||
| const client = new GitHubClient(GitHubClient.parsePath('contentstack/kickstart-next')); | ||
| const result = await client.streamRelease(client.gitTarBallUrl); | ||
|
|
||
| expect(result).to.equal(mockStream); | ||
| }); |
Problem
csdx cm:bootstrapcrashed with an unhandledZ_DATA_ERROR(incorrect header check) when cloning the Kickstart Next.js starter app. Two bugs combined to cause this:streamRelease()did not check the HTTP response status. When thecli-usebranch was absent fromcontentstack/kickstart-next, codeload.github.com returned a404: Not Foundbody. That body stream was silently passed downstream as if it were a valid tarball.extract()had no error handler on thezlib.createUnzip()stream. Node'spipe()does not forward stream errors between stages. When zlib tried to decompress the"404: Not Found"bytes (which have no gzip magic header), it emitted anerrorevent on theUnzipinstance with no listener — causing an unhandled exception that crashed the process instead of rejecting the Promise cleanly.Relates to: DX-10257
Fix
streamRelease()— throwsGithubErrorwith the actual HTTP status code for any4xx/5xxresponse. The existingBootstrap.run()catch block already handlesGithubErrorwithstatus === 404and prints a user-friendly "Unable to find a repo" message; no caller changes needed.extract()— extracts thezlib.createUnzip()instance and attaches.on('error', reject)directly to it, so zlib errors reject the Promise rather than escaping as unhandled events.Test plan
packages/contentstack-bootstrap/test/github.test.jsstreamReleasethrowsGithubError(404)on a 404 responsestreamReleasethrowsGithubError(500)on a 500 responsestreamReleasereturns the data stream on a 200 responsestreamReleasesendsAuthorizationheader for private reposstreamReleasethrows immediately for private repos with no tokenextractrejects withZ_DATA_ERROR(not a process crash) on invalid gzip datacsdx cm:bootstrap→ Kickstart Next.js ran end-to-end successfully after the missingcli-usebranch was created on the repo🤖 Generated with Claude Code