security: fix the 3.8.2 audit findings (3.9.0) - #17
Conversation
…ting probe validator Groundwork for the 3.8.2 ClawHub audit fixes. Adds three primitives to scripts/security.py; no call site is routed through them yet. urlopen_authenticated() is urlopen with a redirect handler that drops the Authorization header when a redirect leaves the origin. CPython's HTTPRedirectHandler copies every header except content-length and content-type onto the redirected request, so a 30x pointing at another host hands the Basic-Auth application password to that host verbatim. Verified against two local servers before and after: the cross-host target received "Basic dXNlcjpTRUNSRVQtQVBQLVBBU1NXT1JE" before, and nothing after. Same-origin redirects keep the header, so trailing-slash and canonical-URL behaviour is unaffected. requests already strips it, so the four requests-based scripts were never exposed to this. validate_probe_url() is the site audit's missing boundary: http:// is allowed, because detecting an HTTPS redirect is one of the audit's own checks, but any host resolving to a private, loopback, link-local, multicast, reserved or unspecified address is refused. The address check itself moved into _assert_public_host() so it is shared with validate_remote_url() rather than duplicated; a DNS-rebinding window remains between the check and urlopen's own resolution, and the docstring says so. 37 existing tests pass; compileall clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…authenticated The nine scripts that send an Authorization header with urllib called urllib.request.urlopen directly, and CPython's HTTPRedirectHandler copies every header except content-length/content-type onto a redirected request - so a redirect to another host carried the site's app password with it. All eleven call sites now go through security.urlopen_authenticated, which strips Authorization when a redirect leaves the origin (scheme, host or effective port). The four scripts that authenticate through requests are unaffected: requests.SessionRedirectMixin.rebuild_auth already does this. The create_post/upload_media tests patched urllib.request.urlopen, which is no longer the seam those scripts call; they now patch the module's own urlopen_authenticated. test_site_audit still patches urllib.request.urlopen because the site audit is unauthenticated and keeps calling it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…reaches site_audit._get accepted any URL and _ssl_notafter opened a TLS connection to whatever host came back, so "audit http://192.168.1.1/" or a redirect to 169.254.169.254 turned the skill into an SSRF primitive against whatever the agent's host can reach. The audit now opens through security.urlopen_probe, which validates the URL and re-validates the target of every redirect it follows - validating only the caller's URL is not enough, since a public site is free to answer a 302 to an internal address. _ssl_notafter validates its bare hostname through validate_probe_host, and main() validates up front so a refusal reads as a safety error rather than as an unreachable site. audit() catches SafetyError separately for the same reason: a blocked address is a refusal, not a site that failed to respond. http:// stays permitted here - detecting a missing HTTPS redirect is one of the audit's own checks - which is why this is a separate validator from validate_remote_url rather than a loosening of it. Tests: same_origin's truth table, the redirect handler dropping Authorization cross-origin (and keeping it on a WordPress canonical-URL redirect), the probe validators, and a static invariant that no script sending an Authorization header calls urllib.request.urlopen directly. Every address case uses an IP literal, so no test resolves a name over the network. The create_post, upload_media and site_audit tests now patch the seam their script actually calls. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…d dataset path Two defaults that were advisory become enforced. http:// to a public host printed "SECURITY WARNING" and then sent the app password in the clear anyway. A warning nobody reads is not a control, and an app password read off the wire is the whole site. check_wp_url_scheme now refuses; WP_ALLOW_HTTP=1 restores the warn-and-continue behaviour for a plaintext staging host, and WP_REQUIRE_HTTPS=1 still refuses and wins over it, so an environment that pinned it stays strict. Local hosts (localhost and the .local/.test/.localhost suffixes) are exempt exactly as before - there is no wire for a credential to leak on. warn_insecure_wp_url is renamed require_secure_wp_url at all fourteen call sites: the name described the old behaviour, and the new one has to exit 2 through die_safety rather than raise an uncaught traceback at the CLI boundary. check_wp_url_scheme is the pure half that raises, so the policy is testable without SystemExit. seed_content read its --dataset with a bare open(), so it would read whatever path the agent named; it now goes through validate_local_file with the 2 MB text ceiling, like every other local read in the skill. BREAKING CHANGE: a site configured over http:// now fails instead of warning. Set WP_ALLOW_HTTP=1 to keep it working, or move the site to https://. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… on refusal
A friendly key outside the plugin's allowlist was written as a raw postmeta
key with a warning, so a typo'd name silently created a junk meta row - or
overwrote a key another plugin owns. WP_REQUIRE_ALLOWLIST=1 existed but was
opt-in, which is the wrong way round for a write that is hard to notice and
hard to undo.
_map_meta_keys now refuses an unknown key. WP_ALLOW_RAW_META=1 restores the
write-with-a-warning behaviour for a deliberately custom key, and
WP_REQUIRE_ALLOWLIST=1 still refuses and wins over it.
seo_meta reports failure as {"error": ...} rather than by raising, so a
refused write printed its error and still exited 0 - which CI and an agent
both read as success. Both result paths now exit 1 on an error result.
BREAKING CHANGE: writing a non-allowlisted SEO meta key now fails. Set
WP_ALLOW_RAW_META=1 to keep writing raw postmeta keys.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The SKILL.md permissions block declared shell: "none (Python only; no shell-out)". That was false - wp_cli.py spawns python3 <script> subprocesses (argv list, never shell=True) and wp.sh is a bash wrapper around it. A scanner comparing the declaration with the code reads that as a lie about the skill's capabilities, which is how it should read it. SKILL.md also told users to run "bash INSTALL.sh" from the skill directory, but that installer lives in the git repo and is not part of the packaged skill, so the instruction failed for everyone who installed from ClawHub. The text now names where the scripts actually live for each install route, rather than moving the installer into the payload. requests was unpinned. requirements.txt now ships inside the skill with requests>=2.32.3 - 2.32.0 fixed CVE-2024-35195, where a Session that made one verify=False request silently skipped certificate verification for later requests to the same host. Two more from the audit: jetengine_fields --list-all was declared but never read, so passing it silently returned the same filtered output (it now includes private underscore-prefixed meta as documented), and the table block example in references/gutenberg-blocks.md closed with <!-- /wp:heading -->. Version 3.9.0, not 3.8.3: two defaults change behaviour for existing users. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Validating the audit's addresses meant a hostname that does not resolve raised SafetyError, so main() exited 2 with "Safety error: Could not resolve host" before audit() printed anything - where 3.8.2 printed the unreachable JSON and exited non-zero. A typo'd domain is not an attempt to reach internal infrastructure, and a machine-readable contract should not change shape because the target does not exist. _assert_public_host now raises HostResolutionError, a SafetyError subclass, so callers that only care "this was refused" (the media and local-file paths) are unchanged, while site_audit tells the two apart: an unresolvable name is reported as a site that did not respond, a refused address as blocked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
wordpress-api-pro/wordpress-api-pro/scripts/security.py
Lines 118 to 122 in bf9cf3c
Reject non-global shared address space
When the audit target or a redirect resolves to RFC 6598 shared space, such as 100.64.0.1, every predicate here is false (ipaddress reports it as neither private nor reserved), so site_audit http://100.64.0.1/ passes validation and attempts the connection. On hosts with services reachable through carrier-grade NAT space, this leaves the newly added SSRF boundary bypassable; shared/non-global address ranges must also be rejected.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| req = urllib.request.Request(url, method='GET') | ||
| req.add_header('Authorization', auth) | ||
| with urllib.request.urlopen(req) as r: | ||
| with urlopen_authenticated(req) as r: |
There was a problem hiding this comment.
When describe_cpt.py is run directly with a public plaintext URL such as --url http://example.com, this redirect-safe opener still sends the Basic Authorization header on the initial request. Unlike the other authenticated CLIs changed here, main() never calls require_secure_wp_url, so the new default refusal and WP_ALLOW_HTTP opt-in are bypassed for this documented command.
Useful? React with 👍 / 👎.
describe_cpt sends a Basic Authorization header but its main() was the one authenticated CLI that never called the guard, so "describe_cpt --url http://example.com" put the app password on the wire in plaintext - the 3.9.0 refusal and its WP_ALLOW_HTTP opt-out both passed it by. The routing commit gave it a redirect-safe opener, which protects the redirect but not the initial request. The regression test is the static invariant beside the urlopen one: every script that sends an Authorization header must call require_secure_wp_url. Verified it fails with the guard removed, so it is not vacuous. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in
Rather than just adding the one line, the regression test is a static invariant beside the existing urlopen one: every script that sends an Two other commits landed since the reviewed 68 tests passing. @codex review |
|
@codex review |
|
Codex Review: Something went wrong. Try again later by commenting “@codex review”. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76ef6beb2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for address in addresses: | ||
| if any( |
There was a problem hiding this comment.
When site_audit runs in an environment that routes RFC 6598 shared address space, a target such as http://100.64.0.1/ passes this denylist because Python reports that address as neither private, loopback, link-local, multicast, reserved, nor unspecified, even though it is not globally reachable. This leaves the new unauthenticated audit usable to probe internal CGNAT/shared-network services; reject addresses where address.is_global is false rather than enumerating only selected non-public categories.
Useful? React with 👍 / 👎.
…listed one Enumerating non-public categories misses whatever the enumeration forgot, and it forgot RFC 6598 shared address space: 100.64.0.1 is none of private, loopback, link-local, multicast, reserved or unspecified to Python, so it passed - and it is routable on every network running CGNAT, which is most consumer ISPs and a good deal of cloud infrastructure. Verified against the stdlib before fixing: 100.64.0.1 sets no flag this code was testing. is_global is now the allowlist half and carries the rule. The named flags stay as the deny half rather than being replaced by it: 64:ff9b::/96 (NAT64) reports is_global True and is still not somewhere this skill should be pointed. This tightens validate_remote_url (media downloads) by the same change, since both validators share the helper. Reported by Codex on PR #17. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Confirmed and fixed in
Note this also tightens 72 tests passing. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64c928a03e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return True | ||
|
|
||
| warn_insecure_wp_url(site['url']) | ||
| require_secure_wp_url(site['url']) |
There was a problem hiding this comment.
When an executed batch contains an HTTPS site followed by a non-local HTTP site, earlier posts are modified before require_secure_wp_url raises SystemExit for the later site, abruptly terminating the batch without its summary or processing remaining targets. The dry-run cannot reveal this because it returns before the guard. Validate every selected site's URL before confirmation and before the first write so this deterministic safety refusal cannot leave a partially applied batch.
Useful? React with 👍 / 👎.
| @@ -117,7 +117,7 @@ def run_command(site_config, command, args): | |||
| print("Error: Missing credentials. Set WP_URL, WP_USERNAME, WP_APP_PASSWORD or use config file", file=sys.stderr) | |||
There was a problem hiding this comment.
When wp_cli.py executes a group whose later member uses a non-local HTTP URL, commands for earlier members complete before this guard exits the entire wrapper on the insecure member. For write commands that leaves the group only partially processed; preflight all group URLs before entering the execution loop rather than validating one target at a time in run_command.
Useful? React with 👍 / 👎.
Turning the http:// warning into a refusal gave it a failure mode the warning never had: it fires inside the per-site loop. A batch with an https site followed by a non-local http one modified the first site, exited 2 on the second, and printed no summary - and the dry run could not reveal it, because the dry-run path returns before the guard. batch_update and wp_cli now validate the whole selection before launching anything, through security.require_secure_wp_urls, which reports every offending site rather than only the first one a loop happens to reach. The per-site guard stays where it was: preflight means it never fires mid-loop, not that nothing should check. A dry run reports the same problems as warnings without exiting. It makes no requests, so there is nothing to refuse, and planning is exactly when a user wants to learn that one of twelve sites is configured over http. Reported by Codex on PR #17 (two findings, same shape). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both fixed in
On the dry-run half of your first note: a dry run now reports the same problems as warnings without exiting. It makes no requests, so there is nothing to refuse, and planning is exactly when someone wants to learn that one of twelve sites is configured over http. Tests cover that every offender is named (not just the first), the exit code, and - as static assertions - that the preflight precedes the loop in both files. 77 tests passing, plus the repo's own @codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d527387c8f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| except SafetyError as e: | ||
| add("reach", "blocked", str(e), "fail", "refused by the address safety rule") | ||
| return {"url": url, "reachable": False, "findings": findings} |
There was a problem hiding this comment.
When an initially public site redirects to a private or otherwise forbidden address, this branch records a distinct blocked finding, but --summary discards every finding whenever reachable is false and prints only Site unreachable.. That misreports a deliberate safety refusal as a connectivity failure and hides the offending address; preserve the blocked reason in summary mode.
Useful? React with 👍 / 👎.
The blocked finding added earlier in this PR only reached the JSON output. _summary discarded every finding whenever reachable was false and printed a flat "Site unreachable.", so in the mode a human actually reads, a deliberate safety refusal was indistinguishable from a site that was down - and the address that was refused never appeared at all. The summary now renders whatever the audit recorded, and falls back to that line only when there is nothing to show. An unresolvable host still reads as "site did not respond", from its own finding rather than from a hardcoded string. Reported by Codex on PR #17. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in The summary now renders whatever the audit recorded and falls back to "Site unreachable." only when there is nothing to show. An unresolvable host still reads as "site did not respond" - but from its own finding rather than from a hardcoded string, so the two outcomes stay distinct in both output modes. 80 tests passing. @codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Closes out the ClawHub security audit of 3.8.2 (
wordpress-api-pro-3.8.2-security-audit, sha2565572bdc…88c87).What was actually wrong
1. Credentials crossed hosts on a redirect. Nine scripts sent an
Authorizationheader through a bareurllib.request.urlopen. CPython'sHTTPRedirectHandler.redirect_requestcopies every header exceptcontent-length/content-typeonto the redirected request, so a redirect to another host carried the site's app password with it. Proven with a two-server experiment before the fix, and again after:Same-origin redirects still carry the header, so WordPress canonical-URL behaviour is untouched.
The audit reported 13 affected scripts. It is nine:
acf_fields,detect_plugins,jetengine_fieldsandseo_metaauthenticate throughrequests, whoseSessionRedirectMixin.rebuild_authalready strips the header. That is library behaviour I did not re-verify locally, becauserequestsis not installed in this environment.2. The no-auth site audit was an SSRF primitive.
_getaccepted any URL and_ssl_notafteropened a TLS connection to whatever host came back.site_audit http://192.168.1.1/reached it, and so did a public site answering302 http://169.254.169.254/. Validating only the caller's URL would not have closed the second case, sourlopen_probere-validates the target of every redirect it follows.3. Three guards existed in
security.pyand were simply not wired up —validate_local_fileforseed_content's--dataset, and the two defaults below.Breaking changes
http://to a non-local host is refused (was: warning, then sent the password anyway)WP_ALLOW_HTTP=1WP_ALLOW_RAW_META=1localhostand the.local/.test/.localhostsuffixes stay exempt from the first.WP_REQUIRE_HTTPS=1andWP_REQUIRE_ALLOWLIST=1still refuse and win over the new hatches, so an environment that pinned either stays strict. Hence 3.9.0 rather than 3.8.3.Disclosure fixes
shell: "none (Python only; no shell-out)"was false —wp_cli.pyspawnspython3 <script>subprocesses (argv list, nevershell=True) andwp.shis a bash wrapper around it.SKILL.mdtold users to runbash INSTALL.shfrom the skill directory; that installer lives in the repo and is not in the packaged payload. Reworded rather than moving the file, so the repo's own install path keeps working.requestspinned to>=2.32.3in arequirements.txtthat ships inside the skill (2.32.0 fixed CVE-2024-35195).Also:
seo_metaprinted{"error": …}and exited 0, which CI and an agent both read as success;jetengine_fields --list-allwas declared but never read; the table block example closed with<!-- /wp:heading -->.Tests
64 passing (was 37). New coverage:
same_origin's truth table, the redirect handler droppingAuthorizationcross-origin and keeping it same-origin, the probe validators and redirect re-validation, the two flipped defaults with both escape hatches and the precedence between them, and a static invariant that no script sending anAuthorizationheader callsurllib.request.urlopendirectly. Every address case uses an IP literal, so no test resolves a name over the network.Three suites previously patched
urllib.request.urlopen, which is no longer the seam those scripts call; they now patch the seam each script actually uses.On the audit's own verdict
SkillSpector's
score: 100 / CRITICAL / DO_NOT_INSTALLis not credible for this skill — it is an agentic WordPress tool doing the things it says it does. ClawScan'ssuspiciouswas, and the five findings above are what was behind it.Not in scope
jetengine_fieldshas the same exit-0-on-error shape asseo_metadid. It was not in the audit and is not touched here.Publish a GitHub Release after merge so ClawHub re-scans.
🤖 Generated with Claude Code