Skip to content

Commit a5fc648

Browse files
niemyjskiCopilot
niemyjski
and
Copilot
committed
fix: harden custom fields — race condition, haserror, Idx injection, PATCH system field guard
- Quota race condition: move field creation into EventCustomFieldService.CreateFieldAsync() behind a distributed lock (ILockProvider, key: custom-field-create:{orgId}) so concurrent requests from the same org cannot race past the 20-field quota check. - haserror never written: add hasError parameter to PersistentEvent.UpdateSessionStart() and wire it through EventRepository.UpdateSessionStartLastActivityAsync(). Session plugin now computes hasError = session has any 'error'-type event and passes it to both call sites. - Inbound Idx injection: ClearCustomFieldSlots now removes all new-format managed slot keys (keyword-N, date-N, bool-N etc.) before re-populating from ev.Data, preventing clients from injecting pre-computed slot values. Legacy keys (e.g. sessionend-d) are preserved for backward-compatibility with pre-PR Elasticsearch data. - PATCH system field guard: add IsSystemField check in PatchEventCustomFieldAsync (was already guarded in DELETE but missing in PATCH). Tests added: - PatchField_RejectsSystemField - PostField_ConcurrentCreation_AtQuotaLimit_OnlyOneSucceeds - UpdateSessionStart_SetsHasError_WhenTrue - Event_WithPrePopulatedIdx_IsStripped_AndRecomputedFromData Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 845084b commit a5fc648

7 files changed

Lines changed: 207 additions & 36 deletions

File tree

‎src/Exceptionless.Core/Extensions/PersistentEventExtensions.cs‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,7 @@ public static bool HasSessionEndTime(this PersistentEvent ev)
7575
return null;
7676
}
7777

78-
public static bool UpdateSessionStart(this PersistentEvent ev, DateTime lastActivityUtc, bool isSessionEnd = false)
78+
public static bool UpdateSessionStart(this PersistentEvent ev, DateTime lastActivityUtc, bool isSessionEnd = false, bool hasError = false)
7979
{
8080
if (!ev.IsSessionStart())
8181
return false;
@@ -102,6 +102,11 @@ public static bool UpdateSessionStart(this PersistentEvent ev, DateTime lastActi
102102
ev.Data.Remove(Event.KnownDataKeys.SessionEnd);
103103
}
104104

105+
if (hasError)
106+
ev.Data[Event.KnownDataKeys.SessionHasError] = true;
107+
else
108+
ev.Data.Remove(Event.KnownDataKeys.SessionHasError);
109+
105110
return true;
106111
}
107112

‎src/Exceptionless.Core/Plugins/EventProcessor/Default/70_SessionPlugin.cs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,8 @@ private async Task ProcessManualSessionsAsync(ICollection<EventContext> contexts
8989
});
9090

9191
// try to update an existing session
92-
string? sessionStartEventId = await UpdateSessionStartEventAsync(projectId, session.Key, lastSessionEvent.Event.Date.UtcDateTime, sessionEndEvent is not null);
92+
bool sessionHasError = session.Any(ctx => String.Equals(ctx.Event.Type, Event.KnownTypes.Error, StringComparison.Ordinal));
93+
string? sessionStartEventId = await UpdateSessionStartEventAsync(projectId, session.Key, lastSessionEvent.Event.Date.UtcDateTime, sessionEndEvent is not null, sessionHasError);
9394

9495
// do we already have a session start for this session id?
9596
if (!String.IsNullOrEmpty(sessionStartEventId) && sessionStartEvent is not null)
@@ -179,11 +180,12 @@ private async Task ProcessAutoSessionsAsync(ICollection<EventContext> contexts)
179180

180181
session.ForEach(s => s.Event.SetSessionId(sessionId));
181182

183+
bool identitySessionHasError = session.Any(ctx => String.Equals(ctx.Event.Type, Event.KnownTypes.Error, StringComparison.Ordinal));
182184
if (isNewSession)
183185
{
184186
if (sessionStartEvent is not null)
185187
{
186-
sessionStartEvent.Event.UpdateSessionStart(lastSessionEvent.Event.Date.UtcDateTime, lastSessionEvent.Event.IsSessionEnd());
188+
sessionStartEvent.Event.UpdateSessionStart(lastSessionEvent.Event.Date.UtcDateTime, lastSessionEvent.Event.IsSessionEnd(), identitySessionHasError);
187189
sessionStartEvent.SetProperty("SetSessionStartEventId", true);
188190
}
189191
else
@@ -203,7 +205,7 @@ private async Task ProcessAutoSessionsAsync(ICollection<EventContext> contexts)
203205
sessionStartEvent.IsCancelled = true;
204206
}
205207

206-
await UpdateSessionStartEventAsync(projectId, sessionId, lastSessionEvent.Event.Date.UtcDateTime, lastSessionEvent.Event.IsSessionEnd());
208+
await UpdateSessionStartEventAsync(projectId, sessionId, lastSessionEvent.Event.Date.UtcDateTime, lastSessionEvent.Event.IsSessionEnd(), identitySessionHasError);
207209
}
208210
}
209211
}

‎src/Exceptionless.Core/Repositories/EventRepository.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ public async Task<bool> UpdateSessionStartLastActivityAsync(string id, DateTime
6161
if (ev is null)
6262
return false;
6363

64-
if (!ev.UpdateSessionStart(lastActivityUtc, isSessionEnd))
64+
if (!ev.UpdateSessionStart(lastActivityUtc, isSessionEnd, hasError))
6565
return false;
6666

6767
await SaveAsync(ev, o => o.Notifications(sendNotifications));

‎src/Exceptionless.Core/Services/EventCustomFieldService.cs‎

Lines changed: 53 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
using Exceptionless.Core.Models;
33
using Exceptionless.Core.Repositories;
44
using Foundatio.Extensions.Hosting.Startup;
5+
using Foundatio.Lock;
56
using Foundatio.Repositories.Elasticsearch.CustomFields;
67
using Foundatio.Repositories.Models;
78
using Microsoft.Extensions.Logging;
@@ -12,6 +13,7 @@ public class EventCustomFieldService : IStartupAction
1213
{
1314
private readonly IEventRepository _eventRepository;
1415
private readonly ICustomFieldDefinitionRepository _customFieldDefinitionRepository;
16+
private readonly ILockProvider _lockProvider;
1517
private readonly ILogger<EventCustomFieldService> _logger;
1618

1719
private const int MaxKeywordLength = 256;
@@ -49,10 +51,12 @@ public class EventCustomFieldService : IStartupAction
4951
public EventCustomFieldService(
5052
IEventRepository eventRepository,
5153
ICustomFieldDefinitionRepository customFieldDefinitionRepository,
54+
ILockProvider lockProvider,
5255
ILoggerFactory loggerFactory)
5356
{
5457
_eventRepository = eventRepository;
5558
_customFieldDefinitionRepository = customFieldDefinitionRepository;
59+
_lockProvider = lockProvider;
5660
_logger = loggerFactory.CreateLogger<EventCustomFieldService>();
5761
}
5862

@@ -88,6 +92,49 @@ public static bool IsSystemField(string fieldName)
8892
return SystemFields.Any(f => String.Equals(f.Name, fieldName, StringComparison.OrdinalIgnoreCase));
8993
}
9094

95+
/// <summary>
96+
/// Creates a new custom field definition under a distributed lock so concurrent requests
97+
/// from the same organization cannot race past the quota check.
98+
/// Returns null when the field cannot be created (quota exceeded or duplicate name).
99+
/// </summary>
100+
public async Task<CustomFieldDefinition?> CreateFieldAsync(
101+
string organizationId,
102+
string name,
103+
string indexType,
104+
int maxFieldsPerOrganization,
105+
string? description = null,
106+
int? displayOrder = null,
107+
CancellationToken cancellationToken = default)
108+
{
109+
// Ensure system fields are provisioned under the lock so they always occupy slot 1 of their type.
110+
await EnsureSystemFieldsAsync(organizationId);
111+
112+
await using var fieldLock = await _lockProvider.AcquireAsync($"custom-field-create:{organizationId}", TimeSpan.FromSeconds(30), cancellationToken: cancellationToken);
113+
if (fieldLock is null)
114+
{
115+
_logger.LogWarning("Could not acquire custom field creation lock for organization {OrganizationId}", organizationId);
116+
return null;
117+
}
118+
119+
// Re-read the field mapping inside the lock for an accurate count.
120+
var existingPage = await _customFieldDefinitionRepository.FindByTenantAsync(nameof(PersistentEvent), organizationId);
121+
var allActive = new List<CustomFieldDefinition>(existingPage.Documents);
122+
while (await existingPage.NextPageAsync())
123+
allActive.AddRange(existingPage.Documents);
124+
125+
// System fields are not counted against the user quota.
126+
var userDefinedActiveCount = allActive.Count(f => !IsSystemField(f.Name));
127+
if (userDefinedActiveCount >= maxFieldsPerOrganization)
128+
return null;
129+
130+
// Case-insensitive duplicate check inside the lock.
131+
if (allActive.Any(f => String.Equals(f.Name, name, StringComparison.OrdinalIgnoreCase)))
132+
return null;
133+
134+
return await _customFieldDefinitionRepository.AddFieldAsync(
135+
nameof(PersistentEvent), organizationId, name, indexType, description, displayOrder ?? 0);
136+
}
137+
91138
private async Task OnDocumentsChangingAsync(object sender, DocumentsChangeEventArgs<PersistentEvent> args)
92139
{
93140
if (args.ChangeType == ChangeType.Removed)
@@ -139,7 +186,7 @@ private async Task OnDocumentsChangingAsync(object sender, DocumentsChangeEventA
139186

140187
private void ProcessEventCustomFields(PersistentEvent ev, IDictionary<string, CustomFieldDefinition> fieldMapping)
141188
{
142-
ClearManagedCustomFieldSlots(ev);
189+
ClearCustomFieldSlots(ev);
143190

144191
if (fieldMapping.Count == 0 || ev.Data is null || ev.Data.Count == 0)
145192
return;
@@ -177,11 +224,15 @@ private void ProcessEventCustomFields(PersistentEvent ev, IDictionary<string, Cu
177224
ev.Idx = null;
178225
}
179226

180-
private static void ClearManagedCustomFieldSlots(PersistentEvent ev)
227+
private static void ClearCustomFieldSlots(PersistentEvent ev)
181228
{
182229
if (ev.Idx is null || ev.Idx.Count == 0)
183230
return;
184231

232+
// Only clear new-format managed slot keys (type-N, e.g. keyword-1, date-2).
233+
// Legacy keys (e.g. sessionend-d from pre-PR data) are preserved so that ES queries
234+
// that check both formats (GetOpenSessionsAsync) remain backward-compatible.
235+
// Client-injected new-format slots are therefore stripped before server re-population.
185236
foreach (var idxKey in ev.Idx.Keys.Where(IsManagedCustomFieldSlotKey).ToArray())
186237
ev.Idx.Remove(idxKey);
187238

‎src/Exceptionless.Web/Controllers/OrganizationController.cs‎

Lines changed: 31 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1079,48 +1079,44 @@ public async Task<ActionResult<CustomFieldDefinitionResponse>> PostEventCustomFi
10791079
// so normalizing here ensures the stored value and the runtime switch agree.
10801080
var normalizedIndexType = model.IndexType.ToLowerInvariant();
10811081

1082-
// Ensure system fields are provisioned first so they always occupy slot 1 of their type.
1083-
// This prevents user fields from taking precedence over system slots.
1084-
await _eventCustomFieldService.EnsureSystemFieldsAsync(id);
1085-
1086-
// Fetch ALL active definitions. FindByTenantAsync pages at 1000 documents — more than sufficient
1087-
// for any org, but we iterate pages defensively for accurate quota and dup-name enforcement.
1088-
var existingPage = await _customFieldDefinitionRepository.FindByTenantAsync(nameof(PersistentEvent), id);
1089-
var allActive = new List<CustomFieldDefinition>(existingPage.Documents);
1090-
while (await existingPage.NextPageAsync())
1091-
allActive.AddRange(existingPage.Documents);
1092-
1093-
// Count only active user-defined fields (system fields do not count toward the user quota).
1094-
var userDefinedActiveCount = allActive.Count(f => !EventCustomFieldService.IsSystemField(f.Name));
1095-
if (userDefinedActiveCount >= _options.CustomFieldOptions.MaxFieldsPerOrganization)
1096-
return Problem(
1097-
detail: $"Maximum of {_options.CustomFieldOptions.MaxFieldsPerOrganization} custom fields per organization has been reached.",
1098-
statusCode: StatusCodes.Status400BadRequest);
1099-
1100-
// Check case-insensitive duplicate against active fields only.
1101-
// Soft-deleted field names are available for reuse; Foundatio will assign a new slot to the replacement.
1102-
if (allActive.Any(f => String.Equals(f.Name, model.Name, StringComparison.OrdinalIgnoreCase)))
1103-
return Problem(
1104-
detail: $"A custom field named '{model.Name}' already exists for this organization.",
1105-
statusCode: StatusCodes.Status400BadRequest);
1106-
1107-
CustomFieldDefinition definition;
1082+
// CreateFieldAsync holds a distributed lock so concurrent requests from the same org
1083+
// cannot race past the quota check. It returns null on quota exceeded or duplicate name.
1084+
CustomFieldDefinition? definition;
11081085
try
11091086
{
1110-
definition = await _customFieldDefinitionRepository.AddFieldAsync(
1111-
nameof(PersistentEvent),
1087+
definition = await _eventCustomFieldService.CreateFieldAsync(
11121088
id,
11131089
model.Name,
11141090
normalizedIndexType,
1091+
_options.CustomFieldOptions.MaxFieldsPerOrganization,
11151092
model.Description,
1116-
model.DisplayOrder);
1093+
model.DisplayOrder,
1094+
HttpContext.RequestAborted);
11171095
}
11181096
catch (Exception ex) when (ex is ArgumentException or ValidationException or InvalidOperationException or DocumentValidationException)
11191097
{
11201098
ModelState.AddModelError("general", ex.Message);
11211099
return ValidationProblem(ModelState);
11221100
}
11231101

1102+
if (definition is null)
1103+
{
1104+
// Determine which error to surface: quota or duplicate.
1105+
var existingPage = await _customFieldDefinitionRepository.FindByTenantAsync(nameof(PersistentEvent), id);
1106+
var allActive = new List<CustomFieldDefinition>(existingPage.Documents);
1107+
while (await existingPage.NextPageAsync())
1108+
allActive.AddRange(existingPage.Documents);
1109+
1110+
if (allActive.Any(f => String.Equals(f.Name, model.Name, StringComparison.OrdinalIgnoreCase)))
1111+
return Problem(
1112+
detail: $"A custom field named '{model.Name}' already exists for this organization.",
1113+
statusCode: StatusCodes.Status400BadRequest);
1114+
1115+
return Problem(
1116+
detail: $"Maximum of {_options.CustomFieldOptions.MaxFieldsPerOrganization} custom fields per organization has been reached.",
1117+
statusCode: StatusCodes.Status400BadRequest);
1118+
}
1119+
11241120
return StatusCode(StatusCodes.Status201Created, CustomFieldDefinitionResponse.FromDefinition(definition));
11251121
}
11261122

@@ -1152,6 +1148,12 @@ public async Task<ActionResult<CustomFieldDefinitionResponse>> PatchEventCustomF
11521148
if (definition.IsDeleted)
11531149
return NotFound();
11541150

1151+
// System fields cannot be modified by users.
1152+
if (EventCustomFieldService.IsSystemField(definition.Name))
1153+
return Problem(
1154+
detail: $"'{definition.Name}' is a reserved system field and cannot be modified.",
1155+
statusCode: StatusCodes.Status400BadRequest);
1156+
11551157
bool changed = false;
11561158
if (model.Description is not null)
11571159
{

‎tests/Exceptionless.Tests/CustomFields/CustomFieldApiTests.cs‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
using Exceptionless.Core.Extensions;
12
using Exceptionless.Core.Models;
23
using Exceptionless.Core.Repositories;
34
using Exceptionless.Core.Services;
@@ -9,6 +10,7 @@
910
using Foundatio.Repositories.Elasticsearch.CustomFields;
1011
using Microsoft.AspNetCore.Mvc;
1112
using Xunit;
13+
using DataDictionary = Exceptionless.Core.Models.DataDictionary;
1214

1315
namespace Exceptionless.Tests.CustomFields;
1416

@@ -857,4 +859,82 @@ public Task PostField_RejectsUnicodeFieldName()
857859
.StatusCodeShouldBeUnprocessableEntity()
858860
);
859861
}
862+
863+
[Fact]
864+
public async Task PatchField_RejectsSystemField()
865+
{
866+
// Ensure system fields are provisioned
867+
var service = GetService<EventCustomFieldService>();
868+
await service.EnsureSystemFieldsAsync(SampleDataService.TEST_ORG_ID);
869+
870+
// System fields are hidden from the GET list endpoint; retrieve directly from the repository
871+
var fieldMapping = await _customFieldDefinitionRepository.GetFieldMappingAsync(
872+
nameof(PersistentEvent), SampleDataService.TEST_ORG_ID);
873+
Assert.True(fieldMapping.TryGetValue(Event.KnownDataKeys.SessionEnd, out var systemField));
874+
Assert.NotNull(systemField);
875+
876+
var problem = await SendRequestAsAsync<ProblemDetails>(r => r
877+
.AsTestOrganizationUser()
878+
.Patch()
879+
.AppendPaths("organizations", SampleDataService.TEST_ORG_ID, "event-custom-fields", systemField.Id)
880+
.Content(new UpdateCustomFieldDefinition { Description = "hacked" })
881+
.ExpectedStatus(System.Net.HttpStatusCode.BadRequest)
882+
);
883+
884+
Assert.NotNull(problem);
885+
Assert.Contains("reserved system field", problem.Detail, StringComparison.OrdinalIgnoreCase);
886+
}
887+
888+
[Fact]
889+
public async Task PostField_ConcurrentCreation_AtQuotaLimit_OnlyOneSucceeds()
890+
{
891+
const int maxFields = 3;
892+
var service = GetService<EventCustomFieldService>();
893+
894+
// Fill up to maxFields - 1
895+
for (int i = 0; i < maxFields - 1; i++)
896+
{
897+
var result = await service.CreateFieldAsync(
898+
SampleDataService.TEST_ORG_ID, $"field_{i}", "keyword", maxFields,
899+
cancellationToken: TestContext.Current.CancellationToken);
900+
Assert.NotNull(result);
901+
}
902+
903+
// Fire two concurrent creates for the final slot
904+
var task1 = service.CreateFieldAsync(SampleDataService.TEST_ORG_ID, "race_a", "keyword", maxFields,
905+
cancellationToken: TestContext.Current.CancellationToken);
906+
var task2 = service.CreateFieldAsync(SampleDataService.TEST_ORG_ID, "race_b", "keyword", maxFields,
907+
cancellationToken: TestContext.Current.CancellationToken);
908+
909+
var results = await Task.WhenAll(task1, task2);
910+
var succeeded = results.Count(r => r is not null);
911+
912+
// Exactly one must succeed; quota must not be exceeded
913+
Assert.Equal(1, succeeded);
914+
}
915+
916+
[Fact]
917+
public void UpdateSessionStart_SetsHasError_WhenTrue()
918+
{
919+
var ev = new PersistentEvent
920+
{
921+
OrganizationId = SampleDataService.TEST_ORG_ID,
922+
ProjectId = SampleDataService.TEST_PROJECT_ID,
923+
Type = Event.KnownTypes.Session,
924+
Date = DateTimeOffset.UtcNow,
925+
Data = new DataDictionary()
926+
};
927+
928+
var now = DateTime.UtcNow;
929+
930+
// hasError = true should add SessionHasError
931+
var updated = ev.UpdateSessionStart(now, hasError: true);
932+
Assert.True(updated);
933+
Assert.True(ev.Data.ContainsKey(Event.KnownDataKeys.SessionHasError));
934+
Assert.Equal(true, ev.Data[Event.KnownDataKeys.SessionHasError]);
935+
936+
// hasError = false should remove it
937+
ev.UpdateSessionStart(now, hasError: false);
938+
Assert.False(ev.Data.ContainsKey(Event.KnownDataKeys.SessionHasError));
939+
}
860940
}

‎tests/Exceptionless.Tests/CustomFields/CustomFieldIndexingTests.cs‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -423,6 +423,37 @@ await _customFieldDefinitionRepository.AddFieldAsync(
423423
Assert.Single(results.Documents);
424424
}
425425

426+
[Fact]
427+
public async Task Event_WithPrePopulatedIdx_IsStripped_AndRecomputedFromData()
428+
{
429+
// Register "severity" as a custom field — the slot (e.g. keyword-1 or keyword-2) depends on
430+
// which system fields are provisioned first, so we don't hard-code the slot number.
431+
var definition = await _customFieldDefinitionRepository.AddFieldAsync(
432+
nameof(PersistentEvent), TestConstants.OrganizationId, "severity", "keyword");
433+
await RefreshDataAsync();
434+
435+
var ev = GenerateEvent();
436+
// Inject a bogus value into the managed slot and a different bogus value into a non-slot key.
437+
var injectedSlotKey = definition.GetIdxName(); // e.g. "keyword-1" or "keyword-2"
438+
ev.Idx = new DataDictionary
439+
{
440+
[injectedSlotKey] = "injected_via_client"
441+
};
442+
ev.Data ??= new DataDictionary();
443+
ev.Data["severity"] = "low";
444+
445+
// Running through the pipeline fires DocumentsChanging which calls ClearCustomFieldSlots,
446+
// which removes all managed new-format slot keys and re-populates from ev.Data.
447+
var context = await _pipeline.RunAsync(ev, GetOrganization(), GetProject());
448+
Assert.False(context.HasError, context.ErrorMessage);
449+
450+
// The in-memory event Idx must contain "low" (server-computed from data)
451+
// and must NOT contain the client-injected "injected_via_client" value.
452+
Assert.NotNull(context.Event.Idx);
453+
Assert.Contains(context.Event.Idx.Values, v => "low".Equals(v?.ToString()));
454+
Assert.DoesNotContain(context.Event.Idx.Values, v => "injected_via_client".Equals(v?.ToString()));
455+
}
456+
426457
private Organization GetOrganization()
427458
{
428459
return _organizationData.GenerateSampleOrganization(_billingManager, _plans);

0 commit comments

Comments
 (0)