fix: Check the token response body read error where the body is load-bearing - #68
Open
keelerm84 wants to merge 2 commits into
Open
fix: Check the token response body read error where the body is load-bearing#68keelerm84 wants to merge 2 commits into
keelerm84 wants to merge 2 commits into
Conversation
…bearing
authorizeSalesforce read the token response body and then ignored the read
error until after both status branches had already used the bytes. The error
was only ever logged, which is why it never surfaced as a defect.
The obvious fix -- hoist "if err != nil { return err, false }" above the status
branches -- is wrong, and this commit deliberately does not do it. A 401 or 403
whose body read fails would then return a non-permanent error, so the daemon
would retry forever against a credential Salesforce has already rejected. That
trades a cosmetically tidy check for the exact failure the permanent flag exists
to prevent.
The status line arrives ahead of the body, so net/http delivers a complete
StatusCode whether or not the body read finished. A truncated body therefore puts
nothing about the status code in doubt, and each branch can judge the read error
on whether it actually depends on the body:
- 401 and 403: the status code alone proves the credential is rejected, and the
body only feeds the log line. The classification stays permanent.
- Any other non-200: the body becomes the error message, so a failed read is
returned instead of a half-received message standing in for the failure.
- 200: the body is the token document, so a failed read is returned rather than
a partial document being parsed for a token.
The generic non-200 branch also returned errors.New on the body alone, which
prints nothing when the body is empty -- and a 503 with no body from a load
balancer in front of Salesforce is ordinary. The status code now goes in the
message either way, so the caller always has something to log.
Tests cover the read failure against each status class that is treated
differently, asserting the returned error and the permanent flag. Verified
discriminating: the 401 and 403 rows fail against the naive hoist, and the 500
row plus both message rows fail against the code before this change.
The non-200 branch returned the read error bare while the branch beside it built a message carrying the status code. Both paths report the same failure to the same caller, which only logs it, and "unexpected EOF" alone does not say what failed. Wrapped with %w rather than concatenated, so errors.Is still reaches the underlying read error.
tanderson-ld
approved these changes
Aug 31, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
authorizeSalesforcecaptured the error from reading the token response body but didnot inspect it until after both status branches had already used those bytes -- the
error was passed to
log.Printin each branch, which is how it surfaced without everbeing handled.
The naive fix, hoisting one check above the status branches, would be a regression. A
401 or 403 on a body read failure currently returns a permanent error, and hoisting
makes it non-permanent, leaving the daemon retrying forever against a credential
Salesforce has already rejected.
So the read error is judged per branch, according to whether the branch depends on
the bytes. The status line arrives ahead of the body, so
net/httpreturns acomplete
StatusCodewhether or not the read finished; a truncated body casts nodoubt on the status.
only feeds the log line. Stays permanent.
reported here.
json.Unmarshal.Two further fixes in the same lines. The generic non-200 branch returned
errors.New(string(errorBody)), which on an empty body is a non-nil error thatprints nothing -- a bodyless 503 from a load balancer in front of Salesforce is
ordinary, and the existing
503 is retriedtest case passed only because it assertederr != nil. The message now always carries the status code, on both the read-failurepath and the normal one. The local was also renamed to
readErr, since which error isin hand is the whole bug.
The new tests fix one grant, since every line under test runs after
client.Doandexisting coverage already establishes that response handling is grant-independent.
They were verified to discriminate in both directions: against the original code, and
against the naive hoist, which fails the 401 and 403 rows on the permanent flag.
Note
Overview
authorizeSalesforcenow treats a truncated token response body differently depending on HTTP status, instead of only logging the read error while still using partial bytes.For 401/403, the failure stays permanent and returns
Salesforce Unauthorizedeven when the body read fails—so bad credentials are not retried forever. For other non-200 responses, a body read failure is returned (wrapped with the status code); otherwise the error message always includes the status and body text, fixing empty errors on bodyless responses like 503. For 200, a partial body read is returned before JSON parsing so a truncated token is never stored.Adds
authorize_readerr_test.gowith a truncating mock token server and tests that pin the permanent vs retry behavior and non-empty transient error messages (including guarding against a naive “check readErr first” regression).Reviewed by Cursor Bugbot for commit 099edf9. Bugbot is set up for automated code reviews on this repo. Configure here.