diff --git a/dotnet/EcencyApi.Tests/NotificationsAuthorizationTests.cs b/dotnet/EcencyApi.Tests/NotificationsAuthorizationTests.cs new file mode 100644 index 00000000..f5118e46 --- /dev/null +++ b/dotnet/EcencyApi.Tests/NotificationsAuthorizationTests.cs @@ -0,0 +1,77 @@ +using EcencyApi.Handlers; +using Xunit; + +namespace EcencyApi.Tests; + +/// +/// The authorization decision for /private-api/notifications, which had two defects: +/// a request could satisfy the guard with a `user` field and no valid code at all, and a +/// valid code was then overridden by that field anyway. +/// +/// Kept separate from path construction because these are the rules that decide whose +/// data is served, and a regression here is silent rather than a visible break. +/// +public class NotificationsAuthorizationTests +{ + [Theory] + [InlineData(null)] + [InlineData("")] + public void WithoutAValidCodeTheRequestIsUnauthorized(string? validated) + { + // No code at all. + var (username, fullScope) = PrivateApi.ResolveNotificationsTarget(validated, null); + Assert.Null(username); + Assert.False(fullScope); + + // THE BYPASS: naming an account used to be accepted in place of a code. + var named = PrivateApi.ResolveNotificationsTarget(validated, "victim"); + Assert.Null(named.Username); + Assert.False(named.FullScope); + } + + [Fact] + public void AValidCodeAloneServesThatAccountsCompleteFeed() + { + var (username, fullScope) = PrivateApi.ResolveNotificationsTarget("good-karma", null); + + Assert.Equal("good-karma", username); + Assert.True(fullScope); + } + + [Theory] + [InlineData("good-karma")] + // Hive names are lowercase, but the comparison must not hinge on that. + [InlineData("Good-Karma")] + [InlineData("GOOD-KARMA")] + public void NamingYourOwnAccountIsStillASelfView(string requested) + { + var (username, fullScope) = PrivateApi.ResolveNotificationsTarget("good-karma", requested); + + Assert.Equal(requested, username); + Assert.True(fullScope); + } + + [Fact] + public void NamingAnotherAccountIsServedTheRestrictedFeed() + { + // Still permitted: Decks builds notification columns for arbitrary accounts and + // notifications are largely public. It just does not unlock the complete feed. + var (username, fullScope) = PrivateApi.ResolveNotificationsTarget("good-karma", "someone-else"); + + Assert.Equal("someone-else", username); + Assert.False(fullScope); + } + + [Fact] + public void OnlyASelfViewEverSetsFullScope() + { + // The property that matters: for any requested account other than the validated + // one, fullScope is false. A near-miss must not slip through. + foreach (var other in new[] { "good-karm", "good-karma2", "ood-karma", " good-karma", "good_karma" }) + { + Assert.False( + PrivateApi.ResolveNotificationsTarget("good-karma", other).FullScope, + other); + } + } +} diff --git a/dotnet/EcencyApi.Tests/NotificationsPathTests.cs b/dotnet/EcencyApi.Tests/NotificationsPathTests.cs new file mode 100644 index 00000000..ee8c6f6c --- /dev/null +++ b/dotnet/EcencyApi.Tests/NotificationsPathTests.cs @@ -0,0 +1,139 @@ +using EcencyApi.Handlers; +using Xunit; + +namespace EcencyApi.Tests; + +/// +/// The notifications handler builds an upstream path by interpolating four +/// caller-supplied body values: the account name, the filter, a paging cursor and a +/// limit. Body values are arbitrary strings, so anything structural left unescaped is +/// re-parsed when the string becomes a Uri — and the upstream call carries this +/// service's credentials, so a redirected path is a real problem. Same reasoning as +/// PostTipsPathTests. +/// +public class NotificationsPathTests +{ + [Fact] + public void RealRequestsAreUnchanged() + { + // Hive names, filter names, notification ids and integer limits are all + // unreserved characters; escaping must be a no-op for them or this would + // change every live request. + Assert.Equal( + "activities/good-karma", + PrivateApi.NotificationsPath("good-karma", null, null, null, false)); + Assert.Equal( + "follows/good-karma", + PrivateApi.NotificationsPath("good-karma", "follows", null, null, false)); + Assert.Equal( + "activities/user.name?since=f-179530372", + PrivateApi.NotificationsPath("user.name", null, "f-179530372", null, false)); + Assert.Equal( + "follows/good-karma?since=f-179530372&limit=50", + PrivateApi.NotificationsPath("good-karma", "follows", "f-179530372", "50", false)); + Assert.Equal( + "activities/good-karma?limit=50", + PrivateApi.NotificationsPath("good-karma", null, null, "50", false)); + } + + [Theory] + // A slash would add path segments and address a different resource. This is the + // shape that made the nginx per-path allowlist load-bearing rather than routing + // hygiene: `unread-count?x=` as a username reached a different upstream endpoint. + [InlineData("a/b", null)] + [InlineData("a", "b/c")] + // A question mark would truncate the path and turn the rest into a query. + [InlineData("a?x=1", null)] + [InlineData("a", "b?x=1")] + // A hash would truncate the path at a fragment. + [InlineData("a#f", null)] + [InlineData("a", "b#f")] + public void StructuralCharactersCannotEscapeTheirSegment(string username, string? filter) + { + var path = PrivateApi.NotificationsPath(username, filter, null, null, false); + + Assert.NotNull(path); + Assert.DoesNotContain("?x=1", path); + Assert.DoesNotContain("#f", path); + // The only separators left are the ones this builder wrote itself. + Assert.Equal(1, path!.Split('/').Length - 1); + } + + [Theory] + // Dot segments cannot be fixed by escaping: Uri decodes %2E back to `.` before it + // removes dot segments, so they have to be rejected outright. + [InlineData(".", null)] + [InlineData("..", null)] + [InlineData("a", ".")] + [InlineData("a", "..")] + public void DotSegmentsAreRejected(string username, string? filter) + { + Assert.Null(PrivateApi.NotificationsPath(username, filter, null, null, false)); + } + + [Fact] + public void QueryValuesCannotAddParameters() + { + // A cursor or limit carrying `&` would otherwise append parameters of its own. + var path = PrivateApi.NotificationsPath("good-karma", null, "a&limit=999", null, false); + Assert.Equal("activities/good-karma?since=a%26limit%3D999", path); + + var withLimit = PrivateApi.NotificationsPath("good-karma", null, null, "1&x=2", false); + Assert.Equal("activities/good-karma?limit=1%26x%3D2", withLimit); + } + + [Fact] + public void LimitJoinsWithAmpersandOnlyWhenSinceIsPresent() + { + // Preserves the original branching: limit rides `&` when since is present and + // `?` when it is not, so an existing client's paging URLs do not change shape. + Assert.Equal( + "activities/x?since=s&limit=10", + PrivateApi.NotificationsPath("x", null, "s", "10", false)); + Assert.Equal( + "activities/x?limit=10", + PrivateApi.NotificationsPath("x", null, null, "10", false)); + } + + [Fact] + public void FullScopeIsAppendedOnlyForASelfView() + { + // Omitting the flag is the SAFE direction: enotify defaults to chain-derived + // activity only, so a cross-account view needs no parameter at all. + Assert.Equal( + "activities/good-karma", + PrivateApi.NotificationsPath("good-karma", null, null, null, false)); + + Assert.Equal( + "activities/good-karma?scope=full", + PrivateApi.NotificationsPath("good-karma", null, null, null, true)); + } + + [Fact] + public void FullScopeJoinsCorrectlyWithExistingQueryValues() + { + Assert.Equal( + "follows/good-karma?since=f-179530372&limit=50&scope=full", + PrivateApi.NotificationsPath("good-karma", "follows", "f-179530372", "50", true)); + + Assert.Equal( + "activities/good-karma?limit=50&scope=full", + PrivateApi.NotificationsPath("good-karma", null, null, "50", true)); + + Assert.Equal( + "activities/good-karma?since=s&scope=full", + PrivateApi.NotificationsPath("good-karma", null, "s", null, true)); + } + + [Fact] + public void ACallerCannotForgeTheScopeParameter() + { + // scope is decided by the handler from the validated code, never read from the + // body. A value trying to smuggle its own parameter is escaped into a literal, + // and the real one is appended last regardless. + var path = PrivateApi.NotificationsPath("good-karma", null, "s&scope=full", null, false); + + Assert.Equal("activities/good-karma?since=s%26scope%3Dfull", path); + Assert.DoesNotContain("&scope=full", path); + } +} diff --git a/dotnet/EcencyApi/Config.cs b/dotnet/EcencyApi/Config.cs index e0d2e586..de3eddaf 100644 --- a/dotnet/EcencyApi/Config.cs +++ b/dotnet/EcencyApi/Config.cs @@ -12,6 +12,15 @@ public static class Config public static string PrivateApiAuth { get; } = Env("PRIVATE_API_AUTH") ?? "privateapiauth"; + /// + /// Shared secret presented to enotify to unlock a user's complete notification feed. + /// enotify has no authentication of its own and defaults to chain-derived activity + /// only, so without this a self-view silently loses favorites, bookmarks, Points + /// transfers and the aggregates. Must match [APP] INTERNAL_TOKEN there. + /// + public static string EnotifyInternalToken { get; } = + Env("ENOTIFY_INTERNAL_TOKEN") ?? ""; + public static string HsClientSecret { get; } = Env("HIVESIGNER_SECRET") ?? "hivesignerclientsecret"; diff --git a/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs b/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs index 445453de..f99969d2 100644 --- a/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs +++ b/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs @@ -10,55 +10,131 @@ namespace EcencyApi.Handlers; /// public static partial class PrivateApi { - // POST ^/private-api/notifications$ - public static async Task Notifications(HttpContext ctx) + /// + /// Who a notifications request is for, and whether it may see the complete feed. + /// + /// A null Username means unauthorized. `requestedUser` is the body's `user` field + /// after JS-truthiness, or null when it was absent or falsy. + /// + /// Two rules, both of which were wrong before: + /// - a validated code is REQUIRED. `requestedUser` used to satisfy the guard on its + /// own, so an unauthenticated caller could name any account. + /// - only a SELF view sees the complete feed. Naming another account is still + /// supported, because Decks builds notification columns for arbitrary accounts and + /// notifications are largely public, but it is served enotify's restricted feed. + /// + public static (string? Username, bool FullScope) ResolveNotificationsTarget( + string? validatedUsername, string? requestedUser) { - var body = await ctx.ReadBody(); - var username = await ValidateCode(body); - var user = body.Field("user"); + if (string.IsNullOrEmpty(validatedUsername)) + { + return (null, false); + } - if (string.IsNullOrEmpty(username)) + if (requestedUser == null) { - if (!JsJson.IsTruthy(user)) - { - await ctx.SendText(401, "Unauthorized"); - return; - } - username = UserData1Helpers.Template(user); + return (validatedUsername, true); } - // if user defined but not same as user's code - if (JsJson.IsTruthy(user)) + + return ( + requestedUser, + string.Equals(requestedUser, validatedUsername, StringComparison.OrdinalIgnoreCase)); + } + + /// Header enotify reads the shared secret from. + public const string EnotifyInternalTokenHeader = "X-Ecency-Internal-Token"; + + /// + /// Upstream path for the notifications feed, or null when a value cannot be + /// expressed as a single path segment. + /// + /// Every caller-supplied value is escaped. These are arbitrary body strings, so + /// one carrying `/`, `?` or `#` would otherwise be re-parsed as URL structure + /// once this string becomes a Uri, addressing a different upstream resource with + /// this service's credentials attached. Same reasoning as PostTipsPath. + /// + /// Hive account names, filter names, notification ids and integer limits are all + /// unreserved characters, which EscapeDataString leaves byte-identical, so real + /// traffic is unaffected. + /// + public static string? NotificationsPath( + string username, string? filter, string? since, string? limit, bool fullScope) + { + if (IsDotSegment(username) || (filter != null && IsDotSegment(filter))) { - username = UserData1Helpers.Template(user); + return null; } - var filter = body.Field("filter"); - var since = body.Field("since"); - var limit = body.Field("limit"); + var u = filter != null + ? $"{Uri.EscapeDataString(filter)}/{Uri.EscapeDataString(username)}" + : $"activities/{Uri.EscapeDataString(username)}"; - var u = $"activities/{username}"; + var query = new List(); - if (JsJson.IsTruthy(filter)) + if (since != null) { - u = $"{UserData1Helpers.Template(filter)}/{username}"; + query.Add($"since={Uri.EscapeDataString(since)}"); } - if (JsJson.IsTruthy(since)) + if (limit != null) { - u += $"?since={UserData1Helpers.Template(since)}"; + query.Add($"limit={Uri.EscapeDataString(limit)}"); } - if (JsJson.IsTruthy(since) && JsJson.IsTruthy(limit)) + // Opts in to the complete feed. enotify defaults to chain-derived activity only, + // so omitting this is the safe direction: a cross-account view, or any request + // that never reaches this handler, gets the restricted feed. + if (fullScope) { - u += $"&limit={UserData1Helpers.Template(limit)}"; + query.Add("scope=full"); } - if (!JsJson.IsTruthy(since) && JsJson.IsTruthy(limit)) + return query.Count == 0 ? u : $"{u}?{string.Join("&", query)}"; + } + + // POST ^/private-api/notifications$ + public static async Task Notifications(HttpContext ctx) + { + var body = await ctx.ReadBody(); + var user = body.Field("user"); + + // IsTruthy here rather than in the resolver, to keep the JS truthiness parity + // this port is built on while the decision itself stays pure and testable. + var (username, fullScope) = ResolveNotificationsTarget( + await ValidateCode(body), + JsJson.IsTruthy(user) ? UserData1Helpers.Template(user) : null); + + if (username == null) + { + await ctx.SendText(401, "Unauthorized"); + return; + } + + var filter = body.Field("filter"); + var since = body.Field("since"); + var limit = body.Field("limit"); + + var u = NotificationsPath( + username, + JsJson.IsTruthy(filter) ? UserData1Helpers.Template(filter) : null, + JsJson.IsTruthy(since) ? UserData1Helpers.Template(since) : null, + JsJson.IsTruthy(limit) ? UserData1Helpers.Template(limit) : null, + fullScope); + + if (u == null) { - u += $"?limit={UserData1Helpers.Template(limit)}"; + await ctx.SendText(400, "Invalid user or filter"); + return; } - await Upstream.Pipe(ApiClient.ApiRequest(u, HttpMethod.Get), ctx); + // The secret rides alongside scope=full. enotify honours the parameter only when + // the header matches, and fails closed otherwise, so a missing or wrong token + // costs this user their own private activity rather than exposing anyone else's. + var extraHeaders = fullScope && Config.EnotifyInternalToken.Length > 0 + ? new[] { new KeyValuePair(EnotifyInternalTokenHeader, Config.EnotifyInternalToken) } + : null; + + await Upstream.Pipe(ApiClient.ApiRequest(u, HttpMethod.Get, extraHeaders), ctx); } // GET ^/private-api/pub-notifications/:username