fix: exit non-zero on an error result in acf_fields and jetengine_fields (3.9.1) - #18
Conversation
…r (3.9.1)
Both print {"error": ...} and returned 0 on a failed write, so CI and an agent
both read a failure as success. 3.9.0 fixed exactly this in seo_meta and I
flagged the other two as out of scope rather than fixing them; they are the
same bug and the scope argument does not survive a second look.
The check is now one shared security.exit_on_error_result rather than three
copies, with a static test that fails if a script returning an error dict does
not call it - verified to fail with the call removed, so it is not vacuous.
Also recorded in the changelog: requests 2.32.5 was verified empirically to
strip Authorization on a cross-host redirect and keep it on a same-origin one,
the same semantics as 3.9.0's urllib opener. 3.9.0 asserted that from library
documentation without being able to run it; it is now proven, and the four
scripts that authenticate through requests need no equivalent fix.
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. |
![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
Here are some automated review suggestions for this pull request.
Reviewed commit: 92755623b0
ℹ️ 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".
| if isinstance(result, dict) and result.get("error"): | ||
| sys.exit(1) |
There was a problem hiding this comment.
When an ACF or JetEngine read succeeds and the returned custom fields include a truthy field named error (including an explicit --field error lookup), the result is ordinary user data but this generic check exits with status 1. Both getters return their field dictionaries directly, so key presence cannot reliably identify a helper failure; use a distinguishable error result or otherwise keep successful field data from reaching this predicate.
Useful? React with 👍 / 👎.
Sniffing for an "error" key cannot identify a failure in these scripts: the
ACF and JetEngine getters return the SITE's own field dictionary, so a custom
field named "error" - or an explicit "--field error" lookup - is ordinary data
that my check would have turned into exit 1 on a perfectly successful read.
Failure is now a type. security.ErrorResult is a dict subclass built by
error_result(), so the JSON output is byte-identical and callers can still
index it, but exit_on_error_result tests isinstance rather than guessing from
content. All thirteen bare {"error": ...} returns across acf_fields,
jetengine_fields and seo_meta go through it.
Two static tests hold the line: no script may build a bare {"error": ...}
return, and any script that builds a failure must check it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed in The ACF and JetEngine getters return the site's own field dictionary. A WordPress custom field named Failure is now carried by a type. Demonstrated: Two static tests hold the line: no script may build a bare 87 tests passing. @codex review |
|
Codex Review: Didn't find any major issues. Keep it up! 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
Two follow-ups from #17, both of which I had left open there.
1. The exit-code bug was in three scripts, not one
acf_fields,jetengine_fieldsandseo_metaall report a failed write as{"error": ...}rather than by raising, so the CLI printed the error and still exited 0 — which CI and an agent both read as success. #17 fixedseo_metaand I listed the other two under "not in scope" because they were not in the audit.That was the wrong call. It is the same bug, in scripts that sit beside the one I fixed, and "not in the audit" is not a reason to ship a known failure. Both are fixed here.
The check is now one shared
security.exit_on_error_resultinstead of three copies, with a static test that fails if a script returning an error dict does not call it. I verified that test fails with the call removed, so it is not vacuous.Nothing changes for a successful call, and the JSON still goes to stdout — only the exit code differs.
2. The
requestsclaim is now proven rather than asserted#17 narrowed the credential-leak finding from the audit's 13 scripts to 9, on the grounds that
requestsstripsAuthorizationacross hosts by itself. I could not run that locally then —requestswas not installed — and said so rather than claiming it.Now measured, against
requests2.32.5 in a throwaway venv, with two local servers:Same semantics as the urllib opener #17 added, in both directions. The four
requests-based scripts need no equivalent fix, and the narrowing in #17 was correct.One detail worth recording: my first same-origin control was mis-designed — it used two servers, so the redirect crossed a port, and
requestsstrips there too (itsshould_strip_authtreats a port change as origin-crossing, exactly assame_origindoes). The control above redirects within one server.Tests: 80 → 85.
🤖 Generated with Claude Code