fix: audit improvements — bugs, security, PHP 8.1 types, PHP 8.4 compat#6
Draft
tccengineering wants to merge 14 commits into
Draft
fix: audit improvements — bugs, security, PHP 8.1 types, PHP 8.4 compat#6tccengineering wants to merge 14 commits into
tccengineering wants to merge 14 commits into
Conversation
- Typo $commend → $comment caused undefined variable return from get_by_id() - update_comment() discarded get_by_id() result, always returned null - create_comment() called set_comment_meta() without the required $comment argument - delete_comment() used $comment->ID instead of $comment->comment_ID Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- get() previously overrode \$default immediately with the empty config array; now returns \$default correctly when the config name does not exist - glob() can return false on failure; use ?: [] to ensure a safe iterable - Sanitise WP_ENV before interpolating into a filesystem glob path Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Sanitise REQUEST_URI with wp_unslash/sanitize_url before use - Replace strtok() (stateful) with parse_url() to safely strip query string - Check the return value of preg_match() directly instead of checking whether the \$matches array is non-empty; the old check silently swallowed regex errors Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace extract() in register_endpoints() and build_route() with explicit array key access; extract() on arbitrary data pollutes the local namespace - Emit _doing_it_wrong() when an endpoint is registered without a permission_callback so developers are notified rather than silently getting public access by default Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- endpoint() now accepts an optional \$auth_only flag (default false); when true, only the wp_ajax_ hook is registered so the endpoint is restricted to logged-in users — existing callers are unaffected - Add static verify_nonce() helper so callbacks have a one-liner to validate nonces and die with 403 on failure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- wp_mail filters were registered anew on every EmailView instantiation and could never be removed; use a static flag so they are registered once - shortcodes() used preg_replace() where the replacement string is parsed for \$1/\\1 backreferences; replace with preg_replace_callback() returning the value via closure so special characters in parameter values are treated literally Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
gettype() === 'object' accepts any object; instanceof checks the exact expected type and is more readable and type-safe. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use ?? throughout rather than the verbose isset() ? x : y form. Also simplify Templater constructor by assigning directly to properties. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Both methods were copy-pasted verbatim into all four concrete factories. Defining them once in WPModelFactory eliminates the duplication; each factory's set_*_meta() method now delegates to apply_meta_fields(). Also declare abstract wrap() on WPModelFactory to enforce the contract. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- ControllerSetup does not exist in the codebase; remove the use statement - 13 commented-out assert() calls scattered across model and view files served no purpose and added noise; removed entirely Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Typed properties on all classes (string, array, ?Config, ?\WP_Post, etc.) - Parameter and return types on every public/protected method - WPMeta and WPTaxonomyTerms interfaces updated with typed signatures; all concrete implementations updated to match - Templater: fix bug introduced in null-coalescing commit where \$slug local variable was removed but still referenced in template_slug assignment - CommentModel::update() also fixed to use comment_ID key (not ID) - UserModelFactory PHPDoc @param corrected from WP_Post to WP_User - Render return type expressed as string|false|null to reflect actual behaviour: outputs directly when \$output=true (returns null), returns string|false when \$output=false Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Dynamic properties are deprecated since PHP 8.2 and remain E_DEPRECATED in PHP 8.4. Five model classes use __set() to intentionally forward unknown property writes as dynamic properties on the model — mark them with #[\AllowDynamicProperties] to make the intent explicit and silence the deprecation. PHP 9 will require this attribute for any class that creates dynamic properties. Also fixes two issues in EmailView: - Static property $filters_registered lacked a type declaration (untyped static properties are deprecated in PHP 8.4) - $content_templater->subject = $subject wrote a dynamic property onto Templater (same deprecation) and was also a latent bug: the value was never passed into $this->params so it was invisible to extract() inside the template. Pass it via the params array at construction instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: TheCodeCompany/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Application: collapse three controller setup loops into one pass, cutting apply_filters() calls from 6N to 6 per controller and fixing a bug where filter-replaced instances were lost between loops - REST: remove redundant current_request assignment and set_current_endpoint() call in handle_callback(); handle_perm_callback() always runs first so the work was already done, wasting a full endpoint regex scan per request - EmailView: fix send() returning false for all array-recipient calls regardless of outcome; now returns true only if every recipient succeeds Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Config::get()default overwrite,Routestrtok/match check,RESTextract() removal,EmailViewsubject param never reaching templateREQUEST_URIsanitised viasanitize_url(wp_unslash(...)),WP_ENVrestricted to[a-zA-Z0-9_-],preg_replacebackreference injection closed viapreg_replace_callback,_doing_it_wrong()warning when RESTpermission_callbackis omitted#[\AllowDynamicProperties]on model classes;Templaterdynamic property bug fixed; untyped static properties resolvedWPModelFactorygainswrap_models()andapply_meta_fields()base methods (duplicate code removed from all four factories);AdminAjax::$auth_onlyparameter andverify_nonce()helper addedEmailView::send()multi-recipient return value fixedCommits (13)
Test plan
CommentModelFactory::get_by_id(),create_comment(),delete_comment()against a real WordPress installConfig::get()returns$defaultwhen key is absentEmailViewand confirm{{ subject }}is interpolated in the body templatepermission_callbackand confirm_doing_it_wrong()fires$auth_only = trueand confirm unauthenticated requests 403E_DEPRECATEDdynamic-property notices under PHP 8.4handle_perm_callback+handle_callbackboth resolve correctly with a singleset_current_endpoint()callEmailView::send()and confirm the return value reflects actual send results🤖 Generated with Claude Code