Conversation
📝 WalkthroughWalkthroughThis pull request refactors the Cloudflare connector to align with an updated SDK pattern. Configuration field definitions transition from public exports to a centralized structure with field grouping support. The connector's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/connector/role.go (1)
394-401: Typo: "couldflare" should be "cloudflare".The error messages on lines 396 and 400 have a typo:
- "couldflare-connector: only users can have role membership revoked", + "cloudflare-connector: only users can have role membership revoked", ... - return nil, fmt.Errorf("couldflare-connector: only users can have role membership revoked") + return nil, fmt.Errorf("cloudflare-connector: only users can have role membership revoked")
🤖 Fix all issues with AI agents
In `@go.mod`:
- Around line 111-114: The go.mod lists vulnerable indirect dependencies
golang.org/x/crypto v0.34.0 and golang.org/x/oauth2 v0.26.0; update these
transitive modules to fixed releases by running: go get
golang.org/x/crypto@v0.45.0 golang.org/x/oauth2@v0.27.0 and then go mod tidy so
the go.mod entries for golang.org/x/crypto and golang.org/x/oauth2 are bumped to
the patched versions and the dependency graph is cleaned.
In `@pkg/config/config.go`:
- Around line 39-42: The FieldsDependentOn call currently inverts the
dependency: change the argument order so emailIdField depends on apiKeyField by
calling field.FieldsDependentOn([]field.SchemaField{emailIdField},
[]field.SchemaField{apiKeyField}) instead of
field.FieldsDependentOn([]field.SchemaField{apiKeyField},
[]field.SchemaField{emailIdField}); this ensures the required relationship is
correct per the FieldsDependentOn(fields, required) signature.
In `@pkg/connector/connector.go`:
- Around line 33-51: The New function can return a nil client when apiKey is set
but emailId is empty; update New() to validate inputs early: if apiToken is
empty and apiKey provided without emailId, return a clear error (e.g., "apiKey
requires emailId") instead of constructing a Cloudflare with nil client, or more
generally after attempting both cloudflare.NewWithAPIToken and cloudflare.New
check if client == nil and return an error; reference the New() function, the
Cloudflare struct, and the apiToken/apiKey/emailId variables and ensure callers
relying on Validate() get a deterministic failure.
🧹 Nitpick comments (3)
pkg/connector/user.go (2)
81-87: Consider returning&rs.SyncOpResults{}instead ofnilfor consistency.The
EntitlementsandGrantsmethods returnnilfor theSyncOpResultspointer, while similar methods inrole.goreturn&rs.SyncOpResults{}. For API consistency, consider returning an empty struct:func (o *UserResourceType) Entitlements(_ context.Context, _ *v2.Resource, _ rs.SyncOpAttrs) ([]*v2.Entitlement, *rs.SyncOpResults, error) { - return nil, nil, nil + return nil, &rs.SyncOpResults{}, nil } func (o *UserResourceType) Grants(_ context.Context, _ *v2.Resource, _ rs.SyncOpAttrs) ([]*v2.Grant, *rs.SyncOpResults, error) { - return nil, nil, nil + return nil, &rs.SyncOpResults{}, nil }
59-65: Inconsistent error message casing.The error messages use inconsistent casing for "Cloudflare":
- Line 59:
"Cloudflare: invalid page token error"- Line 65:
"cloudflare: could not retrieve users: %w"Consider standardizing to one style (preferably lowercase
cloudflare:to match conventional Go error formatting).pkg/connector/connector.go (1)
33-45: Clarify authentication method precedence.Both authentication methods can execute if the user provides both
apiTokenandapiKey+emailId. The current flow allows the API key method to overwrite the token-based client on line 41.If this precedence is intentional, consider adding an
elseor a comment. Otherwise, consider usingelse ifto prevent accidental overwrites:if apiToken != "" { client, err = cloudflare.NewWithAPIToken(apiToken, cloudflare.HTTPClient(httpClient)) if err != nil { return nil, nil, err } - } - - if apiKey != "" && emailId != "" { + } else if apiKey != "" && emailId != "" { client, err = cloudflare.New(apiKey, emailId, cloudflare.HTTPClient(httpClient))
go.mod
Outdated
| golang.org/x/crypto v0.34.0 // indirect | ||
| golang.org/x/exp v0.0.0-20250128182459-e0ece0dbea4c // indirect | ||
| golang.org/x/net v0.35.0 // indirect | ||
| golang.org/x/oauth2 v0.26.0 // indirect |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if crypto or oauth2 packages are directly imported anywhere in the codebase
echo "=== Checking for direct usage of x/crypto ==="
rg -l 'golang.org/x/crypto' --type go
echo "=== Checking for direct usage of x/oauth2 ==="
rg -l 'golang.org/x/oauth2' --type go
echo "=== Check go.sum for actual versions ==="
cat go.sum | grep -E 'golang.org/x/(crypto|oauth2)' | head -20Repository: ConductorOne/baton-cloudflare
Length of output: 3203
🏁 Script executed:
#!/bin/bash
# Check the go.mod file directly to see the full dependency list
echo "=== Full go.mod file showing direct dependencies ==="
cat go.mod
echo -e "\n=== Check if crypto or oauth2 are direct dependencies ==="
grep -E "golang.org/x/(crypto|oauth2)" go.mod
echo -e "\n=== Look for what actually depends on these packages ==="
cat go.sum | grep "golang.org/x/crypto\|golang.org/x/oauth2" | wc -lRepository: ConductorOne/baton-cloudflare
Length of output: 6792
🌐 Web query:
GO-2025-3487 GO-2025-4116 GO-2025-4134 GO-2025-4135 golang.org/x/crypto vulnerability
💡 Result:
These Go vulnerability IDs all affect golang.org/x/crypto, specifically the ssh and/or ssh/agent packages, and are primarily denial-of-service (panic / memory exhaustion) issues:
| Go vuln ID | Alias | Affected area | Impact | Fixed in golang.org/x/crypto |
|---|---|---|---|---|
| GO-2025-3487 | CVE-2025-22869 | golang.org/x/crypto/ssh |
DoS: slow/incomplete key exchange can cause pending content to be read into memory but never transmitted (notably for servers implementing file transfer protocols). | v0.35.0 (upgrade if < v0.35.0) [1] |
| GO-2025-4116 | CVE-2025-47913 | golang.org/x/crypto/ssh/agent |
DoS: SSH client can panic if it receives SSH_AGENT_SUCCESS when it expects a typed response. |
v0.43.0 (upgrade if < v0.43.0) [2][3] |
| GO-2025-4134 | CVE-2025-58181 | golang.org/x/crypto/ssh |
DoS: unbounded memory consumption when parsing GSSAPI auth requests due to missing validation of mechanism count. | v0.45.0 (upgrade if < v0.45.0) [4] |
| GO-2025-4135 | CVE-2025-47914 | golang.org/x/crypto/ssh/agent |
DoS: agent servers may panic on malformed “new identity” requests due to an out-of-bounds read (message size not validated). | v0.45.0 (upgrade if < v0.45.0) [5] |
Recommended remediation
To cover all four issues at once, upgrade to golang.org/x/crypto >= v0.45.0 (this is the highest “fixed in” version among the listed vulns) [4][5].
Example:
go get golang.org/x/crypto@v0.45.0
go mod tidySources: [1] [2] [3] [4] [5]
🌐 Web query:
GO-2025-3488 golang.org/x/oauth2 vulnerability
💡 Result:
GO-2025-3488 is a Denial-of-Service (DoS) vulnerability in golang.org/x/oauth2 where a malformed, attacker-controlled token can trigger unexpected/unbounded memory consumption during token parsing. It specifically impacts golang.org/x/oauth2/jws.Verify. [1][2]
- Aliases: CVE-2025-22868, GHSA-6v2p-p543-phr9 [1][2]
- Affected:
golang.org/x/oauth2< v0.27.0 (notablygolang.org/x/oauth2/jws) [1][2] - Fixed in: v0.27.0 [1][2]
- Severity (commonly reported): CVSS 3.1 7.5 (HIGH) (availability impact) [3]
What to do
-
Upgrade:
go get golang.org/x/oauth2@v0.27.0 go mod tidy
(Or upgrade to any newer version.) [1][2]
-
If you can’t upgrade immediately, reduce exposure by ensuring untrusted tokens aren’t fed into
jws.Verify(e.g., gate by size/structure before verification), since the issue is triggered by malformed token parsing. [1][2]
Sources: [1] OSV entry for GO-2025-3488, [2] Go Vulnerability Database report, [3] GitLab advisory (CVE-2025-22868).
Upgrade golang.org/x/crypto and golang.org/x/oauth2 to patched versions.
Multiple confirmed HIGH severity vulnerabilities affect these indirect dependencies:
golang.org/x/crypto v0.34.0 (4 DoS vulnerabilities in ssh/agent):
- GO-2025-3487 (CVE-2025-22869): Memory exhaustion from incomplete key exchange
- GO-2025-4116 (CVE-2025-47913): Client panic on unexpected SSH_AGENT_SUCCESS
- GO-2025-4134 (CVE-2025-58181): Unbounded memory consumption in GSSAPI auth parsing
- GO-2025-4135 (CVE-2025-47914): Agent server panic on malformed "new identity" requests
- Fixed in v0.45.0+
golang.org/x/oauth2 v0.26.0 (DoS via token parsing):
- GO-2025-3488 (CVE-2025-22868): Unexpected memory consumption during malformed token parsing
- Fixed in v0.27.0+
Upgrade to golang.org/x/crypto >= v0.45.0 and golang.org/x/oauth2 >= v0.27.0 via:
go get golang.org/x/crypto@v0.45.0 golang.org/x/oauth2@v0.27.0
go mod tidy
These are transitive dependencies pulled through baton-sdk and other packages. While no direct SSH or OAuth2 code is present in this repository, upgrading eliminates exposure if these packages are exercised through the dependency chain.
🧰 Tools
🪛 OSV Scanner (2.3.2)
[HIGH] 111-111: golang.org/x/crypto 0.34.0: Potential denial of service in golang.org/x/crypto
(GO-2025-3487)
[HIGH] 111-111: golang.org/x/crypto 0.34.0: Potential denial of service in golang.org/x/crypto/ssh/agent
(GO-2025-4116)
[HIGH] 111-111: golang.org/x/crypto 0.34.0: Unbounded memory consumption in golang.org/x/crypto/ssh
(GO-2025-4134)
[HIGH] 111-111: golang.org/x/crypto 0.34.0: Malformed constraint may cause denial of service in golang.org/x/crypto/ssh/agent
(GO-2025-4135)
[HIGH] 111-111: golang.org/x/crypto 0.34.0: golang.org/x/crypto/ssh/agent vulnerable to panic if message is malformed due to out of bounds read
[HIGH] 111-111: golang.org/x/crypto 0.34.0: golang.org/x/crypto Vulnerable to Denial of Service (DoS) via Slow or Incomplete Key Exchange
[HIGH] 111-111: golang.org/x/crypto 0.34.0: golang.org/x/crypto/ssh allows an attacker to cause unbounded memory consumption
[HIGH] 114-114: golang.org/x/oauth2 0.26.0: Unexpected memory consumption during token parsing in golang.org/x/oauth2
(GO-2025-3488)
[HIGH] 114-114: golang.org/x/oauth2 0.26.0: golang.org/x/oauth2 Improper Validation of Syntactic Correctness of Input vulnerability
🤖 Prompt for AI Agents
In `@go.mod` around lines 111 - 114, The go.mod lists vulnerable indirect
dependencies golang.org/x/crypto v0.34.0 and golang.org/x/oauth2 v0.26.0; update
these transitive modules to fixed releases by running: go get
golang.org/x/crypto@v0.45.0 golang.org/x/oauth2@v0.27.0 and then go mod tidy so
the go.mod entries for golang.org/x/crypto and golang.org/x/oauth2 are bumped to
the patched versions and the dependency graph is cleaned.
pkg/config/config.go
Outdated
| field.FieldsDependentOn( | ||
| []field.SchemaField{apiKeyField}, | ||
| []field.SchemaField{emailIdField}, | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for FieldsDependentOn usage patterns
rg -n "FieldsDependentOn" --type go -C 3
# Also look for the function definition in baton-sdk imports/modules
rg -n "func.*FieldsDependentOn" --type go
# Check the config.go file itself
echo "=== Full config.go content ==="
cat pkg/config/config.goRepository: ConductorOne/baton-cloudflare
Length of output: 4620
Reverse the argument order in FieldsDependentOn.
The function signature shows FieldsDependentOn(fields []SchemaField, required []SchemaField) with documentation stating: "the provided fields are valid if and only if every field in required are also present."
The current code has FieldsDependentOn([apiKeyField], [emailIdField]), which means "apiKeyField is valid only if emailIdField is present." This is backwards. When using API key authentication with Cloudflare, the email address is required alongside the API key. The correct relationship is: emailIdField depends on apiKeyField.
Swap the arguments:
Diff
field.FieldsDependentOn(
- []field.SchemaField{apiKeyField},
- []field.SchemaField{emailIdField},
+ []field.SchemaField{emailIdField},
+ []field.SchemaField{apiKeyField},
),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| field.FieldsDependentOn( | |
| []field.SchemaField{apiKeyField}, | |
| []field.SchemaField{emailIdField}, | |
| ), | |
| field.FieldsDependentOn( | |
| []field.SchemaField{emailIdField}, | |
| []field.SchemaField{apiKeyField}, | |
| ), |
🤖 Prompt for AI Agents
In `@pkg/config/config.go` around lines 39 - 42, The FieldsDependentOn call
currently inverts the dependency: change the argument order so emailIdField
depends on apiKeyField by calling
field.FieldsDependentOn([]field.SchemaField{emailIdField},
[]field.SchemaField{apiKeyField}) instead of
field.FieldsDependentOn([]field.SchemaField{apiKeyField},
[]field.SchemaField{emailIdField}); this ensures the required relationship is
correct per the FieldsDependentOn(fields, required) signature.
| if apiToken != "" { | ||
| client, err = cloudflare.NewWithAPIToken(apiToken, cloudflare.HTTPClient(httpClient)) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| } | ||
|
|
||
| if apiKey != "" && emailId != "" { | ||
| client, err = cloudflare.New(apiKey, emailId, cloudflare.HTTPClient(httpClient)) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| } | ||
|
|
||
| return &Cloudflare{ | ||
| client: client, | ||
| accountId: accountId, | ||
| emailId: emailId, | ||
| }, nil | ||
| }, nil, nil |
There was a problem hiding this comment.
Potential nil client when API key is provided without email.
The configuration requires at least one of apiToken or apiKey via FieldsAtLeastOneUsed, but the emailIdField dependency may be configured incorrectly (see config.go comment). If a user provides apiKey without emailId:
- Line 33:
apiToken != ""is false, client remains nil - Line 40:
apiKey != "" && emailId != ""is false (emailId is empty), client remains nil - A nil client is returned, causing a delayed failure in
Validate()
Consider adding early validation in New():
Proposed fix
+ if apiToken == "" && (apiKey == "" || emailId == "") {
+ return nil, nil, fmt.Errorf("cloudflare: either api-token or both api-key and email-id must be provided")
+ }
+
if apiToken != "" {🤖 Prompt for AI Agents
In `@pkg/connector/connector.go` around lines 33 - 51, The New function can return
a nil client when apiKey is set but emailId is empty; update New() to validate
inputs early: if apiToken is empty and apiKey provided without emailId, return a
clear error (e.g., "apiKey requires emailId") instead of constructing a
Cloudflare with nil client, or more generally after attempting both
cloudflare.NewWithAPIToken and cloudflare.New check if client == nil and return
an error; reference the New() function, the Cloudflare struct, and the
apiToken/apiKey/emailId variables and ensure callers relying on Validate() get a
deterministic failure.
cmd/baton-cloudflare/main.go
Outdated
| cfg.Config, | ||
| connectorrunner.WithDefaultCapabilitiesConnectorBuilder(&connector.Cloudflare{}), | ||
| connector.New, | ||
| connectorrunner.WithSessionStoreEnabled(), |
There was a problem hiding this comment.
I think the session store is not required, we just leave it out when it isn't https://github.com/ConductorOne/baton-boomi/blob/1d520ee555650f94d1f25559b96a576834a4402f/cmd/baton-boomi/main.go#L18-L24
laurenleach
force-pushed
the
containerize-cloudflare
branch
from
6e8930a to
c2ace46
Compare
|
Claude finished @laurenleach's task in 5m 43s —— View job PR Review: Containerize baton-cloudflare connectorIssues: 7 (2 introduced by PR, 5 pre-existing) Breaking ChangesNone detected (no resource type ID changes, entitlement slug changes, or resource ID derivation changes). IssuesIntroduced by this PR:
Pre-existing (not regressions, but worth fixing):
Reviewer Feedback Check
DocumentationUp to date — no resource types, entitlements, or auth methods were changed. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| apiKeyField = field.StringField( | ||
| "api-key", | ||
| field.WithIsSecret(true), | ||
| field.WithDisplayName("API Key"), | ||
| field.WithDescription("The api key for the Cloudflare account."), | ||
| field.WithIsSecret(true), | ||
| field.WithRequired(true), | ||
| ) | ||
| ApiTokenField = field.StringField( | ||
| apiTokenField = field.StringField( | ||
| "api-token", | ||
| field.WithIsSecret(true), | ||
| field.WithDisplayName("API Token"), | ||
| field.WithDescription("The api token for the Cloudflare account."), | ||
| field.WithIsSecret(true), | ||
| field.WithRequired(true), | ||
| ) | ||
| AccountIdField = field.StringField( | ||
| accountIdField = field.StringField( | ||
| "account-id", | ||
| field.WithRequired(true), | ||
| field.WithDisplayName("Account ID"), | ||
| field.WithDescription("The account id for the Cloudflare account."), | ||
| field.WithRequired(true), | ||
| ) | ||
| EmailIdField = field.StringField( | ||
| emailIdField = field.StringField( | ||
| "email-id", | ||
| field.WithDisplayName("Email ID"), | ||
| field.WithDescription("The email id for the Cloudflare account."), | ||
| field.WithRequired(true), | ||
| ) | ||
|
|
||
| FieldRelationships = []field.SchemaFieldRelationship{ | ||
| field.FieldsAtLeastOneUsed(ApiTokenField, ApiKeyField), | ||
| field.FieldsDependentOn( | ||
| []field.SchemaField{ApiKeyField}, | ||
| []field.SchemaField{EmailIdField}, | ||
| ), | ||
| configurationFields = []field.SchemaField{ | ||
| apiKeyField, | ||
| apiTokenField, | ||
| accountIdField, | ||
| emailIdField, | ||
| } |
There was a problem hiding this comment.
All four fields (api-key, api-token, account-id, email-id) are now marked WithRequired(true). This breaks the previous behavior where users could authenticate with either api-token alone or api-key + email-id.
The old code used field.FieldsAtLeastOneUsed(apiTokenField, apiKeyField) and field.FieldsDependentOn(...) to enforce this correctly. These constraints have been removed and replaced with field groups, but field groups are UI-only metadata — they don't enforce validation.
Recommended fix: Either re-add field.WithConstraints(...) alongside the field groups, or remove WithRequired(true) from apiKeyField, apiTokenField, and emailIdField and rely on the constraint relationships instead.
| if apiToken != "" { | ||
| client, err = cloudflare.NewWithAPIToken(apiToken, cloudflare.HTTPClient(httpClient)) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| } | ||
|
|
||
| if apiKey != "" && emailId != "" { | ||
| client, err = cloudflare.New(apiKey, emailId, cloudflare.HTTPClient(httpClient)) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| } |
There was a problem hiding this comment.
Both if blocks can execute sequentially — if the user provides both apiToken and apiKey+emailId, the second block silently overwrites the token-based client. Use else if to make these mutually exclusive.
Also, if neither auth method matches (e.g., apiKey provided without emailId), client remains nil and will cause nil pointer panics during sync. Consider adding a guard after the auth blocks:
if client == nil {
return nil, nil, fmt.Errorf("baton-cloudflare: either api-token or both api-key and email-id must be provided")
}
|
Claude finished @laurenleach's task in 7m 35s —— View job PR Review: Containerize baton-cloudflare connectorIssues: 8 (2 introduced by PR, 6 pre-existing) Breaking ChangesNone detected (no resource type ID changes, entitlement slug changes, or resource ID derivation changes). IssuesIntroduced by this PR:
Pre-existing (not regressions, worth fixing):
Reviewer Feedback Check
DocumentationDocs exist at |
| apiKeyField = field.StringField( | ||
| "api-key", | ||
| field.WithIsSecret(true), | ||
| field.WithDisplayName("API Key"), | ||
| field.WithDescription("The api key for the Cloudflare account."), | ||
| field.WithIsSecret(true), | ||
| field.WithRequired(true), | ||
| ) | ||
| ApiTokenField = field.StringField( | ||
| apiTokenField = field.StringField( | ||
| "api-token", | ||
| field.WithIsSecret(true), | ||
| field.WithDisplayName("API Token"), | ||
| field.WithDescription("The api token for the Cloudflare account."), | ||
| field.WithIsSecret(true), | ||
| field.WithRequired(true), | ||
| ) | ||
| AccountIdField = field.StringField( | ||
| accountIdField = field.StringField( | ||
| "account-id", | ||
| field.WithRequired(true), | ||
| field.WithDisplayName("Account ID"), | ||
| field.WithDescription("The account id for the Cloudflare account."), | ||
| field.WithRequired(true), | ||
| ) | ||
| EmailIdField = field.StringField( | ||
| emailIdField = field.StringField( | ||
| "email-id", | ||
| field.WithDisplayName("Email ID"), | ||
| field.WithDescription("The email id for the Cloudflare account."), | ||
| field.WithRequired(true), | ||
| ) | ||
|
|
||
| FieldRelationships = []field.SchemaFieldRelationship{ | ||
| field.FieldsAtLeastOneUsed(ApiTokenField, ApiKeyField), | ||
| field.FieldsDependentOn( | ||
| []field.SchemaField{ApiKeyField}, | ||
| []field.SchemaField{EmailIdField}, | ||
| ), | ||
| configurationFields = []field.SchemaField{ | ||
| apiKeyField, | ||
| apiTokenField, | ||
| accountIdField, | ||
| emailIdField, | ||
| } |
There was a problem hiding this comment.
All four fields now have WithRequired(true), but the field.WithConstraints(...) with FieldsAtLeastOneUsed and FieldsDependentOn have been removed. Field groups are UI-only metadata — they don't enforce validation at runtime.
This means users must provide all of api-key, api-token, account-id, and email-id, breaking the prior either/or authentication model where they could use api-token alone or api-key + email-id.
The old config_test.go (now deleted) validated this constraint behavior with test cases like "with api token" (valid), "with api key but missing email ID" (invalid), etc.
Fix: Remove WithRequired(true) from apiKeyField, apiTokenField, and emailIdField, and re-add field.WithConstraints(...):
| apiKeyField = field.StringField( | |
| "api-key", | |
| field.WithIsSecret(true), | |
| field.WithDisplayName("API Key"), | |
| field.WithDescription("The api key for the Cloudflare account."), | |
| field.WithIsSecret(true), | |
| field.WithRequired(true), | |
| ) | |
| ApiTokenField = field.StringField( | |
| apiTokenField = field.StringField( | |
| "api-token", | |
| field.WithIsSecret(true), | |
| field.WithDisplayName("API Token"), | |
| field.WithDescription("The api token for the Cloudflare account."), | |
| field.WithIsSecret(true), | |
| field.WithRequired(true), | |
| ) | |
| AccountIdField = field.StringField( | |
| accountIdField = field.StringField( | |
| "account-id", | |
| field.WithRequired(true), | |
| field.WithDisplayName("Account ID"), | |
| field.WithDescription("The account id for the Cloudflare account."), | |
| field.WithRequired(true), | |
| ) | |
| EmailIdField = field.StringField( | |
| emailIdField = field.StringField( | |
| "email-id", | |
| field.WithDisplayName("Email ID"), | |
| field.WithDescription("The email id for the Cloudflare account."), | |
| field.WithRequired(true), | |
| ) | |
| FieldRelationships = []field.SchemaFieldRelationship{ | |
| field.FieldsAtLeastOneUsed(ApiTokenField, ApiKeyField), | |
| field.FieldsDependentOn( | |
| []field.SchemaField{ApiKeyField}, | |
| []field.SchemaField{EmailIdField}, | |
| ), | |
| configurationFields = []field.SchemaField{ | |
| apiKeyField, | |
| apiTokenField, | |
| accountIdField, | |
| emailIdField, | |
| } | |
| apiKeyField = field.StringField( | |
| "api-key", | |
| field.WithDisplayName("API Key"), | |
| field.WithDescription("The api key for the Cloudflare account."), | |
| field.WithIsSecret(true), | |
| ) | |
| apiTokenField = field.StringField( | |
| "api-token", | |
| field.WithDisplayName("API Token"), | |
| field.WithDescription("The api token for the Cloudflare account."), | |
| field.WithIsSecret(true), | |
| ) | |
| accountIdField = field.StringField( | |
| "account-id", | |
| field.WithDisplayName("Account ID"), | |
| field.WithDescription("The account id for the Cloudflare account."), | |
| field.WithRequired(true), | |
| ) | |
| emailIdField = field.StringField( | |
| "email-id", | |
| field.WithDisplayName("Email ID"), | |
| field.WithDescription("The email id for the Cloudflare account."), | |
| ) | |
| configurationFields = []field.SchemaField{ | |
| apiKeyField, | |
| apiTokenField, | |
| accountIdField, | |
| emailIdField, | |
| } |
And add constraints back to the Config definition:
var Config = field.NewConfiguration(
configurationFields,
field.WithConnectorDisplayName("Cloudflare"),
field.WithHelpUrl("/docs/baton/cloudflare"),
field.WithIconUrl("/static/app-icons/cloudflare.svg"),
field.WithConstraints(
field.FieldsAtLeastOneUsed(apiTokenField, apiKeyField),
field.FieldsDependentOn(
[]field.SchemaField{emailIdField},
[]field.SchemaField{apiKeyField},
),
),
field.WithFieldGroups([]field.SchemaFieldGroup{...}),
)
| if apiToken != "" { | ||
| client, err = cloudflare.NewWithAPIToken(apiToken, cloudflare.HTTPClient(httpClient)) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| } | ||
|
|
||
| if apiKey != "" && emailId != "" { | ||
| client, err = cloudflare.New(apiKey, emailId, cloudflare.HTTPClient(httpClient)) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
| } | ||
|
|
||
| return &Cloudflare{ | ||
| client: client, | ||
| accountId: accountId, | ||
| emailId: emailId, | ||
| }, nil | ||
| }, nil, nil |
There was a problem hiding this comment.
Two issues here:
-
Both
ifblocks can execute sequentially. If the user provides bothapiTokenandapiKey+emailId, the second block silently overwrites the token-based client. Useelse ifto make these mutually exclusive. -
Nil client possible. If neither auth method matches (e.g.,
apiKeyprovided withoutemailIddue to the now-broken config validation),clientremainsniland will cause nil pointer panics during sync.
| if apiToken != "" { | |
| client, err = cloudflare.NewWithAPIToken(apiToken, cloudflare.HTTPClient(httpClient)) | |
| if err != nil { | |
| return nil, err | |
| return nil, nil, err | |
| } | |
| } | |
| if apiKey != "" && emailId != "" { | |
| client, err = cloudflare.New(apiKey, emailId, cloudflare.HTTPClient(httpClient)) | |
| if err != nil { | |
| return nil, err | |
| return nil, nil, err | |
| } | |
| } | |
| return &Cloudflare{ | |
| client: client, | |
| accountId: accountId, | |
| emailId: emailId, | |
| }, nil | |
| }, nil, nil | |
| if apiToken != "" { | |
| client, err = cloudflare.NewWithAPIToken(apiToken, cloudflare.HTTPClient(httpClient)) | |
| if err != nil { | |
| return nil, nil, err | |
| } | |
| } else if apiKey != "" && emailId != "" { | |
| client, err = cloudflare.New(apiKey, emailId, cloudflare.HTTPClient(httpClient)) | |
| if err != nil { | |
| return nil, nil, err | |
| } | |
| } | |
| if client == nil { | |
| return nil, nil, fmt.Errorf("baton-cloudflare: either api-token or both api-key and email-id must be provided") | |
| } | |
| return &Cloudflare{ | |
| client: client, | |
| accountId: accountId, | |
| emailId: emailId, | |
| }, nil, nil |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
27-27: Declaregenerateas PHONY.The
generatetarget should be declared.PHONYsince it's not a real file. This prevents issues if a file namedgenerateever exists.Proposed fix
+.PHONY: generate generate: $(GENERATED_CONF)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 27, The Makefile's generate target is not declared phony; add a .PHONY declaration for the target name "generate" so make won't confuse the target with a same-named file—update the Makefile to include ".PHONY: generate" (either in an existing .PHONY list or as its own line) to cover the generate target that depends on the GENERATED_CONF variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Makefile`:
- Line 27: The Makefile's generate target is not declared phony; add a .PHONY
declaration for the target name "generate" so make won't confuse the target with
a same-named file—update the Makefile to include ".PHONY: generate" (either in
an existing .PHONY list or as its own line) to cover the generate target that
depends on the GENERATED_CONF variable.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Makefilecmd/baton-cloudflare/config.gocmd/baton-cloudflare/config_test.gocmd/baton-cloudflare/main.gopkg/config/config.gopkg/connector/connector.gopkg/connector/model.gopkg/connector/role.gopkg/connector/user.go
💤 Files with no reviewable changes (3)
- pkg/connector/model.go
- cmd/baton-cloudflare/config_test.go
- cmd/baton-cloudflare/config.go
3 participants
Summary
This PR containerizes the baton-cloudflare connector following the patterns from baton-databricks#35 and baton-contentful#48.
Changes
Testing
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com
Summary by CodeRabbit
New Features
Chores