Require providing allowed abilities in Ability_Function_Resolver, and make it non-static (for the most part)#61
Conversation
…e it non-static (for the most part).
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Note: The test failures here are unrelated to the PR and are happening because of this package's general incompatibility with WordPress 7.0 Beta 1. See #60 |
| array( | ||
| 'error' => sprintf( 'Ability "%s" was not specified in the allowed abilities list.', $ability_name ), | ||
| 'code' => 'ability_not_allowed', | ||
| ) |
There was a problem hiding this comment.
This exposes the existence of an ability even when it's not in the allowed abilities list. Is that a concern?
There was a problem hiding this comment.
How does it expose that?
The check for whether the ability exists only happens right after that (see below). All this message does is tell the agent that the given ability name is not allowed, regardless of whether it's an actual ability the site has or something the agent hallucinated.
There was a problem hiding this comment.
Because the error code is different, ability_not_allowed vs ability_not_found. Similar to 403 vs 404.
There was a problem hiding this comment.
Hmm that's not quite correct is it. This change looks like it'll result in ability_not_allowed being returned for any ability that's not registered.
There was a problem hiding this comment.
It's not about whether it's registered - the given list for the class instance is an allowlist for just that agent.
If the ability that's looked up here is not in that list, it only means the dev didn't allow calling it. Doesn't indicate whether it's registered or not.
JasonTheAdams
left a comment
JasonTheAdams
left a comment
There was a problem hiding this comment.
This will mean the dev has to pass the abilities both to the prompt builder and to the Ability_Function_Resolver instance, but I think that's fair at this point. We can improve this later.
Otherwise, I do recall at least one person showing code that used this, so we'll want to communicate this BC-break in some capacity.
Make `WP_AI_Client_Ability_Function_Resolver` non-static and require specifying the allowed abilities list in the constructor. This hardens security by ensuring that only explicitly specified abilities can be executed, preventing potential vulnerabilities such as prompt injection from triggering arbitrary abilities. The constructor accepts either `WP_Ability` objects or ability name strings. If an ability is not in the allowed list, an error response with code `ability_not_allowed` is returned. Developed in #11103. Upstream: WordPress/wp-ai-client#61. Props felixarntz, gziolo, JasonTheAdams, dkotter, johnbillion. Fixes #64769. git-svn-id: https://develop.svn.wordpress.org/trunk@61795 602fd350-edb4-49c9-b593-d223f7449a82
Make `WP_AI_Client_Ability_Function_Resolver` non-static and require specifying the allowed abilities list in the constructor. This hardens security by ensuring that only explicitly specified abilities can be executed, preventing potential vulnerabilities such as prompt injection from triggering arbitrary abilities. The constructor accepts either `WP_Ability` objects or ability name strings. If an ability is not in the allowed list, an error response with code `ability_not_allowed` is returned. Developed in WordPress/wordpress-develop#11103. Upstream: WordPress/wp-ai-client#61. Props felixarntz, gziolo, JasonTheAdams, dkotter, johnbillion. Fixes #64769. Built from https://develop.svn.wordpress.org/trunk@61795 git-svn-id: http://core.svn.wordpress.org/trunk@61101 1a063a9b-81f0-0310-95a4-ce76da25c4cd
4 participants
This addresses a security weakness: At no point so far we were checking (or at least encouraging) that the abilities called in a message are actually among the abilities allowed for the prompt.
Not checking this can lead to security vulnerabilities, e.g. through prompt injection.
While developers today could work around this by manually checking prior to using
Ability_Function_Resolver, this is not intuitive at all. Since it's a security concern, it needs to be baked in and mandatory.This is a breaking change. The
Ability_Function_Resolverclass becomes (mostly) non-static, and will require an instance for usage going forward. It will require passing the list of abilities in the constructor, in the same shape it's used onPrompt_Builder::using_abilities().