Stop dereferencing a null Errors list on unexpected response bodies - #69
Open
Pichatorn-A4K wants to merge 1 commit into
Open
Stop dereferencing a null Errors list on unexpected response bodies#69Pichatorn-A4K wants to merge 1 commit into
Pichatorn-A4K wants to merge 1 commit into
Conversation
Every caller in CertCentralClient did
Errors errors = JsonConvert.DeserializeObject<Errors>(response.Response);
x.Errors = errors.errors;
which produces a null Errors list, or throws, in three cases that are all
reachable against the live API:
* an empty body - DeserializeObject<Errors>("") returns null outright, so
the assignment itself NREs;
* a JSON body that is not the {"errors":[...]} envelope - .errors is null.
GET /services/v2/account/metadata answers literally {} on an account with
no custom fields;
* a non-JSON body - a proxy or WAF error page raises JsonReaderException from
*outside* Request()'s try/catch, so it does not surface as a CA error at
all. DigiCert's responses are fronted by a CDN, so an HTML body is a real
shape on the wire.
Downstream there are eleven Errors[0] / .First() / .Count sites, so any of
those cases became a NullReferenceException instead of a diagnosable message.
ParseErrors() never returns null and never throws, and labels which of the
three cases occurred. CertCentralResponse also carries the HTTP status now,
so the message can name it - previously 4xx and 5xx were indistinguishable
to every caller.
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
1 participant
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.
Every caller in CertCentralClient did
which produces a null Errors list, or throws, in three cases that are all
reachable against the live API:
the assignment itself NREs;
GET /services/v2/account/metadata answers literally {} on an account with
no custom fields;
outside Request()'s try/catch, so it does not surface as a CA error at
all. DigiCert's responses are fronted by a CDN, so an HTML body is a real
shape on the wire.
Downstream there are eleven Errors[0] / .First() / .Count sites, so any of
those cases became a NullReferenceException instead of a diagnosable message.
ParseErrors() never returns null and never throws, and labels which of the
three cases occurred. CertCentralResponse also carries the HTTP status now,
so the message can name it - previously 4xx and 5xx were indistinguishable
to every caller.
Found during a pre-engagement audit of the shipped 2.4.1
net8.0assembly; verified by building and running the change against a fake client offline. Companion PRs from the same audit: #65, #66, #67.🤖 Generated with Claude Code