Allow custom actions to return encrypted secret values - #1128
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if handler == nil { | ||
| return errors.New("action handler cannot be nil") | ||
| } | ||
| if hasSecretReturnTypes(schema) { |
There was a problem hiding this comment.
🟠 Bug: Register/RegisterAction/RegisterResourceAction now hard-reject any schema with an is_secret return type, and registration errors propagate out of addConnectorBuilderProviders/addActionManager to NewConnector — so the whole connector refuses to start, not just this action. Before this PR is_secret on config.Field meant only "the UI obscures this" (pkg/field.WithIsSecret → schemaFieldToV1), so an existing connector that marks a return type secret goes from working to a total startup failure. pkg/sdk/version.go is unchanged at v0.29.0, so downstream gets no version signal for the break.
Either bump the minor in pkg/sdk/version.go and add a migration note, or soften the rejection for a release (register the action via the non-secret path with a deprecation warning) so the break is opt-in rather than fatal at NewConnector.
There was a problem hiding this comment.
There are currently no connectors with is_secret in their return schemas, so this is acceptable. We only bump versions when cutting a release, which is a separate process from pull requests.
| ctxzap.Extract(ctx).Error("panic in action handler", | ||
| zap.String("resource_type", resourceTypeID), | ||
| zap.String("action", actionName), | ||
| zap.Any("panic", r), | ||
| zap.Stack("stack")) | ||
| oa.SetError(ctx, fmt.Errorf("panic in action handler: %v", r)) | ||
| oa.SetError(ctx, errors.New("panic in action handler")) |
There was a problem hiding this comment.
🟡 Suggestion: panic redaction is applied in invokeRegisteredAction, which is the shared path for every handler. Non-secret action handlers previously logged zap.Any("panic", r) and surfaced panic in action handler: %v; both now drop the panic value, so ordinary connector panics lose their message and leave only a stack. Consider keeping the value when !handler.returnsSecrets so existing actions don't regress in debuggability.
| // WithOnDemandInvokeAction creates an option for invoking an action. | ||
| // If resourceTypeID is provided, it invokes a resource-scoped action. | ||
| func WithOnDemandInvokeAction(c1zPath string, action string, resourceTypeID string, args *structpb.Struct) Option { | ||
| return WithOnDemandInvokeActionWithEncryption(c1zPath, action, resourceTypeID, args, nil) |
There was a problem hiding this comment.
🟡 Suggestion: WithOnDemandInvokeActionWithEncryption and local.NewActionInvokerWithEncryption have no caller that supplies recipients — pkg/cli/commands.go:307 still uses WithOnDemandInvokeAction, which passes nil. So baton --invoke-action against a secret action always fails with at least one encryption config is required for secret action results, and connector developers have no way to exercise the new path locally. Consider wiring a recipient flag through the CLI, or noting the gap in the PR.
General PR Review: Allow custom actions to return encrypted secret valuesBlocking Issues: 1 (carried over from the previous review, still unresolved) | Suggestions: 2 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness: the additive proto/wire changes, both generated Risk triage per Security IssuesNone found. Plaintext never reaches Correctness Issues
Suggestions
Prompt for AI agents |
4d95157 to
63531e4
Compare
| ) | ||
| if resultErr != nil { | ||
| result.response = nil | ||
| result.err = resultErr |
There was a problem hiding this comment.
🟡 Suggestion: result.err = resultErr replaces the handler's own error rather than wrapping it. prepareActionResult validates plaintext names/bytes even when encrypt is false (i.e. when the handler already failed), so a handler that returns both a real failure and a malformed PlaintextData surfaces only the SDK validation message — the actual cause is dropped along with result.response. Consider errors.Join(result.err, resultErr) or wrapping so the original failure stays visible in oa.Rv["error"].
| seen[plaintext.GetName()] = struct{}{} | ||
| } | ||
|
|
||
| if !encrypt { |
There was a problem hiding this comment.
🟡 Suggestion: a handler registered via RegisterWithSecrets that returns no PlaintextData and no error settles as COMPLETE with empty encrypted_data — including when the secret return_types field is is_required. That is the silent-failure shape C1 cannot distinguish from a successful credential issuance. plan.md lists "no plaintext" as a coverage cell, but actions_test.go has no case for a secret handler returning zero values; either assert the accepted behavior or reject a missing required secret return type here.
kans
left a comment
kans
left a comment
There was a problem hiding this comment.
I'd tackle the error handling comment before merging.
Connectors can now register ActionHandlerWithSecrets so plaintext return values never leave the connector process; the SDK encrypts them for the request's recipients before invoke and status responses. Co-authored-by: Cursor <cursoragent@cursor.com>
63531e4 to
c27aa04
Compare
| if !encrypt { | ||
| return nil, nil | ||
| } | ||
|
|
||
| for _, name := range handler.requiredSecretReturnNames { | ||
| if _, ok := seen[name]; !ok { | ||
| return nil, fmt.Errorf("required secret return type %q is missing", name) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 Suggestion: the required-secret gate closes the is_required cell, but the optional cell is still silent and untested. A handler registered via RegisterWithSecrets that returns no PlaintextData and no error for a non-required secret return type settles COMPLETE with empty encrypted_data — indistinguishable from a successful issuance by the caller. plan.md:43 lists "no plaintext" in the output coverage model and no test asserts that cell. Add a test in pkg/actions/actions_test.go covering success with an optional secret absent (asserting COMPLETE and empty encrypted_data), or reject the outcome as with required secrets.
False positive. We know that no connectors use is_secret in return schemas for custom actions.
2 participants
Summary
ActionHandlerWithSecretsso connectors can returnPlaintextDataseparately from the public actionStruct. The SDK encrypts those values for the request'sencryption_configsbefore they appear on invoke or status responses.ActionHandlerregistrations are unchanged. Secret-markedreturn_types(is_secret) must use the new registration path; plaintext never sits inOutstandingActionor on the wire.NewActionInvokerWithEncryption.Verification plan and evidence:
docs/verification/encrypted-action-results/.Test plan
go test -count=1 ./pkg/actions ./pkg/connectorbuilder ./pkg/connectorrunner ./pkg/tasks/localgo test -count=1 -run '^TestActionInvokeTaskThreadsEncryptionConfigs$' ./pkg/tasks/c1apigo test -race -count=1 ./pkg/actions ./pkg/connectorbuilderbuf lintandbuf breaking --against '.git#branch=main'ActionHandlerWithSecretscan decrypt invoke and statusencrypted_datawith the matching private keyMade with Cursor