-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix some image attachments in comments not loading in game #30866
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This conditional may technically be pointless, but I felt proxying .ppy.sh
links is too much proxying, and also makes network logs look ugly.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds feasible, will update as such.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll point out that the media-url proxying on discussions is mainly so someone can't just drop in an image for link tracking or fingerprinting users and only partly for caching if the host falls over. If there's a way to do that without proxying, that would be preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the proxying happens on all user posted images...
media-url thing is specifically because the markdown parsing happens on frontend so it doesn't have the camo key and can't just generate the actual proxied link like in backend.
it's currently only beatmapset discussions it happens on frontend (on web) so that's why it's been sitting there.
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. |
Links are checked to be in the ppy.sh domain here.
s-ul.eu
do not show in game #30791OnlineStore
osu-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
OsuMarkdownImage
s (more or less similar to how osu-web did it), but I figured it would be a more solid approach to cover the entireOnlineStore
layer instead, since the end goal is to avoid looking up resources from third-party links.