Centralize and harden vault secret identifier validation by russell-stern · Pull Request #22119 · smartcontractkit/chainlink

russell-stern · 2026-04-21T20:26:48Z

Added secret identifier validation to all calls through the gateway
Centralized validator logic and removed duplicated logic from plugin
Added a smoke test to make sure the gateway and workflows are validating the secret identifier
Added a test helper to create ReportPlugin and updated all tests to use the new helper
Added tests to make sure owner specific overrides are applied in validation

github-actions · 2026-04-21T20:28:04Z

✅ No conflicts with other open PRs targeting develop

trunk-io · 2026-04-21T20:44:43Z

Failed Test	Failure Summary	Logs
`Test_CRE_V2_Aptos_Suite`	The test failed without a specific error message, indicating an unspecified failure during execution.	Logs ↗︎
`Test_CRE_V2_Aptos_Suite/[v2]_Aptos/Aptos_Write_Read_Roundtrip`	The test failed because it could not find the expected user log message during the Aptos write/read roundtrip test.	Logs ↗︎
`Test_CRE_V2_Aptos_Suite/[v2]_Aptos`		Logs ↗︎
`Test_CRE_V2_Aptos_Suite/[v2]_Aptos/Aptos_Write_Read_Roundtrip`		Logs ↗︎

... and 2 more

View Full Report ↗︎ ⋅ Docs

cedric-cordenier · 2026-04-22T13:08:17Z

+			},
+		}
+		fetcher := &callbackBlobFetcher{fn: func([]byte) error { return nil }}
+		result, err := r.broadcastBlobPayloads(t.Context(), fetcher, 1, payloads, ids)


🤔 This will never fail I think -- your callbackBlobFetcher isn't the real fetcher that would be injected, so in other words even if you passed a payload that was too big, the test would still succeed

I would suggest instead checking against the plugin limit value instantiated by the NewReportingPlugin function

I have that check below but you're right the runBroadcast isn't actually testing anything. I removed the call altogether and since the blob limit is per payload the batch size doesn't matter. I removed the loop that created multiple payloads and we're just checking against the max size payload for each request type

prashantkumar1982

Thanks for these new validation tests :)

But looking at these, I think we should do this validation in more layers in our stack. See detailed comments.

prashantkumar1982 · 2026-04-22T19:43:17Z

+		name            string
+		id              *vaultcommon.SecretIdentifier
+		maxIDLen        int
+		maxOwnerLen     int


These 3 limits on max values should ideally be tested inside the validator too.
OCR plugin is too deep in our stack to be the only place to validate these.
I mean look at this code:

chainlink/core/capabilities/vault/validator.go

Lines 59 to 61 in 01f4475

if req.Id.Key == "" || req.Id.Namespace == "" || req.Id.Owner == "" {

return errors.New("secret ID must have key, namespace and owner set at index " + strconv.Itoa(idx) + ":" + req.Id.String())

}

This code too should be checking these limits and failing.

@cedric-cordenier what do you think?

Yes agreed 👍

github-actions · 2026-04-24T01:12:58Z

I see you updated files related to core. Please run make gocs in the root directory to add a changeset as well as in the text include at least one of the following tags:

#added For any new functionality added.
#breaking_change For any functionality that requires manual action for the node to boot.
#bugfix For bug fixes.
#changed For any change to the existing functionality.
#db_update For any feature that introduces updates to database schema.
#deprecation_notice For any upcoming deprecation functionality.
#internal For changesets that need to be excluded from the final changelog.
#nops For any feature that is NOP facing and needs to be in the official Release Notes for the release.
#removed For any functionality/config that is removed.
#updated For any functionality that is updated.
#wip For any change that is not ready yet and external communication about it should be held off till it is feature complete.

jmank88 · 2026-04-28T21:20:51Z

+
+// ownerOverrideLimiter is a BoundLimiter that applies different size limits based on the owner in context.
+// This lets tests verify that the validator correctly threads the owner through context to the limiter.
+type ownerOverrideLimiter struct {


I'm not sure that faking this is a good idea. What do the tests actually evaluate if we skip past coverage of a real limiter? Typically we just pass a settings.Getter to the limits factory.

This is testing to make sure that we're passing in the owner into the context during the validation calls. There were places where we weren't and weren't applying higher limits for owners that had it set. This is only used in tests that are testing if we're applying the right limits per owner

prashantkumar1982 · 2026-04-30T07:26:07Z

 		return capabilities.CapabilityResponse{}, errors.New("unsupported method: can only call GetSecrets via capability interface")
 	}

 	r := &vaultcommon.GetSecretsRequest{}


I know we've been missing this code always.
But we should call validation on this request object too.
Reason is because this field is also coming from a workflow user, and can have invalid details.

@cedric-cordenier : We were always missing validation for GetSecrets() path whcih comes via workflows. The content is not validated anywhere before plugin.

cl-sonarqube-production · 2026-05-01T16:27:30Z

Quality Gate passed

Issues
2 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.5% Duplication on New Code

See analysis details on SonarQube

cedric-cordenier reviewed Apr 22, 2026

View reviewed changes

Comment thread core/services/ocr2/plugins/vault/plugin_test.go Outdated

cedric-cordenier reviewed Apr 22, 2026

View reviewed changes

Comment thread core/services/ocr2/plugins/vault/plugin_test.go Outdated

cedric-cordenier reviewed Apr 22, 2026

View reviewed changes

russell-stern force-pushed the priv-434-blob-broadcaster-property-test branch from b6f88a8 to 1bc64cc Compare April 22, 2026 17:03

cedric-cordenier previously approved these changes Apr 22, 2026

View reviewed changes

prashantkumar1982 requested changes Apr 22, 2026

View reviewed changes

russell-stern dismissed cedric-cordenier’s stale review via 2ea239f April 24, 2026 01:11

russell-stern force-pushed the priv-434-blob-broadcaster-property-test branch 3 times, most recently from e2ff881 to 98b873e Compare April 28, 2026 14:33

russell-stern changed the title ~~Added tests for vault blob broadcast size and batch request limit in …~~ Centralize and harden vault secret identifier validation Apr 28, 2026

russell-stern marked this pull request as ready for review April 28, 2026 18:50

russell-stern requested review from a team as code owners April 28, 2026 18:50

product-security-plaid-production Bot requested review from DylanTinianov and jmank88 April 28, 2026 18:50

jmank88 reviewed Apr 28, 2026

View reviewed changes

prashantkumar1982 reviewed Apr 30, 2026

View reviewed changes

russell-stern added 3 commits April 30, 2026 16:11

Added extra validation in gateway and validate observation

2821c5e

Fixed linting issue

176f819

Added validation for get secrets requests through execute

9e578d6

russell-stern force-pushed the priv-434-blob-broadcaster-property-test branch from 0f7644e to 9e578d6 Compare April 30, 2026 20:15

russell-stern requested a review from prashantkumar1982 April 30, 2026 20:17

Fixed linter errors

ba8b5a6

	if req.Id.Key == "" \|\| req.Id.Namespace == "" \|\| req.Id.Owner == "" {
	return errors.New("secret ID must have key, namespace and owner set at index " + strconv.Itoa(idx) + ":" + req.Id.String())
	}

Conversation

russell-stern commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trunk-io Bot commented Apr 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cedric-cordenier Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

russell-stern Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

prashantkumar1982 left a comment

Choose a reason for hiding this comment

Uh oh!

prashantkumar1982 Apr 22, 2026

Choose a reason for hiding this comment

Uh oh!

cedric-cordenier Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Apr 24, 2026

Uh oh!

jmank88 Apr 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

russell-stern Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

prashantkumar1982 Apr 30, 2026

Choose a reason for hiding this comment

Uh oh!

cl-sonarqube-production Bot commented May 1, 2026

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

russell-stern commented Apr 21, 2026 •

edited

Loading

github-actions Bot commented Apr 21, 2026 •

edited

Loading

trunk-io Bot commented Apr 21, 2026 •

edited

Loading

jmank88 Apr 28, 2026 •

edited

Loading