From 12f64166acdffd28170df5dad1c5e7b15176828a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 18 Sep 2026 12:58:38 +0000 Subject: [PATCH 1/3] Initial plan From 814d9ddcfee74e8c56ddca9bf5a71770208fc3d3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:04:41 +0000 Subject: [PATCH 2/3] Honor site-relative OAuth state parameter on install redirect Co-authored-by: johnkors <206726+johnkors@users.noreply.github.com> --- .../Hosting/IAppBuilderExtensions.cs | 3 + .../Hosting/ServiceCollectionExtensions.cs | 6 + .../SlackbotCodeTokenExchangeMiddleware.cs | 44 +++++++- .../DistributionRedirectTests.cs | 103 ++++++++++++++++++ 4 files changed, 155 insertions(+), 1 deletion(-) create mode 100644 source/test/Slackbot.Net.SlackClients.Http.Tests/DistributionRedirectTests.cs diff --git a/source/src/Slackbot.Net.Endpoints/Hosting/IAppBuilderExtensions.cs b/source/src/Slackbot.Net.Endpoints/Hosting/IAppBuilderExtensions.cs index 65738b5..6a6f9a0 100644 --- a/source/src/Slackbot.Net.Endpoints/Hosting/IAppBuilderExtensions.cs +++ b/source/src/Slackbot.Net.Endpoints/Hosting/IAppBuilderExtensions.cs @@ -36,6 +36,9 @@ public static IApplicationBuilder UseSlackbot( /// NB! The path you run this middleware must: /// - match redirect_uri in your 1st redirect to Slack /// - be a valid redirect_uri in your Slack app configuration + /// On a successful install the user is sent to the OAuth `state` parameter when it holds a + /// site-relative path (so an install can return to where it started), and to + /// otherwise. /// /// /// diff --git a/source/src/Slackbot.Net.Endpoints/Hosting/ServiceCollectionExtensions.cs b/source/src/Slackbot.Net.Endpoints/Hosting/ServiceCollectionExtensions.cs index 1e5754c..c5f2882 100644 --- a/source/src/Slackbot.Net.Endpoints/Hosting/ServiceCollectionExtensions.cs +++ b/source/src/Slackbot.Net.Endpoints/Hosting/ServiceCollectionExtensions.cs @@ -38,5 +38,11 @@ public class OAuthOptions { public string CLIENT_ID { get; set; } public string CLIENT_SECRET { get; set; } + + /// + /// Where the user ends up after a successful install. Used as-is unless the install was + /// started with a site-relative path in the OAuth `state` parameter, in which case that path + /// wins - resolved against this uri's origin when it is absolute. + /// public string SuccessRedirectUri { get; set; } = "/success?default=1"; } diff --git a/source/src/Slackbot.Net.Endpoints/Middlewares/SlackbotCodeTokenExchangeMiddleware.cs b/source/src/Slackbot.Net.Endpoints/Middlewares/SlackbotCodeTokenExchangeMiddleware.cs index 8a8a592..535edeb 100644 --- a/source/src/Slackbot.Net.Endpoints/Middlewares/SlackbotCodeTokenExchangeMiddleware.cs +++ b/source/src/Slackbot.Net.Endpoints/Middlewares/SlackbotCodeTokenExchangeMiddleware.cs @@ -39,7 +39,8 @@ await installationHandler.Install(new Workspace response.Access_Token )); - ctx.Response.Redirect(options.Value.SuccessRedirectUri); + var state = ctx.Request.Query["state"].FirstOrDefault(); + ctx.Response.Redirect(ResolveRedirectUri(state, options.Value.SuccessRedirectUri)); } else { @@ -48,4 +49,45 @@ await installationHandler.Install(new Workspace await ctx.Response.WriteAsync(response.Error); } } + + /// + /// Slack round-trips the OAuth `state` parameter untouched, so an app can use it to carry + /// the page the install was started from. Only site-relative paths are honored, and they are + /// resolved against 's origin so the user stays + /// on the same site. Anything else falls back to . + /// + private static string ResolveRedirectUri(string state, string successRedirectUri) + { + if (!IsSiteRelative(state)) + { + return successRedirectUri; + } + + if (Uri.TryCreate(successRedirectUri, UriKind.Absolute, out var successUri) && + (successUri.Scheme == Uri.UriSchemeHttp || successUri.Scheme == Uri.UriSchemeHttps)) + { + return new Uri(successUri, state).AbsoluteUri; + } + + return state; + } + + /// + /// Accepts `/foo` but not absolute (`https://host/foo`) or protocol-relative (`//host`, + /// `/\host`) values, which would turn the callback into an open redirect. + /// + private static bool IsSiteRelative(string state) + { + if (string.IsNullOrEmpty(state) || state[0] != '/') + { + return false; + } + + if (state.Length > 1 && (state[1] == '/' || state[1] == '\\')) + { + return false; + } + + return !state.Any(char.IsControl); + } } diff --git a/source/test/Slackbot.Net.SlackClients.Http.Tests/DistributionRedirectTests.cs b/source/test/Slackbot.Net.SlackClients.Http.Tests/DistributionRedirectTests.cs new file mode 100644 index 0000000..fbef81d --- /dev/null +++ b/source/test/Slackbot.Net.SlackClients.Http.Tests/DistributionRedirectTests.cs @@ -0,0 +1,103 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.DependencyInjection; +using Slackbot.Net.Abstractions.Hosting; +using Slackbot.Net.Endpoints.Hosting; +using Slackbot.Net.Tests.Helpers; + +namespace Slackbot.Net.Tests; + +public class DistributionRedirectTests +{ + private const string OauthAccessResponse = + """{"ok":true,"access_token":"xoxb-token","scope":"chat:write","team":{"id":"T1","name":"Team"},"app_id":"A1"}"""; + + [Fact] + public async Task InstallsWorkspaceAndRedirectsToSuccessRedirectUri() + { + var (ctx, handler) = await Install(null, "/success?default=1"); + + var workspace = Assert.Single(handler.Installed); + Assert.Equal("T1", workspace.TeamId); + Assert.Equal("Team", workspace.TeamName); + Assert.Equal("xoxb-token", workspace.Token); + Assert.Equal(StatusCodes.Status302Found, ctx.Response.StatusCode); + Assert.Equal("/success?default=1", ctx.Response.Headers.Location.ToString()); + } + + [Theory] + // A site-relative state keeps the origin of an absolute SuccessRedirectUri + [InlineData("/admin/slack?installed=1", "https://example.com/success?default=1", + "https://example.com/admin/slack?installed=1")] + [InlineData("/", "https://example.com/success", "https://example.com/")] + // ... and is used as-is when SuccessRedirectUri is relative too + [InlineData("/admin/slack", "/success?default=1", "/admin/slack")] + // Anything that could leave the site is ignored + [InlineData("https://evil.example/steal", "https://example.com/success", "https://example.com/success")] + [InlineData("//evil.example", "https://example.com/success", "https://example.com/success")] + [InlineData("/\\evil.example", "https://example.com/success", "https://example.com/success")] + [InlineData("admin/slack", "https://example.com/success", "https://example.com/success")] + [InlineData("", "https://example.com/success", "https://example.com/success")] + public async Task RedirectsToStateWhenItIsSiteRelative(string state, string successRedirectUri, + string expectedLocation) + { + var (ctx, _) = await Install(state, successRedirectUri); + + Assert.Equal(expectedLocation, ctx.Response.Headers.Location.ToString()); + } + + private static async Task<(HttpContext Context, RecordingInstallationHandler Handler)> Install(string state, + string successRedirectUri) + { + var services = new ServiceCollection(); + services.AddLogging(); + services.AddSlackBotEvents(); + services.AddSlackbotDistribution(o => + { + o.CLIENT_ID = "client-id"; + o.CLIENT_SECRET = "client-secret"; + o.SuccessRedirectUri = successRedirectUri; + }); + // Serves a canned oauth.v2.access response, so no call goes to Slack + services.ConfigureHttpClientDefaults(b => + b.ConfigurePrimaryHttpMessageHandler(() => new StubHttpMessageHandler(OauthAccessResponse))); + + await using var provider = services.BuildServiceProvider(new ServiceProviderOptions { ValidateScopes = true }); + await using var scope = provider.CreateAsyncScope(); + + var pipeline = new ApplicationBuilder(provider).UseSlackbotDistribution().Build(); + + var query = QueryString.Create("code", "the-code"); + if (state != null) + { + query = query.Add(QueryString.Create("state", state)); + } + + var ctx = new DefaultHttpContext { RequestServices = scope.ServiceProvider }; + ctx.Request.Scheme = "https"; + ctx.Request.Host = new HostString("example.com"); + ctx.Request.QueryString = query; + + await pipeline(ctx); + + var handler = (RecordingInstallationHandler)scope.ServiceProvider + .GetRequiredService(); + return (ctx, handler); + } + + private sealed class RecordingInstallationHandler : IWorkspaceInstallationHandler + { + public List Installed { get; } = []; + + public Task Install(Workspace workspace) + { + Installed.Add(workspace); + return Task.CompletedTask; + } + + public Task Uninstall(string teamId) + { + return Task.CompletedTask; + } + } +} From 27b0c3c330daa8e265a6d58d865524846087ad41 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 18 Sep 2026 13:07:20 +0000 Subject: [PATCH 3/3] Name the state guard IsLocalUrl to match framework validation convention Co-authored-by: johnkors <206726+johnkors@users.noreply.github.com> --- .../SlackbotCodeTokenExchangeMiddleware.cs | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/source/src/Slackbot.Net.Endpoints/Middlewares/SlackbotCodeTokenExchangeMiddleware.cs b/source/src/Slackbot.Net.Endpoints/Middlewares/SlackbotCodeTokenExchangeMiddleware.cs index 535edeb..e0eb693 100644 --- a/source/src/Slackbot.Net.Endpoints/Middlewares/SlackbotCodeTokenExchangeMiddleware.cs +++ b/source/src/Slackbot.Net.Endpoints/Middlewares/SlackbotCodeTokenExchangeMiddleware.cs @@ -58,7 +58,7 @@ await installationHandler.Install(new Workspace /// private static string ResolveRedirectUri(string state, string successRedirectUri) { - if (!IsSiteRelative(state)) + if (!IsLocalUrl(state)) { return successRedirectUri; } @@ -73,21 +73,22 @@ private static string ResolveRedirectUri(string state, string successRedirectUri } /// - /// Accepts `/foo` but not absolute (`https://host/foo`) or protocol-relative (`//host`, - /// `/\host`) values, which would turn the callback into an open redirect. + /// Accepts site-relative urls like `/foo`, but not absolute (`https://host/foo`) or + /// protocol-relative (`//host`, `/\host`) ones, which would turn the callback into an open + /// redirect. /// - private static bool IsSiteRelative(string state) + private static bool IsLocalUrl(string url) { - if (string.IsNullOrEmpty(state) || state[0] != '/') + if (string.IsNullOrEmpty(url) || url[0] != '/') { return false; } - if (state.Length > 1 && (state[1] == '/' || state[1] == '\\')) + if (url.Length > 1 && (url[1] == '/' || url[1] == '\\')) { return false; } - return !state.Any(char.IsControl); + return !url.Any(char.IsControl); } }