Serve notifications for the validated account only

dotnet/EcencyApi.Tests/NotificationsAuthorizationTests.cs

@@ -10,6 +10,7 @@ namespace EcencyApi.Tests;
 ///
 /// 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.
+/// The account is the validated code only, including when the body names someone else.
 /// </summary>
 public class NotificationsAuthorizationTests
 {
@@ -40,38 +41,131 @@ public void AValidCodeAloneServesThatAccountsCompleteFeed()
 
     [Theory]
     [InlineData("good-karma")]
-    // Hive names are lowercase, but the comparison must not hinge on that.
+    // The body's spelling of the same account must not replace the code's account.
     [InlineData("Good-Karma")]
     [InlineData("GOOD-KARMA")]
-    public void NamingYourOwnAccountIsStillASelfView(string requested)
+    public void NamingYourOwnAccountStillServesTheCode(string requested)
     {
         var (username, fullScope) = PrivateApi.ResolveNotificationsTarget("good-karma", requested);
 
-        Assert.Equal(requested, username);
+        Assert.Equal("good-karma", username);
         Assert.True(fullScope);
     }
 
     [Fact]
-    public void NamingAnotherAccountIsServedTheRestrictedFeed()
+    public void ABodyNamingAnotherAccountDoesNotOverrideTheCode()
     {
-        // Still permitted: Decks builds notification columns for arbitrary accounts and
-        // notifications are largely public. It just does not unlock the complete feed.
+        // Previously this returned someone-else (a restricted feed). The body does not
+        // choose the account: the code's feed is served, not the one named here.
         var (username, fullScope) = PrivateApi.ResolveNotificationsTarget("good-karma", "someone-else");
 
-        Assert.Equal("someone-else", username);
-        Assert.False(fullScope);
+        Assert.Equal("good-karma", username);
+        Assert.NotEqual("someone-else", username);
+        Assert.True(fullScope);
     }
 
     [Fact]
-    public void OnlyASelfViewEverSetsFullScope()
+    public void ANearMissNameDoesNotSwitchTheAccount()
     {
-        // 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);
+            var resolved = PrivateApi.ResolveNotificationsTarget("good-karma", other);
+            Assert.Equal("good-karma", resolved.Username);
+            Assert.True(resolved.FullScope);
         }
     }
 }
+
+/// <summary>
+/// The notifications handler itself: a code for one account cannot be pointed at
+/// another by the body's <c>user</c> field, and nothing is fetched when the code
+/// does not validate.
+/// </summary>
+[Collection("notifications-auth")]
+public class NotificationsHandlerTests : IDisposable
+{
+    private readonly CurationDeskTestSupport.Recorder _upstream = new();
+
+    public NotificationsHandlerTests()
+    {
+        // `code` "as:alice" validates as alice; anything else is invalid.
+        PrivateApi.NotificationsValidateCode = body =>
+        {
+            var code = body["code"]?.GetValue<string>();
+            return Task.FromResult(code != null && code.StartsWith("as:", StringComparison.Ordinal) ? code[3..] : null);
+        };
+        PrivateApi.NotificationsUpstream = (endpoint, method, headers) =>
+            _upstream.Handle(endpoint, method, headers ?? Array.Empty<KeyValuePair<string, string>>(), null);
+    }
+
+    public void Dispose()
+    {
+        PrivateApi.NotificationsValidateCode = PrivateApi.ValidateCode;
+        PrivateApi.NotificationsUpstream =
+            (endpoint, method, extraHeaders) =>
+                EcencyApi.Infrastructure.ApiClient.ApiRequest(endpoint, method, extraHeaders);
+    }
+
+    [Fact]
+    public async Task ACodeAloneFetchesThatAccountsCompleteFeed()
+    {
+        var ctx = CurationDeskTestSupport.Post(
+            "/private-api/notifications",
+            """{"code":"as:alice"}""");
+        await PrivateApi.Notifications(ctx);
+
+        Assert.Equal(200, ctx.Response.StatusCode);
+        var call = Assert.Single(_upstream.Calls);
+        Assert.Equal("activities/alice?scope=full", call.Endpoint);
+        Assert.Equal(HttpMethod.Get, call.Method);
+    }
+
+    [Theory]
+    [InlineData("alice")]
+    [InlineData("Alice")]
+    public async Task NamingTheCodesOwnAccountStillFetchesTheCode(string user)
+    {
+        var ctx = CurationDeskTestSupport.Post(
+            "/private-api/notifications",
+            $$"""{"code":"as:alice","user":"{{user}}"}""");
+        await PrivateApi.Notifications(ctx);
+
+        Assert.Equal(200, ctx.Response.StatusCode);
+        var call = Assert.Single(_upstream.Calls);
+        Assert.Equal("activities/alice?scope=full", call.Endpoint);
+    }
+
+    [Theory]
+    [InlineData("victim")]
+    [InlineData("victim/unread-count")]
+    [InlineData("victim?x=1")]
+    public async Task ABodyNamingAnotherAccountServesTheCodesAccount(string user)
+    {
+        var ctx = CurationDeskTestSupport.Post(
+            "/private-api/notifications",
+            $$"""{"code":"as:alice","user":"{{user}}","filter":"follows","limit":20}""");
+        await PrivateApi.Notifications(ctx);
+
+        Assert.Equal(200, ctx.Response.StatusCode);
+        var call = Assert.Single(_upstream.Calls);
+        Assert.Equal(HttpMethod.Get, call.Method);
+        Assert.Equal("follows/alice?limit=20&scope=full", call.Endpoint);
+        Assert.DoesNotContain("victim", call.Endpoint);
+    }
+
+    [Fact]
+    public async Task NamingAnAccountWithoutAValidCodeIsUnauthorized()
+    {
+        var ctx = CurationDeskTestSupport.Post(
+            "/private-api/notifications",
+            """{"code":"bogus","user":"victim"}""");
+        await PrivateApi.Notifications(ctx);
+
+        Assert.Equal(401, ctx.Response.StatusCode);
+        Assert.Equal("Unauthorized", CurationDeskTestSupport.Body(ctx));
+        Assert.Empty(_upstream.Calls);
+    }
+}
+
+[CollectionDefinition("notifications-auth", DisableParallelization = true)]
+public class NotificationsAuthCollection { }

dotnet/EcencyApi/Handlers/PrivateApi.UserData1.cs

@@ -13,15 +13,17 @@ public static partial class PrivateApi
     /// <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.
+    /// A null Username means unauthorized. The account is only the one
+    /// <c>ValidateCode</c> resolved — the same rule as <see cref="UnreadNotifications"/>.
+    /// <paramref name="requestedUser"/> is the body's <c>user</c> field after
+    /// JS-truthiness, or null when it was absent or falsy. It does not select the
+    /// account. It used to: an unauthenticated caller could pass the guard by naming
+    /// any account, and a valid code was then overwritten by that field with no
+    /// comparison (ecency/vision-api#90).
     ///
-    /// 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.
+    /// An authorized request is therefore always a self-view, so it may see the
+    /// complete feed. A body that names another account still reads the code's
+    /// account; it is not a way to ask for someone else's activity.
     /// </summary>
     public static (string? Username, bool FullScope) ResolveNotificationsTarget(
         string? validatedUsername, string? requestedUser)
@@ -31,20 +33,24 @@ public static (string? Username, bool FullScope) ResolveNotificationsTarget(
             return (null, false);
         }
 
-        if (requestedUser == null)
-        {
-            return (validatedUsername, true);
-        }
-
-        return (
-            requestedUser,
-            string.Equals(requestedUser, validatedUsername, StringComparison.OrdinalIgnoreCase));
+        // Kept on the signature so a caller can show a body name was supplied, and so
+        // a test can fail if that name starts being returned.
+        _ = requestedUser;
+        return (validatedUsername, true);
     }
 
+    /// <summary>Signed-code validation for the notifications feed, replaceable for tests (no chain RPC).</summary>
+    internal static Func<JsonObject, Task<string?>> NotificationsValidateCode = ValidateCode;
+
+    /// <summary>The notifications feed's upstream call, replaceable so tests can observe the path.</summary>
+    internal static Func<string, HttpMethod, IEnumerable<KeyValuePair<string, string>>?, Task<UpstreamResponse>>
+        NotificationsUpstream =
+            (endpoint, method, extraHeaders) => ApiClient.ApiRequest(endpoint, method, extraHeaders);
+
     /// <summary>
     /// Whether a device request may act on `requestedUsername`. A push registration
     /// belongs to the account the code was issued for, so the device endpoints only
-    /// accept that account. Same case rule as ResolveNotificationsTarget.
+    /// accept that account. Comparison is case-insensitive, matching Hive names.
     /// </summary>
     public static bool IsOwnDeviceRequest(string? validatedUsername, string? requestedUsername) =>
         !string.IsNullOrEmpty(validatedUsername)
@@ -98,8 +104,9 @@ public static bool IsOwnDeviceRequest(string? validatedUsername, string? request
         }
 
         // 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.
+        // so omitting this is the safe direction: any request that does not ask,
+        // including one that never reaches this handler, gets the restricted feed.
+        // This handler asks only after a code has validated, and only for that account.
         if (fullScope)
         {
             query.Add("scope=full");
@@ -116,8 +123,10 @@ public static async Task Notifications(HttpContext ctx)
 
         // 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.
+        // The body's user is passed in so the resolver can ignore it on purpose:
+        // it must not become the account that is queried.
         var (username, fullScope) = ResolveNotificationsTarget(
-            await ValidateCode(body),
+            await NotificationsValidateCode(body),
             JsJson.IsTruthy(user) ? UserData1Helpers.Template(user) : null);
 
         if (username == null)
@@ -150,7 +159,7 @@ await ValidateCode(body),
             ? new[] { new KeyValuePair<string, string>(EnotifyInternalTokenHeader, Config.EnotifyInternalToken) }
             : null;
 
-        await Upstream.Pipe(ApiClient.ApiRequest(u, HttpMethod.Get, extraHeaders), ctx);
+        await Upstream.Pipe(NotificationsUpstream(u, HttpMethod.Get, extraHeaders), ctx);
     }
 
     // GET ^/private-api/pub-notifications/:username