-
-
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?
Changes from all commits
2417d4d
2a7133d
9a89d40
83f8fa7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence. | ||
// See the LICENCE file in the repository root for full licence text. | ||
|
||
using System; | ||
using NUnit.Framework; | ||
using osu.Framework.Allocation; | ||
using osu.Framework.Graphics; | ||
using osu.Framework.Graphics.Textures; | ||
using osu.Framework.Platform; | ||
using osu.Game.Graphics.Containers.Markdown; | ||
using osu.Game.Online; | ||
|
||
namespace osu.Game.Tests.Visual.Online | ||
{ | ||
public partial class TestSceneImageProxying : OsuTestScene | ||
{ | ||
[Resolved] | ||
private GameHost host { get; set; } = null!; | ||
|
||
[Test] | ||
public void TestExternalImageLink() | ||
{ | ||
AddStep("load image", () => Child = new OsuMarkdownContainer | ||
{ | ||
RelativeSizeAxes = Axes.Both, | ||
Text = "![](https://github.com/ppy/osu-wiki/blob/master/wiki/Announcement_messages/img/notification.png?raw=true)", | ||
}); | ||
} | ||
|
||
[Test] | ||
public void TestLocalImageLink() | ||
{ | ||
AddStep("load image", () => Child = new OsuMarkdownContainer | ||
{ | ||
RelativeSizeAxes = Axes.Both, | ||
Text = "![](https://osu.ppy.sh/help/wiki/shared/news/banners/monthly-beatmapping-contest.png)", | ||
}); | ||
} | ||
|
||
[Test] | ||
public void TestInvalidImageLink() | ||
{ | ||
AddStep("load image", () => Child = new OsuMarkdownContainer | ||
{ | ||
RelativeSizeAxes = Axes.Both, | ||
Text = "![](https://this-site-does-not-exist.com/img.png)", | ||
}); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence. | ||
// See the LICENCE file in the repository root for full licence text. | ||
|
||
using System; | ||
using osu.Framework.IO.Stores; | ||
using osu.Framework.Logging; | ||
|
||
namespace osu.Game.Online | ||
{ | ||
public class OsuOnlineStore : OnlineStore | ||
{ | ||
private readonly string apiEndpointUrl; | ||
|
||
public OsuOnlineStore(string apiEndpointUrl) | ||
{ | ||
this.apiEndpointUrl = apiEndpointUrl; | ||
} | ||
|
||
protected override string GetLookupUrl(string url) | ||
Check failure on line 19 in osu.Game/Online/OsuOnlineStore.cs GitHub Actions / Code Quality
Check failure on line 19 in osu.Game/Online/OsuOnlineStore.cs GitHub Actions / Code Quality
Check failure on line 19 in osu.Game/Online/OsuOnlineStore.cs GitHub Actions / Test (Windows, windows-latest, SingleThread)
Check failure on line 19 in osu.Game/Online/OsuOnlineStore.cs GitHub Actions / Test (Windows, windows-latest, SingleThread)
Check failure on line 19 in osu.Game/Online/OsuOnlineStore.cs GitHub Actions / Build only (iOS)
Check failure on line 19 in osu.Game/Online/OsuOnlineStore.cs GitHub Actions / Build only (iOS)
Check failure on line 19 in osu.Game/Online/OsuOnlineStore.cs GitHub Actions / Test (Windows, windows-latest, MultiThreaded)
Check failure on line 19 in osu.Game/Online/OsuOnlineStore.cs GitHub Actions / Test (Windows, windows-latest, MultiThreaded)
Check failure on line 19 in osu.Game/Online/OsuOnlineStore.cs GitHub Actions / Test (Linux, ubuntu-latest, SingleThread)
Check failure on line 19 in osu.Game/Online/OsuOnlineStore.cs GitHub Actions / Test (Linux, ubuntu-latest, SingleThread)
Check failure on line 19 in osu.Game/Online/OsuOnlineStore.cs GitHub Actions / Test (Linux, ubuntu-latest, MultiThreaded)
|
||
{ | ||
if (!Uri.TryCreate(url, UriKind.Absolute, out Uri? uri) || !uri.Host.EndsWith(@".ppy.sh", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
Logger.Log($@"Blocking resource lookup from external website: {url}", LoggingTarget.Network, LogLevel.Important); | ||
return string.Empty; | ||
} | ||
|
||
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.
https://i.ppy.sh/b5810b2de4431c96218200725bcdb795376c780f/68747470733a2f2f7570322e636c6179746f6e2e63632f6970707973682e706e67
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.