diff --git a/acceptance/testdata/telemetry/no-telemetry-for-extension.txtar b/acceptance/testdata/telemetry/no-telemetry-for-extension.txtar index b87b3e252ed..fcb820e0c2a 100644 --- a/acceptance/testdata/telemetry/no-telemetry-for-extension.txtar +++ b/acceptance/testdata/telemetry/no-telemetry-for-extension.txtar @@ -1,4 +1,6 @@ -# Extensions should not generate telemetry events +# Third-party extensions must not generate telemetry events, since the +# extension command name can be a user-authored identifier (e.g. an +# organization or project name). [!exec:bash] skip env GH_PRIVATE_ENABLE_TELEMETRY=1 diff --git a/acceptance/testdata/telemetry/telemetry-for-official-extension-stub.txtar b/acceptance/testdata/telemetry/telemetry-for-official-extension-stub.txtar new file mode 100644 index 00000000000..c64739af45d --- /dev/null +++ b/acceptance/testdata/telemetry/telemetry-for-official-extension-stub.txtar @@ -0,0 +1,20 @@ +# Official extension stubs (the hidden commands suggesting installation of +# GitHub-owned extensions) are safe to report via telemetry: their command +# names come from a fixed, hard-coded registry and do not contain any +# user-authored identifiers. + +env GH_PRIVATE_ENABLE_TELEMETRY=1 +env GH_TELEMETRY=log +env GH_TELEMETRY_SAMPLE_RATE=100 + +# `stack` is registered in extensions.OfficialExtensions. Since no real +# extension is installed, the hidden stub runs and, in a non-TTY session, +# prints install instructions without prompting. +exec gh stack +stderr 'gh extension install github/gh-stack' + +# The stub invocation records a command_invocation event for the stub's +# command path. +stderr 'Telemetry payload:' +stderr '"type": "command_invocation"' +stderr '"command": "gh stack"' diff --git a/pkg/cmd/root/extension.go b/pkg/cmd/root/extension.go index 45a60332b8d..c432290aa49 100644 --- a/pkg/cmd/root/extension.go +++ b/pkg/cmd/root/extension.go @@ -79,7 +79,14 @@ func NewCmdExtension(io *iostreams.IOStreams, em extensions.ExtensionManager, ex } cmdutil.DisableAuthCheck(cmd) - cmdutil.DisableTelemetry(cmd) + // Extensions are user-installed and their names can be arbitrary + // (potentially including sensitive identifiers such as project or + // organization names), so we must not record telemetry for them by + // default. Official GitHub-owned extensions are a known, fixed set and + // can safely contribute their command name to telemetry. + if !extensions.IsOfficial(ext.Name(), ext.Owner()) { + cmdutil.DisableTelemetry(cmd) + } return cmd } diff --git a/pkg/cmd/root/extension_registration_test.go b/pkg/cmd/root/extension_registration_test.go index 828f51ca066..61d73b1b9b9 100644 --- a/pkg/cmd/root/extension_registration_test.go +++ b/pkg/cmd/root/extension_registration_test.go @@ -57,6 +57,9 @@ func TestNewCmdRoot_ExtensionRegistration(t *testing.T) { NameFunc: func() string { return extName }, + OwnerFunc: func() string { + return "" + }, }) } diff --git a/pkg/cmd/root/extension_test.go b/pkg/cmd/root/extension_test.go index 5e9e9b9bcf5..9bb5037a593 100644 --- a/pkg/cmd/root/extension_test.go +++ b/pkg/cmd/root/extension_test.go @@ -144,6 +144,9 @@ func TestNewCmdExtension_Updates(t *testing.T) { NameFunc: func() string { return tt.extName }, + OwnerFunc: func() string { + return "" + }, UpdateAvailableFunc: func() bool { return tt.extUpdateAvailable }, @@ -199,6 +202,9 @@ func TestNewCmdExtension_UpdateCheckIsNonblocking(t *testing.T) { NameFunc: func() string { return "major-update" }, + OwnerFunc: func() string { + return "" + }, UpdateAvailableFunc: func() bool { return true }, @@ -234,3 +240,60 @@ func TestNewCmdExtension_UpdateCheckIsNonblocking(t *testing.T) { t.Fatal("extension update check should have exited") } } + +func TestNewCmdExtension_TelemetryEnabledForOfficialExtensions(t *testing.T) { + tests := []struct { + name string + extName string + extOwner string + wantTelemetryOff bool + }{ + { + name: "official extension records telemetry", + extName: "stack", + extOwner: "github", + wantTelemetryOff: false, + }, + { + name: "official name with third-party owner disables telemetry", + extName: "stack", + extOwner: "williammartin", + wantTelemetryOff: true, + }, + { + name: "official name with empty owner disables telemetry", + extName: "stack", + extOwner: "", + wantTelemetryOff: true, + }, + { + name: "official extension name with mixed case disables telemetry", + extName: "STACK", + extOwner: "github", + wantTelemetryOff: true, + }, + { + name: "third-party extension disables telemetry", + extName: "my-custom-ext", + extOwner: "someone", + wantTelemetryOff: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + em := &extensions.ExtensionManagerMock{} + ext := &extensions.ExtensionMock{ + NameFunc: func() string { return tt.extName }, + OwnerFunc: func() string { return tt.extOwner }, + } + + cmd := root.NewCmdExtension(ios, em, ext, func(extensions.ExtensionManager, extensions.Extension) (*update.ReleaseInfo, error) { + return nil, nil + }) + + assert.Equal(t, tt.wantTelemetryOff, cmd.Annotations["telemetry"] == "disabled") + }) + } +} diff --git a/pkg/extensions/official.go b/pkg/extensions/official.go index a07c426df91..dc6bdc919b0 100644 --- a/pkg/extensions/official.go +++ b/pkg/extensions/official.go @@ -1,6 +1,8 @@ package extensions import ( + "strings" + "github.com/cli/cli/v2/internal/ghrepo" ) @@ -24,3 +26,28 @@ var OfficialExtensions = []OfficialExtension{ {Name: "aw", Owner: "github", Repo: "gh-aw"}, {Name: "stack", Owner: "github", Repo: "gh-stack"}, } + +// IsOfficial reports whether the given extension command name and owner +// match an entry in the OfficialExtensions registry. Owner must be +// checked alongside name because a user may have installed a third-party +// extension that happens to share a name with one of ours (e.g. +// `someuser/gh-stack` predates `github/gh-stack` becoming official). +// Owner will be empty for local extensions, in which case the extension +// is treated as non-official. +// +// Comparison is case-sensitive: on case-sensitive filesystems a user can +// install a private extension whose name differs only in casing (e.g. +// `gh-STACK`), and we must not treat that as official. Owner comparison +// is case-insensitive because GitHub usernames and organization names +// are themselves case-insensitive. +func IsOfficial(name, owner string) bool { + if owner == "" { + return false + } + for _, ext := range OfficialExtensions { + if ext.Name == name && strings.EqualFold(ext.Owner, owner) { + return true + } + } + return false +} diff --git a/pkg/extensions/official_test.go b/pkg/extensions/official_test.go index 047af580af1..6d16ece2cf9 100644 --- a/pkg/extensions/official_test.go +++ b/pkg/extensions/official_test.go @@ -13,3 +13,61 @@ func TestOfficialExtension_Repository(t *testing.T) { assert.Equal(t, "gh-stack", repo.RepoName()) assert.Equal(t, "github.com", repo.RepoHost()) } + +func TestIsOfficial(t *testing.T) { + tests := []struct { + name string + extName string + extOwner string + want bool + }{ + { + name: "known official extension matches", + extName: "stack", + extOwner: "github", + want: true, + }, + { + name: "official name with different owner is not official", + extName: "stack", + extOwner: "williammartin", + want: false, + }, + { + name: "official name with empty owner is not official", + extName: "stack", + extOwner: "", + want: false, + }, + { + name: "owner comparison is case-insensitive", + extName: "stack", + extOwner: "GitHub", + want: true, + }, + { + name: "mixed-case name does not match", + extName: "STACK", + extOwner: "github", + want: false, + }, + { + name: "unknown name is not official", + extName: "not-a-real-extension", + extOwner: "github", + want: false, + }, + { + name: "empty name is not official", + extName: "", + extOwner: "github", + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, IsOfficial(tt.extName, tt.extOwner)) + }) + } +}