From 540c551f191691778e25bdcabce3efc3645bc7ad Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Wed, 26 Aug 2026 15:33:16 +0000 Subject: [PATCH] [management] Make the live rotation test prove what it claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps a review found in the live suite. The rotation test finished by renaming the provider and expecting that to succeed. A rename touches none of the fields the check looks at, so it is stored without asking the vendor anything — it would have passed just as well against a key the rotation had already replaced. It now moves the upstream by a trailing slash, which reaches the same host but differs as a string, so the check runs and the stored key is what has to satisfy it. And the refused-url test only asserted the error. An error is not the same fact as an absent record, so it now lists the providers and looks for the one that must not be there — which the refused-key test beside it already did. The discovery upstream override is logged. It sends the stored credential to a host the caller named, which the same permission set can already do by pointing the record there and letting the write check it — but that leaves an activity event behind, and this would otherwise leave nothing. --- .../credential_check_live_test.go | 19 +++++++++++++++++-- .../internals/modules/agentnetwork/manager.go | 17 ++++++++++++----- 2 files changed, 29 insertions(+), 7 deletions(-) diff --git a/e2e/agentnetwork/credential_check_live_test.go b/e2e/agentnetwork/credential_check_live_test.go index f859cb329..f674b578e 100644 --- a/e2e/agentnetwork/credential_check_live_test.go +++ b/e2e/agentnetwork/credential_check_live_test.go @@ -134,6 +134,14 @@ func TestLiveProviderUrlCheck(t *testing.T) { require.Equal(t, http.StatusUnprocessableEntity, apiErr.StatusCode) require.Contains(t, strings.ToLower(apiErr.Message), "could not be reached", "the message must name the url rather than the credential") + + // An error is not the same fact as an absent record: a handler that saved + // first and reported afterwards would satisfy everything above. + all, listErr := srv.ListProviders(ctx) + require.NoError(t, listErr) + for _, p := range all { + require.NotEqual(t, "e2e-cred-badurl", p.Name, "a refused provider must not be stored") + } } // TestLiveProviderUpdateKeepsTheWorkingKey is the state the check exists to @@ -164,8 +172,15 @@ func TestLiveProviderUpdateKeepsTheWorkingKey(t *testing.T) { // The stored key is never returned by the API, so the proof that it // survived is that an edit which reuses it still passes its check. A // replaced key would fail here exactly as the rotation just did. - renamed := credentialProviderRequest(tc, "e2e-cred-rotate-renamed", "") - updated, err := srv.UpdateProvider(ctx, prov.Id, renamed) + // + // The trailing slash is what makes that an actual check: an edit touching + // neither the url, the key nor the catalog entry is stored without asking + // the vendor anything, so a rename alone would pass whatever is on the + // record. Only the host is read out of the upstream, so the same vendor is + // reached — but the string differs, and the check runs. + recheck := credentialProviderRequest(tc, "e2e-cred-rotate-renamed", "") + recheck.UpstreamUrl = tc.upstream + "/" + updated, err := srv.UpdateProvider(ctx, prov.Id, recheck) require.NoError(t, err, "the working key must still be the stored one") require.Equal(t, "e2e-cred-rotate-renamed", updated.Name) } diff --git a/management/internals/modules/agentnetwork/manager.go b/management/internals/modules/agentnetwork/manager.go index 640ecf060..69aefc62f 100644 --- a/management/internals/modules/agentnetwork/manager.go +++ b/management/internals/modules/agentnetwork/manager.go @@ -230,13 +230,20 @@ func (m *managerImpl) DiscoverProviderModels(ctx context.Context, accountID, use // whichever vendor endpoint they picked. req.CatalogID = record.ProviderID req.APIKey = record.APIKey - // The upstream is the one field the caller may override, and only for - // entries that serve their listing from it. Those are the operator's - // own endpoints, reached with their own credential, so an unsaved URL - // is no more reachable here than a saved one — and the SSRF guard - // treats both alike. + // The upstream is the one field the caller may override, so that a URL + // typed into the form can be listed against before it is saved. + // + // It sends the stored credential to a host the caller named, which is + // a capability they already have: the same permission set updates the + // record's upstream, and that write runs this same check against + // whatever it is pointed at. What it would not otherwise be is silent, + // since the write leaves an activity event behind — so the override is + // recorded here. if strings.TrimSpace(req.UpstreamURL) == "" { req.UpstreamURL = record.UpstreamURL + } else if req.UpstreamURL != record.UpstreamURL { + log.WithContext(ctx).Infof("agent network provider %s listed against caller-supplied upstream %s by user %s", + recordID, req.UpstreamURL, userID) } }