Skip to content

fix: use gofalcon SDK's actual error interfaces for status extraction#63

Open
c1-dev-bot[bot] wants to merge 1 commit into
mainfrom
fix/cxh-2022-error-extraction
Open

fix: use gofalcon SDK's actual error interfaces for status extraction#63
c1-dev-bot[bot] wants to merge 1 commit into
mainfrom
fix/cxh-2022-error-extraction

Conversation

@c1-dev-bot

@c1-dev-bot c1-dev-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown

Summary

Fixes broken error extraction in the CrowdStrike connector that caused all API errors to be classified as codes.Unknown (gRPC), which surfaces as HTTP 500 with no useful error classification.

Root Cause

extractCrowdStrikeError defined a csError interface requiring GetErrors() []models.MsaAPIError, but no gofalcon SDK type implements this interface. The SDK's typed error responses (e.g. QueryUserV1InternalServerError) use Code() int and GetPayload() instead. This meant the structured error extraction always returned (0, ""), forcing every error through the string-matching fallback.

The fallback itself had gaps — it checked for "500" and "503" but not "502" (Bad Gateway) or "504" (Gateway Timeout), and missed common OAuth2 error patterns. Errors from infrastructure (CDN/load balancer 502s) or OAuth2 token failures fell through to the default codes.Unknown case.

Changes

  • Replace broken csError interface with httpCoder (Code() int) that typed gofalcon SDK errors actually implement
  • Add runtime.APIError detection for status codes not covered by the swagger spec (the gofalcon SDK's default handler for unexpected HTTP responses)
  • Expand 5xx fallback to include 502, 504, "bad gateway", "gateway timeout"
  • Add OAuth2 error patterns ("access_denied", "invalid client") to authentication fallback

Impact

Errors from the CrowdStrike Falcon API will now be properly classified (e.g. 502 → codes.Unavailable instead of codes.Unknown), enabling correct retry behavior and meaningful error messages for users.

Fixes: CXH-2022


Automated PR Notice

This PR was automatically created by c1-dev-bot as a potential implementation.

This code requires:

  • Human review of the implementation approach
  • Manual testing to verify correctness
  • Approval from the appropriate team before merging

…ction

The extractCrowdStrikeError function defined a csError interface
(GetErrors() []models.MsaAPIError) that no gofalcon SDK type
implements. This caused ALL error extraction to fail silently,
falling through to string-matching which missed HTTP 502, 504,
and other status codes — producing codes.Unknown (gRPC) which
maps to HTTP 500 with no useful classification.

Fix by:
- Using the httpCoder interface (Code() int) that typed gofalcon
  response errors actually implement
- Detecting runtime.APIError for unhandled status codes
- Adding 502, 504, "bad gateway", "gateway timeout" to fallback
- Adding OAuth2 error patterns ("access_denied", "invalid client")

Fixes: CXH-2022
@c1-dev-bot
c1-dev-bot Bot requested a review from a team July 10, 2026 19:14
@linear-code

linear-code Bot commented Jul 10, 2026

Copy link
Copy Markdown

CXH-2022

Comment thread pkg/connector/errors.go
// Authentication errors
if strings.Contains(errMsg, "401") || strings.Contains(errMsg, "unauthorized") {
if strings.Contains(errMsg, "401") || strings.Contains(errMsg, "unauthorized") ||
strings.Contains(errMsg, "access_denied") || strings.Contains(errMsg, "invalid client") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: The OAuth2 error code is invalid_client (underscore), but this matches "invalid client" (space), so it likely won't match the actual token-endpoint error string (e.g. golang.org/x/oauth2 renders oauth2: "invalid_client" ...). Consider strings.Contains(errMsg, "invalid_client"). (Confidence: medium — most such failures also contain "401"/"unauthorized" and would still be caught.)

Comment thread pkg/connector/errors.go
//
// 2. Generic runtime.APIError for status codes not covered by the swagger
// spec — these carry the code in the APIError.Code field.
func extractCrowdStrikeError(err error) (int, string) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Suggestion: This changes error classification substantially — the old csError/GetErrors() path was dead code, so nearly all errors previously fell through to codes.Unknown. There are no tests for extractCrowdStrikeError. A table-driven errors_test.go covering a typed gofalcon error (e.g. QueryUserV1Forbidden), a *runtime.APIError, and the string fallback would guard this behavior against regressions. (Confidence: medium)

@github-actions

Copy link
Copy Markdown
Contributor

Connector PR Review: fix: use gofalcon SDK's actual error interfaces for status extraction

Blocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base a7a546f884ce.
Review mode: full
View review run

Review Summary
Reviewed the full PR diff for security and correctness. The fix is sound: I verified against the vendored SDK that gofalcon typed error responses (e.g. QueryUserV1BadRequest) implement Code() int + Error(), satisfying the new httpCoder interface, and that *runtime.APIError exposes a Code int field caught by the second branch (it has no Code() method, so the two branches do not overlap). The removed models import is genuinely unused. The old csError / GetErrors() interface was matched by no SDK type, so it was dead code that pushed all errors to codes.Unknown; this change correctly restores structured status extraction. No security or blocking correctness issues found.

Security Issues
None found.

Correctness Issues
None found.

Suggestions

  • pkg/connector/errors.go:46 — Added OAuth2 pattern "invalid client" uses a space, but the OAuth2 error code is invalid_client (underscore); likely will not match the real error string.
  • pkg/connector/errors.go:96 — No tests for the substantially changed extractCrowdStrikeError behavior; a table-driven test would guard against regressions.
  • go.mod:74github.com/go-openapi/runtime is now a direct import (errors.go:9) but still annotated // indirect; go mod tidy would promote it and a CI tidy check could fail. (Not inlined; go.mod is unchanged in this PR.)
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

Suggestions:

In pkg/connector/errors.go:
- Around line 46: The added OAuth2 authentication fallback pattern checks
  strings.Contains(errMsg, "invalid client") with a space, but the OAuth2 spec
  error code is "invalid_client" with an underscore. Change the literal to
  "invalid_client" so it matches real token-endpoint error strings.
- Around line 96: extractCrowdStrikeError now performs the real status-code
  extraction (the previous csError/GetErrors path was dead code). Add an
  errors_test.go with table-driven cases covering a typed gofalcon error that
  implements Code()/Error() (e.g. QueryUserV1Forbidden), a runtime.APIError with
  Code >= 400, and an error that only matches the string fallback, asserting the
  extracted status code plus message and the resulting gRPC code from wrapCrowdStrikeError.

In go.mod:
- Around line 74: github.com/go-openapi/runtime is now imported directly in
  pkg/connector/errors.go but is still marked indirect. Run go mod tidy to
  promote it to a direct require and keep the manifest consistent with the code.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants