diff --git a/cmd/oap/internal/chatcmd/sessionviewlink.go b/cmd/oap/internal/chatcmd/sessionviewlink.go index 7d3400f1..0949def4 100644 --- a/cmd/oap/internal/chatcmd/sessionviewlink.go +++ b/cmd/oap/internal/chatcmd/sessionviewlink.go @@ -38,7 +38,7 @@ type tuiSessionViewMinter struct { webdBaseURL string } -func (m *tuiSessionViewMinter) MintSessionViewLink(sessionRef string, _ identity.Principal, _ string) (string, error) { +func (m *tuiSessionViewMinter) MintSessionViewLink(sessionRef string, _ identity.Principal) (string, error) { return channelkinds.ComposeSessionViewURL(m.webdBaseURL, sessionRef) } diff --git a/internal/cmd/channelsd/artifact_view_minter_test.go b/internal/cmd/channelsd/artifact_view_minter_test.go index 21d0b13c..322fbca9 100644 --- a/internal/cmd/channelsd/artifact_view_minter_test.go +++ b/internal/cmd/channelsd/artifact_view_minter_test.go @@ -38,9 +38,8 @@ func TestNewArtifactViewMinter_HappyPath(t *testing.T) { artifactID := "art-abc123" sessionRef := "default/sess-1" subject := identity.RawSubject("user:alice") - backLink := "https://example.slack.com/archives/C1/p170?thread_ts=170&cid=C1" - u, err := m.MintArtifactViewLink(artifactID, sessionRef, subject, backLink) + u, err := m.MintArtifactViewLink(artifactID, sessionRef, subject) require.NoError(t, err, "MintArtifactViewLink should succeed") assert.True(t, strings.HasPrefix(u, base+"/artifact-view?"), "URL must start with base+/artifact-view?") assert.Contains(t, u, "d=", "URL must contain d= query param") @@ -57,7 +56,6 @@ func TestNewArtifactViewMinter_HappyPath(t *testing.T) { require.NoError(t, err, "Verify with aud=webd must succeed — audience-override regression guard") assert.Equal(t, artifactID, p.ArtifactID) assert.Equal(t, passthroughlink.AudienceWebd, p.Audience) - assert.Equal(t, backLink, p.BackLink) } // TestNewArtifactViewMinter_EmptyBaseURL verifies that a missing webd base URL @@ -66,7 +64,7 @@ func TestNewArtifactViewMinter_EmptyBaseURL(t *testing.T) { signer := passthroughlink.New(make([]byte, 32)) m := newArtifactViewMinter(signer, func() string { return "" }) - u, err := m.MintArtifactViewLink("art-1", "ns/sess", identity.RawSubject(""), "") + u, err := m.MintArtifactViewLink("art-1", "ns/sess", identity.RawSubject("")) require.NoError(t, err, "empty base URL must return nil error (clean skip)") assert.Empty(t, u, "empty base URL must return empty URL string") } diff --git a/internal/cmd/channelsd/minter_wiring_race_test.go b/internal/cmd/channelsd/minter_wiring_race_test.go index 9c89092b..ba7a1b4e 100644 --- a/internal/cmd/channelsd/minter_wiring_race_test.go +++ b/internal/cmd/channelsd/minter_wiring_race_test.go @@ -30,7 +30,7 @@ import ( type stubArtifactViewMinter struct{ id string } func (s *stubArtifactViewMinter) MintArtifactViewLink( - _, _ string, _ identity.Principal, _ string, + _, _ string, _ identity.Principal, ) (string, error) { return "https://webd.example/artifact-view?stub=" + s.id, nil } diff --git a/internal/cmd/channelsd/session_view_minter.go b/internal/cmd/channelsd/session_view_minter.go index 9ce11021..02c14c58 100644 --- a/internal/cmd/channelsd/session_view_minter.go +++ b/internal/cmd/channelsd/session_view_minter.go @@ -36,14 +36,14 @@ func newSessionViewMinter(webdBaseURL func() string) *sessionViewMinter { } // MintSessionViewLink returns "/session-view//" for -// the given sessionRef ("ns/name"). subject and backLink are accepted for -// interface parity with channelkinds.SessionViewMinter (and -// ArtifactViewMinter) but are deliberately NOT embedded in the URL — the +// the given sessionRef ("ns/name"). subject is accepted for interface parity +// with channelkinds.SessionViewMinter (and ArtifactViewMinter) but is +// deliberately NOT embedded in the URL — the // session-view page is a durable plain-path link, not a signed capability; // CheckInteract at open time is the sole authorization boundary. Returns // ("", nil) when the webd base URL is empty (webd not yet installed or the // ConfigMap not yet populated) — callers treat an empty URL as a clean skip. -func (m *sessionViewMinter) MintSessionViewLink(sessionRef string, _ identity.Principal, _ string) (string, error) { +func (m *sessionViewMinter) MintSessionViewLink(sessionRef string, _ identity.Principal) (string, error) { if m == nil || m.WebdBaseURL == nil { return "", fmt.Errorf("session view minter not configured") } diff --git a/internal/cmd/channelsd/session_view_minter_test.go b/internal/cmd/channelsd/session_view_minter_test.go index f3220d31..02305f12 100644 --- a/internal/cmd/channelsd/session_view_minter_test.go +++ b/internal/cmd/channelsd/session_view_minter_test.go @@ -29,7 +29,7 @@ func TestNewSessionViewMinter_HappyPath(t *testing.T) { m := newSessionViewMinter(func() string { return "https://webd.example" }) subj := identity.RawSubject("user:alice") - u, err := m.MintSessionViewLink("ns1/sess1", subj, "") + u, err := m.MintSessionViewLink("ns1/sess1", subj) require.NoError(t, err, "MintSessionViewLink should succeed") assert.Equal(t, "https://webd.example/session-view/ns1/sess1", u, "URL must be a plain path with no ?d=&sig= — access is CheckInteract at open time, not a signed capability") @@ -39,7 +39,7 @@ func TestNewSessionViewMinter_HappyPath(t *testing.T) { // the base URL doesn't produce a double slash. func TestNewSessionViewMinter_TrimsTrailingSlash(t *testing.T) { m := newSessionViewMinter(func() string { return "https://webd.example/" }) - u, err := m.MintSessionViewLink("ns1/sess1", identity.Principal{}, "") + u, err := m.MintSessionViewLink("ns1/sess1", identity.Principal{}) require.NoError(t, err) assert.Equal(t, "https://webd.example/session-view/ns1/sess1", u) } @@ -48,7 +48,7 @@ func TestNewSessionViewMinter_TrimsTrailingSlash(t *testing.T) { // are URL-escaped independently. func TestNewSessionViewMinter_EscapesSegments(t *testing.T) { m := newSessionViewMinter(func() string { return "https://webd.example" }) - u, err := m.MintSessionViewLink("my ns/my sess", identity.Principal{}, "") + u, err := m.MintSessionViewLink("my ns/my sess", identity.Principal{}) require.NoError(t, err) assert.Equal(t, "https://webd.example/session-view/my%20ns/my%20sess", u) } @@ -57,7 +57,7 @@ func TestNewSessionViewMinter_EscapesSegments(t *testing.T) { // URL returns ("", nil) — a clean skip, not an error. func TestNewSessionViewMinter_EmptyBaseURL(t *testing.T) { m := newSessionViewMinter(func() string { return "" }) - u, err := m.MintSessionViewLink("ns1/sess1", identity.Principal{}, "") + u, err := m.MintSessionViewLink("ns1/sess1", identity.Principal{}) require.NoError(t, err, "empty base URL must return nil error (clean skip)") assert.Empty(t, u, "empty base URL must return empty URL string") } @@ -66,7 +66,7 @@ func TestNewSessionViewMinter_EmptyBaseURL(t *testing.T) { // sessionRef without a "/" is rejected rather than producing a malformed URL. func TestNewSessionViewMinter_MalformedSessionRef_ReturnsError(t *testing.T) { m := newSessionViewMinter(func() string { return "https://webd.example" }) - _, err := m.MintSessionViewLink("no-slash", identity.Principal{}, "") + _, err := m.MintSessionViewLink("no-slash", identity.Principal{}) require.Error(t, err, "sessionRef without ns/name must be rejected") assert.Contains(t, err.Error(), "sessionRef") } diff --git a/internal/cmd/channelsd/webhook_inbound.go b/internal/cmd/channelsd/webhook_inbound.go index 2eea8b40..aaf4e2d4 100644 --- a/internal/cmd/channelsd/webhook_inbound.go +++ b/internal/cmd/channelsd/webhook_inbound.go @@ -77,8 +77,14 @@ func webhookInboundHandler(ctx context.Context, cli client.Client, pl channelkin // the owner grant is an addition to the session, never a gate on it, so a // derivation failure logs and the delivery dispatches without one. var preTurn map[string]string + // external carries kind-specific routing/origin metadata onto the session's + // input binding (spec.inputChannel.external — the pipeline copies it). Today + // github's TriggerOriginURLProvider populates origin_url (the PR html_url) so + // webd's artifact-view page can link back to the pull request. + var external map[string]string if k, ok := chregistry.Get(ch.Spec.Kind); ok { - if op, ok := k.WebhookReceiver(channelkinds.Deps{}).(channelkinds.TriggerOwnerProvider); ok { + recv := k.WebhookReceiver(channelkinds.Deps{}) + if op, ok := recv.(channelkinds.TriggerOwnerProvider); ok { subject, has, oerr := op.TriggerOwnerSubject(&ch, p.DeliveryEvent, p.RawDelivery) switch { case oerr != nil: @@ -88,6 +94,16 @@ func webhookInboundHandler(ctx context.Context, cli client.Client, pl channelkin preTurn = map[string]string{spiceboxv1alpha1.AnnotationTriggerOwnerSubject: subject} } } + if op, ok := recv.(channelkinds.TriggerOriginURLProvider); ok { + u, has, oerr := op.TriggerOriginURL(&ch, p.DeliveryEvent, p.RawDelivery) + switch { + case oerr != nil: + logger.Info("webhook inbound: origin url derivation failed; session input binding will have no origin link", + "ns", p.ChannelNamespace, "name", p.ChannelName, "key", p.ChannelKey, "err", oerr.Error()) + case has: + external = map[string]string{"origin_url": u} + } + } } dec, err := pl.Deliver(ctx, channelkinds.InboundEvent{ Channel: &ch, @@ -97,6 +113,7 @@ func webhookInboundHandler(ctx context.Context, cli client.Client, pl channelkin RawDelivery: p.RawDelivery, DeliveryEvent: p.DeliveryEvent, PreTurnAnnotations: preTurn, + External: external, }) if err != nil { logger.Info("webhook inbound: pipeline.Deliver errored", diff --git a/internal/cmd/channelsd/webhook_inbound_origin_test.go b/internal/cmd/channelsd/webhook_inbound_origin_test.go new file mode 100644 index 00000000..ca18ff45 --- /dev/null +++ b/internal/cmd/channelsd/webhook_inbound_origin_test.go @@ -0,0 +1,77 @@ +package main + +import ( + "context" + "encoding/json" + "testing" + + "github.com/nats-io/nats.go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + "github.com/authzed/openagentprimitives/pkg/channels/channelkinds" +) + +// TestWebhookInboundHandler_DerivesOriginURL pins the handoff that lets the +// artifact-view page link back to the PR: the github receiver's +// TriggerOriginURLProvider derives the PR html_url from the verified delivery, +// and the handler places it on InboundEvent.External["origin_url"], which the +// pipeline copies onto the session's input binding. +func TestWebhookInboundHandler_DerivesOriginURL(t *testing.T) { + cli := fake.NewClientBuilder().WithScheme(attachScheme(t)).WithObjects(webhookTestChannel()).Build() + pipe := &fakeWebhookPipeline{dec: channelkinds.InboundDecision{Outcome: channelkinds.OutcomeRouted}} + logger, _ := captureLogger() + payload := webhookTestPayload() + payload.DeliveryEvent = "pull_request" + payload.RawDelivery = []byte(`{"action":"opened","number":42,` + + `"pull_request":{"html_url":"https://github.com/demo-org/platform/pull/42",` + + `"user":{"login":"demo-user","id":4172237}},` + + `"repository":{"full_name":"demo-org/platform"}}`) + raw, err := json.Marshal(payload) + require.NoError(t, err) + + webhookInboundHandler(context.Background(), cli, pipe, logger, &nats.Msg{Data: raw}) + + require.True(t, pipe.Called()) + assert.Equal(t, "https://github.com/demo-org/platform/pull/42", + pipe.observed().External["origin_url"], + "the PR html_url, derived from the verified delivery, must reach the session input binding") +} + +// A delivery the provider cannot derive an origin URL from still dispatches — +// the origin link is additive, never a gate — and an undecodable delivery is +// logged, not silent. +func TestWebhookInboundHandler_OriginURLDerivationFailure_StillDispatches(t *testing.T) { + cases := []struct { + name string + event string + rawDelivery []byte + wantLog string + }{ + {name: "non-pull_request event: no origin_url, no log", event: "ping", + rawDelivery: []byte(`{"zen":"x"}`)}, + {name: "undecodable delivery: no origin_url, logged", event: "pull_request", + rawDelivery: []byte("not json"), wantLog: "origin url derivation failed"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cli := fake.NewClientBuilder().WithScheme(attachScheme(t)).WithObjects(webhookTestChannel()).Build() + pipe := &fakeWebhookPipeline{dec: channelkinds.InboundDecision{Outcome: channelkinds.OutcomeRouted}} + logger, read := captureLogger() + payload := webhookTestPayload() + payload.DeliveryEvent = tc.event + payload.RawDelivery = tc.rawDelivery + raw, err := json.Marshal(payload) + require.NoError(t, err) + + webhookInboundHandler(context.Background(), cli, pipe, logger, &nats.Msg{Data: raw}) + + require.True(t, pipe.Called(), "the session must still start; the origin link is additive, not a gate") + assert.NotContains(t, pipe.observed().External, "origin_url") + if tc.wantLog != "" { + assert.Contains(t, read(), tc.wantLog) + } + }) + } +} diff --git a/internal/cmd/webd/main.go b/internal/cmd/webd/main.go index c04674ce..c9975971 100644 --- a/internal/cmd/webd/main.go +++ b/internal/cmd/webd/main.go @@ -786,26 +786,26 @@ var _ sessions.Deps = (*artifactViewDeps)(nil) // ArtifactID; the returned artifactID is always "" for this purpose. Any // other purpose (including empty) is rejected — the bearer capability is // fail-closed: a link that doesn't match a known purpose grants nothing. -func (d *artifactViewDeps) VerifyLink(raw string) (string, string, string, error) { +func (d *artifactViewDeps) VerifyLink(raw string) (string, string, error) { p, err := d.cookieSigner.Verify(raw, passthroughlink.WithExpectedIssuer(passthroughlink.IssuerChannelsd), passthroughlink.WithExpectedAudience(passthroughlink.AudienceWebd)) if err != nil { - return "", "", "", err + return "", "", err } switch p.Purpose { case passthroughlink.PurposeArtifactView: if p.ArtifactID == "" || p.SessionRef == "" { - return "", "", "", fmt.Errorf("link missing artifactId/sessionRef") + return "", "", fmt.Errorf("link missing artifactId/sessionRef") } - return p.ArtifactID, p.SessionRef, p.BackLink, nil + return p.ArtifactID, p.SessionRef, nil case passthroughlink.PurposeSessionView: if p.SessionRef == "" { - return "", "", "", fmt.Errorf("link missing sessionRef") + return "", "", fmt.Errorf("link missing sessionRef") } - return "", p.SessionRef, p.BackLink, nil + return "", p.SessionRef, nil default: - return "", "", "", fmt.Errorf("link purpose %q is not supported", p.Purpose) + return "", "", fmt.Errorf("link purpose %q is not supported", p.Purpose) } } @@ -1238,21 +1238,6 @@ func (d *artifactViewDeps) ArtifactMeta(ctx context.Context, ns, sess, artifactI return head.Name, head.Description, nil } -// ChannelKind returns the AgentSession's input-channel kind (e.g. "slack") so the -// live-view toolbar can pick the right thread-link icon. Returns "" (with nil -// error) when the session has no input channel (kubectl-driven sessions). On a -// Get error it returns ("", err) so the page can log and fall back. -func (d *artifactViewDeps) ChannelKind(ctx context.Context, ns, sess string) (string, error) { - var cur spiceboxv1alpha1.AgentSession - if err := d.K8s().Get(ctx, client.ObjectKey{Namespace: ns, Name: sess}, &cur); err != nil { - return "", err - } - if cur.Spec.InputChannel == nil { - return "", nil - } - return cur.Spec.InputChannel.Kind, nil -} - // SessionViews implements artifactview.Deps: returns the granted // session_views.interactions for the session's AgentClass, resolved by // agentcaps.ResolveSessionViews from spec — NEVER status, so a capability diff --git a/internal/cmd/webd/origin_links.go b/internal/cmd/webd/origin_links.go new file mode 100644 index 00000000..e9d90b5e --- /dev/null +++ b/internal/cmd/webd/origin_links.go @@ -0,0 +1,79 @@ +package main + +import ( + "context" + "net/url" + + "sigs.k8s.io/controller-runtime/pkg/client" + + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" + "github.com/authzed/openagentprimitives/pkg/channels/channelkinds" + channelregistry "github.com/authzed/openagentprimitives/pkg/channels/channelkinds/registry" + "github.com/authzed/openagentprimitives/pkg/web/webui/artifactview" +) + +// originLinksFrom walks the session's input then output channel bindings, +// dispatches each through its kind's OriginLinkProvider (the registry, never an +// `if kind == …` switch), and returns the resolved links de-duplicated by Href +// in input-first order. A kind implementing no OriginLinkProvider, or a binding +// with no resolvable origin yet (a Slack thread not rooted, a permalink that +// failed to resolve), contributes nothing. Pure function of the session so it is +// unit-testable without a cluster. +func originLinksFrom(sess *spiceboxv1alpha1.AgentSession) []artifactview.OriginLink { + var out []artifactview.OriginLink + seen := map[string]bool{} + add := func(b *spiceboxv1alpha1.ChannelBinding) { + if b == nil { + return + } + k, ok := channelregistry.Get(b.Kind) + if !ok { + return + } + p, ok := k.(channelkinds.OriginLinkProvider) + if !ok { + return + } + link, ok := p.OriginLink(b) + if !ok || link.URL == "" || seen[link.URL] { + return + } + // Generic backstop: these hrefs derive from the user-writable + // External map and are rendered as . A kind vouches for its + // own host (slack: *.slack.com), but every link must at least be an + // https URL — never a javascript:/data: scheme or a relative ref — + // before it reaches the toolbar. Fail-closed: drop anything that is + // not. + if !isHTTPSOriginHref(link.URL) { + return + } + seen[link.URL] = true + out = append(out, artifactview.OriginLink{Href: link.URL, Label: link.Label, Icon: link.Icon}) + } + add(sess.Spec.InputChannel) + add(sess.Spec.OutputChannel) + return out +} + +// isHTTPSOriginHref reports whether href is an https URL with a host — the +// scheme backstop every origin link must pass before it is rendered as an +// , independent of any per-kind host check. Rejects javascript:/data: +// schemes, plain http, and relative refs. +func isHTTPSOriginHref(href string) bool { + parsed, err := url.Parse(href) + return err == nil && parsed.Scheme == "https" && parsed.Host != "" +} + +// OriginLinks implements artifactview.Deps: it loads the session and derives its +// origin links. A Get failure logs and returns no links — never an error, the +// toolbar simply shows nothing. Best-effort by design: an origin link is a +// convenience affordance, never a gate, so no failure here is surfaced as one. +func (d *artifactViewDeps) OriginLinks(ctx context.Context, ns, sess string) []artifactview.OriginLink { + var cur spiceboxv1alpha1.AgentSession + if err := d.K8s().Get(ctx, client.ObjectKey{Namespace: ns, Name: sess}, &cur); err != nil { + d.logger.Error(err, "artifactview: OriginLinks session lookup failed; no origin links", + "ns", ns, "sess", sess) + return nil + } + return originLinksFrom(&cur) +} diff --git a/internal/cmd/webd/origin_links_test.go b/internal/cmd/webd/origin_links_test.go new file mode 100644 index 00000000..ad78007c --- /dev/null +++ b/internal/cmd/webd/origin_links_test.go @@ -0,0 +1,131 @@ +package main + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" + "github.com/authzed/openagentprimitives/pkg/web/webui/artifactview" +) + +func ghBinding(url string) *spiceboxv1alpha1.ChannelBinding { + b := &spiceboxv1alpha1.ChannelBinding{Name: "demo-reviewbot-gh", Kind: "github", Key: "pr:demo-org/platform#42"} + if url != "" { + b.External = map[string]string{"origin_url": url} + } + return b +} + +func slackBinding(permalink string) *spiceboxv1alpha1.ChannelBinding { + b := &spiceboxv1alpha1.ChannelBinding{Name: "demo-reviewbot-slack", Kind: "slack", + External: map[string]string{"channel_id": "C1", "thread_ts": "1700.1"}} + if permalink != "" { + b.External["permalink"] = permalink + } + return b +} + +// TestOriginLinksFrom exercises the walk over input+output bindings, the +// registry dispatch to each kind's OriginLinkProvider, and de-duplication — +// the registry is populated by webd's own blank imports (github, slack). +func TestOriginLinksFrom(t *testing.T) { + const prURL = "https://github.com/demo-org/platform/pull/42" + const slackURL = "https://demo.slack.com/archives/C1/p17001" + ghLink := func() artifactview.OriginLink { + return artifactview.OriginLink{Href: prURL, Label: "View the pull request", Icon: "github"} + } + slackLink := func() artifactview.OriginLink { + return artifactview.OriginLink{Href: slackURL, Label: "Open the Slack thread", Icon: "slack"} + } + + cases := []struct { + name string + in *spiceboxv1alpha1.ChannelBinding + out *spiceboxv1alpha1.ChannelBinding + want []artifactview.OriginLink + }{ + { + name: "reviewbot: github input + slack output, input first", + in: ghBinding(prURL), + out: slackBinding(slackURL), + want: []artifactview.OriginLink{ghLink(), slackLink()}, + }, + { + name: "interactive slack: input only, no output", + in: slackBinding(slackURL), + out: nil, + want: []artifactview.OriginLink{slackLink()}, + }, + { + name: "input==output same thread: de-duped to one link", + in: slackBinding(slackURL), + out: slackBinding(slackURL), + want: []artifactview.OriginLink{slackLink()}, + }, + { + name: "kubectl-driven: no bindings, empty", + in: nil, + out: nil, + want: nil, + }, + { + name: "input kind without an OriginLinkProvider (browser): contributes nothing", + in: &spiceboxv1alpha1.ChannelBinding{Name: "b", Kind: "browser"}, + out: nil, + want: nil, + }, + { + // Fail-closed: no github.com fallback (GHES host is unknowable from + // the binding). A session with no persisted origin_url contributes + // no link rather than a possibly-wrong github.com one. + name: "github without origin_url: omitted (no github.com fallback)", + in: ghBinding(""), + out: nil, + want: nil, + }, + { + name: "slack output without permalink and no team_id: omitted", + in: ghBinding(prURL), + out: slackBinding(""), + want: []artifactview.OriginLink{ghLink()}, + }, + { + // External is user-writable; a non-https href (even if a kind let it + // through) is dropped by the generic backstop in originLinksFrom. + name: "non-https origin href: dropped by the scheme backstop", + in: ghBinding("javascript:alert(1)"), + out: nil, + want: nil, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + sess := &spiceboxv1alpha1.AgentSession{} + sess.Spec.InputChannel = tc.in + sess.Spec.OutputChannel = tc.out + assert.Equal(t, tc.want, originLinksFrom(sess)) + }) + } +} + +func TestIsHTTPSOriginHref(t *testing.T) { + cases := []struct { + name string + href string + want bool + }{ + {"https with host", "https://github.com/x/y/pull/1", true}, + {"https slack", "https://demo.slack.com/archives/C1/p1", true}, + {"http not https", "http://github.com/x", false}, + {"javascript scheme", "javascript:alert(1)", false}, + {"data scheme", "data:text/html,", false}, + {"relative ref", "/x/y", false}, + {"empty", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, isHTTPSOriginHref(tc.href)) + }) + } +} diff --git a/internal/cmd/webd/session_view_minter.go b/internal/cmd/webd/session_view_minter.go index c588888f..d8f4911e 100644 --- a/internal/cmd/webd/session_view_minter.go +++ b/internal/cmd/webd/session_view_minter.go @@ -25,6 +25,6 @@ func newWebdSessionViewMinter(trustedURLGet func() string) *webdSessionViewMinte return &webdSessionViewMinter{trustedURLGet: trustedURLGet} } -func (m *webdSessionViewMinter) MintSessionViewLink(sessionRef string, _ identity.Principal, _ string) (string, error) { +func (m *webdSessionViewMinter) MintSessionViewLink(sessionRef string, _ identity.Principal) (string, error) { return channelkinds.ComposeSessionViewURL(m.trustedURLGet(), sessionRef) } diff --git a/internal/cmd/webd/verifylink_test.go b/internal/cmd/webd/verifylink_test.go index 714b01e4..d0ef2bd1 100644 --- a/internal/cmd/webd/verifylink_test.go +++ b/internal/cmd/webd/verifylink_test.go @@ -22,15 +22,13 @@ func TestArtifactViewDeps_VerifyLink_SessionView(t *testing.T) { d := &artifactViewDeps{cookieSigner: signer} m := &sessionviewlink.Minter{Signer: signer} - backLink := "https://example.slack.com/archives/C1/p170?thread_ts=170&cid=C1" - raw, err := m.Mint("default/sess-1", identity.RawSubject("user:alice"), backLink) + raw, err := m.Mint("default/sess-1", identity.RawSubject("user:alice")) require.NoError(t, err) - artifactID, sessionRef, gotBackLink, err := d.VerifyLink(raw) + artifactID, sessionRef, err := d.VerifyLink(raw) require.NoError(t, err, "a valid session_view link must verify") assert.Empty(t, artifactID, "a session_view link carries no ArtifactID") assert.Equal(t, "default/sess-1", sessionRef) - assert.Equal(t, backLink, gotBackLink) } // TestArtifactViewDeps_VerifyLink_SessionView_MissingSessionRefRejected proves @@ -50,7 +48,7 @@ func TestArtifactViewDeps_VerifyLink_SessionView_MissingSessionRefRejected(t *te }) require.NoError(t, err) - _, _, _, err = d.VerifyLink(raw) + _, _, err = d.VerifyLink(raw) require.Error(t, err, "a session_view link with no SessionRef must be rejected") } @@ -72,7 +70,7 @@ func TestArtifactViewDeps_VerifyLink_ArtifactViewStillRequiresBoth(t *testing.T) }) require.NoError(t, err) - _, _, _, err = d.VerifyLink(raw) + _, _, err = d.VerifyLink(raw) require.Error(t, err, "artifact_view link missing SessionRef must still be rejected") } @@ -92,6 +90,6 @@ func TestArtifactViewDeps_VerifyLink_UnknownPurposeRejected(t *testing.T) { }) require.NoError(t, err) - _, _, _, err = d.VerifyLink(raw) + _, _, err = d.VerifyLink(raw) require.Error(t, err, "an unrecognized link purpose must be rejected") } diff --git a/internal/cmd/webd/viewminter_test.go b/internal/cmd/webd/viewminter_test.go index df2e553d..3b071d5d 100644 --- a/internal/cmd/webd/viewminter_test.go +++ b/internal/cmd/webd/viewminter_test.go @@ -27,7 +27,7 @@ func TestNewArtifactViewMinter_MintsLinkWebdCanVerify(t *testing.T) { m := newArtifactViewMinter(key, func() string { return base }) require.NotNil(t, m, "a non-empty key must yield a minter") - link, err := m.MintArtifactViewLink("art-1", "default/sess-1", identity.RawSubject("user:alice"), "") + link, err := m.MintArtifactViewLink("art-1", "default/sess-1", identity.RawSubject("user:alice")) require.NoError(t, err) require.True(t, strings.HasPrefix(link, base+"/artifact-view?"), "link = %s", link) diff --git a/pkg/channels/channelkinds/agent_ui_url.go b/pkg/channels/channelkinds/agent_ui_url.go index a57392d8..1d0e25aa 100644 --- a/pkg/channels/channelkinds/agent_ui_url.go +++ b/pkg/channels/channelkinds/agent_ui_url.go @@ -45,8 +45,8 @@ func ComposeAgentUIURL(base, sessionRef string) (string, error) { // NO signed capability and NO TTL: the shell authorizes at open time, so the // link is durable and safe to bake into a message at post time. // -// It takes only the sessionRef — a subject and backLink would be embedded -// nowhere. Nil means webd is not configured for this process; senders decide +// It takes only the sessionRef — a subject would be embedded nowhere. Nil +// means webd is not configured for this process; senders decide // whether that is a quiet skip or a loud failure, per how central the offer is // to their surface. type AgentUIMinter interface { diff --git a/pkg/channels/channelkinds/browser/sender_live_view_offer.go b/pkg/channels/channelkinds/browser/sender_live_view_offer.go index d9a3d92a..213b6847 100644 --- a/pkg/channels/channelkinds/browser/sender_live_view_offer.go +++ b/pkg/channels/channelkinds/browser/sender_live_view_offer.go @@ -51,7 +51,7 @@ func (s *liveViewOfferSender) Send(ctx context.Context, sess channelkinds.Sessio } sessionRef := sess.Namespace + "/" + sess.Name - url, err := s.minter.MintArtifactViewLink(p.ArtifactID, sessionRef, s.principal, "") + url, err := s.minter.MintArtifactViewLink(p.ArtifactID, sessionRef, s.principal) if err != nil { logger.Info("browser live_view_offer: mint failed", "artifactID", p.ArtifactID, "err", err.Error()) s.sink.Emit(MsgSendError{Session: refOf(sess), Kind: env.Kind, Err: "mint artifact view link: " + err.Error(), At: timeNowUTC()}) diff --git a/pkg/channels/channelkinds/browser/sender_live_view_offer_test.go b/pkg/channels/channelkinds/browser/sender_live_view_offer_test.go index 52aaa074..33a834bb 100644 --- a/pkg/channels/channelkinds/browser/sender_live_view_offer_test.go +++ b/pkg/channels/channelkinds/browser/sender_live_view_offer_test.go @@ -18,7 +18,7 @@ type fakeMinter struct { err error } -func (f *fakeMinter) MintArtifactViewLink(artifactID, sessionRef string, subject identity.Principal, backLink string) (string, error) { +func (f *fakeMinter) MintArtifactViewLink(artifactID, sessionRef string, subject identity.Principal) (string, error) { return f.url, f.err } diff --git a/pkg/channels/channelkinds/browser/sender_session_view_offer.go b/pkg/channels/channelkinds/browser/sender_session_view_offer.go index 20050dbd..1fb8722b 100644 --- a/pkg/channels/channelkinds/browser/sender_session_view_offer.go +++ b/pkg/channels/channelkinds/browser/sender_session_view_offer.go @@ -53,7 +53,7 @@ func (s *sessionViewOfferSender) Send(ctx context.Context, sess channelkinds.Ses // No per-viewer identity is meaningful: the link carries no signed // capability, so s.principal has no bearing on the composed URL. Passed // anyway to keep this sender symmetric with liveViewOfferSender. - url, err := s.minter.MintSessionViewLink(p.SessionRef, s.principal, "") + url, err := s.minter.MintSessionViewLink(p.SessionRef, s.principal) if err != nil { logger.Info("browser session_view_offer: mint failed", "sessionRef", p.SessionRef, "err", err.Error()) s.sink.Emit(MsgSendError{Session: refOf(sess), Kind: env.Kind, Err: "mint session view link: " + err.Error(), At: timeNowUTC()}) diff --git a/pkg/channels/channelkinds/browser/sender_session_view_offer_test.go b/pkg/channels/channelkinds/browser/sender_session_view_offer_test.go index 263d3eb8..40a01e0e 100644 --- a/pkg/channels/channelkinds/browser/sender_session_view_offer_test.go +++ b/pkg/channels/channelkinds/browser/sender_session_view_offer_test.go @@ -19,7 +19,7 @@ type fakeSessionViewMinter struct { err error } -func (f *fakeSessionViewMinter) MintSessionViewLink(sessionRef string, subject identity.Principal, backLink string) (string, error) { +func (f *fakeSessionViewMinter) MintSessionViewLink(sessionRef string, subject identity.Principal) (string, error) { return f.url, f.err } diff --git a/pkg/channels/channelkinds/github/kind.go b/pkg/channels/channelkinds/github/kind.go index 047f7215..1938a82f 100644 --- a/pkg/channels/channelkinds/github/kind.go +++ b/pkg/channels/channelkinds/github/kind.go @@ -21,6 +21,7 @@ import ( "context" "errors" "fmt" + "net/url" "slices" "strings" @@ -159,6 +160,44 @@ func (Kind) FeatureSupport() map[channelfeatures.Feature]channelkinds.FeatureReq // output, composed by the tool that posts it, not by this kind). func (Kind) RenderMention(externalID string) string { return externalID } +var _ channelkinds.OriginLinkProvider = Kind{} + +// OriginLink returns a human-facing link to the pull request this binding +// correlates to, satisfying channelkinds.OriginLinkProvider. It uses only the +// host-correct html_url persisted at session creation (External["origin_url"], +// captured from the verified webhook payload — correct on a GitHub Enterprise +// Server host). ok=false when it is absent. +// +// There is deliberately NO github.com fallback: this kind supports GHES, where +// the host is customer-specific and unknowable from the binding alone (the +// GHES base URL lives on the RelationshipSource spec, not here). A reconstructed +// github.com///pull/ would leak internal org/repo names to a +// third party, and could point viewers at a public repo squatting the same +// path. A missing link is the safe degrade; new sessions always persist +// origin_url. External is user-writable, so the URL is returned only when it is +// a plausible https URL. +func (Kind) OriginLink(binding *spiceboxv1alpha1.ChannelBinding) (channelkinds.OriginLink, bool) { + if binding == nil { + return channelkinds.OriginLink{}, false + } + u := binding.External["origin_url"] + if !isHTTPSURL(u) { + return channelkinds.OriginLink{}, false + } + return channelkinds.OriginLink{URL: u, Label: "View the pull request", Icon: "github"}, true +} + +// isHTTPSURL reports whether u parses as an https URL with a host — the trust +// check for a user-writable persisted origin_url. GHES hosts are arbitrary and +// customer-specific, so this validates the scheme only, not the host. +func isHTTPSURL(u string) bool { + if u == "" { + return false + } + parsed, err := url.Parse(u) + return err == nil && parsed.Scheme == "https" && parsed.Host != "" +} + // SupportedMentionLookups returns nil — a pull request's participants have no // AP directory to look up against. func (Kind) SupportedMentionLookups() []channelkinds.MentionLookupKind { return nil } diff --git a/pkg/channels/channelkinds/github/origin_link_test.go b/pkg/channels/channelkinds/github/origin_link_test.go new file mode 100644 index 00000000..c3741f73 --- /dev/null +++ b/pkg/channels/channelkinds/github/origin_link_test.go @@ -0,0 +1,58 @@ +package github + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" + "github.com/authzed/openagentprimitives/pkg/channels/channelkinds" +) + +func TestKind_OriginLink(t *testing.T) { + k := Kind{} + cases := []struct { + name string + binding *spiceboxv1alpha1.ChannelBinding + wantLink channelkinds.OriginLink + wantOK bool + }{ + { + name: "persisted html_url wins (GHES host-correct)", + binding: &spiceboxv1alpha1.ChannelBinding{Kind: "github", Key: "pr:demo-org/platform#42", + External: map[string]string{"origin_url": "https://ghe.example.com/demo-org/platform/pull/42"}}, + wantLink: channelkinds.OriginLink{URL: "https://ghe.example.com/demo-org/platform/pull/42", + Label: "View the pull request", Icon: "github"}, + wantOK: true, + }, + { + // Fail-closed: there is no github.com fallback. On GHES a + // reconstructed github.com URL would leak internal org/repo names + // and could point at a squatted repo; the binding carries no host, + // so a missing link is the only safe answer. + name: "no persisted url: ok=false (no github.com fallback)", + binding: &spiceboxv1alpha1.ChannelBinding{Kind: "github", Key: "pr:demo-org/platform#42"}, + wantOK: false, + }, + { + name: "non-https origin_url: ok=false", + binding: &spiceboxv1alpha1.ChannelBinding{Kind: "github", + External: map[string]string{"origin_url": "javascript:alert(1)"}}, + wantOK: false, + }, + { + name: "http (not https) origin_url: ok=false", + binding: &spiceboxv1alpha1.ChannelBinding{Kind: "github", + External: map[string]string{"origin_url": "http://ghe.example.com/demo-org/platform/pull/42"}}, + wantOK: false, + }, + {name: "nil binding: ok=false", binding: nil, wantOK: false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, ok := k.OriginLink(tc.binding) + assert.Equal(t, tc.wantOK, ok) + assert.Equal(t, tc.wantLink, got) + }) + } +} diff --git a/pkg/channels/channelkinds/github/receiver.go b/pkg/channels/channelkinds/github/receiver.go index 3e322696..d613c0de 100644 --- a/pkg/channels/channelkinds/github/receiver.go +++ b/pkg/channels/channelkinds/github/receiver.go @@ -247,6 +247,30 @@ func (receiver) TriggerOwnerSubject(ch *spiceboxv1alpha1.Channel, event string, var _ channelkinds.TriggerOwnerProvider = receiver{} +// TriggerOriginURL returns the pull request's own html_url from a verified +// pull_request delivery, satisfying channelkinds.TriggerOriginURLProvider. The +// URL is host-correct — it is whatever host the delivery came from, so a GitHub +// Enterprise Server install yields its own host rather than github.com. ok=false +// (not an error) for a non-pull_request event or a payload that carries no +// html_url; the channelsd inbound handler then starts the session without the +// origin link. Re-decodes the same narrow prEvent, preserving its "a payload +// change upstream cannot alter behavior without a code change" property. +func (receiver) TriggerOriginURL(_ *spiceboxv1alpha1.Channel, event string, body []byte) (string, bool, error) { + if event != "pull_request" || len(body) == 0 { + return "", false, nil + } + var ev prEvent + if err := json.Unmarshal(body, &ev); err != nil { + return "", false, fmt.Errorf("github: decode pull_request payload for origin url: %w", err) + } + if ev.PullRequest.HTMLURL == "" { + return "", false, nil + } + return ev.PullRequest.HTMLURL, true, nil +} + +var _ channelkinds.TriggerOriginURLProvider = receiver{} + // TriggerSlotInstances names this pull request as the one instance a class's // slots may bind at session mint. // diff --git a/pkg/channels/channelkinds/github/receiver_origin_test.go b/pkg/channels/channelkinds/github/receiver_origin_test.go new file mode 100644 index 00000000..bf648210 --- /dev/null +++ b/pkg/channels/channelkinds/github/receiver_origin_test.go @@ -0,0 +1,46 @@ +package github + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestTriggerOriginURL_ReturnsPRHTMLURL pins that a verified pull_request +// delivery yields the PR's own html_url (host-correct — it is whatever host the +// delivery came from, so GitHub Enterprise is covered). Driven from the real +// payload fixture, same as the owner/facts tests, so a wrong json tag can't pass. +func TestTriggerOriginURL_ReturnsPRHTMLURL(t *testing.T) { + u, ok, err := receiver{}.TriggerOriginURL(testChannel(t), "pull_request", fixtureBody(t, "pull_request_opened.json")) + require.NoError(t, err) + require.True(t, ok) + assert.Equal(t, "https://github.com/demo-org/platform/pull/42", u) +} + +func TestTriggerOriginURL_NonPROrMissing(t *testing.T) { + cases := []struct { + name string + event string + body string + }{ + {name: "non-pull_request event: ok=false", event: "ping", body: `{"zen":"x"}`}, + {name: "empty body: ok=false, no decode error", event: "pull_request", body: ``}, + {name: "pull_request without html_url: ok=false", event: "pull_request", + body: `{"action":"opened","number":7,"pull_request":{},"repository":{"full_name":"demo-org/platform"}}`}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + u, ok, err := receiver{}.TriggerOriginURL(testChannel(t), tc.event, []byte(tc.body)) + require.NoError(t, err) + assert.False(t, ok) + assert.Empty(t, u) + }) + } +} + +func TestTriggerOriginURL_UndecodableBodyErrors(t *testing.T) { + _, ok, err := receiver{}.TriggerOriginURL(testChannel(t), "pull_request", []byte("not json")) + require.Error(t, err) + assert.False(t, ok) +} diff --git a/pkg/channels/channelkinds/kind.go b/pkg/channels/channelkinds/kind.go index f82b7fb3..ac5bb817 100644 --- a/pkg/channels/channelkinds/kind.go +++ b/pkg/channels/channelkinds/kind.go @@ -652,10 +652,10 @@ type PortalLinkMinter interface { type ArtifactViewMinter interface { // MintArtifactViewLink mints a webd artifact-view deep-link. The minter // derives Subject and SubjectVerified from subject, so the verified flag - // can only originate from the identity authority. backLink is optional and - // opaque — the web UI renders it as a "back to origin" link; pass "" when - // the kind has no permalink concept. - MintArtifactViewLink(artifactID, sessionRef string, subject identity.Principal, backLink string) (string, error) + // can only originate from the identity authority. The artifact-view page's + // "back to origin" links are derived from the session at view time, not + // carried in this link. + MintArtifactViewLink(artifactID, sessionRef string, subject identity.Principal) (string, error) } // SessionViewMinter composes "/session-view//" durable @@ -664,10 +664,9 @@ type ArtifactViewMinter interface { // authorization happens at open time via the page's own CheckInteract. type SessionViewMinter interface { // MintSessionViewLink returns the session-view page URL for sessionRef - // ("/"). subject and backLink are accepted for parity with - // ArtifactViewMinter but are NOT embedded in the URL — they have no - // bearing on who may open it. - MintSessionViewLink(sessionRef string, subject identity.Principal, backLink string) (string, error) + // ("/"). subject is accepted for parity with ArtifactViewMinter + // but is NOT embedded in the URL — it has no bearing on who may open it. + MintSessionViewLink(sessionRef string, subject identity.Principal) (string, error) } // AuthzReader exposes the read-only SpiceDB operations a channel-kind @@ -729,6 +728,36 @@ type SessionOwnerProvider interface { SessionOwner(ev InboundEvent) (subject string, provides bool) } +// OriginLink is one resolved "back to origin" link: the URL to open, the +// human-facing Label a view renders as its tooltip/accessible text, and an +// Icon key naming the glyph the view draws. Label and Icon keep the kind's +// own presentation choices inside the kind — the artifact-view UI renders +// them verbatim rather than branching on the channel kind (see +// pkg/web/webui/artifactview/ui). Icon is a presentation vocabulary, not the +// kind name: kinds sharing a glyph return the same key, and only a kind that +// needs a brand-new glyph obliges a new UI icon component. +type OriginLink struct { + URL string + Label string + Icon string +} + +// OriginLinkProvider is an OPTIONAL Kind capability — discovered by +// type-assertion, never a method on Kind — that turns one of a session's +// channel bindings into a human-facing link back to where the conversation +// lives: a Slack thread, a GitHub pull request. A kind with no such surface +// does not implement it and contributes no link. ok=false means "this binding +// has no resolvable origin yet" (a Slack thread not rooted, a permalink that +// failed to resolve) and the caller drops it silently. The kind is also the +// place any per-kind URL trust check lives (a Slack permalink must be a +// *.slack.com https URL) — the binding's External map is user-writable, so a +// kind returns ok=false rather than a link it cannot vouch for. Consumed by +// webd's artifact-view page to offer a "back to origin" link per distinct +// origin. +type OriginLinkProvider interface { + OriginLink(binding *spiceboxv1alpha1.ChannelBinding) (link OriginLink, ok bool) +} + // OwnerGroupProvider is an optional Kind capability: return the channel's live // membership subject-set ref (e.g. "slack_channel:#member") for use as an // owner source when this kind is the output channel, and whether one exists. diff --git a/pkg/channels/channelkinds/local/sender_live_view_offer.go b/pkg/channels/channelkinds/local/sender_live_view_offer.go index 6bc043d4..d7a1fdc5 100644 --- a/pkg/channels/channelkinds/local/sender_live_view_offer.go +++ b/pkg/channels/channelkinds/local/sender_live_view_offer.go @@ -51,7 +51,7 @@ func (s *liveViewOfferSender) Send(ctx context.Context, sess channelkinds.Sessio } sessionRef := sess.Namespace + "/" + sess.Name - url, err := s.minter.MintArtifactViewLink(p.ArtifactID, sessionRef, s.principal, "") + url, err := s.minter.MintArtifactViewLink(p.ArtifactID, sessionRef, s.principal) if err != nil { logger.Info("local live_view_offer: mint failed", "artifactID", p.ArtifactID, "err", err.Error()) s.sink.Emit(MsgSendError{Session: refOf(sess), Kind: env.Kind, Err: "mint artifact view link: " + err.Error(), At: timeNowUTC()}) diff --git a/pkg/channels/channelkinds/local/sender_live_view_offer_test.go b/pkg/channels/channelkinds/local/sender_live_view_offer_test.go index 267b0940..23f41f0b 100644 --- a/pkg/channels/channelkinds/local/sender_live_view_offer_test.go +++ b/pkg/channels/channelkinds/local/sender_live_view_offer_test.go @@ -18,7 +18,7 @@ type fakeMinter struct { err error } -func (f *fakeMinter) MintArtifactViewLink(artifactID, sessionRef string, subject identity.Principal, backLink string) (string, error) { +func (f *fakeMinter) MintArtifactViewLink(artifactID, sessionRef string, subject identity.Principal) (string, error) { return f.url, f.err } diff --git a/pkg/channels/channelkinds/local/sender_session_view_offer.go b/pkg/channels/channelkinds/local/sender_session_view_offer.go index 814bf3ce..fa572646 100644 --- a/pkg/channels/channelkinds/local/sender_session_view_offer.go +++ b/pkg/channels/channelkinds/local/sender_session_view_offer.go @@ -54,7 +54,7 @@ func (s *sessionViewOfferSender) Send(ctx context.Context, sess channelkinds.Ses // No per-viewer identity is meaningful: the link carries no signed // capability, so s.principal has no bearing on the composed URL. Passed // anyway to keep this sender symmetric with liveViewOfferSender. - url, err := s.minter.MintSessionViewLink(p.SessionRef, s.principal, "") + url, err := s.minter.MintSessionViewLink(p.SessionRef, s.principal) if err != nil { logger.Info("local session_view_offer: mint failed", "sessionRef", p.SessionRef, "err", err.Error()) s.sink.Emit(MsgSendError{Session: refOf(sess), Kind: env.Kind, Err: "mint session view link: " + err.Error(), At: timeNowUTC()}) diff --git a/pkg/channels/channelkinds/local/sender_session_view_offer_test.go b/pkg/channels/channelkinds/local/sender_session_view_offer_test.go index 88bce6f7..33c38051 100644 --- a/pkg/channels/channelkinds/local/sender_session_view_offer_test.go +++ b/pkg/channels/channelkinds/local/sender_session_view_offer_test.go @@ -19,7 +19,7 @@ type fakeSessionViewMinter struct { err error } -func (f *fakeSessionViewMinter) MintSessionViewLink(sessionRef string, subject identity.Principal, backLink string) (string, error) { +func (f *fakeSessionViewMinter) MintSessionViewLink(sessionRef string, subject identity.Principal) (string, error) { return f.url, f.err } diff --git a/pkg/channels/channelkinds/slack/kind.go b/pkg/channels/channelkinds/slack/kind.go index 290a647d..8fe70eca 100644 --- a/pkg/channels/channelkinds/slack/kind.go +++ b/pkg/channels/channelkinds/slack/kind.go @@ -15,7 +15,9 @@ import ( "context" "fmt" "net/http" + "net/url" "os" + "strings" "sync" "time" @@ -425,6 +427,59 @@ func (*Kind) RenderMention(externalID string) string { return "<@" + externalID + ">" } +var _ channelkinds.OriginLinkProvider = (*Kind)(nil) + +// OriginLink returns a link back to the Slack thread this binding addresses, +// satisfying channelkinds.OriginLinkProvider. It prefers the canonical permalink +// persisted onto the binding at first thread-root (External["permalink"], +// resolved by the sender via chat.getPermalink on a bot-rooted thread such as +// reviewbot's). When that is absent — the interactive case, where a human rooted +// the thread before the agent so no first-send permalink was captured — it falls +// back to reconstructing a thread deep-link from the team_id/channel_id/thread_ts +// the listener stamps at session creation. ok=false only when neither is +// available (no thread rooted yet). +// +// External is user-writable, so the persisted permalink is trusted only when it +// is an https *.slack.com URL — a value that is not (a javascript: URL, an +// attacker's host) is ignored and the reconstruction fallback is used instead. +// The reconstructed URL is safe by construction (BuildSlackThreadURL hardcodes +// app.slack.com). +func (*Kind) OriginLink(binding *spiceboxv1alpha1.ChannelBinding) (channelkinds.OriginLink, bool) { + if binding == nil { + return channelkinds.OriginLink{}, false + } + if u := binding.External["permalink"]; isSlackURL(u) { + return slackOriginLink(u), true + } + if u := BuildSlackThreadURL(binding.External["team_id"], binding.External["channel_id"], binding.External["thread_ts"]); u != "" { + return slackOriginLink(u), true + } + return channelkinds.OriginLink{}, false +} + +// slackOriginLink wraps a validated Slack URL with this kind's fixed +// presentation — the tooltip text and the "slack" glyph key the artifact-view +// UI maps to the Slack mark — so that choice lives here, not in the view. +func slackOriginLink(u string) channelkinds.OriginLink { + return channelkinds.OriginLink{URL: u, Label: "Open the Slack thread", Icon: "slack"} +} + +// isSlackURL reports whether u is an https URL whose host is slack.com or a +// subdomain of it — the trust check for a user-writable persisted permalink. +// Both the team permalink host (.slack.com) and the reconstructed deep +// link (app.slack.com) satisfy it. +func isSlackURL(u string) bool { + if u == "" { + return false + } + parsed, err := url.Parse(u) + if err != nil || parsed.Scheme != "https" { + return false + } + host := parsed.Hostname() + return host == "slack.com" || strings.HasSuffix(host, ".slack.com") +} + // SupportedMentionLookups returns the set of identifier shapes this // Slack kind can resolve. v1 supports all three (email, name, any). // The implementations are in pkg/channels/channelkinds/slack/lookup.go. diff --git a/pkg/channels/channelkinds/slack/listener.go b/pkg/channels/channelkinds/slack/listener.go index 1ced0360..507a320e 100644 --- a/pkg/channels/channelkinds/slack/listener.go +++ b/pkg/channels/channelkinds/slack/listener.go @@ -870,18 +870,6 @@ func (l *slackListener) handleLiveViewClick(ctx context.Context, cb slackapi.Int return true, nil } - // Best-effort thread back-link. A getPermalink failure must NOT block - // the view — log it and mint with an empty back-link. - backLink := "" - if v.T != "" { - if pl, err := sc.GetPermalinkContext(ctx, &slackapi.PermalinkParameters{Channel: channelID, Ts: v.T}); err != nil { - logger.Info("live_view click: getPermalink failed; back-link omitted", - "channel", channelID, "thread", v.T, "err", err.Error()) - } else { - backLink = pl - } - } - // Canonical SpiceDB subject for the clicker. This is NOT merely audit // metadata: on an OIDC-off install (no WebAuthenticator for the kind), // webd/identityd's trust-link fallback mints the session cookie from @@ -898,7 +886,7 @@ func (l *slackListener) handleLiveViewClick(ctx context.Context, cb slackapi.Int identity.RawExternalID(ext.ExternalID), identity.Email(ext.Email), ).AllowSynthetic() - url, err := minter.MintArtifactViewLink(v.A, v.S, subject, backLink) + url, err := minter.MintArtifactViewLink(v.A, v.S, subject) if err != nil { logger.Info("live_view click: mint failed", "artifactID", v.A, "session", v.S, "err", err.Error()) if perr := PostEphemeral(ctx, sc, channelID, cb.User.ID, @@ -929,7 +917,7 @@ func (l *slackListener) handleLiveViewClick(ctx context.Context, cb slackapi.Int return true, fmt.Errorf("live_view click: postEphemeral: %w", err) } logger.Info("live_view click: posted fresh open link", - "artifactID", v.A, "session", v.S, "clicker", cb.User.ID, "withBackLink", backLink != "") + "artifactID", v.A, "session", v.S, "clicker", cb.User.ID) return true, nil } return false, nil diff --git a/pkg/channels/channelkinds/slack/listener_live_view_test.go b/pkg/channels/channelkinds/slack/listener_live_view_test.go index 52a1abf8..c2de79e9 100644 --- a/pkg/channels/channelkinds/slack/listener_live_view_test.go +++ b/pkg/channels/channelkinds/slack/listener_live_view_test.go @@ -17,21 +17,21 @@ import ( // recordingMinter implements channelkinds.ArtifactViewMinter, recording the // args it was called with and returning canned results. type recordingMinter struct { - gotArtifact, gotSession, gotBackLink string - gotSubject identity.Subject - gotSubjectVerified bool - url string - err error - called bool + gotArtifact, gotSession string + gotSubject identity.Subject + gotSubjectVerified bool + url string + err error + called bool } -func (m *recordingMinter) MintArtifactViewLink(artifactID, sessionRef string, subject identity.Principal, backLink string) (string, error) { +func (m *recordingMinter) MintArtifactViewLink(artifactID, sessionRef string, subject identity.Principal) (string, error) { m.called = true subj, err := subject.Subject() if err != nil { return "", err } - m.gotArtifact, m.gotSession, m.gotSubject, m.gotBackLink = artifactID, sessionRef, subj, backLink + m.gotArtifact, m.gotSession, m.gotSubject = artifactID, sessionRef, subj m.gotSubjectVerified = subject.EmailVerified() return m.url, m.err } @@ -93,53 +93,32 @@ func TestHandleLiveViewClick(t *testing.T) { value liveViewButtonValue minter *recordingMinter nilMinter bool - permalink string - permErr error postEphemeralErr error check func(t *testing.T, l *slackListener, c *fakeSlackClient, m *recordingMinter, handled bool, err error) }{ { - name: "happy path: mints fresh, posts ephemeral Open button with back-link", - value: goodValue(), - minter: &recordingMinter{url: freshURL}, - permalink: "https://example.slack.com/archives/C01/p170?thread_ts=170&cid=C01", + name: "happy path: mints fresh, posts ephemeral Open button", + value: goodValue(), + minter: &recordingMinter{url: freshURL}, check: func(t *testing.T, l *slackListener, c *fakeSlackClient, m *recordingMinter, handled bool, err error) { require.NoError(t, err) assert.True(t, handled, "click must be handled") require.True(t, m.called, "minter must be called") assert.Equal(t, "art-1", m.gotArtifact) assert.Equal(t, "default/sess-1", m.gotSession) - assert.Equal(t, "https://example.slack.com/archives/C01/p170?thread_ts=170&cid=C01", m.gotBackLink, "fresh mint carries the permalink back-link") - require.Len(t, c.getPermalinkCalls, 1, "getPermalink called once") - assert.Equal(t, "C01", c.getPermalinkCalls[0].Channel) - assert.Equal(t, "1700000000.000100", c.getPermalinkCalls[0].Ts, "permalink uses the thread ts") require.Len(t, c.postEphemeralCalls, 1, "one ephemeral to the clicker") assert.Equal(t, "U_CLICKER", c.postEphemeralCalls[0].userID) assert.Equal(t, "C01", c.postEphemeralCalls[0].channelID) }, }, { - name: "permalink failure: still mints with empty back-link", - value: goodValue(), - minter: &recordingMinter{url: freshURL}, - permErr: errors.New("channel_not_found"), - check: func(t *testing.T, l *slackListener, c *fakeSlackClient, m *recordingMinter, handled bool, err error) { - require.NoError(t, err) - assert.True(t, handled) - require.True(t, m.called, "mint still happens") - assert.Empty(t, m.gotBackLink, "back-link omitted when getPermalink fails") - require.Len(t, c.postEphemeralCalls, 1, "Open button still posted") - }, - }, - { - name: "no thread_ts: skips permalink, mints, posts ephemeral", + name: "no thread_ts: still mints, posts ephemeral", value: liveViewButtonValue{V: "live_view", A: "art-1", S: "default/sess-1", C: "C01", T: ""}, minter: &recordingMinter{url: freshURL}, check: func(t *testing.T, l *slackListener, c *fakeSlackClient, m *recordingMinter, handled bool, err error) { require.NoError(t, err) assert.True(t, handled) - assert.Empty(t, c.getPermalinkCalls, "no permalink call without a thread ts") - assert.Empty(t, m.gotBackLink) + require.True(t, m.called) require.Len(t, c.postEphemeralCalls, 1) }, }, @@ -198,7 +177,7 @@ func TestHandleLiveViewClick(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - c := &fakeSlackClient{getPermalinkResult: tc.permalink, getPermalinkErr: tc.permErr, postEphemeralErr: tc.postEphemeralErr} + c := &fakeSlackClient{postEphemeralErr: tc.postEphemeralErr} deps := channelkinds.Deps{} if !tc.nilMinter { deps.ArtifactViewMinter = tc.minter diff --git a/pkg/channels/channelkinds/slack/origin_link_test.go b/pkg/channels/channelkinds/slack/origin_link_test.go new file mode 100644 index 00000000..b32560df --- /dev/null +++ b/pkg/channels/channelkinds/slack/origin_link_test.go @@ -0,0 +1,84 @@ +package slack + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" + "github.com/authzed/openagentprimitives/pkg/channels/channelkinds" +) + +func TestKind_OriginLink(t *testing.T) { + k := &Kind{} + t.Run("persisted permalink present", func(t *testing.T) { + link, ok := k.OriginLink(&spiceboxv1alpha1.ChannelBinding{Kind: "slack", + External: map[string]string{"permalink": "https://demo.slack.com/archives/C1/p1700000000000100"}}) + assert.True(t, ok) + assert.Equal(t, channelkinds.OriginLink{URL: "https://demo.slack.com/archives/C1/p1700000000000100", + Label: "Open the Slack thread", Icon: "slack"}, link) + }) + t.Run("interactive session: reconstruct from the routing tuple when no permalink", func(t *testing.T) { + // A human-initiated Slack session's binding carries team_id+channel_id+ + // thread_ts (stamped by the listener at creation) but no sender-resolved + // permalink — the thread predates the agent. OriginLink reconstructs a + // thread deep-link so the artifact view still links back to the thread. + link, ok := k.OriginLink(&spiceboxv1alpha1.ChannelBinding{Kind: "slack", + External: map[string]string{"team_id": "T1", "channel_id": "C1", "thread_ts": "1700000000.000100"}}) + assert.True(t, ok) + assert.Equal(t, channelkinds.OriginLink{URL: "https://app.slack.com/client/T1/C1/thread/C1-1700000000.000100", + Label: "Open the Slack thread", Icon: "slack"}, link) + }) + t.Run("permalink wins over reconstruction", func(t *testing.T) { + link, ok := k.OriginLink(&spiceboxv1alpha1.ChannelBinding{Kind: "slack", + External: map[string]string{"team_id": "T1", "channel_id": "C1", "thread_ts": "1700.1", + "permalink": "https://demo.slack.com/archives/C1/p17001"}}) + assert.True(t, ok) + assert.Equal(t, "https://demo.slack.com/archives/C1/p17001", link.URL) + }) + t.Run("untrusted permalink host: ignored, falls back to reconstruction", func(t *testing.T) { + // External is user-writable. A permalink that is not a *.slack.com https + // URL is not trusted — the reconstructed deep link is used instead. + link, ok := k.OriginLink(&spiceboxv1alpha1.ChannelBinding{Kind: "slack", + External: map[string]string{"team_id": "T1", "channel_id": "C1", "thread_ts": "1700.1", + "permalink": "https://evil.example/login"}}) + assert.True(t, ok) + assert.Equal(t, "https://app.slack.com/client/T1/C1/thread/C1-1700.1", link.URL) + }) + t.Run("javascript: permalink and no tuple: ok=false", func(t *testing.T) { + _, ok := k.OriginLink(&spiceboxv1alpha1.ChannelBinding{Kind: "slack", + External: map[string]string{"permalink": "javascript:alert(1)"}}) + assert.False(t, ok) + }) + t.Run("no permalink and incomplete tuple (no team_id): ok=false", func(t *testing.T) { + _, ok := k.OriginLink(&spiceboxv1alpha1.ChannelBinding{Kind: "slack", + External: map[string]string{"channel_id": "C1", "thread_ts": "1700000000.000100"}}) + assert.False(t, ok) + }) + t.Run("nil binding: ok=false", func(t *testing.T) { + _, ok := k.OriginLink(nil) + assert.False(t, ok) + }) +} + +func TestIsSlackURL(t *testing.T) { + cases := []struct { + name string + u string + want bool + }{ + {"team permalink host", "https://demo.slack.com/archives/C1/p1", true}, + {"app deep link host", "https://app.slack.com/client/T1/C1/thread/C1-1", true}, + {"bare slack.com", "https://slack.com/x", true}, + {"http not https", "http://demo.slack.com/x", false}, + {"foreign host", "https://evil.example/login", false}, + {"slack.com substring attack", "https://slack.com.evil.example/x", false}, + {"javascript scheme", "javascript:alert(1)", false}, + {"empty", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, isSlackURL(tc.u)) + }) + } +} diff --git a/pkg/channels/channelkinds/slack/sender.go b/pkg/channels/channelkinds/slack/sender.go index 0d2bc87c..52f81952 100644 --- a/pkg/channels/channelkinds/slack/sender.go +++ b/pkg/channels/channelkinds/slack/sender.go @@ -286,10 +286,11 @@ func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, e if err == nil && forkRootTS != "" && len(res.External) == 0 { // ensure() created this thread; sendWithAttachments only captures a // root it created itself. Report ours so the relay patches the - // child's binding + LabelChannelKey. - res = channelkinds.SubChannelSendResult{External: map[string]string{ - "thread_ts": forkRootTS, "channel_id": channelID, - }} + // child's binding + LabelChannelKey — through threadOriginExternal so + // the artifact-view origin link gets a permalink too. + res = channelkinds.SubChannelSendResult{ + External: threadOriginExternal(ctx, s.client, log.FromContext(ctx), channelID, forkRootTS), + } } return res, err } @@ -371,7 +372,7 @@ func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, e } if rootTS != "" { return channelkinds.SubChannelSendResult{ - External: map[string]string{"thread_ts": rootTS, "channel_id": channelID}, + External: threadOriginExternal(ctx, s.client, logger, channelID, rootTS), }, nil } } @@ -460,15 +461,37 @@ func (s *slackSender) Send(ctx context.Context, sess channelkinds.SessionInfo, e rootTS = postedTS // plain cron first-send: the reply itself is the root } return channelkinds.SubChannelSendResult{ - External: map[string]string{ - "thread_ts": rootTS, - "channel_id": ch, - }, + External: threadOriginExternal(ctx, s.client, logger, ch, rootTS), }, nil } return channelkinds.SubChannelSendResult{}, nil } +// threadOriginExternal builds the SubChannelSendResult.External a first-send +// thread-root hands back to channelsd's outbound relay: the thread_ts/channel_id +// the relay keys the binding on, plus a best-effort canonical permalink +// (chat.getPermalink) that webd's artifact-view page renders as a "back to +// origin" link. The permalink is resolved here because only the sender holds a +// Slack client. A getPermalink failure is logged and the permalink omitted — it +// must never block the send, exactly as the former click-time lookup did. +// +// A package function (taking the client) rather than a method so every +// first-root path shares it — the user_message postMessage and streaming-bubble +// paths, the attachment thread root, and the tool_session bubble (on +// toolSessionSender, whose client field differs) — instead of each inventing +// its own permalink-free External. Callers MUST invoke it outside any lock: +// GetPermalinkContext is a network call. +func threadOriginExternal(ctx context.Context, cli slackClient, logger interface{ Info(string, ...any) }, channelID, rootTS string) map[string]string { + ext := map[string]string{"thread_ts": rootTS, "channel_id": channelID} + if pl, err := cli.GetPermalinkContext(ctx, &slackapi.PermalinkParameters{Channel: channelID, Ts: rootTS}); err != nil { + logger.Info("slack first-send: getPermalink failed; thread origin link omitted", + "channel", channelID, "thread", rootTS, "err", err.Error()) + } else if pl != "" { + ext["permalink"] = pl + } + return ext +} + // buildUserMessageBlocks composes the Block Kit block set for the agent's // final user_message chat.postMessage. Layout: // @@ -773,8 +796,10 @@ func (s *slackSender) sendWithAttachments( } threadTS = rootTS initialComment = "" // already posted; do not duplicate + // Route through threadOriginExternal so this first-root send captures a + // permalink for the artifact-view origin link, like the text paths do. res = channelkinds.SubChannelSendResult{ - External: map[string]string{"thread_ts": rootTS, "channel_id": channelID}, + External: threadOriginExternal(ctx, s.client, logger, channelID, rootTS), } } diff --git a/pkg/channels/channelkinds/slack/sender_live_view_offer_test.go b/pkg/channels/channelkinds/slack/sender_live_view_offer_test.go index e5e1a043..0b8a75e7 100644 --- a/pkg/channels/channelkinds/slack/sender_live_view_offer_test.go +++ b/pkg/channels/channelkinds/slack/sender_live_view_offer_test.go @@ -39,7 +39,7 @@ type stubArtifactViewMinter struct { err error } -func (m *stubArtifactViewMinter) MintArtifactViewLink(_, _ string, _ identity.Principal, _ string) (string, error) { +func (m *stubArtifactViewMinter) MintArtifactViewLink(_, _ string, _ identity.Principal) (string, error) { return m.url, m.err } diff --git a/pkg/channels/channelkinds/slack/sender_permalink_test.go b/pkg/channels/channelkinds/slack/sender_permalink_test.go new file mode 100644 index 00000000..b6a49713 --- /dev/null +++ b/pkg/channels/channelkinds/slack/sender_permalink_test.go @@ -0,0 +1,91 @@ +package slack + +import ( + "context" + "encoding/json" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" + "github.com/authzed/openagentprimitives/pkg/channels/channelevents" + "github.com/authzed/openagentprimitives/pkg/channels/channelkinds" +) + +func firstSendSession() channelkinds.SessionInfo { + return channelkinds.SessionInfo{ + Namespace: "default", Name: "cron-session", + Channel: &spiceboxv1alpha1.ChannelBinding{ + Name: "slack-eng", Kind: "slack", + // thread_ts intentionally absent: the cron/bot first-send case that + // roots the thread. + External: map[string]string{"channel_id": "C01ABCDEF"}, + }, + } +} + +// TestSender_FirstSend_CapturesPermalink: on the send that roots the thread, +// the sender resolves the canonical permalink and returns it in +// SubChannelSendResult.External so the relay can persist it for the +// artifact-view origin link. +func TestSender_FirstSend_CapturesPermalink(t *testing.T) { + c := &fakeSlackClient{postedTS: "1234.5678", getPermalinkResult: "https://slack.test/archives/C01ABCDEF/p12345678"} + s := newSender(c) + res, err := s.Send(context.Background(), firstSendSession(), userMessageEnvelope(t, "first cron post")) + require.NoError(t, err, "Send") + assert.Equal(t, "1234.5678", res.External["thread_ts"], "thread root still captured") + assert.Equal(t, "https://slack.test/archives/C01ABCDEF/p12345678", res.External["permalink"], "canonical permalink captured") + require.Len(t, c.getPermalinkCalls, 1, "getPermalink called once on first root") + assert.Equal(t, "C01ABCDEF", c.getPermalinkCalls[0].Channel) + assert.Equal(t, "1234.5678", c.getPermalinkCalls[0].Ts) +} + +// TestSender_FirstSend_PermalinkFailureOmitsLink: a getPermalink failure must +// never block the send; the thread still roots and permalink is simply absent. +func TestSender_FirstSend_PermalinkFailureOmitsLink(t *testing.T) { + c := &fakeSlackClient{postedTS: "1234.5678", getPermalinkErr: errors.New("channels:read missing")} + s := newSender(c) + res, err := s.Send(context.Background(), firstSendSession(), userMessageEnvelope(t, "first cron post")) + require.NoError(t, err, "send must succeed despite getPermalink failure") + assert.Equal(t, "1234.5678", res.External["thread_ts"], "thread root still captured") + _, has := res.External["permalink"] + assert.False(t, has, "permalink omitted when getPermalink fails") +} + +// TestSender_AttachmentFirstRoot_CapturesPermalink: a reviewbot-style first +// output that is an ATTACHMENT (github-in/slack-out, no inbound thread_ts) roots +// the thread via the files.uploadV2 path. That root must capture a permalink +// too — the output binding carries no team_id, so OriginLink's reconstruction +// fallback cannot rebuild a Slack link otherwise and the artifact view would +// show no origin. +func TestSender_AttachmentFirstRoot_CapturesPermalink(t *testing.T) { + c := &fakeSlackClient{postedTS: "888.1", getPermalinkResult: "https://slack.test/archives/C01ABCDEF/p8881"} + af := &stubAssetFetcher{bytes: []byte("hi"), mime: "text/html", filename: "report.html"} + s := &slackSender{deps: channelkinds.Deps{AssetFetcher: af}, client: c} + sess := channelkinds.SessionInfo{ + Namespace: "default", Name: "reviewbot", + Channel: &spiceboxv1alpha1.ChannelBinding{ + // no thread_ts: the attachment upload must root the thread first. + External: map[string]string{"channel_id": "C01ABCDEF"}, + }, + } + pl := channelevents.OutboundUserMessagePayload{ + Text: "Here is the review", + Attachments: []channelevents.AttachmentRef{{RenderName: "ar-1", MIME: "text/html", Filename: "review.html"}}, + } + plBytes, err := json.Marshal(pl) + require.NoError(t, err, "marshal payload") + env := channelevents.Envelope{ + Version: 1, Kind: channelevents.KindUserMessage, + Session: channelevents.SessionRef{Namespace: "default", Name: "reviewbot"}, + Payload: plBytes, + } + res, err := s.Send(context.Background(), sess, env) + require.NoError(t, err, "Send") + assert.Equal(t, "888.1", res.External["thread_ts"], "attachment upload rooted the thread") + assert.Equal(t, "https://slack.test/archives/C01ABCDEF/p8881", res.External["permalink"], + "attachment root resolves a permalink for the artifact-view origin link") + require.Len(t, c.getPermalinkCalls, 1, "getPermalink called once on the attachment root") +} diff --git a/pkg/channels/channelkinds/slack/sender_session_view_offer.go b/pkg/channels/channelkinds/slack/sender_session_view_offer.go index 181fb10a..fd3340ce 100644 --- a/pkg/channels/channelkinds/slack/sender_session_view_offer.go +++ b/pkg/channels/channelkinds/slack/sender_session_view_offer.go @@ -80,9 +80,9 @@ func (s *sessionViewOfferSender) Send(ctx context.Context, sess channelkinds.Ses // No per-viewer identity is available in this proactive path (unlike // live_view_offer's click handler, which mints fresh per clicker): the // session-view page enforces its own CheckInteract at open time, so the - // subject/backLink args below are accepted for interface parity only and - // have no bearing on who may actually open the link. - url, err := s.minter.MintSessionViewLink(p.SessionRef, identity.Principal{}, "") + // subject arg below is accepted for interface parity only and has no + // bearing on who may actually open the link. + url, err := s.minter.MintSessionViewLink(p.SessionRef, identity.Principal{}) if err != nil { return channelkinds.SubChannelSendResult{}, fmt.Errorf("session_view_offer sender: mint link: %w", err) } diff --git a/pkg/channels/channelkinds/slack/sender_session_view_offer_test.go b/pkg/channels/channelkinds/slack/sender_session_view_offer_test.go index 680b3ee2..e83d181b 100644 --- a/pkg/channels/channelkinds/slack/sender_session_view_offer_test.go +++ b/pkg/channels/channelkinds/slack/sender_session_view_offer_test.go @@ -36,7 +36,7 @@ type stubSessionViewMinter struct { err error } -func (m *stubSessionViewMinter) MintSessionViewLink(_ string, _ identity.Principal, _ string) (string, error) { +func (m *stubSessionViewMinter) MintSessionViewLink(_ string, _ identity.Principal) (string, error) { return m.url, m.err } diff --git a/pkg/channels/channelkinds/slack/sender_tool_session.go b/pkg/channels/channelkinds/slack/sender_tool_session.go index af7b52d9..0f2bd42d 100644 --- a/pkg/channels/channelkinds/slack/sender_tool_session.go +++ b/pkg/channels/channelkinds/slack/sender_tool_session.go @@ -341,11 +341,22 @@ func (s *toolSessionSender) apply(ctx context.Context, sess channelkinds.Session if rootTS == "" { rootTS = ref.msgTS } - res := channelkinds.SubChannelSendResult{ - External: map[string]string{"channel_id": ref.channelID, "thread_ts": rootTS}, - } + snapChannelID := ref.channelID ref.mu.Unlock() - return res, nil + + // On the apply that ESTABLISHED the root, resolve the canonical permalink + // (chat.getPermalink) so webd's artifact-view can link back to this Slack + // thread — the output binding carries no team_id, so OriginLink's + // reconstruction fallback cannot rebuild one for a tool_session-first + // session. Only on `first`, and OUTSIDE ref.mu: getPermalink is a network + // call, and the relay patches a thread root exactly once (its thread_ts + // guard), so later applies carry the same root and need no permalink — + // spending a getPermalink per debounced flush would be pure waste. + ext := map[string]string{"channel_id": snapChannelID, "thread_ts": rootTS} + if first && rootTS != "" { + ext = threadOriginExternal(ctx, s.cli, log.FromContext(ctx), snapChannelID, rootTS) + } + return channelkinds.SubChannelSendResult{External: ext}, nil } // post does the first chat.postMessage and records the ts. diff --git a/pkg/channels/channelkinds/webhook.go b/pkg/channels/channelkinds/webhook.go index 1bc13f90..e38e811a 100644 --- a/pkg/channels/channelkinds/webhook.go +++ b/pkg/channels/channelkinds/webhook.go @@ -195,6 +195,19 @@ type TriggerOwnerProvider interface { TriggerOwnerSubject(ch *spiceboxv1alpha1.Channel, event string, body []byte) (subject string, ok bool, err error) } +// TriggerOriginURLProvider is an OPTIONAL WebhookReceiver capability — the same +// type-assertion shape as TriggerOwnerProvider — that derives a human-facing +// origin URL from a VERIFIED delivery (a pull request's html_url), so the +// session's input binding can record where the trigger came from and webd's +// artifact-view page can link back to it. The URL is a provider-supplied +// STRUCTURAL field (GitHub's own html_url), host-correct for GitHub Enterprise +// where reconstructing a github.com URL would be wrong. Best-effort: ok=false +// when the delivery names no such URL, and the channelsd inbound handler starts +// the session without the link rather than failing. +type TriggerOriginURLProvider interface { + TriggerOriginURL(ch *spiceboxv1alpha1.Channel, event string, body []byte) (url string, ok bool, err error) +} + // TriggerSlotInstance is one resource id a verified webhook delivery names, // paired with the resource type a class slot's fillFrom must match it // against. A slot binds it only when the slot's own resourceType agrees — diff --git a/pkg/channels/channelsd/outbound/relay.go b/pkg/channels/channelsd/outbound/relay.go index 6ea8808a..9206faea 100644 --- a/pkg/channels/channelsd/outbound/relay.go +++ b/pkg/channels/channelsd/outbound/relay.go @@ -1065,6 +1065,18 @@ func (r *Relay) patchOutputChannel(ctx context.Context, sess *spiceboxv1alpha1.A return nil } key := fmt.Sprintf("thread:%s:%s", channelID, threadTS) + // The canonical thread permalink (chat.getPermalink), when the Sender + // resolved one on this first-root send, rides the same patch as the + // thread_ts it is derived from — webd reads it off the binding for the + // artifact-view "back to origin" link. Omitted (never written empty) when + // resolution failed: this patch runs once per thread root (the thread_ts + // guard above blocks re-patching the same thread), so a failed permalink is + // not backfilled for this thread — the artifact view falls back to a thread + // link reconstructed from channel_id/thread_ts instead. + extPatch := map[string]string{"channel_id": channelID, "thread_ts": threadTS} + if pl := captured["permalink"]; pl != "" { + extPatch["permalink"] = pl + } // Patch BOTH spec.outputChannel.{key,external} AND // metadata.labels[LabelOutputChannelKey] in a single MergePatch so // the index label is in sync with the spec atomically — there is no @@ -1079,11 +1091,8 @@ func (r *Relay) patchOutputChannel(ctx context.Context, sess *spiceboxv1alpha1.A }, "spec": map[string]any{ "outputChannel": map[string]any{ - "key": key, - "external": map[string]string{ - "channel_id": channelID, - "thread_ts": threadTS, - }, + "key": key, + "external": extPatch, }, }, } @@ -1112,6 +1121,9 @@ func (r *Relay) patchOutputChannel(ctx context.Context, sess *spiceboxv1alpha1.A } mergedExternal["channel_id"] = channelID mergedExternal["thread_ts"] = threadTS + if pl := captured["permalink"]; pl != "" { + mergedExternal["permalink"] = pl + } sess.Spec.OutputChannel.External = mergedExternal sess.Spec.OutputChannel.Key = key @@ -1172,6 +1184,10 @@ func (r *Relay) patchInputChannelThreadTS(ctx context.Context, sess *spiceboxv1a return nil } key := fmt.Sprintf("thread:%s:%s", channelID, threadTS) + extPatch := map[string]string{"channel_id": channelID, "thread_ts": threadTS} + if pl := captured["permalink"]; pl != "" { + extPatch["permalink"] = pl + } // Patch spec.inputChannel.external AND LabelChannelKey atomically so // the inbound pipeline's label lookup sees the new thread root without // a window where the key is set but the label isn't (or vice versa). @@ -1183,11 +1199,8 @@ func (r *Relay) patchInputChannelThreadTS(ctx context.Context, sess *spiceboxv1a }, "spec": map[string]any{ "inputChannel": map[string]any{ - "key": key, - "external": map[string]string{ - "channel_id": channelID, - "thread_ts": threadTS, - }, + "key": key, + "external": extPatch, }, }, } diff --git a/pkg/channels/channelsd/outbound/relay_permalink_test.go b/pkg/channels/channelsd/outbound/relay_permalink_test.go new file mode 100644 index 00000000..5c7a0f4a --- /dev/null +++ b/pkg/channels/channelsd/outbound/relay_permalink_test.go @@ -0,0 +1,56 @@ +package outbound + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "sigs.k8s.io/controller-runtime/pkg/client" + + spiceboxv1alpha1 "github.com/authzed/openagentprimitives/pkg/apis/v1alpha1" +) + +const testPermalink = "https://slack.test/archives/C01ABCDEF/p12345678" + +// TestRelay_PatchOutputChannel_PersistsPermalink: the canonical thread permalink +// the sender captured on first root is written onto spec.outputChannel.external +// alongside thread_ts, both persisted and reflected in memory, so webd can read +// it off the session for the artifact-view origin link. +func TestRelay_PatchOutputChannel_PersistsPermalink(t *testing.T) { + nc := connectNATS(t) + sess := cronSession("cron-session", map[string]string{"channel_id": "C01ABCDEF"}) + cli := fakeClientWith(t, sess) + r := &Relay{NC: nc, K8s: cli} + + err := r.patchOutputChannel(context.Background(), sess, map[string]string{ + "thread_ts": "1234.5678", "channel_id": "C01ABCDEF", "permalink": testPermalink, + }) + require.NoError(t, err, "patchOutputChannel") + + assert.Equal(t, testPermalink, sess.Spec.OutputChannel.External["permalink"], "in-memory reflection carries permalink") + + var patched spiceboxv1alpha1.AgentSession + require.NoError(t, cli.Get(context.Background(), client.ObjectKey{Namespace: "default", Name: "cron-session"}, &patched)) + assert.Equal(t, testPermalink, patched.Spec.OutputChannel.External["permalink"], "persisted permalink") + assert.Equal(t, "1234.5678", patched.Spec.OutputChannel.External["thread_ts"], "thread_ts still persisted") +} + +// TestRelay_PatchOutputChannel_NoPermalink_WritesNone: when the sender captured +// no permalink (getPermalink failed), no permalink key is written. +func TestRelay_PatchOutputChannel_NoPermalink_WritesNone(t *testing.T) { + nc := connectNATS(t) + sess := cronSession("cron-session", map[string]string{"channel_id": "C01ABCDEF"}) + cli := fakeClientWith(t, sess) + r := &Relay{NC: nc, K8s: cli} + + err := r.patchOutputChannel(context.Background(), sess, map[string]string{ + "thread_ts": "1234.5678", "channel_id": "C01ABCDEF", + }) + require.NoError(t, err, "patchOutputChannel") + + var patched spiceboxv1alpha1.AgentSession + require.NoError(t, cli.Get(context.Background(), client.ObjectKey{Namespace: "default", Name: "cron-session"}, &patched)) + _, has := patched.Spec.OutputChannel.External["permalink"] + assert.False(t, has, "no permalink key when none captured") +} diff --git a/pkg/controllers/agentsession/restart_decide.go b/pkg/controllers/agentsession/restart_decide.go index c0c6ff0d..a8f027fd 100644 --- a/pkg/controllers/agentsession/restart_decide.go +++ b/pkg/controllers/agentsession/restart_decide.go @@ -200,11 +200,19 @@ func BuildChildSession(parent *spiceboxv1alpha1.AgentSession, pr *spiceboxv1alph // forkChildBinding derives a restart-fork child's copy of one ChannelBinding. // Returns nil for a nil binding. The child gets its own subject prefix and // records its predecessor via InheritFrom. On a branching fork (restart-from- -// here) thread_ts is dropped from a deep-copied External so the Slack sender -// posts at channel level and the returned ts becomes the new thread root -// (first-send capture); every other External key survives. An inherit fork -// keeps thread_ts so the child resumes the same thread. Shared by the input and -// output bindings — both branch the same way. +// here) thread_ts AND permalink are dropped from a deep-copied External so the +// Slack sender posts at channel level and the returned ts becomes the new +// thread root (first-send capture); every other External key survives. An +// inherit fork keeps them so the child resumes the same thread. Shared by the +// input and output bindings — both branch the same way. +// +// permalink is dropped alongside thread_ts because it is the canonical link to +// the PARENT thread (resolved when that thread was first rooted). A branching +// child lives in a different thread, so carrying the parent's permalink would +// make the child's artifact-view "back to origin" link point at the wrong +// conversation until — and, if the child's first-send getPermalink fails, +// permanently past — its own first send. Dropping it lets OriginLink fall back +// to reconstructing the child's own thread deep-link from the new thread_ts. func forkChildBinding(cb *spiceboxv1alpha1.ChannelBinding, parentName, childSubjectPrefix string, branchesThread bool) *spiceboxv1alpha1.ChannelBinding { if cb == nil { return nil @@ -215,7 +223,7 @@ func forkChildBinding(cb *spiceboxv1alpha1.ChannelBinding, parentName, childSubj if branchesThread && len(copied.External) > 0 { ext := make(map[string]string, len(copied.External)) for k, v := range copied.External { - if k != "thread_ts" { + if k != "thread_ts" && k != "permalink" { ext[k] = v } } diff --git a/pkg/controllers/agentsession/restart_decide_test.go b/pkg/controllers/agentsession/restart_decide_test.go index 677eb0cc..08db7979 100644 --- a/pkg/controllers/agentsession/restart_decide_test.go +++ b/pkg/controllers/agentsession/restart_decide_test.go @@ -116,11 +116,11 @@ func TestBuildChildSession_CarriesForwardSpec(t *testing.T) { } // TestBuildChildSession_ClearsThreadTSAndChannelKeyLabel verifies that a -// restart-from-here (-fk) fork's ChannelBinding has thread_ts stripped (so -// the Slack sender posts at channel level, creating a new thread root) and -// LabelChannelKey is absent (the old hash was tied to the parent's -// thread_ts). An inherit fork does NOT do this — see -// TestBuildChildSession_InheritForkStaysInTheParentThread. +// restart-from-here (-fk) fork's ChannelBinding has thread_ts (and the +// parent-thread permalink) stripped (so the Slack sender posts at channel +// level, creating a new thread root) and LabelChannelKey is absent (the old +// hash was tied to the parent's thread_ts). An inherit fork does NOT do this — +// see TestBuildChildSession_InheritForkStaysInTheParentThread. func TestBuildChildSession_ClearsThreadTSAndChannelKeyLabel(t *testing.T) { parent := &v1alpha1.AgentSession{ ObjectMeta: metav1.ObjectMeta{ @@ -141,6 +141,7 @@ func TestBuildChildSession_ClearsThreadTSAndChannelKeyLabel(t *testing.T) { "thread_ts": "1234.5678", "team_id": "T99", "bot_user_id": "B1", + "permalink": "https://demo.slack.com/archives/C123/p12345678", }, }, }, @@ -157,6 +158,13 @@ func TestBuildChildSession_ClearsThreadTSAndChannelKeyLabel(t *testing.T) { assert.NotContains(t, child.Spec.InputChannel.External, "thread_ts", "thread_ts must be cleared on child to spawn a new thread") + // permalink must be absent too: it is the canonical link to the PARENT + // thread. Carrying it would make the child's artifact-view origin link point + // at the wrong conversation; stripping it lets OriginLink reconstruct the + // child's own thread link once first-send roots it. + assert.NotContains(t, child.Spec.InputChannel.External, "permalink", + "parent-thread permalink must be cleared on a branching fork") + // Other external keys that are needed for routing must survive. assert.Equal(t, "C123", child.Spec.InputChannel.External["channel_id"]) assert.Equal(t, "T99", child.Spec.InputChannel.External["team_id"]) @@ -239,7 +247,8 @@ func TestBuildChildSession_InheritForkStaysInTheParentThread(t *testing.T) { Spec: v1alpha1.AgentSessionSpec{ InputChannel: &v1alpha1.ChannelBinding{ Name: "ch", Kind: "slack", - External: map[string]string{"team_id": "T1", "channel_id": "C1", "thread_ts": "111.1"}, + External: map[string]string{"team_id": "T1", "channel_id": "C1", "thread_ts": "111.1", + "permalink": "https://demo.slack.com/archives/C1/p1111"}, }, }, } @@ -250,6 +259,8 @@ func TestBuildChildSession_InheritForkStaysInTheParentThread(t *testing.T) { require.NotNil(t, inherit.Spec.InputChannel) assert.Equal(t, "111.1", inherit.Spec.InputChannel.External["thread_ts"], "a continuation must stay in the thread the user is reading") + assert.Equal(t, "https://demo.slack.com/archives/C1/p1111", inherit.Spec.InputChannel.External["permalink"], + "same thread ⇒ the parent permalink still points at the right conversation, so it is kept") assert.Equal(t, "key-hash", inherit.Labels[v1alpha1.LabelChannelKey], "same thread ⇒ same channel key; Deliver prefers the active child") assert.NotContains(t, inherit.Annotations, v1alpha1.AnnotationForkedFromThread, diff --git a/pkg/platform/identity/passthroughlink/link.go b/pkg/platform/identity/passthroughlink/link.go index 5dfd86dd..4b107c65 100644 --- a/pkg/platform/identity/passthroughlink/link.go +++ b/pkg/platform/identity/passthroughlink/link.go @@ -115,11 +115,6 @@ type Payload struct { // points at (Purpose == PurposeArtifactView). webd resolves the // artifact's SpiceDB `view` permission for the visitor. ArtifactID string `json:"artifactId,omitempty"` - // BackLink is an optional, opaque URL the consuming web UI renders as a - // "back to origin" link (e.g. a Slack thread permalink for artifact_view - // links). Empty when the minting channel kind has no permalink concept. - // Tamper-proof (it rides inside the signed body) but not confidential. - BackLink string `json:"backLink,omitempty"` // AgentIdentityRef is "/" — the // AgentIdentity in the workshop namespace W whose credential a // PurposeWorkshopCredential link connects. Empty for every other purpose. diff --git a/pkg/platform/identity/passthroughlink/link_test.go b/pkg/platform/identity/passthroughlink/link_test.go index 662d267c..bec7cd40 100644 --- a/pkg/platform/identity/passthroughlink/link_test.go +++ b/pkg/platform/identity/passthroughlink/link_test.go @@ -218,32 +218,6 @@ func TestMintVerify_ArtifactViewLink(t *testing.T) { assert.Equal(t, PurposeArtifactView, p.Purpose) } -// TestMintVerify_BackLinkRoundTrips guards the optional, opaque BackLink -// claim used by artifact_view links to carry a "back to origin" URL. -func TestMintVerify_BackLinkRoundTrips(t *testing.T) { - s := New(make([]byte, 32)) - const back = "https://example.slack.com/archives/C1/p1700000000000100?thread_ts=1700000000.000100&cid=C1" - raw, err := s.Mint(Payload{ - Purpose: PurposeArtifactView, - ArtifactID: "art-1", - SessionRef: "ns/sess", - BackLink: back, - ExpiresAt: time.Now().Add(time.Hour).Unix(), - }) - require.NoError(t, err, "Mint") - - p, err := s.Verify(raw) - require.NoError(t, err, "Verify") - assert.Equal(t, back, p.BackLink, "BackLink round-trips through Mint→Verify") - - // Empty BackLink must be omitted from the wire (omitempty). - raw2, err := s.Mint(Payload{ExpiresAt: time.Now().Add(time.Hour).Unix()}) - require.NoError(t, err, "Mint without BackLink") - p2, err := s.Verify(raw2) - require.NoError(t, err, "Verify without BackLink") - assert.Empty(t, p2.BackLink, "absent BackLink decodes to empty string") -} - // TestVerify_RejectsMissingJTI guards that a signed token whose payload lacks // a jti is rejected. Mint always sets jti, so a jti-less token cannot have come // from a legitimate minter (a buggy/forged minter, or a hand-rolled token). We diff --git a/pkg/platform/identity/passthroughlink/sessionviewlink/minter.go b/pkg/platform/identity/passthroughlink/sessionviewlink/minter.go index e4f3d774..3f34bb2c 100644 --- a/pkg/platform/identity/passthroughlink/sessionviewlink/minter.go +++ b/pkg/platform/identity/passthroughlink/sessionviewlink/minter.go @@ -31,11 +31,9 @@ type Minter struct { } // Mint returns the raw signed "." token authorizing sessionRef for -// subject, with backLink carried through (tamper-proof, not confidential) for -// the consuming page's "back to origin" affordance. Callers compose the final -// deep-link URL around this token, using the same "?d=&sig=" split -// viewlink's minters use. -func (m *Minter) Mint(sessionRef string, subject identity.Principal, backLink string) (string, error) { +// subject. Callers compose the final deep-link URL around this token, using the +// same "?d=&sig=" split viewlink's minters use. +func (m *Minter) Mint(sessionRef string, subject identity.Principal) (string, error) { if m == nil || m.Signer == nil { return "", fmt.Errorf("session view minter not configured") } @@ -52,7 +50,6 @@ func (m *Minter) Mint(sessionRef string, subject identity.Principal, backLink st SessionRef: sessionRef, Subject: subj, SubjectVerified: subject.EmailVerified(), - BackLink: backLink, ExpiresAt: time.Now().Add(SessionViewLinkTTL).Unix(), }) if err != nil { diff --git a/pkg/platform/identity/passthroughlink/sessionviewlink/minter_test.go b/pkg/platform/identity/passthroughlink/sessionviewlink/minter_test.go index f3e9a6b7..5b0be64b 100644 --- a/pkg/platform/identity/passthroughlink/sessionviewlink/minter_test.go +++ b/pkg/platform/identity/passthroughlink/sessionviewlink/minter_test.go @@ -11,10 +11,9 @@ import ( "github.com/authzed/openagentprimitives/pkg/platform/identity/passthroughlink" ) -// TestMinter_HappyPath verifies mint→verify round-trips the sessionRef, -// subject, and backLink, sets Purpose=PurposeSessionView, carries NO -// ArtifactID, and stamps a long (~7 day) TTL — distinct from the 30-minute -// artifact-view link. +// TestMinter_HappyPath verifies mint→verify round-trips the sessionRef and +// subject, sets Purpose=PurposeSessionView, carries NO ArtifactID, and stamps a +// long (~7 day) TTL — distinct from the 30-minute artifact-view link. func TestMinter_HappyPath(t *testing.T) { signer := passthroughlink.New(make([]byte, 32), passthroughlink.WithIssuer(passthroughlink.IssuerChannelsd), @@ -24,8 +23,7 @@ func TestMinter_HappyPath(t *testing.T) { before := time.Now() subject := identity.RawSubject("user:alice") - backLink := "https://example.slack.com/archives/C1/p170?thread_ts=170&cid=C1" - raw, err := m.Mint("default/sess-1", subject, backLink) + raw, err := m.Mint("default/sess-1", subject) require.NoError(t, err, "Mint should succeed") require.NotEmpty(t, raw) @@ -41,7 +39,6 @@ func TestMinter_HappyPath(t *testing.T) { subjectStr, err := subject.Subject() require.NoError(t, err) assert.Equal(t, subjectStr, p.Subject, "Subject round-trips") - assert.Equal(t, backLink, p.BackLink, "BackLink round-trips") wantExpiry := before.Add(SessionViewLinkTTL) gotExpiry := time.Unix(p.ExpiresAt, 0) @@ -55,7 +52,7 @@ func TestMinter_SubjectVerified_VerifiedEmail(t *testing.T) { m := &Minter{Signer: signer} principal := identity.VerifiedEmail("alice@example.com", "") - raw, err := m.Mint("ns/sess-1", principal, "") + raw, err := m.Mint("ns/sess-1", principal) require.NoError(t, err) p, err := signer.Verify(raw) @@ -70,7 +67,7 @@ func TestMinter_SubjectVerified_Unverified(t *testing.T) { m := &Minter{Signer: signer} principal := identity.FromExternal("local", "", "alice", "").AllowSynthetic() - raw, err := m.Mint("ns/sess-1", principal, "") + raw, err := m.Mint("ns/sess-1", principal) require.NoError(t, err) p, err := signer.Verify(raw) @@ -82,7 +79,7 @@ func TestMinter_SubjectVerified_Unverified(t *testing.T) { // mint time — a session-view link with no session to point at is meaningless. func TestMinter_EmptySessionRef_Errors(t *testing.T) { m := &Minter{Signer: passthroughlink.New(make([]byte, 32))} - _, err := m.Mint("", identity.RawSubject("user:alice"), "") + _, err := m.Mint("", identity.RawSubject("user:alice")) require.Error(t, err) assert.Contains(t, err.Error(), "sessionRef") } @@ -91,7 +88,7 @@ func TestMinter_EmptySessionRef_Errors(t *testing.T) { // fails closed rather than panicking. func TestMinter_NilSigner_Errors(t *testing.T) { m := &Minter{} - _, err := m.Mint("ns/sess", identity.RawSubject("user:alice"), "") + _, err := m.Mint("ns/sess", identity.RawSubject("user:alice")) require.Error(t, err) } @@ -104,7 +101,7 @@ func TestVerify_RejectsWrongPurpose(t *testing.T) { signer := passthroughlink.New(make([]byte, 32)) m := &Minter{Signer: signer} - sessionRaw, err := m.Mint("default/sess-1", identity.RawSubject("user:alice"), "") + sessionRaw, err := m.Mint("default/sess-1", identity.RawSubject("user:alice")) require.NoError(t, err) sp, err := signer.Verify(sessionRaw) require.NoError(t, err) @@ -146,7 +143,7 @@ func TestVerify_RejectsExpired(t *testing.T) { func TestVerify_RejectsTamperedSignature(t *testing.T) { signer := passthroughlink.New(make([]byte, 32)) m := &Minter{Signer: signer} - raw, err := m.Mint("default/sess-1", identity.RawSubject("user:alice"), "") + raw, err := m.Mint("default/sess-1", identity.RawSubject("user:alice")) require.NoError(t, err) tampered := []byte(raw) diff --git a/pkg/platform/identity/passthroughlink/viewlink/minter.go b/pkg/platform/identity/passthroughlink/viewlink/minter.go index eecbb411..6df81b19 100644 --- a/pkg/platform/identity/passthroughlink/viewlink/minter.go +++ b/pkg/platform/identity/passthroughlink/viewlink/minter.go @@ -33,8 +33,8 @@ type Minter struct { // the given artifact, session, and subject. Returns ("", nil) when the webd // base URL is empty (webd not yet installed or ConfigMap not yet populated) — // callers treat an empty URL as a clean skip (no post, no error log). -func (m *Minter) MintArtifactViewLink(artifactID, sessionRef string, subject identity.Principal, backLink string) (string, error) { - return m.mint("/artifact-view", artifactID, sessionRef, subject, backLink) +func (m *Minter) MintArtifactViewLink(artifactID, sessionRef string, subject identity.Principal) (string, error) { + return m.mint("/artifact-view", artifactID, sessionRef, subject) } // MintArtifactDownloadLink returns "/artifact-download?d=&sig=": @@ -44,14 +44,14 @@ func (m *Minter) MintArtifactViewLink(artifactID, sessionRef string, subject ide // view grant, so it reuses PurposeArtifactView and the same CheckView gate — // no separate authorization. Returns ("", nil) on an empty base URL, same clean // skip as the view link (the desktop case with no external origin wired). -func (m *Minter) MintArtifactDownloadLink(artifactID, sessionRef string, subject identity.Principal, backLink string) (string, error) { - return m.mint("/artifact-download", artifactID, sessionRef, subject, backLink) +func (m *Minter) MintArtifactDownloadLink(artifactID, sessionRef string, subject identity.Principal) (string, error) { + return m.mint("/artifact-download", artifactID, sessionRef, subject) } // mint builds a signed webd deep-link at the given path. The payload is // identical across link kinds (view/download); only the path differs, so the // same signature authorizes both under the one CheckView gate. -func (m *Minter) mint(path, artifactID, sessionRef string, subject identity.Principal, backLink string) (string, error) { +func (m *Minter) mint(path, artifactID, sessionRef string, subject identity.Principal) (string, error) { if m == nil || m.Signer == nil { return "", fmt.Errorf("artifact view minter not configured") } @@ -74,7 +74,6 @@ func (m *Minter) mint(path, artifactID, sessionRef string, subject identity.Prin SessionRef: sessionRef, Subject: subj, SubjectVerified: subject.EmailVerified(), - BackLink: backLink, ExpiresAt: time.Now().Add(ArtifactViewLinkTTL).Unix(), }) if err != nil { diff --git a/pkg/platform/identity/passthroughlink/viewlink/minter_test.go b/pkg/platform/identity/passthroughlink/viewlink/minter_test.go index 09c9b14e..e62af169 100644 --- a/pkg/platform/identity/passthroughlink/viewlink/minter_test.go +++ b/pkg/platform/identity/passthroughlink/viewlink/minter_test.go @@ -21,7 +21,7 @@ func TestMinter_DownloadLink(t *testing.T) { base := "https://webd.example.com" m := &Minter{Signer: signer, WebdBaseURL: func() string { return base }} - u, err := m.MintArtifactDownloadLink("art-abc", "default/sess-1", identity.RawSubject("user:alice"), "") + u, err := m.MintArtifactDownloadLink("art-abc", "default/sess-1", identity.RawSubject("user:alice")) require.NoError(t, err) assert.True(t, strings.HasPrefix(u, base+"/artifact-download?d="), "must point at the download endpoint") assert.Contains(t, u, "&sig=") @@ -42,7 +42,7 @@ func TestMinter_DownloadLink(t *testing.T) { // external origin is wired yet. func TestMinter_DownloadLink_EmptyBaseSkips(t *testing.T) { m := &Minter{Signer: passthroughlink.New(make([]byte, 32)), WebdBaseURL: func() string { return "" }} - u, err := m.MintArtifactDownloadLink("art-abc", "default/sess-1", identity.RawSubject("user:alice"), "") + u, err := m.MintArtifactDownloadLink("art-abc", "default/sess-1", identity.RawSubject("user:alice")) require.NoError(t, err) assert.Empty(t, u) } @@ -65,8 +65,7 @@ func TestMinter_HappyPath(t *testing.T) { sessionRef := "default/sess-1" subject := identity.RawSubject("user:alice") - backLink := "https://example.slack.com/archives/C1/p170?thread_ts=170&cid=C1" - u, err := m.MintArtifactViewLink(artifactID, sessionRef, subject, backLink) + u, err := m.MintArtifactViewLink(artifactID, sessionRef, subject) require.NoError(t, err, "MintArtifactViewLink should succeed") assert.True(t, strings.HasPrefix(u, base+"/artifact-view?"), "URL must start with base+/artifact-view?") assert.Contains(t, u, "d=", "URL must contain d= query param") @@ -90,7 +89,6 @@ func TestMinter_HappyPath(t *testing.T) { // Explicit aud assertion: the minter must set aud=webd even though the // signer was constructed with WithAudience(AudienceIdentityd). assert.Equal(t, passthroughlink.AudienceWebd, p.Audience, "Audience must be webd, not identityd") - assert.Equal(t, backLink, p.BackLink, "BackLink round-trips through the minted link") } // TestMinter_SubjectVerified_VerifiedEmail verifies that minting with a @@ -105,7 +103,7 @@ func TestMinter_SubjectVerified_VerifiedEmail(t *testing.T) { } principal := identity.VerifiedEmail("alice@example.com", "") - u, err := m.MintArtifactViewLink("art-1", "ns/sess-1", principal, "") + u, err := m.MintArtifactViewLink("art-1", "ns/sess-1", principal) require.NoError(t, err) after, ok := strings.CutPrefix(u, base+"/artifact-view?d=") @@ -133,7 +131,7 @@ func TestMinter_SubjectVerified_Unverified(t *testing.T) { } principal := identity.FromExternal("local", "", "alice", "").AllowSynthetic() - u, err := m.MintArtifactViewLink("art-1", "ns/sess-1", principal, "") + u, err := m.MintArtifactViewLink("art-1", "ns/sess-1", principal) require.NoError(t, err) after, ok := strings.CutPrefix(u, base+"/artifact-view?d=") @@ -156,7 +154,7 @@ func TestMinter_EmptyBaseURL(t *testing.T) { WebdBaseURL: func() string { return "" }, } - u, err := m.MintArtifactViewLink("art-1", "ns/sess", identity.RawSubject(""), "") + u, err := m.MintArtifactViewLink("art-1", "ns/sess", identity.RawSubject("")) require.NoError(t, err, "empty base URL must return nil error (clean skip)") assert.Empty(t, u, "empty base URL must return empty URL string") } @@ -170,7 +168,7 @@ func TestMinter_TrailingSlash(t *testing.T) { WebdBaseURL: func() string { return "https://webd.example.com/" }, } - u, err := m.MintArtifactViewLink("art-1", "ns/sess", identity.RawSubject("").AllowSynthetic(), "") + u, err := m.MintArtifactViewLink("art-1", "ns/sess", identity.RawSubject("").AllowSynthetic()) require.NoError(t, err) assert.False(t, strings.Contains(u, "//artifact-view"), "double-slash must be stripped; got %q", u) } @@ -184,7 +182,7 @@ func TestMinter_EmptyArtifactID(t *testing.T) { WebdBaseURL: func() string { return "https://webd.example.com" }, } - _, err := m.MintArtifactViewLink("", "ns/sess", identity.RawSubject(""), "") + _, err := m.MintArtifactViewLink("", "ns/sess", identity.RawSubject("")) require.Error(t, err, "empty artifactID must return an error") assert.Contains(t, err.Error(), "artifactID") } diff --git a/pkg/web/webui/artifactview/artifactview_test.go b/pkg/web/webui/artifactview/artifactview_test.go index 9f6844f7..02946ba7 100644 --- a/pkg/web/webui/artifactview/artifactview_test.go +++ b/pkg/web/webui/artifactview/artifactview_test.go @@ -32,11 +32,11 @@ const bakedCSP = `default-src 'none'; img-src 'self' data:; style-src 'unsafe-in // interface; an unset func field would panic, but fakeAV() populates the // happy-path defaults so the shell + content flows succeed end to end. type fakeDeps struct { - verifyLink func(raw string) (string, string, string, error) + verifyLink func(raw string) (string, string, error) checkView func(ctx context.Context, artifactID, subject string) (bool, error) checkInteract func(ctx context.Context, ns, sess, subject string) (bool, error) artifactMeta func(ctx context.Context, ns, sess, artifactID string) (string, string, error) - channelKind func(ctx context.Context, ns, sess string) (string, error) + originLinks func(ctx context.Context, ns, sess string) []artifactview.OriginLink sessionViews func(ctx context.Context, ns, sess string) []string resolveRender func(ctx context.Context, ns, sess, artifactID string) (string, error) contentRender func(ctx context.Context, ns, sess, artifactID string) (string, bool, error) @@ -56,7 +56,7 @@ type fakeDeps struct { logger logr.Logger } -func (f *fakeDeps) VerifyLink(raw string) (string, string, string, error) { return f.verifyLink(raw) } +func (f *fakeDeps) VerifyLink(raw string) (string, string, error) { return f.verifyLink(raw) } func (f *fakeDeps) CheckView(ctx context.Context, artifactID, subject string) (bool, error) { return f.checkView(ctx, artifactID, subject) } @@ -76,11 +76,11 @@ func (f *fakeDeps) ArtifactMeta(ctx context.Context, ns, sess, artifactID string } return f.artifactMeta(ctx, ns, sess, artifactID) } -func (f *fakeDeps) ChannelKind(ctx context.Context, ns, sess string) (string, error) { - if f.channelKind == nil { - return "", nil +func (f *fakeDeps) OriginLinks(ctx context.Context, ns, sess string) []artifactview.OriginLink { + if f.originLinks == nil { + return nil } - return f.channelKind(ctx, ns, sess) + return f.originLinks(ctx, ns, sess) } func (f *fakeDeps) ResolveRender(ctx context.Context, ns, sess, artifactID string) (string, error) { return f.resolveRender(ctx, ns, sess, artifactID) @@ -220,15 +220,12 @@ func (f *fakeDeps) Logger() logr.Logger { return f.logger } // two fields it exercises. func fakeAV() *fakeDeps { return &fakeDeps{ - verifyLink: func(raw string) (string, string, string, error) { - return "artifact-x", "default/s1", "", nil + verifyLink: func(raw string) (string, string, error) { + return "artifact-x", "default/s1", nil }, checkView: func(ctx context.Context, artifactID, subject string) (bool, error) { return true, nil }, - channelKind: func(ctx context.Context, ns, sess string) (string, error) { - return "slack", nil - }, resolveRender: func(ctx context.Context, ns, sess, artifactID string) (string, error) { return "ar-1", nil }, @@ -314,7 +311,7 @@ func TestDownload_HappyPath_ServesBytesAsAttachment(t *testing.T) { // the artifact id + a MIME-based extension, sanitized. func TestDownload_FilenameFallbackFromMIME(t *testing.T) { av := fakeAV() - av.verifyLink = func(raw string) (string, string, string, error) { return "art-xyz", "default/s1", "", nil } + av.verifyLink = func(raw string) (string, string, error) { return "art-xyz", "default/s1", nil } av.fetchRender = func(ctx context.Context, ns, sess, renderName string) ([]byte, string, error) { return []byte("data"), "text/html", nil } @@ -511,10 +508,46 @@ func TestShell_ShowsArtifactTitleAndDescription(t *testing.T) { assert.Contains(t, body, "Live view — Quarterly Report", "document title carries the artifact name") } -func TestShell_RendersBackLinkWhenPresent(t *testing.T) { +func TestShell_RendersOriginLinksWhenPresent(t *testing.T) { + av := fakeAV() // default CheckInteract=true → a session participant + av.originLinks = func(ctx context.Context, ns, sess string) []artifactview.OriginLink { + return []artifactview.OriginLink{ + {Href: "https://github.com/demo-org/platform/pull/42", Label: "View the pull request", Icon: "github"}, + {Href: "https://example.slack.com/archives/C1/p170", Label: "Open the Slack thread", Icon: "slack"}, + } + } + s := newServer(t, "user:abc", av) + + rec := httptest.NewRecorder() + s.ServeHTTP(rec, req(trustedHost, "/artifact-view?d=AA&sig=BB")) + + require.Equal(t, http.StatusOK, rec.Code) + body := rec.Body.String() + // The origin links are passed to the client via props (json key "originLinks"); + // the React app renders one "back to origin" affordance per entry, using the + // kind-supplied label (tooltip) and icon key — never branching on the kind. + assert.Contains(t, body, `"originLinks":[`, "bootstrap props carry the origin links array") + assert.Contains(t, body, `"href":"https://github.com/demo-org/platform/pull/42"`, "props carry the PR link") + assert.Contains(t, body, `"label":"View the pull request"`, "props carry the PR link's tooltip label") + assert.Contains(t, body, `"icon":"github"`, "props carry the PR link's icon key") + assert.Contains(t, body, `"href":"https://example.slack.com/archives/C1/p170"`, "props carry the Slack thread link") +} + +// TestShell_OriginLinksGatedForNonParticipant covers the security gate: origin +// links are SESSION-scoped (a private Slack permalink, a private-repo PR URL), +// so a subject who passes the artifact's CheckView but is NOT a session +// participant (CheckInteract=false — a platform admin, or any user on an +// org-visible session) must receive NO origin links, even though the Deps would +// resolve them. +func TestShell_OriginLinksGatedForNonParticipant(t *testing.T) { av := fakeAV() - av.verifyLink = func(raw string) (string, string, string, error) { - return "artifact-x", "default/s1", "https://example.slack.com/archives/C1/p170?thread_ts=170&cid=C1", nil + av.checkInteract = func(ctx context.Context, ns, sess, subject string) (bool, error) { + return false, nil // viewer may VIEW the artifact but is not in the conversation + } + av.originLinks = func(ctx context.Context, ns, sess string) []artifactview.OriginLink { + return []artifactview.OriginLink{ + {Href: "https://example.slack.com/archives/C1/p170", Label: "Open the Slack thread", Icon: "slack"}, + } } s := newServer(t, "user:abc", av) @@ -523,21 +556,19 @@ func TestShell_RendersBackLinkWhenPresent(t *testing.T) { require.Equal(t, http.StatusOK, rec.Code) body := rec.Body.String() - // The back link is now passed to the client via props (json key "backLink"); - // the React app renders the "Back to thread" affordance. The bootstrap JSON is - // HTML-escaped, so the URL's "&" appears as "&" — match the stable prefix. - assert.Contains(t, body, `"backLink":"https://example.slack.com/archives/C1/p170`, "bootstrap props carry the back-link URL") + assert.Contains(t, body, `"originLinks":[]`, "a non-participant viewer gets an empty origin links array") + assert.NotContains(t, body, "example.slack.com", "the private Slack permalink must not leak to a non-participant") } -func TestShell_OmitsBackLinkWhenEmpty(t *testing.T) { - av := fakeAV() // fakeAV's verifyLink returns "" back-link +func TestShell_OriginLinksEmptyIsAnArray(t *testing.T) { + av := fakeAV() // fakeAV's originLinks unset → the page coerces nil to [] s := newServer(t, "user:abc", av) rec := httptest.NewRecorder() s.ServeHTTP(rec, req(trustedHost, "/artifact-view?d=AA&sig=BB")) require.Equal(t, http.StatusOK, rec.Code) - assert.Contains(t, rec.Body.String(), `"backLink":""`, "bootstrap props carry an empty back-link when none is set") + assert.Contains(t, rec.Body.String(), `"originLinks":[]`, "empty origin links serialize as an array, never null") } func TestShell_FallsBackToArtifactIDWhenNoName(t *testing.T) { @@ -631,8 +662,8 @@ func TestShell_Forbidden_DoesNotLeakContentURL(t *testing.T) { func TestShell_BadLink_Forbidden(t *testing.T) { av := fakeAV() - av.verifyLink = func(raw string) (string, string, string, error) { - return "", "", "", assert.AnError + av.verifyLink = func(raw string) (string, string, error) { + return "", "", assert.AnError } s := newServer(t, "user:abc", av) @@ -650,9 +681,9 @@ func TestShell_BadLink_Forbidden(t *testing.T) { func TestShell_UnsignedParams_AdminPath(t *testing.T) { av := fakeAV() var verifyLinkCalled bool - av.verifyLink = func(raw string) (string, string, string, error) { + av.verifyLink = func(raw string) (string, string, error) { verifyLinkCalled = true - return "", "", "", assert.AnError + return "", "", assert.AnError } var gotArtifactID, gotSubject string av.checkView = func(ctx context.Context, artifactID, subject string) (bool, error) { @@ -669,7 +700,10 @@ func TestShell_UnsignedParams_AdminPath(t *testing.T) { assert.Equal(t, "user:admin", gotSubject, "CheckView runs with the authenticated cookie subject") assert.Equal(t, "artifact-x", gotArtifactID, "CheckView runs with the query artifact id") assert.Contains(t, rec.Body.String(), `data-app="artifact-view"`, "shell mounts the React app") - assert.Contains(t, rec.Body.String(), `"backLink":""`, "the unsigned admin path carries no back link") + // Origin links are session-derived now, not carried in the link, so the + // unsigned/durable path produces them too (the whole point of the change). + // This fake returns none, so an empty array — never null. + assert.Contains(t, rec.Body.String(), `"originLinks":[]`, "the unsigned admin path still derives origin links from the session") } // TestShell_UnsignedParams_ForbiddenStillGated confirms the unsigned path is NOT @@ -724,9 +758,9 @@ func TestShell_NoLinkNoParams_Forbidden(t *testing.T) { func TestShell_SignedPath_TakesPrecedence(t *testing.T) { av := fakeAV() var verifiedRaw string - av.verifyLink = func(raw string) (string, string, string, error) { + av.verifyLink = func(raw string) (string, string, error) { verifiedRaw = raw - return "signed-artifact", "default/s1", "", nil + return "signed-artifact", "default/s1", nil } var gotArtifactID string av.checkView = func(ctx context.Context, artifactID, subject string) (bool, error) { @@ -1003,9 +1037,9 @@ func TestUnboundSessionRef_RefusedOnEveryRoute(t *testing.T) { return []artifactview.RevisionMeta{{Seq: 1, RevisionID: "rev-1", RenderName: "ar-1"}}, nil } var touched []string - av.channelKind = func(ctx context.Context, ns, sess string) (string, error) { - touched = append(touched, "ChannelKind "+ns+"/"+sess) - return "slack", nil + av.originLinks = func(ctx context.Context, ns, sess string) []artifactview.OriginLink { + touched = append(touched, "OriginLinks "+ns+"/"+sess) + return nil } av.artifactMeta = func(ctx context.Context, ns, sess, artifactID string) (string, string, error) { touched = append(touched, "ArtifactMeta "+ns+"/"+sess) diff --git a/pkg/web/webui/artifactview/bind.go b/pkg/web/webui/artifactview/bind.go index c11d19c3..3442195f 100644 --- a/pkg/web/webui/artifactview/bind.go +++ b/pkg/web/webui/artifactview/bind.go @@ -17,7 +17,6 @@ import ( type viewBinding struct { artifactID string ns, sess string - backLink string // revisions is the binding probe's own result, carried so the first // consumer (the snapshot / the revision list) does not re-query for what // the probe already read. @@ -99,7 +98,7 @@ func denyAuthzError() *viewDenial { // refusal is logged with the full triple so an operator can tell the two apart. func bindView(ctx context.Context, av Deps, q url.Values) (viewBinding, *viewDenial) { subject := webui.SubjectFromContext(ctx) - artifactID, sessionRef, backLink, err := resolveViewParams(av, q) + artifactID, sessionRef, err := resolveViewParams(av, q) if err != nil { av.Logger().Info("artifactview: no verifiable artifact-view params; refusing", "subject", subject, "err", err.Error()) @@ -135,7 +134,7 @@ func bindView(ctx context.Context, av Deps, q url.Values) (viewBinding, *viewDen "artifactID", artifactID, "sessionRef", sessionRef, "subject", subject) return viewBinding{}, denyNoAccess() } - return viewBinding{artifactID: artifactID, ns: ns, sess: sess, backLink: backLink, revisions: revisions}, nil + return viewBinding{artifactID: artifactID, ns: ns, sess: sess, revisions: revisions}, nil } // mirrorsAllowed reports whether this subject may receive the SESSION-scoped diff --git a/pkg/web/webui/artifactview/deps.go b/pkg/web/webui/artifactview/deps.go index 284265d9..37381c8a 100644 --- a/pkg/web/webui/artifactview/deps.go +++ b/pkg/web/webui/artifactview/deps.go @@ -9,7 +9,7 @@ import ( // Deps is the artifact viewer's dependency interface. internal/cmd/webd's concrete deps // value implements it; the WebUI casts webui.Deps to this. type Deps interface { - VerifyLink(raw string) (artifactID, sessionRef, backLink string, err error) + VerifyLink(raw string) (artifactID, sessionRef string, err error) CheckView(ctx context.Context, artifactID, subject string) (bool, error) // CheckInteract is agentsession#interact — session MEMBERSHIP, which // CheckView is a strict superset of (`view = parent->interact + @@ -25,10 +25,14 @@ type Deps interface { // agent-supplied title/summary). Both empty (with nil error) when the head // has no name/description — the shell falls back to the artifact id. ArtifactMeta(ctx context.Context, ns, sess, artifactID string) (name, description string, err error) - // ChannelKind returns the AgentSession's input-channel kind (e.g. "slack"), - // used to pick the thread-link icon in the live-view toolbar. Returns "" - // (with nil error) when the kind is unknown. - ChannelKind(ctx context.Context, ns, sess string) (string, error) + // OriginLinks returns the de-duplicated, input-first set of "back to origin" + // links for the session — a GitHub pull request, a Slack thread — each + // derived from a channel binding by that kind's OriginLinkProvider. Empty for + // a kubectl-driven session, or one whose bound kinds expose no origin link. + // Never errors: a lookup failure degrades to fewer links and is logged by the + // implementation. Read at page build so it reflects whatever the session's + // bindings currently carry, rather than anything baked into a link. + OriginLinks(ctx context.Context, ns, sess string) []OriginLink // SessionViews returns the granted session_views.interactions for the // session's AgentClass — the interaction kinds a browser view of this session // may submit (e.g. "user_message"). Nil when the capability is absent, @@ -117,6 +121,18 @@ type Deps interface { Logger() logr.Logger } +// OriginLink is one "back to where this came from" link shown in the +// artifact-view toolbar: the Href to open, the Label the UI renders as its +// tooltip, and an Icon key naming the glyph. Label and Icon are supplied by the +// originating kind's OriginLinkProvider — the artifactview package and its UI +// never branch on the channel kind, they render what the kind chose. Built by +// Deps.OriginLinks from the session's channel bindings. +type OriginLink struct { + Href string `json:"href"` + Label string `json:"label"` + Icon string `json:"icon"` +} + // StatusSnapshot mirrors the session's presentation status for the live-view. type StatusSnapshot struct { Phase string `json:"phase"` // AgentSession phase, e.g. "Running" diff --git a/pkg/web/webui/artifactview/durable_link_test.go b/pkg/web/webui/artifactview/durable_link_test.go index fc14bdcd..1fc84e62 100644 --- a/pkg/web/webui/artifactview/durable_link_test.go +++ b/pkg/web/webui/artifactview/durable_link_test.go @@ -47,9 +47,9 @@ func durableLinkRequest(t *testing.T, sessionRef, artifactID string) *http.Reque func TestDurableLink_ComposedURLResolvesForAnAuthorizedViewer(t *testing.T) { av := fakeAV() var verifyLinkCalled bool - av.verifyLink = func(raw string) (string, string, string, error) { + av.verifyLink = func(raw string) (string, string, error) { verifyLinkCalled = true - return "", "", "", assert.AnError + return "", "", assert.AnError } var gotArtifactID, gotSubject string av.checkView = func(ctx context.Context, artifactID, subject string) (bool, error) { diff --git a/pkg/web/webui/artifactview/helpers.go b/pkg/web/webui/artifactview/helpers.go index 948f4da2..798542ed 100644 --- a/pkg/web/webui/artifactview/helpers.go +++ b/pkg/web/webui/artifactview/helpers.go @@ -14,7 +14,7 @@ import ( // caller maps it to the same 403 "link invalid or expired" as a bad signature. var errNoViewParams = errors.New("artifactview: no signed link and no artifactId+sessionRef params") -// resolveViewParams resolves the (artifactID, sessionRef, backLink) for an +// resolveViewParams resolves the (artifactID, sessionRef) for an // artifact-view request from EITHER of the two ways this page can be // addressed: // @@ -33,10 +33,9 @@ var errNoViewParams = errors.New("artifactview: no signed link and no artifactId // // On the signed path a bad signature is a hard error — it does NOT fall through // to the durable path. Otherwise both artifactId and sessionRef must be -// non-empty; backLink is "" for the durable path, which names no originating -// thread. Every caller still runs CheckView on the result before serving +// non-empty. Every caller still runs CheckView on the result before serving // anything. -func resolveViewParams(av Deps, q url.Values) (artifactID, sessionRef, backLink string, err error) { +func resolveViewParams(av Deps, q url.Values) (artifactID, sessionRef string, err error) { // A genuine signed link ALWAYS carries both d and sig, so require both to // select the signed branch. A stray/partial d-or-sig on an otherwise-durable // request then falls through to the artifactId/sessionRef params (still @@ -46,9 +45,9 @@ func resolveViewParams(av Deps, q url.Values) (artifactID, sessionRef, backLink } artifactID, sessionRef = q.Get("artifactId"), q.Get("sessionRef") if artifactID == "" || sessionRef == "" { - return "", "", "", errNoViewParams + return "", "", errNoViewParams } - return artifactID, sessionRef, "", nil + return artifactID, sessionRef, nil } // urlsFor mints ONE content-capability token for a render and returns both the diff --git a/pkg/web/webui/artifactview/live_test.go b/pkg/web/webui/artifactview/live_test.go index 1b8b4ad2..6e53d049 100644 --- a/pkg/web/webui/artifactview/live_test.go +++ b/pkg/web/webui/artifactview/live_test.go @@ -24,7 +24,7 @@ import ( // handler tests. Only the fields the live handler exercises need defaults; the // rest stay nil and would panic if called (none are on the live path). type liveFakeDeps struct { - verifyLink func(raw string) (string, string, string, error) + verifyLink func(raw string) (string, string, error) checkView func(ctx context.Context, artifactID, subject string) (bool, error) signContentToken func(ns, sess, renderName, artifactID string) (string, error) sandboxBaseURL func() string @@ -37,7 +37,7 @@ type liveFakeDeps struct { logger logr.Logger } -func (f *liveFakeDeps) VerifyLink(raw string) (string, string, string, error) { +func (f *liveFakeDeps) VerifyLink(raw string) (string, string, error) { return f.verifyLink(raw) } func (f *liveFakeDeps) CheckView(ctx context.Context, artifactID, subject string) (bool, error) { @@ -86,8 +86,8 @@ func (f *liveFakeDeps) PreviewChildRender(ctx context.Context, ns, sess, revID s func (f *liveFakeDeps) ArtifactMeta(ctx context.Context, ns, sess, artifactID string) (string, string, error) { return "", "", nil } -func (f *liveFakeDeps) ChannelKind(ctx context.Context, ns, sess string) (string, error) { - return "", nil +func (f *liveFakeDeps) OriginLinks(ctx context.Context, ns, sess string) []OriginLink { + return nil // the live-socket tests don't exercise origin links } func (f *liveFakeDeps) SessionViews(ctx context.Context, ns, sess string) []string { return nil // the live-socket tests don't exercise session_views @@ -150,8 +150,8 @@ func (f *liveFakeDeps) Logger() logr.Logger { return f.logger } // overrides only the fields it exercises. func liveTestAV(trustedOrigin *string) *liveFakeDeps { return &liveFakeDeps{ - verifyLink: func(raw string) (string, string, string, error) { - return "artifact-x", "default/s1", "", nil + verifyLink: func(raw string) (string, string, error) { + return "artifact-x", "default/s1", nil }, checkView: func(ctx context.Context, artifactID, subject string) (bool, error) { return true, nil @@ -390,9 +390,9 @@ func TestLive_UnsignedParams_AdminPath(t *testing.T) { var trusted string av := liveTestAV(&trusted) var verifyLinkCalled bool - av.verifyLink = func(raw string) (string, string, string, error) { + av.verifyLink = func(raw string) (string, string, error) { verifyLinkCalled = true - return "", "", "", assert.AnError + return "", "", assert.AnError } var gotArtifactID string av.checkView = func(ctx context.Context, artifactID, subject string) (bool, error) { diff --git a/pkg/web/webui/artifactview/page.go b/pkg/web/webui/artifactview/page.go index d6030d97..0db9d0c1 100644 --- a/pkg/web/webui/artifactview/page.go +++ b/pkg/web/webui/artifactview/page.go @@ -22,8 +22,10 @@ type artifactViewProps struct { HostURL string `json:"hostUrl"` ContentURL string `json:"contentUrl"` // raw render URL; empty alongside an empty HostURL SandboxOrigin string `json:"sandboxOrigin"` // bare scheme://host; "" means "no bridge" - BackLink string `json:"backLink"` // deep link back to the originating thread; "" when unsigned - ChannelKind string `json:"channelKind"` // originating channel kind for the thread icon; "" ⇒ generic + // OriginLinks are the "back to origin" links derived from the session's + // channel bindings (a GitHub PR, a Slack thread) — one per distinct origin, + // input first. Always an array (never null) so the client can map over it. + OriginLinks []OriginLink `json:"originLinks"` // Interactions lists the session_views interaction kinds this session's // class grants (e.g. ["user_message"]) — empty when the capability is // absent/inactive. The client shows the chat/annotator only when this is @@ -45,7 +47,7 @@ type artifactViewProps struct { // token, and returns props for the React shell. Failures return a // *webui.PageError so the framework renders a styled system page. // -// Every session-scoped lookup below (ChannelKind, ArtifactMeta, SessionViews, +// Every session-scoped lookup below (OriginLinks, ArtifactMeta, SessionViews, // ResolveRender) takes its (ns, sess) from the binding, so the shell's props — // including the Ns/Name the client stamps onto its /interact posts — can only // ever name the session the artifact actually lives in. @@ -56,14 +58,22 @@ func shellPageBuild(av Deps) func(context.Context, *http.Request) (any, webui.Pa return nil, webui.PageMeta{}, denial.pageError() } artifactID, ns, sess, sessionRef := b.artifactID, b.ns, b.sess, b.sessionRef() - // Best-effort: the thread-link icon depends on the originating channel - // kind. A lookup failure is non-fatal — the client falls back to a - // generic icon when channelKind is "". - channelKind, ckErr := av.ChannelKind(ctx, ns, sess) - if ckErr != nil { - av.Logger().Error(ckErr, "artifactview page: ChannelKind errored; falling back to generic icon", - "sessionRef", sessionRef) - channelKind = "" + // "Back to origin" links derived from the session's channel bindings are + // SESSION-scoped data — a private Slack thread permalink (team/channel + // IDs, the fact a DM exists), a private-repo PR URL — so they are gated + // exactly like the other session mirrors: only a session PARTICIPANT + // (CheckInteract, via mirrorsAllowed) sees them. CheckView admitted this + // page, but it is a strict superset of interact, so a platform admin or, + // on an org-visible session, any authenticated user reaches it without + // being in the conversation — and must not be handed the origin. A + // best-effort lookup (OriginLinks never errors), coerced to a non-nil + // slice so the bootstrap JSON is always an array the client can map over. + var originLinks []OriginLink + if b.mirrorsAllowed(ctx, av, webui.SubjectFromContext(ctx)) { + originLinks = av.OriginLinks(ctx, ns, sess) + } + if originLinks == nil { + originLinks = []OriginLink{} } title, description := artifactID, "" if name, desc, merr := av.ArtifactMeta(ctx, ns, sess, artifactID); merr != nil { @@ -112,8 +122,7 @@ func shellPageBuild(av Deps) func(context.Context, *http.Request) (any, webui.Pa // it must be the bare scheme://host — and "" when the configured value // is not one, which the client reads as "no bridge", not "any origin". SandboxOrigin: sandboxOrigin(av), - BackLink: b.backLink, - ChannelKind: channelKind, + OriginLinks: originLinks, Interactions: interactions, Ns: ns, Name: sess, diff --git a/pkg/web/webui/artifactview/revision_test.go b/pkg/web/webui/artifactview/revision_test.go index ea0cc7eb..3fb47a4c 100644 --- a/pkg/web/webui/artifactview/revision_test.go +++ b/pkg/web/webui/artifactview/revision_test.go @@ -73,9 +73,9 @@ func TestRevision_HappyPath_Returns200WithContentURL(t *testing.T) { func TestRevision_UnsignedParams_AdminPath(t *testing.T) { av := fakeAVWithRevisions() var verifyLinkCalled bool - av.verifyLink = func(raw string) (string, string, string, error) { + av.verifyLink = func(raw string) (string, string, error) { verifyLinkCalled = true - return "", "", "", assert.AnError + return "", "", assert.AnError } var gotArtifactID, gotSubject string av.checkView = func(ctx context.Context, artifactID, subject string) (bool, error) { @@ -123,8 +123,8 @@ func TestRevision_CheckViewFalse_Forbidden(t *testing.T) { func TestRevision_BadLink_Forbidden(t *testing.T) { av := fakeAVWithRevisions() - av.verifyLink = func(raw string) (string, string, string, error) { - return "", "", "", assert.AnError + av.verifyLink = func(raw string) (string, string, error) { + return "", "", assert.AnError } s := newServer(t, "user:abc", av) diff --git a/pkg/web/webui/artifactview/ui/ArtifactView.test.tsx b/pkg/web/webui/artifactview/ui/ArtifactView.test.tsx index b77aafa2..b2645eac 100644 --- a/pkg/web/webui/artifactview/ui/ArtifactView.test.tsx +++ b/pkg/web/webui/artifactview/ui/ArtifactView.test.tsx @@ -32,8 +32,7 @@ const baseProps = { hostUrl: "", contentUrl: "", sandboxOrigin: "https://example.com", - backLink: "/thread/1", - channelKind: "slack", + originLinks: [] as { href: string; label: string; icon: string }[], ns: "default", name: "sess-1", artifactId: "art-1", @@ -217,3 +216,29 @@ describe("ArtifactView – chat panel gating", () => { vi.unstubAllGlobals(); }); }); + +describe("ArtifactView – origin links toolbar", () => { + it("renders one external link per origin, with the right hrefs", () => { + const { container } = render( + , + ); + // ThreadLink is the only toolbar anchor opened in a new tab (Download uses + // the download attribute, not target=_blank). + const links = Array.from(container.querySelectorAll('a[target="_blank"]')) as HTMLAnchorElement[]; + expect(links.map((a) => a.getAttribute("href"))).toEqual([ + "https://github.com/demo-org/platform/pull/42", + "https://example.slack.com/archives/C1/p170", + ]); + }); + + it("renders no origin link when there are none", () => { + const { container } = render(); + expect(container.querySelectorAll('a[target="_blank"]').length).toBe(0); + }); +}); diff --git a/pkg/web/webui/artifactview/ui/ArtifactView.tsx b/pkg/web/webui/artifactview/ui/ArtifactView.tsx index 6ecfde9f..1db0ccc1 100644 --- a/pkg/web/webui/artifactview/ui/ArtifactView.tsx +++ b/pkg/web/webui/artifactview/ui/ArtifactView.tsx @@ -277,7 +277,9 @@ export function ArtifactView(props: ArtifactViewProps) { > - + {props.originLinks.map((l) => ( + + ))} {pinned && (