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

Inline SVG icons without the use of file_get_contents or WP_Filesystem #67

Open
daviedR opened this issue Oct 8, 2019 · 8 comments
Open
Labels
enhancement New feature or request

Comments

@daviedR
Copy link
Contributor

daviedR commented Oct 8, 2019

Since file_get_contents is not allowed in theme codes (Theme Check) and the only possible way to get the SVG codes out of SVG icon files is using WP_Filesystem.

Loading WP_Filesystem on frontend seems like a performance hindrance.

That's why we need to use a more direct technique for better performance. One that is easiest to do is putting all SVG icons' content in an array (just like what Twenty Nineteen and Twenty Twenty do).

What's need to do:

  • Change the gulp vendors task to print SVG file content instead of copying the files into assets/icons directory.
  • Deprecate suki/frontend/svg_icon_path filter, because we no longer access the icon files.
  • Change the suki_icon render function to adapt this new technique.
  • Check Suki Pro's Custom Icons compatibility.
@daviedR daviedR added the enhancement New feature or request label Oct 8, 2019
@daviedR daviedR added this to the 1.2.0 milestone Oct 8, 2019
@AlchemyUnited
Copy link

Also, if the SVG strings are not inlined, that is, they're stored elsewhere before being "injected" into a page, then technically, those SV string should be escaped.

If you're interested, at one point I wrote a PoC for storing the SVG strings in a class, and that class also having an echoSVG() method that did the escaping. Off the top of my head, you could add a filter(s) that would allow people manipulate the SVG library, yet still ensure that any SVG they added would also be escaped.

That code is somewhere in bowels of my HD. I can dig it up if you're interested.

@daviedR
Copy link
Contributor Author

daviedR commented Oct 8, 2019

Sure! That would be really helpful.

Yeah, I was thinking about the escaping too.

I just looked into Twenty Twenty theme's code, and they just did a normal escaping, like remove the newlines, white spaces, and tabs. Nothing specific about security risk.

@AlchemyUnited
Copy link

"Nothing specific about security risk."

I'm not so sure that's the case. What I did was done in the Summer of 2018 and I do remember searching to see what best practices were. That led me to - I think - to some repo on GitHub.

This looks to be dated AFTER I did mine. Let me dig around...

https://wordpress.stackexchange.com/questions/312625/escaping-svg-with-kses

@daviedR
Copy link
Contributor Author

daviedR commented Oct 8, 2019

Using kses to limit the attributes and inner tags is for security purpose :)
But I think we need more attributes to whitelist, like stroke, etc.

But that's still a good reference to use.

@AlchemyUnited
Copy link

This is what I used

https://github.com/darylldoyle/svg-sanitizer

There might be something newer and better but as of mid-summer 2018 it was the best I could find but the need that I had.

I found the code but need to repo it. I'll do that soon and put the link here.

@daviedR
Copy link
Contributor Author

daviedR commented Oct 9, 2019

That sanitizer is currently used on the popular Safe SVG plugin by the same developer.

Hmm, I am not sure if including this sanitizer built into the theme is worth it.

@AlchemyUnited
Copy link

I'm not sure you have a choice really. It's likely in a steady state so fork it and add your own namespace so there's no conflicts.

@daviedR daviedR modified the milestone: 1.2.0 Oct 25, 2019
@daviedR daviedR removed this from the 1.2.0 milestone Dec 17, 2019
@kleuter
Copy link

kleuter commented Jan 20, 2021

My suki pro doesn't display .svg social icons at all:
Warning: ftp_fget() expects parameter 1 to be resource, null given
I should have put
define('FS_METHOD', 'direct');
to the config to make it work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants