Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions inc/GitSync/GitSync.php
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,11 @@ public function unbind( string $slug, bool $purge = false ): array|\WP_Error {
$purged = false;

if ( $purge && is_dir($absolute) ) {
$abspath = rtrim(ABSPATH, '/');
$validation = PathSecurity::validateContainment($absolute, $abspath);
$root = $binding->resolveContainmentRoot();
$validation = PathSecurity::validateContainment($absolute, $root);
if ( ! $validation['valid'] ) {
return new \WP_Error(
'path_outside_abspath',
'path_outside_binding_root',
sprintf('Refusing to purge "%s": %s', $absolute, $validation['message'] ?? 'containment failed'),
array( 'status' => 403 )
);
Expand All @@ -118,7 +118,7 @@ public function unbind( string $slug, bool $purge = false ): array|\WP_Error {
// Native PHP recursion so this works on managed hosts where
// exec() is disabled. RecursiveIteratorIterator with
// CHILD_FIRST lets unlink/rmdir happen depth-first.
$purge_err = $this->removeTree($validation['real_path']);
$purge_err = $this->removeTree( (string) ( $validation['real_path'] ?? $absolute ) );
if ( is_wp_error($purge_err) ) {
return $purge_err;
}
Expand Down Expand Up @@ -255,9 +255,9 @@ public function updatePolicy( string $slug, array $patch ): array|\WP_Error {
* be leading-slash, no traversal, not sensitive.
*/
private function validateLocalPath( GitSyncBinding $binding ): true|\WP_Error {
$relative = ltrim(str_replace('\\', '/', $binding->local_path), '/');
$relative = GitSyncBinding::normalizeRelativePath($binding->local_path);
if ( '' === $relative ) {
return new \WP_Error('invalid_local_path', 'local_path cannot resolve to ABSPATH itself.', array( 'status' => 400 ));
return new \WP_Error('invalid_local_path', 'local_path cannot resolve to the binding root itself.', array( 'status' => 400 ));
}
if ( PathSecurity::hasTraversal($relative) ) {
return new \WP_Error('path_traversal', 'local_path contains traversal segments (`.`, `..`).', array( 'status' => 400 ));
Expand All @@ -267,12 +267,12 @@ private function validateLocalPath( GitSyncBinding $binding ): true|\WP_Error {
}

// If the target already exists, run the symlink-safe check too.
$abspath = rtrim(ABSPATH, '/');
$absolute = $abspath . '/' . rtrim($relative, '/');
$root = $binding->resolveContainmentRoot();
$absolute = $binding->resolveAbsolutePath();
if ( is_dir($absolute) ) {
$containment = PathSecurity::validateContainment($absolute, $abspath);
$containment = PathSecurity::validateContainment($absolute, $root);
if ( ! $containment['valid'] ) {
return new \WP_Error('path_outside_abspath', sprintf('local_path resolves outside ABSPATH: %s', $containment['message'] ?? ''), array( 'status' => 403 ));
return new \WP_Error('path_outside_binding_root', sprintf('local_path resolves outside its binding root: %s', $containment['message'] ?? ''), array( 'status' => 403 ));
}
}

Expand Down
42 changes: 34 additions & 8 deletions inc/GitSync/GitSyncBinding.php
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* GitSync Binding
*
* Value object for a single binding between a site-owned local directory
* (relative to ABSPATH) and a GitHub repository.
* and a GitHub repository.
*
* Storage: serialized inside the `datamachine_gitsync_bindings` option.
* Validation lives here so the shape has one authoritative definition.
Expand Down Expand Up @@ -212,18 +212,44 @@ public function toArray(): array {
}

/**
* Resolve `local_path` to an absolute filesystem path under ABSPATH.
* Resolve `local_path` to an absolute filesystem path.
*
* `local_path` is stored leading-slash (e.g. `/wp-content/uploads/wiki/`)
* and interpreted relative to ABSPATH. Keeping the stored form portable
* lets a binding move between installs with different ABSPATHs without
* a migration step.
* and interpreted relative to the portable WordPress install shape. Paths
* under `/wp-content/` resolve through WP_CONTENT_DIR so managed hosts whose
* writable content directory lives outside ABSPATH still work.
*
* @return string Absolute path with no trailing slash.
*/
public function resolveAbsolutePath(): string {
$relative = ltrim(str_replace('\\', '/', $this->local_path), '/');
$abspath = rtrim(ABSPATH, '/');
return $abspath . '/' . rtrim($relative, '/');
$relative = self::normalizeRelativePath($this->local_path);
if ( self::isWpContentPath($relative) ) {
$remainder = substr($relative, strlen('wp-content'));
return rtrim(WP_CONTENT_DIR, '/') . rtrim($remainder, '/');
}

return rtrim(ABSPATH, '/') . '/' . rtrim($relative, '/');
}

/**
* Resolve the filesystem root this binding must remain inside.
*
* @return string Absolute root path with no trailing slash.
*/
public function resolveContainmentRoot(): string {
$relative = self::normalizeRelativePath($this->local_path);
if ( self::isWpContentPath($relative) ) {
return rtrim(WP_CONTENT_DIR, '/');
}

return rtrim(ABSPATH, '/');
}

public static function normalizeRelativePath( string $path ): string {
return ltrim(str_replace('\\', '/', $path), '/');
}

private static function isWpContentPath( string $relative ): bool {
return 'wp-content' === $relative || str_starts_with($relative, 'wp-content/');
}
}
9 changes: 4 additions & 5 deletions inc/GitSync/GitSyncFetcher.php
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,12 @@ public function pull( GitSyncBinding $binding, array $args = array() ): array|\W
}

// Containment belt-and-suspenders: once the dir exists, re-validate
// that it really lives under ABSPATH (symlink-safe).
$abspath_root = rtrim(ABSPATH, '/');
$containment = PathSecurity::validateContainment($absolute, $abspath_root);
// that it really lives under its binding root (symlink-safe).
$containment = PathSecurity::validateContainment($absolute, $binding->resolveContainmentRoot());
if ( ! $containment['valid'] ) {
return new \WP_Error('path_outside_abspath', $containment['message'] ?? 'containment failed', array( 'status' => 403 ));
return new \WP_Error('path_outside_binding_root', $containment['message'] ?? 'containment failed', array( 'status' => 403 ));
}
$absolute = $containment['real_path'];
$absolute = (string) ( $containment['real_path'] ?? $absolute );

$tree = GitHubRemote::fetchTree($slug, $binding->branch, $pat);
if ( is_wp_error($tree) ) {
Expand Down
20 changes: 20 additions & 0 deletions tests/smoke-gitsync.php
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
define('ABSPATH', $scratch . '/');
}

if (! defined('WP_CONTENT_DIR') ) {
define('WP_CONTENT_DIR', rtrim($scratch, '/') . '-content');
}

$GLOBALS['__dmc_options'] = array();
$GLOBALS['__dmc_http_mock'] = array();
$GLOBALS['__dmc_http_capture'] = array();
Expand Down Expand Up @@ -239,6 +243,22 @@ public static function apiRequest( string $method, string $url, array $body, str
$assert(! is_wp_error($bound), 'bind succeeded');
$assert(false === is_dir(ABSPATH . 'content/wiki/'), 'bind did not create directory');

$content_bound = $gs->bind(
array(
'slug' => 'wp-content-binding',
'local_path' => '/wp-content/uploads/wiki/',
'remote_url' => 'https://github.com/Automattic/a8c-wiki-woocommerce',
)
);
$assert(! is_wp_error($content_bound), 'wp-content bind succeeded');
$content_binding = ( new \DataMachineCode\GitSync\GitSyncRegistry() )->get('wp-content-binding');
$assert($content_binding instanceof \DataMachineCode\GitSync\GitSyncBinding, 'wp-content binding stored');
$assert(
rtrim(WP_CONTENT_DIR, '/') . '/uploads/wiki' === $content_binding->resolveAbsolutePath(),
'wp-content binding resolves through WP_CONTENT_DIR'
);
$gs->unbind('wp-content-binding');

$dup = $gs->bind(array( 'slug' => 'wiki', 'local_path' => '/x/', 'remote_url' => 'https://github.com/a/b' ));
$assert(is_wp_error($dup) && 'binding_exists' === $dup->get_error_code(), 'refuses duplicate slug');

Expand Down
Loading