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
6 changes: 3 additions & 3 deletions src/api/class-client.php
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ public static function get_content( int|string $content_id, int|string|null $pro
return false;
}

$url = sprintf( '%s/projects/%d/content/%s', \BeyondWords\Core\Urls::get_api_url(), $project_id, $content_id );
$url = sprintf( '%s/projects/%d/content/%s', \BeyondWords\Core\Urls::get_api_url(), $project_id, rawurlencode( (string) $content_id ) );

return self::call_api( 'GET', $url );
}
Expand Down Expand Up @@ -194,7 +194,7 @@ public static function update_audio( int $post_id ): array|null|false {
return false;
}

$url = sprintf( '%s/projects/%d/content/%s', \BeyondWords\Core\Urls::get_api_url(), $project_id, $content_id );
$url = sprintf( '%s/projects/%d/content/%s', \BeyondWords\Core\Urls::get_api_url(), $project_id, rawurlencode( (string) $content_id ) );
$body = \BeyondWords\Post\Content::get_content_params( $post_id );
$response = self::call_api( 'PUT', $url, $body, $post_id );

Expand Down Expand Up @@ -240,7 +240,7 @@ public static function delete_audio_by_ids( int|string|false $project_id, int|st
return false;
}

$url = sprintf( '%s/projects/%d/content/%s', \BeyondWords\Core\Urls::get_api_url(), $project_id, $content_id );
$url = sprintf( '%s/projects/%d/content/%s', \BeyondWords\Core\Urls::get_api_url(), $project_id, rawurlencode( (string) $content_id ) );
$response = self::call_api( 'DELETE', $url, '', $post_id, [], self::DELETE_TIMEOUT );

if ( 204 !== wp_remote_retrieve_response_code( $response ) ) {
Expand Down
7 changes: 6 additions & 1 deletion src/editor/components/content-id/class-content-id.php
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,15 @@ public static function save( $post_id ) {
}

if ( isset( $_POST['beyondwords_content_id'] ) ) {
// Content IDs are interpolated into BeyondWords API URL paths, so they
// need a stricter charset than sanitize_text_field() alone allows —
// see \BeyondWords\Post\Meta::sanitize_content_id().
update_post_meta(
$post_id,
'beyondwords_content_id',
sanitize_text_field( wp_unslash( $_POST['beyondwords_content_id'] ) )
\BeyondWords\Post\Meta::sanitize_content_id(
sanitize_text_field( wp_unslash( $_POST['beyondwords_content_id'] ) )
)
);
}

Expand Down
28 changes: 28 additions & 0 deletions src/post/class-meta.php
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,34 @@ public static function get_content_id( int $post_id, bool $fallback = false ): s
return false;
}

/**
* Sanitize a BeyondWords Content ID.
*
* Content IDs are BeyondWords-issued numeric IDs or UUIDs, so the only valid
* characters are alphanumerics and hyphens — the same `[a-zA-Z0-9\-]+` charset
* the plugin's own inspect REST route enforces. Anything outside that set is
* rejected (blanked) rather than stored, because the Content ID is later
* interpolated into BeyondWords API URL paths in \BeyondWords\Api\Client, where
* characters such as `/`, `.`, `?`, `#` and `&` would let a crafted value steer
* an authenticated, organization-API-key-scoped request to an attacker-chosen
* endpoint (path/query injection).
*
* Used both directly by the classic-editor save handler and as the
* `sanitize_callback` for the `beyondwords_content_id` post meta, so the
* block-editor / REST write path is validated identically.
*
* @since 7.0.0
*
* @param mixed $content_id Raw submitted Content ID.
*
* @return string Sanitized Content ID, or '' when the value is invalid.
*/
public static function sanitize_content_id( $content_id ): string {
$content_id = sanitize_text_field( (string) $content_id );

return preg_match( '/^[a-zA-Z0-9-]*$/', $content_id ) ? $content_id : '';
Comment thread
gouravkhunger marked this conversation as resolved.
}

/**
* Get the (legacy) Podcast ID for a WordPress Post.
*
Expand Down
11 changes: 10 additions & 1 deletion src/post/class-sync.php
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,16 @@ public static function register_meta(): void {
];

foreach ( $keys as $key ) {
register_meta( 'post', $key, $options );
$meta_options = $options;

// Content IDs are interpolated into BeyondWords API URL paths, so
// the block-editor / REST write path needs the same strict charset
// validation as the classic editor — see Meta::sanitize_content_id().
if ( 'beyondwords_content_id' === $key ) {
$meta_options['sanitize_callback'] = [ \BeyondWords\Post\Meta::class, 'sanitize_content_id' ];
}

register_meta( 'post', $key, $meta_options );
}
}
}
Expand Down
32 changes: 32 additions & 0 deletions tests/phpunit/api/test-client.php
Original file line number Diff line number Diff line change
Expand Up @@ -939,6 +939,38 @@ public function get_content_calls_expected_url()
delete_option('beyondwords_project_id');
}

/**
* The Content ID is charset-validated before storage, but Client must still
* defensively rawurlencode() it so any value that reaches the URL builder
* can't inject extra path or query segments into the authenticated request.
*
* @test
*/
public function get_content_rawurlencodes_the_content_id()
{
update_option('beyondwords_api_key', BEYONDWORDS_TESTS_API_KEY);
update_option('beyondwords_project_id', BEYONDWORDS_TESTS_PROJECT_ID);

$captured_url = null;
$filter = function ($preempt, $args, $url) use (&$captured_url) {
$captured_url = $url;
return $preempt;
};
add_filter('pre_http_request', $filter, 1, 3);

Client::get_content('a/b?c=1');

remove_filter('pre_http_request', $filter, 1);

// The slash and query characters are percent-encoded, so the URL keeps
// its intended `/content/<id>` shape instead of gaining an attacker path.
$this->assertStringContainsString('/content/a%2Fb%3Fc%3D1', (string) $captured_url);
$this->assertStringNotContainsString('/content/a/b?c=1', (string) $captured_url);

delete_option('beyondwords_api_key');
delete_option('beyondwords_project_id');
}

/**
* @test
*
Expand Down
26 changes: 25 additions & 1 deletion tests/phpunit/editor/classic/test-metabox.php
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,13 @@ public function player_embed_neutralises_content_id_xss()
// Set the meta after creation so no save_post handler can overwrite the
// payload before we render — this keeps the assertions meaningful.
update_post_meta($postId, 'beyondwords_project_id', 12345);
update_post_meta($postId, 'beyondwords_content_id', $payload);

// Content IDs are charset-validated on save (Meta::sanitize_content_id), so
// update_post_meta() would blank this payload and nothing would render. Write
// it straight to the DB instead — a hostile value can now only reach the
// renderer via a raw row (legacy data, a direct DB write, or a future
// regression), which is exactly the case this escaping defence must survive.
$this->store_raw_content_id($postId, $payload);

$html = $this->capture_output(function () use ($postId) {
Metabox::player_embed($postId);
Expand Down Expand Up @@ -219,6 +225,24 @@ public function player_embed_neutralises_content_id_xss()
wp_delete_post($postId, true);
}

/**
* Write a Content ID straight into post meta, bypassing the
* Meta::sanitize_content_id() callback registered on the meta key, so a
* hostile value can still be put in front of the renderer.
*/
private function store_raw_content_id($post_id, $value)
{
global $wpdb;

$wpdb->insert($wpdb->postmeta, [
'post_id' => $post_id,
'meta_key' => 'beyondwords_content_id',
'meta_value' => $value,
]);

wp_cache_delete($post_id, 'post_meta');
}

/**
* @test
* @dataProvider errors_provider
Expand Down
19 changes: 19 additions & 0 deletions tests/phpunit/editor/components/content-id/test-content-id.php
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,25 @@ public function save_provider()
'postValue' => 'abc<b>def</b>ghi',
'expect' => 'abcdefghi',
],
// A crafted Content ID must not be able to inject path/query segments
// into the authenticated BeyondWords API URL — see the fix in
// Meta::sanitize_content_id().
'Path traversal + query blanked' => [
'postValue' => 'x/../../projects/999/content/abc?force=1',
'expect' => '',
],
'Forward slash blanked' => [
'postValue' => 'a/b',
'expect' => '',
],
'Query string blanked' => [
'postValue' => 'abc?force=1',
'expect' => '',
],
'Ampersand blanked' => [
'postValue' => 'a&b',
'expect' => '',
],
];
}

Expand Down
47 changes: 38 additions & 9 deletions tests/phpunit/player/renderer/test-javascript.php
Original file line number Diff line number Diff line change
Expand Up @@ -57,13 +57,18 @@ public function render()
}

/**
* Stored-XSS regression: dangerous characters in a param value must never
* break out of the single-quoted onload='...' attribute, whatever the payload.
* Stored-XSS regression: dangerous characters in a Content ID must never
* break out of the single-quoted onload='...' attribute, whatever the value.
*
* Content IDs are now charset-validated on save (Meta::sanitize_content_id),
* so a hostile value can only reach the renderer via a raw row — legacy data,
* a direct DB write, or a future regression. We store one straight in the DB,
* bypassing the meta sanitiser, to keep the renderer's output-escaping second
* line of defence under test.
*
* Asserts the security *property* — one intact <script> and the value survives
* as inert JSON data — rather than a specific escaping mechanism, so it stays
* valid if the escaping strategy is ever refactored. The injected value reaches
* the renderer via `beyondwords_content_id`, the documented editor/REST sink.
* valid if the escaping strategy is ever refactored.
*
* @test
* @dataProvider dangerousContentIdProvider
Expand All @@ -74,10 +79,13 @@ public function render_neutralises_a_dangerous_content_id($payload)
'post_title' => 'JavascriptTest::render_neutralises_a_dangerous_content_id',
'meta_input' => [
'beyondwords_project_id' => BEYONDWORDS_TESTS_PROJECT_ID,
'beyondwords_content_id' => $payload,
],
]);

// The meta sanitiser would blank this value, so store it raw to simulate
// a legacy/externally-written row reaching the renderer.
$this->store_raw_content_id($post->ID, $payload);

$html = Javascript::render($post);

// A real HTML parser must see exactly one <script> — a breakout would
Expand All @@ -101,10 +109,31 @@ public function render_neutralises_a_dangerous_content_id($payload)
}

/**
* Payloads that survive sanitize_text_field() (the meta sanitiser registered
* for beyondwords_content_id) — quotes, ampersands and Unicode. It strips
* < > tags, so tag-based payloads can only reach the renderer via a filter;
* those are covered by render_neutralises_dangerous_sdk_params_from_filter().
* Write a Content ID straight into post meta, bypassing the
* Meta::sanitize_content_id() callback registered on the meta key, to
* reproduce a hostile value that predates or otherwise circumvents input
* validation so the renderer's output escaping stays under test.
*/
private function store_raw_content_id($post_id, $value)
{
global $wpdb;

$wpdb->insert($wpdb->postmeta, [
'post_id' => $post_id,
'meta_key' => 'beyondwords_content_id',
'meta_value' => $value,
]);

wp_cache_delete($post_id, 'post_meta');
}

/**
* Values that would break out of, or corrupt, the onload attribute if the
* renderer didn't neutralise them — quotes, ampersands and Unicode. They no
* longer survive the beyondwords_content_id meta sanitiser (which blanks
* anything outside [a-zA-Z0-9-]), so the test injects them via a raw DB write
* to isolate the renderer's own output-escaping defence. Tag-based payloads
* are covered by render_neutralises_dangerous_sdk_params_from_filter().
*/
public function dangerousContentIdProvider()
{
Expand Down
45 changes: 43 additions & 2 deletions tests/phpunit/post/test-meta.php
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,45 @@ public function get_content_id_provider()
];
}

/**
* A Content ID is interpolated into BeyondWords API URL paths, so only the
* `[a-zA-Z0-9\-]+` charset the inspect REST route allows may be stored —
* anything with path/query characters is blanked to defeat URL injection.
*
* @test
* @dataProvider sanitize_content_id_provider
*
* @param mixed $input Raw submitted Content ID.
* @param string $expected Expected sanitized value.
*/
public function sanitize_content_id($input, $expected)
{
$this->assertSame($expected, Meta::sanitize_content_id($input));
}

public function sanitize_content_id_provider()
{
return [
// Legitimate values pass through unchanged.
'UUID' => ['9279c9e0-e0b5-4789-9040-f44478ed3e9e', '9279c9e0-e0b5-4789-9040-f44478ed3e9e'],
'numeric id' => ['1234567', '1234567'],
'all-zero UUID' => ['00000000-0000-0000-0000-000000000000', '00000000-0000-0000-0000-000000000000'],
'empty string' => ['', ''],
'surrounding whitespace' => [' 9279c9e0-e0b5 ', '9279c9e0-e0b5'],
// Anything outside [a-zA-Z0-9-] is blanked.
'path traversal + query' => ['x/../../projects/999/content/abc?force=1', ''],
'forward slash' => ['abc/def', ''],
'dot segment' => ['abc.def', ''],
'query string' => ['abc?force=1', ''],
'fragment' => ['abc#frag', ''],
'ampersand' => ['a&b', ''],
'colon' => ['abc:def', ''],
'underscore' => ['content_id', ''],
'space' => ['a b', ''],
'script tag' => ['<script>alert(1)</script>', ''],
];
}

/**
* Get API response body from post meta field.
*
Expand Down Expand Up @@ -300,11 +339,13 @@ public function remove_all_beyondwords_metadata()
'post_title' => 'MetaTest:removeAllBeyondwordsMetadata',
]);

// Set all BeyondWords meta keys
// Set all BeyondWords meta keys. Use a hyphen rather than an underscore so
// the value survives beyondwords_content_id's strict sanitiser
// (Meta::sanitize_content_id) while remaining valid for every other key.
$beyondwordsKeys = Utils::get_post_meta_keys('all');

foreach ($beyondwordsKeys as $key) {
update_post_meta($postId, $key, 'test_value');
update_post_meta($postId, $key, 'test-value');
}

// Set a non-BeyondWords meta key that should NOT be removed
Expand Down
33 changes: 31 additions & 2 deletions tests/phpunit/post/test-sync.php
Original file line number Diff line number Diff line change
Expand Up @@ -566,7 +566,9 @@ public function register_meta()
'beyondwords_generate_audio' => 'beyondwords_generate_audio',
'beyondwords_disabled' => 'beyondwords_disabled',
'beyondwords_error_message' => 'beyondwords_error_message',
'beyondwords_content_id' => 'beyondwords_content_id',
// Content ID is charset-validated on save (Meta::sanitize_content_id),
// so it needs a valid value here rather than the meta-key placeholder.
'beyondwords_content_id' => BEYONDWORDS_TESTS_CONTENT_ID,
'beyondwords_podcast_id' => 'beyondwords_podcast_id',
'beyondwords_preview_token' => 'beyondwords_preview_token',
'beyondwords_project_id' => 'beyondwords_project_id',
Expand All @@ -591,7 +593,7 @@ public function register_meta()
$this->assertSame('beyondwords_error_message', get_post_meta($postId, 'beyondwords_error_message', true));

$this->assertArrayHasKey('beyondwords_content_id', $meta);
$this->assertSame('beyondwords_content_id', get_post_meta($postId, 'beyondwords_content_id', true));
$this->assertSame(BEYONDWORDS_TESTS_CONTENT_ID, get_post_meta($postId, 'beyondwords_content_id', true));

$this->assertArrayHasKey('beyondwords_podcast_id', $meta);
$this->assertSame('beyondwords_podcast_id', get_post_meta($postId, 'beyondwords_podcast_id', true));
Expand Down Expand Up @@ -623,6 +625,33 @@ public function register_meta()
wp_delete_post($postId, true);
}

/**
* The beyondwords_content_id meta is registered with a strict sanitize
* callback so the block-editor / REST write path can't persist a Content ID
* that would inject path or query segments into an authenticated BeyondWords
* API URL (see Meta::sanitize_content_id).
*
* @test
*/
public function register_meta_sanitizes_content_id_on_write()
{
Sync::register_meta();

$postId = self::factory()->post->create([
'post_title' => 'SyncTest::registerMetaSanitizesContentIdOnWrite',
]);

// A crafted value with URL path/query characters is blanked on save.
update_post_meta($postId, 'beyondwords_content_id', 'x/../../projects/999/content/abc?force=1');
$this->assertSame('', get_post_meta($postId, 'beyondwords_content_id', true));

// A legitimate UUID Content ID is stored unchanged.
update_post_meta($postId, 'beyondwords_content_id', BEYONDWORDS_TESTS_CONTENT_ID);
$this->assertSame(BEYONDWORDS_TESTS_CONTENT_ID, get_post_meta($postId, 'beyondwords_content_id', true));

wp_delete_post($postId, true);
}

function remove_delete_actions($core) {
// Actions for deleting/trashing/restoring posts
remove_action('before_delete_post', array($core, 'onDeletePost'));
Expand Down
Loading