fix(notifications): require a valid code, and escape the upstream path

dotnet/EcencyApi.Tests/NotificationsAuthorizationTests.cs

@@ -0,0 +1,77 @@
+using EcencyApi.Handlers;
+using Xunit;
+
+namespace EcencyApi.Tests;
+
+/// <summary>
+/// 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.
+/// </summary>
+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);
+        }
+    }
+}

dotnet/EcencyApi.Tests/NotificationsPathTests.cs

@@ -0,0 +1,139 @@
+using EcencyApi.Handlers;
+using Xunit;
+
+namespace EcencyApi.Tests;
+
+/// <summary>
+/// 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.
+/// </summary>
+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);
+    }
+}

dotnet/EcencyApi/Config.cs

@@ -12,6 +12,15 @@ public static class Config
     public static string PrivateApiAuth { get; } =
         Env("PRIVATE_API_AUTH") ?? "privateapiauth";
 
+    /// <summary>
+    /// 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.
+    /// </summary>
+    public static string EnotifyInternalToken { get; } =
+        Env("ENOTIFY_INTERNAL_TOKEN") ?? "";
+
     public static string HsClientSecret { get; } =
         Env("HIVESIGNER_SECRET") ?? "hivesignerclientsecret";
 

dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs

@@ -10,55 +10,131 @@ namespace EcencyApi.Handlers;
 /// </summary>
 public static partial class PrivateApi
 {
-    // POST ^/private-api/notifications$
-    public static async Task Notifications(HttpContext ctx)
+    /// <summary>
+    /// 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.
+    /// </summary>
+    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));
+    }
+
+    /// <summary>Header enotify reads the shared secret from.</summary>
+    public const string EnotifyInternalTokenHeader = "X-Ecency-Internal-Token";
+
+    /// <summary>
+    /// 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.
+    /// </summary>
+    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<string>();
 
-        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<string, string>(EnotifyInternalTokenHeader, Config.EnotifyInternalToken) }
+            : null;
+
+        await Upstream.Pipe(ApiClient.ApiRequest(u, HttpMethod.Get, extraHeaders), ctx);
     }
 
     // GET ^/private-api/pub-notifications/:username