Fix daily challenge not querying beatmap properly#32596
Fix daily challenge not querying beatmap properly#32596smoogipoo merged 5 commits intoppy:masterfrom
Conversation
Noticed during review of ppy#32571. The reproduction scenario is as follows: 1. Download beatmap used in daily challenge 2. Go to editor and modify it 3. Go to daily challenge, wherein the availability tracker will notice the MD5 mismatch, block the button, and require a redownload 4. Redownload the beatmap 5. Start gameplay 6. Gameplay start will fail due to web not issuing a submission token because the attempt to start gameplay ended up using the modified version of the map from step (2) rather than the redownloaded original from step (4). Thankfully, due to (6), this is not exploitable, but nevertheless pretty bad. Probably regressed somewhere around ppy#31747 actually.
|
Does this mean it's also an issue for playlists and multiplayer (new implementation)? |
|
For whatever reason it did not appear to be in testing, but I shall double check because I can't see why it would not be from these snippets... |
|
Aside from the default values (from Score submission should be alright in either case because we create scores with both values. Unless I'm missing something, does that sound about right? |
bdach
force-pushed
the
fix-bogus-daily-challenge-lookup
branch
from
6c378be to
f3524ad
Compare
|
(please disregard force push above, mis-pushed) |
…aily challenge (and expand to other online play modes)
|
Okay, to answer your questions properly:
I dunno how I was testing this, but indeed it is an issue there also. I was perhaps misled by the fact that a lot of the screen looks correct because it's using mostly online metadata, but the issue does appear when you try to advance to gameplay.
Yes, score submission is not affected, and user style would be a problem. However, because we are protected from this server-side anyway, and as such this is mostly a client-side inconvenience (which is mostly inscrutable to end users), I'm kinda thinking that maybe we can get away with something like f736015 which is completely doable client-side and we don't have to involve freestyle into that conversation at all with that fix. Let me know what you think. If you think that's acceptable then maybe we want to be applying that same which would make testing this much easier too. |
| return; | ||
|
|
||
| var beatmap = beatmaps.QueryBeatmap(b => b.OnlineID == item.Beatmap.OnlineID); | ||
| var beatmap = beatmaps.QueryBeatmap($@"{nameof(BeatmapInfo.OnlineID)} = $0 AND {nameof(BeatmapInfo.MD5Hash)} = {nameof(BeatmapInfo.OnlineMD5Hash)}", item.Beatmap.OnlineID); |
There was a problem hiding this comment.
Maybe we should use == to keep things consistent?
|
I've fixed tests more or less by applying local workarounds as necessary. Also let me know if you'd want to see the following done:
because it's not clear to me whether I should follow up on that (proooobably in a separate PR?) |
|
Fixes look fine to me. That followup sounds good to apply in a separate PR (just in case there's even more test breakage). |
2 participants
Noticed during review of #32571 (cc @smoogipoo).
The reproduction scenario is as follows:
Thankfully, due to (6), this is not exploitable (so I'm not calling for a cavalry charge here), but nevertheless pretty bad.
Probably regressed somewhere around #31747 actually.
Not sure whether this requires tests or not, will attempt on request.