Fix some image attachments in comments not loading in game #30866
Conversation
osu.Game/Online/OsuOnlineStore.cs
Outdated
| if (Uri.TryCreate(url, UriKind.Absolute, out Uri? uri) && uri.Host.EndsWith(@".ppy.sh", StringComparison.OrdinalIgnoreCase)) | ||
| return url; | ||
|
|
||
| return $@"{apiEndpointUrl}/beatmapsets/discussions/media-url?url={url}"; |
There was a problem hiding this comment.
This conditional may technically be pointless, but I felt proxying .ppy.sh links is too much proxying, and also makes network logs look ugly.
| if (Uri.TryCreate(url, UriKind.Absolute, out Uri? uri) && uri.Host.EndsWith(@".ppy.sh", StringComparison.OrdinalIgnoreCase)) | ||
| return url; | ||
|
|
||
| return $@"{apiEndpointUrl}/beatmapsets/discussions/media-url?url={url}"; |
There was a problem hiding this comment.
not sure if this is a big deal, but it feels wrong to me for lazer to be explicitly depending on a non-api route, even if it will still function properly in this case. and it should probably be updated to not have a uri specific to discussions 🤔
There was a problem hiding this comment.
I'm not entirely sure what the concern is with a non-API route. As for the definition of the endpoint looking like it's specific to discussions, that's probably something to bring to @ppy/team-web, if action is deemed necessary and feasible.
There was a problem hiding this comment.
Yeah I dunno about this.
I believe that endpoint should only be used for discussion links (are we somehow limiting this? could an abusive user be using this endpoint to create arbitrary proxied links?). If we're using it for more than this it should be something more generalised.
Can the store be used local to the discussions/comments system? And the default osu!-side online store just blocks all non-ppy resources?
There was a problem hiding this comment.
Sounds feasible, will update as such.
There was a problem hiding this comment.
I believe that endpoint should only be used for discussion links (are we somehow limiting this? could an abusive user be using this endpoint to create arbitrary proxied links?)
I guess this is more of a web issue then...
There was a problem hiding this comment.
I am so baffled as to how things got as far as this PR without talking about any of the concerns with using the route in question in such an undocumented manner first.
To me this either should have not been ever PR'd, or should have at least been preempted with a "is this even ok to do" sanity check.
Wholehearted agreement with @cl8n, and if anything I think that remark was too subtle. This 100% feels too broken to live and is susceptible to breakage in the future because no sane person would assume the discussions route would be used by client in contexts it is clearly not supposed to be used in.
The route either needs to be somehow generalised, or the entire burden of doing this should be offloaded to web by doing something like replacing the broken URLs with the proxied URLs on the server side so that the proxying is transparent to the client.
There was a problem hiding this comment.
hm yeah, I'm thinking of changing the url to something more generic (ppy/osu-web#11686). Dunno if api will need its own specific url under /api but I suppose it can be added...
There was a problem hiding this comment.
@bdach I think you've extremely raised the tone up there, I will say I have chose to open this PR so that if it's alright then it's alright and if it's not then the points of why and whatever are discussed appropriately in here with easier access to the diff, period.
|
As per #30866 (comment), resource lookups from external websites are now blocked (with reasonably loud warning logs), and an exception is given to osu! markdown images which are used by comments, wiki, and future overlays to come such as beatmap discussions and news. The warning logs can turn into errors to grab our attention via sentry, but perhaps that's an overkill. |
Did you test changelogs? Specifically on production? I have a suspicion they would be broken by this. |
|
Huh, I thought changelogs have been using markdown already. Thankfully it doesn't do any kind of online link lookups so it's not broken by this. osu/osu.Game/Overlays/Changelog/ChangelogEntry.cs Lines 187 to 198 in d179fe3 |
|
I meant images embedded in changelogs...? Are you saying those just don't display? |
|
Yes, the code above shows that everything in the changelogs is being treated as text and nothing but that. |
|
Am I hallucinating or did images used to be a thing on the changelog overlay? |
Well there's #12465, which is probably a definite proof that we never had support for it in the first place. |
s-ul.eudo not show in game #30791OnlineStoreosu-framework#6433End result can be tested by viewing comments such as this one, where previously the game will not load such images, but now it should.
I was initially going to implement this locally in
OsuMarkdownImages (more or less similar to how osu-web did it), but I figured it would be a more solid approach to cover the entireOnlineStorelayer instead, since the end goal is to avoid looking up resources from third-party links.