Skip to content

Commit dd2b355

Browse files
marandanetomarandaneto
authored
fix: avoid mutating caller property dictionaries (#267)
* fix: avoid mutating caller properties * ci: fix semgrep workflow parsing * address pr review feedback * fix: support dictionary copies on netstandard2.0
1 parent 2c2d89f commit dd2b355

10 files changed

Lines changed: 207 additions & 26 deletions

‎.changeset/calm-hedgehogs-copy.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
---
2+
"PostHog": patch
3+
"PostHog.AspNetCore": patch
4+
"PostHog.AI": patch
5+
---
6+
7+
Avoid mutating caller-provided property dictionaries when capturing events, capturing exceptions, and using identify, group identify, page view, screen view, or survey capture helpers.

‎src/PostHog/Api/CapturedEvent.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,7 @@ public CapturedEvent(
2727
DistinctId = distinctId;
2828
Timestamp = timestamp;
2929

30-
Properties = properties ?? new Dictionary<string, object>();
30+
Properties = properties?.Copy() ?? new Dictionary<string, object>();
3131

3232
// Every event has to have these properties.
3333
Properties[PostHogProperties.DistinctId] = distinctId; // See `get_distinct_id` in PostHog/posthog api/capture.py line 321

‎src/PostHog/Capture/CaptureExtensions.cs‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -263,9 +263,9 @@ public static bool Capture(
263263
Dictionary<string, object> personPropertiesToSet,
264264
Dictionary<string, object> personPropertiesToSetOnce)
265265
{
266-
properties ??= new Dictionary<string, object>();
267-
properties["$set"] = personPropertiesToSet;
268-
properties["$set_once"] = personPropertiesToSetOnce;
266+
properties = properties?.Copy() ?? new Dictionary<string, object>();
267+
properties["$set"] = personPropertiesToSet.Copy();
268+
properties["$set_once"] = personPropertiesToSetOnce.Copy();
269269

270270
return NotNull(client).Capture(
271271
distinctId,
@@ -474,7 +474,7 @@ public static bool CaptureSurveyResponses(
474474
IReadOnlyList<string> surveyResponses,
475475
Dictionary<string, object>? properties)
476476
{
477-
properties ??= new Dictionary<string, object>();
477+
properties = properties?.Copy() ?? new Dictionary<string, object>();
478478
properties["$survey_id"] = surveyId;
479479

480480
if (NotNull(surveyResponses).Count > 0)
@@ -542,7 +542,7 @@ static bool CaptureSpecialEvent(
542542
Dictionary<string, object>? properties,
543543
bool sendFeatureFlags = false)
544544
{
545-
properties ??= new Dictionary<string, object>();
545+
properties = properties?.Copy() ?? new Dictionary<string, object>();
546546
properties[eventPropertyName] = eventPropertyValue;
547547
if (!sendFeatureFlags)
548548
{

‎src/PostHog/Extensions/GroupIdentifyAsyncExtensions.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
using PostHog.Api;
22
using PostHog.Json;
3+
using PostHog.Library;
34
using static PostHog.Library.Ensure;
45

56
namespace PostHog; // Intentionally put in the root namespace.
@@ -141,7 +142,7 @@ static async Task<ApiResult> GroupIdentifyWithNameAsync(
141142
Dictionary<string, object>? properties,
142143
CancellationToken cancellationToken)
143144
{
144-
properties ??= new Dictionary<string, object>();
145+
properties = properties?.Copy() ?? new Dictionary<string, object>();
145146
properties["name"] = name;
146147

147148
return distinctId is null

‎src/PostHog/Extensions/IdentifyPersonAsyncExtensions.cs‎

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
using PostHog.Api;
2+
using PostHog.Library;
23
using static PostHog.Library.Ensure;
34

45
namespace PostHog; // Intentionally put in the root namespace.
@@ -192,16 +193,21 @@ public static async Task<ApiResult> IdentifyAsync(
192193
Dictionary<string, object>? personPropertiesToSetOnce,
193194
CancellationToken cancellationToken)
194195
{
195-
if (email is not null)
196+
if (email is not null || name is not null)
196197
{
197-
personPropertiesToSet ??= new Dictionary<string, object>();
198-
personPropertiesToSet["email"] = email;
199-
}
198+
var enrichedProperties = personPropertiesToSet?.Copy() ?? new Dictionary<string, object>();
200199

201-
if (name is not null)
202-
{
203-
personPropertiesToSet ??= new Dictionary<string, object>();
204-
personPropertiesToSet["name"] = name;
200+
if (email is not null)
201+
{
202+
enrichedProperties["email"] = email;
203+
}
204+
205+
if (name is not null)
206+
{
207+
enrichedProperties["name"] = name;
208+
}
209+
210+
personPropertiesToSet = enrichedProperties;
205211
}
206212

207213
return await NotNull(client).IdentifyAsync(distinctId,

‎src/PostHog/Library/CollectionExtensions.cs‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,17 @@ public static IReadOnlyDictionary<TKey, TValue> ToReadOnlyDictionary<TKey, TValu
3737
Func<TItem, TValue> valueSelector) where TKey : notnull
3838
=> new ReadOnlyDictionary<TKey, TValue>(enumerable.ToDictionary(keySelector, valueSelector));
3939

40+
/// <summary>
41+
/// Creates a shallow copy of a dictionary.
42+
/// </summary>
43+
/// <param name="dictionary">The dictionary to copy.</param>
44+
/// <typeparam name="TKey">The key type.</typeparam>
45+
/// <typeparam name="TValue">The value type.</typeparam>
46+
/// <returns>A new dictionary containing the same keys and values.</returns>
47+
public static Dictionary<TKey, TValue> Copy<TKey, TValue>(
48+
this IReadOnlyDictionary<TKey, TValue> dictionary) where TKey : notnull
49+
=> dictionary.ToDictionary(pair => pair.Key, pair => pair.Value);
50+
4051
/// <summary>
4152
/// Similar to Python's hash merging, this method merges the contents of one dictionary into another.
4253
/// The values of the other dictionary will overwrite the values of the original dictionary.

‎src/PostHog/PostHogClient.cs‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,6 +310,8 @@ bool CaptureCore(
310310
return false;
311311
}
312312

313+
properties = properties?.Copy();
314+
313315
// If custom timestamp provided, add it to properties
314316
if (timestamp.HasValue)
315317
{
@@ -513,7 +515,7 @@ bool CaptureExceptionCore(
513515
try
514516
{
515517
var host = _options.Value.HostUrl.ToString().TrimEnd('/').Replace(".i.", ".", StringComparison.Ordinal);
516-
properties ??= [];
518+
properties = properties?.Copy() ?? [];
517519
var identity = PostHogContextHelper.ResolveIdentity(distinctId, PostHogContext.Current);
518520
if (identity.IsPersonless && !properties.ContainsKey(PostHogProperties.ProcessPersonProfile))
519521
{

‎tests/UnitTests/Api/CapturedEventTests.cs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,10 @@ public void PreservesProvidedProperties()
7171
};
7272
var capturedEvent = new CapturedEvent("test-event", "user-1", properties, DateTimeOffset.UtcNow);
7373

74+
Assert.NotSame(properties, capturedEvent.Properties);
75+
Assert.Equal(2, properties.Count);
76+
Assert.Equal("custom_value", properties["custom_prop"]);
77+
Assert.Equal(42, properties["number_prop"]);
7478
Assert.Equal("custom_value", capturedEvent.Properties["custom_prop"]);
7579
Assert.Equal(42, capturedEvent.Properties["number_prop"]);
7680
}

‎tests/UnitTests/CaptureExtensionsTests.cs‎

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,97 @@
1+
using NSubstitute;
2+
using PostHog;
3+
using PostHog.Features;
4+
5+
namespace CaptureExtensionsTests;
6+
7+
public class TheCaptureExtensions
8+
{
9+
[Fact]
10+
public void CaptureWithPersonPropertiesDoesNotMutateProvidedProperties()
11+
{
12+
var client = Substitute.For<IPostHogClient>();
13+
var properties = new Dictionary<string, object> { ["source"] = "test" };
14+
var personPropertiesToSet = new Dictionary<string, object> { ["name"] = "Max" };
15+
var personPropertiesToSetOnce = new Dictionary<string, object> { ["initial_url"] = "/blog" };
16+
17+
client.Capture(
18+
"distinct-id",
19+
"event",
20+
properties,
21+
personPropertiesToSet,
22+
personPropertiesToSetOnce);
23+
24+
Assert.Equal(new Dictionary<string, object> { ["source"] = "test" }, properties);
25+
client.Received(1).Capture(
26+
"distinct-id",
27+
"event",
28+
Arg.Is<Dictionary<string, object>>(captured => HasCopiedPersonProperties(
29+
captured,
30+
properties,
31+
personPropertiesToSet,
32+
personPropertiesToSetOnce)),
33+
groups: null,
34+
flags: (FeatureFlagEvaluations?)null,
35+
timestamp: null);
36+
}
37+
38+
[Fact]
39+
public void CapturePageViewDoesNotMutateProvidedProperties()
40+
{
41+
var client = Substitute.For<IPostHogClient>();
42+
var properties = new Dictionary<string, object> { ["source"] = "test" };
43+
44+
client.CapturePageView("distinct-id", "/pricing", properties);
45+
46+
Assert.Equal(new Dictionary<string, object> { ["source"] = "test" }, properties);
47+
client.Received(1).Capture(
48+
"distinct-id",
49+
"$pageview",
50+
Arg.Is<Dictionary<string, object>>(captured =>
51+
!ReferenceEquals(captured, properties)
52+
&& (string)captured["source"] == "test"
53+
&& (string)captured["$current_url"] == "/pricing"),
54+
groups: null,
55+
flags: (FeatureFlagEvaluations?)null,
56+
timestamp: null);
57+
}
58+
59+
[Fact]
60+
public void CaptureSurveyResponsesDoesNotMutateProvidedProperties()
61+
{
62+
var client = Substitute.For<IPostHogClient>();
63+
var properties = new Dictionary<string, object> { ["source"] = "test" };
64+
65+
client.CaptureSurveyResponses(
66+
"distinct-id",
67+
"survey-id",
68+
["first", "second"],
69+
properties);
70+
71+
Assert.Equal(new Dictionary<string, object> { ["source"] = "test" }, properties);
72+
client.Received(1).Capture(
73+
"distinct-id",
74+
"survey sent",
75+
Arg.Is<Dictionary<string, object>>(captured =>
76+
!ReferenceEquals(captured, properties)
77+
&& (string)captured["$survey_id"] == "survey-id"
78+
&& (string)captured["$survey_response"] == "first"
79+
&& (string)captured["survey_response_1"] == "second"),
80+
groups: null,
81+
flags: (FeatureFlagEvaluations?)null,
82+
timestamp: null);
83+
}
84+
85+
static bool HasCopiedPersonProperties(
86+
Dictionary<string, object> captured,
87+
Dictionary<string, object> properties,
88+
Dictionary<string, object> personPropertiesToSet,
89+
Dictionary<string, object> personPropertiesToSetOnce)
90+
=> !ReferenceEquals(captured, properties)
91+
&& captured["$set"] is Dictionary<string, object> set
92+
&& !ReferenceEquals(set, personPropertiesToSet)
93+
&& (string)set["name"] == "Max"
94+
&& captured["$set_once"] is Dictionary<string, object> setOnce
95+
&& !ReferenceEquals(setOnce, personPropertiesToSetOnce)
96+
&& (string)setOnce["initial_url"] == "/blog";
97+
}

‎tests/UnitTests/PostHogClientTests.cs‎

Lines changed: 63 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -127,16 +127,20 @@ public async Task SendsCorrectPayloadWithPersonProperties()
127127
container.FakeTimeProvider.SetUtcNow(new DateTimeOffset(2024, 1, 21, 19, 08, 23, TimeSpan.Zero));
128128
var requestHandler = container.FakeHttpMessageHandler.AddCaptureResponse();
129129
var client = container.Activate<PostHogClient>();
130+
var personPropertiesToSet = new Dictionary<string, object> { ["age"] = 36 };
131+
var personPropertiesToSetOnce = new Dictionary<string, object> { ["join_date"] = "2024-01-21" };
130132

131133
var result = await client.IdentifyAsync(
132134
distinctId: "some-distinct-id",
133135
email: "wildling-lover@example.com",
134136
name: "Jon Snow",
135-
personPropertiesToSet: new() { ["age"] = 36 },
136-
personPropertiesToSetOnce: new() { ["join_date"] = "2024-01-21" },
137+
personPropertiesToSet,
138+
personPropertiesToSetOnce,
137139
CancellationToken.None);
138140

139141
Assert.Equal(1, result.Status);
142+
Assert.Equal(new Dictionary<string, object> { ["age"] = 36 }, personPropertiesToSet);
143+
Assert.Equal(new Dictionary<string, object> { ["join_date"] = "2024-01-21" }, personPropertiesToSetOnce);
140144
var received = requestHandler.GetReceivedRequestBody(indented: true);
141145
Assert.Equal($$"""
142146
{
@@ -245,7 +249,7 @@ public async Task SendsCorrectPayload()
245249
}
246250

247251
[Fact]
248-
public async Task CancellationTokenOverloadOverwritesNameProperty()
252+
public async Task CancellationTokenOverloadDoesNotOverwriteInputNameProperty()
249253
{
250254
var container = new TestContainer();
251255
var requestHandler = container.FakeHttpMessageHandler.AddCaptureResponse();
@@ -264,7 +268,7 @@ public async Task CancellationTokenOverloadOverwritesNameProperty()
264268
CancellationToken.None);
265269

266270
Assert.Equal(1, result.Status);
267-
Assert.Equal("PostHog", properties["name"]);
271+
Assert.Equal("Old Name", properties["name"]);
268272
using var document = JsonDocument.Parse(requestHandler.GetReceivedRequestBody(indented: false));
269273
var root = document.RootElement;
270274
var groupSet = root.GetProperty("properties").GetProperty("$group_set");
@@ -274,7 +278,7 @@ public async Task CancellationTokenOverloadOverwritesNameProperty()
274278
}
275279

276280
[Fact]
277-
public async Task DistinctIdCancellationTokenOverloadOverwritesNameProperty()
281+
public async Task DistinctIdCancellationTokenOverloadDoesNotOverwriteInputNameProperty()
278282
{
279283
var container = new TestContainer();
280284
var requestHandler = container.FakeHttpMessageHandler.AddCaptureResponse();
@@ -294,7 +298,7 @@ public async Task DistinctIdCancellationTokenOverloadOverwritesNameProperty()
294298
CancellationToken.None);
295299

296300
Assert.Equal(1, result.Status);
297-
Assert.Equal("PostHog", properties["name"]);
301+
Assert.Equal("Old Name", properties["name"]);
298302
using var document = JsonDocument.Parse(requestHandler.GetReceivedRequestBody(indented: false));
299303
var root = document.RootElement;
300304
var groupSet = root.GetProperty("properties").GetProperty("$group_set");
@@ -415,6 +419,34 @@ static JsonElement GetOnlyBatchItem(FakeHttpMessageHandler.RequestHandler batchH
415419

416420
public class TheCaptureMethod
417421
{
422+
[Fact]
423+
public async Task DoesNotMutateOrRetainProvidedProperties()
424+
{
425+
var container = new TestContainer(services => services.Configure<PostHogOptions>(options =>
426+
{
427+
options.SuperProperties["super"] = "property";
428+
}));
429+
var requestHandler = container.FakeHttpMessageHandler.AddBatchResponse();
430+
var client = container.Activate<PostHogClient>();
431+
var timestamp = new DateTimeOffset(2024, 1, 21, 19, 8, 23, TimeSpan.Zero);
432+
var properties = new Dictionary<string, object> { ["source"] = "before" };
433+
var groups = new GroupCollection { new Group("company", "acme") };
434+
435+
Assert.True(client.Capture("test-user", "test-event", properties, groups, flags: null, timestamp));
436+
Assert.Equal(new Dictionary<string, object> { ["source"] = "before" }, properties);
437+
438+
properties["source"] = "after";
439+
await client.FlushAsync();
440+
441+
using var document = JsonDocument.Parse(requestHandler.GetReceivedRequestBody(indented: false));
442+
var capturedProperties = document.RootElement.GetProperty("batch")[0].GetProperty("properties");
443+
Assert.Equal("before", capturedProperties.GetProperty("source").GetString());
444+
Assert.Equal("property", capturedProperties.GetProperty("super").GetString());
445+
Assert.Equal("acme", capturedProperties.GetProperty("$groups").GetProperty("company").GetString());
446+
Assert.True(capturedProperties.GetProperty("$is_server").GetBoolean());
447+
Assert.Equal(timestamp, capturedProperties.GetProperty("timestamp").GetDateTimeOffset());
448+
}
449+
418450
[Fact]
419451
public async Task BeforeSendCanModifyFullyEnrichedEventBeforeUpload()
420452
{
@@ -436,13 +468,13 @@ public async Task BeforeSendCanModifyFullyEnrichedEventBeforeUpload()
436468
}));
437469
var requestHandler = container.FakeHttpMessageHandler.AddBatchResponse();
438470
var client = container.Activate<PostHogClient>();
471+
var inputProperties = new Dictionary<string, object> { ["secret"] = "remove-me" };
439472

440-
client.Capture(
441-
"test-user",
442-
"before-send-event",
443-
new Dictionary<string, object> { ["secret"] = "remove-me" });
473+
client.Capture("test-user", "before-send-event", inputProperties);
444474
await client.FlushAsync();
445475

476+
Assert.Equal("remove-me", inputProperties["secret"]);
477+
446478
using var document = JsonDocument.Parse(requestHandler.GetReceivedRequestBody(indented: false));
447479
var properties = document.RootElement
448480
.GetProperty("batch")[0]
@@ -1183,6 +1215,27 @@ public async Task CaptureDefaultsToNotSendingFeatureFlagsEvenWhenLocalEvaluation
11831215

11841216
public class TheCaptureExceptionMethod
11851217
{
1218+
[Fact]
1219+
public async Task DoesNotMutateProvidedProperties()
1220+
{
1221+
var (_, requestHandler, client) = CreateClient();
1222+
var properties = new Dictionary<string, object> { ["source"] = "test" };
1223+
1224+
Assert.True(client.CaptureException(
1225+
new InvalidOperationException("boom"),
1226+
"some-distinct-id",
1227+
properties,
1228+
groups: null,
1229+
flags: null,
1230+
timestamp: DateTimeOffset.UtcNow));
1231+
Assert.Equal(new Dictionary<string, object> { ["source"] = "test" }, properties);
1232+
1233+
await client.FlushAsync();
1234+
var (_, _, capturedProperties) = ParseSingleEvent(requestHandler.GetReceivedRequestBody(indented: false));
1235+
Assert.Equal("test", capturedProperties.GetProperty("source").GetString());
1236+
Assert.Equal("System.InvalidOperationException", capturedProperties.GetProperty("$exception_type").GetString());
1237+
}
1238+
11861239
[Fact]
11871240
public async Task CaptureExceptionWithDivideByZeroException() // based on PostHog/posthog-python test_exception_capture
11881241
{

0 commit comments

Comments
 (0)