From 1e752bbcc8b35630f172c0d84b5ca215956168d7 Mon Sep 17 00:00:00 2001 From: "Matthew M. Emma" Date: Sat, 8 Aug 2026 13:09:54 -0700 Subject: [PATCH] Correct the security policy and cover two untested paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SECURITY.md recommended a version that destroys credentials. It listed all of 2.0.x as supported and told readers to "upgrade to 2.0.0", but 2.0.0 and 2.0.1 are precisely the releases that over-encrypt the stored client secret until it cannot be read. Both are now marked unsupported, with the fault, its blast radius and the remedy stated plainly, and the 1.x plaintext warning separated out so its instruction to rotate credentials is not lost among the newer issue. Also covers two paths that had no tests at all. Logger redaction is what stands between a debug session and a client secret in debug.log. It now has tests for redaction of secrets and bearer tokens, including nested arrays, plus truncation and the bounded ring buffer. Renderer's override chain is a documented headline feature — copying a template into a theme is how users are told to customise markup safely. It now has tests for child-theme precedence over parent over plugin, that an override renders and receives its data, that directory traversal cannot escape the template directories, and that notices escape their message. Tests build real fixture files, so the WP_Filesystem sniff is scoped away from tests/ where that runtime abstraction does not exist. 86 -> 101 tests, 350 -> 384 assertions. PHPCS clean. Co-Authored-By: Claude Opus 5 --- .github/ISSUE_TEMPLATE/bug_report.yml | 2 +- CHANGELOG.md | 26 ++++ SECURITY.md | 40 ++++-- phpcs.xml.dist | 7 +- procore-connect.php | 4 +- readme.txt | 10 +- tests/unit/LoggerTest.php | 152 ++++++++++++++++++++++ tests/unit/RendererTest.php | 178 ++++++++++++++++++++++++++ 8 files changed, 404 insertions(+), 15 deletions(-) create mode 100644 tests/unit/LoggerTest.php create mode 100644 tests/unit/RendererTest.php diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml index 864a4f7..00ed899 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.yml +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -60,7 +60,7 @@ body: id: versions attributes: label: Versions - placeholder: 'Procore Connect 2.0.0, WordPress 6.9, PHP 8.2' + placeholder: 'Procore Connect 2.0.3, WordPress 7.0, PHP 8.3' validations: required: true diff --git a/CHANGELOG.md b/CHANGELOG.md index 10b6aba..d1ce21a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,31 @@ All notable changes to Procore Connect are documented here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [2.0.4] — 2026-08-08 + +### Security + +- `SECURITY.md` no longer recommends a version that destroys credentials. It listed all + of 2.0.x as supported and told readers to "upgrade to 2.0.0" — but 2.0.0 and 2.0.1 are + precisely the versions that over-encrypt the stored client secret until it is + unreadable. Both are now marked unsupported, with the fault, its blast radius and the + remedy stated plainly. The 1.x plaintext-storage warning is separated out and keeps its + instruction to rotate credentials. + +### Added + +- Tests for `Support\Logger`: redaction of secrets from log context, including nested + arrays, plus truncation and the bounded ring buffer. This is what stands between a + debug session and a client secret in `debug.log`, and it had no coverage. +- Tests for `Frontend\Renderer`: the child-theme → parent-theme → plugin override chain, + that an override actually renders and receives its data, directory-traversal refusal, + and notice escaping. Copying a template into a theme is a documented headline feature + and it had no coverage. + +### Changed + +- Refreshed the version placeholder in the bug-report template. + ## [2.0.3] — 2026-08-08 ### Fixed @@ -206,6 +231,7 @@ every install was non-functional regardless of configuration. - Initial release. +[2.0.4]: https://github.com/ibuilder/ProcoreWP/releases/tag/v2.0.4 [2.0.3]: https://github.com/ibuilder/ProcoreWP/releases/tag/v2.0.3 [2.0.2]: https://github.com/ibuilder/ProcoreWP/releases/tag/v2.0.2 [2.0.1]: https://github.com/ibuilder/ProcoreWP/releases/tag/v2.0.1 diff --git a/SECURITY.md b/SECURITY.md index 806361b..21cde11 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -2,16 +2,36 @@ ## Supported versions -| Version | Supported | -|---------|-----------| -| 2.0.x | ✅ | -| 1.x | ❌ — see the note below | - -Version 1.x is not supported and should not be used. It stored the Procore client secret -and access token as plaintext in an autoloaded WordPress option, ran its connection test -from an unverified `$_POST` key, and registered its settings with no sanitize callback. -It also could not authenticate against the live API. Upgrade to 2.0.0 and rotate any -credentials that were stored by 1.x, on the assumption they were exposed. +| Version | Supported | Notes | +|---------|-----------|-------| +| 2.0.3 | ✅ | Current | +| 2.0.2 | ⚠️ | Safe, but does not repair a credential already damaged by 2.0.0 or 2.0.1 | +| 2.0.0 – 2.0.1 | ❌ | **Destroy the stored client secret.** See below | +| 1.x | ❌ | Stores credentials in plaintext. See below | + +### 2.0.0 and 2.0.1 — stored credential destroyed + +These versions attached the settings sanitizer to `sanitize_option_{$option}` and +encrypted the client secret unconditionally. WordPress runs that filter on *every* +`update_option()` for the option, including the plugin's own internal writes, so the +stored secret gained an encryption layer on each write until it could no longer be +decrypted. The site then failed to authenticate with nothing on screen to explain why. + +This is a data-integrity fault rather than a disclosure: the value was over-encrypted, +never exposed. Upgrade to **2.0.3**, which unwraps and restores the value automatically +and reports what it did. If it cannot be recovered the secret is cleared and you are +asked to re-enter it. + +### 1.x — credentials stored in plaintext + +Version 1.x stored the Procore client secret, access token and refresh token as plaintext +in an **autoloaded** WordPress option, read into memory on every page request and present +in every database backup. It also ran its connection test from an unverified `$_POST` key +and registered its settings with no sanitize callback, and it could not authenticate +against the live API at all. + +Upgrade to 2.0.3 and **rotate any credentials 1.x stored**, on the assumption they were +exposed. ## Reporting a vulnerability diff --git a/phpcs.xml.dist b/phpcs.xml.dist index 0aed712..d2dfab8 100644 --- a/phpcs.xml.dist +++ b/phpcs.xml.dist @@ -81,8 +81,13 @@ /tests/wp-shims.php + - /tests/wp-shims.php + /tests/* /tests/wp-shims.php diff --git a/procore-connect.php b/procore-connect.php index 5de4d46..db5b2ed 100644 --- a/procore-connect.php +++ b/procore-connect.php @@ -3,7 +3,7 @@ * Plugin Name: Procore Connect * Plugin URI: https://github.com/ibuilder/ProcoreWP * Description: Connect WordPress to the Procore construction management platform. Display projects, teams, drawings, RFIs and more with shortcodes, blocks and a cached REST proxy. - * Version: 2.0.3 + * Version: 2.0.4 * Requires at least: 6.5 * Requires PHP: 7.4 * Author: ibuilder @@ -22,7 +22,7 @@ defined( 'ABSPATH' ) || exit; -const VERSION = '2.0.3'; +const VERSION = '2.0.4'; define( 'PROCORE_CONNECT_VERSION', VERSION ); define( 'PROCORE_CONNECT_FILE', __FILE__ ); diff --git a/readme.txt b/readme.txt index 6f3839d..22e633a 100644 --- a/readme.txt +++ b/readme.txt @@ -4,7 +4,7 @@ Tags: procore, construction, project management, shortcode, api Requires at least: 6.5 Tested up to: 7.0 Requires PHP: 7.4 -Stable tag: 2.0.3 +Stable tag: 2.0.4 License: GPLv2 or later License URI: https://www.gnu.org/licenses/gpl-2.0.html @@ -127,6 +127,11 @@ Yes. Caching goes through the transient API, so a persistent object cache such a == Changelog == += 2.0.4 = + +* Security documentation no longer recommends 2.0.0 or 2.0.1, the versions that damage the stored Client Secret. Both are now marked unsupported with the remedy stated plainly. +* Added test coverage for log redaction, which keeps secrets out of debug.log, and for the theme template override chain. + = 2.0.3 = * **Repairs credentials damaged by 2.0.0 or 2.0.1.** Those versions re-encrypted the stored Client Secret on every settings save until it could no longer be read, leaving the site unable to authenticate with no explanation. 2.0.2 stopped the damage; 2.0.3 undoes it. The upgrade unwraps the value, re-stores it correctly, and tells you what it did. If it cannot be recovered it is cleared and you are asked to enter it again. @@ -178,6 +183,9 @@ A complete rewrite. See the upgrade notice below before updating. == Upgrade Notice == += 2.0.4 = +Documentation and test coverage only. No functional change from 2.0.3. + = 2.0.3 = Repairs a Client Secret damaged by 2.0.0 or 2.0.1, which re-encrypted it on every save until it stopped working. Upgrading restores it automatically and reports the result. Recommended for anyone who ran 2.0.0 or 2.0.1. diff --git a/tests/unit/LoggerTest.php b/tests/unit/LoggerTest.php new file mode 100644 index 0000000..030d58b --- /dev/null +++ b/tests/unit/LoggerTest.php @@ -0,0 +1,152 @@ +assertSame( array(), Logger::entries() ); + } + + /** + * An enabled logger records the message and its level. + * + * @return void + */ + public function test_records_message_and_level(): void { + Logger::warning( 'throttled' ); + + $entries = Logger::entries(); + + $this->assertCount( 1, $entries ); + $this->assertSame( 'warning', $entries[0]['level'] ); + $this->assertSame( 'throttled', $entries[0]['message'] ); + } + + /** + * Credentials passed as context must never be written. + * + * @return void + */ + public function test_redacts_credentials_from_context(): void { + Logger::error( + 'auth failed', + array( + 'client_secret' => 'sk_live_should_never_appear', + 'access_token' => 'tok_should_never_appear', + 'refresh_token' => 'ref_should_never_appear', + 'code' => 'authcode_should_never_appear', + 'password' => 'pw_should_never_appear', + 'status' => 401, + ) + ); + + $dump = (string) wp_json_encode( Logger::entries() ); + + foreach ( array( 'sk_live_should_never_appear', 'tok_should_never_appear', 'ref_should_never_appear', 'authcode_should_never_appear', 'pw_should_never_appear' ) as $secret ) { + $this->assertStringNotContainsString( $secret, $dump ); + } + + // Non-sensitive context is still useful and must survive. + $this->assertStringContainsString( '401', $dump ); + } + + /** + * Redaction must reach credentials nested inside arrays. + * + * @return void + */ + public function test_redacts_nested_credentials(): void { + Logger::error( + 'request failed', + array( + 'request' => array( + 'headers' => array( 'Authorization' => 'Bearer leaked_token_value' ), + 'body' => array( 'client_secret' => 'nested_secret_value' ), + ), + ) + ); + + $dump = (string) wp_json_encode( Logger::entries() ); + + $this->assertStringNotContainsString( 'leaked_token_value', $dump ); + $this->assertStringNotContainsString( 'nested_secret_value', $dump ); + $this->assertStringContainsString( '[redacted]', $dump ); + } + + /** + * Long context values are truncated so the log cannot balloon. + * + * @return void + */ + public function test_truncates_very_long_context_values(): void { + Logger::info( 'big', array( 'body' => str_repeat( 'x', 5000 ) ) ); + + $entries = Logger::entries(); + + $this->assertLessThanOrEqual( 500, strlen( (string) $entries[0]['context']['body'] ) ); + } + + /** + * The ring buffer must stay bounded, newest first. + * + * @return void + */ + public function test_ring_buffer_is_bounded_and_newest_first(): void { + for ( $i = 0; $i < 60; $i++ ) { + Logger::info( 'entry ' . $i ); + } + + $entries = Logger::entries(); + + $this->assertCount( 50, $entries ); + $this->assertSame( 'entry 59', $entries[0]['message'] ); + } + + /** + * Clearing must empty the buffer. + * + * @return void + */ + public function test_clear_empties_the_buffer(): void { + Logger::info( 'something' ); + Logger::clear(); + + $this->assertSame( array(), Logger::entries() ); + } +} diff --git a/tests/unit/RendererTest.php b/tests/unit/RendererTest.php new file mode 100644 index 0000000..1d18292 --- /dev/null +++ b/tests/unit/RendererTest.php @@ -0,0 +1,178 @@ +child = get_stylesheet_directory() . '/procore-connect'; + $this->parent = get_template_directory() . '/procore-connect'; + + foreach ( array( $this->child, $this->parent ) as $dir ) { + if ( ! is_dir( $dir ) ) { + mkdir( $dir, 0777, true ); + } + } + + $this->remove_overrides(); + } + + /** + * Remove anything a test wrote into the theme directories. + * + * @return void + */ + protected function tearDown(): void { + $this->remove_overrides(); + + parent::tearDown(); + } + + /** + * Delete the override files used by these tests. + * + * @return void + */ + private function remove_overrides(): void { + foreach ( array( $this->child, $this->parent ) as $dir ) { + $files = glob( $dir . '/*.php' ); + + foreach ( is_array( $files ) ? $files : array() as $file ) { + unlink( $file ); + } + } + } + + /** + * With no override present, the plugin's own template is used. + * + * @return void + */ + public function test_falls_back_to_the_plugin_template(): void { + $located = Renderer::locate( 'collection' ); + + $this->assertStringContainsString( 'templates/collection.php', str_replace( '\\', '/', $located ) ); + $this->assertFileExists( $located ); + } + + /** + * A parent theme override beats the plugin default. + * + * @return void + */ + public function test_parent_theme_overrides_the_plugin(): void { + file_put_contents( $this->parent . '/collection.php', 'assertSame( + str_replace( '\\', '/', $this->parent . '/collection.php' ), + str_replace( '\\', '/', Renderer::locate( 'collection' ) ) + ); + } + + /** + * A child theme override beats the parent theme. + * + * @return void + */ + public function test_child_theme_wins_over_parent(): void { + file_put_contents( $this->parent . '/collection.php', 'child . '/collection.php', 'assertSame( + str_replace( '\\', '/', $this->child . '/collection.php' ), + str_replace( '\\', '/', Renderer::locate( 'collection' ) ) + ); + } + + /** + * An override actually renders, and receives the template data. + * + * @return void + */ + public function test_override_renders_and_receives_data(): void { + file_put_contents( $this->child . '/collection.php', 'assertSame( 'OVERRIDE:Projects', Renderer::render( 'collection', array( 'title' => 'Projects' ) ) ); + } + + /** + * A directory traversal attempt must not escape the template directories. + * + * @return void + */ + public function test_refuses_directory_traversal(): void { + foreach ( array( '../../wp-config', '..\\..\\wp-config', 'collection/../../../secret' ) as $attempt ) { + $located = Renderer::locate( $attempt ); + + $this->assertStringNotContainsString( 'wp-config', $located ); + $this->assertStringNotContainsString( 'secret', $located ); + } + } + + /** + * An unknown template resolves to nothing rather than erroring. + * + * @return void + */ + public function test_unknown_template_resolves_to_nothing(): void { + $this->assertSame( '', Renderer::locate( 'does-not-exist' ) ); + $this->assertSame( '', Renderer::render( 'does-not-exist', array() ) ); + } + + /** + * An empty name must not resolve to a directory. + * + * @return void + */ + public function test_empty_template_name_resolves_to_nothing(): void { + $this->assertSame( '', Renderer::locate( '' ) ); + } + + /** + * Public notices must be escaped and carry a sanitized modifier class. + * + * @return void + */ + public function test_notice_escapes_its_message(): void { + $html = Renderer::notice( '', 'error' ); + + $this->assertStringNotContainsString( '