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

Font Library: add a 'manage font' exclusive user permission to manage the install/remove fonts. #55280

Closed
matiasbenedetto opened this issue Oct 11, 2023 · 36 comments
Labels
[Feature] Font Library [Feature] Typography Font and typography-related issues and PRs Needs Decision Needs a decision to be actionable or relevant [Type] Enhancement A suggestion for improvement.

Comments

@matiasbenedetto
Copy link
Contributor

matiasbenedetto commented Oct 11, 2023

What?

Add a 'manage font' exclusive user permission to manage the install/remove fonts.

Why?

Currently, we are using edit_theme_options. Should we create/use a more specific permission?


Props to @spacedmonkey and @felixarntz, who originally proposed this here:
https://wordpress.slack.com/archives/C02RQBWTW/p1696514853414459

@matiasbenedetto matiasbenedetto added [Type] Enhancement A suggestion for improvement. Needs Decision Needs a decision to be actionable or relevant [Feature] Typography Font and typography-related issues and PRs labels Oct 11, 2023
@jffng
Copy link
Contributor

jffng commented Jan 5, 2024

Who will get default access to this capability? Super Admins and Admins only?

Should we create/use a more specific permission?

I think so, but is it just a shorthand for existing capabilities? I believe the main permissions that are specific to managing fonts are:

  • The ability to create new posts in the database, storing the font face/family definition
  • Uploading and deleting of font assets

@felixarntz
Copy link
Member

I believe this capability would by default need to be a so-called "meta capability". It wouldn't be added to the WordPress database (we rarely do that with new capabilities these days anyway), but it would be programmatically granted based on other capabilities present, and potentially additional logic.

One important consideration here is that installing fonts (as far as I understand) requires access to the filesystem. This is somewhat sensitive and right now only administrators can perform such operations (install plugins, themes, language packs for example). Probably we'll need to follow that with fonts, although there may be room to diverge for that particular use-case. We also have to keep in mind that some WordPress installation disable filesystem access entirely, in which scenario effectively nobody could install new fonts. They would need to be managed through direct filesystem access outside of WordPress in that case.

That also leads to another point: manage_fonts would be very generic as a capability. Maybe it is indeed enough, but managing entails several distinct actions I assume. We don't need to be too pedantic about this, and lots of granular capabilities for this would probably be overkill, but I am specifically speaking about the installation bit. Installing and uninstalling fonts is probably more sensitive/restricted than e.g. editing some configuration related to a font in the database (if there is any such configuration, I'm not sure). So potentially something like install_fonts, edit_fonts, and uninstall_fonts could be more appropriate.

@azaozz
Copy link
Contributor

azaozz commented Jan 11, 2024

We don't need to be too pedantic about this, and lots of granular capabilities for this would probably be overkill

Yep, thinking so too. So who would really need a (separate) capability to manage fonts? Should someone be able to manage fonts but not manage/change the site's CSS? Think that wouldn't make sense? Then how often would someone need to actually manage the fonts on a site? Once per week, or month, or maybe year?

Thinking that perhaps a separate manage_fonts capability will not be needed. Seems it makes the most sense to let the users that can manage the site's CSS (or overall design, theme, etc.) also be able to manage the fonts. Seems that would work well even for huge sites where capabilities are really granular. Such sites usually have an "admin support desk" that can install plugins or change options that are inaccessible for authors and editors. Editing the site's CSS/looks and installing fonts can be performed the same way.

Seems site admins are the right choice for that, no need of any changes currently. If the need for a dedicated user that can manage fonts but is restricted from managing other options arises, lets add that then.

@felixarntz
Copy link
Member

While I don't think we have to go too granular with font permissions, I do think at least one font-specific capability is necessary. See below:

Should someone be able to manage fonts but not manage/change the site's CSS?

While that conceptually may be related, the specific action of installing fonts writes things to the filesystem. Editing CSS does not. So there is a clear difference there.

Editing the site's CSS/looks and installing fonts can be performed the same way.

Per the above, I don't think that's true. Editing CSS in WordPress only writes to the database, while installing fonts writes to the filesystem. I think a separate permission check for this is needed, if for no other reason at least due to server configuration implications, which may prevent installing fonts.

@azaozz
Copy link
Contributor

azaozz commented Jan 11, 2024

installing fonts writes things to the filesystem

Hmm, not more than uploading images? Seems (in the back-end) uploading font files should be treated in the same way as uploading images. It's not like font files can be used to upload any file types that can contain exploits? They are just another type of media files.

@felixarntz
Copy link
Member

Not sure. Fonts could potentially be handled similarly to things in the uploads folder. On the other hand, there's languages, plugins, and themes, all of which are handled in a roughly similar way, and installing each of them requires specific capabilities.

I wouldn't categorically say those files can't contain exploits. Specifically, I wonder why shouldn't fonts be similar to language packs (which in my impression have similarly high or low risk for exploits)?

@azaozz
Copy link
Contributor

azaozz commented Jan 11, 2024

On the other hand, there's languages, plugins, and themes, all of which are handled in a roughly similar way...

Yes but all of these can contain direct exploits. Themes and plugins have PHP files and are "most dangerous".

I wonder why shouldn't fonts be similar to language packs

Language packs are generally trusted by WP to not contain HTML and JS. In that sense their "exploitability" is very high too.

Afaik font files that are commonly used on the internet cannot contain executable code (good to confirm this in any case). Think there are malicious fonts that try to trick the user by making certain characters invisible, etc. but no direct exploits are possible. This seems less harmful and less exploitable than having a malicious PDF file for example.

So yes, there is a big difference between language packs and font files when it comes to exploitability.

@felixarntz
Copy link
Member

Totally agree for plugins and themes. However, without much further research, I think we can't reasonably assume that font files are safer to trust/use than language packs. Until we know otherwise, I think our baseline needs to be on the side of security (better too much than too little).

I think it would be great to get the security team's perspective on this to make a decision.

@azaozz
Copy link
Contributor

azaozz commented Jan 11, 2024

I think our baseline needs to be on the side of security

Yep, it is prudent to be on the side of security, however as explained above font files are not similar in any way to language packs. If they need to be compared to anything they are closer to images and PDFs.

Agree, it would be great to get the opinion of the security team. Until then I think font files should probably be treated the same way as any other file type that can be uploaded to WP?

In any case, this PR is for determining if another capability may be needed to allow users other than admins to install fonts. I think it is not needed unless it is proven practical and useful. Frankly I don't see the need for someone to be able to install fonts but not be able to edit_theme_options.

Further more if we think font files can be exploited it is even better to not allow more users to upload them, right? Same as with plugins themes and language packs.

@creativecoder
Copy link
Contributor

I believe the main permissions that are specific to managing fonts are:

  • The ability to create new posts in the database, storing the font face/family definition
  • Uploading and deleting of font assets

That's correct, and I think we can consider these two separately for which capabilities to use. The font library is still useful without being able to upload fonts, as the font source can be a remote url.

Managing font settings

This involves managing custom post types and post meta. It's closely tied to Global Styles, which uses the edit_theme_options capability, so I think checking edit_theme_options for creating, editing, and deleting font settings makes sense.

File uploads

This is when a font file is uploaded to the site, and the font settings are configured to use the file url after upload. By default font files are stored in wp-content/fonts using wp_handle_upload. This is what may need a new capability.

It not, continuing to use edit_theme_options probably makes the most sense, as it's limited to administrators and already used for managing font settings and Global Styles.

@getdave
Copy link
Contributor

getdave commented Feb 12, 2024

@azaozz @felixarntz @creativecoder I've read through this Issue.

It seems that most folks agree that edit_theme_options is sufficient for managing fonts, with the only point of indecision being over the ability to upload fonts, because that writes to the filesystem.

It seems we were waiting on the WordPress Security folks to provide input on how "exploitable" font files might be and that could form the basis for coming to a decision here.

Have I understood this correctly? If so I will prompt those folks for input on this issue in the hope we can come to a decision about firming up the user permission for the Font Library feature.

@felixarntz
Copy link
Member

felixarntz commented Feb 12, 2024

@getdave That's the most important question to answer, and I agree we need the security team's input for this one.

Additionally though, I think in any case at least a single capability like manage_fonts or similar would be important, as managing fonts has nothing to do with editing theme options.

It could be a meta capability that uses edit_theme_options as a default, but still it should be distinctly configurable independently of theme options.

@peterwilsoncc
Copy link
Contributor

@getdave There are two issues with uploading that I am seeing:

define( 'DISALLOW_FILE_MODS', true ); in wp-config.php is intended to prevent access to the file system (outside of the uploads folder) and remove options to modify files. An example of this can be seen in the Settings > General > Site Language. If the constant is set to true, you only see installed languages. If the constant is false, then it's possible to install languages from wordpress.org

The idea of the constant is that WP won't upload files outside of the uploads file regardless of the file settings. In cases in which the file settings are disabled at a server level chmod -w fonts the UI is present but the upload fails. The UI shouldn't be present with constant set. This is the issue users have seen on wordpress.org

no-file-access

I've started experimenting in PR #58957 but I am sure that there are things I am missing with the JavaScript permissions and other edge cases.

@felixarntz
Copy link
Member

@peterwilsoncc I tend to agree with you in that this needs to respect DISALLOW_FILE_MODS, but that only applies if we consider fonts similar to plugins, themes, and language packs. There have been alternative thoughts voiced before to consider fonts similar to uploads, and if we were to do that, then this constant wouldn't be relevant. It would likely require lots of hosts to give the special treatment of the wp-content/uploads folder to the wp-content/fonts folder as well though.

To be clear, I'm not vouching for either treatment, but just wanted to clarify the main point of discussion that needs to be addressed here: Should fonts be treated more like uploads (i.e. can be trusted and are expected to be always writable) or more like plugins/themes/language packs?

@peterwilsoncc
Copy link
Contributor

To be clear, I'm not vouching for either treatment, but just wanted to clarify the main point of discussion that needs to be addressed here: Should fonts be treated more like uploads (i.e. can be trusted and are expected to be always writable) or more like plugins/themes/language packs?

I think they are more akin to themes or plugins than uploads. Mainly because they can be used to completely change the look of a site. Most WP sites are created in a client services environment in which a designer has chosen fonts carefully.

On a more practical level, I don't think they can be treated as uploads as they are outside the folder. Servers locking file mods down via file permissions use the DISALLOW_FILE_MODS to signal to WP that only the uploads folder is writable. The result is that if try to treat the fonts folder as uploads, we'll just create a flood of bug reports as uploads fail.

@swissspidy
Copy link
Member

Given that the current capability being used is edit_theme_options, that also makes them more akin to plugins and themes, not regular file uploads such as images or documents.

@peterwilsoncc
Copy link
Contributor

I think the PR #58957 is in a suitable shape for respecting file write permissions.

@creativecoder
Copy link
Contributor

creativecoder commented Feb 15, 2024

I put together an experimental PR for another way we could handle respecting DISALLOW_FILE_MODS

  • Prevent uploading fonts when DISALLOW_FILE_MODS is set to true or the fonts dir is not writable
  • When installing a font and uploads are not allowed, fall back to using the remote font url from the collection

#59104

This allows sites with immutable/transient file systems to install fonts and load them from a remote server.

However, there are other issues to consider, like font collections may not want to be used as a CDN, or the site owner may have privacy concerns: see #58229. Those can be handled separately by adding other settings or filtering, but I wanted to mention them because they intersect with this issue here.

@peterwilsoncc
Copy link
Contributor

@creativecoder I don't think this is necessary as it can be managed via permissions. For situations such as #58696 the following filter can be used:

add_filter(
	'file_mod_allowed',
	function ( $allowed, $context ) {
		if ( 'can_modify_font_faces' === $context ) {
			return true;
		}
		return $allowed;
	},
	10,
	2
);

Hosts with immutable file systems (Pantheon, WP VIP, Altis DXP, etc, etc) outside of the uploads folder, can use that filter and current_user_can() & canUser() will return the expected result.

@peterwilsoncc
Copy link
Contributor

@creativecoder I think I misunderstood what you were saying a few days ago. I'll add a few comments to your PR.

As WordPress 6.5 is in it's final stages, I think it's better to set up the roles and caps based on how WordPress is currently managing fonts and come back to allowing for CDNs and similar later. Ultimately this is best decided by @youknowriad @getdave @swissspidy and @dream-encode.

@creativecoder
Copy link
Contributor

@peterwilsoncc Thanks for taking another look at this issue and reviewing the PR--I'll work on updating the PR tomorrow.

As WordPress 6.5 is in it's final stages, I think it's better to set up the roles and caps based on how WordPress is currently managing fonts and come back to allowing for CDNs and similar later.

Agreed on handling CDNs and triggering remote font hosting later--this needs more time and thought.

@creativecoder
Copy link
Contributor

Here's a PR that combines approaches, adding a meta capability for upload_fonts, as well as capability checks in the UI for font creation/deletion. Please review!

#59294

@azaozz
Copy link
Contributor

azaozz commented Feb 23, 2024

Was with the impression that a decision was reached here about not adding another user capability as the edit_theme_options is sufficient. As far as I see there are no examples of how that new capability can be used to benefit (the majority of) WP users. I.e. how/why a user with edit_theme_options should not be able to upload font files, and a user without edit_theme_options should be able to, and how these users will be able to utilize the newly uploaded font files.

Also seems the use of DISALLOW_FILE_MODS to prevent uploading of font files is going to make managing fonts in WP somewhat inconsistent. Seems that when the constant is set parts of the UI will have to be disabled and the users will have to use font files from third party URLs/CDNs resulting in lesser or insufficient user privacy.

Imho the most important part in all new WP features is the UX. That includes things like "easy to understand and use", "makes sense", "is logical", "has consistent behaviour", etc. so unsure if using DISALLOW_FILE_MODS in this case would be good.

Also see #59294 (comment)

@creativecoder
Copy link
Contributor

Reviewing the above conversation about whether a new capability is needed for uploading fonts seems to hinge on how you categorize font uploads: are they more like media uploads or more like themes/plugins/language packs?

I see them more like media uploads, specifically:

First, fonts are not versioned files and are unique to each site. For example, each site within a multisite network gets its own fonts folder, just like uploads. Say you use version control to manage your site files. Typically you'd put themes, plugins, and core files in version control, and not track uploads. I think generally you'd also keep uploaded font files out of version control. You can't composer install a font file onto a WordPress site like you can with a plugin.

Second, the code that handles font uploads is the same as media uploads (wp_handle_upload with upload_dir and upload_mimes filters).


This will require hosts to allow a writable wp-content/fonts folder (which can be relocated with a filter) for the font library to work, and that feels like a significant change. But I think that's a good thing. Typography is a really important feature for modern websites and thus setting the expectation that WordPress hosting includes support for font uploads and the font library seems appropriate to me.

@felixarntz
Copy link
Member

felixarntz commented Feb 26, 2024

The discussion here still seems to be unresolved. To be clear, I personally am not strongly leaning in either direction, but we have to make a real decision here. As far as I can tell there are two options:

  • Either we consider font files like media files. This implies:
    • Font files are considered safe from a security perspective, just like media files.
    • Hosts will be required to set up wp-content/fonts with similar permissions as wp-content/uploads.
    • DISALLOW_FILE_MODS is irrelevant.
  • Or we consider font files like plugins, themes, and language packs.
    • Font files are considered to be potentially harmful from a security perspective.
    • wp-content/fonts may or may not be writable, depending on the host configuration.
    • DISALLOW_FILE_MODS has to be respected.

If we go for the first, then we could go with edit_theme_options. But if it's the latter, we have to introduce a new capability, at least in order to respect DISALLOW_FILE_MODS. What we cannot do is a hybrid where we ignore DISALLOW_FILE_MODS but don't require hosts to make the filesystem permissions change, because that would simply lead to inconsistent behavior where many sites would still not be able to upload fonts even if WordPress thinks it's possible.

I think some of the discussion here goes too much into detail on the capabilities to use. The central question to decide on is whether font files can be trusted like media, or whether they cannot, and additionally, if they can be trusted, whether it's justifiable to make a disruptive change that requires many hosting providers to change their folder permissions for the new wp-content/fonts folder. I'm not saying at all that it's not worth it, but it is a disruptive change, whereas the other would just work. However, I acknowledge the UX may be better if we went for the disruptive change.

@azaozz
Copy link
Contributor

azaozz commented Feb 26, 2024

I personally am not strongly leaning in either direction

Same here.

Either we consider font files like media files.
Or we consider font files like plugins, themes, and language packs.

Right, this is a very good question. As far as I see font files are considered to be "media" and not "code" (executable). For comparison SVG files are also considered to be "media" (afaik) despite that they can contain scripting (and are considered insecure/exploitable).

However I agree the most important thing here is to look at this from the user perspective. At the end of the day I don't think it matters to the users whether font files are considered "software", "scripts", or "media" as long as the UX and UI for managing them in WP is consistent and works well. This is the top priority imho, and any solution can be "coded" and made to work even if it is "inconveniencing" some hosting providers by requiring them to make a change and treat wp-content/fonts the same way as wp-content/uploads.

Of course, this is just my opinion. I may be missing or misunderstanding something, and will be happy to change it if that's the case :)

@peterwilsoncc
Copy link
Contributor

This conversation is becoming splintered across the issue and the PR. Cross posting #59294 (comment)

This is what I am trying to convey, it's not a choice that can be made now. It was decided when the decision was made to store fonts outside of the uploads folder.

For systems in which the file system is immutable, the files simply won't upload so we need to hide the UI to avoid making false promises. On systems such as WP VIP it's not simply a case of mkdir fonts; chmod ... as the files are stored on a different server.

[there's more but that's the key bit so ... snip]

@swissspidy
Copy link
Member

We're a bit late in a cycle to have such fundamental discussions about whether fonts are considered to be media or not, that's a bit concerning 😞 😕

If we just think about capabilities, IMO it's a no brainer to have more fine-grained capabilities using map_meta_cap. Larger sites will inevitably have a need to tweak these permissions that way. Plus it gives us more flexibility to tweak the behavior later

Regardless of what we do, introducing a new wp-content/fonts folder is a big change and hosts will have to adapt, no doubt about it. That's why a dev-note is so important (which is not published yet AFAIK)

@felixarntz
Copy link
Member

+1

If this is supposed to go into 6.5, I think it is unreasonable to go with the requirement of wp-content/fonts being writable - there's too little time for the ecosystem to make the necessary changes, and as mentioned it would take a bit more time until there's even a dev note informing about the requirement.

So I think if the consensus is that font files should be treated like media, this may need to be delayed to give the ecosystem enough time to be prepared. Or at the very least, there would still need to be checks in place to hide or disable the UI for when the requirements are not met by the site.

@azaozz
Copy link
Contributor

azaozz commented Feb 26, 2024

This conversation is becoming splintered...

Right, will reply on #59294 (comment).

@joemcgill
Copy link
Member

Having just read through the discussion, my take is that worrying about specific meta caps is not as important at this stage as respecting the fundamental capabilities WP has to determine whether a file can be uploaded and used in an environment. I would encourage a decision to be made to either:

a) Move the uploads destination for fonts files to the wp-contents/uploads directory, to ensure that font uploads (and the related UI) works consistently across all hosting environment from day one.

b) Keep the current uploads location (i.e., wp-content/fonts) but ensure the feature respects DISABLE_FILE_MODS and ensures the new fonts folder is writable before presenting the UI for uploading a file that will result in a frustrating error message.

Personally, I would suggest moving the directory and treating fonts as an uploaded asset—just like media. As @peterwilsoncc has conveyed, there are simply too many environments that only allow files to be uploaded to the uploads directory for WP to assume otherwise. This would provide the most consistent user experience across all platforms and avoid a bunch of extra logic and overhead to account for environments where the fonts folder is not writable. If that's not possible for some reason, then affordances MUST be made in the UI to account for environments where the uploads will fail or else we will be shipping a frustrating experience to users on those environments.

Adding additional meta-caps, or filters that would allow different environments to choose how to support font files is something that we can always add later based on user feedback and clear use cases,. Using edit_theme_options caps as a basis for these permissions for now is probably fine.

@mtias
Copy link
Member

mtias commented Feb 27, 2024

The upload folder being wp-content/fonts was an explicit design decision to treat the font library as a distinct resource, not coupled with media as a representation. I think that's still the correct choice long term. I recall @m was also favoring that approach.

Given the small surface area the UI has at the moment (global styles; site editor), honoring DISALLOW_FILE_MODS and not exposing the interface at all seems fairly straightforward. I'd imagine we'll make access to the library wider and more standalone in future iterations. That also goes for revisiting more specific capabilities once and if there's further real world usage feedback on what would work best for most environments and user needs. It's hard to anticipate all of these preemptively and too easy to step into overbearing abstractions.

@swissspidy
Copy link
Member

For more context, citing #53965 regarding that wp-content/fonts decision:

  • We should keep uploads for things you insert into posts.
  • If we end up adding folders to media library and someone adds a "fonts" folder for some reason.

@felixarntz
Copy link
Member

@mtias Makes sense to me, my only concern is regarding:

That also goes for revisiting more specific capabilities once and if there's further real world usage feedback on what would work best for most environments and user needs.

I completely agree if we're thinking about fine grained capabilities for font management. However, due to the DISALLOW_FILE_MODS constraint I believe we at least need one additional capability right away to couple it to that condition (e.g. something like upload_fonts as previously proposed somewhere as a name). We can't use an existing capability like edit_theme_options for that, since editing theme options should never be blocked by DISALLOW_FILE_MODS.

So I think if we want to keep new capabilities to a minimum, I'd suggest:

  • Use a new capability (such as upload_fonts) for uploading fonts. Other than the user role, this will depend on DISALLOW_FILE_MODS not being enabled.
  • Use edit_theme_options for all other font management / usage.

@aaronjorbin
Copy link
Member

While I think ultimately some refined capabilities is best, I think @felixarntz's assessment of MVP here is correct.

The upload folder being wp-content/fonts was an explicit design decision to treat the font library as a distinct resource, not coupled with media as a representation. I think that's still the correct choice long term. I recall @m was also favoring that approach.

Can you please share the link to this discussion to minimize the pieces that are relitigated?

@mikachan
Copy link
Member

I'm going to close this issue in line with the decisions outlined in this post, but please feel free to continue the discussion if needed and re-open if required.

#59699 has also been opened to discuss architectural decisions for fonts directory fallbacks.

@mikachan mikachan closed this as not planned Won't fix, can't repro, duplicate, stale Mar 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Font Library [Feature] Typography Font and typography-related issues and PRs Needs Decision Needs a decision to be actionable or relevant [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Status: Done
Development

Successfully merging a pull request may close this issue.