From c2324542258eff099c46c0de96085a9388814894 Mon Sep 17 00:00:00 2001 From: ecency Date: Tue, 1 Sep 2026 15:40:43 +0000 Subject: [PATCH 1/4] fix(notifications): require a valid code, and escape the upstream path Closes half of #90. Two defects in the notifications handler. Authentication. The guard accepted a bare `user` body field in place of a code, so the 401 was only reachable when `user` was absent. Supplying it was enough to be served that account's notifications with no authentication at all. A valid code is now required before anything else is read. Path building. username, filter, since and limit were interpolated into the upstream path through Template(), which emulates JS string coercion and does no URL encoding. A value carrying / ? or # was therefore re-parsed as URL structure once the string became a Uri, reaching a different upstream endpoint with this service's credentials attached. That is what left the api-proxy per-path allowlist acting as a security control rather than routing hygiene. NotificationsPath() now escapes every segment and rejects dot segments, exactly as PostTipsPath() already does for the tips handlers, and for the same reason. Escaping is a no-op for real values: account names, filter names, notification ids and integer limits are all unreserved characters, so live requests are byte-identical. NOT changed, deliberately: a caller with a valid code can still name another account via `user`. Decks depends on it. Its notifications column is built from a free-text account search box and passes settings.username alongside the signed-in user's code (vision-web deck-notifications-column.tsx), so removing the override would break a shipped feature. This narrows the exposure from anyone on the internet to any signed-in user; whether that should be narrowed further is a product decision tracked in #90. Tests mirror PostTipsPathTests: real requests unchanged, structural characters cannot escape their segment, dot segments rejected, and query values cannot append parameters of their own. No dotnet SDK on the machine this was written on, so the build and tests were not run locally. CI runs both on pull_request. --- .../EcencyApi.Tests/NotificationsPathTests.cs | 97 +++++++++++++++++++ .../Handlers/PrivateApi.UserData1.cs | 86 +++++++++++----- 2 files changed, 158 insertions(+), 25 deletions(-) create mode 100644 dotnet/EcencyApi.Tests/NotificationsPathTests.cs diff --git a/dotnet/EcencyApi.Tests/NotificationsPathTests.cs b/dotnet/EcencyApi.Tests/NotificationsPathTests.cs new file mode 100644 index 00000000..4ff086a6 --- /dev/null +++ b/dotnet/EcencyApi.Tests/NotificationsPathTests.cs @@ -0,0 +1,97 @@ +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)); + Assert.Equal( + "follows/good-karma", + PrivateApi.NotificationsPath("good-karma", "follows", null, null)); + Assert.Equal( + "activities/user.name?since=f-179530372", + PrivateApi.NotificationsPath("user.name", null, "f-179530372", null)); + Assert.Equal( + "follows/good-karma?since=f-179530372&limit=50", + PrivateApi.NotificationsPath("good-karma", "follows", "f-179530372", "50")); + Assert.Equal( + "activities/good-karma?limit=50", + PrivateApi.NotificationsPath("good-karma", null, null, "50")); + } + + [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); + + 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)); + } + + [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); + Assert.Equal("activities/good-karma?since=a%26limit%3D999", path); + + var withLimit = PrivateApi.NotificationsPath("good-karma", null, null, "1&x=2"); + 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")); + Assert.Equal( + "activities/x?limit=10", + PrivateApi.NotificationsPath("x", null, null, "10")); + } +} diff --git a/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs b/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs index 445453de..ea31f528 100644 --- a/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs +++ b/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs @@ -10,6 +10,47 @@ namespace EcencyApi.Handlers; /// public static partial class PrivateApi { + /// + /// 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) + { + if (IsDotSegment(username) || (filter != null && IsDotSegment(filter))) + { + return null; + } + + var u = filter != null + ? $"{Uri.EscapeDataString(filter)}/{Uri.EscapeDataString(username)}" + : $"activities/{Uri.EscapeDataString(username)}"; + + if (since != null) + { + u += $"?since={Uri.EscapeDataString(since)}"; + + if (limit != null) + { + u += $"&limit={Uri.EscapeDataString(limit)}"; + } + } + else if (limit != null) + { + u += $"?limit={Uri.EscapeDataString(limit)}"; + } + + return u; + } + // POST ^/private-api/notifications$ public static async Task Notifications(HttpContext ctx) { @@ -17,16 +58,21 @@ public static async Task Notifications(HttpContext ctx) var username = await ValidateCode(body); var user = body.Field("user"); + // A valid code is required. This guard used to accept a bare `user` field in + // place of one, so an unauthenticated caller could name any account and be + // served its notifications. if (string.IsNullOrEmpty(username)) { - if (!JsJson.IsTruthy(user)) - { - await ctx.SendText(401, "Unauthorized"); - return; - } - username = UserData1Helpers.Template(user); + await ctx.SendText(401, "Unauthorized"); + return; } - // if user defined but not same as user's code + + // Decks builds a notifications column for an arbitrary account and sends that + // name here alongside the signed-in user's own code (vision-web + // deck-notifications-column.tsx passes settings.username). Kept deliberately so + // that feature keeps working, but it is now reachable only by an authenticated + // caller rather than by anyone. Whether one account should be able to read + // another's Ecency-private activity at all is a separate product question. if (JsJson.IsTruthy(user)) { username = UserData1Helpers.Template(user); @@ -36,26 +82,16 @@ public static async Task Notifications(HttpContext ctx) var since = body.Field("since"); var limit = body.Field("limit"); - var u = $"activities/{username}"; + 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); - if (JsJson.IsTruthy(filter)) + if (u == null) { - u = $"{UserData1Helpers.Template(filter)}/{username}"; - } - - if (JsJson.IsTruthy(since)) - { - u += $"?since={UserData1Helpers.Template(since)}"; - } - - if (JsJson.IsTruthy(since) && JsJson.IsTruthy(limit)) - { - u += $"&limit={UserData1Helpers.Template(limit)}"; - } - - if (!JsJson.IsTruthy(since) && JsJson.IsTruthy(limit)) - { - u += $"?limit={UserData1Helpers.Template(limit)}"; + await ctx.SendText(400, "Invalid user or filter"); + return; } await Upstream.Pipe(ApiClient.ApiRequest(u, HttpMethod.Get), ctx); From da4c67dd1af594b52ee2e623898e3d41f68371d8 Mon Sep 17 00:00:00 2001 From: ecency Date: Tue, 1 Sep 2026 15:54:57 +0000 Subject: [PATCH 2/4] Downgrade a cross-account notifications view to scope=public Follow-up to the auth fix in this PR, and the product half of #90. Decks builds a notifications column for an arbitrary account and passes that name alongside the signed-in user's own code. That stays supported: notifications are largely public data and the column exists for that reason. But the feed also carries Ecency-only activity that is not public. Favorites and bookmarks reveal who a user follows and what they saved, and Points transfers, streaks and the monthly/weekly aggregates exist nowhere on chain. So a request for SOMEONE ELSE's notifications now carries scope=public, which enotify restricts to chain-derived types (ecency/enotify-py#21). This service is the only layer that can make that call, because it is the only one that has validated who is asking. The comparison is case-insensitive, and a request for your own account is unaffected: without the flag the upstream path is byte-identical to what it was before scope existed. scope is derived from the validated code and never read from the body, so a caller cannot ask for a wider view than they are entitled to. NotificationsPath now assembles query values through a list, which keeps the original `?` then `&` ordering while making the appended parameter unambiguous. Tests cover the flag off and on, joining against existing since/limit values, and that a since value trying to smuggle its own scope parameter is escaped into a literal rather than overriding the real one. Still no dotnet SDK locally, so CI remains the gate. --- .../EcencyApi.Tests/NotificationsPathTests.cs | 63 +++++++++++++++---- .../Handlers/PrivateApi.UserData1.cs | 46 +++++++++----- 2 files changed, 83 insertions(+), 26 deletions(-) diff --git a/dotnet/EcencyApi.Tests/NotificationsPathTests.cs b/dotnet/EcencyApi.Tests/NotificationsPathTests.cs index 4ff086a6..2f5c0ac3 100644 --- a/dotnet/EcencyApi.Tests/NotificationsPathTests.cs +++ b/dotnet/EcencyApi.Tests/NotificationsPathTests.cs @@ -21,19 +21,19 @@ public void RealRequestsAreUnchanged() // change every live request. Assert.Equal( "activities/good-karma", - PrivateApi.NotificationsPath("good-karma", null, null, null)); + PrivateApi.NotificationsPath("good-karma", null, null, null, false)); Assert.Equal( "follows/good-karma", - PrivateApi.NotificationsPath("good-karma", "follows", null, null)); + PrivateApi.NotificationsPath("good-karma", "follows", null, null, false)); Assert.Equal( "activities/user.name?since=f-179530372", - PrivateApi.NotificationsPath("user.name", null, "f-179530372", null)); + 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")); + PrivateApi.NotificationsPath("good-karma", "follows", "f-179530372", "50", false)); Assert.Equal( "activities/good-karma?limit=50", - PrivateApi.NotificationsPath("good-karma", null, null, "50")); + PrivateApi.NotificationsPath("good-karma", null, null, "50", false)); } [Theory] @@ -50,7 +50,7 @@ public void RealRequestsAreUnchanged() [InlineData("a", "b#f")] public void StructuralCharactersCannotEscapeTheirSegment(string username, string? filter) { - var path = PrivateApi.NotificationsPath(username, filter, null, null); + var path = PrivateApi.NotificationsPath(username, filter, null, null, false); Assert.NotNull(path); Assert.DoesNotContain("?x=1", path); @@ -68,17 +68,17 @@ public void StructuralCharactersCannotEscapeTheirSegment(string username, string [InlineData("a", "..")] public void DotSegmentsAreRejected(string username, string? filter) { - Assert.Null(PrivateApi.NotificationsPath(username, filter, null, null)); + 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); + 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"); + var withLimit = PrivateApi.NotificationsPath("good-karma", null, null, "1&x=2", false); Assert.Equal("activities/good-karma?limit=1%26x%3D2", withLimit); } @@ -89,9 +89,50 @@ public void LimitJoinsWithAmpersandOnlyWhenSinceIsPresent() // `?` 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")); + PrivateApi.NotificationsPath("x", null, "s", "10", false)); Assert.Equal( "activities/x?limit=10", - PrivateApi.NotificationsPath("x", null, null, "10")); + PrivateApi.NotificationsPath("x", null, null, "10", false)); + } + + [Fact] + public void PublicScopeIsAppendedOnlyForACrossAccountView() + { + // Nobody's own feed changes shape: without the flag the path is byte-identical + // to what it was before scope existed. + Assert.Equal( + "activities/good-karma", + PrivateApi.NotificationsPath("good-karma", null, null, null, false)); + + Assert.Equal( + "activities/good-karma?scope=public", + PrivateApi.NotificationsPath("good-karma", null, null, null, true)); + } + + [Fact] + public void PublicScopeJoinsCorrectlyWithExistingQueryValues() + { + Assert.Equal( + "follows/good-karma?since=f-179530372&limit=50&scope=public", + PrivateApi.NotificationsPath("good-karma", "follows", "f-179530372", "50", true)); + + Assert.Equal( + "activities/good-karma?limit=50&scope=public", + PrivateApi.NotificationsPath("good-karma", null, null, "50", true)); + + Assert.Equal( + "activities/good-karma?since=s&scope=public", + 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. + var path = PrivateApi.NotificationsPath("good-karma", null, "s&scope=all", null, true); + + Assert.Equal("activities/good-karma?since=s%26scope%3Dall&scope=public", path); + Assert.EndsWith("&scope=public", path); } } diff --git a/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs b/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs index ea31f528..5599698c 100644 --- a/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs +++ b/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs @@ -23,7 +23,8 @@ public static partial class PrivateApi /// unreserved characters, which EscapeDataString leaves byte-identical, so real /// traffic is unaffected. /// - public static string? NotificationsPath(string username, string? filter, string? since, string? limit) + public static string? NotificationsPath( + string username, string? filter, string? since, string? limit, bool publicScope) { if (IsDotSegment(username) || (filter != null && IsDotSegment(filter))) { @@ -34,21 +35,26 @@ public static partial class PrivateApi ? $"{Uri.EscapeDataString(filter)}/{Uri.EscapeDataString(username)}" : $"activities/{Uri.EscapeDataString(username)}"; + var query = new List(); + if (since != null) { - u += $"?since={Uri.EscapeDataString(since)}"; + query.Add($"since={Uri.EscapeDataString(since)}"); + } - if (limit != null) - { - u += $"&limit={Uri.EscapeDataString(limit)}"; - } + if (limit != null) + { + query.Add($"limit={Uri.EscapeDataString(limit)}"); } - else if (limit != null) + + // Restricts the upstream feed to chain-derived activity. Set only when one + // account is asking for another's notifications, so nobody's own feed changes. + if (publicScope) { - u += $"?limit={Uri.EscapeDataString(limit)}"; + query.Add("scope=public"); } - return u; + return query.Count == 0 ? u : $"{u}?{string.Join("&", query)}"; } // POST ^/private-api/notifications$ @@ -69,13 +75,22 @@ public static async Task Notifications(HttpContext ctx) // Decks builds a notifications column for an arbitrary account and sends that // name here alongside the signed-in user's own code (vision-web - // deck-notifications-column.tsx passes settings.username). Kept deliberately so - // that feature keeps working, but it is now reachable only by an authenticated - // caller rather than by anyone. Whether one account should be able to read - // another's Ecency-private activity at all is a separate product question. + // deck-notifications-column.tsx passes settings.username). That stays supported: + // notifications are largely public data and the column exists for that reason. + // + // But the feed also carries Ecency-only activity that is not public, notably + // favorites and bookmarks, which reveal who a user follows and what they saved, + // plus Points transfers and the various streaks and aggregates. So a request for + // SOMEONE ELSE's notifications is downgraded to scope=public, which enotify + // restricts to chain-derived types. This service is the only layer that can make + // that call, because it is the only one that has validated who is asking. + var publicScope = false; + if (JsJson.IsTruthy(user)) { - username = UserData1Helpers.Template(user); + var requested = UserData1Helpers.Template(user); + publicScope = !string.Equals(requested, username, StringComparison.OrdinalIgnoreCase); + username = requested; } var filter = body.Field("filter"); @@ -86,7 +101,8 @@ public static async Task Notifications(HttpContext ctx) username, JsJson.IsTruthy(filter) ? UserData1Helpers.Template(filter) : null, JsJson.IsTruthy(since) ? UserData1Helpers.Template(since) : null, - JsJson.IsTruthy(limit) ? UserData1Helpers.Template(limit) : null); + JsJson.IsTruthy(limit) ? UserData1Helpers.Template(limit) : null, + publicScope); if (u == null) { From f30cef0b4feff5f9faf123e275fd8130c50f5d17 Mon Sep 17 00:00:00 2001 From: ecency Date: Tue, 1 Sep 2026 16:26:51 +0000 Subject: [PATCH 3/4] Invert the flag: ask for scope=full on a self-view, not scope=public Follows the enotify review. enotify now defaults to chain-derived activity only and requires scope=full to widen, because it performs no authentication of its own and its host was reachable from the public internet, so an opt-in restriction protected nothing against a caller who simply omitted the parameter. This side flips to match. A self-view asks for scope=full; a cross-account view sends no scope parameter at all, which is the safe direction: any request that never reaches this handler now gets the restricted feed rather than the whole one. Behaviour for users is unchanged in both directions. Deploy this before the enotify change: scope=full is ignored until enotify ships, so there is no window where a user loses their own favorites, bookmarks or aggregates. The forgery test now asserts the opposite direction too: a since value carrying `scope=full` is escaped into a literal and, with the flag off, no real scope parameter is appended for it to piggyback on. --- .../EcencyApi.Tests/NotificationsPathTests.cs | 25 ++++++++++--------- .../Handlers/PrivateApi.UserData1.cs | 22 ++++++++-------- 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/dotnet/EcencyApi.Tests/NotificationsPathTests.cs b/dotnet/EcencyApi.Tests/NotificationsPathTests.cs index 2f5c0ac3..ee8c6f6c 100644 --- a/dotnet/EcencyApi.Tests/NotificationsPathTests.cs +++ b/dotnet/EcencyApi.Tests/NotificationsPathTests.cs @@ -96,32 +96,32 @@ public void LimitJoinsWithAmpersandOnlyWhenSinceIsPresent() } [Fact] - public void PublicScopeIsAppendedOnlyForACrossAccountView() + public void FullScopeIsAppendedOnlyForASelfView() { - // Nobody's own feed changes shape: without the flag the path is byte-identical - // to what it was before scope existed. + // 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=public", + "activities/good-karma?scope=full", PrivateApi.NotificationsPath("good-karma", null, null, null, true)); } [Fact] - public void PublicScopeJoinsCorrectlyWithExistingQueryValues() + public void FullScopeJoinsCorrectlyWithExistingQueryValues() { Assert.Equal( - "follows/good-karma?since=f-179530372&limit=50&scope=public", + "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=public", + "activities/good-karma?limit=50&scope=full", PrivateApi.NotificationsPath("good-karma", null, null, "50", true)); Assert.Equal( - "activities/good-karma?since=s&scope=public", + "activities/good-karma?since=s&scope=full", PrivateApi.NotificationsPath("good-karma", null, "s", null, true)); } @@ -129,10 +129,11 @@ public void PublicScopeJoinsCorrectlyWithExistingQueryValues() 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. - var path = PrivateApi.NotificationsPath("good-karma", null, "s&scope=all", null, true); + // 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%3Dall&scope=public", path); - Assert.EndsWith("&scope=public", path); + Assert.Equal("activities/good-karma?since=s%26scope%3Dfull", path); + Assert.DoesNotContain("&scope=full", path); } } diff --git a/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs b/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs index 5599698c..d1925fdd 100644 --- a/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs +++ b/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs @@ -24,7 +24,7 @@ public static partial class PrivateApi /// traffic is unaffected. /// public static string? NotificationsPath( - string username, string? filter, string? since, string? limit, bool publicScope) + string username, string? filter, string? since, string? limit, bool fullScope) { if (IsDotSegment(username) || (filter != null && IsDotSegment(filter))) { @@ -47,11 +47,12 @@ public static partial class PrivateApi query.Add($"limit={Uri.EscapeDataString(limit)}"); } - // Restricts the upstream feed to chain-derived activity. Set only when one - // account is asking for another's notifications, so nobody's own feed changes. - if (publicScope) + // 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) { - query.Add("scope=public"); + query.Add("scope=full"); } return query.Count == 0 ? u : $"{u}?{string.Join("&", query)}"; @@ -81,15 +82,16 @@ public static async Task Notifications(HttpContext ctx) // But the feed also carries Ecency-only activity that is not public, notably // favorites and bookmarks, which reveal who a user follows and what they saved, // plus Points transfers and the various streaks and aggregates. So a request for - // SOMEONE ELSE's notifications is downgraded to scope=public, which enotify - // restricts to chain-derived types. This service is the only layer that can make + // SOMEONE ELSE's notifications is left at enotify's restricted default, and only + // a self-view asks for scope=full. This service is the only layer that can make // that call, because it is the only one that has validated who is asking. - var publicScope = false; + // Only a caller asking for their OWN notifications gets the complete feed. + var fullScope = true; if (JsJson.IsTruthy(user)) { var requested = UserData1Helpers.Template(user); - publicScope = !string.Equals(requested, username, StringComparison.OrdinalIgnoreCase); + fullScope = string.Equals(requested, username, StringComparison.OrdinalIgnoreCase); username = requested; } @@ -102,7 +104,7 @@ public static async Task Notifications(HttpContext ctx) JsJson.IsTruthy(filter) ? UserData1Helpers.Template(filter) : null, JsJson.IsTruthy(since) ? UserData1Helpers.Template(since) : null, JsJson.IsTruthy(limit) ? UserData1Helpers.Template(limit) : null, - publicScope); + fullScope); if (u == null) { From 1420aec1273a81f834247e0ec726c6d5ccc18acb Mon Sep 17 00:00:00 2001 From: ecency Date: Tue, 1 Sep 2026 16:51:16 +0000 Subject: [PATCH 4/4] Present the enotify secret, and make the authorization decision testable Two review findings. enotify now requires X-Ecency-Internal-Token alongside scope=full, because a query parameter cannot gate private data on a service with no authentication of its own. This side presents it, from ENOTIFY_INTERNAL_TOKEN, only on a self-view. enotify fails closed, so a missing or wrong token costs that user their own private activity rather than exposing anyone else's. Qodo was right that the authorization itself had no test: the suite covered URI construction only, so both original defects could recur silently. ResolveNotificationsTarget() now holds the decision as a pure function and is tested directly: no valid code is unauthorized even when an account is named, which is the exact bypass; a code alone serves that account's complete feed; the self-view comparison is case-insensitive; naming another account is still permitted but never sets full scope, including for near-miss names. IsTruthy stays in the handler so the port keeps its JS truthiness parity while the decision stays pure. Still no dotnet SDK locally, so CI remains the gate. --- .../NotificationsAuthorizationTests.cs | 77 +++++++++++++++++++ dotnet/EcencyApi/Config.cs | 9 +++ .../Handlers/PrivateApi.UserData1.cs | 76 +++++++++++------- 3 files changed, 135 insertions(+), 27 deletions(-) create mode 100644 dotnet/EcencyApi.Tests/NotificationsAuthorizationTests.cs 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/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 d1925fdd..f99969d2 100644 --- a/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs +++ b/dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs @@ -10,6 +10,40 @@ namespace EcencyApi.Handlers; /// public static partial class PrivateApi { + /// + /// 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) + { + if (string.IsNullOrEmpty(validatedUsername)) + { + return (null, false); + } + + if (requestedUser == null) + { + return (validatedUsername, true); + } + + 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. @@ -62,39 +96,20 @@ public static partial class PrivateApi public static async Task Notifications(HttpContext ctx) { var body = await ctx.ReadBody(); - var username = await ValidateCode(body); var user = body.Field("user"); - // A valid code is required. This guard used to accept a bare `user` field in - // place of one, so an unauthenticated caller could name any account and be - // served its notifications. - if (string.IsNullOrEmpty(username)) + // 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; } - // Decks builds a notifications column for an arbitrary account and sends that - // name here alongside the signed-in user's own code (vision-web - // deck-notifications-column.tsx passes settings.username). That stays supported: - // notifications are largely public data and the column exists for that reason. - // - // But the feed also carries Ecency-only activity that is not public, notably - // favorites and bookmarks, which reveal who a user follows and what they saved, - // plus Points transfers and the various streaks and aggregates. So a request for - // SOMEONE ELSE's notifications is left at enotify's restricted default, and only - // a self-view asks for scope=full. This service is the only layer that can make - // that call, because it is the only one that has validated who is asking. - // Only a caller asking for their OWN notifications gets the complete feed. - var fullScope = true; - - if (JsJson.IsTruthy(user)) - { - var requested = UserData1Helpers.Template(user); - fullScope = string.Equals(requested, username, StringComparison.OrdinalIgnoreCase); - username = requested; - } - var filter = body.Field("filter"); var since = body.Field("since"); var limit = body.Field("limit"); @@ -112,7 +127,14 @@ public static async Task Notifications(HttpContext ctx) 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