Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 51 additions & 60 deletions client.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
"sort"
"strings"
"sync"
"sync/atomic"
"time"

"github.com/getsentry/sentry-go/internal/debug"
Expand Down Expand Up @@ -294,12 +295,13 @@ type ClientOptions struct {
type Client struct {
mu sync.RWMutex
options ClientOptions
dsn *Dsn
dsn *protocol.Dsn
eventProcessors []EventProcessor
integrations []Integration
externalTraceResolver externalContextTraceResolver
sdkIdentifier string
sdkVersion string
sdkInfo atomic.Pointer[protocol.SdkInfo]
// Transport is read-only. Replacing the transport of an existing client is
// not supported, create a new client instead.
Transport Transport
Expand Down Expand Up @@ -395,10 +397,10 @@ func NewClient(options ClientOptions) (*Client, error) {
}
}

var dsn *Dsn
var dsn *protocol.Dsn
if options.Dsn != "" {
var err error
dsn, err = NewDsn(options.Dsn)
dsn, err = protocol.NewDsn(options.Dsn)
if err != nil {
return nil, err
}
Expand All @@ -412,32 +414,37 @@ func NewClient(options ClientOptions) (*Client, error) {
reportRecorder: report.NoopRecorder(),
reportProvider: report.NoopProvider(),
}
client.refreshSDKInfo()

if !options.DisableClientReports {
a := report.NewAggregator()
client.reportRecorder = a
client.reportProvider = a
}

client.setupTransport()

// noop Telemetry Buffers and Processor fow now
// if !options.DisableTelemetryBuffer {
// client.setupTelemetryProcessor()
// } else
if options.EnableLogs {
client.batchLogger = newLogBatchProcessor(&client)
client.batchLogger.Start()
}
// We currently disallow using custom Transport with the new Telemetry Processor, due to the difference in transport signatures.
// The option should be enabled when the new Transport interface signature changes.
if !options.DisableTelemetryBuffer && client.options.Transport == nil {
client.setupTelemetryProcessor()
Comment thread
sentry[bot] marked this conversation as resolved.
} else {
if client.options.Transport != nil {
debuglog.Println("Cannot enable Telemetry Processor with custom Transport: fallback to old transport")
}
client.setupTransport()
client.setupIntegrations()

if !options.DisableMetrics {
client.batchMeter = newMetricBatchProcessor(&client)
client.batchMeter.Start()
if options.EnableLogs {
client.batchLogger = newLogBatchProcessor(&client)
client.batchLogger.Start()
}
if !options.DisableMetrics {
client.batchMeter = newMetricBatchProcessor(&client)
client.batchMeter.Start()
}
}
if options.OrgID != 0 && client.dsn != nil {
client.dsn.SetOrgID(options.OrgID)
}
client.setupIntegrations()

return &client, nil
}
Expand Down Expand Up @@ -467,38 +474,15 @@ func (client *Client) setupTransport() {
case *internalAsyncTransportAdapter:
tr.recorder = client.reportRecorder
tr.provider = client.reportProvider
tr.sdkInfo = &client.sdkInfo
}
}

transport.Configure(opts)
client.Transport = transport
}

func (client *Client) setupTelemetryProcessor() { // nolint: unused
if client.options.DisableTelemetryBuffer {
return
}

if client.dsn == nil {
debuglog.Println("Telemetry buffer disabled: no DSN configured")
return
}

// We currently disallow using custom Transport with the new Telemetry Processor, due to the difference in transport signatures.
// The option should be enabled when the new Transport interface signature changes.
if client.options.Transport != nil {
debuglog.Println("Cannot enable Telemetry Processor/Buffers with custom Transport: fallback to old transport")
if client.options.EnableLogs {
client.batchLogger = newLogBatchProcessor(client)
client.batchLogger.Start()
}
if !client.options.DisableMetrics {
client.batchMeter = newMetricBatchProcessor(client)
client.batchMeter.Start()
}
return
}

func (client *Client) setupTelemetryProcessor() {
transport := httpInternal.NewAsyncTransport(httpInternal.TransportOptions{
Dsn: client.options.Dsn,
HTTPClient: client.options.HTTPClient,
Expand All @@ -508,8 +492,9 @@ func (client *Client) setupTelemetryProcessor() { // nolint: unused
CaCerts: client.options.CaCerts,
Recorder: client.reportRecorder,
Provider: client.reportProvider,
SdkInfo: &client.sdkInfo,
})
client.Transport = &internalAsyncTransportAdapter{transport: transport}
client.Transport = &internalAsyncTransportAdapter{transport: transport, sdkInfo: &client.sdkInfo}

buffers := map[ratelimit.Category]telemetry.Buffer[protocol.TelemetryItem]{
ratelimit.CategoryError: telemetry.NewRingBuffer[protocol.TelemetryItem](ratelimit.CategoryError, 100, telemetry.OverflowPolicyDropOldest, 1, 0, client.reportRecorder),
Expand All @@ -519,12 +504,8 @@ func (client *Client) setupTelemetryProcessor() { // nolint: unused
ratelimit.CategoryTraceMetric: telemetry.NewRingBuffer[protocol.TelemetryItem](ratelimit.CategoryTraceMetric, 10*100, telemetry.OverflowPolicyDropOldest, 100, 5*time.Second, client.reportRecorder),
}

sdkInfo := &protocol.SdkInfo{
Name: client.sdkIdentifier,
Version: client.sdkVersion,
}

client.telemetryProcessor = telemetry.NewProcessor(buffers, transport, &client.dsn.Dsn, sdkInfo, client.reportRecorder)
client.setupIntegrations()
client.telemetryProcessor = telemetry.NewProcessor(buffers, transport, client.dsn, &client.sdkInfo, client.reportRecorder)
}

func (client *Client) setupIntegrations() {
Expand Down Expand Up @@ -554,6 +535,7 @@ func (client *Client) setupIntegrations() {
sort.Slice(client.integrations, func(i, j int) bool {
return client.integrations[i].Name() < client.integrations[j].Name()
})
client.refreshSDKInfo()
}

// AddEventProcessor adds an event processor to the client. It must not be
Expand Down Expand Up @@ -874,9 +856,10 @@ func (client *Client) EventFromCheckIn(checkIn *CheckIn, monitorConfig *MonitorC

func (client *Client) SetSDKIdentifier(identifier string) {
client.mu.Lock()
defer client.mu.Unlock()

client.sdkIdentifier = identifier
client.mu.Unlock()

client.refreshSDKInfo()
}

func (client *Client) GetSDKIdentifier() string {
Expand Down Expand Up @@ -982,15 +965,7 @@ func (client *Client) prepareEvent(event *Event, hint *EventHint, scope EventMod
}

event.Platform = "go"
event.Sdk = SdkInfo{
Name: client.GetSDKIdentifier(),
Version: SDKVersion,
Integrations: client.listIntegrations(),
Packages: []SdkPackage{{
Name: "sentry-go",
Version: SDKVersion,
}},
}
event.Sdk = *client.sdkInfo.Load()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: A Client created via a struct literal can cause a nil pointer dereference in prepareEvent because client.sdkInfo is not initialized.
Severity: LOW

Suggested Fix

In prepareEvent, add a nil check after calling client.sdkInfo.Load() and before dereferencing the pointer. If the loaded value is nil, you should handle it gracefully, for example by skipping the event.Sdk assignment.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: client.go#L968

Potential issue: In `prepareEvent`, the code dereferences the result of
`client.sdkInfo.Load()` without a preceding nil check. If a `Client` is instantiated
directly as a struct literal (e.g., `&Client{...}`) instead of through the `NewClient()`
constructor, the `sdkInfo` atomic pointer is never initialized and remains `nil`. A
subsequent call to an event capture method on this client, which in turn calls
`prepareEvent`, will cause a panic due to a nil pointer dereference. While this is an
unsupported usage pattern not currently found in production paths, it represents a
latent vulnerability.


if scope != nil {
event = scope.ApplyToEvent(event, hint, client)
Expand Down Expand Up @@ -1061,6 +1036,22 @@ func (client *Client) integrationAlreadyInstalled(name string) bool {
return false
}

func (client *Client) refreshSDKInfo() {
client.mu.RLock()
info := protocol.SdkInfo{
Name: client.sdkIdentifier,
Version: client.sdkVersion,
Integrations: client.listIntegrations(),
Packages: []protocol.SdkPackage{{
Name: "sentry-go",
Version: client.sdkVersion,
}},
}
client.mu.RUnlock()

client.sdkInfo.Store(&info)
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Race in refreshSDKInfo can store stale SDK info

Low Severity

refreshSDKInfo reads sdkIdentifier under mu.RLock() but calls sdkInfo.Store() after releasing the lock. If two concurrent SetSDKIdentifier calls interleave — where thread 2 completes its full set-and-refresh cycle between thread 1's RUnlock and Store — thread 1's Store overwrites thread 2's newer value with a stale one. The sdkInfo atomic pointer then holds an outdated name while sdkIdentifier reflects the latest. Moving the Store call inside the RLock section would close this window.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 7786612. Configure here.


// sample returns true with the given probability, which must be in the range
// [0.0, 1.0].
func sample(probability float64) bool {
Expand Down
34 changes: 21 additions & 13 deletions client_reports_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,28 @@ import (
"net/http"
"net/http/httptest"
"strings"
"sync"
"testing"
"time"

"github.com/getsentry/sentry-go/internal/ratelimit"
"github.com/getsentry/sentry-go/internal/testutils"
"github.com/getsentry/sentry-go/report"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
"github.com/stretchr/testify/require"
)

// TestClientReports_Integration tests that client reports are properly generated
// and sent when events are dropped for various reasons.
func TestClientReports_Integration(t *testing.T) {
var receivedBodies [][]byte
var mu sync.Mutex
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
body, _ := io.ReadAll(r.Body)
mu.Lock()
receivedBodies = append(receivedBodies, body)
mu.Unlock()
w.WriteHeader(http.StatusOK)
_, _ = w.Write([]byte(`{"id":"test-event-id"}`))
}))
Expand Down Expand Up @@ -73,21 +79,23 @@ func TestClientReports_Integration(t *testing.T) {
}

var got report.ClientReport
found := false
for _, b := range receivedBodies {
for _, line := range bytes.Split(b, []byte("\n")) {
if json.Unmarshal(line, &got) == nil && len(got.DiscardedEvents) > 0 {
found = true
break
require.Eventually(t, func() bool {
mu.Lock()
bodies := make([][]byte, len(receivedBodies))
copy(bodies, receivedBodies)
mu.Unlock()

for _, b := range bodies {
for _, line := range bytes.Split(b, []byte("\n")) {
var report report.ClientReport
if json.Unmarshal(line, &report) == nil && len(report.DiscardedEvents) > 0 {
got = report
return true
}
}
}
if found {
break
}
}
if !found {
t.Fatal("no client report found in envelope bodies")
}
return false
}, time.Second, 10*time.Millisecond, "no client report found in envelope bodies with: %v", got)

if got.Timestamp.IsZero() {
t.Error("client report missing timestamp")
Expand Down
68 changes: 60 additions & 8 deletions client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,19 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"net"
"net/http"
"net/http/httptest"
"strings"
"sync"
"sync/atomic"
"testing"
"time"

"github.com/getsentry/sentry-go/internal/debuglog"
internalHttp "github.com/getsentry/sentry-go/internal/http"
"github.com/getsentry/sentry-go/internal/testutils"
"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
pkgErrors "github.com/pkg/errors"
Expand Down Expand Up @@ -1063,9 +1068,59 @@ func TestClientSetsUpTransport(t *testing.T) {
Transport: &MockTransport{},
})
require.IsType(t, &MockTransport{}, client.Transport)
}

type namedIntegration struct{ name string }

func (n *namedIntegration) Name() string { return n.name }
func (n *namedIntegration) SetupOnce(_ *Client) {}

func TestTelemetryEnvelopeCarriesIntegrations(t *testing.T) {
var (
mu sync.Mutex
bodies [][]byte
)
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
b, err := io.ReadAll(r.Body)
require.NoError(t, err)
mu.Lock()
bodies = append(bodies, b)
mu.Unlock()
w.WriteHeader(http.StatusOK)
}))
defer srv.Close()

dsn := strings.Replace(srv.URL, "//", "//pubkey@", 1) + "/1"
client, err := NewClient(ClientOptions{
Dsn: dsn,
Integrations: func(defaults []Integration) []Integration {
return append(defaults, &namedIntegration{name: "CustomRegressionIntegration"})
},
})
require.NoError(t, err)
t.Cleanup(func() { client.Close() })

client.CaptureMessage("ping", nil, &MockScope{})
require.True(t, client.Flush(testutils.FlushTimeout()), "flush timed out")

mu.Lock()
defer mu.Unlock()
require.NotEmpty(t, bodies, "expected at least one envelope to be sent")

body := bodies[0]
nl := bytes.IndexByte(body, '\n')
require.Positive(t, nl, "envelope body missing header newline")
var header struct {
Sdk struct {
Name string `json:"name"`
Integrations []string `json:"integrations"`
} `json:"sdk"`
}
require.NoError(t, json.Unmarshal(body[:nl], &header))

client, _ = NewClient(ClientOptions{})
require.IsType(t, &noopTransport{}, client.Transport)
assert.Equal(t, sdkIdentifier, header.Sdk.Name)
assert.Contains(t, header.Sdk.Integrations, "CustomRegressionIntegration")
assert.Contains(t, header.Sdk.Integrations, "ContextifyFrames")
}

func TestClient_SetupTelemetryBuffer_NoDSN(t *testing.T) {
Expand All @@ -1078,13 +1133,10 @@ func TestClient_SetupTelemetryBuffer_NoDSN(t *testing.T) {
t.Fatalf("unexpected error: %v", err)
}

if client.telemetryProcessor != nil {
t.Fatal("expected telemetryProcessor to be nil when DSN is missing")
}

if _, ok := client.Transport.(*noopTransport); !ok {
t.Fatalf("expected noopTransport, got %T", client.Transport)
if client.telemetryProcessor == nil {
t.Fatal("expected telemetryProcessor to not be nil when DSN is missing")
}
require.IsType(t, &internalHttp.NoopTransport{}, client.Transport.(*internalAsyncTransportAdapter).transport)
}

type multiClientEnv struct {
Expand Down
3 changes: 2 additions & 1 deletion dynamic_sampling_context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"testing"

"github.com/getsentry/sentry-go/internal/protocol"
"github.com/getsentry/sentry-go/internal/testutils"
)

Expand Down Expand Up @@ -192,7 +193,7 @@ func TestDynamicSamplingContextFromScope(t *testing.T) {
},
},
client: func() *Client {
dsn, _ := NewDsn("http://public@example.com/sentry/1")
dsn, _ := protocol.NewDsn("http://public@example.com/sentry/1")
return &Client{
options: ClientOptions{
Dsn: dsn.String(),
Expand Down
Loading
Loading