Add typed credential issuance inputs - #1120
c1-squire-dev[bot] wants to merge 35 commits into
Conversation
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if !ok || math.IsNaN(kind.NumberValue) || math.IsInf(kind.NumberValue, 0) || math.Trunc(kind.NumberValue) != kind.NumberValue || kind.NumberValue < -maxSafeJSONInteger || kind.NumberValue > maxSafeJSONInteger { | ||
| return fmt.Errorf("request data field %q must be an integer", name) | ||
| } | ||
| if err := field.ValidateIntRules(schemaField.GetIntField().GetRules(), int(kind.NumberValue), name); err != nil { |
There was a problem hiding this comment.
🟠 Bug: The pkg/field validators were written for config parsing, where a zero value means "unset" and rules are skipped unless validate_empty is set (pkg/field/validation.go:31, :141, :263). Reusing them for explicitly submitted request values means a caller can bypass every declared rule by sending the zero value: {"ttl_seconds": 0} passes Gte: 60, Lte: 3600, {"region": ""} passes the pattern rule, and a non-required string-slice field accepts [] despite min_items. Set validate_empty on the rules before delegating (or apply the comparisons directly here) so a present-but-zero value is validated rather than skipped; note this also makes presence semantics inconsistent, since present[name] counts "" as present for constraints while the rule validators treat it as absent. Confidence: high.
There was a problem hiding this comment.
Squire (openai/gpt-5.6-sol): Addressed in 23ce127. Request validation clones rules with validate_empty enabled, including repeated item rules, and covers zero integers plus empty strings, lists, and maps.
| google.protobuf.Timestamp expires_at = 5; | ||
| // Values for the selected descriptor's request_schema. The SDK validates | ||
| // these before invoking the connector implementation. | ||
| google.protobuf.Struct request_data = 6; |
There was a problem hiding this comment.
🟠 Bug: IssueCredentialRequest.request_data has no counterpart on Task.IssueCredentialTask (proto/c1/connectorapi/baton/v1/baton.proto:131), and pkg/tasks/c1api/issue_credential.go:41 — the only in-repo producer of IssueCredentialRequest — does not populate it. On the c1 task path typed inputs are therefore always dropped, so any connector that declares a request_schema field with is_required: true will fail every hosted issuance with request data field "x" is required. Add request_data to IssueCredentialTask and forward t.GetRequestData() in the handler; if that is intentionally deferred, say so in the PR so connectors do not declare required typed fields yet. Confidence: high.
| } | ||
| } | ||
| present := make(map[string]bool, len(values)) | ||
| for name, schemaField := range fields { |
There was a problem hiding this comment.
🟡 Suggestion: This iterates the fields map, so when a request violates more than one field the returned error is chosen by Go's randomized map order. That makes the host-visible InvalidArgument message vary between identical calls and weakens the plan's O1 "stable error message" oracle. Iterating schema.GetFields() in declaration order (looking values up by name) makes the first reported failure deterministic. Confidence: high.
There was a problem hiding this comment.
Squire (openai/gpt-5.6-sol): Addressed in 23ce127. Known fields now validate in schema declaration order; unknown names are sorted before reporting.
| if !ok { | ||
| return fmt.Errorf("request data field %q must be a string", name) | ||
| } | ||
| if err := field.ValidateStringRules(schemaField.GetStringField().GetRules(), kind.StringValue, name); err != nil { |
There was a problem hiding this comment.
🟡 Suggestion: field.ValidateStringRules embeds the rejected value in its message (value must match pattern %s but got '%s', and likewise for in/not_in/eq), and that error is returned to the host as InvalidArgument in credentials.go:218 and recorded via RecordTaskFailure. c1.config.v1.Field carries is_secret, so a connector declaring a secret typed input would have submitted secret material echoed into gRPC errors and task-failure records. Consider redacting the value for fields with is_secret set (or omitting values from request-data validation errors entirely). Confidence: medium.
There was a problem hiding this comment.
Squire (openai/gpt-5.6-sol): Addressed in 23ce127. Credential request schemas now reject is_secret fields at publication/validation, and the proto documents that request_data is observable host task data.
General PR Review: Add typed credential issuance inputsBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryThe new commits repin Risk triage per Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| } | ||
| continue | ||
| } | ||
| present[name] = true |
There was a problem hiding this comment.
🟡 Suggestion: presence and emptiness use different notions here, so constraints can be satisfied by values that requiredness rejects. present[name] is set for any non-null key, while line 288 treats ""/[]/{} as absent for is_required. Concretely: fields a,b with no rules and AT_LEAST_ONE(a,b) — submitting {"a": ""} passes the constraint while giving the connector no usable value; conversely a host that always serializes zero values (global: false) trips MUTUALLY_EXCLUSIVE spuriously (that's exactly the "constraint" table case at credential_issue_request_data_test.go:144). Consider computing present[name] as !credentialIssueRequestValueIsEmpty(value) so both checks agree, and add table cases for empty-value-vs-constraint. (confidence: high on the behavior, medium on which semantics you want)
There was a problem hiding this comment.
Squire (openai/gpt-5.6-sol): Addressed in a9265e0. Constraint presence now uses the same empty-value predicate as requiredness, with empty string/list/map regression coverage.
| if constraint == nil { | ||
| return fmt.Errorf("request schema constraint is required") | ||
| } | ||
| if constraint.GetKind() == config.ConstraintKind_CONSTRAINT_KIND_UNSPECIFIED { |
There was a problem hiding this comment.
🟡 Suggestion: schema validation only rejects CONSTRAINT_KIND_UNSPECIFIED, but validateCredentialIssueConstraint has a default: that returns "unknown request schema constraint kind". A schema carrying a kind this build doesn't know (a newer connector's descriptor validated by an older host via the exported ValidateCredentialIssueRequestSchema) therefore passes publication and then fails every issuance request instead of failing fast. Reject unrecognized ConstraintKind values here so the failure surfaces at capability validation rather than per-request. (confidence: high)
There was a problem hiding this comment.
Squire (openai/gpt-5.6-sol): Addressed in a9265e0. Schema validation now allow-lists every supported constraint kind and rejects unknown enum values before request validation.
| minInt, maxInt := -maxSafeJSONInteger, maxSafeJSONInteger | ||
| if strconv.IntSize == 32 { | ||
| minInt, maxInt = float64(-1<<31), float64(1<<31-1) | ||
| } | ||
| if !ok || math.IsNaN(kind.NumberValue) || math.IsInf(kind.NumberValue, 0) || math.Trunc(kind.NumberValue) != kind.NumberValue || kind.NumberValue < -maxSafeJSONInteger || kind.NumberValue > maxSafeJSONInteger || kind.NumberValue < minInt || kind.NumberValue > maxInt { | ||
| return fmt.Errorf("request data field %q must be an integer", name) | ||
| } | ||
| rules := cloneIntRulesForRequest(schemaField.GetIntField().GetRules()) | ||
| if err := field.ValidateIntRules(rules, int(kind.NumberValue), name); err != nil { |
There was a problem hiding this comment.
🟡 Suggestion: the 32-bit clamp removes the implementation-defined conversion, but two things remain. (1) The accepted range is now architecture-dependent in a validator whose doc comment says host and connector share it "so host and connector validation cannot drift" — ttl_seconds: 3e9 is accepted on amd64 and rejected on a 32-bit build of the same schema. (2) The rejection message is wrong for that case: 3e9 is an integer, it's just outside the platform int. Prefer a distinct message (e.g. must be within the supported integer range) and, if you want arch-independence, range-check against ±(2^53-1) only and pass an int64 down. Relatedly, ValidateCredentialIssueRequestSchema (line 183) doesn't reject Int64Rules bounds outside ±(2^53-1), so Gte: 1<<60 publishes a silently unsatisfiable field. (confidence: high)
There was a problem hiding this comment.
Squire (openai/gpt-5.6-sol): Addressed in a9265e0. Validation stays architecture-independent by evaluating int64 directly, reports out-of-range values distinctly, and rejects schema rule bounds outside the JSON-safe range.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| func validateCredentialIssueIntRules(rules *config.Int64Rules, value int64, name string) error { | ||
| if rules == nil { | ||
| return nil | ||
| } | ||
| if rules.GetIsRequired() && value == 0 { | ||
| return fmt.Errorf("request data field %q is required", name) | ||
| } | ||
| if rules.HasEq() && value != rules.GetEq() { | ||
| return fmt.Errorf("request data field %q must equal %d", name, rules.GetEq()) | ||
| } | ||
| if rules.HasLt() && value >= rules.GetLt() { | ||
| return fmt.Errorf("request data field %q must be less than %d", name, rules.GetLt()) | ||
| } | ||
| if rules.HasLte() && value > rules.GetLte() { | ||
| return fmt.Errorf("request data field %q must be less than or equal to %d", name, rules.GetLte()) | ||
| } | ||
| if rules.HasGt() && value <= rules.GetGt() { | ||
| return fmt.Errorf("request data field %q must be greater than %d", name, rules.GetGt()) | ||
| } | ||
| if rules.HasGte() && value < rules.GetGte() { | ||
| return fmt.Errorf("request data field %q must be greater than or equal to %d", name, rules.GetGte()) | ||
| } | ||
| if len(rules.GetIn()) > 0 && !slices.Contains(rules.GetIn(), value) { | ||
| return fmt.Errorf("request data field %q must be one of %v", name, rules.GetIn()) | ||
| } | ||
| if slices.Contains(rules.GetNotIn(), value) { | ||
| return fmt.Errorf("request data field %q contains a disallowed value", name) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (high confidence): this hand-rolls the Int64Rules evaluation that field.ValidateIntRules already implements, while the string/bool/slice/map arms of validateCredentialIssueRequestValue still delegate to pkg/field. The file's own doc comment says host and connector validation "cannot drift", but a rule added to Int64Rules later will be enforced for config fields and silently ignored here. Error wording also now diverges (request data field "ttl" must be greater than or equal to 60 vs. the string arm's field ttl: value must match pattern ...). Consider adding an int64-taking variant in pkg/field (the only reason to fork was ValidateIntRules' platform-dependent int parameter) and calling it from both places.
There was a problem hiding this comment.
Squire (openai/gpt-5.6-sol): Addressed in 04e19b5. pkg/field now exposes ValidateInt64Rules; ValidateIntRules delegates to it, and credential request validation uses the shared int64 implementation.
| } | ||
| continue | ||
| } | ||
| present[name] = !credentialIssueRequestValueIsEmpty(value) |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): the presence rule is now type-asymmetric. credentialIssueRequestValueIsEmpty only treats empty string/list/object as absent, so an explicitly-submitted 0 or false still counts as present for constraints — while validateCredentialIssueIntRules (line 408) treats 0 as the absent/zero value for Int64Rules.is_required. So {"ttl_seconds": 0, "permanent": true} trips MUTUALLY_EXCLUSIVE, and AT_LEAST_ONE is satisfied by 0/false but not by "". Either fold number-0/bool-false into the same rule or document that presence is "explicitly submitted, non-empty for collections/strings only" — the host has to reimplement this rule to narrow schemas consistently, so it should be stated rather than inferred.
There was a problem hiding this comment.
Squire (openai/gpt-5.6-sol): Addressed in 04e19b5 by documenting the intentional config-field semantics at the presence calculation: empty strings/collections are absent, while explicitly submitted 0 and false are present. Int64Rules.is_required retains its established zero-value behavior.
|
|
||
| t.Run("rejects integer rules outside the JSON-safe range", func(t *testing.T) { | ||
| schema := v2.CredentialIssueRequestSchema_builder{Fields: []*config.Field{ | ||
| config.Field_builder{Name: "ttl", IntField: config.IntField_builder{Rules: config.Int64Rules_builder{ | ||
| Gte: proto.Int64(1 << 60), | ||
| }.Build()}.Build()}.Build(), | ||
| }}.Build() | ||
| require.ErrorContains(t, ValidateCredentialIssueRequestSchema(schema), "outside the supported JSON integer range") |
There was a problem hiding this comment.
🟡 Suggestion (high confidence): the new int validator is only exercised through Gte. Nothing covers validateCredentialIssueIntRules' eq/lt/lte/gt/in/not_in/is_required branches, validateCredentialIssueIntRuleBounds' in/not_in loop, or the new request-path message must be within the supported integer range (e.g. ttl_seconds: 1e18). Since this replaced the shared field.ValidateIntRules with a fresh implementation, a table over {rule kind} × {below, boundary, above} plus one out-of-range data case would pin it; TestValidateCredentialIssueRequestConstraintTreatsEmptyValuesAsAbsent similarly covers 3 of 5 field kinds and could add number/bool rows.
There was a problem hiding this comment.
Squire (openai/gpt-5.6-sol): Addressed in 04e19b5. The shared pkg/field suite already covers every Int64Rules branch; I added direct int64-width coverage, request-path JSON-range coverage, and schema-bound coverage for scalar, in, and not_in rules.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| value, ok := values[name] | ||
| _, isNull := value.GetKind().(*structpb.Value_NullValue) | ||
| if !ok || value == nil || value.GetKind() == nil || isNull { | ||
| if schemaField.GetIsRequired() { | ||
| return fmt.Errorf("request data field %q is required", name) | ||
| } | ||
| continue | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (medium confidence): rules-level is_required is only reachable when the key is present. An absent/null field continues here after checking Field.is_required, so validateCredentialIssueRequestValue — and with it Int64Rules.is_required / StringRules.is_required / StringMapRules.is_required — never runs. A connector declaring IntField{Rules:{IsRequired:true}} without Field.is_required gets enforcement for {"ttl":0} but silently none when ttl is omitted, and ValidateCredentialIssueRequestSchema does not warn about the mismatch. Either treat a rules-level is_required as implying field requiredness, or reject the combination at schema validation so the trap surfaces at capability publication.
There was a problem hiding this comment.
Squire (openai/gpt-5.6-sol): Addressed by treating rules-level is_required as field requiredness for every input type whose rules expose it. Missing/null and present-but-empty checks now use the combined requirement, with a regression test for Int64Rules.is_required without Field.is_required.
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Secondary field names are only read by DEPENDENT_ON, but schema publication accepted them on every constraint kind, silently ignoring them at request evaluation. Reject nonempty secondary_field_names unless the kind is DEPENDENT_ON, and document the restriction on CredentialIssueRequestSchema. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Reject DEPENDENT_ON constraints whose field_names and secondary_field_names overlap, matching the public FieldsDependentOn DSL: a self-satisfied dependency is a malformed schema, and the request evaluator would otherwise count one submitted field on both sides. Reject schemas whose unconditionally required string and string-list fields provably exceed the 65536-byte request-data cap at publication. The bound is a conservative saturating protobuf lower bound (cap+1 sentinel, clamped uint64 rules, full per-element Value framing, required-list floor of max(1, min_items)) computed from declared Len/MinLen/MinItems so every satisfying request would previously fail request validation anyway. Optional fields contribute zero and cross-constraint branches are not summed; no solver. Pin the existing MaxUint64 pre-conversion size guard with direct regression cases for 65537 and math.MaxUint64, scalar and list item. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Append CO-4 (DEPENDENT_ON lists must be disjoint) and CO-5 (required-field aggregate feasibility at publication) to the frozen verification plan, and refresh the evidence table and command log to revision 8232332 with the actual gate results from this run. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Add the reviewer's remaining regressions: the required-list MinItems=0 floor keeps a computed bound matching the [""] minimal fixture exactly, and a cap-adjacent 64x1020-byte list (wiring 65679 bytes) proves the aggregate check rejects a schema the pre-fix framing arithmetic admitted. Drop the version-fragile //nolint:gosec directive: golangci-lint 2.12.2 (the CI pin) does not flag the bounded conversion, so the directive tripped nolintlint as unused in CI. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The MaxUint64 schema subtest exits in the pre-existing per-field rule-bound guard, so it proves publication rejection but not the new saturating arithmetic. Add TestCredentialIssueFieldMinSizeSaturates, which calls credentialIssueClampLength, credentialIssueMinStringLength, and credentialIssueRequestFieldMinSize directly with MaxUint64 rules and asserts the cap+1 sentinel, and relabel the schema subtest to say what it actually exercises. Move the 64x1020 cap-adjacent list into TestCredentialIssueSchemaListAggregateBoundary, which owns the 64-item cap crossing, keeping the MinItems-zero-floor test about the zero floor alone. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The validator caps MinItems at 64 before the list lower-bound helper runs, so credentialIssueSaturatingMul's huge-operand guards are unreachable through schema validation. Pin them with a direct table: MaxUint64 item count, MaxUint64 item bytes, both combined, and a count just above the sentinel, each asserting the cap+1 result, plus a finite case confirming the guards do not corrupt below-cap sizes. No giant lists or strings are allocated. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
The empty-item per-element contribution is tag1 + SizeBytes(tag3 + SizeBytes(0)) = 4 bytes, so the first saturating MinItems count is 16381 (16380 still fits once entry framing is counted), not the 32769 previously claimed. Pin both sides of the real boundary so the division-bound guard's trip point is exact. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| // The division-bound guard specifically: with empty items each | ||
| // element contributes tag1 + SizeBytes(tag3 + SizeBytes(0)) = 4, so | ||
| // the first saturating count is 16381 (16380 items still fit: the | ||
| // 16380x4 payload plus entry framing stays under the sentinel). This | ||
| // count saturates via b > limit/a rather than the b >= limit arm. | ||
| config.RepeatedStringRules_builder{MinItems: proto.Uint64(16381)}.Build(), |
There was a problem hiding this comment.
🟡 Suggestion: the per-item contribution of 4 is now right, but 16381 still does not reach the b > limit/a arm. With a = 4 and limit = credentialIssueRequestSizeLimit = 65537, limit/a is 16384, so the division guard first fires at 16385; credentialIssueSaturatingMul(4, 16381) returns 65524 unsaturated and the sentinel comes from the outer credentialIssueSaturatingAdd in credentialIssueStructEntryMinSize. The division-bound guard the comment says it pins is still uncovered — 16385 exercises it.
| // The division-bound guard specifically: with empty items each | |
| // element contributes tag1 + SizeBytes(tag3 + SizeBytes(0)) = 4, so | |
| // the first saturating count is 16381 (16380 items still fit: the | |
| // 16380x4 payload plus entry framing stays under the sentinel). This | |
| // count saturates via b > limit/a rather than the b >= limit arm. | |
| config.RepeatedStringRules_builder{MinItems: proto.Uint64(16381)}.Build(), | |
| // The division-bound guard specifically: with empty items each | |
| // element contributes tag1 + SizeBytes(tag3 + SizeBytes(0)) = 4, so | |
| // limit/a is 16384 and 16385 is the first count that saturates via | |
| // the b > limit/a arm rather than b >= limit. (Counts from 16381 up | |
| // also saturate, but only later, in the entry-framing add.) | |
| config.RepeatedStringRules_builder{MinItems: proto.Uint64(16385)}.Build(), |
1 participant
Squire (openai/gpt-5.6-sol):
Summary
This is the Baton SDK half of IGA-4093.
Verification
GOTOOLCHAIN=go1.25.2 go test -race ./pkg/field ./pkg/connectorbuilder ./pkg/tasks/c1apiGOTOOLCHAIN=go1.25.2 go test ./...(exit 0, no failing packages)GOTOOLCHAIN=go1.25.2 golangci-lint run --timeout=10m(0 issues)buf lintbuf breaking --against '.git#tag=v0.26.0'make protogen(clean tree afterward)The compatibility harness commands still fail under the default Go 1.27
toolchain: they recursively build the v0.26.0 baseline, whose
cockroachdb/swissdependency excludes Go 1.27.make lintunder thedefault toolchain hits the same class of limit (the installed
golangci-lint 2.9.0 binary is built with Go 1.26 and panics on Go 1.27
export data). Both reproduce without these changes; the equivalent full
suite and full-repo lint above pass under the matching GOTOOLCHAIN.