Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| var resData accountResponse | ||
| if err = json.Unmarshal(resBody, &resData); err != nil { | ||
| ctx.Logger().Error(err, "failed to parse response") | ||
| return false, extraData, err |
There was a problem hiding this comment.
Parsing failure incorrectly invalidates verified access tokens
Medium Severity
The old verifyAccessToken returned verified=true unconditionally on HTTP 200, since the status code alone confirms the token is valid. The new code adds JSON body parsing and returns verified=false with an error if parsing fails. This means a valid token could be incorrectly reported as unverified due to a transient body-read issue or an API response format change — even though the HTTP 200 already confirmed validity. The LWA data extraction failure here is incorrectly treated as a verification failure.
There was a problem hiding this comment.
@lukem-ts cursor is correct in its explanation here. The changes here for lightweight analyze do change the detector behavior: if a response body doesn't parse, the finding would be considered live (i.e. verified) before, but now the error would be propagated, making it an "indeterminate" finding.
It's a question of judgment about how error handling should be done in the detectors. For now, let's keep existing detector behavior the same as it was before lightweight analyze, and only log parsing errors (like you've done here).
There was a problem hiding this comment.
For some reason GitHub is not showing the "add a suggestion" UI buttons, so I can't write a patch here directly, so here goes:
You probably want to change line 171 from this:
return false, extraData, err
to this in order to preserve the old behavior:
return true, extraData, nil
| Email: &resData.Info.Email, | ||
| }) | ||
|
|
||
| return true, extraData, nil, resData.AccessToken |
There was a problem hiding this comment.
Removed access_token validation in refresh token verification
Low Severity
The old code explicitly validated that access_token existed in the OAuth response and was a string, returning verified=false with an error if missing. The new code unconditionally returns verified=true with resData.AccessToken, which defaults to an empty string if the field is absent from the JSON. This means a malformed 200 response without an access_token would result in verified=true with an empty newAccessToken being set in AnalysisInfo.
There was a problem hiding this comment.
Yes, it looks like cursor is correct here also: if the API response doesn't include an access_token, instead of indeterminate status, the finding will get verified status?
| var resData accountResponse | ||
| if err = json.Unmarshal(resBody, &resData); err != nil { | ||
| ctx.Logger().Error(err, "failed to parse response") | ||
| return false, extraData, err |
There was a problem hiding this comment.
@lukem-ts cursor is correct in its explanation here. The changes here for lightweight analyze do change the detector behavior: if a response body doesn't parse, the finding would be considered live (i.e. verified) before, but now the error would be propagated, making it an "indeterminate" finding.
It's a question of judgment about how error handling should be done in the detectors. For now, let's keep existing detector behavior the same as it was before lightweight analyze, and only log parsing errors (like you've done here).
| var resData accountResponse | ||
| if err = json.Unmarshal(resBody, &resData); err != nil { | ||
| ctx.Logger().Error(err, "failed to parse response") | ||
| return false, extraData, err |
There was a problem hiding this comment.
For some reason GitHub is not showing the "add a suggestion" UI buttons, so I can't write a patch here directly, so here goes:
You probably want to change line 171 from this:
return false, extraData, err
to this in order to preserve the old behavior:
return true, extraData, nil
| lwa.AugmentExtraData(extraData, lwa.Fields{ | ||
| ID: &resData.Info.UUID, | ||
| Name: &resData.Info.Name, | ||
| Email: &resData.Info.Email, | ||
| }) |
There was a problem hiding this comment.
Just to confirm: there is no appropriate field in the digitalocean API response that we could use for the URL field?
| Email: &resData.Info.Email, | ||
| }) | ||
|
|
||
| return true, extraData, nil, resData.AccessToken |
There was a problem hiding this comment.
Yes, it looks like cursor is correct here also: if the API response doesn't include an access_token, instead of indeterminate status, the finding will get verified status?
3 participants
This draft PR is a PoC for what "lightweight analysis" could look like.
Note
Medium Risk
Changes detector verification flows for OpenAI and DigitalOcean to always read/close HTTP response bodies and attach richer
ExtraData, which could affect verification behavior and memory/logging characteristics. Risk is moderate because it touches live API calls and parsing in token verification paths.Overview
Adds a new
lightweight_analyzehelper package to support lightweight analysis by copying/closing HTTP response bodies and standardizing commonExtraDatafields (lwa.*).Updates the OpenAI and DigitalOceanV2 detectors’ verification code to use a logger-aware context, always preserve the raw API response in
ExtraData(lwa.response), and (when parsing succeeds) augment results with standard identity fields (id/name/email); DigitalOcean verification now also switches to typed response structs and returnsExtraDatafrom both access- and refresh-token verification paths.Written by Cursor Bugbot for commit ac8f950. This will update automatically on new commits. Configure here.