fix(security): config PUT derives its preserved system-owned fields from the PATCH deny-list (BaseDeniedFields) so ProDowngradedUtc/OnboardedAt can no longer be written by a tenant admin — a client-supplied ProDowngradedUtc refreshed the retention-downgrade grace anchor indefinitely, keeping the Pro retention cap on a Community tenant

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Oliver Kieselbach
2026-08-30 02:01:32 +02:00
parent 8c0cb2c423
commit 21d200a1e4
3 changed files with 73 additions and 35 deletions
@@ -680,6 +680,51 @@ public class TenantConfigPatchServiceTests
Assert.Equal(schema.Count, schema.Select(f => f.Name).Distinct().Count());
}
/// <summary>
/// The full-model PUT (config/{tenantId}) must honour the same deny-list as the field
/// patch: a client body carrying ProDowngradedUtc (retention-downgrade grace anchor),
/// OnboardedAt, DomainName, plan/trial or provenance fields must not change the stored
/// values. Driven by the deny-list itself so a new denied field is covered automatically.
/// </summary>
[Fact]
public void PreserveDeniedFields_OverwritesEveryDeniedModelPropertyFromStored()
{
var props = typeof(TenantConfiguration)
.GetProperties(System.Reflection.BindingFlags.Public | System.Reflection.BindingFlags.Instance)
.Where(p => p.CanWrite && TenantConfigPatchService.BaseDeniedFields.Contains(p.Name))
.ToList();
Assert.Contains(props, p => p.Name == "ProDowngradedUtc");
Assert.Contains(props, p => p.Name == "OnboardedAt");
Assert.Contains(props, p => p.Name == "DomainName");
var stored = new TenantConfiguration { TenantId = "t1" };
var incoming = new TenantConfiguration { TenantId = "t1" };
foreach (var p in props)
{
p.SetValue(stored, Distinct(p.PropertyType, 1));
p.SetValue(incoming, Distinct(p.PropertyType, 2));
Assert.NotEqual(p.GetValue(stored), p.GetValue(incoming));
}
TenantConfigPatchService.PreserveDeniedFields(incoming, stored);
foreach (var p in props)
Assert.Equal(p.GetValue(stored), p.GetValue(incoming));
}
private static object Distinct(Type t, int seed)
{
var u = Nullable.GetUnderlyingType(t) ?? t;
if (u == typeof(string)) return $"value-{seed}";
if (u == typeof(bool)) return seed % 2 == 0;
if (u == typeof(int)) return seed;
if (u == typeof(long)) return (long)seed;
if (u == typeof(DateTime)) return new DateTime(2026, 1, seed, 0, 0, 0, DateTimeKind.Utc);
if (u == typeof(DateTimeOffset)) return new DateTimeOffset(2026, 1, seed, 0, 0, 0, TimeSpan.Zero);
if (u.IsEnum) return Enum.GetValues(u).GetValue(seed % Enum.GetValues(u).Length)!;
throw new NotSupportedException($"Add a Distinct() case for {u.Name}");
}
[Fact]
public void FieldsSchema_DeniedFields_MatchThePatchDenyList()
{
@@ -110,11 +110,15 @@ namespace AutopilotMonitor.Functions.Functions.Config
// admins are served the FULL config so this is normally a no-op for them.)
config.RestoreRedactedSecretsFrom(existingConfig);
// DomainName is tenant identity: seeded once from the first login's UPN (AuthFunction)
// and never client-writable — for ANY caller, GA included — mirroring the patch flow
// ("identity — never writable"). Without this preserve the full-model PUT persisted an
// arbitrary string here, which is rendered into the welcome/farewell mails.
config.DomainName = existingConfig.DomainName;
// System-owned fields are never client-writable — for ANY caller, GA included:
// identity (DomainName, seeded once from the first login's UPN), onboarding/auth
// provenance (OnboardedAt/OnboardedBy/LastAuthClientId*), app-reg homing
// (HomedAppClientId — the 2026-07-31 stale-round-trip incident), and plan/trial/
// downgrade state (PlanTier/Trial*/ProDowngradedUtc — mutable only via the plan
// and trial endpoints; a client-supplied ProDowngradedUtc would refresh the
// retention-downgrade grace anchor indefinitely). The set is the PATCH endpoint's
// BaseDeniedFields, so the two write paths cannot drift.
TenantConfigPatchService.PreserveDeniedFields(config, existingConfig);
// Shared model validation (rate limits, contact address, webhook/Teams SSRF, custom
// headers, notification channels, diagnostics SAS, retention cap) — single source
@@ -164,26 +168,6 @@ namespace AutopilotMonitor.Functions.Functions.Config
config.DisabledUntil = existingConfig.DisabledUntil;
}
// App-reg homing is IMMUTABLE via the generic PUT — for ALL callers, GA included.
// The only writers are the consent-driven auto-flip and POST config/{tenantId}/
// app-homing (AppHomingFunction). Prod incident 2026-07-31 18:03Z: the consent
// auto-flip landed, and 600ms later the frontend's routine "persist validation
// toggle" PUT — round-tripping a config loaded BEFORE the flip — silently reverted
// the homing to legacy, because a GA caller's stale null was indistinguishable
// from an intentional revert. A full-model PUT can never carry that intent.
config.HomedAppClientId = existingConfig.HomedAppClientId;
// Last-seen auth provenance is system-written (AuthFunction) — never via PUT,
// for ANY caller: a round-tripped stale view must not rewind the observability.
config.LastAuthClientId = existingConfig.LastAuthClientId;
config.LastAuthClientIdSince = existingConfig.LastAuthClientIdSince;
// OnboardedBy is system-written once at first login (AuthFunction) and immutable
// after that — never via PUT, for ANY caller: a revoke → re-approve cycle
// auto-promotes whatever UPN is stored here, so a client-supplied value could
// smuggle in an arbitrary promotable user (or null out the audit provenance).
config.OnboardedBy = existingConfig.OnboardedBy;
// Safety: if GA gate is off, force UnrestrictedMode to false
if (!config.UnrestrictedModeEnabled)
{
@@ -193,16 +177,6 @@ namespace AutopilotMonitor.Functions.Functions.Config
// MaxNdjsonPayloadSizeMB is table-only — always preserve existing value
config.MaxNdjsonPayloadSizeMB = existingConfig.MaxNdjsonPayloadSizeMB;
// Plan/trial fields are mutable ONLY via the dedicated plan/trial endpoints
// (PATCH config/{tenantId}/plan, POST config/{tenantId}/trial). The generic PUT
// deserializes the full model, so without this preserve a round-tripped stale
// view would silently reset the tenant's edition/trial — for ALL callers, GA included.
config.PlanTier = existingConfig.PlanTier;
config.TrialExpiresUtc = existingConfig.TrialExpiresUtc;
config.TrialStartedUtc = existingConfig.TrialStartedUtc;
config.TrialConsumed = existingConfig.TrialConsumed;
config.TrialGrantedBy = existingConfig.TrialGrantedBy;
// Save configuration (retention cap already enforced by ValidateModel above)
var writeSource = ResolveWriteSource(req.Query["intent"]);
await _configService.SaveConfigurationAsync(config, writeSource, null);
@@ -127,6 +127,25 @@ namespace AutopilotMonitor.Functions.Services
"ProDowngradedUtc",
};
/// <summary>
/// Full-model PUT counterpart of <see cref="BaseDeniedFields"/>: copies every denied model
/// property from the stored config onto the incoming one so a client body can never write
/// them — for ANY caller, GA included. The PUT deserializes the whole model, so a hand-kept
/// preserve list drifts (ProDowngradedUtc/OnboardedAt were missing, letting a tenant admin
/// refresh the retention-downgrade grace anchor forever). Deriving it from the single
/// deny-list keeps both write paths in lock-step. Server-stamped fields (TenantId,
/// UpdatedBy, LastUpdated) are copied too; the PUT overwrites them afterwards.
/// </summary>
internal static void PreserveDeniedFields(TenantConfiguration target, TenantConfiguration stored)
{
foreach (var name in BaseDeniedFields)
{
// PartitionKey/RowKey/Timestamp/ETag are table plumbing, not model properties.
if (ModelProperties.TryGetValue(name, out var prop))
prop.SetValue(target, prop.GetValue(stored));
}
}
/// <summary>
/// Additional deny-list for non-GA tiers (phase 2): the GA-only toggles the PUT
/// endpoint silently reverts for tenant admins are an explicit 400 here.