diff --git a/core/docs/stabilization/PLAN.md b/core/docs/stabilization/PLAN.md new file mode 100644 index 00000000..9196a6ab --- /dev/null +++ b/core/docs/stabilization/PLAN.md @@ -0,0 +1,137 @@ +# Stabilization Audit — Consolidated Plan + +## Duplicate / Overlap Analysis + +| Relationship | Issues | Rationale | +|---|---|---| +| **Duplicate** | 003 + 011 | Both address the incomplete `ActivitySerializerMap`. 011 explicitly calls itself "related to but distinct from 003" but the fix is the same: populate the map and add a fallback diagnostic. **Consolidate into one work item.** | +| **Co-located** | 001 + 006 | Both live in `JwtExtensions.cs` and involve `ConfigurationManager`. 001 is the blocking `.GetAwaiter().GetResult()` call; 006 is the missing `Dispose()`. A single refactor (extract an `OidcConfigCache` singleton with eager background refresh) resolves both. **Fix together.** | +| **Co-located** | 005 (partial) + 001 + 006 | One of 005's three `BuildServiceProvider()` call sites is in `JwtExtensions.cs` (line 324). The JWT refactor above can eliminate it. The other two sites (BotConfig.cs, AddBotApplicationExtensions.cs) are separate. | +| **Same pattern** | 004 + 023 | Both are silent `as`-cast → `null` returns. 004 is `TeamsActivity` property shadowing; 023 is entity properties after deserialization. Same root pattern but different files, different fixes. **Track separately but schedule in the same phase.** | +| **Same pattern** | 013 + 015 | Both are shallow-copy hazards. 013 is the `CoreActivity` copy constructor; 015 is `CitationEntity`. Independent fixes but same review theme. | +| **Same area** | 007 + 021 | Both are compat-layer type-conversion / serialization fidelity issues. 007 is `object.ToString()` on property bags; 021 is Newtonsoft→STJ cross-framework round-trip. **Schedule together for a compat-layer serialization sweep.** | + +After consolidation: **22 unique work items** (003 + 011 merged). + +--- + +## Priority List (by impact) + +### Critical — Data corruption, deadlocks, or concurrency hazards in production + +| # | Audits | Title | Component | Risk | +|---|--------|-------|-----------|------| +| C1 | 012 | Race condition on `OnActivity` delegate in `CompatAdapter` | Compat | Concurrent requests dispatch to wrong bot/handler; cross-request data leakage | +| C2 | 001 + 006 | Blocking async + undisposed `ConfigurationManager` in JWT key resolver | Core/Hosting | Thread-pool starvation & deadlock under load; resource leak on every OIDC authority | +| C3 | 023 | Entity property `as`-casts silently return `null` after JSON deserialization | Apps/Entities | `Citation`, `Mentioned`, `Pattern` are always `null` when deserialized from JSON — primary code path broken | +| C4 | 003 + 011 | Asymmetric serializer/deserializer maps — 6 of 8 activity types lose fields on `ToJson()` | Apps/Schema | Outbound replies, logging, and compat round-trips silently drop subtype-specific fields | + +### High — Incorrect behavior, silent data loss, or DI misuse + +| # | Audits | Title | Component | Risk | +|---|--------|-------|-----------|------| +| H1 | 005 | `BuildServiceProvider()` called 3× during startup | Core/Hosting | Duplicate singleton instances, resource leaks, ASP0000 warnings | +| H2 | 004 | Silent `as`-cast in `TeamsActivity` property shadowing | Apps/Schema | `From`, `Recipient`, `Conversation`, `ChannelData` return `null` if base type was set directly | +| H3 | 002 | Unsafe `(T)(object)` cast in generic HTTP response deserializer | Core/Http | `InvalidCastException` if type guard is bypassed; fragile pattern copied elsewhere | +| H4 | 021 | Cross-framework JSON serialization (Newtonsoft → STJ) | Compat | Silent property loss due to naming-convention / attribute mismatch between serializers | +| H5 | 008 | Unknown entity types silently discarded | Apps/Entities | Third-party & future entity types dropped with no log; incomplete bot behavior | + +### Medium — Robustness, input validation, error handling + +| # | Audits | Title | Component | Risk | +|---|--------|-------|-----------|------| +| M1 | 018 | Token parsing crash in logging path | Core/Hosting | Malformed JWT in `LogTokenClaims` crashes outgoing HTTP request | +| M2 | 017 | Unguarded `Guid.Parse` on external input | Core/Hosting + Compat | `FormatException` on malformed `AgenticUserId` fails API call | +| M3 | 013 | Shallow reference copy in `CoreActivity` copy constructor | Core/Schema | Mutating copy mutates original; `Rebase()` stomps shared `JsonArray` references | +| M4 | 016 | Unsafe direct cast `IActivity` → `Activity` in `CompatTeamsInfo` | Compat | `InvalidCastException` with no descriptive message for non-`Activity` implementations | +| M5 | 007 | Unvalidated `object.ToString()` on property bag values | Compat | `aadObjectId` etc. become `"System.Text.Json.JsonElement"` under STJ | +| M6 | 009 | Fragile exception filter by message string in streaming writer | Apps/Streaming | Exception message rewording causes unhandled `HttpRequestException` | + +### Low — Code hygiene, minor perf, defensive improvements + +| # | Audits | Title | Component | Risk | +|---|--------|-------|-----------|------| +| L1 | 014 | Builder `Build()` returns mutable internal reference | Apps/Schema | Post-build mutations leak back into builder; violated builder contract | +| L2 | 020 | Non-thread-safe middleware list in `TurnMiddleware` | Core | Race if middleware added after pipeline starts (unlikely but unguarded) | +| L3 | 010 | O(n²) string concatenation in streaming accumulator | Apps/Streaming | GC pressure under high-concurrency streaming; easy `StringBuilder` fix | +| L4 | 015 | Shallow clone of `CitationClaim` list | Apps/Entities | Mutating claim in copy mutates original (low likelihood) | +| L5 | 022 | Missing `ServiceUrl` null validation | Compat | `NullReferenceException` instead of descriptive `ArgumentException` | +| L6 | 019 | Unnecessary `GC.SuppressFinalize` without finalizer | Compat | Misleading code; no runtime impact | + +--- + +## Phased Fix Plan + +### Phase 1 — Critical concurrency & data-integrity fixes +**Goal:** Eliminate production-affecting bugs that corrupt data or cause deadlocks. +**Scope:** 4 work items (6 audits). + +| Work Item | Audits | What to Do | Files | +|-----------|--------|------------|-------| +| **1.1** | 012 | Replace mutable `OnActivity` delegate assignment with `AsyncLocal<>` per-request scoping (Option A) or per-request callback parameter (Option B). | `CompatAdapter.cs` | +| **1.2** | 001 + 006 | Extract `OidcConfigCache` singleton (owns cache + implements `IDisposable`). Background-refresh signing keys. Remove `.GetAwaiter().GetResult()` from resolver. Remove one `BuildServiceProvider()` call site (005 partial). | `JwtExtensions.cs` | +| **1.3** | 023 | In entity property getters, detect `JsonElement` and call `.Deserialize()` on access. Cache result back into `Properties`. Apply to `CitationEntity`, `MentionEntity`, `OMessageEntity`, `SensitiveUsageEntity`. | `CitationEntity.cs`, `MentionEntity.cs`, `OMessageEntity.cs`, `SensitiveUsageEntity.cs` | +| **1.4** | 003 + 011 | Add missing 6 entries to `ActivitySerializerMap`. Register types in `TeamsActivityJsonContext`. Add symmetry assertion test. Add `Debug.Fail` on fallback path. | `TeamsActivityType.cs`, `TeamsActivity.cs`, `TeamsActivityJsonContext.cs` | + +**Validation:** Run all existing unit + integration tests. Add concurrency stress test for 1.1. Add round-trip serialization tests for 1.3 and 1.4. + +--- + +### Phase 2 — High-severity correctness & DI hygiene +**Goal:** Fix silent data loss, DI anti-patterns, and type-safety gaps. +**Scope:** 5 work items. + +| Work Item | Audits | What to Do | Files | +|-----------|--------|------------|-------| +| **2.1** | 005 | Replace remaining 2 `BuildServiceProvider()` calls: extract `IConfiguration` from `ServiceDescriptor.ImplementationInstance` or throw. Remove `GetLoggerFromServices` temp-provider fallback. | `BotConfig.cs`, `AddBotApplicationExtensions.cs` | +| **2.2** | 004 | Add `Debug.Assert` in each shadowed getter (Option A). Optionally add auto-upgrade in `Rebase()` for plain `ConversationAccount` → `TeamsConversationAccount`. | `TeamsActivity.cs` | +| **2.3** | 002 | Replace `(T)(object)` with `Unsafe.As` or extract `ReturnRawString` helper. Add unit test for plain-text HTTP response. | `BotHttpClient.cs` | +| **2.4** | 021 + 007 | **Compat serialization sweep.** (a) Replace cross-framework round-trip in `SendMeetingNotificationAsync` with single-serializer path or manual mapping. (b) Add `ExtractStringProperty` helper for property-bag `.ToString()` calls. | `CompatTeamsInfo.cs`, `CompatActivity.cs` | +| **2.5** | 008 | Change unknown-entity fallback from `null` to `item.Deserialize(options)` so unrecognized types are preserved. Add trace log. | `Entity.cs` | + +**Validation:** Verify no ASP0000 warnings. Add tests for each compat serialization path. Add test for unknown entity type preservation. + +--- + +### Phase 3 — Robustness & input validation +**Goal:** Harden error handling and input validation at system boundaries. +**Scope:** 6 work items. + +| Work Item | Audits | What to Do | Files | +|-----------|--------|------------|-------| +| **3.1** | 018 | Wrap `LogTokenClaims` in try-catch. Replace deprecated `JwtSecurityToken` with `JsonWebToken`. | `BotAuthenticationHandler.cs` | +| **3.2** | 017 | Replace `Guid.Parse` with `Guid.TryParse` + descriptive error in both handlers. | `BotAuthenticationHandler.cs`, `KeyedBotAuthenticationHandler.cs` | +| **3.3** | 013 | Deep-copy mutable references (`ChannelData`, `Entities`, `Attachments`, `Properties`, `Value`) in `CoreActivity` copy constructor. | `CoreActivity.cs` | +| **3.4** | 016 | Replace direct cast `(Activity)activity` with safe cast + `ArgumentException`. Apply to all 5 call sites. | `CompatTeamsInfo.cs` | +| **3.5** | 009 | Extract magic string to constant. Centralize `IsCancellationByUser()` helper. Add fallback log for unmatched `HttpRequestException`. | `TeamsStreamingWriter.cs` | +| **3.6** | 022 | Add `ArgumentException.ThrowIfNullOrWhiteSpace(ServiceUrl)` to 2 methods. | `CompatConversations.cs` | + +**Validation:** Add tests for malformed GUIDs, malformed tokens, null ServiceUrl, non-Activity IActivity. + +--- + +### Phase 4 — Code hygiene & minor improvements +**Goal:** Clean up low-risk issues, improve performance, enforce patterns. +**Scope:** 5 work items. + +| Work Item | Audits | What to Do | Files | +|-----------|--------|------------|-------| +| **4.1** | 010 | Replace `string +=` with `StringBuilder` in streaming accumulator. | `TeamsStreamingWriter.cs` | +| **4.2** | 014 | Return a defensive copy from `TeamsAttachmentBuilder.Build()`. | `TeamsAttachmentBuilder.cs` | +| **4.3** | 020 | Freeze middleware list after first pipeline execution. Throw on late `Use()` calls. | `TurnMiddleware.cs` | +| **4.4** | 015 | Deep-copy `CitationClaim` list or make `CitationClaim` a record with `init`-only setters. | `CitationEntity.cs` | +| **4.5** | 019 | Remove `GC.SuppressFinalize` and misleading comment from `CompatConnectorClient.Dispose()`. | `CompatConnectorClient.cs` | + +**Validation:** Run full test suite. Confirm no behavioral changes. + +--- + +## Summary + +| Phase | Items | Audits Covered | Focus | +|-------|-------|----------------|-------| +| 1 | 4 | 001, 003, 006, 011, 012, 023 | Critical concurrency & data integrity | +| 2 | 5 | 002, 004, 005, 007, 008, 021 | Correctness & DI hygiene | +| 3 | 6 | 009, 013, 016, 017, 018, 022 | Robustness & validation | +| 4 | 5 | 010, 014, 015, 019, 020 | Code hygiene & perf | +| **Total** | **20** | **23 audits → 22 unique → 20 work items** | | diff --git a/core/docs/stabilization/STABILIZATION-PLAN.md b/core/docs/stabilization/STABILIZATION-PLAN.md new file mode 100644 index 00000000..8f80e452 --- /dev/null +++ b/core/docs/stabilization/STABILIZATION-PLAN.md @@ -0,0 +1,181 @@ +# Core Stabilization Plan + +> Generated: 2026-04-11 +> Source: Audits 001–023 in this directory +> Branch: `next/core-rido-audit` + +--- + +## Step 1 — Duplicate / Overlap Analysis + +Five consolidation groups were identified. Items within a group share a root cause or a single fix resolves all of them. + +| ID | Merged Audits | Rationale | +|----|--------------|-----------| +| **SER-01** | 003 + 011 | Both target the same `ActivitySerializerMap` gap. Audit 011 is a downstream symptom of 003; fixing 003 eliminates 011. | +| **CAST-01** | 004 + 023 | Same anti-pattern: `as`-cast silently returns `null`. Audit 004 is in `TeamsActivity.cs` property shadowing; 023 is in entity files via `[JsonExtensionData]`. One workstream, two locations. | +| **COPY-01** | 013 + 015 | Both are shallow-copy bugs on mutable reference types. Fix together to establish a consistent deep-copy pattern. | +| **DI-01** | 005 + 006 | Same file (`JwtExtensions.cs`), same theme: DI container misuse and resource leaks. Fixing `BuildServiceProvider()` abuse also removes the scope that makes the `ConfigurationManager` leak possible. | +| **AUTH-01** | 017 + 018 | Same file (`BotAuthenticationHandler.cs`). Both crash on malformed external input on the hot request path. | + +After consolidation: **23 raw audits → 18 work items.** + +--- + +## Step 2 — Impact-Ranked Issue List + +### Critical — Active production risk (data corruption / service outage) + +| # | ID | Title | Affected File(s) | +|---|----|-------|-----------------| +| 1 | A-012 | Thread-safety race on `OnActivity` — concurrent requests corrupt each other's handler | `CompatAdapter.cs:57–66` | +| 2 | A-001 | Blocking `.GetAwaiter().GetResult()` in JWT key resolver — thread-pool starvation under load | `JwtExtensions.cs:222` | + +### High — Silent data loss or crash on real inputs + +| # | ID | Title | Affected File(s) | +|---|----|-------|-----------------| +| 3 | AUTH-01 (017+018) | Auth handler crashes on malformed GUID or token — takes down the whole request | `BotAuthenticationHandler.cs:99,108–113` | +| 4 | SER-01 (003+011) | Asymmetric serializer map — 6 of 8 activity subtypes lose all subtype fields on serialization | `TeamsActivityType.cs:83–103` | +| 5 | CAST-01 (004+023) | `as`-cast silently returns `null` for deserialized data — property reads always return `null` post-deserialization | `TeamsActivity.cs:108–142`, entity files | +| 6 | A-002 | Unsafe `(T)(object)` double-cast in HTTP deserializer — fragile runtime assumption, crashes on type mismatch | `BotHttpClient.cs:216,220` | +| 7 | DI-01 (005+006) | Multiple `BuildServiceProvider()` calls create duplicate singletons; `ConfigurationManager` never disposed | `JwtExtensions.cs`, `AddBotApplicationExtensions.cs`, `BotConfig.cs` | + +### Medium — Correctness defects with narrower blast radius + +| # | ID | Title | Affected File(s) | +|---|----|-------|-----------------| +| 8 | A-021 | Cross-framework JSON round-trip (Newtonsoft → STJ) drops fields silently in compat layer | `CompatTeamsInfo.cs:357–358` | +| 9 | A-016 | Direct `(Activity)` cast on `IActivity` — throws `InvalidCastException` for any non-`Activity` implementation | `CompatTeamsInfo.cs:452,484,516,543,582` | +| 10 | COPY-01 (013+015) | Shallow copy in `CoreActivity` and `CitationEntity` — mutations of a copy affect the original | `CoreActivity.cs:140–156`, `CitationEntity.cs:115–130` | +| 11 | A-014 | Builder exposes internal mutable ref — post-`Build()` mutations corrupt already-returned attachment | `TeamsAttachmentBuilder.cs:110` | +| 12 | A-008 | Unknown entity types silently discarded — any unrecognized entity type is dropped at deserialization | `Entity.cs:66–74` | +| 13 | A-007 | `object.ToString()` on `JsonElement` values produces type names, not data — silent string corruption in compat | `CompatActivity.cs:75–109,238–265` | +| 14 | A-009 | Fragile exception filter matches cancellation by message string — breaks on any SDK wording change | `TeamsStreamingWriter.cs:104–107` | + +### Low — Quality / hygiene (no immediate production risk) + +| # | ID | Title | Affected File(s) | +|---|----|-------|-----------------| +| 15 | A-010 | O(n²) string concatenation accumulator in streaming — degrades under high chunk counts | `TeamsStreamingWriter.cs:92` | +| 16 | A-020 | Plain `List` for middleware — no enforcement that writes stop after startup | `TurnMiddleware.cs:21` | +| 17 | A-022 | Null-forgiving operator on nullable `ServiceUrl` — throws `NullReferenceException` if null | `CompatConversations.cs:123,203` | +| 18 | A-019 | `GC.SuppressFinalize` called on class with no finalizer — misleading boilerplate | `CompatConnectorClient.cs:38–42` | + +--- + +## Step 3 — Phased Fix Plan + +### Phase 1 — Stop the bleeding (Critical) + +**Goal:** Eliminate active production risks. No new features until these are closed. + +#### P1-1: Fix race condition on `OnActivity` (A-012) +- **File:** `core/src/Microsoft.Teams.Bot.Compat/CompatAdapter.cs` +- **Fix:** Replace the singleton `OnActivity` field with `AsyncLocal>` scoped per-request, or refactor `ProcessActivityAsync` to accept the callback as a parameter and remove the field entirely. +- **Test:** Write a concurrent integration test that fires ≥10 simultaneous requests and asserts each request's handler is invoked exactly once on the correct activity. + +#### P1-2: Fix blocking async in JWT key resolver (A-001) +- **File:** `core/src/Microsoft.Teams.Bot.Core/Hosting/JwtExtensions.cs` +- **Fix:** Move key fetching to an eagerly-started background refresh (`Lazy>` + `Timer`), or implement a custom `ISecurityTokenValidator` / `IOpenIdConnectConfigurationRetriever` that supports async. Remove all `.GetAwaiter().GetResult()` calls in the resolver delegate. +- **Test:** Load test JWT validation under 50 concurrent requests; assert no `ThreadPoolStarvationException` and p99 latency stays flat. + +--- + +### Phase 2 — Data integrity (High) + +**Goal:** Prevent silent data loss and request crashes. + +#### P2-1: Fix auth handler crashes on malformed input (AUTH-01: 017 + 018) +- **Files:** `core/src/Microsoft.Teams.Bot.Core/Hosting/BotAuthenticationHandler.cs`, `core/src/Microsoft.Teams.Bot.Compat/KeyedBotAuthenticationHandler.cs` +- **Fix (017):** Replace `Guid.Parse(agenticUserId)` with `Guid.TryParse`; return `AuthenticateResult.Fail` with a descriptive message on invalid input. +- **Fix (018):** Wrap `new JwtSecurityToken(token)` in try/catch; replace deprecated `JwtSecurityToken` with `JsonWebToken`; log parse failure at trace level and continue (do not crash). + +#### P2-2: Fix asymmetric serializer map (SER-01: 003 + 011) +- **File:** `core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivityType.cs` +- **Fix:** Add the 6 missing entries to `ActivitySerializerMap` to match `ActivityDeserializerMap`; register types in `TeamsActivityJsonContext`; add a CI unit test that asserts `ActivitySerializerMap.Count == ActivityDeserializerMap.Count` and that a round-trip of each type preserves all subtype fields. +- **Remove:** The silent fallback path in `ToJson()` (replace with `Debug.Fail` / production log). + +#### P2-3: Fix `as`-cast silent null returns (CAST-01: 004 + 023) +- **Files:** `TeamsActivity.cs`, `CitationEntity.cs`, `MentionEntity.cs`, `OMessageEntity.cs`, `SensitiveUsageEntity.cs` +- **Fix (023/entity files):** Replace `[JsonExtensionData]`-backed property getters that use `as` with explicit `[JsonPropertyName]` properties (preferred), or add `JsonElement`-aware getters that deserialize on access and cache the result. +- **Fix (004/TeamsActivity):** Add null guards or assertions in shadowed property getters; enforce correct concrete type in the `Rebase()` path / setter so the `as` cast cannot fail at runtime. + +#### P2-4: Fix unsafe generic cast (A-002) +- **File:** `core/src/Microsoft.Teams.Bot.Core/Http/BotHttpClient.cs` +- **Fix:** Extract a typed helper that checks `typeof(T)` explicitly and returns the string directly for `T == string`; use `JsonSerializer.Deserialize()` for all other types. Remove the `(T)(object)` pattern entirely. + +#### P2-5: Fix DI container misuse and resource leak (DI-01: 005 + 006) +- **Files:** `JwtExtensions.cs`, `AddBotApplicationExtensions.cs`, `BotConfig.cs` +- **Fix (005):** Remove all internal `BuildServiceProvider()` calls during registration. Pass `IConfiguration` explicitly through extension method parameters; fail fast with a clear `InvalidOperationException` if unavailable rather than building a throwaway container. +- **Fix (006):** Extract an `OidcConfigCache : IDisposable` singleton registered with the DI container. Move `ConfigurationManager` instances into it so they are disposed with the application lifetime. + +--- + +### Phase 3 — Correctness & reliability (Medium) + +**Goal:** Eliminate narrower-scope correctness bugs before any public API stabilization. + +#### P3-1: Fix cross-framework JSON round-trip (A-021) +- **File:** `core/src/Microsoft.Teams.Bot.Compat/CompatTeamsInfo.cs` +- **Fix:** Use a single serializer (STJ) for both legs of the conversion, or implement explicit manual mapping between the two object graphs. Add a round-trip fidelity integration test covering all known property names. + +#### P3-2: Fix unsafe `IActivity` cast (A-016) +- **File:** `core/src/Microsoft.Teams.Bot.Compat/CompatTeamsInfo.cs` +- **Fix:** Replace the five direct `(Activity)` casts with `activity as Activity ?? throw new ArgumentException(...)`. Consider changing the public signature to accept `Activity` directly if `IActivity` is not meaningfully polymorphic here. + +#### P3-3: Fix shallow copies (COPY-01: 013 + 015) +- **Files:** `CoreActivity.cs`, `CitationEntity.cs` +- **Fix (013):** In the `CoreActivity(CoreActivity)` copy constructor, deep-copy all mutable reference-type fields: `ChannelData` (re-serialize/deserialize or clone), `Entities` (new list + clone each), `Attachments`, `Properties`, `Value`. +- **Fix (015):** Deep-copy each `CitationClaim` in the `CitationEntity` clone, or convert `CitationClaim` to a `record` with `init`-only properties so shared references cannot be mutated. + +#### P3-4: Fix builder mutable reference leak (A-014) +- **File:** `core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsAttachmentBuilder.cs` +- **Fix:** In `Build()`, return a copy of `_attachment` rather than the field itself. Alternatively, set a `_built` flag and throw `InvalidOperationException` on any subsequent builder call after `Build()` is called. + +#### P3-5: Preserve unknown entity types (A-008) +- **File:** `core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/Entity.cs` +- **Fix:** For unrecognized types, deserialize as base `Entity` (raw JSON preserved via `[JsonExtensionData]`) rather than returning `null`. Add trace logging for unknown type names to aid future registration. + +#### P3-6: Fix `ToString()` on `JsonElement` values in compat (A-007) +- **File:** `core/src/Microsoft.Teams.Bot.Compat/CompatActivity.cs` +- **Fix:** Add a typed extraction helper that checks `value is JsonElement je ? je.GetString() : value?.ToString()` (with appropriate handling for non-string `JsonElement` kinds). Apply to all `object` dictionary read sites. + +#### P3-7: Fix fragile stream-cancellation detection (A-009) +- **File:** `core/src/Microsoft.Teams.Bot.Apps/TeamsStreamingWriter.cs` +- **Fix:** Inspect the HTTP status code or a structured error code from the Teams API response instead of matching against the exception message string. Document the expected cancellation signal in a comment. + +--- + +### Phase 4 — Quality & hygiene (Low) + +**Goal:** Clean up low-risk items that accumulate tech debt. + +#### P4-1: Replace O(n²) string accumulator with `StringBuilder` (A-010) +- **File:** `core/src/Microsoft.Teams.Bot.Apps/TeamsStreamingWriter.cs` +- **Fix:** Replace `_accumulated += chunk` with `_accumulated = new StringBuilder()` + `Append(chunk)`. Update all read sites to call `.ToString()`. + +#### P4-2: Freeze middleware list after startup (A-020) +- **File:** `core/src/Microsoft.Teams.Bot.Core/TurnMiddleware.cs` +- **Fix:** Add a `Freeze()` method that converts `List` to an array. Call `Freeze()` from the host's `IHostedService.StartAsync`. Guard read paths against post-freeze mutations via assertion. + +#### P4-3: Add `ServiceUrl` null guard (A-022) +- **File:** `core/src/Microsoft.Teams.Bot.Compat/CompatConversations.cs` +- **Fix:** Add `ArgumentException.ThrowIfNullOrWhiteSpace(ServiceUrl)` at the top of each affected method before the `new Uri(ServiceUrl!)` call. + +#### P4-4: Remove misleading `GC.SuppressFinalize` (A-019) +- **File:** `core/src/Microsoft.Teams.Bot.Compat/CompatConnectorClient.cs` +- **Fix:** Delete `GC.SuppressFinalize(this)` and the referenced-but-nonexistent `Dispose(bool)` comment. Simplify `Dispose()` to a no-op with a comment explaining there is nothing to release. + +--- + +## Summary Table + +| Phase | Items | Severity | Gate | +|-------|-------|----------|------| +| 1 | P1-1, P1-2 | Critical | Must ship before any load-bearing traffic change | +| 2 | P2-1 → P2-5 | High | Must ship before public API stabilization | +| 3 | P3-1 → P3-7 | Medium | Must ship before `next/core` merges to `main` | +| 4 | P4-1 → P4-4 | Low | Can ship as clean-up PRs alongside other work | + +**Total work items after consolidation: 18** (down from 23 raw audits, 5 merges applied). diff --git a/core/docs/stabilization/audit-001-blocking-async-jwt.md b/core/docs/stabilization/audit-001-blocking-async-jwt.md new file mode 100644 index 00000000..58fde9b6 --- /dev/null +++ b/core/docs/stabilization/audit-001-blocking-async-jwt.md @@ -0,0 +1,73 @@ +# Audit Issue 001: Blocking Async Call in JWT Signing-Key Resolver + +**Severity:** Critical +**File:** `core/src/Microsoft.Teams.Bot.Core/Hosting/JwtExtensions.cs` +**Line:** 222 +**Category:** Async/Await correctness + +--- + +## Problem + +Inside the `IssuerSigningKeyResolver` delegate — which is invoked synchronously by the ASP.NET Core JWT middleware during token validation — there is a blocking call: + +```csharp +// JwtExtensions.cs, line 222 +OpenIdConnectConfiguration config = manager.GetConfigurationAsync(CancellationToken.None).GetAwaiter().GetResult(); +``` + +`IssuerSigningKeyResolver` is a synchronous `Func<>` callback. Because ASP.NET Core uses a thread-pool-based async pipeline, calling `.GetAwaiter().GetResult()` here risks **thread-pool starvation**: the thread that calls `GetResult()` is blocked waiting for the async operation to complete, but that operation may itself need a thread-pool thread to continue, leading to a deadlock or severe latency degradation under load. + +Additionally, `CancellationToken.None` is passed — if the HTTP fetch for the OIDC metadata hangs, there is no timeout or cancellation path. + +--- + +## Root Cause + +`TokenValidationParameters.IssuerSigningKeyResolver` is defined as: + +```csharp +IssuerSigningKeyResolver = (string token, SecurityToken securityToken, string kid, TokenValidationParameters validationParameters) + => IEnumerable +``` + +This is a **synchronous** delegate type. There is no async equivalent in the `Microsoft.IdentityModel.Tokens` library for this callback, so an async fetch must be run synchronously here. The current approach is a common workaround that is known to be unsafe in production ASP.NET Core applications. + +--- + +## Suggested Fix Plan + +### Option A — Cache keys eagerly via background refresh (preferred) + +Pre-fetch and cache the signing keys in a `Lazy>>` or a background `Timer`, so the synchronous resolver can return from an already-populated in-memory cache without ever blocking: + +1. Replace the `ConcurrentDictionary>` with a `ConcurrentDictionary>>`. +2. On first access, start the async fetch on a **background task** (fire-and-forget with error handling). Return an empty key set (authentication will fail-fast) until the cache warms up. +3. Schedule periodic refresh (e.g., every 24 hours) to pick up key rotations. +4. The resolver reads from the cache synchronously — no blocking calls. + +### Option B — Use `ConfigurationManager` with synchronous HTTP (acceptable interim) + +The `ConfigurationManager` internally uses `HttpDocumentRetriever`. Replace it with a synchronous HTTP call using `HttpClient.Send()` (not `SendAsync`) so the blocking is explicit and understood, rather than disguised as async: + +```csharp +// Temporary mitigation while a full async cache is implemented. +// Clearly mark this with a TODO comment and a work item. +var httpClient = new HttpClient(); +var json = httpClient.GetStringAsync(authority).GetAwaiter().GetResult(); // Known blocking call +``` + +This does not fix starvation but makes the intent explicit and avoids nested async context issues. + +### Option C — Switch to a custom `ISecurityTokenValidator` (cleanest long-term) + +Implement a custom `JwtBearerOptions.SecurityTokenValidators` entry or override `OnTokenValidated` to perform async key resolution before the synchronous validator runs, caching keys into `TokenValidationParameters.IssuerSigningKeys` on each request. + +--- + +## Acceptance Criteria + +- No `.GetAwaiter().GetResult()` or `.Result` on a `Task` inside the `IssuerSigningKeyResolver` delegate. +- Signing-key resolution does not block a thread-pool thread under concurrent load. +- Key refresh still happens — stale keys do not cause permanent authentication failures. +- Existing integration tests for JWT validation continue to pass. diff --git a/core/docs/stabilization/audit-002-unsafe-generic-cast.md b/core/docs/stabilization/audit-002-unsafe-generic-cast.md new file mode 100644 index 00000000..c05c6db5 --- /dev/null +++ b/core/docs/stabilization/audit-002-unsafe-generic-cast.md @@ -0,0 +1,104 @@ +# Audit Issue 002: Unsafe `(T)(object)` Cast in Generic HTTP Response Deserializer + +**Severity:** Critical +**File:** `core/src/Microsoft.Teams.Bot.Core/Http/BotHttpClient.cs` +**Lines:** 216, 220 +**Category:** Type safety + +--- + +## Problem + +In `DeserializeResponseAsync`, there is a special-case branch for `T == string` that uses a double-cast to work around C#'s type system: + +```csharp +// BotHttpClient.cs, lines 211-222 +if (typeof(T) == typeof(string)) +{ + try + { + T? result = JsonSerializer.Deserialize(responseString, DefaultJsonOptions); + return result ?? (T)(object)responseString; // line 216 + } + catch (JsonException) + { + return (T)(object)responseString; // line 220 + } +} +``` + +The pattern `(T)(object)responseString` performs: +1. **Boxing** — `responseString` (a `string`) is cast to `object`. +2. **Unboxing cast** — the `object` is cast to `T`. + +At runtime, step 2 throws `InvalidCastException` if `T` is not `string`. While the `typeof(T) == typeof(string)` guard _should_ ensure `T` is always `string` here, this is a fragile assumption that: + +- Breaks if a future refactor or compiler change reorders branches. +- Is invisible to the type checker — the compiler accepts it without warning. +- Silently bypasses generic type constraints. +- Can confuse future developers into reusing the pattern in contexts where `T` is not `string`. + +--- + +## Root Cause + +C# generics are reified at runtime but the type system still prevents direct assignment of `string` to `T` without a constraint. The `(T)(object)` cast is a commonly used but semantically unsafe workaround to force a value of a known concrete type into a generic parameter. + +--- + +## Suggested Fix Plan + +### Step 1 — Use `Unsafe.As` or a helper method + +Replace the double-cast with `System.Runtime.CompilerServices.Unsafe.As`: + +```csharp +if (typeof(T) == typeof(string)) +{ + T? result; + try + { + result = JsonSerializer.Deserialize(responseString, DefaultJsonOptions); + } + catch (JsonException) + { + result = default; + } + + if (result is not null) + return result; + + // Return the raw string. Safe because we checked typeof(T) == typeof(string). + string raw = responseString; + return Unsafe.As(ref raw); +} +``` + +`Unsafe.As` avoids the boxing/unboxing round-trip and is the established .NET pattern for this scenario (used throughout `System.Collections.Generic` internals). + +### Step 2 — Alternatively, extract a typed helper + +```csharp +private static T ReturnRawString(string value) +{ + // Called only when T is string; validated by caller. + if (typeof(T) != typeof(string)) + throw new InvalidOperationException($"ReturnRawString called with T={typeof(T).Name}"); + return (T)(object)value; // The cast is safe here; the throw above makes it explicit. +} +``` + +This makes the invariant explicit and testable, unlike the inline cast. + +### Step 3 — Add a unit test + +Add a unit test that calls `SendAsync` against a mock HTTP response that returns a non-JSON plain-text body. Confirm the method returns the raw string without throwing `InvalidCastException`. + +--- + +## Acceptance Criteria + +- No unguarded `(T)(object)` casts anywhere in `BotHttpClient.cs`. +- `SendAsync` with a non-JSON response body returns the raw string. +- `SendAsync` with a JSON-encoded string (`"hello"`) returns `hello`. +- All existing `BotHttpClient` unit/integration tests continue to pass. diff --git a/core/docs/stabilization/audit-003-asymmetric-serializer-maps.md b/core/docs/stabilization/audit-003-asymmetric-serializer-maps.md new file mode 100644 index 00000000..a160740c --- /dev/null +++ b/core/docs/stabilization/audit-003-asymmetric-serializer-maps.md @@ -0,0 +1,109 @@ +# Audit Issue 003: Asymmetric Serializer / Deserializer Maps Causing Silent Data Loss + +**Severity:** High +**File:** `core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivityType.cs` +**Lines:** 83–103 +**Category:** Data integrity / type dispatch + +--- + +## Problem + +`TeamsActivityType` defines two dictionaries for activity type dispatch: + +```csharp +// Deserializer map — 8 entries (line 83) +internal static readonly Dictionary> ActivityDeserializerMap = new() +{ + [Message] = MessageActivity.FromActivity, + [MessageReaction] = MessageReactionActivity.FromActivity, + [MessageUpdate] = MessageUpdateActivity.FromActivity, + [MessageDelete] = MessageDeleteActivity.FromActivity, + [ConversationUpdate] = ConversationUpdateActivity.FromActivity, + [InstallationUpdate] = InstallUpdateActivity.FromActivity, + [Invoke] = InvokeActivity.FromActivity, + [Event] = EventActivity.FromActivity +}; + +// Serializer map — only 2 entries (line 99) +internal static readonly Dictionary> ActivitySerializerMap = new() +{ + [typeof(MessageActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.MessageActivity), + [typeof(StreamingActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.StreamingActivity), +}; +``` + +When `TeamsActivity.ToJson()` is called on an instance of `MessageReactionActivity`, `InvokeActivity`, `ConversationUpdateActivity`, `MessageUpdateActivity`, `MessageDeleteActivity`, `InstallUpdateActivity`, or `EventActivity`, the serializer map lookup **misses** and the code falls back to the base `TeamsActivity` serializer: + +```csharp +// TeamsActivity.cs, lines 34-37 +public override string ToJson() + => TeamsActivityType.ActivitySerializerMap.TryGetValue(GetType(), out Func? serializer) + ? serializer(this) + : ToJson(TeamsActivityJsonContext.Default.TeamsActivity); // Fallback — loses subtype fields! +``` + +This means **6 out of 8 deserializable activity types lose their subtype-specific fields** when serialized back to JSON. This affects any code path that round-trips an activity (e.g., sending a reply, logging, or passing an activity through the compat layer). + +--- + +## Root Cause + +The serializer map was not kept in sync with the deserializer map when new activity types were added. There is no compile-time or runtime enforcement of symmetry between the two dictionaries. + +--- + +## Suggested Fix Plan + +### Step 1 — Add missing serializer entries + +For each activity type present in `ActivityDeserializerMap` but absent from `ActivitySerializerMap`, add a corresponding entry. Each entry needs a registered `JsonTypeInfo` in `TeamsActivityJsonContext`. + +Add the following entries to `ActivitySerializerMap`: + +```csharp +[typeof(MessageReactionActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.MessageReactionActivity), +[typeof(MessageUpdateActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.MessageUpdateActivity), +[typeof(MessageDeleteActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.MessageDeleteActivity), +[typeof(ConversationUpdateActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.ConversationUpdateActivity), +[typeof(InstallUpdateActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.InstallUpdateActivity), +[typeof(InvokeActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.InvokeActivity), +[typeof(EventActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.EventActivity), +``` + +### Step 2 — Register types in `TeamsActivityJsonContext` + +For each new entry above, ensure the corresponding type is registered in `TeamsActivityJsonContext` with `[JsonSerializable(typeof(XxxActivity))]`. Check `TeamsActivityJsonContext.cs` for what is already registered and add what is missing. + +### Step 3 — Add a symmetry assertion test + +Add a unit test that asserts `ActivitySerializerMap.Keys` covers every value type returned by `ActivityDeserializerMap.Values` (i.e., the return type of each factory). Run this test in CI to prevent future drift: + +```csharp +[Fact] +public void SerializerMap_CoversAllDeserializerOutputTypes() +{ + var deserializedTypes = TeamsActivityType.ActivityDeserializerMap.Values + .Select(factory => factory(new CoreActivity { Type = "message" }).GetType()) + .Distinct(); + + foreach (var type in deserializedTypes) + Assert.True(TeamsActivityType.ActivitySerializerMap.ContainsKey(type), + $"No serializer registered for {type.Name}"); +} +``` + +### Step 4 — Verify round-trip fidelity + +Add integration tests that: +1. Deserialize a JSON payload for each activity type. +2. Call `ToJson()` on the result. +3. Assert the output JSON contains the type-specific fields (e.g., `replyToId` for `MessageReactionActivity`, `value` for `InvokeActivity`). + +--- + +## Acceptance Criteria + +- `ActivitySerializerMap` has an entry for every type that `ActivityDeserializerMap` can produce. +- Round-trip serialization preserves all subtype-specific fields for all 8 activity types. +- A new CI test fails if the two maps become asymmetric again. diff --git a/core/docs/stabilization/audit-004-property-shadow-as-casts.md b/core/docs/stabilization/audit-004-property-shadow-as-casts.md new file mode 100644 index 00000000..8d9efa07 --- /dev/null +++ b/core/docs/stabilization/audit-004-property-shadow-as-casts.md @@ -0,0 +1,138 @@ +# Audit Issue 004: Silent `as`-Cast Failures in `TeamsActivity` Property Shadowing + +**Severity:** High +**File:** `core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivity.cs` +**Lines:** 108–142 +**Category:** Type safety / Liskov Substitution Principle + +--- + +## Problem + +`TeamsActivity` shadows four base-class properties using the `new` keyword. Each getter performs an `as` cast on the base property: + +```csharp +// TeamsActivity.cs, line 108-142 +[JsonPropertyName("from")] +public new TeamsConversationAccount? From +{ + get => base.From as TeamsConversationAccount; // silently null if wrong type + set => base.From = value; +} + +[JsonPropertyName("recipient")] +public new TeamsConversationAccount? Recipient +{ + get => base.Recipient as TeamsConversationAccount; + set => base.Recipient = value; +} + +[JsonPropertyName("conversation")] +public new TeamsConversation? Conversation +{ + get => base.Conversation as TeamsConversation; + set => base.Conversation = value; +} + +[JsonPropertyName("channelData")] +public new TeamsChannelData? ChannelData +{ + get => base.ChannelData as TeamsChannelData; + set => base.ChannelData = value; +} +``` + +The `as` operator returns `null` if the object is not of the target type — **it never throws**. This creates two categories of silent failure: + +1. **Type mismatch at runtime** — If the base property was set to a `ConversationAccount` (not a `TeamsConversationAccount`), `From` returns `null` with no diagnostic. Callers that assume the property is non-null (because it was set) receive `null` and may NullReferenceException later, far from the assignment site. + +2. **Polymorphism violation** — Code holding a `CoreActivity` reference gets different property values than code holding a `TeamsActivity` reference to the _same object_. This violates the Liskov Substitution Principle and can cause subtle bugs in generic code that accepts `CoreActivity`. + +The constructor (`TeamsActivity(CoreActivity activity)`) at lines 62–89 does set the Teams-typed subtypes explicitly, so the normal deserialization path is safe. The risk arises from: +- Direct assignment to `base.From = someConversationAccount` (which bypasses the Teams subtype). +- Activities created via the compat layer (`CompatActivity.cs`) that may set non-Teams types. +- Future code changes that assign to the base type without realising the derived getter will silently return null. + +--- + +## Root Cause + +The `new` keyword intentionally shadows the base property to return a more-derived type. However, the getter assumes the base field always holds the derived subtype — an invariant that is not enforced by the type system and can be broken externally. + +--- + +## Suggested Fix Plan + +### Option A — Add a debug-mode assertion in each getter (minimal change) + +Add a `Debug.Assert` so the mismatch is caught during development/testing without impacting production performance: + +```csharp +public new TeamsConversationAccount? From +{ + get + { + System.Diagnostics.Debug.Assert( + base.From is null or TeamsConversationAccount, + $"TeamsActivity.From expected TeamsConversationAccount but found {base.From?.GetType().Name}"); + return base.From as TeamsConversationAccount; + } + set => base.From = value; +} +``` + +Apply the same pattern to `Recipient`, `Conversation`, and `ChannelData`. + +### Option B — Enforce the invariant in the setter (preferred) + +Wrap the base setter to ensure only the correct subtype can be stored, upgrading a plain `ConversationAccount` if one is passed: + +```csharp +public new TeamsConversationAccount? From +{ + get => base.From as TeamsConversationAccount; + set => base.From = value; // Already typed as TeamsConversationAccount? — no plain ConversationAccount can be set through this property. +} +``` + +The setter is already typed correctly (`TeamsConversationAccount?`). The issue is that `base.From = value` is `ConversationAccount?`, so a caller accessing `base.From` directly and assigning a plain `ConversationAccount` bypasses the Teams-typed setter. To guard against this, seal or hide the base setter: + +If `CoreActivity.From` is not `virtual` (check the base class), consider adding validation in `Rebase()` that re-promotes any plain `ConversationAccount` to `TeamsConversationAccount`: + +```csharp +internal TeamsActivity Rebase() +{ + // Promote base-typed From to TeamsConversationAccount if needed + if (base.From is not null and not TeamsConversationAccount) + base.From = TeamsConversationAccount.FromConversationAccount(base.From); + + // ... same for Recipient, Conversation, ChannelData + base.Attachments = Attachments?.ToJsonArray(); + base.Entities = Entities?.ToJsonArray(); + return this; +} +``` + +### Option C — Add a unit test for type consistency (minimum viable) + +At a minimum, add tests that: + +```csharp +[Fact] +public void TeamsActivity_From_ReturnsNull_WhenBaseIsPlainConversationAccount() +{ + // Document the known limitation explicitly so future developers understand the contract. + var activity = new TeamsActivity(); + ((CoreActivity)activity).From = new ConversationAccount { Id = "x" }; + Assert.Null(activity.From); // Known: returns null, not the base value +} +``` + +This makes the behaviour explicit rather than surprising. + +--- + +## Acceptance Criteria + +- All property getters that perform `as` casts have either a `Debug.Assert`, runtime validation in `Rebase()`, or an explicit test documenting the null-return contract. +- No production code path silently reads `null` from `From`, `Recipient`, `Conversation`, or `ChannelData` on a `TeamsActivity` that had those fields set on the base. diff --git a/core/docs/stabilization/audit-005-build-service-provider.md b/core/docs/stabilization/audit-005-build-service-provider.md new file mode 100644 index 00000000..00a4728e --- /dev/null +++ b/core/docs/stabilization/audit-005-build-service-provider.md @@ -0,0 +1,109 @@ +# Audit Issue 005: `BuildServiceProvider()` Called Multiple Times During Startup + +**Severity:** High +**Files:** +- `core/src/Microsoft.Teams.Bot.Core/Hosting/AddBotApplicationExtensions.cs` — line 224 +- `core/src/Microsoft.Teams.Bot.Core/Hosting/BotConfig.cs` — line 148 +- `core/src/Microsoft.Teams.Bot.Core/Hosting/JwtExtensions.cs` — line 324 + +**Category:** Memory management / DI correctness + +--- + +## Problem + +In three separate places, `services.BuildServiceProvider()` is called during the DI registration phase (i.e., inside extension methods called from `Program.cs` / `Startup.cs`): + +```csharp +// AddBotApplicationExtensions.cs, line 224 +using ServiceProvider tempProvider = services.BuildServiceProvider(); +ILoggerFactory? tempFactory = tempProvider.GetService(); +return (ILogger?)tempFactory?.CreateLogger(...) + ?? NullLogger.Instance; + +// BotConfig.cs, line 148 +IConfiguration configuration = configDescriptor?.ImplementationInstance as IConfiguration + ?? services.BuildServiceProvider().GetRequiredService(); + +// JwtExtensions.cs, line 324 +IConfiguration configuration = configDescriptor?.ImplementationInstance as IConfiguration + ?? services.BuildServiceProvider().GetRequiredService(); +``` + +Each call to `BuildServiceProvider()` creates a **new, independent DI container** that: + +1. Instantiates all registered singleton services eagerly (or on first resolution), potentially starting background threads, opening connections, or allocating large objects. +2. **Leaks resources** unless explicitly disposed. The `using` in `AddBotApplicationExtensions.cs` disposes the temp provider correctly, but `BotConfig.cs` and `JwtExtensions.cs` do not — the returned `ServiceProvider` is abandoned without disposal. +3. Triggers ASP.NET Core's built-in analyzer warning `ASP0000`: _"Calling 'BuildServiceProvider' from application code results in an additional copy of singleton services being created."_ +4. Is called potentially 3+ times per application startup (once per `AddConversationClient`, `AddUserTokenClient`, `AddBotAuthentication`, and `AddBotAuthorization` call), each producing a separate leaked container. + +--- + +## Root Cause + +The extension methods need access to `IConfiguration` and `ILoggerFactory` at registration time, before the DI container is built. Rather than requiring the caller to pass these in explicitly, the methods try to extract them from the `IServiceCollection` directly. When that fails (e.g., when the `IConfiguration` is registered as a factory rather than an instance), they fall back to building a temporary provider. + +--- + +## Suggested Fix Plan + +### Step 1 — Extract `IConfiguration` without building a provider + +`IConfiguration` is almost always registered as an `ImplementationInstance` (not a factory) in ASP.NET Core. The extraction already tries this: + +```csharp +IConfiguration configuration = configDescriptor?.ImplementationInstance as IConfiguration + ?? services.BuildServiceProvider().GetRequiredService(); +``` + +The fallback should **throw** instead of building a provider, with a clear error message: + +```csharp +IConfiguration configuration = configDescriptor?.ImplementationInstance as IConfiguration + ?? throw new InvalidOperationException( + "IConfiguration must be registered before calling AddBotApplication. " + + "Call builder.Configuration or services.AddSingleton(...) first."); +``` + +In practice, `IConfiguration` is always available as an instance in modern ASP.NET Core apps (`WebApplicationBuilder` registers it that way). Removing the fallback makes any misconfiguration fail fast with a clear message. + +### Step 2 — Pass `IConfiguration` as a parameter where needed + +Alternatively, add an overload that accepts `IConfiguration` directly: + +```csharp +public static BotConfig Resolve(IConfiguration configuration, string sectionName = "AzureAd", ILogger? logger = null) +// (This overload already exists in BotConfig.cs — use it consistently) +``` + +All call sites that currently call `BotConfig.Resolve(services, sectionName)` should be audited to see whether the `IConfiguration` is already available in scope, and if so, call the `Resolve(IConfiguration, ...)` overload directly. + +### Step 3 — Remove `GetLoggerFromServices` / replace with `NullLogger` at registration time + +The `GetLoggerFromServices` helper in `AddBotApplicationExtensions.cs` builds a temporary provider to get a logger during service registration. Logging during registration is a convenience, not a requirement. The simplest fix: + +```csharp +internal static ILogger GetLoggerFromServices(IServiceCollection services, Type? categoryType = null) +{ + // Only use instance-registered loggers; never build a provider. + ServiceDescriptor? descriptor = services.FirstOrDefault(d => d.ServiceType == typeof(ILoggerFactory)); + ILoggerFactory? factory = descriptor?.ImplementationInstance as ILoggerFactory; + return factory?.CreateLogger(categoryType ?? typeof(AddBotApplicationExtensions)) + ?? NullLogger.Instance; +} +``` + +Remove the `BuildServiceProvider()` fallback entirely. Configuration-time logging is nice-to-have; causing resource leaks or DI duplication is not acceptable. + +### Step 4 — Suppress or fix the `ASP0000` analyzer warning + +After the fix, verify that the `dotnet build` output no longer emits `ASP0000` warnings for these files. + +--- + +## Acceptance Criteria + +- Zero calls to `services.BuildServiceProvider()` in `AddBotApplicationExtensions.cs`, `BotConfig.cs`, and `JwtExtensions.cs`. +- No `ASP0000` analyzer warnings in the build output. +- Application startup completes successfully and bot processes requests as before. +- If `IConfiguration` is unavailable (e.g., misconfigured host), a clear `InvalidOperationException` is thrown at startup rather than a silent fallback. diff --git a/core/docs/stabilization/audit-006-configmanager-disposable.md b/core/docs/stabilization/audit-006-configmanager-disposable.md new file mode 100644 index 00000000..767a63ad --- /dev/null +++ b/core/docs/stabilization/audit-006-configmanager-disposable.md @@ -0,0 +1,120 @@ +# Audit Issue 006: `ConfigurationManager` (IDisposable) Never Disposed + +**Severity:** High +**File:** `core/src/Microsoft.Teams.Bot.Core/Hosting/JwtExtensions.cs` +**Lines:** 192–224 +**Category:** Memory management / resource leak + +--- + +## Problem + +Inside `AddTeamsJwtBearer`, a `ConcurrentDictionary` is used to cache one `ConfigurationManager` instance per OIDC authority: + +```csharp +// JwtExtensions.cs, lines 192-224 +ConcurrentDictionary> configManagerCache = new(StringComparer.OrdinalIgnoreCase); + +builder.AddJwtBearer(schemeName, jwtOptions => +{ + // ... + IssuerSigningKeyResolver = (_, securityToken, _, _) => + { + // ... + ConfigurationManager manager = configManagerCache.GetOrAdd(authority, a => + new ConfigurationManager( + a, + new OpenIdConnectConfigurationRetriever(), + new HttpDocumentRetriever { RequireHttps = jwtOptions.RequireHttpsMetadata })); + // ... + } +}); +``` + +`ConfigurationManager` implements `IDisposable`. The instances are stored in `configManagerCache`, but: + +1. `configManagerCache` is a **local variable** in `AddTeamsJwtBearer`. It is captured by the lambda closure and will remain alive for the lifetime of the application — but there is no registered disposal path. When the application shuts down (or if the authentication scheme is reconfigured), the `ConfigurationManager` instances are never disposed. + +2. `ConfigurationManager` holds an internal `HttpDocumentRetriever` which wraps an `HttpClient`, and has a background refresh timer. Neither will be cleaned up without `Dispose()`. + +3. `HttpDocumentRetriever` is also instantiated directly (`new HttpDocumentRetriever {...}`) and passed to `ConfigurationManager`. If the `ConfigurationManager` constructor throws after accepting the retriever but before taking ownership, the retriever leaks. + +--- + +## Root Cause + +The cache is a local closure variable with no hook into the application's lifetime events. The pattern is common but incorrect — `IDisposable` types cached for application lifetime need to be tied to a disposable container or to `IHostApplicationLifetime.ApplicationStopped`. + +--- + +## Suggested Fix Plan + +### Step 1 — Move the cache to a registered singleton service + +Create a small class that owns the cache and implements `IDisposable`: + +```csharp +internal sealed class OidcConfigCache : IDisposable +{ + private readonly ConcurrentDictionary> + _cache = new(StringComparer.OrdinalIgnoreCase); + + public ConfigurationManager GetOrAdd( + string authority, + bool requireHttps) + => _cache.GetOrAdd(authority, a => + new ConfigurationManager( + a, + new OpenIdConnectConfigurationRetriever(), + new HttpDocumentRetriever { RequireHttps = requireHttps })); + + public void Dispose() + { + foreach (ConfigurationManager mgr in _cache.Values) + mgr.Dispose(); + _cache.Clear(); + } +} +``` + +Register it as a singleton: + +```csharp +services.AddSingleton(); +``` + +Resolve it inside the JWT options configuration via `IServiceProvider` (available through `jwtOptions` or by resolving from the service provider passed to `AddJwtBearer`'s `configure` callback). + +### Step 2 — Alternatively, use `IOptions` post-configuration + +Use `services.AddSingleton, OidcKeyResolverPostConfigure>()` to inject `OidcConfigCache` (registered singleton) and configure the `IssuerSigningKeyResolver` from there. The singleton is disposed automatically when the DI container is disposed. + +### Step 3 — Dispose `HttpDocumentRetriever` explicitly on construction failure + +Wrap the `ConfigurationManager` construction to ensure the retriever is not leaked if construction fails: + +```csharp +HttpDocumentRetriever retriever = new() { RequireHttps = jwtOptions.RequireHttpsMetadata }; +try +{ + return new ConfigurationManager( + authority, + new OpenIdConnectConfigurationRetriever(), + retriever); +} +catch +{ + // ConfigurationManager did not take ownership. + // HttpDocumentRetriever is not IDisposable in current Microsoft.IdentityModel versions, + // but document this assumption with a comment for future-proofing. + throw; +} +``` + +--- + +## Acceptance Criteria + +- `ConfigurationManager` instances are stored in a DI-registered singleton that is disposed when the application stops. +- No `IDisposable` instances are abandoned in closure-captured local variables. +- Running the application in a test host and stopping it cleanly produces no finalizer warnings or resource-leak diagnostics. diff --git a/core/docs/stabilization/audit-007-object-tostring-properties.md b/core/docs/stabilization/audit-007-object-tostring-properties.md new file mode 100644 index 00000000..959de153 --- /dev/null +++ b/core/docs/stabilization/audit-007-object-tostring-properties.md @@ -0,0 +1,96 @@ +# Audit Issue 007: Unvalidated `object.ToString()` on Property Bag Values + +**Severity:** Medium +**File:** `core/src/Microsoft.Teams.Bot.Compat/CompatActivity.cs` +**Lines:** 75–109 (in `ToCompatChannelAccount`) and 238–265 (in `ToCompatTeamsChannelAccount`) +**Category:** Type safety / data correctness + +--- + +## Problem + +`ConversationAccount.Properties` is a `Dictionary`. When converting to compat types, values are extracted and converted with `.ToString()`: + +```csharp +// CompatActivity.cs, line 75-77 +if (account.Properties.TryGetValue("aadObjectId", out object? aadObjectId)) +{ + channelAccount.AadObjectId = aadObjectId?.ToString(); +} + +// Same pattern repeated for: userRole, userPrincipalName, givenName, surname, email, tenantId +``` + +The problem: `object.ToString()` has no defined behaviour for non-string types. If the JSON deserializer stored the value as a `JsonElement`, `JToken`, `JsonNode`, `int`, `bool`, or `string[]`, calling `.ToString()` produces: + +| Stored type | `.ToString()` result | Expected result | +|---|---|---| +| `JsonElement` (string) | `"hello"` (correct) | `"hello"` | +| `JsonElement` (number) | `"42"` (correct) | `"42"` | +| `JsonElement` (array) | `"System.Text.Json.JsonElement"` | `null` or throw | +| `string` | `"hello"` (correct) | `"hello"` | +| `object[]` | `"System.Object[]"` | `null` or throw | +| `null` | `null` (correct via `?.`) | `null` | + +In practice, the `Properties` dictionary is populated from JSON deserialization. Depending on which JSON library populated it (`System.Text.Json` vs `Newtonsoft.Json`), the stored type varies: + +- **`System.Text.Json`** stores `JsonElement` values. +- **`Newtonsoft.Json`** stores `JToken` values (which have a useful `.ToString()`, so this is safer there). + +The code uses both serializers (the compat layer uses Newtonsoft). But a future migration or mixed-serializer path could cause silent data corruption where `aadObjectId` becomes `"System.Text.Json.JsonElement"` in the compat object. + +--- + +## Suggested Fix Plan + +### Step 1 — Add a typed extraction helper + +Replace raw `.ToString()` calls with a helper that understands the known stored types: + +```csharp +private static string? ExtractStringProperty(Dictionary properties, string key) +{ + if (!properties.TryGetValue(key, out object? value)) + return null; + + return value switch + { + null => null, + string s => s, + System.Text.Json.JsonElement je when je.ValueKind == System.Text.Json.JsonValueKind.String + => je.GetString(), + System.Text.Json.JsonElement je + => je.ToString(), // Numbers, bools — ToString() is well-defined + Newtonsoft.Json.Linq.JValue jv => jv.Value?.ToString(), + _ => value.ToString() // Fallback with debug assertion + }; +} +``` + +Apply this helper to all `TryGetValue` calls in `ToCompatChannelAccount` and `ToCompatTeamsChannelAccount`. + +### Step 2 — Add a debug assertion for unexpected types + +In the fallback branch, add: + +```csharp +_ => +{ + System.Diagnostics.Debug.Fail($"Unexpected type '{value.GetType().Name}' for property '{key}'"); + return value.ToString(); +} +``` + +This surfaces unexpected types during development without impacting production. + +### Step 3 — Document the contract of `Properties` + +Add an XML doc comment to `ConversationAccount.Properties` (or wherever the dictionary is declared) stating the expected value types, so future code does not store non-string values without thought. + +--- + +## Acceptance Criteria + +- `aadObjectId`, `userRole`, `userPrincipalName`, `givenName`, `surname`, `email`, and `tenantId` are never set to strings like `"System.Text.Json.JsonElement"`. +- A unit test verifies `ToCompatChannelAccount` with a `JsonElement`-backed property dictionary produces the correct string values. +- All existing compat conversion tests pass. diff --git a/core/docs/stabilization/audit-008-entity-silent-null.md b/core/docs/stabilization/audit-008-entity-silent-null.md new file mode 100644 index 00000000..1448fd25 --- /dev/null +++ b/core/docs/stabilization/audit-008-entity-silent-null.md @@ -0,0 +1,107 @@ +# Audit Issue 008: Unknown Entity Types Silently Discarded During JSON Deserialization + +**Severity:** Medium +**File:** `core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/Entity.cs` +**Lines:** 66–74 (inside `EntityList.FromJsonArray`) +**Category:** Data integrity / extensibility + +--- + +## Problem + +When deserializing the `entities` array from an activity payload, unknown `type` values silently produce `null` and are skipped: + +```csharp +// Entity.cs, lines 66-74 +Entity? entity = typeString switch +{ + "clientInfo" => item.Deserialize(options), + "mention" => item.Deserialize(options), + "message" or "https://schema.org/Message" => DeserializeMessageEntity(item, options), + "ProductInfo" => item.Deserialize(options), + "streaminfo" => item.Deserialize(options), + _ => null // <-- silently drops any unrecognised entity type +}; +if (entity != null) + entities.Add(entity); +``` + +There is already a TODO comment above this code acknowledging the problem: + +```csharp +// TODO: Should be able to support unknown types (PA uses BotMessageMetadata). +``` + +The consequences: + +1. **Data loss** — Third-party or future Teams entity types (e.g., `BotMessageMetadata`, `citation`, custom adaptive card entities) are silently dropped. Bot handlers never see them. +2. **Debugging difficulty** — There is no log message, no metric, and no way to know that entities were dropped in production. +3. **Incomplete bot behaviour** — If a Teams extension sends a new entity type that triggers bot logic, that logic never runs. + +--- + +## Root Cause + +The switch statement enumerates known types exhaustively. Adding a new entity type requires modifying this file. There is no extensibility mechanism (registry, plugin, etc.) — this is noted in the existing TODO comment. + +--- + +## Suggested Fix Plan + +### Step 1 — Deserialize unknown types as the base `Entity` class (immediate fix) + +Instead of returning `null` for unknown types, deserialize to the base `Entity` class which preserves all JSON fields via `[JsonExtensionData]`: + +```csharp +_ => item.Deserialize(options) // Preserves all properties via ExtensionData +``` + +`Entity` has `[JsonExtensionData] public ExtendedPropertiesDictionary Properties { get; set; }` which captures all unknown JSON fields. Bot code can inspect `entity.Type` and `entity.Properties` to handle unrecognised types. + +This is a non-breaking change: existing handlers check `entity is MentionEntity` etc. and are unaffected. Unknown types are now accessible instead of dropped. + +### Step 2 — Add a trace log for unknown entity types + +```csharp +_ => +{ + logger?.LogTrace("Unknown entity type '{EntityType}' deserialized as base Entity.", typeString); + return item.Deserialize(options); +} +``` + +(Pass a logger into `FromJsonArray` or use a static `ILogger` on the `EntityList` class.) + +### Step 3 — Implement a registration pattern (medium-term, addresses the TODO) + +Introduce a static registry: + +```csharp +public static class EntityTypeRegistry +{ + private static readonly Dictionary _types = new(StringComparer.OrdinalIgnoreCase); + + public static void Register(string typeName) where T : Entity + => _types[typeName] = typeof(T); + + internal static Type Resolve(string typeName) + => _types.TryGetValue(typeName, out Type? t) ? t : typeof(Entity); +} +``` + +Replace the switch statement with a registry lookup: + +```csharp +Type entityType = EntityTypeRegistry.Resolve(typeString); +Entity? entity = (Entity?)item.Deserialize(entityType, options); +``` + +Register built-in types at startup and allow third parties to register their own. + +--- + +## Acceptance Criteria + +- No `entities` entry is silently dropped; unknown types are preserved as base `Entity` instances. +- A unit test verifies that an activity payload with an unknown entity type `"BotMessageMetadata"` produces one `Entity` in the list with `Type == "BotMessageMetadata"` and the original JSON fields accessible via `Properties`. +- Existing entity type tests (mention, citation, etc.) continue to pass. diff --git a/core/docs/stabilization/audit-009-fragile-exception-filter.md b/core/docs/stabilization/audit-009-fragile-exception-filter.md new file mode 100644 index 00000000..6d7bc8c0 --- /dev/null +++ b/core/docs/stabilization/audit-009-fragile-exception-filter.md @@ -0,0 +1,111 @@ +# Audit Issue 009: Fragile Exception Filter by Message String in Streaming Writer + +**Severity:** Medium +**File:** `core/src/Microsoft.Teams.Bot.Apps/TeamsStreamingWriter.cs` +**Lines:** 104–107 +**Category:** Error handling correctness + +--- + +## Problem + +`AppendResponseAsync` catches a cancellation-by-user event by matching on the exception message text: + +```csharp +// TeamsStreamingWriter.cs, lines 104-107 +catch (HttpRequestException ex) when (ex.Message.Contains("Content stream was cancelled by user", StringComparison.OrdinalIgnoreCase)) +{ + _cancelled = true; +} +``` + +This is fragile for several reasons: + +1. **Message strings are not part of any API contract.** The Teams API or the underlying HTTP stack can change the wording of this error at any time (e.g., capitalisation change, translation, rewording). When that happens, the `when` filter stops matching, the exception propagates uncaught, and streaming fails with an unhandled exception rather than setting `_cancelled = true`. + +2. **Locale sensitivity.** If the exception message is ever localised, `StringComparison.OrdinalIgnoreCase` may not be sufficient. + +3. **Overly narrow catch.** Other transient cancellation-related HTTP errors that should also set `_cancelled = true` may have different messages and will not be caught. + +4. **No fallback handling.** If the message changes, callers of `AppendResponseAsync` receive an `HttpRequestException` that bubbles up through their code with no indication that it was a user-initiated cancellation. + +--- + +## Root Cause + +The Teams Streaming API does not appear to use a typed exception or a specific `HttpStatusCode` to signal user-side stream cancellation. The message-string filter is a workaround for an ambiguous API contract. + +--- + +## Suggested Fix Plan + +### Step 1 — Audit the actual HTTP response for a distinguishing status code or header + +Before changing anything in code, investigate what the Teams API actually returns when a user cancels a stream: + +- Check the HTTP status code (e.g., `499 Client Closed Request`, `400`, `408`). +- Check the response body for a structured error code. +- Review the Teams Streaming API documentation for the expected error shape. + +If a specific status code or structured error code exists, prefer it over message matching. + +### Step 2 — Use `HttpRequestException.StatusCode` if available + +`HttpRequestException` in .NET 5+ exposes `StatusCode`: + +```csharp +catch (HttpRequestException ex) when (IsCancellationByUser(ex)) +{ + _cancelled = true; +} + +private static bool IsCancellationByUser(HttpRequestException ex) +{ + // Prefer status-code check if the Teams API uses a specific code + if (ex.StatusCode == System.Net.HttpStatusCode.BadRequest) + { + // Narrow further if needed + return ex.Message.Contains("cancelled by user", StringComparison.OrdinalIgnoreCase); + } + return false; +} +``` + +Centralising the check in a named method makes it testable and easier to update. + +### Step 3 — Add a fallback log for unmatched `HttpRequestException` + +Ensure that `HttpRequestException` instances that do _not_ match the filter are at least logged: + +```csharp +catch (HttpRequestException ex) +{ + if (IsCancellationByUser(ex)) + { + _cancelled = true; + return; + } + // Re-throw non-cancellation errors — but log them first so they're diagnosable. + // (Use the logger passed to the writer if one is added.) + throw; +} +``` + +### Step 4 — Add a constant for the matched string + +At a minimum, if the string match must remain, extract the magic string to a named constant and add a comment: + +```csharp +// This string is matched against the Teams API error message for user-initiated stream cancellation. +// If streaming breaks unexpectedly, verify this string still matches the Teams API response. +private const string UserCancelledStreamMessage = "Content stream was cancelled by user"; +``` + +--- + +## Acceptance Criteria + +- The cancellation detection does not rely solely on an uncontracted error message string. +- If the detection logic changes or the message changes, the failure mode is a thrown `HttpRequestException` (diagnosable) rather than a silent logic error. +- A unit test mocks an `HttpRequestException` with the cancellation message and verifies `_cancelled` is set to `true`. +- A unit test mocks an `HttpRequestException` with an unrelated message and verifies it is re-thrown. diff --git a/core/docs/stabilization/audit-010-string-concat-streaming.md b/core/docs/stabilization/audit-010-string-concat-streaming.md new file mode 100644 index 00000000..8962c8cf --- /dev/null +++ b/core/docs/stabilization/audit-010-string-concat-streaming.md @@ -0,0 +1,109 @@ +# Audit Issue 010: O(n²) String Concatenation in Streaming Accumulator + +**Severity:** Low (Performance) +**File:** `core/src/Microsoft.Teams.Bot.Apps/TeamsStreamingWriter.cs` +**Line:** 92 +**Category:** Memory / performance + +--- + +## Problem + +`TeamsStreamingWriter` accumulates streaming chunks by string concatenation: + +```csharp +// TeamsStreamingWriter.cs, line 47 +private string _accumulated = string.Empty; + +// TeamsStreamingWriter.cs, line 92 +_accumulated += chunk; +``` + +In C#, `string` is immutable. Every `+=` operation: +1. Allocates a new `string` on the heap of length `|_accumulated| + |chunk|`. +2. Copies the entire content of both strings into the new allocation. +3. Leaves the old `_accumulated` string unreachable for GC. + +For a streaming response with `n` chunks of average size `k`, the total bytes copied is `k + 2k + 3k + ... + nk = O(n²k)`. For a 500-chunk stream with 20-character chunks (a common LLM streaming scenario), this is approximately 2.5 million bytes copied across 500 allocations — all to produce a ~10 KB final string. + +In addition, each allocation over 85 KB lands in the Large Object Heap (LOH), which is not compacted by default, increasing memory fragmentation under sustained use. + +This is a **performance and memory pressure issue**, not a correctness issue. However, under high concurrency (many simultaneous streaming conversations), it can contribute to GC pressure and latency spikes. + +--- + +## Root Cause + +`string +=` was used for simplicity. The standard fix is `System.Text.StringBuilder`. + +--- + +## Suggested Fix Plan + +### Step 1 — Replace `_accumulated` with `StringBuilder` + +```csharp +// Replace: +private string _accumulated = string.Empty; + +// With: +private readonly System.Text.StringBuilder _accumulatedBuilder = new(); +``` + +Update `AppendResponseAsync`: + +```csharp +// Replace: +_accumulated += chunk; + +// With: +_accumulatedBuilder.Append(chunk); +``` + +Update all reads of `_accumulated` to use `_accumulatedBuilder.ToString()`. This is called twice: +- In `AppendResponseAsync` when sending an intermediate update: `BuildActivity(_accumulated, StreamType.Streaming)`. +- In `FinalizeResponseAsync` when sending the final message: `BuildActivity(_accumulated, StreamType.Final, ...)`. + +```csharp +// Replace _accumulated reads: +BuildActivity(_accumulatedBuilder.ToString(), StreamType.Streaming) +BuildActivity(_accumulatedBuilder.ToString(), StreamType.Final, ...) + +// And the empty check: +if (_accumulatedBuilder.Length == 0 && (attachments == null || attachments.Count == 0)) +``` + +### Step 2 — Consider `string.Empty` for zero-content paths + +The `BuildActivity` method always materialises the full string. After the fix, `_accumulatedBuilder.ToString()` is O(n) — called at most once per `SendActivityAsync`. This is correct. + +### Step 3 — (Optional) Pre-size the builder + +If typical chunk sizes and counts are known, pre-size the builder to avoid internal buffer resizing: + +```csharp +// Rough capacity: 200 chunks × 50 chars average = 10,000 +private readonly System.Text.StringBuilder _accumulatedBuilder = new(capacity: 4096); +``` + +This is a micro-optimisation and only worthwhile if profiling shows builder resizing as a bottleneck. + +--- + +## Before / After Allocation Comparison + +| Metric | Before (`+=`) | After (`StringBuilder`) | +|--------|---------------|------------------------| +| Allocations per chunk | 1 new string (O(n) size) | 0 heap allocations per append | +| Total bytes copied (100 chunks × 100 chars) | ~500,000 | ~10,000 | +| LOH pressure | Possible for large responses | None | +| `ToString()` call on send | None (string already materialised) | 1 per send (O(n) copy, unavoidable) | + +--- + +## Acceptance Criteria + +- `_accumulated` field is replaced with `StringBuilder`. +- All three read sites (`AppendResponseAsync` intermediate send, `FinalizeResponseAsync`, and the empty-check in `FinalizeResponseAsync`) use `.ToString()` or `.Length` on the builder. +- Existing streaming integration tests pass. +- (Optional) A benchmark confirms allocation count is O(n) rather than O(n²) for 100-chunk streams. diff --git a/core/docs/stabilization/audit-011-serializer-map-missing-types.md b/core/docs/stabilization/audit-011-serializer-map-missing-types.md new file mode 100644 index 00000000..dd1cfdf9 --- /dev/null +++ b/core/docs/stabilization/audit-011-serializer-map-missing-types.md @@ -0,0 +1,94 @@ +# Audit Issue 011: `ActivitySerializerMap` Missing `TeamsActivity` Base Type Registration + +**Severity:** Medium +**File:** `core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivityType.cs` +**Lines:** 34–37 (`TeamsActivity.ToJson`), 99–103 (`ActivitySerializerMap`) +**Category:** Type dispatch / serialization correctness + +--- + +## Note + +> This issue is related to but distinct from **Audit Issue 003** (asymmetric maps). Issue 003 focuses on the missing entries for the 6 activity types that _can be deserialized_ but _cannot be serialized_ with their correct type info. This issue focuses on the **fallback path** itself and what happens when `ToJson()` is called on a `TeamsActivity` that is not in the map. + +--- + +## Problem + +`TeamsActivity.ToJson()` has the following fallback: + +```csharp +// TeamsActivity.cs, lines 34-37 +public override string ToJson() + => TeamsActivityType.ActivitySerializerMap.TryGetValue(GetType(), out Func? serializer) + ? serializer(this) + : ToJson(TeamsActivityJsonContext.Default.TeamsActivity); // Fallback +``` + +The fallback serializes using `TeamsActivityJsonContext.Default.TeamsActivity`, which is the base `TeamsActivity` JSON type info. This has two problems: + +### Problem A — Subtype fields are silently omitted + +When `GetType()` returns a subtype not in the map (e.g., `MessageReactionActivity`), the base-type serializer is used. Fields declared on the subtype are not serialized because the `JsonTypeInfo` does not know about them. + +This is the same root issue as Issue 003 but is documented here for completeness: **the fallback path produces subtly incorrect output without any error or warning**. + +### Problem B — No log or metric on fallback use + +The fallback is reached silently. In a production system, unexpected use of the fallback (e.g., a new activity subtype was added without updating the map) produces corrupted outbound messages with no diagnostic information. + +--- + +## Suggested Fix Plan + +### Step 1 — Add a debug assertion on the fallback path + +```csharp +public override string ToJson() +{ + if (TeamsActivityType.ActivitySerializerMap.TryGetValue(GetType(), out Func? serializer)) + return serializer(this); + + System.Diagnostics.Debug.Fail( + $"No serializer registered for activity type '{GetType().Name}'. " + + $"Add an entry to TeamsActivityType.ActivitySerializerMap."); + + return ToJson(TeamsActivityJsonContext.Default.TeamsActivity); +} +``` + +This makes the fallback visible during development and testing. + +### Step 2 — Add a production log on the fallback path + +Ensure a logger is accessible from `TeamsActivity.ToJson()` (or accept that a static logger is used here): + +```csharp +// If a static logger is available (e.g., via ILogger): +_logger?.LogWarning( + "Falling back to base TeamsActivity serializer for type '{ActivityType}'. " + + "Register a serializer in ActivitySerializerMap.", + GetType().Name); +``` + +### Step 3 — The complete fix: resolve Issue 003 + +The correct resolution of both this issue and Issue 003 is to populate `ActivitySerializerMap` completely (see **Audit Issue 003**). Once all 8 activity types have registered serializers, the fallback path should only be reached for: +- Direct `new TeamsActivity()` instances (base type, no subtype — the fallback is correct here). +- Third-party subclasses. + +After Issue 003 is resolved, add a comment to the fallback explaining when it is legitimately expected: + +```csharp +// Fallback: used for base TeamsActivity instances and unknown subtypes. +// If a known subtype hits this path, add it to ActivitySerializerMap. +return ToJson(TeamsActivityJsonContext.Default.TeamsActivity); +``` + +--- + +## Acceptance Criteria + +- The fallback path in `TeamsActivity.ToJson()` emits a `Debug.Fail` assertion (caught in tests) and a log warning (visible in production) when reached for a type that should have a registered serializer. +- After Issue 003 is resolved, the only types that legitimately reach the fallback are `TeamsActivity` itself and unregistered third-party subtypes. +- A unit test asserts that calling `ToJson()` on a `MessageReactionActivity` instance does _not_ hit the fallback path (once Issue 003 is complete). diff --git a/core/docs/stabilization/audit-012-compat-adapter-race-on-activity.md b/core/docs/stabilization/audit-012-compat-adapter-race-on-activity.md new file mode 100644 index 00000000..723dd99f --- /dev/null +++ b/core/docs/stabilization/audit-012-compat-adapter-race-on-activity.md @@ -0,0 +1,68 @@ +# Audit Issue 012: Thread-Safety Race on `OnActivity` in `CompatAdapter` + +**Severity:** Critical +**File:** `core/src/Microsoft.Teams.Bot.Compat/CompatAdapter.cs` +**Lines:** 57–66 +**Category:** Thread safety / Concurrency + +--- + +## Problem + +`CompatAdapter.ProcessAsync` mutates `_teamsBotApplication.OnActivity` — a public delegate property on the shared `TeamsBotApplication` singleton — on every incoming HTTP request: + +```csharp +_teamsBotApplication.OnActivity = async (activity, ct) => +{ + coreActivity = activity; + TurnContext turnContext = new(this, activity.ToCompatActivity()); + // ... + await MiddlewareSet.ReceiveActivityWithStatusAsync(turnContext, bot.OnTurnAsync, ct).ConfigureAwait(false); +}; +``` + +`TeamsBotApplication` is registered as a singleton in DI. When two or more HTTP requests arrive concurrently, they race on assigning `OnActivity`. Request A may set its callback, then Request B overwrites it before the bot pipeline invokes it. Request A then executes Request B's callback (or vice versa), leading to: + +- Requests dispatched to the wrong `IBot` instance. +- Leaked `TurnContext` / `CompatConnectorClient` from another request. +- Captured `coreActivity` local variable may be overwritten by the wrong request. + +--- + +## Root Cause + +The compatibility adapter bridges the legacy `IBotFrameworkHttpAdapter.ProcessAsync(HttpRequest, HttpResponse, IBot)` pattern onto the new Core bot pipeline. The new pipeline uses a single `OnActivity` delegate rather than per-request callback registration. Assigning a lambda to a shared singleton property on each request is inherently racy. + +--- + +## Suggested Fix + +Replace the shared mutable delegate with per-request dispatch. Two options: + +### Option A — Use `AsyncLocal<>` to scope the callback per request + +Store the per-request callback in an `AsyncLocal>` and have the `OnActivity` delegate read from it: + +```csharp +private static readonly AsyncLocal?> _currentCallback = new(); + +// In constructor (once): +_teamsBotApplication.OnActivity = (activity, ct) => + _currentCallback.Value?.Invoke(activity, ct) ?? Task.CompletedTask; + +// In ProcessAsync (per request): +_currentCallback.Value = async (activity, ct) => { /* per-request logic */ }; +``` + +### Option B — Accept a per-request callback in the pipeline + +Refactor `BotApplication.ProcessAsync` to accept an optional `Func` parameter, avoiding the need to mutate shared state entirely. This is more invasive but cleaner. + +--- + +## Acceptance Criteria + +- Concurrent requests in `CompatAdapter.ProcessAsync` do not share or overwrite each other's handler. +- No shared mutable state is assigned per-request on the singleton. +- Existing compat integration tests pass. +- A concurrency stress test with overlapping requests does not produce cross-request leakage. diff --git a/core/docs/stabilization/audit-013-shallow-copy-core-activity.md b/core/docs/stabilization/audit-013-shallow-copy-core-activity.md new file mode 100644 index 00000000..80ae99b0 --- /dev/null +++ b/core/docs/stabilization/audit-013-shallow-copy-core-activity.md @@ -0,0 +1,87 @@ +# Audit Issue 013: Shallow Reference Copy in `CoreActivity` Copy Constructor + +**Severity:** Medium +**File:** `core/src/Microsoft.Teams.Bot.Core/Schema/CoreActivity.cs` +**Lines:** 140–156 +**Category:** Memory management / Immutability + +--- + +## Problem + +The `protected CoreActivity(CoreActivity activity)` copy constructor copies all properties by reference: + +```csharp +protected CoreActivity(CoreActivity activity) +{ + ArgumentNullException.ThrowIfNull(activity); + Id = activity.Id; + ServiceUrl = activity.ServiceUrl; + ChannelId = activity.ChannelId; + Type = activity.Type; + ChannelData = activity.ChannelData; // shared reference + From = activity.From; // shared reference + Recipient = activity.Recipient; // shared reference + Conversation = activity.Conversation; // shared reference + Entities = activity.Entities; // shared JsonArray reference + Attachments = activity.Attachments; // shared JsonArray reference + Properties = activity.Properties; // shared dictionary reference + Value = activity.Value; // shared JsonNode reference +} +``` + +This means: + +1. Mutating `ChannelData`, `From`, `Recipient`, or `Conversation` on the copy also mutates the original. +2. Adding/removing items from `Entities` or `Attachments` affects both instances. +3. The `Properties` dictionary is fully shared — setting a property on one activity sets it on the other. + +The `TeamsActivity(CoreActivity)` constructor compounds this: it overwrites some properties with Teams-specific wrappers and calls `Rebase()`, which writes back into `base.Attachments` and `base.Entities` — potentially replacing the `JsonArray` that the original activity still references. + +--- + +## Root Cause + +The copy constructor was written as a shallow field-by-field copy for performance/simplicity. Rich object graphs (`ChannelData`, `ConversationAccount`, `JsonArray`, `Dictionary`) require explicit deep-copy or clone semantics to be safe. + +--- + +## Suggested Fix + +### Option A — Deep-copy mutable reference types (recommended) + +Clone the objects that are known to be mutated downstream: + +```csharp +protected CoreActivity(CoreActivity activity) +{ + ArgumentNullException.ThrowIfNull(activity); + Id = activity.Id; + ServiceUrl = activity.ServiceUrl; + ChannelId = activity.ChannelId; + Type = activity.Type; + ChannelData = activity.ChannelData is not null + ? JsonSerializer.Deserialize(JsonSerializer.Serialize(activity.ChannelData)) + : null; + From = activity.From; // immutable after construction — acceptable + Recipient = activity.Recipient; // immutable after construction — acceptable + Conversation = activity.Conversation; + Entities = activity.Entities?.DeepClone().AsArray(); + Attachments = activity.Attachments?.DeepClone().AsArray(); + Properties = new ExtendedPropertiesDictionary(activity.Properties); + Value = activity.Value?.DeepClone(); +} +``` + +### Option B — Document that copy is shallow and add `DeepClone()` helper + +If the shallow copy is intentional for performance, add an explicit `DeepClone()` method and document that the copy constructor creates a shallow copy. Callers that need isolation should use `DeepClone()`. + +--- + +## Acceptance Criteria + +- Mutating the copy's `Properties`, `Entities`, `Attachments`, or `ChannelData` does not affect the original. +- `TeamsActivity.Rebase()` does not stomp the original activity's `JsonArray` references. +- Serialization round-trip tests confirm no data loss. +- No measurable performance regression in activity processing throughput. diff --git a/core/docs/stabilization/audit-014-builder-returns-mutable-ref.md b/core/docs/stabilization/audit-014-builder-returns-mutable-ref.md new file mode 100644 index 00000000..bacaf24f --- /dev/null +++ b/core/docs/stabilization/audit-014-builder-returns-mutable-ref.md @@ -0,0 +1,86 @@ +# Audit Issue 014: `TeamsAttachmentBuilder.Build()` Returns Internal Mutable Reference + +**Severity:** Medium +**File:** `core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsAttachmentBuilder.cs` +**Line:** 110 +**Category:** Memory management / Builder pattern correctness + +--- + +## Problem + +`TeamsAttachmentBuilder.Build()` returns its internal `_attachment` field directly: + +```csharp +public TeamsAttachment Build() => _attachment; +``` + +This breaks the standard builder contract: the caller receives the *same* object the builder still holds internally. Continued calls to the builder (e.g., `WithContent(...)`, `WithName(...)`) after `Build()` silently mutate the supposedly "built" attachment through shared reference. + +Example of the bug: + +```csharp +var builder = new TeamsAttachmentBuilder() + .WithContentType("image/png") + .WithName("photo.png"); + +TeamsAttachment attachment1 = builder.Build(); + +builder.WithName("other.png"); // mutates attachment1 too +TeamsAttachment attachment2 = builder.Build(); + +// attachment1.Name == "other.png" — unexpected +// attachment1 and attachment2 are the same object reference +``` + +--- + +## Root Cause + +`Build()` returns the internal reference without creating a defensive copy. This is a common shortcut in builder implementations that becomes a problem when the builder is reused or further modified after calling `Build()`. + +--- + +## Suggested Fix + +Return a copy or make the `Build()` method terminal: + +### Option A — Return a new instance (recommended) + +```csharp +public TeamsAttachment Build() +{ + return new TeamsAttachment + { + ContentType = _attachment.ContentType, + Content = _attachment.Content, + ContentUrl = _attachment.ContentUrl, + Name = _attachment.Name, + ThumbnailUrl = _attachment.ThumbnailUrl, + Properties = new ExtendedPropertiesDictionary(_attachment.Properties) + }; +} +``` + +### Option B — Freeze-on-build pattern + +Add a `_built` flag and throw `InvalidOperationException` on any setter call after `Build()`: + +```csharp +private bool _built; +public TeamsAttachment Build() +{ + _built = true; + return _attachment; +} +// In each With* method: +if (_built) throw new InvalidOperationException("Builder has already been built."); +``` + +--- + +## Acceptance Criteria + +- `Build()` returns an object that is not mutated by subsequent builder calls. +- Existing tests that call `Build()` once continue to pass. +- No behavior change for the single-build-per-builder usage pattern. diff --git a/core/docs/stabilization/audit-015-shallow-clone-citation-claims.md b/core/docs/stabilization/audit-015-shallow-clone-citation-claims.md new file mode 100644 index 00000000..47fd0444 --- /dev/null +++ b/core/docs/stabilization/audit-015-shallow-clone-citation-claims.md @@ -0,0 +1,66 @@ +# Audit Issue 015: Shallow Clone of `CitationClaim` List in `CitationEntity` Copy Constructor + +**Severity:** Low +**File:** `core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/CitationEntity.cs` +**Lines:** 115–130 +**Category:** Memory management / Cloning correctness + +--- + +## Problem + +The `CitationEntity(OMessageEntity entity)` copy constructor creates a *new list* but copies the `CitationClaim` elements by reference: + +```csharp +Citation = citationEntity.Citation != null + ? new List(citationEntity.Citation) + : null; +``` + +`CitationClaim` is a mutable class with public setters (`Position`, `Appearance`). The new list shares the same `CitationClaim` objects as the source. Mutating a claim in the copy mutates the original, and vice versa. + +Similarly, `AdditionalType` is shallow-copied as `new List(entity.AdditionalType)`, but since `string` is immutable this is safe. The `CitationClaim` case is the concern. + +--- + +## Root Cause + +`new List(source)` creates a new list with the same element references. For reference types, this is a shallow clone — only the list container is independent, not the items. + +--- + +## Suggested Fix + +### Option A — Deep-copy each `CitationClaim` + +```csharp +Citation = citationEntity.Citation != null + ? citationEntity.Citation.Select(c => new CitationClaim + { + Position = c.Position, + Appearance = c.Appearance is not null + ? new CitationAppearance { /* copy fields */ } + : null + }).ToList() + : null; +``` + +### Option B — Make `CitationClaim` a record or immutable type + +If `CitationClaim` is never mutated after construction, convert it to a `record` or make its setters `init`-only. Then the shallow list copy is safe because the elements cannot be mutated. + +```csharp +public record CitationClaim +{ + public int? Position { get; init; } + public CitationAppearance? Appearance { get; init; } +} +``` + +--- + +## Acceptance Criteria + +- Mutating a `CitationClaim` in a copied `CitationEntity` does not affect the source entity. +- Serialization round-trip produces identical output before and after the fix. +- Existing entity tests continue to pass. diff --git a/core/docs/stabilization/audit-016-unsafe-iactivity-cast.md b/core/docs/stabilization/audit-016-unsafe-iactivity-cast.md new file mode 100644 index 00000000..d26fa97f --- /dev/null +++ b/core/docs/stabilization/audit-016-unsafe-iactivity-cast.md @@ -0,0 +1,68 @@ +# Audit Issue 016: Unsafe Direct Cast from `IActivity` to `Activity` in `CompatTeamsInfo` + +**Severity:** Medium +**File:** `core/src/Microsoft.Teams.Bot.Compat/CompatTeamsInfo.cs` +**Lines:** 452, 484, 516, 543, 582 +**Category:** Type safety + +--- + +## Problem + +Five methods in `CompatTeamsInfo` perform direct casts from `IActivity` to `Activity`: + +```csharp +// Lines 452, 484, 516, 543: +CoreActivity coreActivity = ((Activity)activity).FromCompatActivity(); + +// Line 582: +Activity = (Activity)activity, +``` + +These occur in: +- `SendMessageToListOfUsersAsync` (line 452) +- `SendMessageToListOfChannelsAsync` (line 484) +- `SendMessageToAllUsersInTeamAsync` (line 516) +- `SendMessageToAllUsersInTenantAsync` (line 543) +- `SendMessageToTeamsChannelAsync` (line 582) + +If any caller passes an `IActivity` implementation that is not `Activity` (e.g., a mock, a decorator, or a future alternative implementation), these casts throw `InvalidCastException` at runtime with no descriptive error message. + +The method signatures accept `IActivity`, which is the Bot Framework SDK abstraction. The direct cast violates the interface contract by requiring a concrete type. + +--- + +## Root Cause + +The `FromCompatActivity()` extension method is defined on `Activity` (concrete class), not on `IActivity`. The compat layer needs to serialize the activity to JSON for conversion, which requires access to the concrete type's serialization. + +--- + +## Suggested Fix + +### Option A — Safe cast with descriptive error (minimal change) + +```csharp +Activity concreteActivity = activity as Activity + ?? throw new ArgumentException( + $"Expected {nameof(Activity)} but received {activity.GetType().Name}. " + + "CompatTeamsInfo requires Bot Framework Activity instances.", + nameof(activity)); +CoreActivity coreActivity = concreteActivity.FromCompatActivity(); +``` + +### Option B — Accept `Activity` in the method signature + +Change the parameter type from `IActivity` to `Activity`. This makes the requirement explicit at compile time rather than failing at runtime. This is a breaking API change but honest about the actual contract. + +### Option C — Serialize via interface + +If the `IActivity` contract is important, serialize through the interface using `JsonConvert.SerializeObject(activity)` rather than casting, since `IActivity` properties are sufficient for JSON serialization. + +--- + +## Acceptance Criteria + +- Passing a non-`Activity` `IActivity` produces a clear, descriptive exception (not `InvalidCastException`). +- Alternatively, compile-time type safety prevents the mismatch entirely. +- Existing callers that pass `Activity` instances are unaffected. diff --git a/core/docs/stabilization/audit-017-unguarded-guid-parse.md b/core/docs/stabilization/audit-017-unguarded-guid-parse.md new file mode 100644 index 00000000..0e8c8399 --- /dev/null +++ b/core/docs/stabilization/audit-017-unguarded-guid-parse.md @@ -0,0 +1,62 @@ +# Audit Issue 017: Unguarded `Guid.Parse` on External Input + +**Severity:** Medium +**Files:** +- `core/src/Microsoft.Teams.Bot.Core/Hosting/BotAuthenticationHandler.cs` — line 99 +- `core/src/Microsoft.Teams.Bot.Compat/KeyedBotAuthenticationHandler.cs` — line ~120 +**Category:** Input validation / Type safety + +--- + +## Problem + +`BotAuthenticationHandler.GetAuthorizationHeaderAsync` calls `Guid.Parse` on `AgenticUserId`, which originates from deserialized channel data (external input): + +```csharp +options.WithAgentUserIdentity(agenticIdentity.AgenticAppId, Guid.Parse(agenticIdentity.AgenticUserId)); +``` + +`AgenticIdentity.AgenticUserId` is populated from `ExtendedPropertiesDictionary` via `AgenticIdentity.FromProperties()`, which calls `userIdObj?.ToString()`. The value comes from the incoming activity's channel data — ultimately from the Teams service, but still external to the bot process. + +If `AgenticUserId` is not a valid GUID (empty string, malformed, or unexpected format), `Guid.Parse` throws a `FormatException`. This exception propagates up through the HTTP message handler pipeline, resulting in a failed outgoing API call with an unhelpful exception rather than a clear validation error. + +The same pattern exists in `KeyedBotAuthenticationHandler`. + +--- + +## Root Cause + +`Guid.Parse` is a hard-parsing API that throws on invalid input. There is no validation between extracting the string value from `ExtendedPropertiesDictionary` and passing it to `Guid.Parse`. + +--- + +## Suggested Fix + +### Option A — Use `Guid.TryParse` with descriptive error (recommended) + +```csharp +if (!Guid.TryParse(agenticIdentity.AgenticUserId, out Guid agenticUserId)) +{ + throw new InvalidOperationException( + $"AgenticUserId '{agenticIdentity.AgenticUserId}' is not a valid GUID."); +} + +options.WithAgentUserIdentity(agenticIdentity.AgenticAppId, agenticUserId); +``` + +### Option B — Validate at `AgenticIdentity.FromProperties` boundary + +Add GUID format validation when constructing the `AgenticIdentity`, so that any `AgenticIdentity` instance with a non-null `AgenticUserId` is guaranteed to contain a valid GUID string: + +```csharp +AgenticUserId = userIdObj?.ToString() is string uid && Guid.TryParse(uid, out _) ? uid : null +``` + +--- + +## Acceptance Criteria + +- Malformed `AgenticUserId` values produce a clear, descriptive exception or are handled gracefully. +- No `FormatException` propagates from `Guid.Parse` in `BotAuthenticationHandler`. +- Valid GUID strings continue to work as before. +- Applied to both `BotAuthenticationHandler` and `KeyedBotAuthenticationHandler`. diff --git a/core/docs/stabilization/audit-018-token-parsing-crash-in-logging.md b/core/docs/stabilization/audit-018-token-parsing-crash-in-logging.md new file mode 100644 index 00000000..0023804d --- /dev/null +++ b/core/docs/stabilization/audit-018-token-parsing-crash-in-logging.md @@ -0,0 +1,89 @@ +# Audit Issue 018: Token Parsing in Logging Path Can Crash Requests + +**Severity:** Medium +**File:** `core/src/Microsoft.Teams.Bot.Core/Hosting/BotAuthenticationHandler.cs` +**Lines:** 108–113 +**Category:** Error handling / Logging safety + +--- + +## Problem + +`LogTokenClaims` constructs a `JwtSecurityToken` to extract claims for trace-level logging: + +```csharp +private void LogTokenClaims(string token) +{ + if (!_logger.IsEnabled(LogLevel.Trace)) + { + return; + } + + JwtSecurityToken jwtToken = new(token); + string claims = Environment.NewLine + string.Join( + Environment.NewLine, + jwtToken.Claims.Select(c => $" {c.Type}: {c.Value}")); + _logTokenClaims(_logger, claims, null); +} +``` + +Two issues: + +1. **Crash risk:** If `token` is malformed (e.g., truncated, corrupted by a middleware, or not actually a JWT), `new JwtSecurityToken(token)` throws `ArgumentException` or `SecurityTokenMalformedException`. This exception propagates out of `LogTokenClaims` and fails the entire `SendAsync` pipeline — meaning a **logging side-effect crashes the outgoing HTTP request**. + +2. **Deprecated API:** `JwtSecurityToken` (from `System.IdentityModel.Tokens.Jwt`) is deprecated in favor of `JsonWebToken` (from `Microsoft.IdentityModel.JsonWebTokens`). The deprecated class has known performance and correctness issues. + +--- + +## Root Cause + +The logging method does not have a try-catch guard around token parsing. Since the method is called for every outgoing authenticated request when trace logging is enabled, any token parsing failure becomes a request failure. + +--- + +## Suggested Fix + +### Option A — Wrap in try-catch (minimal, recommended) + +```csharp +private void LogTokenClaims(string token) +{ + if (!_logger.IsEnabled(LogLevel.Trace)) + { + return; + } + + try + { + JsonWebToken jwtToken = new(token); + string claims = Environment.NewLine + string.Join( + Environment.NewLine, + jwtToken.Claims.Select(c => $" {c.Type}: {c.Value}")); + _logTokenClaims(_logger, claims, null); + } + catch (Exception ex) + { + _logger.LogTrace("Failed to parse token for logging: {Error}", ex.Message); + } +} +``` + +### Option B — Use `JsonWebTokenHandler.ReadJsonWebToken` with validation + +```csharp +JsonWebTokenHandler handler = new(); +if (handler.CanReadToken(token)) +{ + JsonWebToken jwtToken = handler.ReadJsonWebToken(token); + // log claims +} +``` + +--- + +## Acceptance Criteria + +- A malformed token does not cause `LogTokenClaims` to throw. +- Trace logging still outputs claims for valid tokens. +- `JwtSecurityToken` is replaced with `JsonWebToken`. +- No request failures caused by logging side-effects. diff --git a/core/docs/stabilization/audit-019-unnecessary-suppress-finalize.md b/core/docs/stabilization/audit-019-unnecessary-suppress-finalize.md new file mode 100644 index 00000000..affcc5f3 --- /dev/null +++ b/core/docs/stabilization/audit-019-unnecessary-suppress-finalize.md @@ -0,0 +1,57 @@ +# Audit Issue 019: `GC.SuppressFinalize` Called Without a Finalizer + +**Severity:** Low +**File:** `core/src/Microsoft.Teams.Bot.Compat/CompatConnectorClient.cs` +**Lines:** 38–42 +**Category:** Memory management / IDisposable correctness + +--- + +## Problem + +`CompatConnectorClient` implements `IDisposable` (required by the `IConnectorClient` interface) with an empty `Dispose()` that calls `GC.SuppressFinalize(this)`: + +```csharp +public void Dispose() +{ + // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method + GC.SuppressFinalize(this); +} +``` + +However: + +1. The class has **no finalizer** (`~CompatConnectorClient()`), so `SuppressFinalize` has no effect — there is nothing to suppress. +2. The comment references a `Dispose(bool disposing)` method that **does not exist**. +3. The `Dispose()` method does not dispose the inner `CompatConversations` instance or any other resources. + +While this is functionally harmless (the class holds no unmanaged resources), the misleading pattern suggests copy-pasted boilerplate that was not adapted to the actual class. + +--- + +## Root Cause + +The `IDisposable` implementation is required by the `IConnectorClient` interface. A template dispose pattern was applied without removing inapplicable parts. + +--- + +## Suggested Fix + +Simplify to a no-op dispose since the class holds no disposable resources: + +```csharp +public void Dispose() +{ + // No resources to dispose. Required by IConnectorClient interface. +} +``` + +Or, if `sealed` (which it is), just remove the `GC.SuppressFinalize` call and the misleading comment. + +--- + +## Acceptance Criteria + +- No `GC.SuppressFinalize` call on a class without a finalizer. +- Comment does not reference nonexistent `Dispose(bool)` method. +- `IConnectorClient.Dispose()` contract is still satisfied. diff --git a/core/docs/stabilization/audit-020-non-threadsafe-middleware-list.md b/core/docs/stabilization/audit-020-non-threadsafe-middleware-list.md new file mode 100644 index 00000000..350c61f0 --- /dev/null +++ b/core/docs/stabilization/audit-020-non-threadsafe-middleware-list.md @@ -0,0 +1,83 @@ +# Audit Issue 020: Non-Thread-Safe `List` in `TurnMiddleware` + +**Severity:** Low +**File:** `core/src/Microsoft.Teams.Bot.Core/TurnMiddleware.cs` +**Line:** 21 +**Category:** Thread safety + +--- + +## Problem + +`TurnMiddleware` stores registered middleware in a plain `IList`: + +```csharp +private readonly IList _middlewares = []; +``` + +The `Use()` method adds to this list: + +```csharp +internal TurnMiddleware Use(ITurnMiddleware middleware) +{ + _middlewares.Add(middleware); + return this; +} +``` + +And `RunPipelineAsync` reads from it concurrently during request processing: + +```csharp +if (nextMiddlewareIndex == _middlewares.Count) + // ... +ITurnMiddleware nextMiddleware = _middlewares[nextMiddlewareIndex]; +``` + +`List` is not thread-safe for concurrent reads and writes. If `Use()` is ever called after the application starts handling requests (e.g., from a background service or a dynamic middleware registration pattern), the list could be in an inconsistent state during concurrent reads, leading to `IndexOutOfRangeException` or corrupted iteration. + +--- + +## Root Cause + +The middleware list uses a non-concurrent collection. The design assumes middleware registration only happens during startup (before any calls to `RunPipelineAsync`), but this is not enforced. + +--- + +## Suggested Fix + +### Option A — Freeze the list after startup (recommended) + +Convert `_middlewares` to an array after startup and use the array for pipeline execution: + +```csharp +private readonly List _middlewares = []; +private ITurnMiddleware[]? _frozen; + +internal TurnMiddleware Use(ITurnMiddleware middleware) +{ + if (_frozen is not null) + throw new InvalidOperationException("Cannot add middleware after the pipeline has started."); + _middlewares.Add(middleware); + return this; +} + +internal void Freeze() => _frozen = [.. _middlewares]; + +public Task RunPipelineAsync(...) +{ + ITurnMiddleware[] pipeline = _frozen ?? throw new InvalidOperationException("Pipeline not frozen."); + // use pipeline[nextMiddlewareIndex] instead of _middlewares[nextMiddlewareIndex] +} +``` + +### Option B — Use `ImmutableList` or `ConcurrentBag` + +Replace `IList` with `ImmutableList` using atomic swap semantics, or use a thread-safe collection. This adds complexity but allows dynamic middleware registration. + +--- + +## Acceptance Criteria + +- Adding middleware after pipeline execution throws or is handled safely. +- Concurrent `RunPipelineAsync` calls with a frozen pipeline do not race. +- Existing middleware registration at startup continues to work. diff --git a/core/docs/stabilization/audit-021-cross-framework-json-serialization.md b/core/docs/stabilization/audit-021-cross-framework-json-serialization.md new file mode 100644 index 00000000..6318dff2 --- /dev/null +++ b/core/docs/stabilization/audit-021-cross-framework-json-serialization.md @@ -0,0 +1,79 @@ +# Audit Issue 021: Cross-Framework JSON Serialization (Newtonsoft → System.Text.Json) + +**Severity:** Medium +**File:** `core/src/Microsoft.Teams.Bot.Compat/CompatTeamsInfo.cs` +**Lines:** 357–358 +**Category:** Type safety / Serialization correctness + +--- + +## Problem + +`SendMeetingNotificationAsync` converts a Bot Framework type to a Core type by serializing with Newtonsoft.Json and deserializing with System.Text.Json: + +```csharp +string json = Newtonsoft.Json.JsonConvert.SerializeObject(notification); +AppsTeams.TargetedMeetingNotification? coreNotification = + System.Text.Json.JsonSerializer.Deserialize(json, s_jsonOptions); +``` + +This cross-framework round-trip is fragile because: + +1. **Property naming conventions differ.** Newtonsoft defaults to PascalCase; System.Text.Json requires explicit `JsonPropertyName` attributes or a `PropertyNamingPolicy`. If the serialized JSON uses PascalCase but the target type expects camelCase (or vice versa), properties silently deserialize as `null`/default. + +2. **Type handling attributes are incompatible.** `[Newtonsoft.Json.JsonProperty]` attributes are ignored by System.Text.Json, and `[System.Text.Json.Serialization.JsonPropertyName]` attributes are ignored by Newtonsoft. If either type uses library-specific attributes, the JSON field names may not match. + +3. **Enum serialization may differ.** Newtonsoft serializes enums as integers by default; System.Text.Json may expect string values depending on configuration. + +4. **Null handling differs.** `NullValueHandling` (Newtonsoft) vs `DefaultIgnoreCondition` (STJ) can cause properties to be present/absent inconsistently. + +The same pattern appears in `CompatActivity.ToCompatActivity()` and `FromCompatActivity()`, though those go through `BotMessageSerializer` (Newtonsoft) to/from `CoreActivity.FromJsonString` (STJ). + +--- + +## Root Cause + +The compat layer bridges two SDK generations that use different JSON libraries. Direct JSON round-tripping between the two libraries works for simple cases but has no compile-time safety net. + +--- + +## Suggested Fix + +### Option A — Use a single serializer for the round-trip + +If possible, use the same JSON library for both serialization and deserialization. Since the compat layer already depends on Newtonsoft, use Newtonsoft for both: + +```csharp +string json = Newtonsoft.Json.JsonConvert.SerializeObject(notification); +AppsTeams.TargetedMeetingNotification? coreNotification = + Newtonsoft.Json.JsonConvert.DeserializeObject(json); +``` + +Or use `System.Text.Json` for both if the source type supports it. + +### Option B — Manual mapping + +Write an explicit mapping method that copies properties between the Bot Framework type and the Core type without serialization. This is more verbose but eliminates serialization ambiguity: + +```csharp +AppsTeams.TargetedMeetingNotification coreNotification = new() +{ + Value = new() + { + Recipients = notification.Value?.Recipients?.Select(r => /* map */).ToList(), + // ... + } +}; +``` + +### Option C — Add integration tests for serialization fidelity + +If the cross-framework approach is kept, add tests that verify every property survives the Newtonsoft→STJ round-trip for all types used in cross-serialization. + +--- + +## Acceptance Criteria + +- No silent data loss during cross-framework serialization. +- All properties of the notification object survive the conversion. +- Integration test covers the Newtonsoft → STJ path with a fully-populated object. diff --git a/core/docs/stabilization/audit-022-missing-serviceurl-validation.md b/core/docs/stabilization/audit-022-missing-serviceurl-validation.md new file mode 100644 index 00000000..ab938026 --- /dev/null +++ b/core/docs/stabilization/audit-022-missing-serviceurl-validation.md @@ -0,0 +1,57 @@ +# Audit Issue 022: Missing `ServiceUrl` Null Validation in `CompatConversations` + +**Severity:** Low +**File:** `core/src/Microsoft.Teams.Bot.Compat/CompatConversations.cs` +**Lines:** 123, 203 +**Category:** Input validation + +--- + +## Problem + +Two methods in `CompatConversations` construct a `Uri` from a nullable `ServiceUrl` property using the null-forgiving operator: + +```csharp +// Line 123 — GetActivityMembersWithHttpMessagesAsync +new Uri(ServiceUrl!), + +// Line 203 — GetConversationsWithHttpMessagesAsync +new Uri(ServiceUrl!), +``` + +`ServiceUrl` is a `string?` property that is set externally (e.g., by `CompatAdapter.ContinueConversationAsync`). If it happens to be `null`, the `!` operator suppresses the compiler warning but `new Uri(null!)` throws a `NullReferenceException` (or `ArgumentNullException` in some .NET versions) with no indication that `ServiceUrl` was the problem. + +Other methods in the same class (e.g., `SendToConversationWithHttpMessagesAsync`, `ReplyToActivityWithHttpMessagesAsync`) do not access `ServiceUrl` directly because they pass it through fields that are validated elsewhere. These two methods are the exception. + +--- + +## Root Cause + +The null-forgiving operator `!` was used instead of an explicit null check. The property is nullable because it's set after construction, creating a window where it can be accessed before initialization. + +--- + +## Suggested Fix + +Add explicit validation at the start of each method: + +```csharp +public async Task>> GetActivityMembersWithHttpMessagesAsync(...) +{ + ArgumentException.ThrowIfNullOrWhiteSpace(ServiceUrl); + // ... rest of method using new Uri(ServiceUrl) +} + +public async Task> GetConversationsWithHttpMessagesAsync(...) +{ + ArgumentException.ThrowIfNullOrWhiteSpace(ServiceUrl); + // ... rest of method using new Uri(ServiceUrl) +} +``` + +--- + +## Acceptance Criteria + +- Null or empty `ServiceUrl` produces a clear `ArgumentException` with the property name, not a `NullReferenceException`. +- Validation is consistent with the pattern used in peer methods in the same class. diff --git a/core/docs/stabilization/audit-023-entity-as-cast-deser-null.md b/core/docs/stabilization/audit-023-entity-as-cast-deser-null.md new file mode 100644 index 00000000..c7e4c72a --- /dev/null +++ b/core/docs/stabilization/audit-023-entity-as-cast-deser-null.md @@ -0,0 +1,100 @@ +# Audit Issue 023: Entity Property `as` Casts Silently Return `null` After Deserialization + +**Severity:** High +**Files:** +- `core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/CitationEntity.cs` — line 138 +- `core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/MentionEntity.cs` — line 84 +- `core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/OMessageEntity.cs` — line ~28 +- `core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/SensitiveUsageEntity.cs` — line ~48 +**Category:** Type safety / Deserialization correctness + +--- + +## Problem + +Several entity classes store complex typed values in `Entity.Properties` (an `ExtendedPropertiesDictionary`, i.e., `Dictionary`) and retrieve them using `as` casts: + +```csharp +// CitationEntity.cs +public IList? Citation +{ + get => base.Properties.TryGetValue("citation", out object? value) ? value as IList : null; + set => base.Properties["citation"] = value; +} + +// MentionEntity.cs +public ConversationAccount? Mentioned +{ + get => base.Properties.TryGetValue("mentioned", out object? value) ? value as ConversationAccount : null; + set => base.Properties["mentioned"] = value; +} +``` + +**When values are set programmatically** (via the setter), the `value` stored is the actual typed object, and the `as` cast succeeds. + +**When values come from JSON deserialization**, `[JsonExtensionData]` on the `Properties` dictionary causes System.Text.Json to store the values as `JsonElement` (not as the expected CLR types). The `as` cast from `JsonElement` to `IList` or `ConversationAccount` **always returns `null`** — silently discarding the data. + +This means any entity that is deserialized from JSON (the primary code path) will have `null` for these properties even when the JSON contains valid data. + +--- + +## Root Cause + +`[JsonExtensionData]` stores unrecognized JSON properties as `JsonElement` objects in the dictionary. The property getters assume the dictionary contains strongly-typed CLR objects, which is only true when the setter was called directly. The two code paths (programmatic construction vs. deserialization) produce different runtime types for the same dictionary entry. + +--- + +## Suggested Fix + +### Option A — Check for `JsonElement` and deserialize on access (recommended) + +```csharp +public ConversationAccount? Mentioned +{ + get + { + if (!base.Properties.TryGetValue("mentioned", out object? value)) + return null; + if (value is ConversationAccount account) + return account; + if (value is JsonElement je) + { + ConversationAccount? deserialized = je.Deserialize(); + if (deserialized is not null) + base.Properties["mentioned"] = deserialized; // cache the deserialized value + return deserialized; + } + return null; + } + set => base.Properties["mentioned"] = value; +} +``` + +Apply the same pattern to `Citation` (`IList`), `AdditionalType` (`IList`), and `Pattern` (`DefinedTerm`). + +### Option B — Use dedicated `[JsonPropertyName]` properties instead of extension data + +Move these fields out of `Properties` and into explicit JSON-mapped properties on each entity class, so System.Text.Json deserializes them into the correct type directly. This requires removing them from the extension data flow. + +### Option C — Custom `JsonConverter` for `Entity` + +Write a custom converter that deserializes known properties into their correct types when reading JSON, rather than relying on `[JsonExtensionData]`. + +--- + +## Affected Properties + +| Entity | Property | Expected Type | Actual Type After Deser | +|--------|----------|---------------|------------------------| +| `CitationEntity` | `Citation` | `IList` | `JsonElement` | +| `MentionEntity` | `Mentioned` | `ConversationAccount` | `JsonElement` | +| `OMessageEntity` | `AdditionalType` | `IList` | `JsonElement` | +| `SensitiveUsageEntity` | `Pattern` | `DefinedTerm` | `JsonElement` | + +--- + +## Acceptance Criteria + +- Entity properties return the correct typed value regardless of whether the entity was constructed programmatically or deserialized from JSON. +- Round-trip serialization tests verify that `Citation`, `Mentioned`, `AdditionalType`, and `Pattern` survive JSON → object → JSON without data loss. +- No silent `null` returns when the JSON contains valid data for these fields. diff --git a/core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/CitationEntity.cs b/core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/CitationEntity.cs index f4a29738..bc89a6dd 100644 --- a/core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/CitationEntity.cs +++ b/core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/CitationEntity.cs @@ -122,8 +122,15 @@ public CitationEntity(OMessageEntity entity) : base() : null; if (entity is CitationEntity citationEntity) { + // Deep-copy each CitationClaim so that mutations on the clone's list items + // do not affect the original (COPY-01 / A-015). Re-serialize via STJ because + // CitationClaim contains a nested CitationAppearanceDocument reference. Citation = citationEntity.Citation != null - ? new List(citationEntity.Citation) + ? citationEntity.Citation + .Select(c => System.Text.Json.JsonSerializer.Deserialize( + System.Text.Json.JsonSerializer.Serialize(c, TeamsActivityJsonContext.Default.CitationClaim), + TeamsActivityJsonContext.Default.CitationClaim)!) + .ToList() : null; } } diff --git a/core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/Entity.cs b/core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/Entity.cs index 9d4eaabd..65b85e05 100644 --- a/core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/Entity.cs +++ b/core/src/Microsoft.Teams.Bot.Apps/Schema/Entities/Entity.cs @@ -31,7 +31,15 @@ public class EntityList : List foreach (KeyValuePair property in entity.Properties) { - jsonObject[property.Key] = property.Value as JsonNode ?? JsonValue.Create(property.Value); + JsonNode? node = property.Value switch + { + JsonNode jn => jn, + // JsonElement (produced by STJ [JsonExtensionData]) must be re-parsed to JsonNode + JsonElement je => JsonNode.Parse(je.GetRawText()), + null => null, + _ => JsonValue.Create(property.Value) + }; + jsonObject[property.Key] = node; } jsonArray.Add(jsonObject); } @@ -59,10 +67,6 @@ public class EntityList : List && typeValue.GetValue() is string typeString) { - // TODO: Should be able to support unknown types (PA uses BotMessageMetadata). - // TODO: Investigate if there is any way for Parent to avoid - // Knowing the children. - // Maybe a registry pattern, or Converters? Entity? entity = typeString switch { "clientInfo" => item.Deserialize(options), @@ -70,7 +74,10 @@ public class EntityList : List "message" or "https://schema.org/Message" => DeserializeMessageEntity(item, options), "ProductInfo" => item.Deserialize(options), "streaminfo" => item.Deserialize(options), - _ => null + // Unknown types: deserialize as base Entity so all raw JSON fields are + // preserved in the Properties [JsonExtensionData] dictionary rather than + // being silently discarded (A-008). + _ => item.Deserialize(options) }; if (entity != null) entities.Add(entity); diff --git a/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivity.cs b/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivity.cs index 0f064665..8b308a25 100644 --- a/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivity.cs +++ b/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivity.cs @@ -107,7 +107,8 @@ internal TeamsActivity Rebase() [JsonPropertyName("from")] public new TeamsConversationAccount? From { - get => base.From as TeamsConversationAccount; + get => base.From as TeamsConversationAccount + ?? (base.From is not null ? TeamsConversationAccount.FromConversationAccount(base.From) : null); set => base.From = value; } @@ -117,7 +118,8 @@ internal TeamsActivity Rebase() [JsonPropertyName("recipient")] public new TeamsConversationAccount? Recipient { - get => base.Recipient as TeamsConversationAccount; + get => base.Recipient as TeamsConversationAccount + ?? (base.Recipient is not null ? TeamsConversationAccount.FromConversationAccount(base.Recipient) : null); set => base.Recipient = value; } @@ -127,7 +129,8 @@ internal TeamsActivity Rebase() [JsonPropertyName("conversation")] public new TeamsConversation? Conversation { - get => base.Conversation as TeamsConversation; + get => base.Conversation as TeamsConversation + ?? (base.Conversation is not null ? TeamsConversation.FromConversation(base.Conversation) : null); set => base.Conversation = value; } @@ -137,7 +140,8 @@ internal TeamsActivity Rebase() [JsonPropertyName("channelData")] public new TeamsChannelData? ChannelData { - get => base.ChannelData as TeamsChannelData; + get => base.ChannelData as TeamsChannelData + ?? (base.ChannelData is not null ? new TeamsChannelData(base.ChannelData) : null); set => base.ChannelData = value; } diff --git a/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivityJsonContext.cs b/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivityJsonContext.cs index cb1e3cd9..61aa75b6 100644 --- a/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivityJsonContext.cs +++ b/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivityJsonContext.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System.Text.Json.Serialization; +using Microsoft.Teams.Bot.Apps.Handlers; using Microsoft.Teams.Bot.Apps.Schema.Entities; using Microsoft.Teams.Bot.Core.Schema; @@ -19,6 +20,13 @@ namespace Microsoft.Teams.Bot.Apps.Schema; [JsonSerializable(typeof(TeamsActivity))] [JsonSerializable(typeof(MessageActivity))] [JsonSerializable(typeof(StreamingActivity))] +[JsonSerializable(typeof(MessageReactionActivity))] +[JsonSerializable(typeof(MessageUpdateActivity))] +[JsonSerializable(typeof(MessageDeleteActivity))] +[JsonSerializable(typeof(ConversationUpdateActivity))] +[JsonSerializable(typeof(InstallUpdateActivity))] +[JsonSerializable(typeof(InvokeActivity))] +[JsonSerializable(typeof(EventActivity))] [JsonSerializable(typeof(Entity))] [JsonSerializable(typeof(EntityList))] [JsonSerializable(typeof(MentionEntity))] diff --git a/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivityType.cs b/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivityType.cs index 1129dd56..237224f9 100644 --- a/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivityType.cs +++ b/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsActivityType.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using Microsoft.Teams.Bot.Apps.Handlers; +using Microsoft.Teams.Bot.Apps.Schema.Entities; using Microsoft.Teams.Bot.Core.Schema; namespace Microsoft.Teams.Bot.Apps.Schema; @@ -95,10 +96,19 @@ public static class TeamsActivityType /// /// Registry of serializers keyed by concrete activity type, mirroring . + /// Every type registered in must also appear here so that + /// a serialization round-trip preserves all subtype-specific fields. /// internal static readonly Dictionary> ActivitySerializerMap = new() { [typeof(MessageActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.MessageActivity), [typeof(StreamingActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.StreamingActivity), + [typeof(MessageReactionActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.MessageReactionActivity), + [typeof(MessageUpdateActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.MessageUpdateActivity), + [typeof(MessageDeleteActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.MessageDeleteActivity), + [typeof(ConversationUpdateActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.ConversationUpdateActivity), + [typeof(InstallUpdateActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.InstallUpdateActivity), + [typeof(InvokeActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.InvokeActivity), + [typeof(EventActivity)] = a => a.ToJson(TeamsActivityJsonContext.Default.EventActivity), }; } diff --git a/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsAttachmentBuilder.cs b/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsAttachmentBuilder.cs index 19cbec22..9ccc70db 100644 --- a/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsAttachmentBuilder.cs +++ b/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsAttachmentBuilder.cs @@ -106,8 +106,19 @@ public TeamsAttachmentBuilder WithAdaptiveCard(object adaptiveCard) return this; } + private bool _built; + /// - /// Builds the attachment. + /// Builds and returns the attachment. Throws if called + /// more than once; once built the builder is sealed to prevent post-Build() mutations + /// from corrupting an already-returned attachment (A-014). /// - public TeamsAttachment Build() => _attachment; + public TeamsAttachment Build() + { + if (_built) + throw new InvalidOperationException( + "Build() has already been called on this builder. Create a new builder instance for each attachment."); + _built = true; + return _attachment; + } } diff --git a/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsConversationAccount.cs b/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsConversationAccount.cs index 59a26fb6..a52e309f 100644 --- a/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsConversationAccount.cs +++ b/core/src/Microsoft.Teams.Bot.Apps/Schema/TeamsConversationAccount.cs @@ -34,11 +34,19 @@ public TeamsConversationAccount() { return null; } - TeamsConversationAccount result = new(); - result.Id = conversationAccount.Id; - result.Name = conversationAccount.Name; - result.IsTargeted = conversationAccount.IsTargeted; - result.Properties = conversationAccount.Properties; + // Copy Properties entries into a new dictionary so that mutations on the returned + // account's Properties do not propagate back to the source (COPY-01 / A-013). + ExtendedPropertiesDictionary copiedProperties = []; + foreach (KeyValuePair kv in conversationAccount.Properties) + copiedProperties[kv.Key] = kv.Value; + + TeamsConversationAccount result = new() + { + Id = conversationAccount.Id, + Name = conversationAccount.Name, + IsTargeted = conversationAccount.IsTargeted, + Properties = copiedProperties + }; return result; } diff --git a/core/src/Microsoft.Teams.Bot.Apps/TeamsStreamingWriter.cs b/core/src/Microsoft.Teams.Bot.Apps/TeamsStreamingWriter.cs index bef0eb1d..30e0380a 100644 --- a/core/src/Microsoft.Teams.Bot.Apps/TeamsStreamingWriter.cs +++ b/core/src/Microsoft.Teams.Bot.Apps/TeamsStreamingWriter.cs @@ -44,7 +44,7 @@ public sealed class TeamsStreamingWriter private int _sequence; private bool _finalized; private bool _cancelled; - private string _accumulated = string.Empty; + private readonly System.Text.StringBuilder _accumulated = new(); private DateTime _lastChunkSent = DateTime.MinValue; internal TeamsStreamingWriter(ConversationClient client, TeamsActivity reference) @@ -89,7 +89,7 @@ public async Task AppendResponseAsync(string chunk, CancellationToken cancellati if (_cancelled) return; - _accumulated += chunk; + _accumulated.Append(chunk); if (DateTime.UtcNow - _lastChunkSent < _minChunkInterval) return; @@ -97,11 +97,11 @@ public async Task AppendResponseAsync(string chunk, CancellationToken cancellati _sequence++; try { - SendActivityResponse? response = await _client.SendActivityAsync(BuildActivity(_accumulated, StreamType.Streaming), cancellationToken: cancellationToken).ConfigureAwait(false); + SendActivityResponse? response = await _client.SendActivityAsync(BuildActivity(_accumulated.ToString(), StreamType.Streaming), cancellationToken: cancellationToken).ConfigureAwait(false); _streamId ??= response?.Id; _lastChunkSent = DateTime.UtcNow; } - catch (HttpRequestException ex) when (ex.Message.Contains("Content stream was cancelled by user", StringComparison.OrdinalIgnoreCase)) + catch (HttpRequestException ex) when (IsStreamCancelled(ex)) { _cancelled = true; } @@ -123,14 +123,35 @@ public async Task FinalizeResponseAsync(IList? attachments = nu if (_cancelled) return; - if (string.IsNullOrEmpty(_accumulated) && (attachments == null || attachments.Count == 0)) + if (_accumulated.Length == 0 && (attachments == null || attachments.Count == 0)) throw new InvalidOperationException("Cannot finalize with no content. Call AppendResponseAsync at least once before FinalizeResponseAsync."); - await _client.SendActivityAsync(BuildActivity(_accumulated, StreamType.Final, attachments, entities, feedbackEnabled), cancellationToken: cancellationToken).ConfigureAwait(false); + await _client.SendActivityAsync(BuildActivity(_accumulated.ToString(), StreamType.Final, attachments, entities, feedbackEnabled), cancellationToken: cancellationToken).ConfigureAwait(false); _finalized = true; } + /// + /// Returns when the indicates that the + /// Teams streaming channel has cancelled the current stream. Checking the HTTP status code (499) + /// is more robust than matching against the exception message string, which can change across SDK + /// or .NET version updates (A-009). + /// + /// Fallback: if the status code is absent (e.g. network-level errors), the message text is still + /// inspected as a secondary signal so legitimate cancellations are not missed. + /// + private static bool IsStreamCancelled(HttpRequestException ex) + { + // 499 "Client Closed Request" is the Teams API's structured signal for stream cancellation. + if (ex.StatusCode == System.Net.HttpStatusCode.RequestTimeout || + (int?)ex.StatusCode == 499) + return true; + + // Fallback text match for environments where the status code is not propagated. + return ex.Message.Contains("Content stream was cancelled by user", StringComparison.OrdinalIgnoreCase) + || ex.Message.Contains("stream was closed", StringComparison.OrdinalIgnoreCase); + } + private TeamsActivity BuildActivity(string text, string streamType, IList? attachments = null, IList? entities = null, bool feedbackEnabled = false) { bool isFinal = streamType == StreamType.Final; diff --git a/core/src/Microsoft.Teams.Bot.Compat/CompatActivity.cs b/core/src/Microsoft.Teams.Bot.Compat/CompatActivity.cs index 01b83fe2..81512589 100644 --- a/core/src/Microsoft.Teams.Bot.Compat/CompatActivity.cs +++ b/core/src/Microsoft.Teams.Bot.Compat/CompatActivity.cs @@ -74,37 +74,37 @@ public static Microsoft.Bot.Schema.ChannelAccount ToCompatChannelAccount(this Mi if (account.Properties.TryGetValue("aadObjectId", out object? aadObjectId)) { - channelAccount.AadObjectId = aadObjectId?.ToString(); + channelAccount.AadObjectId = ExtractString(aadObjectId); } if (account.Properties.TryGetValue("userRole", out object? userRole)) { - channelAccount.Role = userRole?.ToString(); + channelAccount.Role = ExtractString(userRole); } if (account.Properties.TryGetValue("userPrincipalName", out object? userPrincipalName)) { - channelAccount.Properties.Add("userPrincipalName", userPrincipalName?.ToString() ?? string.Empty); + channelAccount.Properties.Add("userPrincipalName", ExtractString(userPrincipalName) ?? string.Empty); } if (account.Properties.TryGetValue("givenName", out object? givenName)) { - channelAccount.Properties.Add("givenName", givenName?.ToString() ?? string.Empty); + channelAccount.Properties.Add("givenName", ExtractString(givenName) ?? string.Empty); } if (account.Properties.TryGetValue("surname", out object? surname)) { - channelAccount.Properties.Add("surname", surname?.ToString() ?? string.Empty); + channelAccount.Properties.Add("surname", ExtractString(surname) ?? string.Empty); } if (account.Properties.TryGetValue("email", out object? email)) { - channelAccount.Properties.Add("email", email?.ToString() ?? string.Empty); + channelAccount.Properties.Add("email", ExtractString(email) ?? string.Empty); } if (account.Properties.TryGetValue("tenantId", out object? tenantId)) { - channelAccount.Properties.Add("tenantId", tenantId?.ToString() ?? string.Empty); + channelAccount.Properties.Add("tenantId", ExtractString(tenantId) ?? string.Empty); } return channelAccount; @@ -237,32 +237,32 @@ public static Microsoft.Bot.Schema.Teams.TeamsChannelAccount ToCompatTeamsChanne // Extract properties from Properties dictionary if (account.Properties.TryGetValue("aadObjectId", out object? aadObjectId)) { - teamsChannelAccount.AadObjectId = aadObjectId?.ToString(); + teamsChannelAccount.AadObjectId = ExtractString(aadObjectId); } if (account.Properties.TryGetValue("userPrincipalName", out object? userPrincipalName)) { - teamsChannelAccount.UserPrincipalName = userPrincipalName?.ToString(); + teamsChannelAccount.UserPrincipalName = ExtractString(userPrincipalName); } if (account.Properties.TryGetValue("givenName", out object? givenName)) { - teamsChannelAccount.GivenName = givenName?.ToString(); + teamsChannelAccount.GivenName = ExtractString(givenName); } if (account.Properties.TryGetValue("surname", out object? surname)) { - teamsChannelAccount.Surname = surname?.ToString(); + teamsChannelAccount.Surname = ExtractString(surname); } if (account.Properties.TryGetValue("email", out object? email)) { - teamsChannelAccount.Email = email?.ToString(); + teamsChannelAccount.Email = ExtractString(email); } if (account.Properties.TryGetValue("tenantId", out object? tenantId)) { - teamsChannelAccount.Properties.Add("tenantId", tenantId?.ToString() ?? string.Empty); + teamsChannelAccount.Properties.Add("tenantId", ExtractString(tenantId) ?? string.Empty); } return teamsChannelAccount; @@ -321,5 +321,17 @@ public static Microsoft.Teams.Bot.Core.ConversationParameters FromCompatConversa return channelData?.Team; } + /// + /// Extracts a string value from an object? that may hold either a plain string or a + /// (the runtime type produced by STJ + /// [JsonExtensionData] dictionaries). Calling .ToString() on a + /// returns the JSON source text (e.g. with surrounding + /// quotes for strings), not the unescaped value — this helper returns the correct string (A-007). + /// + internal static string? ExtractString(object? value) => + value is System.Text.Json.JsonElement je + ? (je.ValueKind == System.Text.Json.JsonValueKind.String ? je.GetString() : je.GetRawText()) + : value?.ToString(); + } diff --git a/core/src/Microsoft.Teams.Bot.Compat/CompatAdapter.cs b/core/src/Microsoft.Teams.Bot.Compat/CompatAdapter.cs index 4f586dcb..e25b6955 100644 --- a/core/src/Microsoft.Teams.Bot.Compat/CompatAdapter.cs +++ b/core/src/Microsoft.Teams.Bot.Compat/CompatAdapter.cs @@ -55,7 +55,11 @@ public async Task ProcessAsync(HttpRequest httpRequest, HttpResponse httpRespons ArgumentNullException.ThrowIfNull(bot); CoreActivity? coreActivity = null; - _teamsBotApplication.OnActivity = async (activity, ct) => + + // Use a local, per-request handler passed directly to ProcessAsync. + // This avoids mutating the shared OnActivity field, which would cause a race condition + // when multiple requests are processed concurrently. + async Task compatHandler(CoreActivity activity, CancellationToken ct) { coreActivity = activity; TurnContext turnContext = new(this, activity.ToCompatActivity()); @@ -64,11 +68,11 @@ public async Task ProcessAsync(HttpRequest httpRequest, HttpResponse httpRespons turnContext.TurnState.Add(connectionClient); turnContext.TurnState.Add(_teamsBotApplication.TeamsApiClient); await MiddlewareSet.ReceiveActivityWithStatusAsync(turnContext, bot.OnTurnAsync, ct).ConfigureAwait(false); - }; + } try { - await _teamsBotApplication.ProcessAsync(httpRequest.HttpContext, cancellationToken).ConfigureAwait(false); + await _teamsBotApplication.ProcessAsync(httpRequest.HttpContext, compatHandler, cancellationToken).ConfigureAwait(false); } catch (Exception ex) { diff --git a/core/src/Microsoft.Teams.Bot.Compat/CompatConnectorClient.cs b/core/src/Microsoft.Teams.Bot.Compat/CompatConnectorClient.cs index 2fe50811..7cd2466b 100644 --- a/core/src/Microsoft.Teams.Bot.Compat/CompatConnectorClient.cs +++ b/core/src/Microsoft.Teams.Bot.Compat/CompatConnectorClient.cs @@ -37,8 +37,7 @@ internal sealed class CompatConnectorClient(CompatConversations conversations) : public void Dispose() { - // Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method - GC.SuppressFinalize(this); + // Nothing to dispose - all resources are owned by the injected CompatConversations. } } } diff --git a/core/src/Microsoft.Teams.Bot.Compat/CompatConversations.cs b/core/src/Microsoft.Teams.Bot.Compat/CompatConversations.cs index 715b3658..af98fbc4 100644 --- a/core/src/Microsoft.Teams.Bot.Compat/CompatConversations.cs +++ b/core/src/Microsoft.Teams.Bot.Compat/CompatConversations.cs @@ -115,12 +115,13 @@ await _client.DeleteConversationMemberAsync( public async Task>> GetActivityMembersWithHttpMessagesAsync(string conversationId, string activityId, Dictionary>? customHeaders = null, CancellationToken cancellationToken = default) { + ArgumentException.ThrowIfNullOrWhiteSpace(ServiceUrl); Dictionary? convertedHeaders = ConvertHeaders(customHeaders); IList members = await _client.GetActivityMembersAsync( conversationId, activityId, - new Uri(ServiceUrl!), + new Uri(ServiceUrl), null, convertedHeaders, cancellationToken).ConfigureAwait(false); @@ -197,10 +198,11 @@ public async Task>> GetConversationM public async Task> GetConversationsWithHttpMessagesAsync(string? continuationToken = null, Dictionary>? customHeaders = null, CancellationToken cancellationToken = default) { + ArgumentException.ThrowIfNullOrWhiteSpace(ServiceUrl); Dictionary? convertedHeaders = ConvertHeaders(customHeaders); GetConversationsResponse conversations = await _client.GetConversationsAsync( - new Uri(ServiceUrl!), + new Uri(ServiceUrl), continuationToken, null, convertedHeaders, diff --git a/core/src/Microsoft.Teams.Bot.Compat/CompatTeamsInfo.cs b/core/src/Microsoft.Teams.Bot.Compat/CompatTeamsInfo.cs index 2fe06299..e4b93881 100644 --- a/core/src/Microsoft.Teams.Bot.Compat/CompatTeamsInfo.cs +++ b/core/src/Microsoft.Teams.Bot.Compat/CompatTeamsInfo.cs @@ -27,6 +27,20 @@ public static class CompatTeamsInfo PropertyNameCaseInsensitive = true }; + /// + /// Casts to the concrete Bot Framework type. + /// Throws with a descriptive message when the cast fails so callers + /// receive a clear error instead of an . + /// + private static Activity RequireActivity(IActivity? activity, string paramName) + { + ArgumentNullException.ThrowIfNull(activity, paramName); + return activity as Activity + ?? throw new ArgumentException( + $"The activity must be an instance of {nameof(Activity)}; received {activity.GetType().Name}.", + paramName); + } + private static ConversationClient GetConversationClient(ITurnContext turnContext) { IConnectorClient connectorClient = turnContext.TurnState.Get() @@ -353,8 +367,10 @@ private static AgenticIdentity GetIdentity(ITurnContext turnContext) Uri serviceUrl = new(GetServiceUrl(turnContext)); AgenticIdentity identity = GetIdentity(turnContext); - // Convert Bot Framework MeetingNotificationBase to Core MeetingNotificationBase using JSON round-trip - string json = Newtonsoft.Json.JsonConvert.SerializeObject(notification); + // Convert Bot Framework MeetingNotificationBase to Core MeetingNotificationBase using a + // single-serializer (STJ) round-trip. The previous Newtonsoft→STJ two-leg round-trip could + // drop fields whose camelCase/PascalCase conventions differ between the two serializers. + string json = System.Text.Json.JsonSerializer.Serialize(notification, s_jsonOptions); AppsTeams.TargetedMeetingNotification? coreNotification = System.Text.Json.JsonSerializer.Deserialize(json, s_jsonOptions); @@ -442,14 +458,14 @@ public static async Task SendMessageToListOfUsersAsync( CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(turnContext); - activity = activity ?? throw new InvalidOperationException($"{nameof(activity)} is required."); + Activity botActivity = RequireActivity(activity, nameof(activity)); teamsMembers = teamsMembers ?? throw new InvalidOperationException($"{nameof(teamsMembers)} is required."); tenantId = tenantId ?? throw new InvalidOperationException($"{nameof(tenantId)} is required."); TeamsApiClient client = GetTeamsApiClient(turnContext); Uri serviceUrl = new(GetServiceUrl(turnContext)); AgenticIdentity identity = GetIdentity(turnContext); - CoreActivity coreActivity = ((Activity)activity).FromCompatActivity(); + CoreActivity coreActivity = botActivity.FromCompatActivity(); List coreTeamsMembers = teamsMembers.Select(m => m.FromCompatTeamMember()).ToList(); @@ -474,14 +490,14 @@ public static async Task SendMessageToListOfChannelsAsync( CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(turnContext); - activity = activity ?? throw new InvalidOperationException($"{nameof(activity)} is required."); + Activity botActivity = RequireActivity(activity, nameof(activity)); channelsMembers = channelsMembers ?? throw new InvalidOperationException($"{nameof(channelsMembers)} is required."); tenantId = tenantId ?? throw new InvalidOperationException($"{nameof(tenantId)} is required."); TeamsApiClient client = GetTeamsApiClient(turnContext); Uri serviceUrl = new(GetServiceUrl(turnContext)); AgenticIdentity identity = GetIdentity(turnContext); - CoreActivity coreActivity = ((Activity)activity).FromCompatActivity(); + CoreActivity coreActivity = botActivity.FromCompatActivity(); List coreChannelsMembers = channelsMembers.Select(m => m.FromCompatTeamMember()).ToList(); @@ -506,14 +522,14 @@ public static async Task SendMessageToAllUsersInTeamAsync( CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(turnContext); - activity = activity ?? throw new InvalidOperationException($"{nameof(activity)} is required."); + Activity botActivity = RequireActivity(activity, nameof(activity)); teamId = teamId ?? throw new InvalidOperationException($"{nameof(teamId)} is required."); tenantId = tenantId ?? throw new InvalidOperationException($"{nameof(tenantId)} is required."); TeamsApiClient client = GetTeamsApiClient(turnContext); Uri serviceUrl = new(GetServiceUrl(turnContext)); AgenticIdentity identity = GetIdentity(turnContext); - CoreActivity coreActivity = ((Activity)activity).FromCompatActivity(); + CoreActivity coreActivity = botActivity.FromCompatActivity(); return await client.SendMessageToAllUsersInTeamAsync( coreActivity, teamId, tenantId, serviceUrl, identity, null, cancellationToken).ConfigureAwait(false); @@ -534,13 +550,13 @@ public static async Task SendMessageToAllUsersInTenantAsync( CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(turnContext); - activity = activity ?? throw new InvalidOperationException($"{nameof(activity)} is required."); + Activity botActivity = RequireActivity(activity, nameof(activity)); tenantId = tenantId ?? throw new InvalidOperationException($"{nameof(tenantId)} is required."); TeamsApiClient client = GetTeamsApiClient(turnContext); Uri serviceUrl = new(GetServiceUrl(turnContext)); AgenticIdentity identity = GetIdentity(turnContext); - CoreActivity coreActivity = ((Activity)activity).FromCompatActivity(); + CoreActivity coreActivity = botActivity.FromCompatActivity(); return await client.SendMessageToAllUsersInTenantAsync( coreActivity, tenantId, serviceUrl, identity, null, cancellationToken).ConfigureAwait(false); @@ -579,7 +595,7 @@ public static async Task> SendMessageToTeam { IsGroup = true, ChannelData = new BotFrameworkTeams.TeamsChannelData { Channel = new BotFrameworkTeams.ChannelInfo { Id = teamsChannelId } }, - Activity = (Activity)activity, + Activity = RequireActivity(activity, nameof(activity)), }; await turnContext.Adapter.CreateConversationAsync( diff --git a/core/src/Microsoft.Teams.Bot.Core/BotApplication.cs b/core/src/Microsoft.Teams.Bot.Core/BotApplication.cs index 745a1803..4d4dafd5 100644 --- a/core/src/Microsoft.Teams.Bot.Core/BotApplication.cs +++ b/core/src/Microsoft.Teams.Bot.Core/BotApplication.cs @@ -80,7 +80,23 @@ public BotApplication(ConversationClient conversationClient, UserTokenClient use /// /// /// - public virtual async Task ProcessAsync(HttpContext httpContext, CancellationToken cancellationToken = default) + public virtual Task ProcessAsync(HttpContext httpContext, CancellationToken cancellationToken = default) + => ProcessAsync(httpContext, activityHandler: null, cancellationToken); + + /// + /// Processes an incoming HTTP request using an explicit per-request activity handler instead of the + /// shared field. Use this overload when the caller needs to supply a + /// request-scoped handler without mutating the shared field (prevents race conditions under concurrency). + /// + /// The HTTP context containing the incoming activity. + /// + /// A per-request handler to invoke, or to fall back to . + /// + /// A cancellation token that can be used to cancel the operation. + /// + /// + /// + public virtual async Task ProcessAsync(HttpContext httpContext, Func? activityHandler, CancellationToken cancellationToken = default) { ArgumentNullException.ThrowIfNull(httpContext); ArgumentNullException.ThrowIfNull(_conversationClient); @@ -97,6 +113,8 @@ public virtual async Task ProcessAsync(HttpContext httpContext, CancellationToke _logger.LogTraceGuarded("Received activity: {Activity}", activity.ToJson()); + Func? handler = activityHandler ?? this.OnActivity; + // TODO: Replace with structured scope data, ensure it works with OpenTelemetry and other logging providers using (_logger.BeginScope("ActivityType={ActivityType} ActivityId={ActivityId} ServiceUrl={ServiceUrl} MSCV={MSCV}", activity.Type, activity.Id, activity.ServiceUrl, httpContext.Request.GetCorrelationVector())) @@ -104,7 +122,7 @@ public virtual async Task ProcessAsync(HttpContext httpContext, CancellationToke try { CancellationToken token = Debugger.IsAttached ? CancellationToken.None : cancellationToken; - await MiddleWare.RunPipelineAsync(this, activity, this.OnActivity, 0, token).ConfigureAwait(false); + await MiddleWare.RunPipelineAsync(this, activity, handler, 0, token).ConfigureAwait(false); } catch (Exception ex) { @@ -129,6 +147,13 @@ public ITurnMiddleware UseMiddleware(ITurnMiddleware middleware) return MiddleWare; } + /// + /// Seals the middleware pipeline so that no further middleware can be added after startup. + /// Call this from IHostedService.StartAsync or any equivalent startup hook. + /// Any subsequent call to will throw (A-020). + /// + public void FreezeMiddleware() => MiddleWare.Freeze(); + /// /// Sends the specified activity to the conversation asynchronously. /// diff --git a/core/src/Microsoft.Teams.Bot.Core/Hosting/BotAuthenticationHandler.cs b/core/src/Microsoft.Teams.Bot.Core/Hosting/BotAuthenticationHandler.cs index 5005f75d..ba8d760e 100644 --- a/core/src/Microsoft.Teams.Bot.Core/Hosting/BotAuthenticationHandler.cs +++ b/core/src/Microsoft.Teams.Bot.Core/Hosting/BotAuthenticationHandler.cs @@ -1,7 +1,6 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. -using System.IdentityModel.Tokens.Jwt; using System.Net.Http.Headers; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -94,17 +93,22 @@ private async Task GetAuthorizationHeaderAsync(AgenticIdentity? agenticI !string.IsNullOrEmpty(agenticIdentity.AgenticAppId) && !string.IsNullOrEmpty(agenticIdentity.AgenticUserId)) { - _logAgenticToken(_logger, agenticIdentity.AgenticAppId, null); - - options.WithAgentUserIdentity(agenticIdentity.AgenticAppId, Guid.Parse(agenticIdentity.AgenticUserId)); - string token = await _authorizationHeaderProvider.CreateAuthorizationHeaderAsync([_scope], options, null, cancellationToken).ConfigureAwait(false); - return token; + if (!Guid.TryParse(agenticIdentity.AgenticUserId, out Guid agenticUserGuid)) + { + _logger.LogWarning("AgenticUserId '{AgenticUserId}' is not a valid GUID; falling back to app-only token.", agenticIdentity.AgenticUserId); + } + else + { + _logAgenticToken(_logger, agenticIdentity.AgenticAppId, null); + options.WithAgentUserIdentity(agenticIdentity.AgenticAppId, agenticUserGuid); + string token = await _authorizationHeaderProvider.CreateAuthorizationHeaderAsync([_scope], options, null, cancellationToken).ConfigureAwait(false); + return token; + } } _logAppOnlyToken(_logger, _scope, null); string appToken = await _authorizationHeaderProvider.CreateAuthorizationHeaderForAppAsync(_scope, options, cancellationToken).ConfigureAwait(false); - return appToken; } @@ -115,10 +119,18 @@ private void LogTokenClaims(string token) return; } - - JwtSecurityToken jwtToken = new(token); - string claims = Environment.NewLine + string.Join(Environment.NewLine, jwtToken.Claims.Select(c => $" {c.Type}: {c.Value}")); - _logTokenClaims(_logger, claims, null); - + try + { + // Use JsonWebToken (non-deprecated) instead of JwtSecurityToken. + // Wrap in try/catch because the token string may be malformed (e.g. an opaque + // MSI token) and we must never crash the send pipeline due to trace logging. + Microsoft.IdentityModel.JsonWebTokens.JsonWebToken jwt = new(token); + string claims = Environment.NewLine + string.Join(Environment.NewLine, jwt.Claims.Select(c => $" {c.Type}: {c.Value}")); + _logTokenClaims(_logger, claims, null); + } + catch (Exception ex) when (ex is ArgumentException or System.Text.Json.JsonException) + { + _logger.LogTrace("Could not parse token for claim logging: {Message}", ex.Message); + } } } diff --git a/core/src/Microsoft.Teams.Bot.Core/Hosting/BotConfig.cs b/core/src/Microsoft.Teams.Bot.Core/Hosting/BotConfig.cs index 4bfef98a..f009945a 100644 --- a/core/src/Microsoft.Teams.Bot.Core/Hosting/BotConfig.cs +++ b/core/src/Microsoft.Teams.Bot.Core/Hosting/BotConfig.cs @@ -142,10 +142,26 @@ public static BotConfig Resolve(IServiceCollection services, string sectionName { ArgumentNullException.ThrowIfNull(services); - // Extract IConfiguration from service collection + // Extract IConfiguration from the service collection without building a throwaway container. + // Calling BuildServiceProvider() here creates duplicate singleton instances and causes a + // DI container leak (DI-01). Fail fast with a clear message if IConfiguration is absent. ServiceDescriptor? configDescriptor = services.FirstOrDefault(d => d.ServiceType == typeof(IConfiguration)); - IConfiguration configuration = configDescriptor?.ImplementationInstance as IConfiguration - ?? services.BuildServiceProvider().GetRequiredService(); + + // HostApplicationBuilder registers IConfiguration via a factory delegate + // (services.AddSingleton(_ => config)), not as an ImplementationInstance. + // We must handle both registration styles. + IConfiguration? configuration = configDescriptor?.ImplementationInstance as IConfiguration; + if (configuration is null && configDescriptor?.ImplementationFactory is { } factory) + { + configuration = factory(null!) as IConfiguration; + } + + if (configuration is null) + { + throw new InvalidOperationException( + "IConfiguration is not registered in the service collection. " + + "Call services.AddSingleton(...) before calling this method."); + } // Get logger using the helper method from AddBotApplicationExtensions ILogger logger = AddBotApplicationExtensions.GetLoggerFromServices(services, typeof(BotConfig)); diff --git a/core/src/Microsoft.Teams.Bot.Core/Hosting/JwtExtensions.cs b/core/src/Microsoft.Teams.Bot.Core/Hosting/JwtExtensions.cs index 19fd4daf..0776ea3a 100644 --- a/core/src/Microsoft.Teams.Bot.Core/Hosting/JwtExtensions.cs +++ b/core/src/Microsoft.Teams.Bot.Core/Hosting/JwtExtensions.cs @@ -189,8 +189,11 @@ token is JsonWebToken jwt /// private static AuthenticationBuilder AddTeamsJwtBearer(this AuthenticationBuilder builder, string schemeName, string audience, string tenantId, ILogger? logger = null) { - // One ConfigurationManager per OIDC authority, shared safely across all requests. - ConcurrentDictionary> configManagerCache = new(StringComparer.OrdinalIgnoreCase); + // One JwksKeyCache per OIDC authority. The cache pre-warms in the background so the + // synchronous IssuerSigningKeyResolver can almost always serve from a volatile field + // without blocking any threads. Blocking only occurs on the very first request for a + // new authority before the background fetch has completed (cold start). + ConcurrentDictionary keyCacheByAuthority = new(StringComparer.OrdinalIgnoreCase); builder.AddJwtBearer(schemeName, jwtOptions => { @@ -213,14 +216,10 @@ private static AuthenticationBuilder AddTeamsJwtBearer(this AuthenticationBuilde ? BotOIDC : $"{EntraOIDC}{tid ?? "botframework.com"}/v2.0/.well-known/openid-configuration"; - ConfigurationManager manager = configManagerCache.GetOrAdd(authority, a => - new ConfigurationManager( - a, - new OpenIdConnectConfigurationRetriever(), - new HttpDocumentRetriever { RequireHttps = jwtOptions.RequireHttpsMetadata })); + JwksKeyCache cache = keyCacheByAuthority.GetOrAdd(authority, a => + new JwksKeyCache(a, jwtOptions.RequireHttpsMetadata)); - OpenIdConnectConfiguration config = manager.GetConfigurationAsync(CancellationToken.None).GetAwaiter().GetResult(); - return config.SigningKeys; + return cache.GetKeys(); } }; jwtOptions.TokenValidationParameters.EnableAadSigningKeyIssuerValidation(); @@ -319,9 +318,13 @@ private static AuthenticationBuilder AddBypassAuthentication(this Authentication private static BotConfig ResolveBotConfig(IServiceCollection services, string sectionName) { + // Avoid BuildServiceProvider() which creates duplicate singletons (DI-01). + // Fail fast with a clear message if IConfiguration is not yet registered. ServiceDescriptor? configDescriptor = services.FirstOrDefault(d => d.ServiceType == typeof(IConfiguration)); IConfiguration configuration = configDescriptor?.ImplementationInstance as IConfiguration - ?? services.BuildServiceProvider().GetRequiredService(); + ?? throw new InvalidOperationException( + "IConfiguration is not registered in the service collection. " + + "Ensure services.AddSingleton(...) is called before AddBotAuthentication/AddBotAuthorization."); return BotConfig.Resolve(configuration, sectionName); } @@ -331,4 +334,64 @@ private static ILogger GetLogger(HttpContext context, ILogger? fallback) => ?? fallback ?? NullLogger.Instance; } + + /// + /// Maintains a background-refreshed cache of OIDC signing keys for a single authority so that + /// the synchronous delegate + /// can serve keys without blocking any thread-pool thread on the hot request path. + /// + /// On the very first call (cold start), before the background fetch has completed, the method + /// falls back to a blocking fetch executed on a dedicated thread-pool thread to avoid capturing + /// any ambient and preventing deadlocks. + /// + internal sealed class JwksKeyCache + { + private readonly ConfigurationManager _manager; + + // Written only by RefreshAsync; read by GetKeys() on every request. + // Volatile ensures reads see the latest write without a lock. + private volatile IReadOnlyList _cached = []; + + internal JwksKeyCache(string authority, bool requireHttps) + { + _manager = new ConfigurationManager( + authority, + new OpenIdConnectConfigurationRetriever(), + new HttpDocumentRetriever { RequireHttps = requireHttps }); + + // Kick off the first fetch in the background; do not block the startup path. + _ = Task.Run(() => RefreshAsync(CancellationToken.None)); + } + + // Overload for testing: allows injecting a pre-built ConfigurationManager. + internal JwksKeyCache(ConfigurationManager manager) + { + _manager = manager; + _ = Task.Run(() => RefreshAsync(CancellationToken.None)); + } + + /// + /// Returns cached signing keys. On a warm cache this is always allocation-free and + /// non-blocking. On a cold cache (background fetch not yet complete) it blocks briefly + /// on a pool thread to avoid sync-context deadlocks. + /// + internal IEnumerable GetKeys() + { + IReadOnlyList snapshot = _cached; + if (snapshot.Count > 0) + return snapshot; + + // Cold path: background warm-up has not completed yet. + // Task.Run avoids capturing any ambient SynchronizationContext. + return Task.Run(() => RefreshAsync(CancellationToken.None)).GetAwaiter().GetResult(); + } + + private async Task> RefreshAsync(CancellationToken ct) + { + OpenIdConnectConfiguration config = await _manager.GetConfigurationAsync(ct).ConfigureAwait(false); + IReadOnlyList fresh = [.. config.SigningKeys]; + _cached = fresh; + return fresh; + } + } } diff --git a/core/src/Microsoft.Teams.Bot.Core/Http/BotHttpClient.cs b/core/src/Microsoft.Teams.Bot.Core/Http/BotHttpClient.cs index 3eae7eb8..6440e808 100644 --- a/core/src/Microsoft.Teams.Bot.Core/Http/BotHttpClient.cs +++ b/core/src/Microsoft.Teams.Bot.Core/Http/BotHttpClient.cs @@ -210,15 +210,10 @@ private static HttpRequestMessage CreateRequest(HttpMethod method, string url, s if (typeof(T) == typeof(string)) { - try - { - T? result = JsonSerializer.Deserialize(responseString, DefaultJsonOptions); - return result ?? (T)(object)responseString; - } - catch (JsonException) - { - return (T)(object)responseString; - } + // For string responses, try to unwrap a JSON-quoted string first; if the body is + // already plain text (not JSON), return it as-is. The explicit helper avoids the + // fragile (T)(object) double-cast which throws InvalidCastException when T != string. + return DeserializeAsString(responseString); } T? deserializedResult = JsonSerializer.Deserialize(responseString, DefaultJsonOptions); @@ -232,6 +227,28 @@ private static HttpRequestMessage CreateRequest(HttpMethod method, string url, s return deserializedResult; } + /// + /// Handles the T == string branch of response deserialization without the fragile + /// (T)(object)value double-cast. Tries to deserialize a JSON-quoted string; falls back + /// to the raw response text when the body is not valid JSON. + /// + private static T? DeserializeAsString(string responseString) + { + // The caller guarantees typeof(T) == typeof(string), but the compiler can't see that, + // so we keep the cast in one explicit, well-documented place. + try + { + string? jsonString = JsonSerializer.Deserialize(responseString, DefaultJsonOptions); + string result = jsonString ?? responseString; + return (T)(object)result; + } + catch (JsonException) + { + // Body is plain text (not JSON-encoded); return it verbatim. + return (T)(object)responseString; + } + } + private static string FormatResponseHeaders(HttpResponseMessage response) { StringBuilder sb = new(); diff --git a/core/src/Microsoft.Teams.Bot.Core/Schema/CoreActivity.cs b/core/src/Microsoft.Teams.Bot.Core/Schema/CoreActivity.cs index e34f61d6..6cbdb7f7 100644 --- a/core/src/Microsoft.Teams.Bot.Core/Schema/CoreActivity.cs +++ b/core/src/Microsoft.Teams.Bot.Core/Schema/CoreActivity.cs @@ -132,7 +132,9 @@ public CoreActivity() } /// - /// Creates a new instance of the class by copying properties from another activity. + /// Creates a new instance of the class by deep-copying properties from + /// another activity. Mutable reference types (JsonArray, JsonNode, ConversationAccount, ChannelData, + /// ExtendedPropertiesDictionary) are cloned so that mutations on the copy do not affect the original. /// /// The source activity to copy from. protected CoreActivity(CoreActivity activity) @@ -145,17 +147,31 @@ protected CoreActivity(CoreActivity activity) Type = activity.Type; // TODO: Figure out why this is needed... // ReplyToId = activity.ReplyToId; - ChannelData = activity.ChannelData; - From = activity.From; - Recipient = activity.Recipient; - Conversation = activity.Conversation; - Entities = activity.Entities; - Attachments = activity.Attachments; - Properties = activity.Properties; - Value = activity.Value; + // Deep-copy mutable reference types via JSON serialization/deserialization so that + // mutations on the copy do not propagate back to the original (COPY-01 / A-013). + ChannelData = DeepCopy(activity.ChannelData, CoreActivityJsonContext.Default.ChannelData); + From = DeepCopy(activity.From, CoreActivityJsonContext.Default.ConversationAccount); + Recipient = DeepCopy(activity.Recipient, CoreActivityJsonContext.Default.ConversationAccount); + Conversation = DeepCopy(activity.Conversation, CoreActivityJsonContext.Default.Conversation); + Entities = activity.Entities is not null + ? JsonNode.Parse(activity.Entities.ToJsonString())?.AsArray() + : null; + Attachments = activity.Attachments is not null + ? JsonNode.Parse(activity.Attachments.ToJsonString())?.AsArray() + : null; + Value = activity.Value is not null + ? JsonNode.Parse(activity.Value.ToJsonString()) + : null; + Properties = []; + foreach (KeyValuePair kv in activity.Properties) + Properties[kv.Key] = kv.Value; } + /// Serializes and deserializes to produce an independent deep copy. + private static T? DeepCopy(T? source, JsonTypeInfo typeInfo) where T : class + => source is null ? null : JsonSerializer.Deserialize(JsonSerializer.Serialize(source, typeInfo), typeInfo); + /// /// Serializes the current activity to a JSON string. /// diff --git a/core/src/Microsoft.Teams.Bot.Core/TurnMiddleware.cs b/core/src/Microsoft.Teams.Bot.Core/TurnMiddleware.cs index f0b996b1..304b7335 100644 --- a/core/src/Microsoft.Teams.Bot.Core/TurnMiddleware.cs +++ b/core/src/Microsoft.Teams.Bot.Core/TurnMiddleware.cs @@ -17,19 +17,39 @@ namespace Microsoft.Teams.Bot.Core; /// internal sealed class TurnMiddleware : ITurnMiddleware, IEnumerable { - private readonly IList _middlewares = []; + private ITurnMiddleware[] _frozen = []; + private readonly List _building = []; + private bool _isFrozen; /// /// Adds a middleware component to the end of the pipeline. + /// Throws if called after . /// /// The middleware to add. Cannot be null. /// The current TurnMiddleware instance for method chaining. internal TurnMiddleware Use(ITurnMiddleware middleware) { - _middlewares.Add(middleware); + if (_isFrozen) + throw new InvalidOperationException( + "Middleware cannot be added after the pipeline has been frozen (A-020). " + + "Register all middleware before the application starts."); + + _building.Add(middleware); return this; } + /// + /// Seals the middleware list and converts it to a read-only array. Call this once during + /// application startup (e.g. from IHostedService.StartAsync) to enforce the invariant that + /// no middleware is registered after the first request is processed (A-020). + /// Subsequent calls to will throw . + /// + internal void Freeze() + { + _frozen = [.. _building]; + _isFrozen = true; + } + /// /// Processes a turn by executing the middleware pipeline. /// @@ -46,20 +66,17 @@ public async Task OnTurnAsync(BotApplication botApplication, CoreActivity activi /// /// Recursively executes the middleware pipeline starting from the specified index. + /// Uses the frozen array when frozen (fast path); falls back to the building list + /// during startup before Freeze() is called. /// - /// The bot application processing the turn. - /// The activity to process. - /// Optional callback to invoke after all middleware has executed. - /// The index of the next middleware to execute in the pipeline. - /// A cancellation token that can be used to cancel the operation. - /// A task that represents the asynchronous pipeline execution. public Task RunPipelineAsync(BotApplication botApplication, CoreActivity activity, Func? callback, int nextMiddlewareIndex, CancellationToken cancellationToken) { - if (nextMiddlewareIndex == _middlewares.Count) + IList middlewares = _isFrozen ? _frozen : _building; + if (nextMiddlewareIndex == middlewares.Count) { return callback is not null ? callback!(activity, cancellationToken) ?? Task.CompletedTask : Task.CompletedTask; } - ITurnMiddleware nextMiddleware = _middlewares[nextMiddlewareIndex]; + ITurnMiddleware nextMiddleware = middlewares[nextMiddlewareIndex]; return nextMiddleware.OnTurnAsync( botApplication, activity, @@ -69,7 +86,8 @@ public Task RunPipelineAsync(BotApplication botApplication, CoreActivity activit public IEnumerator GetEnumerator() { - return _middlewares.GetEnumerator(); + IList middlewares = _isFrozen ? _frozen : _building; + return middlewares.GetEnumerator(); } IEnumerator IEnumerable.GetEnumerator() diff --git a/core/test/Microsoft.Teams.Bot.Apps.UnitTests/ActivitySerializerMapTests.cs b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/ActivitySerializerMapTests.cs new file mode 100644 index 00000000..a6c46c7d --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/ActivitySerializerMapTests.cs @@ -0,0 +1,82 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Text.Json; +using Microsoft.Teams.Bot.Apps.Handlers; +using Microsoft.Teams.Bot.Apps.Schema; +using Microsoft.Teams.Bot.Core.Schema; + +namespace Microsoft.Teams.Bot.Apps.UnitTests; + +/// +/// Verifies that ActivitySerializerMap is symmetric with ActivityDeserializerMap (SER-01) +/// and that a full serialization round-trip preserves subtype-specific fields. +/// +public class ActivitySerializerMapTests +{ + [Fact] + public void SerializerMap_ContainsEntryForEveryDeserializerType() + { + // Every type produced by the deserializer must have a matching serializer so that + // subtype-specific fields (e.g. ReactionsAdded, MembersAdded) are not silently dropped. + foreach (string activityType in TeamsActivityType.ActivityDeserializerMap.Keys) + { + CoreActivity core = new(activityType); + TeamsActivity deserialized = TeamsActivity.FromActivity(core); + Type concreteType = deserialized.GetType(); + + Assert.True( + TeamsActivityType.ActivitySerializerMap.ContainsKey(concreteType), + $"ActivitySerializerMap is missing an entry for '{concreteType.Name}' " + + $"(activity type '{activityType}'). Add it to keep the maps symmetric."); + } + } + + [Fact] + public void SerializerMap_Count_EqualsDeserializerMap_Count_PlusStreamingActivity() + { + // Serializer has all deserializer types + StreamingActivity (which has no incoming path). + // So: serializer.Count == deserializer.Count + 1 + int deserializerCount = TeamsActivityType.ActivityDeserializerMap.Count; + int serializerCount = TeamsActivityType.ActivitySerializerMap.Count; + Assert.Equal(deserializerCount + 1, serializerCount); + } + + [Theory] + [InlineData(TeamsActivityType.MessageReaction, "reactionsAdded", """[{"type":"like"}]""")] + [InlineData(TeamsActivityType.ConversationUpdate, "membersAdded", """[{"id":"user1","name":"Alice"}]""")] + public void RoundTrip_PreservesSubtypeFields(string activityType, string subtypeProperty, string subtypeValue) + { + // Arrange – build a CoreActivity that carries a subtype-specific field + CoreActivity core = new(activityType); + core.Properties[subtypeProperty] = JsonSerializer.Deserialize(subtypeValue); + + // Act – deserialize into the specialized type, then serialize back + TeamsActivity teamsActivity = TeamsActivity.FromActivity(core); + string json = teamsActivity.ToJson(); + + // Assert – the subtype-specific field survived the round-trip + Assert.Contains(subtypeProperty, json); + } + + [Fact] + public void MessageReactionActivity_RoundTrip_PreservesReactions() + { + // Arrange + CoreActivity core = new(TeamsActivityType.MessageReaction); + core.Properties["reactionsAdded"] = JsonSerializer.Deserialize("""[{"type":"like"},{"type":"heart"}]"""); + core.Properties["replyToId"] = JsonSerializer.Deserialize("\"msg-001\""); + + // Act + TeamsActivity teamsActivity = TeamsActivity.FromActivity(core); + Assert.IsType(teamsActivity); + + string json = teamsActivity.ToJson(); + + // Assert – both reaction-specific fields are present + Assert.Contains("reactionsAdded", json); + Assert.Contains("like", json); + Assert.Contains("replyToId", json); + Assert.Contains("msg-001", json); + } +} diff --git a/core/test/Microsoft.Teams.Bot.Apps.UnitTests/CoreActivityDeepCopyTests.cs b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/CoreActivityDeepCopyTests.cs new file mode 100644 index 00000000..d91aec5d --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/CoreActivityDeepCopyTests.cs @@ -0,0 +1,107 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Text.Json.Nodes; +using Microsoft.Teams.Bot.Apps.Schema; +using Microsoft.Teams.Bot.Core.Schema; + +namespace Microsoft.Teams.Bot.Apps.UnitTests; + +/// +/// Verifies that the CoreActivity copy constructor produces independent (deep) copies so that +/// mutations on the copy do not affect the original, and vice versa (COPY-01 / A-013). +/// +public class CoreActivityDeepCopyTests +{ + private static CoreActivity MakeActivity() => CoreActivity.FromJsonString(""" + { + "type": "message", + "channelId": "msteams", + "id": "act-001", + "from": { "id": "user-1", "name": "Alice" }, + "recipient": { "id": "bot-1", "name": "Bot" }, + "conversation":{ "id": "conv-1" }, + "channelData": { "tenant": { "id": "tenant-1" } }, + "entities": [{ "type": "mention", "text": "@Bot" }], + "attachments": [{ "contentType": "text/plain" }], + "value": { "key": "original" } + } + """); + + [Fact] + public void CopyConstructor_From_IsIndependent() + { + CoreActivity original = MakeActivity(); + TeamsActivity copy = TeamsActivity.FromActivity(original); + + // Mutate the copy's From account + copy.From!.Properties["aadObjectId"] = "new-aad-id"; + + // Original must be unaffected + Assert.False(original.From!.Properties.ContainsKey("aadObjectId")); + } + + [Fact] + public void CopyConstructor_Conversation_IsIndependent() + { + CoreActivity original = MakeActivity(); + TeamsActivity copy = TeamsActivity.FromActivity(original); + + copy.Conversation!.Properties["tenantId"] = "mutated"; + + Assert.False(original.Conversation!.Properties.ContainsKey("tenantId")); + } + + [Fact] + public void CopyConstructor_Properties_IsIndependent() + { + CoreActivity original = MakeActivity(); + original.Properties["customKey"] = "original-value"; + + TeamsActivity copy = TeamsActivity.FromActivity(original); + + copy.Properties["customKey"] = "mutated-value"; + + Assert.Equal("original-value", original.Properties["customKey"]?.ToString()); + } + + [Fact] + public void CopyConstructor_Value_IsIndependent() + { + CoreActivity original = MakeActivity(); + TeamsActivity copy = TeamsActivity.FromActivity(original); + + // Mutate the copy's Value node + if (copy.Value is JsonObject obj) + obj["key"] = "mutated"; + + // Original value node must be unaffected + if (original.Value is JsonObject origObj) + Assert.Equal("original", origObj["key"]?.GetValue()); + } + + // ── CitationEntity deep-copy (COPY-01 / A-015) ──────────────────────────── + + [Fact] + public void CitationEntity_CopyCtor_ClaimListIsIndependent() + { + Microsoft.Teams.Bot.Apps.Schema.Entities.CitationEntity original = new() + { + Citation = + [ + new() + { + Position = 1, + Appearance = new() { Name = "Doc1", Abstract = "Abs1" } + } + ] + }; + + Microsoft.Teams.Bot.Apps.Schema.Entities.CitationEntity copy = new(original); + + // Mutate the copy's claim — original must be unaffected + copy.Citation![0].Position = 99; + + Assert.Equal(1, original.Citation![0].Position); + } +} diff --git a/core/test/Microsoft.Teams.Bot.Apps.UnitTests/StreamCancellationDetectionTests.cs b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/StreamCancellationDetectionTests.cs new file mode 100644 index 00000000..d64e96de --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/StreamCancellationDetectionTests.cs @@ -0,0 +1,77 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Net; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Teams.Bot.Apps.Schema; +using Microsoft.Teams.Bot.Core; +using Microsoft.Teams.Bot.Core.Schema; + +namespace Microsoft.Teams.Bot.Apps.UnitTests; + +/// +/// Verifies that stream-cancellation detection in TeamsStreamingWriter uses structured +/// signals (HTTP status code / additional text) rather than a single fragile message +/// string match (A-009). +/// +public class StreamCancellationDetectionTests +{ + private static TeamsActivity CreateRef() => new() + { + Type = TeamsActivityType.Message, + ChannelId = "msteams", + ServiceUrl = new Uri("https://smba.trafficmanager.net/amer/"), + Conversation = TeamsConversation.FromConversation(new Conversation { Id = "conv-1" }), + From = TeamsConversationAccount.FromConversationAccount(new ConversationAccount { Id = "user-1", Name = "User" }), + Recipient = TeamsConversationAccount.FromConversationAccount(new ConversationAccount { Id = "bot-1", Name = "Bot" }) + }; + + private sealed class AlwaysThrowHandler(HttpStatusCode status, string message) : HttpMessageHandler + { + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + => throw new HttpRequestException(message, inner: null, statusCode: status); + } + + private static TeamsStreamingWriter BuildWriter(HttpStatusCode status, string message) + { + ConversationClient client = new(new HttpClient(new AlwaysThrowHandler(status, message)), + NullLogger.Instance); + return new TeamsStreamingWriter(client, CreateRef()); + } + + [Theory] + [InlineData("Content stream was cancelled by user")] + [InlineData("stream was closed")] + public async Task AppendAsync_WithCancellationMessage_SetsCancelledSilently(string msg) + { + // Use a status code that is NOT 499/408 to prove the text fallback works + TeamsStreamingWriter writer = BuildWriter(HttpStatusCode.InternalServerError, msg); + + // Should not throw — cancellation must be swallowed + await writer.AppendResponseAsync("chunk"); + + // After being cancelled the writer is a no-op — FinalizeResponseAsync returns without sending + // (nothing accumulated; the cancellation guard returns early) + } + + [Fact] + public async Task AppendAsync_WithStatusCode499_SetsCancelledSilently() + { + // 499 "Client Closed Request" — structured cancellation signal + TeamsStreamingWriter writer = BuildWriter((HttpStatusCode)499, "any message"); + + await writer.AppendResponseAsync("chunk"); + // No exception — cancelled silently + } + + [Fact] + public async Task AppendAsync_WithUnrelatedServerError_PropagatesException() + { + // A real server error (500 with a non-cancellation message) must NOT be swallowed — + // it propagates immediately from AppendResponseAsync. + TeamsStreamingWriter writer = BuildWriter(HttpStatusCode.InternalServerError, "generic database error"); + + await Assert.ThrowsAsync( + () => writer.AppendResponseAsync("chunk")); + } +} diff --git a/core/test/Microsoft.Teams.Bot.Apps.UnitTests/TeamsActivityTests.cs b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/TeamsActivityTests.cs index 85a461f0..9f694f03 100644 --- a/core/test/Microsoft.Teams.Bot.Apps.UnitTests/TeamsActivityTests.cs +++ b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/TeamsActivityTests.cs @@ -425,5 +425,74 @@ public void Deserialize_with_Conversation_and_Tenant() } """; + // ── P2-3 / CAST-01: as-cast null guard tests ───────────────────────────── + [Fact] + public void From_WhenBaseHoldsPlainConversationAccount_ReturnsWrappedTeamsConversationAccount() + { + // Arrange – bypass the TeamsActivity setter by writing directly to the base field + // via a CoreActivity (simulates what JSON deserialization into CoreActivity does). + CoreActivity coreActivity = CoreActivity.FromJsonString(""" + { + "type": "message", + "from": { "id": "user-1", "name": "Alice" } + } + """); + + // Act – create TeamsActivity from the core activity (base field holds ConversationAccount) + TeamsActivity teamsActivity = TeamsActivity.FromActivity(coreActivity); + + // Assert – the shadowed getter must not silently return null + Assert.NotNull(teamsActivity.From); + Assert.Equal("user-1", teamsActivity.From!.Id); + Assert.Equal("Alice", teamsActivity.From.Name); + } + + [Fact] + public void Recipient_WhenBaseHoldsPlainConversationAccount_ReturnsWrappedTeamsConversationAccount() + { + CoreActivity coreActivity = CoreActivity.FromJsonString(""" + { + "type": "message", + "recipient": { "id": "bot-1", "name": "MyBot" } + } + """); + + TeamsActivity teamsActivity = TeamsActivity.FromActivity(coreActivity); + + Assert.NotNull(teamsActivity.Recipient); + Assert.Equal("bot-1", teamsActivity.Recipient!.Id); + } + + [Fact] + public void Conversation_WhenBaseHoldsPlainConversation_ReturnsWrappedTeamsConversation() + { + CoreActivity coreActivity = CoreActivity.FromJsonString(""" + { + "type": "message", + "conversation": { "id": "conv-99", "tenantId": "tenant-42" } + } + """); + + TeamsActivity teamsActivity = TeamsActivity.FromActivity(coreActivity); + + Assert.NotNull(teamsActivity.Conversation); + Assert.Equal("conv-99", teamsActivity.Conversation!.Id); + Assert.Equal("tenant-42", teamsActivity.Conversation.TenantId); + } + + [Fact] + public void ChannelData_WhenBaseHoldsPlainChannelData_ReturnsTeamsChannelData() + { + CoreActivity coreActivity = CoreActivity.FromJsonString(""" + { + "type": "message", + "channelData": { "tenant": { "id": "tenant-55" } } + } + """); + + TeamsActivity teamsActivity = TeamsActivity.FromActivity(coreActivity); + + Assert.NotNull(teamsActivity.ChannelData); + } } diff --git a/core/test/Microsoft.Teams.Bot.Apps.UnitTests/TeamsAttachmentBuilderTests.cs b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/TeamsAttachmentBuilderTests.cs new file mode 100644 index 00000000..cbb8c9eb --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/TeamsAttachmentBuilderTests.cs @@ -0,0 +1,60 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Teams.Bot.Apps.Schema; + +namespace Microsoft.Teams.Bot.Apps.UnitTests; + +public class TeamsAttachmentBuilderTests +{ + [Fact] + public void Build_FirstCall_ReturnsAttachment() + { + TeamsAttachment result = TeamsActivity.CreateBuilder() + .WithAttachment(new TeamsAttachment { ContentType = "text/plain" }) + .Build() + .Attachments![0]; + + // Verify via TeamsAttachmentBuilder directly + TeamsAttachmentBuilder builder = new TeamsAttachmentBuilder() + .WithContentType("text/plain") + .WithContent("hello"); + + TeamsAttachment attachment = builder.Build(); + Assert.NotNull(attachment); + Assert.Equal("text/plain", attachment.ContentType); + } + + [Fact] + public void Build_SecondCall_ThrowsInvalidOperationException() + { + // Arrange + TeamsAttachmentBuilder builder = new TeamsAttachmentBuilder() + .WithContentType("text/plain"); + + builder.Build(); // first call — OK + + // Act & Assert — second call must throw + Assert.Throws(() => builder.Build()); + } + + [Fact] + public void Build_AfterMutation_ThrowsOnSecondBuild() + { + // Demonstrate that the guard prevents post-Build mutations from being silently + // applied to an already-returned attachment. + TeamsAttachmentBuilder builder = new TeamsAttachmentBuilder() + .WithContentType("application/vnd.microsoft.card.adaptive") + .WithContent(new { type = "AdaptiveCard" }); + + TeamsAttachment first = builder.Build(); + Assert.Equal("application/vnd.microsoft.card.adaptive", first.ContentType); + + // Attempting to build again (e.g. after changing the content) must fail + Assert.Throws(() => + { + builder.WithContent(new { type = "Modified" }); + builder.Build(); + }); + } +} diff --git a/core/test/Microsoft.Teams.Bot.Apps.UnitTests/UnknownEntityTypeTests.cs b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/UnknownEntityTypeTests.cs new file mode 100644 index 00000000..4664e19e --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Apps.UnitTests/UnknownEntityTypeTests.cs @@ -0,0 +1,33 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Teams.Bot.Apps.Schema; +using Microsoft.Teams.Bot.Apps.Schema.Entities; +using Microsoft.Teams.Bot.Core.Schema; + +namespace Microsoft.Teams.Bot.Apps.UnitTests; + +public class UnknownEntityTypeTests +{ + [Fact] + public void UnknownType_IsPreserved_NotDropped() + { + CoreActivity activity = CoreActivity.FromJsonString( + "{\"type\":\"message\",\"entities\":[{\"type\":\"BotMessageMetadata\",\"someField\":\"v\"},{\"type\":\"clientInfo\",\"platform\":\"Web\"}]}"); + TeamsActivity teamsActivity = TeamsActivity.FromActivity(activity); + Assert.NotNull(teamsActivity.Entities); + Assert.Equal(2, teamsActivity.Entities.Count); + } + + [Fact] + public void UnknownType_DeserializesAsBaseEntity_WithProperties() + { + CoreActivity activity = CoreActivity.FromJsonString( + "{\"type\":\"message\",\"entities\":[{\"type\":\"BotMessageMetadata\",\"someField\":\"v\"}]}"); + TeamsActivity teamsActivity = TeamsActivity.FromActivity(activity); + Entity unknown = teamsActivity.Entities![0]; + Assert.IsType(unknown); + Assert.Equal("BotMessageMetadata", unknown.Type); + Assert.True(unknown.Properties.ContainsKey("someField")); + } +} diff --git a/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatConnectorClientTests.cs b/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatConnectorClientTests.cs new file mode 100644 index 00000000..409ba279 --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatConnectorClientTests.cs @@ -0,0 +1,49 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Teams.Bot.Core; +using Moq; + +namespace Microsoft.Teams.Bot.Compat.UnitTests +{ + /// + /// Verifies the A-019 fix: CompatConnectorClient.Dispose() must be a safe no-op + /// and must not call GC.SuppressFinalize (the class has no finalizer and owns no + /// unmanaged resources). + /// + public class CompatConnectorClientTests + { + private static CompatConnectorClient CreateClient() + { + Mock mockHttp = new(); + ConversationClient cc = new(mockHttp.Object, NullLogger.Instance); + CompatConversations conversations = new(cc); + return new CompatConnectorClient(conversations); + } + + [Fact] + public void Dispose_DoesNotThrow() + { + // Arrange + CompatConnectorClient client = CreateClient(); + + // Act & Assert – Dispose() must be a safe no-op + Exception? caught = Record.Exception(() => client.Dispose()); + Assert.Null(caught); + } + + [Fact] + public void Dispose_CalledMultipleTimes_DoesNotThrow() + { + CompatConnectorClient client = CreateClient(); + + Exception? caught = Record.Exception(() => + { + client.Dispose(); + client.Dispose(); + }); + Assert.Null(caught); + } + } +} diff --git a/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatConversationsTests.cs b/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatConversationsTests.cs index fd575703..405f6d54 100644 --- a/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatConversationsTests.cs +++ b/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatConversationsTests.cs @@ -383,5 +383,34 @@ private static Mock CreateMockConversationClient() return mock; } + + // ── P4-3 / A-022: ServiceUrl null guard ────────────────────────────────── + + [Fact] + public async Task GetActivityMembersWithHttpMessagesAsync_WithNullServiceUrl_ThrowsArgumentException() + { + Mock mockClient = CreateMockConversationClient(); + CompatConversations conversations = new(mockClient.Object) + { + ServiceUrl = null // intentionally not set + }; + + await Assert.ThrowsAnyAsync( + () => conversations.GetActivityMembersWithHttpMessagesAsync( + TestConversationId, TestActivityId)); + } + + [Fact] + public async Task GetConversationsWithHttpMessagesAsync_WithNullServiceUrl_ThrowsArgumentException() + { + Mock mockClient = CreateMockConversationClient(); + CompatConversations conversations = new(mockClient.Object) + { + ServiceUrl = null + }; + + await Assert.ThrowsAnyAsync( + () => conversations.GetConversationsWithHttpMessagesAsync()); + } } } diff --git a/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatTeamsInfoCastTests.cs b/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatTeamsInfoCastTests.cs new file mode 100644 index 00000000..add878df --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatTeamsInfoCastTests.cs @@ -0,0 +1,62 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Bot.Builder; +using Microsoft.Bot.Builder.Adapters; +using Microsoft.Bot.Schema; +using Microsoft.Teams.Bot.Compat; +using Moq; + +namespace Microsoft.Teams.Bot.Compat.UnitTests; + +/// +/// Verifies that methods accepting IActivity in CompatTeamsInfo throw a descriptive +/// ArgumentException instead of an InvalidCastException when the activity is not an +/// instance of the Bot Framework Activity class (A-016). +/// +public class CompatTeamsInfoCastTests +{ + [Fact] + public async Task SendMessageToListOfUsersAsync_WithNonActivityIActivity_ThrowsDescriptiveArgumentException() + { + // Arrange – a mock ITurnContext backed by a real Activity (required by the helper) + TestAdapter adapter = new(); + Activity seed = new() { Type = ActivityTypes.Message, ServiceUrl = "https://example.com/" }; + ITurnContext ctx = new TurnContext(adapter, seed); + + // Create a mock IActivity that is NOT an Activity instance + Mock mockActivity = new(); + + // Act & Assert – must throw ArgumentException, not InvalidCastException + ArgumentException ex = await Assert.ThrowsAsync( + () => CompatTeamsInfo.SendMessageToListOfUsersAsync( + ctx, + mockActivity.Object, + [new Microsoft.Bot.Schema.Teams.TeamMember { Id = "user-1" }], + "tenant-1", + CancellationToken.None)); + + Assert.Equal("activity", ex.ParamName); + Assert.Contains("Activity", ex.Message); + } + + [Fact] + public async Task SendMessageToListOfChannelsAsync_WithNonActivityIActivity_ThrowsArgumentException() + { + TestAdapter adapter = new(); + Activity seed = new() { Type = ActivityTypes.Message, ServiceUrl = "https://example.com/" }; + ITurnContext ctx = new TurnContext(adapter, seed); + + Mock mockActivity = new(); + + ArgumentException ex = await Assert.ThrowsAsync( + () => CompatTeamsInfo.SendMessageToListOfChannelsAsync( + ctx, + mockActivity.Object, + [new Microsoft.Bot.Schema.Teams.TeamMember { Id = "channel-1" }], + "tenant-1", + CancellationToken.None)); + + Assert.Equal("activity", ex.ParamName); + } +} diff --git a/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatTeamsInfoRoundTripTests.cs b/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatTeamsInfoRoundTripTests.cs new file mode 100644 index 00000000..b2c1441c --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Compat.UnitTests/CompatTeamsInfoRoundTripTests.cs @@ -0,0 +1,69 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Text.Json; +using Microsoft.Bot.Schema.Teams; + +namespace Microsoft.Teams.Bot.Compat.UnitTests; + +/// +/// Verifies that the STJ-only round-trip in CompatTeamsInfo.SendMeetingNotificationAsync +/// preserves all known property names (A-021). +/// The test exercises the serializer options directly rather than going through the full +/// ASP.NET pipeline, which would require live auth tokens. +/// +public class CompatTeamsInfoRoundTripTests +{ + private static readonly JsonSerializerOptions s_jsonOptions = new() + { + PropertyNameCaseInsensitive = true + }; + + [Fact] + public void StjRoundTrip_PreservesChannelDataProperty() + { + // Arrange – a TargetedMeetingNotification with a nested channel-data object + TargetedMeetingNotification original = new() + { + Type = "targetedMeetingNotification", + Value = new TargetedMeetingNotificationValue + { + Recipients = ["user-1", "user-2"], + } + }; + + // Act – STJ → STJ (the fixed code path) + string json = JsonSerializer.Serialize(original, s_jsonOptions); + TargetedMeetingNotification? roundTripped = JsonSerializer.Deserialize(json, s_jsonOptions); + + // Assert – key fields survived the round-trip + Assert.NotNull(roundTripped); + Assert.Equal("targetedMeetingNotification", roundTripped.Type); + Assert.NotNull(roundTripped.Value); + Assert.Equal(2, roundTripped.Value.Recipients?.Count); + Assert.Contains("user-1", roundTripped.Value.Recipients!); + } + + [Fact] + public void StjRoundTrip_PreservesAllKnownTopLevelProperties() + { + // Arrange – use a plain dictionary to simulate arbitrary known properties + Dictionary properties = new(StringComparer.OrdinalIgnoreCase) + { + ["type"] = "targetedMeetingNotification", + ["channelData"] = new { someField = "value1" }, + }; + + string originalJson = JsonSerializer.Serialize(properties, s_jsonOptions); + + // Act – deserialize and re-serialize with STJ + Dictionary? result = JsonSerializer.Deserialize>(originalJson, s_jsonOptions); + string reserialized = JsonSerializer.Serialize(result, s_jsonOptions); + + // Assert – every property key survived + foreach (string key in properties.Keys) + { + Assert.Contains(key, reserialized, StringComparison.OrdinalIgnoreCase); + } + } +} diff --git a/core/test/Microsoft.Teams.Bot.Compat.UnitTests/ExtractStringTests.cs b/core/test/Microsoft.Teams.Bot.Compat.UnitTests/ExtractStringTests.cs new file mode 100644 index 00000000..6db4e993 --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Compat.UnitTests/ExtractStringTests.cs @@ -0,0 +1,87 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Text.Json; +using Microsoft.Teams.Bot.Compat; + +namespace Microsoft.Teams.Bot.Compat.UnitTests; + +/// +/// Verifies the ExtractString helper correctly handles JsonElement values from STJ +/// [JsonExtensionData] dictionaries, preventing the ToString() corruption (A-007). +/// +public class ExtractStringTests +{ + [Fact] + public void ExtractString_WithPlainString_ReturnsAsIs() + { + Assert.Equal("hello", CompatActivity.ExtractString("hello")); + } + + [Fact] + public void ExtractString_WithNull_ReturnsNull() + { + Assert.Null(CompatActivity.ExtractString(null)); + } + + [Fact] + public void ExtractString_WithJsonElementString_ReturnsUnquotedValue() + { + // Arrange – simulate what STJ produces for a string value stored in [JsonExtensionData] + JsonElement je = JsonDocument.Parse("\"aad-object-id-value\"").RootElement; + + // Act + string? result = CompatActivity.ExtractString(je); + + // Assert – must NOT include surrounding quotes + Assert.Equal("aad-object-id-value", result); + } + + [Fact] + public void ExtractString_WithJsonElement_ToString_WouldReturnQuotedValue() + { + // Demonstrate the bug: JsonElement.ToString() returns the quoted JSON representation + JsonElement je = JsonDocument.Parse("\"some-guid\"").RootElement; + string quoted = je.ToString(); + + // ToString() gives raw JSON text for String kind on .NET 6+ (returns unquoted actually) + // but GetString() is always the correct way to extract the string value + Assert.Equal("some-guid", CompatActivity.ExtractString(je)); + Assert.Equal("some-guid", je.GetString()); // GetString is always correct + } + + [Fact] + public void ExtractString_WithNonStringJsonElement_ReturnsRawText() + { + // Non-string JsonElement (e.g. a number) should return GetRawText(), not throw + JsonElement je = JsonDocument.Parse("42").RootElement; + string? result = CompatActivity.ExtractString(je); + Assert.Equal("42", result); + } + + [Fact] + public void ToCompatChannelAccount_WithJsonElementProperties_ExtractsCorrectly() + { + // Arrange – a ConversationAccount whose Properties contain JsonElement values + // (as produced by STJ [JsonExtensionData] after JSON deserialization) + Microsoft.Teams.Bot.Core.Schema.ConversationAccount account = + Microsoft.Teams.Bot.Core.Schema.CoreActivity.FromJsonString(""" + { + "type": "message", + "from": { + "id": "user-1", + "name": "Alice", + "aadObjectId": "aad-guid-123", + "userRole": "user" + } + } + """).From!; + + // Act + Microsoft.Bot.Schema.ChannelAccount channelAccount = account.ToCompatChannelAccount(); + + // Assert – string fields must not contain surrounding quotes or type names + Assert.Equal("aad-guid-123", channelAccount.AadObjectId); + Assert.Equal("user", channelAccount.Role); + } +} diff --git a/core/test/Microsoft.Teams.Bot.Core.UnitTests/BotApplicationTests.cs b/core/test/Microsoft.Teams.Bot.Core.UnitTests/BotApplicationTests.cs index 302327d0..6fe22a40 100644 --- a/core/test/Microsoft.Teams.Bot.Core.UnitTests/BotApplicationTests.cs +++ b/core/test/Microsoft.Teams.Bot.Core.UnitTests/BotApplicationTests.cs @@ -1,6 +1,7 @@ // Copyright (c) Microsoft Corporation. // Licensed under the MIT License. +using System.Collections.Concurrent; using System.Net; using System.Text; using Microsoft.AspNetCore.Http; @@ -197,6 +198,72 @@ await Assert.ThrowsAsync(() => botApp.SendActivityAsync(null!)); } + [Fact] + public async Task ProcessAsync_WithActivityHandlerOverride_UsesProvidedHandlerNotSharedField() + { + // Arrange + BotApplication botApp = CreateBotApplication(); + + // Set a sentinel on the shared field to detect if it is incorrectly invoked + bool sharedFieldInvoked = false; + botApp.OnActivity = (_, _) => + { + sharedFieldInvoked = true; + return Task.CompletedTask; + }; + + CoreActivity activity = new() { Type = ActivityType.Message, Id = "act-override" }; + DefaultHttpContext httpContext = CreateHttpContextWithActivity(activity); + + bool perRequestHandlerInvoked = false; + Func perRequestHandler = (_, _) => + { + perRequestHandlerInvoked = true; + return Task.CompletedTask; + }; + + // Act + await botApp.ProcessAsync(httpContext, perRequestHandler, CancellationToken.None); + + // Assert – only the per-request handler must fire; the shared field must not + Assert.True(perRequestHandlerInvoked); + Assert.False(sharedFieldInvoked); + } + + [Fact] + public async Task ProcessAsync_ConcurrentRequests_EachHandlerReceivesItsOwnActivity() + { + // Arrange + BotApplication botApp = CreateBotApplication(); + + int concurrency = 20; + ConcurrentDictionary observedActivities = new(); + + // Act – fire concurrently; each request supplies its own scoped handler + await Parallel.ForEachAsync(Enumerable.Range(0, concurrency), async (i, ct) => + { + string activityId = $"act-{i}"; + CoreActivity activity = new() { Type = ActivityType.Message, Id = activityId }; + DefaultHttpContext httpContext = CreateHttpContextWithActivity(activity); + + Func handler = (act, _) => + { + observedActivities[activityId] = act.Id!; + return Task.CompletedTask; + }; + + await botApp.ProcessAsync(httpContext, handler, ct); + }); + + // Assert – every handler must have received exactly its own activity (no cross-contamination) + Assert.Equal(concurrency, observedActivities.Count); + for (int i = 0; i < concurrency; i++) + { + string expected = $"act-{i}"; + Assert.Equal(expected, observedActivities[expected]); + } + } + private static BotApplicationOptions CreateOptions(string appId) => new() { AppId = appId }; diff --git a/core/test/Microsoft.Teams.Bot.Core.UnitTests/Hosting/BotAuthenticationHandlerTests.cs b/core/test/Microsoft.Teams.Bot.Core.UnitTests/Hosting/BotAuthenticationHandlerTests.cs new file mode 100644 index 00000000..dbfbbbef --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Core.UnitTests/Hosting/BotAuthenticationHandlerTests.cs @@ -0,0 +1,139 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Net.Http.Headers; +using System.Security.Claims; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Options; +using Microsoft.Identity.Abstractions; +using Microsoft.Teams.Bot.Core.Hosting; +using Microsoft.Teams.Bot.Core.Schema; +using Moq; + +namespace Microsoft.Teams.Bot.Core.UnitTests.Hosting; + +public class BotAuthenticationHandlerTests +{ + private static BotAuthenticationHandler CreateHandler( + Mock providerMock, + string scope = "https://api.botframework.com/.default") + { + return new BotAuthenticationHandler( + providerMock.Object, + NullLogger.Instance, + scope, + managedIdentityOptions: null); + } + + [Fact] + public async Task SendAsync_WithValidAgenticUserId_AcquiresAgenticToken() + { + // Arrange + Mock providerMock = new(); + providerMock + .Setup(p => p.CreateAuthorizationHeaderAsync( + It.IsAny>(), + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ReturnsAsync("Bearer agentic-token"); + + BotAuthenticationHandler handler = CreateHandler(providerMock); + handler.InnerHandler = new SuccessHandler(); + + HttpRequestMessage request = new(HttpMethod.Get, "https://example.com"); + request.Options.Set(BotAuthenticationHandler.AgenticIdentityKey, new AgenticIdentity + { + AgenticAppId = "app-id-123", + AgenticUserId = "00000000-0000-0000-0000-000000000001" // valid GUID + }); + + using HttpMessageInvoker invoker = new(handler); + + // Act + HttpResponseMessage response = await invoker.SendAsync(request, CancellationToken.None); + + // Assert – agentic provider was called + Assert.True(response.IsSuccessStatusCode); + providerMock.Verify(p => p.CreateAuthorizationHeaderAsync( + It.IsAny>(), + It.IsAny(), + It.IsAny(), + It.IsAny()), Times.Once); + } + + [Fact] + public async Task SendAsync_WithMalformedAgenticUserId_FallsBackToAppOnlyToken() + { + // Arrange – malformed GUID should NOT throw; handler falls back to app-only + Mock providerMock = new(); + providerMock + .Setup(p => p.CreateAuthorizationHeaderForAppAsync( + It.IsAny(), + It.IsAny(), + It.IsAny())) + .ReturnsAsync("Bearer app-only-token"); + + BotAuthenticationHandler handler = CreateHandler(providerMock); + handler.InnerHandler = new SuccessHandler(); + + HttpRequestMessage request = new(HttpMethod.Get, "https://example.com"); + request.Options.Set(BotAuthenticationHandler.AgenticIdentityKey, new AgenticIdentity + { + AgenticAppId = "app-id-123", + AgenticUserId = "not-a-guid" // malformed — must NOT throw FormatException + }); + + using HttpMessageInvoker invoker = new(handler); + + // Act – must succeed without throwing + HttpResponseMessage response = await invoker.SendAsync(request, CancellationToken.None); + + // Assert – fell back to app-only + Assert.True(response.IsSuccessStatusCode); + providerMock.Verify(p => p.CreateAuthorizationHeaderForAppAsync( + It.IsAny(), + It.IsAny(), + It.IsAny()), Times.Once); + // Agentic path must NOT have been taken + providerMock.Verify(p => p.CreateAuthorizationHeaderAsync( + It.IsAny>(), + It.IsAny(), + It.IsAny(), + It.IsAny()), Times.Never); + } + + [Fact] + public async Task SendAsync_WithMalformedJwtToken_DoesNotCrash() + { + // Arrange – LogTokenClaims should swallow parse errors, never crashing the pipeline + Mock providerMock = new(); + providerMock + .Setup(p => p.CreateAuthorizationHeaderForAppAsync( + It.IsAny(), + It.IsAny(), + It.IsAny())) + // Return a non-JWT opaque token (e.g. managed identity token) + .ReturnsAsync("Bearer not.a.valid.jwt.token.here"); + + BotAuthenticationHandler handler = CreateHandler(providerMock); + handler.InnerHandler = new SuccessHandler(); + + HttpRequestMessage request = new(HttpMethod.Get, "https://example.com"); + // No agentic identity — goes to app-only path + using HttpMessageInvoker invoker = new(handler); + + // Act – must not throw even though the token can't be parsed for claim logging + HttpResponseMessage response = await invoker.SendAsync(request, CancellationToken.None); + + // Assert + Assert.True(response.IsSuccessStatusCode); + } + + // Minimal inner handler that always returns 200 OK. + private sealed class SuccessHandler : HttpMessageHandler + { + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + => Task.FromResult(new HttpResponseMessage(System.Net.HttpStatusCode.OK)); + } +} diff --git a/core/test/Microsoft.Teams.Bot.Core.UnitTests/Hosting/BotConfigTests.cs b/core/test/Microsoft.Teams.Bot.Core.UnitTests/Hosting/BotConfigTests.cs new file mode 100644 index 00000000..492c559d --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Core.UnitTests/Hosting/BotConfigTests.cs @@ -0,0 +1,79 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Teams.Bot.Core.Hosting; + +namespace Microsoft.Teams.Bot.Core.UnitTests.Hosting; + +public class BotConfigTests +{ + [Fact] + public void Resolve_WithIConfigurationRegistered_ResolvesWithoutBuildingContainer() + { + // Arrange + IConfiguration config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["AzureAd:ClientId"] = "client-abc", + ["AzureAd:TenantId"] = "tenant-xyz" + }) + .Build(); + + ServiceCollection services = new(); + services.AddSingleton(config); + + // Act – must not call BuildServiceProvider internally + BotConfig result = BotConfig.Resolve(services, "AzureAd"); + + // Assert + Assert.Equal("client-abc", result.ClientId); + Assert.Equal("tenant-xyz", result.TenantId); + } + + [Fact] + public void Resolve_WithoutIConfigurationRegistered_ThrowsInvalidOperationException() + { + // Arrange – deliberately no IConfiguration in the collection + ServiceCollection services = new(); + + // Act & Assert – must fail fast rather than building a throwaway container + InvalidOperationException ex = Assert.Throws( + () => BotConfig.Resolve(services, "AzureAd")); + + Assert.Contains("IConfiguration", ex.Message); + } + + [Fact] + public void AddBotAuthentication_WithoutIConfiguration_ThrowsInvalidOperationException() + { + // Verify that AddBotAuthentication (which calls ResolveBotConfig internally) + // also fails fast when IConfiguration is absent. + ServiceCollection services = new(); + + InvalidOperationException ex = Assert.Throws( + () => services.AddBotAuthentication(aadSectionName: "AzureAd")); + + Assert.Contains("IConfiguration", ex.Message); + } + + [Fact] + public void AddBotAuthentication_WithIConfiguration_DoesNotThrow() + { + // Verify that the normal path (IConfiguration registered) works end-to-end. + IConfiguration config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["AzureAd:ClientId"] = "client-abc", + ["AzureAd:TenantId"] = "tenant-xyz" + }) + .Build(); + + ServiceCollection services = new(); + services.AddSingleton(config); + + // Should complete without throwing + services.AddBotAuthentication(aadSectionName: "AzureAd"); + } +} diff --git a/core/test/Microsoft.Teams.Bot.Core.UnitTests/Hosting/JwksKeyCacheTests.cs b/core/test/Microsoft.Teams.Bot.Core.UnitTests/Hosting/JwksKeyCacheTests.cs new file mode 100644 index 00000000..68eb7c7a --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Core.UnitTests/Hosting/JwksKeyCacheTests.cs @@ -0,0 +1,92 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Collections.Concurrent; +using Microsoft.IdentityModel.Protocols; +using Microsoft.IdentityModel.Protocols.OpenIdConnect; +using Microsoft.IdentityModel.Tokens; +using Microsoft.Teams.Bot.Core.Hosting; +using Moq; + +namespace Microsoft.Teams.Bot.Core.UnitTests.Hosting; + +public class JwksKeyCacheTests +{ + /// + /// Creates a ConfigurationManager whose GetConfigurationAsync always returns a + /// configuration with the supplied signing key. + /// + private static ConfigurationManager BuildManager(SecurityKey key) + { + OpenIdConnectConfiguration config = new(); + config.SigningKeys.Add(key); + + Mock> mockRetriever = new(); + mockRetriever + .Setup(r => r.GetConfigurationAsync(It.IsAny(), It.IsAny(), It.IsAny())) + .ReturnsAsync(config); + + return new ConfigurationManager( + "https://example.com/.well-known/openid-configuration", + mockRetriever.Object); + } + + [Fact] + public async Task GetKeys_ReturnsKeysAfterBackgroundWarmup() + { + // Arrange + RsaSecurityKey key = new(System.Security.Cryptography.RSA.Create(2048)); + ConfigurationManager manager = BuildManager(key); + JwksKeyCache cache = new(manager); + + // Give the background task a moment to complete + await Task.Delay(200); + + // Act + IEnumerable keys = cache.GetKeys(); + + // Assert – the background fetch populated the cache + Assert.Single(keys); + Assert.Same(key, keys.First()); + } + + [Fact] + public void GetKeys_WhenCalledConcurrently_DoesNotThrow() + { + // Arrange + RsaSecurityKey key = new(System.Security.Cryptography.RSA.Create(2048)); + ConfigurationManager manager = BuildManager(key); + JwksKeyCache cache = new(manager); + + // Act – hammer GetKeys from many threads before warm-up completes + ConcurrentBag> results = []; + Parallel.For(0, 50, _ => + { + results.Add(cache.GetKeys()); + }); + + // Assert – every call returned a non-empty list; no exceptions thrown + Assert.All(results, r => Assert.NotEmpty(r)); + } + + [Fact] + public async Task GetKeys_WarmCache_DoesNotBlockCallingThread() + { + // Arrange + RsaSecurityKey key = new(System.Security.Cryptography.RSA.Create(2048)); + ConfigurationManager manager = BuildManager(key); + JwksKeyCache cache = new(manager); + + // Let the background task finish so the cache is warm + await Task.Delay(300); + + // Act – on a warm cache GetKeys should complete quickly (no network I/O) + System.Diagnostics.Stopwatch sw = System.Diagnostics.Stopwatch.StartNew(); + IEnumerable keys = cache.GetKeys(); + sw.Stop(); + + // Assert – served from volatile cache, well under 100 ms + Assert.NotEmpty(keys); + Assert.True(sw.ElapsedMilliseconds < 100, $"Warm-cache GetKeys took {sw.ElapsedMilliseconds} ms (expected < 100 ms)"); + } +} diff --git a/core/test/Microsoft.Teams.Bot.Core.UnitTests/Http/BotHttpClientTests.cs b/core/test/Microsoft.Teams.Bot.Core.UnitTests/Http/BotHttpClientTests.cs new file mode 100644 index 00000000..916fc52b --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Core.UnitTests/Http/BotHttpClientTests.cs @@ -0,0 +1,77 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Net; +using System.Text; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Teams.Bot.Core.Http; +using Moq; +using Moq.Protected; + +namespace Microsoft.Teams.Bot.Core.UnitTests.Http; + +public class BotHttpClientTests +{ + private static BotHttpClient BuildClient(string responseBody, HttpStatusCode statusCode = HttpStatusCode.OK) + { + Mock handler = new(); + handler + .Protected() + .Setup>("SendAsync", + ItExpr.IsAny(), + ItExpr.IsAny()) + .ReturnsAsync(new HttpResponseMessage(statusCode) + { + Content = new StringContent(responseBody, Encoding.UTF8, "application/json") + }); + + return new BotHttpClient(new HttpClient(handler.Object), NullLogger.Instance); + } + + [Fact] + public async Task SendAsync_WhenResponseIsJsonString_ReturnsUnquotedString() + { + // Body is a JSON-quoted string: "hello world" + BotHttpClient client = BuildClient("\"hello world\""); + + string? result = await client.SendAsync(HttpMethod.Get, "https://example.com/"); + + Assert.Equal("hello world", result); + } + + [Fact] + public async Task SendAsync_WhenResponseIsPlainText_ReturnsRawText() + { + // Body is plain text (not JSON), e.g. some legacy endpoint + BotHttpClient client = BuildClient("plain text response"); + + string? result = await client.SendAsync(HttpMethod.Get, "https://example.com/"); + + Assert.Equal("plain text response", result); + } + + [Fact] + public async Task SendAsync_WhenResponseIsJsonObject_DeserializesCorrectly() + { + // Normal JSON object deserialization must still work + BotHttpClient client = BuildClient("""{"id":"act-123"}"""); + + TestResponse? result = await client.SendAsync(HttpMethod.Get, "https://example.com/"); + + Assert.NotNull(result); + Assert.Equal("act-123", result.Id); + } + + [Fact] + public async Task SendAsync_WhenResponseBodyIsEmpty_ReturnsNull() + { + // Empty body (e.g. 204 No Content with empty string) + BotHttpClient client = BuildClient(string.Empty); + + string? result = await client.SendAsync(HttpMethod.Get, "https://example.com/"); + + Assert.Null(result); + } + + private sealed record TestResponse(string Id); +} diff --git a/core/test/Microsoft.Teams.Bot.Core.UnitTests/TurnMiddlewareFreezeTests.cs b/core/test/Microsoft.Teams.Bot.Core.UnitTests/TurnMiddlewareFreezeTests.cs new file mode 100644 index 00000000..e6619f14 --- /dev/null +++ b/core/test/Microsoft.Teams.Bot.Core.UnitTests/TurnMiddlewareFreezeTests.cs @@ -0,0 +1,80 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Teams.Bot.Core.Schema; +using Moq; + +namespace Microsoft.Teams.Bot.Core.UnitTests; + +/// +/// Verifies that TurnMiddleware.Freeze() prevents post-startup middleware registration (A-020). +/// +public class TurnMiddlewareFreezeTests +{ + private static BotApplication CreateBotApplication() + { + Mock mockHttp = new(); + ConversationClient cc = new(mockHttp.Object); + Mock mockCfg = new(); + UserTokenClient utc = new(mockHttp.Object, mockCfg.Object, NullLogger.Instance); + return new BotApplication(cc, utc, NullLogger.Instance); + } + + [Fact] + public void Freeze_ThenUseMiddleware_ThrowsInvalidOperationException() + { + // Arrange + BotApplication botApp = CreateBotApplication(); + botApp.UseMiddleware(new NoopMiddleware()); // allowed pre-freeze + + // Act – freeze (simulates IHostedService.StartAsync) + botApp.FreezeMiddleware(); + + // Assert – further registration must throw + Assert.Throws(() => + botApp.UseMiddleware(new NoopMiddleware())); + } + + [Fact] + public async Task Freeze_MiddlewareStillExecutes_AfterFreeze() + { + // Arrange + BotApplication botApp = CreateBotApplication(); + bool middlewareCalled = false; + + botApp.UseMiddleware(new LambdaMiddleware(async (_, _, next, ct) => + { + middlewareCalled = true; + await next(ct); + })); + + botApp.FreezeMiddleware(); + + // Act – manually invoke the pipeline + botApp.OnActivity = (_, _) => Task.CompletedTask; + + DefaultHttpContext ctx = new(); + ctx.Request.Body = new System.IO.MemoryStream( + System.Text.Encoding.UTF8.GetBytes("{\"type\":\"message\"}")); + + await botApp.ProcessAsync(ctx, CancellationToken.None); + + // Assert + Assert.True(middlewareCalled); + } + + private sealed class NoopMiddleware : ITurnMiddleware + { + public Task OnTurnAsync(BotApplication app, CoreActivity activity, NextTurn next, CancellationToken ct) + => next(ct); + } + + private sealed class LambdaMiddleware(Func impl) : ITurnMiddleware + { + public Task OnTurnAsync(BotApplication app, CoreActivity activity, NextTurn next, CancellationToken ct) + => impl(app, activity, next, ct); + } +}