Skip to content
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

Add Media Resolver #427

Merged
merged 12 commits into from
Feb 11, 2023
Merged

Add Media Resolver #427

merged 12 commits into from
Feb 11, 2023

Conversation

nuuls
Copy link
Contributor

@nuuls nuuls commented Feb 1, 2023

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Add Tooltip-only Resolver for previously unsupported Media files. Particularly useful for file uploaders that do not show the file extension.

image

@codecov
Copy link

codecov bot commented Feb 1, 2023

Codecov Report

Merging #427 (9ae43ed) into master (828a42d) will increase coverage by 1.03%.
The diff coverage is 89.13%.

@@            Coverage Diff             @@
##           master     #427      +/-   ##
==========================================
+ Coverage   44.62%   45.66%   +1.03%     
==========================================
  Files         101      103       +2     
  Lines        3742     3832      +90     
==========================================
+ Hits         1670     1750      +80     
- Misses       2024     2032       +8     
- Partials       48       50       +2     
Impacted Files Coverage Δ
internal/resolvers/default/media_resolver.go 87.17% <87.17%> (ø)
internal/resolvers/default/link_resolver.go 45.62% <100.00%> (+0.76%) ⬆️
pkg/humanize/bytes.go 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pajlada pajlada self-requested a review February 1, 2023 11:55
@pajlada
Copy link
Member

pajlada commented Feb 5, 2023

I would want to think a bit more what data makes sense to pass along to the user.
Size of the file is good, the humanize/bytes class makes sense for this & the tests are nice.
Type of file is good, I would like to know whether the link I'm clicking is going to be an audio file, a video file, or something else.
I don't personally care for the file extension (unless maybe as an extra to the type)
so maybe the tooltip can instead be simplified to two lines:
File type: Video (MP4)
Size: 13.4 MB

File type: Image (PNG)
Size: 2.3 MB

File Type: Text (JSON)?
Size: 0.3 KB

Some tests for the media resolver to ensure we test the various media types we thing are most important would be nice

response := &resolver.Response{
Status: http.StatusOK,
Link: targetURL,
Tooltip: tooltip.String(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the PathEscape here and it still works fine in Chatterino. Is there any actual need for it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be necessary to make sure any already-escaped strings aren't unescaped by Chatterino.
When Chatterino parses this tooltip it runs a single unescape path.

If we don't call url.PathEscape here, our bad actor tooltip might look like this:
<div>tooltip: %3Cscript%20src=xd.js%3E</div>, and Chatterino's single unescape call will turn that into <div>tooltip: <script src=xd.js></div>

If we call url.PathEscape here, our bad actor tooltip would look like this:
%3Cdiv%3Etooltip: %253Cscript%2520src=xd.js%253E%3C%2Fdiv%3E and Chatterino would turn that into <div>tooltip: %3Cscript%20src=xd.js%3E</div> effectively 'nullifying' the bad actor's html tags.

This could be better solved by never sending actual HTML to Chatterino but this is where we're at 👯

@nuuls
Copy link
Contributor Author

nuuls commented Feb 6, 2023

  • Changed the Type and extension into a sinlge line as requested
  • Removed handling for application/* types as it doesn't really fit the "Media" resolver and is just confusing
  • Made file extension handling a bit more reliable and accurate (previous code was stolen from i.nuuls)
  • Added tests for the Resolver

NGRDH

response := &resolver.Response{
Status: http.StatusOK,
Link: targetURL,
Tooltip: tooltip.String(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'll be necessary to make sure any already-escaped strings aren't unescaped by Chatterino.
When Chatterino parses this tooltip it runs a single unescape path.

If we don't call url.PathEscape here, our bad actor tooltip might look like this:
<div>tooltip: %3Cscript%20src=xd.js%3E</div>, and Chatterino's single unescape call will turn that into <div>tooltip: <script src=xd.js></div>

If we call url.PathEscape here, our bad actor tooltip would look like this:
%3Cdiv%3Etooltip: %253Cscript%2520src=xd.js%253E%3C%2Fdiv%3E and Chatterino would turn that into <div>tooltip: %3Cscript%20src=xd.js%3E</div> effectively 'nullifying' the bad actor's html tags.

This could be better solved by never sending actual HTML to Chatterino but this is where we're at 👯

@pajlada
Copy link
Member

pajlada commented Feb 11, 2023

Just need that pathescape back, otherwise works as expected for me

@pajlada pajlada merged commit ceffd59 into Chatterino:master Feb 11, 2023
@nuuls nuuls deleted the media-resolver branch February 11, 2023 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants