Skip to content

Commit 776fabd

Browse files
committed
Only use MD5 when performing metadata lookups
Both online and offline using the cache. The rationale behind this change is that in the current state of affairs, `TestPartiallyMaliciousSet()` fails in a way that cannot be reconciled without this sort of change. The test exercises a scenario where the beatmap being imported has an online ID in the `.osu` file, but its hash does not match the online hash of the beatmap. This turns out to be a more frequent scenario than envisioned because of users doing stupid things with manual file editing rather than reporting issues properly. The scenario is realistic only because the behaviour of the endpoint responsible for looking up beatmaps is such that if multiple parameters are given (e.g. all three of beatmap MD5, online ID, and filename), it will try the three in succession: https://github.com/ppy/osu-web/blob/f6b341813be270de59ec8f9698428aa6422bc8ae/app/Http/Controllers/BeatmapsController.php#L260-L266 and the local metadata cache implementation reflected this implementation. Because online ID and filename are inherently unreliable in this scenario due to being directly manipulable by clueless or malicious users, neither should not be used as a fallback.
1 parent 1a2e323 commit 776fabd

File tree

4 files changed

+20
-18
lines changed

4 files changed

+20
-18
lines changed

osu.Game/Beatmaps/APIBeatmapMetadataSource.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ public bool TryLookup(BeatmapInfo beatmapInfo, out OnlineBeatmapMetadata? online
3333

3434
Debug.Assert(beatmapInfo.BeatmapSet != null);
3535

36-
var req = new GetBeatmapRequest(beatmapInfo);
36+
var req = new GetBeatmapRequest(beatmapInfo.MD5Hash);
3737

3838
try
3939
{

osu.Game/Beatmaps/LocalCachedBeatmapMetadataSource.cs

+3-9
Original file line numberDiff line numberDiff line change
@@ -89,9 +89,7 @@ public bool TryLookup(BeatmapInfo beatmapInfo, [NotNullWhen(true)] out OnlineBea
8989
return false;
9090
}
9191

92-
if (string.IsNullOrEmpty(beatmapInfo.MD5Hash)
93-
&& string.IsNullOrEmpty(beatmapInfo.Path)
94-
&& beatmapInfo.OnlineID <= 0)
92+
if (string.IsNullOrEmpty(beatmapInfo.MD5Hash))
9593
{
9694
onlineMetadata = null;
9795
return false;
@@ -240,11 +238,9 @@ private bool queryCacheVersion1(SqliteConnection db, BeatmapInfo beatmapInfo, ou
240238
using var cmd = db.CreateCommand();
241239

242240
cmd.CommandText =
243-
@"SELECT beatmapset_id, beatmap_id, approved, user_id, checksum, last_update FROM osu_beatmaps WHERE checksum = @MD5Hash OR beatmap_id = @OnlineID OR filename = @Path";
241+
@"SELECT beatmapset_id, beatmap_id, approved, user_id, checksum, last_update FROM osu_beatmaps WHERE checksum = @MD5Hash";
244242

245243
cmd.Parameters.Add(new SqliteParameter(@"@MD5Hash", beatmapInfo.MD5Hash));
246-
cmd.Parameters.Add(new SqliteParameter(@"@OnlineID", beatmapInfo.OnlineID));
247-
cmd.Parameters.Add(new SqliteParameter(@"@Path", beatmapInfo.Path));
248244

249245
using var reader = cmd.ExecuteReader();
250246

@@ -281,12 +277,10 @@ private bool queryCacheVersion2(SqliteConnection db, BeatmapInfo beatmapInfo, ou
281277
SELECT `b`.`beatmapset_id`, `b`.`beatmap_id`, `b`.`approved`, `b`.`user_id`, `b`.`checksum`, `b`.`last_update`, `s`.`submit_date`, `s`.`approved_date`
282278
FROM `osu_beatmaps` AS `b`
283279
JOIN `osu_beatmapsets` AS `s` ON `s`.`beatmapset_id` = `b`.`beatmapset_id`
284-
WHERE `b`.`checksum` = @MD5Hash OR `b`.`beatmap_id` = @OnlineID OR `b`.`filename` = @Path
280+
WHERE `b`.`checksum` = @MD5Hash
285281
""";
286282

287283
cmd.Parameters.Add(new SqliteParameter(@"@MD5Hash", beatmapInfo.MD5Hash));
288-
cmd.Parameters.Add(new SqliteParameter(@"@OnlineID", beatmapInfo.OnlineID));
289-
cmd.Parameters.Add(new SqliteParameter(@"@Path", beatmapInfo.Path));
290284

291285
using var reader = cmd.ExecuteReader();
292286

osu.Game/Online/API/Requests/GetBeatmapRequest.cs

+15-7
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
22
// See the LICENCE file in the repository root for full licence text.
33

4+
using System.Globalization;
45
using osu.Framework.IO.Network;
56
using osu.Game.Beatmaps;
67
using osu.Game.Online.API.Requests.Responses;
@@ -9,23 +10,30 @@ namespace osu.Game.Online.API.Requests
910
{
1011
public class GetBeatmapRequest : APIRequest<APIBeatmap>
1112
{
12-
public readonly IBeatmapInfo BeatmapInfo;
13-
public readonly string Filename;
13+
public readonly int OnlineID = -1;
14+
public readonly string? MD5Hash;
15+
public readonly string? Filename;
1416

1517
public GetBeatmapRequest(IBeatmapInfo beatmapInfo)
1618
{
17-
BeatmapInfo = beatmapInfo;
19+
OnlineID = beatmapInfo.OnlineID;
20+
MD5Hash = beatmapInfo.MD5Hash;
1821
Filename = (beatmapInfo as BeatmapInfo)?.Path ?? string.Empty;
1922
}
2023

24+
public GetBeatmapRequest(string md5Hash)
25+
{
26+
MD5Hash = md5Hash;
27+
}
28+
2129
protected override WebRequest CreateWebRequest()
2230
{
2331
var request = base.CreateWebRequest();
2432

25-
if (BeatmapInfo.OnlineID > 0)
26-
request.AddParameter(@"id", BeatmapInfo.OnlineID.ToString());
27-
if (!string.IsNullOrEmpty(BeatmapInfo.MD5Hash))
28-
request.AddParameter(@"checksum", BeatmapInfo.MD5Hash);
33+
if (OnlineID > 0)
34+
request.AddParameter(@"id", OnlineID.ToString(CultureInfo.InvariantCulture));
35+
if (!string.IsNullOrEmpty(MD5Hash))
36+
request.AddParameter(@"checksum", MD5Hash);
2937
if (!string.IsNullOrEmpty(Filename))
3038
request.AddParameter(@"filename", Filename);
3139

osu.Game/Tests/Visual/OnlinePlay/TestRoomRequestsHandler.cs

+1-1
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ public bool HandleRequest(APIRequest request, APIUser localUser, BeatmapManager
188188

189189
case GetBeatmapRequest getBeatmapRequest:
190190
{
191-
getBeatmapRequest.TriggerSuccess(createResponseBeatmaps(getBeatmapRequest.BeatmapInfo.OnlineID).Single());
191+
getBeatmapRequest.TriggerSuccess(createResponseBeatmaps(getBeatmapRequest.OnlineID).Single());
192192
return true;
193193
}
194194

0 commit comments

Comments
 (0)