mirror of
https://github.com/DorkSquadRR/DorkNet.git
synced 2026-10-25 15:08:47 -07:00
fix: address the 15 CodeRabbit findings from the wire-audit review
Security:
- roomcode PUT now requires instance ownership (any player could plant
or relocate codes on foreign private instances)
- /platformid/{accountId} requires auth (anonymous Steam/Oculus id
enumeration oracle)
- request tracing no longer captures successful /thread bodies by
default (chat messages landed unredacted in logs); opt in via
Trace__SuccessBodyPaths
- anonymous action-link renewal clamped to 720h per consume
Data integrity:
- buyPurchaseReminder upserts its objective sentinel (second purchase
threw UNIQUE after the wallet was debited)
- progression reward gift + idempotency sentinel commit in one
transaction (crash between them double-minted the reward)
- ClubPermissionsGalleryAndModeration migration defaults corrected to
the entity contract (CommentsJson '[]', ClubChatEnabled true); new
one-shot ClubChatDefaultRepair migration fixes databases that
applied the wrong defaults; bootstrap repairs '' CommentsJson rows
- admin subroom-maxplayers audit entry commits with the mutation
- account purge covers CustomAvatarItems (ownership deleted, created
items reassigned to the system account)
- duplicate photo reports ignored while one is unresolved
Stability:
- bootstrap only swallows the expected duplicate-column error; locked
databases and schema failures now fail startup instead of the first
endpoint
- clone blob write failure degrades to an empty sub-room instead of a
500 after the clone row committed
- resumed photo-feed cursor stamped DateTimeKind.Utc (Npgsql rejects
Unspecified for timestamptz)
- unreadcounts queries per requested room instead of materialising the
whole comment keyspace
Tests:
- destroys-the-probe-session template guard is case-insensitive
74/74 tests pass.
This commit is contained in:
@@ -153,22 +153,30 @@ public class CommentsController(DorkNetDbContext db) : ControllerBase
|
||||
var me = Me;
|
||||
var lastRead = await LastReadAsync(me);
|
||||
|
||||
// Key + owner only: the payload column is irrelevant to counting.
|
||||
var keys = await db.PlayerSettings
|
||||
.Where(s => s.Key.StartsWith(CommentPrefix))
|
||||
.Select(s => new { s.Key, s.PlayerId })
|
||||
.ToListAsync();
|
||||
|
||||
var counts = new Dictionary<long, uint>();
|
||||
foreach (var id in roomIds) counts[id] = 0u;
|
||||
|
||||
foreach (var k in keys)
|
||||
// One bounded query per requested room instead of materialising the
|
||||
// entire comment keyspace: the room filter and the "your own notes
|
||||
// are never unread" filter both run in SQL; only the numeric
|
||||
// comment-id suffix still needs parsing here. Key + owner only: the
|
||||
// payload column is irrelevant to counting.
|
||||
foreach (var id in roomIds)
|
||||
{
|
||||
if (!TrySplitCommentKey(k.Key, out var roomId, out var commentId)) continue;
|
||||
if (!counts.ContainsKey(roomId)) continue;
|
||||
if (k.PlayerId == me) continue; // your own notes are never unread
|
||||
if (commentId <= lastRead.GetValueOrDefault(roomId)) continue;
|
||||
counts[roomId]++;
|
||||
var prefix = $"{CommentPrefix}{id}:";
|
||||
var keys = await db.PlayerSettings
|
||||
.Where(s => s.Key.StartsWith(prefix) && s.PlayerId != me)
|
||||
.Select(s => s.Key)
|
||||
.ToListAsync();
|
||||
|
||||
var floor = lastRead.GetValueOrDefault(id);
|
||||
foreach (var key in keys)
|
||||
{
|
||||
if (!TrySplitCommentKey(key, out var roomId, out var commentId)) continue;
|
||||
if (roomId != id) continue;
|
||||
if (commentId <= floor) continue;
|
||||
counts[id]++;
|
||||
}
|
||||
}
|
||||
|
||||
var result = roomIds
|
||||
|
||||
@@ -584,6 +584,20 @@ public class ImagesController(
|
||||
// ReportEntity has no TargetPhotoId column, so the photo id + blob name
|
||||
// ride in the message text the same way club reports carry "[club {id}]"
|
||||
// — a moderator needs the actual image to act on the report.
|
||||
//
|
||||
// One open report per (reporter, photo): the call is fire-and-forget
|
||||
// and the client can't tell 200 from 404, so retries and re-opens are
|
||||
// likely — same guard as the custom-avatar-item report path.
|
||||
var marker = $"[photo {photo.Id} ";
|
||||
var duplicate = await db.Reports.AnyAsync(r =>
|
||||
r.ReporterPlayerId == reporter && r.ResolvedAt == null && r.Message.StartsWith(marker));
|
||||
if (duplicate)
|
||||
{
|
||||
logger.LogInformation(
|
||||
"[images] duplicate report of photo {PhotoId} by {Reporter} ignored", photo.Id, reporter);
|
||||
return Ok(new RecNetResult { Success = true, Error = string.Empty });
|
||||
}
|
||||
|
||||
var message = $"[photo {photo.Id} {photo.BlobName}] reported from the in-game photo viewer";
|
||||
db.Reports.Add(new ReportEntity
|
||||
{
|
||||
|
||||
@@ -401,8 +401,10 @@ public class PhotosController(
|
||||
itemIds.AddRange(injectedIds.Where(pinned.Contains));
|
||||
}
|
||||
|
||||
// Utc kind explicitly: the ticks came from a UtcNow-derived CreatedAt,
|
||||
// and Npgsql refuses to write an Unspecified DateTime to timestamptz.
|
||||
DateTime? cutoffAt = resumed && cursorTicks > 0 && cursorTicks <= DateTime.MaxValue.Ticks
|
||||
? new DateTime(cursorTicks)
|
||||
? new DateTime(cursorTicks, DateTimeKind.Utc)
|
||||
: (DateTime?)null;
|
||||
var cutoffId = resumed ? cursorId : 0L;
|
||||
if (phase < 0) phase = 0;
|
||||
|
||||
@@ -226,6 +226,11 @@ public class ProgressionEventsController(
|
||||
IsValid = true,
|
||||
SupportsCurrentPlatform = true,
|
||||
};
|
||||
// The sentinel key needs the generated gift id, so this takes two
|
||||
// saves — but they must commit together: a crash between them would
|
||||
// leave a gift with no sentinel, and the next Collect would mint the
|
||||
// reward a second time.
|
||||
await using var tx = await db.Database.BeginTransactionAsync();
|
||||
db.GiftPackages.Add(gift);
|
||||
await db.SaveChangesAsync();
|
||||
|
||||
@@ -237,6 +242,7 @@ public class ProgressionEventsController(
|
||||
ClearedAt = DateTime.UtcNow,
|
||||
});
|
||||
await db.SaveChangesAsync();
|
||||
await tx.CommitAsync();
|
||||
|
||||
return Ok(global::DorkNet.Server.Controllers.API.Avatar.V2.AvatarGiftsController.ToWire(gift));
|
||||
}
|
||||
|
||||
@@ -1642,7 +1642,21 @@ public class RoomsModerationController(
|
||||
|
||||
var newBlob = CloneBlobName(cloneId, orderIndex);
|
||||
var (dstBucket, dstKey) = BlobRouter.Route(newBlob);
|
||||
await storage.PutAsync(dstBucket, dstKey, bytes, "application/octet-stream");
|
||||
try
|
||||
{
|
||||
await storage.PutAsync(dstBucket, dstKey, bytes, "application/octet-stream");
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
// Same reasoning as the read side: the clone row is already
|
||||
// committed, so a storage outage must degrade to an empty
|
||||
// sub-room, not a 500 that leaves a failed-looking clone still
|
||||
// pointing at the source's data.
|
||||
logger.LogWarning(ex,
|
||||
"[room-clone] could not write blob {Blob}; room {Room} sub-room {Sub} starts empty",
|
||||
newBlob, cloneId, orderIndex);
|
||||
return null;
|
||||
}
|
||||
|
||||
db.RoomDataBlobs.Add(new RoomDataBlobEntity
|
||||
{
|
||||
|
||||
@@ -654,13 +654,28 @@ public class StorefrontsBuyController(
|
||||
$"buyPurchaseReminder:{item.Slug}");
|
||||
var giftId = await GrantWithGiftBoxAsync(pid, item, currencyType);
|
||||
|
||||
db.ObjectiveProgress.Add(new ObjectiveProgressEntity
|
||||
// Upsert: (PlayerId, Key) is unique and the same reminder can be
|
||||
// bought again — a blind Add threw "UNIQUE constraint failed" and
|
||||
// turned the second purchase into a 500 AFTER the currency had
|
||||
// already been taken and the gift saved.
|
||||
var progressKey = $"purchaseReminder:{req.PurchaseReminderId}";
|
||||
var progress = await db.ObjectiveProgress
|
||||
.FirstOrDefaultAsync(o => o.PlayerId == pid && o.Key == progressKey);
|
||||
if (progress is null)
|
||||
{
|
||||
PlayerId = pid,
|
||||
Key = $"purchaseReminder:{req.PurchaseReminderId}",
|
||||
IsCompleted = true,
|
||||
ClearedAt = DateTime.UtcNow,
|
||||
});
|
||||
db.ObjectiveProgress.Add(new ObjectiveProgressEntity
|
||||
{
|
||||
PlayerId = pid,
|
||||
Key = progressKey,
|
||||
IsCompleted = true,
|
||||
ClearedAt = DateTime.UtcNow,
|
||||
});
|
||||
}
|
||||
else
|
||||
{
|
||||
progress.IsCompleted = true;
|
||||
progress.ClearedAt = DateTime.UtcNow;
|
||||
}
|
||||
await db.SaveChangesAsync();
|
||||
|
||||
return Ok(PurchaseReminderResponse(newBalance, currencyType, item.Slug, giftId));
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
using Microsoft.AspNetCore.Authorization;
|
||||
using Microsoft.AspNetCore.Mvc;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using DorkNet.Models.Auth;
|
||||
@@ -808,8 +809,12 @@ public class AccountsController(
|
||||
/// Unknown accounts get an empty string — "no platform identity" is the
|
||||
/// honest answer and keeps the promise resolved rather than rejected.
|
||||
/// </summary>
|
||||
// LastPlatformId is a third-party identifier (Steam/Oculus id); without
|
||||
// an auth gate this was an anonymous enumeration oracle over every
|
||||
// account. The 2023 client only ever calls it with a token.
|
||||
[HttpGet("/platformid/{accountId:long}")]
|
||||
[HttpGet("/account/v1/{accountId:long}/platformid")]
|
||||
[Authorize]
|
||||
public async Task<IActionResult> GetPlatformId(long accountId)
|
||||
{
|
||||
var platformId = await db.Players
|
||||
|
||||
@@ -1158,10 +1158,12 @@ public class AdminController(
|
||||
var previous = scene.MaxPlayers;
|
||||
scene.MaxPlayers = Math.Clamp(body.MaxPlayers, 1, 80);
|
||||
scene.DataModifiedAt = DateTime.UtcNow;
|
||||
await db.SaveChangesAsync();
|
||||
|
||||
// LogAsync only stages the AdminActionEntity; save afterwards so the
|
||||
// capacity change and its audit record commit together.
|
||||
await LogAsync("update_subroom_maxplayers", "room", id,
|
||||
$"subRoom={subRoomId} maxPlayers {previous} -> {scene.MaxPlayers}");
|
||||
await db.SaveChangesAsync();
|
||||
|
||||
return Ok(new
|
||||
{
|
||||
|
||||
@@ -223,7 +223,11 @@ public class LinkController(DorkNetDbContext db, ILogger<LinkController> logger)
|
||||
|
||||
payload.Uses++;
|
||||
// AutoRenewHours present ⇒ the code renews rather than ages out.
|
||||
if (req.ValidHours is > 0) payload.ExpiresAt = DateTime.UtcNow.AddHours(req.ValidHours.Value);
|
||||
// Clamped: this route is anonymous and the value is client-supplied,
|
||||
// so an unbounded renewal would let anyone holding the code extend
|
||||
// its life indefinitely past what the creator chose.
|
||||
if (req.ValidHours is > 0)
|
||||
payload.ExpiresAt = DateTime.UtcNow.AddHours(Math.Min(req.ValidHours.Value, 720));
|
||||
|
||||
var encoded = JsonSerializer.Serialize(payload);
|
||||
if (encoded.Length <= ValueLimit) row.Value = encoded;
|
||||
|
||||
@@ -396,14 +396,19 @@ public class MatchController(
|
||||
.Select(p => new { p.Id, p.OwnerPlayerId })
|
||||
.FirstOrDefaultAsync();
|
||||
|
||||
// forceChange only lets a host reclaim a code from one of their OWN
|
||||
// instances; another player's code is never stolen. On refusal the
|
||||
// client just re-reads whatever code we echo back, so we fall
|
||||
// through to the read path instead of erroring.
|
||||
var blocked = clash is not null
|
||||
&& (clash.OwnerPlayerId != pid || forceChange != true);
|
||||
|
||||
var row = await db.PrivateInstances.FirstOrDefaultAsync(p => p.Id == instanceId);
|
||||
|
||||
// Only the instance's owner may set its code (same guard as
|
||||
// MarkPrivate), otherwise any player could plant or relocate a
|
||||
// code onto someone else's instance. forceChange only lets a host
|
||||
// reclaim a code from one of their OWN instances; another
|
||||
// player's code is never stolen. On refusal the client just
|
||||
// re-reads whatever code we echo back, so we fall through to the
|
||||
// read path instead of erroring.
|
||||
var blocked = row is null || row.OwnerPlayerId != pid
|
||||
|| (clash is not null
|
||||
&& (clash.OwnerPlayerId != pid || forceChange != true));
|
||||
|
||||
if (!blocked && row is not null)
|
||||
{
|
||||
if (clash is not null)
|
||||
|
||||
Reference in New Issue
Block a user