V8: Fix(cf curl): Prevent errors when handling binary API responses #3605
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gururajsh
reviewed
Oct 22, 2025
Member
gururajsh
left a comment
There was a problem hiding this comment.
Mostly LGTM. Just one question.
command/v7/curl_command.go
Outdated
| } | ||
|
|
||
| // Check if the response contains binary data | ||
| if IsBinary(responseBodyBytes) { |
Member
There was a problem hiding this comment.
Can we use utf8.Valid(bytesToWrite)? Avoids having custom check.
Contributor
Author
There was a problem hiding this comment.
Neither checking for presnece of null bytes in the response data nor checking for valid utf8 encoding data is sufficient/foolproof. I chose the cheking for presence of null bytes because curl library does the same check to determine the binary data. However , I updated the isBinary() function to rely on Content-Type for known binary MIME types and only use the above check ( presence of null bytes to determine the binray data) for other MIME types. Thanks
ee84d28 to
d45bbe1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the Change
Problem:
When cf cli receives binary data from the API, such as when downloading a droplet with the command
cf curl /v3/droplets/48155813-038b-4fbd-912d-7f8856c39f9c/download > /tmp/droplet
it crashes while trying to process the binary data.
Users can provide the
parameter as a workaround.
e.g:
Solution:
The fix is to detect when the response contains binary data and by writing the raw bytes directly to stdout instead of converting to string, we avoid data corruption.
Why Is This PR Valuable?
This PR would help users trying to download the binary data and trying to redirect the output using a pipe
Applicable Issues
List any applicable GitHub Issues here
How Urgent Is The Change?
Medium