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 filter to is_valid_path_for_site ACL function #6033

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

rbcorrales
Copy link
Member

@rbcorrales rbcorrales commented Dec 6, 2024

Description

This PR adds a filter to the is_valid_path_for_site function to allow overriding its behavior.
The use case for this is not necessarily to circumvent the subsite restrictions when ACL is enabled but also to allow access to other custom directories within the wp-content/uploads path.

This extends the functionality introduced by this PR in 2021: #1975

Changelog Description

Added

  • Introduced the vip_files_acl_is_valid_path_for_site filter to extend the is_valid_path_for_site function. This new filter allows overriding the VIP File System's validation process for multisite setups, which uses the /wp-content/mu-plugins/files/acl/endpoint-check-file-acl.php endpoint to check permissions for paths under /wp-content/uploads. It enables custom use cases, such as accessing directories outside the default site asset paths (but within the uploads folder) while adhering to other ACL rules. This extends the functionality introduced in PR Private Files: Restrict cross-site file access #1975. Warning: Improper use of this filter could expose private data. Developers using it must ensure that the logic they implement enforces proper access control to prevent data leaks between subsites.

Pre-review checklist

Please make sure the items below have been covered before requesting a review:

  • This change works and has been tested locally or in Codespaces (or has an appropriate fallback).
  • This change works and has been tested on a sandbox.
  • This change has relevant unit tests (if applicable).
  • This change uses a rollout method to ease with deployment (if applicable - especially for large scale actions that require writes).
  • This change has relevant documentation additions / updates (if applicable).
  • I've created a changelog description that aligns with the provided examples.

Pre-deploy checklist

  • VIP staff: Ensure any alerts added/updated conform to internal standards (see internal documentation).

Steps to Test

  1. Clone this repo and switch to the private-files-valid-path-filter branch.

  2. Follow the instructions for creating a local dev environment using the repo path as mu-plugins:

> vip dev-env create --slug=test --mu-plugins $(pwd)
  1. Start the environment:
> vip dev-env --slug=test start
  1. Create another subsite:
> vip dev-env --slug=test exec -- wp site create --slug=subsite2 --title="Subsite 2" [email protected]
  1. List the subsites and add both domains to your /etc/hosts file pointing to 127.0.0.1:
> vip dev-env --slug=test exec -- wp site list
  1. Checking ACL for a path inside site 2 will return status 202:
> curl http://subsite2.test.vipdev.lndo.site/wp-content/mu-plugins/files/acl/endpoint-check-file-acl.php -H 'X-Original-Uri: /wp-content/uploads/sites/2/kitty.jpg' -I
HTTP/1.1 202 Accepted
  1. Checking ACL for a path outside site 2 will return status 400:
> curl http://subsite2.test.vipdev.lndo.site/wp-content/mu-plugins/files/acl/endpoint-check-file-acl.php -H 'X-Original-Uri: /wp-content/uploads/custom-uploads/kitty.jpg' -I
HTTP/1.1 400 Bad Request
  1. There should be this message in the logs:
NOTICE: PHP message: PHP Warning:  Blocked request for file path that is not allowed (current site ID: 2 | requested URI: /wp-content/uploads/custom-uploads/kitty.jpg) in /wp/wp-content/mu-plugins/files/acl/endpoint-check-file-acl.php on line 30
  1. Add the following filter as a plugin in a file inside /client-mu-plugins:
function allow_custom_uploads_path( $is_valid, $file_path ) {
	if ( str_starts_with( $file_path, 'custom-uploads/' ) ) {
		return true;
	}

	return $is_valid;
}
add_filter( 'vip_files_acl_is_valid_path_for_site', 'allow_custom_uploads_path', 10, 2 );
  1. Repeat the previous request. It should now return 202 with no errors in the logs:
> curl http://subsite2.test.vipdev.lndo.site/wp-content/mu-plugins/files/acl/endpoint-check-file-acl.php -H 'X-Original-Uri: /wp-content/uploads/custom-uploads/kitty.jpg' -I
HTTP/1.1 202 Accepted

Copy link

sonarqubecloud bot commented Dec 6, 2024

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 30.53%. Comparing base (97c10f8) to head (ee4be5f).
Report is 12 commits behind head on develop.

Additional details and impacted files
@@              Coverage Diff              @@
##             develop    #6033      +/-   ##
=============================================
+ Coverage      30.51%   30.53%   +0.01%     
  Complexity      4811     4811              
=============================================
  Files            289      289              
  Lines          21177    21164      -13     
=============================================
- Hits            6463     6462       -1     
+ Misses         14714    14702      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rbcorrales rbcorrales marked this pull request as ready for review December 6, 2024 22:59
@rbcorrales rbcorrales requested a review from a team as a code owner December 6, 2024 22:59
@rinatkhaziev
Copy link
Contributor

@mjangda do you have any concerns with the proposed changes? functionally they work fine.

@mjangda
Copy link
Member

mjangda commented Dec 12, 2024

Seems fine if there are legitimate uses we've identified for this.

Main risk is that if the code that uses the filter isn't careful, it can end up leaking private data. But that's up the developer using the filter to enforce.

@rinatkhaziev
Copy link
Contributor

yeah, my thoughts exactly.

Copy link
Contributor

@rinatkhaziev rinatkhaziev left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @rbcorrales as well as for thorough testing! Looks good.

@rbcorrales
Copy link
Member Author

Thanks, @rinatkhaziev. I updated the changelog to be more descriptive. Let me know if this looks good.

@rinatkhaziev rinatkhaziev merged commit 611f574 into develop Dec 13, 2024
36 checks passed
@rinatkhaziev rinatkhaziev deleted the private-files-valid-path-filter branch December 13, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants