Harden multisite authorization and stop logging integration credentials - #1958
Open
PatelUtkarsh wants to merge 5 commits into
Open
Harden multisite authorization and stop logging integration credentials#1958PatelUtkarsh wants to merge 5 commits into
PatelUtkarsh wants to merge 5 commits into
Conversation
…s writes Addresses authorization findings from a security review of v4.3.0. Multisite record isolation (Network): Network::network_query_args() preserved any numeric blog_id coming from the request, but a numeric type check is not an authorization check and the Stream tables are shared across the network -- a site-level view_stream user could read another site's activity with ?blog_id=N. Requested blog IDs are now honoured only for users with manage_network_options; everyone else is pinned to the current blog. The read abilities (get-record, get-records, purge-records) already carried equivalent guards; this closes the legacy list-table and AJAX path. ajax_network_admin() also derived network-admin authority purely from an HTTP_REFERER prefix, which is caller-controlled. That let a site user lift the per-blog query restriction and, via blog_id_logged(), record their actions against blog_id 0 and corrupt site attribution. The Referer is now treated as a UI hint only and must be accompanied by a real network capability. WP-CLI is exempt since it has shell-level access and normally no logged-in user. Network-wide settings writes (Ability): update-settings and create-exclusion-rule inherited the default permission callback (WP_STREAM_SETTINGS_CAPABILITY, i.e. manage_options) while Settings::update_all_setting_values() routes to update_site_option() on network-activated installs. A site administrator could therefore change network-wide retention, role access, and audit exclusion rules. Both now use Ability::can_write_settings(), which additionally requires manage_network_options when the write will be network-scoped. GHCR publishing (docker-images.yml): The publish step was guarded by contains(github.ref_name, 'master'), which also matches unprotected branch names such as feature-master-publish. Replaced with an exact ref comparison. This guards against an accidental publish only. The condition is read from the pushed ref's own copy of the workflow, so someone with write access could still edit it on their own branch; closing that requires a deployment branch policy configured in repository settings, which is outside this change.
…tion Adds regression coverage for the boundaries hardened in the previous commit. WP_Stream\Network previously had almost no direct coverage (4.76% of methods), which is how the can_write_settings() either/or bug below went unnoticed through two full green test runs. tests/phpunit/test-class-network.php (new): - A site administrator and a subscriber both have a caller-supplied blog_id discarded in favour of the current blog. - A super admin still gets cross-site filtering, so Network Admin is unaffected. - site_id keeps its existing default. - can_view_network_records() is false for a site administrator and true for a super admin, so a spoofed Referer alone cannot establish network context. - blog_id_logged() keeps site attribution for a site user. test-class-ability-update-settings.php: - A site administrator cannot write network-wide settings. - A super admin is denied when the Stream settings capability is revoked. This pins the AND semantics of can_write_settings(): the network capability is an additional requirement, not a substitute. Note WP_User::has_cap() returns early for super admins before the user_has_cap filter runs, so the test uses map_meta_cap/do_not_allow -- the one restriction that early return honours. Each test was verified to fail against the unfixed code and pass against the fix. Both suites: 401 tests, 0 failures; skipped/incomplete counts unchanged from the develop baseline.
…APIs
Alert destination credentials (get-alerts):
stream/get-alerts requires only view_stream, but alert destinations are
configured behind the Stream settings capability, so returning alert_meta
verbatim dropped credentials across a privilege boundary. A Slack incoming
webhook URL and an IFTTT Maker key are both bearer credentials -- possession
alone is enough to post into the channel or fire the account's applets.
Ability::redact_alert_meta() now replaces those values with a
`{key}_configured` boolean before output, so callers can still tell whether a
destination is set up without receiving the secret. Non-secret configuration
(channel, username, event_name) is untouched. The helper also absorbs the
existing empty-meta-to-stdClass normalization the output schema requires.
Payment gateway credentials (WooCommerce connector):
callback_updated_option() serialized entire third-party gateway settings
arrays into record metadata. Gateways routinely co-locate operational
settings with live API secrets and webhook signing keys, so an enabled
credential-bearing gateway persisted those secrets into stream_meta where any
Stream viewer or record-detail API consumer could read them.
Adds Connector::is_secret_key() / redact_secret_values(), applied to both
old and new values before serialization. Matching is substring/suffix based
rather than an allowlist because connectors log option arrays belonging to
plugins we do not control, and an allowlist cannot anticipate their field
names. Over-redacting costs a little audit detail; under-redacting persists a
live credential. The helper lives on the Connector base class because the
remaining unredacted-secret findings need the same logic.
Note the pattern list was corrected while writing the tests: 'password' did
not match mailserver_pass and no pattern matched rg_gforms_key, both of which
are real targets. A bare 'key' suffix was also rejected as it matched
harmless words such as monkey.
Each redaction test was verified to fail against the unredacted code. Both
suites: 408 tests, 0 failures.
Applies the Connector redaction helper added in the previous commit to the remaining settings that were persisted verbatim. Settings connector (mailserver_pass): callback_updated_option() passed values through sanitize_value(), which only flattens complex types to strings and has no notion of sensitivity, so a reusable mailbox password was stored as both old_value and value. Redaction is now applied per field at the two call sites where the setting name is known -- sanitize_value() itself receives no key and cannot make the decision. Gravity Forms connector (rg_gforms_key, rg_gforms_captcha_private_key): check() logged old/new values for every tracked option including the reCAPTCHA private key, and check_rg_gforms_key() explicitly logged both halves of a license-key change. Both now redact before logging. In the license case the update/delete status is derived before redaction, so the message still reports which happened -- covered by a test. The change also refines the pattern list from the previous commit, driven by test failures rather than assumption: - rg_gforms_captcha_public_key was being over-redacted by the '_key' suffix rule. Public halves of key pairs are meant to be published and redacting them removes audit detail for no security benefit, so PUBLIC_KEY_PATTERNS now exempts public_key / publishable_key / site_key ahead of the secret match. A test asserts the public key survives while the private one does not. - The earlier assertion that 'publishable_key' should be treated as secret was wrong and has been corrected. Each redaction test was verified to fail without the fix while the non-secret control tests continued to pass, confirming they discriminate rather than redacting everything. Both suites: 414 tests, 0 failures.
1. Secret markers now beat the public-name exemption. PUBLIC_KEY_PATTERNS was checked first and returned early, so a name such as secret_site_key or webhook_public_key escaped redaction despite carrying an explicit secret marker -- inverting the over-redact-rather-than-under-redact preference the previous commit claimed. The exemption exists only to stop the broad "_key" suffix rule from catching published key halves, so it is now consulted after the secret substrings and before the suffix rule. 2. REDACTED_PLACEHOLDER is '[redacted]' rather than ''. The empty string made a withheld credential indistinguishable from a cleared field, contradicting the stated rationale of the empty-value test. Unset credentials still log as '' so "never set" and "set but withheld" remain distinguishable; the test now asserts both directions. 3. Extends redaction to EDD and Jetpack, which logged raw old/new option values. EDD is the notable one: it logs arbitrary settings fields including payment gateway API keys. BuddyPress was examined and left alone -- it only logs component activation booleans and page IDs, so there is no credential to redact and a call would be dead code. 4. Adds the wp_stream_secret_alert_meta_keys filter so third-party alert types registered via wp_stream_alert_types can have their own destination secrets redacted from get-alerts output. Tests assert against Connector::REDACTED_PLACEHOLDER rather than a literal so they track the constant. The precedence fix and the filter both have tests verified to fail against the previous behaviour. Both suites: 416 tests, 0 failures.
PatelUtkarsh
marked this pull request as ready for review
August 4, 2026 09:27
PatelUtkarsh
force-pushed
the
fix/multisite-authorization-hardening
branch
from
August 4, 2026 09:38
0b0152c to
a81c3f5
Compare
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
1 participant
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.
Security fixes from a review of v4.3.0.
get-alertsreturned alert destination secrets to lower-privileged viewers, and several connectors logged credentials into record metadata.Checklist
contributing.md).Release Changelog
stream/get-alertsresponses.masterbranch.Release Checklist
Not a release PR — targets
develop.