feat: add workspace-controlled SDK risk protection#2031
Conversation
github-actions
Bot
added
the
size/L
📝 WalkthroughWalkthroughAdds configurable workspace risk-control policy, cached credential/config provenance, host OS and product-model signal collection, and SDK transport enforcement for restricted headers on eligible official HTTPS OpenAPI requests. ChangesRisk-control signal flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Factory
participant CredentialProvider
participant RiskControlTransport
participant LarkAPI
CLI->>Factory: Create invocation configuration
Factory->>CredentialProvider: Resolve account and provenance
CredentialProvider-->>Factory: Workspace-managed status
Factory->>RiskControlTransport: Configure SDK transport policy
RiskControlTransport->>LarkAPI: Eligible request with trusted signals
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2031 +/- ##
==========================================
+ Coverage 75.06% 75.12% +0.05%
==========================================
Files 902 911 +9
Lines 95944 96425 +481
==========================================
+ Hits 72025 72439 +414
- Misses 18380 18413 +33
- Partials 5539 5573 +34 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@1e16cba6736d6be9f3a17325c7ddedaad52f903f🧩 Skill updatenpx skills add larksuite/cli#feat/workspace-sdk-risk-protection -y -g |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/config/risk_control.go`:
- Around line 36-40: Update the RunE configuration-loading error path around
core.LoadOrNotConfigured to preserve already typed errors while wrapping raw
failures in the established typed internal/storage error and attaching the
original error with WithCause(err); continue returning successful configuration
results unchanged.
In `@internal/core/config.go`:
- Around line 69-73: Add adjacent unit coverage for
MultiAppConfig.RiskControlEnabled, directly asserting true when RiskControl is
omitted, true when explicitly enabled, and false when explicitly disabled. Keep
the existing explicit-false round-trip test, but ensure the resolver itself is
exercised for all three states.
In `@internal/credential/credential_provider.go`:
- Around line 263-275: Update the token-resolution flow around ResolveAccount so
every read of p.resolution.tokenSource, including the existing fast path, occurs
after participating in accountOnce or under the same synchronization protecting
resolution writes. Preserve the defaultAcct nil behavior and return the resolved
token source or existing error when initialization completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a0211d66-767a-4021-8556-dc81e41b2a81
📒 Files selected for processing (32)
README.mdREADME.zh.mdcmd/config/config.gocmd/config/risk_control.gocmd/config/risk_control_test.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_proxy_warn_test.gointernal/cmdutil/factory_risk_control_test.gointernal/cmdutil/risk_control.gointernal/cmdutil/risk_control_route_sidecar_test.gointernal/cmdutil/risk_control_test.gointernal/cmdutil/secheader_test.gointernal/cmdutil/transport_test.gointernal/core/config.gointernal/core/config_snapshot.gointernal/core/config_snapshot_test.gointernal/core/config_test.gointernal/credential/credential_provider.gointernal/credential/credential_provider_test.gointernal/credential/default_provider.gointernal/credential/integration_test.gointernal/riskcontrol/host.gointernal/riskcontrol/host_darwin.gointernal/riskcontrol/host_darwin_test.gointernal/riskcontrol/host_linux.gointernal/riskcontrol/host_linux_test.gointernal/riskcontrol/host_other.gointernal/riskcontrol/host_test.gointernal/riskcontrol/host_windows.gointernal/riskcontrol/host_windows_test.gointernal/riskcontrol/transport.gointernal/riskcontrol/transport_test.go
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| config, err := core.LoadOrNotConfigured() | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return a typed error for configuration-load failures.
Returning err directly can bypass the command error-envelope contract. Pass through already typed errors; wrap raw load failures with a typed internal/storage error and .WithCause(err).
Proposed fix
config, err := core.LoadOrNotConfigured()
if err != nil {
- return err
+ if _, ok := errs.ProblemOf(err); ok {
+ return err
+ }
+ return errs.NewInternalError(errs.SubtypeStorage,
+ "failed to load risk-control policy: %v", err).WithCause(err)
}📝 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.
| RunE: func(cmd *cobra.Command, args []string) error { | |
| config, err := core.LoadOrNotConfigured() | |
| if err != nil { | |
| return err | |
| } | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| config, err := core.LoadOrNotConfigured() | |
| if err != nil { | |
| if _, ok := errs.ProblemOf(err); ok { | |
| return err | |
| } | |
| return errs.NewInternalError(errs.SubtypeStorage, | |
| "failed to load risk-control policy: %v", err).WithCause(err) | |
| } |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-39: cmd/config/risk_control.go#L39
Added line #L39 was not covered by tests
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/config/risk_control.go` around lines 36 - 40, Update the RunE
configuration-loading error path around core.LoadOrNotConfigured to preserve
already typed errors while wrapping raw failures in the established typed
internal/storage error and attaching the original error with WithCause(err);
continue returning successful configuration results unchanged.
Sources: Coding guidelines, Learnings
| // RiskControlEnabled resolves the workspace policy. An omitted preference | ||
| // keeps the default-on account-protection behavior. | ||
| func (m *MultiAppConfig) RiskControlEnabled() bool { | ||
| return m != nil && (m.RiskControl == nil || *m.RiskControl) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test the default-on resolver directly.
The round-trip test only verifies explicit false; it never calls RiskControlEnabled(). Cover omitted, explicit true, and explicit false so the default-on protection contract cannot regress unnoticed. Based on coding guidelines, “Every behavior change must include an adjacent test.”
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 71-72: internal/core/config.go#L71-L72
Added lines #L71 - L72 were not covered by tests
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/core/config.go` around lines 69 - 73, Add adjacent unit coverage for
MultiAppConfig.RiskControlEnabled, directly asserting true when RiskControl is
omitted, true when explicitly enabled, and false when explicitly disabled. Keep
the existing explicit-false round-trip test, but ensure the resolver itself is
exercised for all three states.
Sources: Coding guidelines, Linters/SAST tools
| if p.resolution.tokenSource != nil { | ||
| return p.resolution.tokenSource, nil | ||
| } | ||
| if p.defaultAcct == nil { | ||
| return nil, nil | ||
| } | ||
| if _, err := p.ResolveAccount(ctx); err != nil { | ||
| return nil, err | ||
| } | ||
| if p.selectedSource == nil { | ||
| if p.resolution.tokenSource == nil { | ||
| return nil, fmt.Errorf("credential provider resolved an account without selecting a token source") | ||
| } | ||
| return p.selectedSource, nil | ||
| return p.resolution.tokenSource, nil |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Synchronize the resolution fast path.
This reads p.resolution.tokenSource before participating in accountOnce, while ResolveAccount writes the full resolution inside that sync.Once. Concurrent account and token resolution can therefore race, potentially observing a partially initialized token source. Resolve through ResolveAccount before reading the cached resolution, or guard all resolution reads/writes with a mutex.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/credential/credential_provider.go` around lines 263 - 275, Update
the token-resolution flow around ResolveAccount so every read of
p.resolution.tokenSource, including the existing fast path, occurs after
participating in accountOnce or under the same synchronization protecting
resolution writes. Preserve the defaultAcct nil behavior and return the resolved
token source or existing error when initialization completes.
1 participant
Summary
Add workspace-controlled SDK risk protection for access-token requests while keeping credential-provider trust boundaries explicit. Host signals are collected only for workspace-managed config/keychain credentials and are never injected for environment, auth sidecar, or other extension credentials.
Changes
config risk-controlpolicy with default-on, explicit opt-out, and documentation.Test Plan
cmd,internal,shortcuts, andextension.make sidecar-testgo vet -p=1 ./...gofmt -l .produces no output.go mod tidydoes not changego.modorgo.sum.internal/riskcontrol.config risk-controlcommand flow is covered by command-level persistence and validation tests.Related Issues
Summary by CodeRabbit
on,off, anddefaultoptions.