From f1a4d1e731e28f04f0ce75892f42778097b3e655 Mon Sep 17 00:00:00 2001 From: Corina <14900841+corinagum@users.noreply.github.com> Date: Wed, 26 Aug 2026 10:49:26 -0700 Subject: [PATCH 1/4] test: cover file handling as shipped, and fix the .runsettings gitignore path Adds unit and integration coverage for the inbound file handling shipped in PR A, and fixes a .gitignore rule that let integration test secrets be committed. The gitignore rule covered only the pre-migration core/test/IntegrationTests/.runsettings/ path, while test/IntegrationTests/README.md tells contributors to place secrets in test/IntegrationTests/.runsettings/ and describes it as ignored. Secrets written to the documented location were tracked. File handling previously had no presence in the integration suite. FilesIntegrationTests covers DI resolution, a real streamed fetch, a live 401 mapping onto FileUrlExpiredException, and the scope and https gates. FilesAccessorWireShapeTests maps the shape Teams actually sends, which no existing test used. FileDownloaderTransportTests asserts no Authorization header reaches the storage host, which teams.ts already covered and C# did not. No product changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf9d8c98-4629-4ddc-8b36-2c6a82057f94 --- .gitignore | 1 + test/IntegrationTests/CompatTeamsInfoTests.cs | 8 +- .../ConversationClientTests.cs | 2 +- .../IntegrationTests/FilesIntegrationTests.cs | 127 ++++++++++++++++ .../Files/FileDownloaderTransportTests.cs | 131 ++++++++++++++++ .../Files/FilesAccessorWireShapeTests.cs | 140 ++++++++++++++++++ 6 files changed, 404 insertions(+), 5 deletions(-) create mode 100644 test/IntegrationTests/FilesIntegrationTests.cs create mode 100644 test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs create mode 100644 test/Microsoft.Teams.Apps.UnitTests/Files/FilesAccessorWireShapeTests.cs diff --git a/.gitignore b/.gitignore index 18a766024..f5fbb39ff 100644 --- a/.gitignore +++ b/.gitignore @@ -494,4 +494,5 @@ $RECYCLE.BIN/ .claude/settings.local.json # Integration test run settings (contain secrets) +test/IntegrationTests/.runsettings/ core/test/IntegrationTests/.runsettings/ diff --git a/test/IntegrationTests/CompatTeamsInfoTests.cs b/test/IntegrationTests/CompatTeamsInfoTests.cs index d60237902..1d5004d3f 100644 --- a/test/IntegrationTests/CompatTeamsInfoTests.cs +++ b/test/IntegrationTests/CompatTeamsInfoTests.cs @@ -132,7 +132,7 @@ public async Task GetMembersAsync_ReturnsTeamsChannelAccounts() List members = [.. result]; Assert.NotEmpty(members); - foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in members) + foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in members.Take(5)) { _output.WriteLine($"GetMembers: {m.Id} — {m.Name}"); } @@ -152,7 +152,7 @@ public async Task GetPagedMembersAsync_ReturnsPaged() Assert.NotNull(result.Members); Assert.NotEmpty(result.Members); - foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in result.Members) + foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in result.Members.Take(5)) { _output.WriteLine($"PagedMember: {m.Id} — {m.Name}"); } @@ -203,7 +203,7 @@ public async Task GetTeamMembersAsync_ReturnsMembers() List members = [.. result]; Assert.NotEmpty(members); - foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in members) + foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in members.Take(5)) { _output.WriteLine($"TeamMember: {m.Id} — {m.Name}"); } @@ -223,7 +223,7 @@ public async Task GetPagedTeamMembersAsync_ReturnsPaged() Assert.NotNull(result.Members); Assert.NotEmpty(result.Members); - foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in result.Members) + foreach (Microsoft.Bot.Schema.Teams.TeamsChannelAccount m in result.Members.Take(5)) { _output.WriteLine($"PagedTeamMember: {m.Id} — {m.Name}"); } diff --git a/test/IntegrationTests/ConversationClientTests.cs b/test/IntegrationTests/ConversationClientTests.cs index 4a3557981..8e26de26b 100644 --- a/test/IntegrationTests/ConversationClientTests.cs +++ b/test/IntegrationTests/ConversationClientTests.cs @@ -87,7 +87,7 @@ public async Task GetConversationMembers() Assert.NotNull(members); Assert.NotEmpty(members); - foreach (ChannelAccount m in members) + foreach (ChannelAccount m in members.Take(5)) { _output.WriteLine($"Member: {m.Id} — {m.Name}"); } diff --git a/test/IntegrationTests/FilesIntegrationTests.cs b/test/IntegrationTests/FilesIntegrationTests.cs new file mode 100644 index 000000000..e700baf80 --- /dev/null +++ b/test/IntegrationTests/FilesIntegrationTests.cs @@ -0,0 +1,127 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Teams.Apps.Files; +using Microsoft.Teams.Apps.Schema; +using Xunit.Abstractions; + +namespace IntegrationTests; + +/// +/// Integration tests for inbound file handling as shipped today (PR A), covering the two things unit tests +/// structurally cannot reach: that is actually wired into the DI container the app +/// builds, and that its byte-fetch contract holds against a real Microsoft endpoint over a real network. +/// What these do and do not prove. High fidelity for the contract the downloader depends on: +/// an https URL fetched with a plain unauthenticated GET, streamed with ResponseHeadersRead, returning bytes +/// or a clean 401. Low fidelity for provenance: these URLs are not Teams tempauth URLs, because no +/// automated route mints one. **These must never be described as end-to-end file receive.** Manual e2e remains the +/// only evidence for that. +/// Graph is used purely as a conveniently stable first-party HTTP endpoint. No token is sent and no Graph +/// application permission is required, so these run wherever the rest of the suite runs. +/// +public class FilesIntegrationTests : IClassFixture +{ + private readonly IntegrationTestFixture _f; + private readonly ITestOutputHelper _output; + + public FilesIntegrationTests(IntegrationTestFixture fixture, ITestOutputHelper output) + { + _f = fixture; + _f.OutputHelper = output; + _output = output; + } + + // Public, unauthenticated, returns real bytes with a real content-type. Stands in for the "URL that serves the + // file" half of the contract. + private static readonly Uri PublicBytesUrl = new("https://graph.microsoft.com/v1.0/$metadata"); + + // Requires auth, so an unauthenticated GET is a genuine 401 from a live Microsoft service. Stands in for an + // expired `tempauth` URL. + private static readonly Uri UnauthorizedUrl = new("https://graph.microsoft.com/v1.0/me"); + + private FileDownloader Downloader => _f.ServiceProvider.GetRequiredService(); + + // No Timeout: this one performs no I/O, and xUnit only supports Timeout on async tests. + [Fact] + [Trait("Category", "Files")] + public void FileDownloader_ResolvesFromTheContainerTheAppBuilds() + { + // Catches removal of `AddHttpClient()`. TeamsBotApplication falls back to + // FileDownloader.CreateDefault() silently and with no log, so a broken registration degrades to a + // process-wide static HttpClient with no signal and every unit test still passing. + FileDownloader downloader = _f.ServiceProvider.GetRequiredService(); + + Assert.NotNull(downloader); + + // Typed clients are transient, so two resolutions are distinct instances. Asserting that rather than + // singleton-ness documents the registration's actual lifetime instead of guessing at it. + Assert.NotSame(downloader, _f.ServiceProvider.GetRequiredService()); + _output.WriteLine("FileDownloader resolved from DI container"); + } + + [Fact(Timeout = 15000)] + [Trait("Category", "Files")] + public async Task OpenFileStream_FetchesRealBytesOverTheRealNetwork() + { + await using OpenedFileStream stream = await Downloader.OpenFileStreamAsync( + ConversationType.Personal, PublicBytesUrl, contentType: null, priorFetchSucceeded: false, CancellationToken.None); + + // Reading only a prefix is deliberate: it proves HttpCompletionOption.ResponseHeadersRead really streams + // rather than buffering the whole body, which a fake handler cannot demonstrate. + byte[] head = new byte[64]; + int read = await stream.ReadAsync(head, CancellationToken.None); + + Assert.True(read > 0); + Assert.Equal(PublicBytesUrl, stream.SourceUrl); + + // Resolved from the live response, not from the fallback. Real header casing and parameter handling. + Assert.StartsWith("application/xml", stream.ContentType, StringComparison.OrdinalIgnoreCase); + _output.WriteLine($"Fetched {read} bytes, contentType={stream.ContentType}"); + } + + [Fact(Timeout = 15000)] + [Trait("Category", "Files")] + public async Task OpenFileStream_MapsARealUnauthorizedResponseOntoFileUrlExpired() + { + // The failure mode this exists for: if the platform ever answered an expired URL with a redirect to a login + // page, or 200 plus an HTML login body instead of 401, every mocked test would still pass and the downloader + // would hand callers HTML as if it were the file. Only a live call can tell the difference. + FileUrlExpiredException ex = await Assert.ThrowsAsync( + () => Downloader.OpenFileStreamAsync( + ConversationType.Personal, UnauthorizedUrl, contentType: null, priorFetchSucceeded: false, CancellationToken.None)); + + Assert.Equal(FileUrlExpiredReason.FirstFetch, ex.Reason); + _output.WriteLine($"Live 401 mapped to FileUrlExpiredException: {ex.Reason}"); + } + + [Fact(Timeout = 15000)] + [Trait("Category", "Files")] + public async Task OpenFileStream_ReportsRereadWhenAPriorFetchSucceeded() + { + // Same live 401, opposite reason. Pins that the caller-supplied re-fetch state, not the response, chooses + // between FirstFetch and Reread. + FileUrlExpiredException ex = await Assert.ThrowsAsync( + () => Downloader.OpenFileStreamAsync( + ConversationType.Personal, UnauthorizedUrl, contentType: null, priorFetchSucceeded: true, CancellationToken.None)); + + Assert.Equal(FileUrlExpiredReason.Reread, ex.Reason); + } + + [Fact(Timeout = 15000)] + [Trait("Category", "Files")] + public async Task OpenFileStream_RejectsUnsupportedScopesAndNonHttpsUrls_BeforeAnyNetworkCall() + { + // Non-personal scopes have no receive path yet; this pins the shipped behaviour so PR J has to change it + // deliberately rather than by accident. + FileScopeNotSupportedException scopeEx = await Assert.ThrowsAsync( + () => Downloader.OpenFileStreamAsync( + ConversationType.GroupChat, PublicBytesUrl, contentType: null, priorFetchSucceeded: false, CancellationToken.None)); + Assert.Equal(ConversationType.GroupChat, scopeEx.Scope); + + InvalidOperationException httpEx = await Assert.ThrowsAsync( + () => Downloader.OpenFileStreamAsync( + ConversationType.Personal, new Uri("http://graph.microsoft.com/v1.0/$metadata"), contentType: null, priorFetchSucceeded: false, CancellationToken.None)); + Assert.Contains("must use https", httpEx.Message, StringComparison.Ordinal); + } +} diff --git a/test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs b/test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs new file mode 100644 index 000000000..48ec692f3 --- /dev/null +++ b/test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs @@ -0,0 +1,131 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Net; +using System.Net.Http.Headers; +using System.Text; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Teams.Apps.Files; +using Microsoft.Teams.Apps.Schema; + +namespace Microsoft.Teams.Apps.UnitTests.Files; + +/// +/// Transport invariants for the file download request, as shipped today. A download URL embeds its own +/// tempauth credential and points at a third-party storage host, so the request must not carry bot +/// credentials: attaching one can get the request rejected and sends the credential to a host with no business +/// seeing it. +/// Where the invariant actually lives. does not strip anything. It issues a +/// plain GET and inherits whatever its carries, because merges +/// DefaultRequestHeaders into every request. The guarantee therefore rests entirely on the registration: +/// AddHttpClient<FileDownloader>() is deliberately bare, and the AddBotHttpClient call above it, +/// which is what attaches bot authentication, is deliberately not applied to it. +/// So these tests assert the invariant at the level that holds it, the DI container, rather than at a level +/// that would require changing the shipped downloader. teams.ts asserts the equivalent in +/// download.http-client.spec.ts; C# had no equivalent at any level. +/// This matters more once PR J lands, since it adds a Graph path on the same downloader that does +/// authenticate. Once one path attaches a credential and the other must not, "nobody has configured one" stops being +/// a safe place to leave the guarantee. +/// +public class FileDownloaderTransportTests +{ + private const string DownloadUrl = "https://download.example/notes.txt?tempauth=abc"; + + /// Records the outbound request and answers with a canned body, so nothing reaches the network. + private sealed class RecordingHandler : HttpMessageHandler + { + public List Requests { get; } = []; + + public HttpRequestMessage Last => Assert.Single(Requests); + + protected override Task SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) + { + Requests.Add(request); + return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK) + { + Content = new ByteArrayContent(Encoding.UTF8.GetBytes("bytes")), + }); + } + } + + private static IncomingFile PersonalFile(FileDownloader downloader) + => new("notes.txt", ConversationType.Personal, FileSource.BotActivity, downloader) + { + DownloadUrl = new Uri(DownloadUrl), + }; + + /// + /// Builds the container the hosting extensions build, then swaps only the downloader's primary handler for a + /// recorder. Every other piece of the registration, including any handler or default header it configures, is + /// left exactly as shipped, so what the recorder sees is what a real download would send. + /// + private static ServiceProvider BuildAppContainer(RecordingHandler handler, Action? extraConfig = null) + { + ServiceCollection services = new(); + services.AddSingleton(new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["AzureAd:ClientId"] = "files-transport-client-id", + ["AzureAd:TenantId"] = "files-transport-tenant-id", + }) + .Build()); + services.AddLogging(); + services.AddTeamsBotApplication(); + + IHttpClientBuilder builder = services.AddHttpClient() + .ConfigurePrimaryHttpMessageHandler(() => handler); + extraConfig?.Invoke(builder); + + return services.BuildServiceProvider(); + } + + [Fact] + public async Task DiRegisteredDownloader_SendsNoAuthorizationHeader() + { + RecordingHandler handler = new(); + using ServiceProvider provider = BuildAppContainer(handler); + + await PersonalFile(provider.GetRequiredService()).DownloadAsync(); + + // Goes red if anyone attaches bot auth to this registration, adds a credential default header, or applies a + // global auth handler across AddHttpClient registrations. That is the regression worth catching: the code + // comment at the registration site asks for this, and nothing else enforces it. + Assert.False(handler.Last.Headers.Contains("Authorization")); + Assert.Null(handler.Last.Headers.Authorization); + } + + [Fact] + public async Task DiRegisteredDownloader_SendsAPlainGetToTheDownloadUrl() + { + RecordingHandler handler = new(); + using ServiceProvider provider = BuildAppContainer(handler); + + await PersonalFile(provider.GetRequiredService()).DownloadAsync(); + + Assert.Equal(HttpMethod.Get, handler.Last.Method); + Assert.Equal(new Uri(DownloadUrl), handler.Last.RequestUri); + Assert.Null(handler.Last.Content); + } + + /// + /// Characterizes the shipped behaviour that makes the test above the one that matters: the downloader has no + /// defence of its own. This is documentation of a constraint, not an endorsement of it. If C# ever grows an + /// explicit strip, as teams.ts has, this test is the one that should change. + /// + [Fact] + public async Task Download_ForwardsClientDefaults_SoTheTypedClientMustStayCredentialFree() + { + RecordingHandler handler = new(); + HttpClient credentialed = new(handler); + credentialed.DefaultRequestHeaders.Authorization = new AuthenticationHeaderValue("Bearer", "token"); + credentialed.DefaultRequestHeaders.UserAgent.ParseAdd("teams.net-test/1.0"); + + await PersonalFile(new FileDownloader(credentialed)).DownloadAsync(); + + // HttpClient merges DefaultRequestHeaders into every request and offers no per-request opt-out, so a + // credential on the client reaches the storage host. Hence the guarantee has to be held at registration. + Assert.True(handler.Last.Headers.Contains("Authorization")); + Assert.Equal(["teams.net-test/1.0"], handler.Last.Headers.GetValues("User-Agent")); + } +} diff --git a/test/Microsoft.Teams.Apps.UnitTests/Files/FilesAccessorWireShapeTests.cs b/test/Microsoft.Teams.Apps.UnitTests/Files/FilesAccessorWireShapeTests.cs new file mode 100644 index 000000000..3e5296353 --- /dev/null +++ b/test/Microsoft.Teams.Apps.UnitTests/Files/FilesAccessorWireShapeTests.cs @@ -0,0 +1,140 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +using System.Text.Json; +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Teams.Apps.Files; +using Microsoft.Teams.Apps.Schema; +using Microsoft.Teams.Core.Schema; + +namespace Microsoft.Teams.Apps.UnitTests.Files; + +/// +/// Mapping tests against the shape Teams actually puts on the wire for a personal-scope file upload, as opposed to +/// the shapes invents. The distinguishing features of the real payload, none of +/// which the hand-written tests exercise, are that it carries two attachments (the file plus an empty +/// text/html sibling), that content has exactly three keys with no etag despite +/// modelling one, and that the activity has no text property at all +/// when a file is attached with no message. +/// The fixture is constructed by allowlist rather than redacted from a capture: it contains only the seven +/// fields the mapper reads, with synthetic values throughout. A real capture carries identity in at least three +/// places that a denylist misses (the download URL's query token, the user's UPN inside both URL paths, and +/// the activity's from/recipient/conversation/channelData blocks), so an allowlist is +/// what fails closed. Do not add fields here that the mapper does not read. +/// +public class FilesAccessorWireShapeTests +{ + private static readonly NullLogger Log = NullLogger.Instance; + + // These tests only exercise attachment mapping, never a download, so the client is never used. + private static readonly FileDownloader Downloader = new(new HttpClient()); + + private const string ExpectedName = "quarterly report.pdf"; + private const string ExpectedUniqueId = "00000000-0000-4000-8000-00000000f11e"; + + // Percent-encoded spaces, exactly as Teams sends them in the browsable OneDrive path. + private const string ExpectedWebUrl = + "https://example.sharepoint.com/personal/synthetic_user_example_com/Documents/Microsoft%20Teams%20Chat%20Files/quarterly%20report.pdf"; + + private const string ExpectedDownloadUrl = + "https://example.sharepoint.com/personal/synthetic_user_example_com/_layouts/15/download.aspx?UniqueId=00000000-0000-4000-8000-00000000f11e&Translate=false&tempauth=synthetic-not-a-real-token&ApiVersion=2.1"; + + /// + /// The real wire shape with synthetic values. Deliberately minimal: `type` plus the seven allowlisted fields. + /// Every omission is load-bearing, so read the assertions before adding anything. + /// + private const string CapturedShapeJson = $$""" + { + "type": "message", + "conversation": { + "conversationType": "personal" + }, + "attachments": [ + { + "contentType": "application/vnd.microsoft.teams.file.download.info", + "contentUrl": "{{ExpectedWebUrl}}", + "name": "{{ExpectedName}}", + "content": { + "downloadUrl": "{{ExpectedDownloadUrl}}", + "uniqueId": "{{ExpectedUniqueId}}", + "fileType": "pdf" + } + }, + { + "contentType": "text/html", + "content": "" + } + ] + } + """; + + // Hydrates the fixture the way a real receive does: raw wire JSON through the inbound deserializer, then + // FromActivity. Nothing here hand-builds a TeamsAttachment, so a regression in either step fails these tests. + private static MessageActivity CapturedActivity() + => MessageActivity.FromActivity(CoreActivity.FromJsonString(CapturedShapeJson)); + + [Fact] + public async Task RealWireShape_YieldsExactlyOneFile_FromATwoAttachmentActivity() + { + MessageActivity activity = CapturedActivity(); + + // Positive control: assert the sibling really arrived, so "one file" below means the mapper skipped it + // rather than the fixture never having carried it. + Assert.Equal(2, activity.Attachments!.Count); + + IList files = await new FilesAccessor(activity, Log, Downloader).ListAsync(); + + IncomingFile file = Assert.Single(files); + Assert.Equal(ExpectedName, file.Name); + Assert.Equal("pdf", file.Extension); + Assert.Equal(ExpectedUniqueId, file.UniqueId); + Assert.Equal(new Uri(ExpectedWebUrl), file.WebUrl); + Assert.Equal(new Uri(ExpectedDownloadUrl), file.DownloadUrl); + Assert.Equal(ConversationType.Personal, file.Scope); + Assert.Equal(FileSource.BotActivity, file.Source); + + // `TeamsAttachment.ContentUrl` is a `Uri`, so the wire value is normalized before anything reads it. Teams + // percent-encodes the spaces in the OneDrive path; pin that they survive the round-trip, since a URL that + // silently decoded them would no longer address the item. + Assert.Contains("Microsoft%20Teams%20Chat%20Files", file.WebUrl!.AbsoluteUri, StringComparison.Ordinal); + Assert.DoesNotContain(' ', file.WebUrl.AbsoluteUri); + } + + [Fact] + public async Task RealWireShape_DoesNotLeakTheEmptyHtmlSibling() + { + MessageActivity activity = CapturedActivity(); + TeamsAttachment htmlSibling = activity.Attachments![1]; + + // The sibling is the thing the content-type guard has to reject: an attachment whose content is an empty + // string rather than an object, so it would not survive coercion into FileDownloadInfo either. + Assert.Equal(new AttachmentContentType("text/html"), htmlSibling.ContentType); + + IncomingFile file = Assert.Single(await new FilesAccessor(activity, Log, Downloader).ListAsync()); + + // Raw identifies which attachment produced the file, so this pins the mapping to index 0 and proves the + // sibling was skipped rather than mapped into some second, malformed handle. + Assert.Same(activity.Attachments[0], file.Raw); + Assert.NotSame(htmlSibling, file.Raw); + } + + [Fact] + public async Task RealWireShape_MapsWithNoEtagAndNoTextProperty() + { + using JsonDocument fixture = JsonDocument.Parse(CapturedShapeJson); + JsonElement content = fixture.RootElement.GetProperty("attachments")[0].GetProperty("content"); + + // Guards the fixture itself: Teams sends exactly downloadUrl, uniqueId and fileType on this path, so the + // mapper is only ever proven against the absent-etag shape if the fixture keeps it absent. + Assert.Equal(3, content.EnumerateObject().Count()); + Assert.False(content.TryGetProperty("etag", out _)); + Assert.False(fixture.RootElement.TryGetProperty("text", out _)); + + MessageActivity activity = CapturedActivity(); + Assert.Null(activity.Text); + + // The payload still maps, which is what would break if Etag were ever made required or if a missing `text` + // started tripping activity hydration. + Assert.Single(await new FilesAccessor(activity, Log, Downloader).ListAsync()); + } +} From 122bf9e96954b47436b8f04eb1f37c47b237ec67 Mon Sep 17 00:00:00 2001 From: Corina <14900841+corinagum@users.noreply.github.com> Date: Wed, 26 Aug 2026 11:15:36 -0700 Subject: [PATCH 2/4] test: assert against the shipped FileDownloader registration, not a re-registered one BuildAppContainer called AddHttpClient() itself, which registered the downloader independently of the hosting extensions. The transport tests therefore kept passing even when AddTeamsBotApplication stopped registering FileDownloader at all, which defeats the purpose of tests whose stated job is guarding that registration. Override the primary handler on the existing typed client's HttpClientFactoryOptions instead, so resolution fails outright if the production registration goes away. Verified by mutation: removing the registration from the hosting extensions now fails these tests with "No service for type FileDownloader has been registered", where previously they passed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf9d8c98-4629-4ddc-8b36-2c6a82057f94 --- .../Files/FileDownloaderTransportTests.cs | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs b/test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs index 48ec692f3..cef9fed68 100644 --- a/test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs +++ b/test/Microsoft.Teams.Apps.UnitTests/Files/FileDownloaderTransportTests.cs @@ -6,6 +6,7 @@ using System.Text; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Http; using Microsoft.Teams.Apps.Files; using Microsoft.Teams.Apps.Schema; @@ -56,11 +57,16 @@ private static IncomingFile PersonalFile(FileDownloader downloader) }; /// - /// Builds the container the hosting extensions build, then swaps only the downloader's primary handler for a - /// recorder. Every other piece of the registration, including any handler or default header it configures, is - /// left exactly as shipped, so what the recorder sees is what a real download would send. + /// Builds the container the hosting extensions build, then overrides only the primary handler of the typed + /// client they already registered. Deliberately does not call AddHttpClient<FileDownloader>() + /// itself: doing so would register the downloader independently, so these tests would keep passing even if the + /// hosting extensions stopped registering it at all. Reaching into the existing + /// instead means resolution fails outright if the production registration + /// goes away, which is the regression these tests exist to catch. + /// Every other piece of the shipped registration, including any handler or default header it configures, + /// is left intact, so what the recorder sees is what a real download would send. /// - private static ServiceProvider BuildAppContainer(RecordingHandler handler, Action? extraConfig = null) + private static ServiceProvider BuildAppContainer(RecordingHandler handler, Action? extraConfig = null) { ServiceCollection services = new(); services.AddSingleton(new ConfigurationBuilder() @@ -73,9 +79,12 @@ private static ServiceProvider BuildAppContainer(RecordingHandler handler, Actio services.AddLogging(); services.AddTeamsBotApplication(); - IHttpClientBuilder builder = services.AddHttpClient() - .ConfigurePrimaryHttpMessageHandler(() => handler); - extraConfig?.Invoke(builder); + // The name AddHttpClient() registers its options under. + services.Configure(nameof(FileDownloader), options => + { + options.HttpMessageHandlerBuilderActions.Add(b => b.PrimaryHandler = handler); + extraConfig?.Invoke(options); + }); return services.BuildServiceProvider(); } From 4d5e5bbe8943c525c8deff855a798fc0c8760b2a Mon Sep 17 00:00:00 2001 From: Corina <14900841+corinagum@users.noreply.github.com> Date: Wed, 26 Aug 2026 15:29:23 -0700 Subject: [PATCH 3/4] docs: tell contributors to verify the .runsettings ignore rule before writing secrets The README described the .runsettings directory as gitignored, which was the claim that made the gap invisible: the ignore rule tracked only the pre-migration core/test path, so secrets written to the documented location were tracked. Point readers at git check-ignore rather than asking them to trust the sentence, since older branches still carry the stale rule. Also warn against diagnostic verbosity on this suite, which can echo environment variables while these files hold a live client secret. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf9d8c98-4629-4ddc-8b36-2c6a82057f94 --- test/IntegrationTests/README.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/IntegrationTests/README.md b/test/IntegrationTests/README.md index 965938ae8..69e7aa5ab 100644 --- a/test/IntegrationTests/README.md +++ b/test/IntegrationTests/README.md @@ -25,6 +25,18 @@ Tests are configured via `.runsettings` files that set environment variables. Fo Place your `.runsettings` files in the `.runsettings/` directory (gitignored). +> Verify that before you write a secret into it. The ignore rule pointed at a +> pre-migration path until [#659](https://github.com/microsoft/teams.net/pull/659), so on +> older branches this directory is **not** ignored despite what this line says: +> +> ```bash +> git check-ignore -v test/IntegrationTests/.runsettings/botid-prod.runsettings +> # no output means NOT ignored — fix .gitignore first +> ``` +> +> Also avoid `dotnet test -v d` on this suite: diagnostic verbosity can echo environment +> variables, and these files carry a live client secret. + ### Required environment variables ```xml From 049f7b73ab08aefa34deb475c685fbb7deaf1a88 Mon Sep 17 00:00:00 2001 From: Corina <14900841+corinagum@users.noreply.github.com> Date: Wed, 26 Aug 2026 15:40:00 -0700 Subject: [PATCH 4/4] docs: drop the unverified -v d warning from the integration test README The claim that diagnostic verbosity echoes environment variables was asserted rather than measured, and it does not reproduce: a `-v d` run of this suite emitted the runsettings client secret zero times and no `AzureAd__*` names, because VSTest applies those values to the test host process rather than the MSBuild command line. Keep the gitignore verification step, which was verified with git check-ignore. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf9d8c98-4629-4ddc-8b36-2c6a82057f94 --- test/IntegrationTests/README.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/test/IntegrationTests/README.md b/test/IntegrationTests/README.md index 69e7aa5ab..4bd8c6a88 100644 --- a/test/IntegrationTests/README.md +++ b/test/IntegrationTests/README.md @@ -33,9 +33,6 @@ Place your `.runsettings` files in the `.runsettings/` directory (gitignored). > git check-ignore -v test/IntegrationTests/.runsettings/botid-prod.runsettings > # no output means NOT ignored — fix .gitignore first > ``` -> -> Also avoid `dotnet test -v d` on this suite: diagnostic verbosity can echo environment -> variables, and these files carry a live client secret. ### Required environment variables