Prevent access to OpenAI endpoint from outside WPCOM #28309

millerf · 2023-01-12T06:58:07Z

Proposed changes:

Prevent access to WPCOM endpoint

Testing instructions

On your sandbox, use

./bin/jetpack-downloader test jetpack  trunk

Make sure the AI blocks work from a WPCOM site ( sandbox your site and test ).
Following this comment, test that the blocks WORK on an Atomic site (or any jetpack site)

Now apply this patch:

./bin/jetpack-downloader reset jetpack fix/prevent-access-to-endpoint &&  ./bin/jetpack-downloader test jetpack fix/prevent-access-to-endpoint

Make sure the AI blocks work from a WPCOM site ( sandbox your site and test ).
But now you should not be able to make the blocks work on Atomic or Jetpack site.

github-actions · 2023-01-12T06:59:17Z

Are you an Automattician? You can now test your Pull Request on WordPress.com. On your sandbox, run bin/jetpack-downloader test jetpack fix/prevent-access-to-endpoint to get started. More details: p9dueE-5Nn-p2

github-actions · 2023-01-12T06:59:27Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ All commits were linted before commit.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add testing instructions.
🔴 Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

🔴 Action required: We would recommend that you add a section to the PR description to specify whether this PR includes any changes to data or privacy, like so:

## Does this pull request change what data or activity we track or use?

My PR adds *x* and *y*.

Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

Next scheduled release: February 7, 2023.
Scheduled code freeze: January 31, 2023.

jeherve · 2023-01-12T10:05:15Z

Related discussion: p1673452156937829-slack-CBG1CP4EN

…oint on WPCOM

…users

artpi · 2023-01-12T11:49:21Z

projects/plugins/jetpack/_inc/lib/class-jetpack-ai-helper.php

 		}

-		$response = Client::wpcom_json_api_request_as_user(
+		$response = Client::wpcom_json_api_request_as_blog(


No, we want to be able to pinpoint loggedin users for security purposes I think

I followed those instructions: p1673510335525029/1673452156.937829-slack-CBG1CP4EN
But I am happy to revert...

If you prefer to make this feature available to only Jetpack connected users (aka excluding admins that haven't connected their WP.com account) then you might take this into account in the permission checks (non WPCOM ones) as well (also the UI but this is out of this PR's scope).

In general, this decision also affects the permission checks so it would be nice to reach a decision before further reviewing.

cc @jeherve

fgiannar

Many thanks for working on this!

Left a couple of inline comments :)

fgiannar · 2023-01-13T09:28:31Z

projects/plugins/jetpack/_inc/lib/class-jetpack-ai-helper.php

 		}

-		$response = Client::wpcom_json_api_request_as_user(
+		$response = Client::wpcom_json_api_request_as_blog(


If you prefer to make this feature available to only Jetpack connected users (aka excluding admins that haven't connected their WP.com account) then you might take this into account in the permission checks (non WPCOM ones) as well (also the UI but this is out of this PR's scope).

In general, this decision also affects the permission checks so it would be nice to reach a decision before further reviewing.

cc @jeherve

fgiannar · 2023-01-13T09:29:27Z

...ts/plugins/jetpack/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-ai.php

 	 */
 	public function __construct() {
-		$this->is_wpcom = false;
+		$this->is_wpcom                     = false;


Do we actually use/need this?

I checked and it does not seem to be used anywhere...

CC @artpi

I think when @jeherve introduced this he meant to use wpcom_is_wpcom_only_endpoint. I think we could remove it

fgiannar · 2023-01-13T09:31:22Z

...ts/plugins/jetpack/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-ai.php

+	 */
+	public function permissions_check() {
+
+		if ( ! current_user_can( 'edit_posts' ) ) {


Assuming we agree to make the request to WPCOM using wpcom_json_api_request_as_blog this wouldn't work as in such cases (blog token used) we'd get the rest_forbidden error

Yes, that is why we are not wpcom_json_api_request_as_blog

fgiannar · 2023-01-13T09:33:48Z

...ts/plugins/jetpack/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-ai.php

+
+		if ( defined( 'IS_WPCOM' ) && IS_WPCOM ) {
+			// Only allow WPCOM simple sites and Atomic
+			if ( get_current_site()->id !== 1 ) {


You might want to check the suggested implementation in Slack, in terms of allowing site level authentication (blog token) for AT site on WPCOM: p1673521187444489/1673452156.937829-slack-CBG1CP4EN

fgiannar · 2023-01-13T09:35:57Z

...ts/plugins/jetpack/_inc/lib/core-api/wpcom-endpoints/class-wpcom-rest-api-v2-endpoint-ai.php

+			if ( get_current_site()->id !== 1 ) {
+				return true;
+			}
+		} elseif ( ( new Automattic\Jetpack\Status\Host() )->is_woa_site() ) {


To ensure backwards compatibility you might prefer to only keep the WoA check on WPCOM context only. This way, when you decide to make the feature available to self-hosted sites, it will also be available in previous versions of the Jetpack plugin. Not a blocker though!

artpi

I am not a fan of this approach at all.

This prevention check is something that Jetpack code should not even be aware of
We need a way to quickly and dynamically change the value of it.

#28338 introduced the jetpack_ai_enabled filter which we can use to implement this check on the WPCOM end just by overriding the value to false for Jetpack sites (unless they have a special sticker?)

artpi · 2023-01-18T11:45:28Z

I would close this PR and make a diff on wpcom end.

jeherve · 2023-01-18T12:18:32Z

#28338 introduced the jetpack_ai_enabled filter which we can use to implement this check on the WPCOM end just by overriding the value to false for Jetpack sites (unless they have a special sticker?)

I'll let @fgiannar weigh in on that, but I don't believe the filter is enough for our needs here.

I am not a fan of this approach at all.

This prevention check is something that Jetpack code should not even be aware of
We need a way to quickly and dynamically change the value of it.

This is indeed what we were discussing in p1673452156937829-slack-CBG1CP4EN

artpi · 2023-01-18T12:28:34Z

I don't believe the filter is enough for our needs here.

Stepping through code:

Endpoint is called on the JP end, jetpack_ai_enabled is forced by the users to return true, proxies to WPCOM.
WPCOM code is loaded, jetpack_ai_enabled has a default value of true because of IS_WPCOM, but we hook into this to return false for JP sites
WPCOM_REST_API_V2_Endpoint_AI constructor returns before hooking into rest_api_init
WPCOM endpoint returns 404
User sees 404 in the editor instead of 403. Still acceptable

This is indeed what we were discussing in p1673452156937829-slack-CBG1CP4EN

Yes, I know, that would be a vote for decoupling syncing of this endpoint :D.
What I would love is to introduce the capability for Jetpack users to supply their own OpenAI token if they prefer to use it that way, but that would require some business plan discussion.
Then we could see if decoupling still makes sense.

fgiannar · 2023-01-18T15:38:57Z

#28338 introduced the jetpack_ai_enabled filter which we can use to implement this check on the WPCOM end just by overriding the value to false for Jetpack sites (unless they have a special sticker?)

I think that would be easier to decide once we reach a decision on blog vs user tokens used to proxy the request to WPCOM

artpi · 2023-01-18T15:45:42Z

I think that would be easier to decide once

I am deciding here that we are not going to wpcom_json_api_request_as_blog, for multiple reasons:

It messes up with caching
Vast majority of Atomic sites have 1 admin anyway
When we introduce billing than the site owner will get billed for the use generated by authors/editors. This may be unexpected and very troubling.

It is safer for now to request as user, so that we can see the usage in the wild. Once we see actual usage, we can revisit this decision of course.

fgiannar · 2023-01-18T15:55:03Z

I am deciding here that we are not going to wpcom_json_api_request_as_blog

Thank you, Artur!

That will indeed simplify the permission callback on both envs. Just making sure here that on Atomic and self hosted (future) sites, we need to make sure the current user is connected in order to provide this feature.

millerf · 2023-01-25T11:07:39Z

Closing in favor of a WPCOM-based filter. See previous comments

fgiannar · 2023-01-26T07:32:35Z

Closng in favor of a WPCOM-based filter. See previous comments

@millerf Can we please make sure to address the part of sanitizing request parameters in a follow up PR?

millerf · 2023-01-26T10:11:01Z

@millerf Can we please make sure to address the part of sanitizing request parameters in a follow up PR?

@fgiannar I have a PR here

millerf added 2 commits January 12, 2023 07:36

Prevent use of OpenAI endpoint in WPCOM environment

dbba26e

Create fix-prevent-access-to-endpoint

eb42dd5

github-actions bot added the [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ label Jan 12, 2023

github-actions bot added the [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. label Jan 12, 2023

Set wpcom_is_wpcom_only_endpoint oustide of IS_WPCOM

0f2eef6

millerf marked this pull request as draft January 12, 2023 07:50

millerf requested review from fgiannar and jeherve January 12, 2023 07:50

millerf self-assigned this Jan 12, 2023

Move permission_check back in the endpoints definition

da08de2

jeherve added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Block] AI Image [Block] AI Paragraph labels Jan 12, 2023

github-actions bot added the [Status] In Progress label Jan 12, 2023

millerf added 2 commits January 12, 2023 11:12

Add logic to prevent non- wimple and non-WoA sites to access the endp…

5aeb77b

…oint on WPCOM

Use wpcom_json_api_request_as_blog to allow access for non-connected …

ce36394

…users

artpi reviewed Jan 12, 2023

View reviewed changes

Sanitize inputs

68a4aab

fgiannar requested changes Jan 13, 2023

View reviewed changes

Implemented Foteini's feedback

ff6cc8b

artpi requested changes Jan 18, 2023

View reviewed changes

millerf closed this Jan 25, 2023

github-actions bot removed [Status] Needs Author Reply We need more details from you. This label will be auto-added until the PR meets all requirements. [Status] In Progress labels Jan 25, 2023

millerf mentioned this pull request Jan 26, 2023

[AI] Add/sanitize openai input #28583

Merged

artpi mentioned this pull request Jan 26, 2023

Block non-wpcom sites from accessing the Ai features from the WPCOM end #28598

Closed

Prevent access to OpenAI endpoint from outside WPCOM #28309

Prevent access to OpenAI endpoint from outside WPCOM #28309

Uh oh!

Conversation

millerf commented Jan 12, 2023

Proposed changes:

Testing instructions

Uh oh!

github-actions bot commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeherve commented Jan 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fgiannar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artpi left a comment

Choose a reason for hiding this comment

Uh oh!

artpi commented Jan 18, 2023

Uh oh!

jeherve commented Jan 18, 2023

Uh oh!

artpi commented Jan 18, 2023

Uh oh!

fgiannar commented Jan 18, 2023

Uh oh!

artpi commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgiannar commented Jan 18, 2023

Uh oh!

millerf commented Jan 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fgiannar commented Jan 26, 2023

Uh oh!

millerf commented Jan 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

github-actions bot commented Jan 12, 2023 •

edited

Loading

github-actions bot commented Jan 12, 2023 •

edited

Loading

artpi commented Jan 18, 2023 •

edited

Loading

millerf commented Jan 25, 2023 •

edited

Loading