Repository navigation
Conversation
…put they produced An XFA button click through run_javascript and the form_set_script tool returned the document's whole waiting script output as their own. In automation nothing else takes that output, so the next click or script change returned the alerts, console lines and errors of opening and of earlier edits as its own. Several tools that apply an edit (the accessibility edits, js_set_document_script, form_set_actions, pdfa_convert, ocr_recognize, doc_compare_mark, form_detect_fields, and js_run when its script changes fields) also left their edit's output, such as the warnings for XFA data they could not write, in that output. Session::js_output_mark and take_js_output_since take only what scripts added after a mark, and leave the rest for take_js_output. The click path and form_set_script use them. The edit tools add their edit's errors to the result's warnings, and js_run also returns the alerts, console lines and requests of the field scripts that its changes ran.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Scoped JavaScript output capture crates/engine/src/js.rs, crates/engine/src/tests.rs |
The engine marks current JavaScript output and takes output added after that mark. XFA button clicks use this scoped capture. Tests check that click output excludes earlier output and that failed clicks do not repeat errors. |
Automation output and warning responses crates/automation/src/a11y.rs, crates/automation/src/a11y/documents.rs, crates/automation/src/a11y/form_tools.rs, crates/automation/src/a11y/ocr.rs, crates/automation/src/tools.rs, crates/automation/tests/it/automation.rs |
Automation operations attach newly generated errors to responses as warnings. js_run includes alerts, console entries, and requests generated during its execution. The integration tests cover output scoping and warnings from values that cannot be written to structured XFA data. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Suggested reviewers: aiongg, mtlcl410
Merge Risk: 
b4b3d
-
The reviewed output-scoping and warning changes have no remaining identified merge-blocking issue. Merge after normal checks.
Security architecture risk: 
b4b3d
-
The change improves attribution of script output to the operation that produced it. Newly returned script requests remain response data in the reviewed code; whether external clients execute those requests with separate authorization is not established.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The demonstrated exposure change is additional script-originated response data for the selected document, not demonstrated access to another document or service. External clients' handling of those newly returned request records remains outside the inspected scope.
Security Findings and Attack Paths
- observed — The changed js_run response path serializes script requests rather than executing them. The focused source comparison therefore establishes broader request visibility, but not a new in-repository request-execution attack path.
Trust Boundaries and Controls
- observed — The pre-existing execution controls remain: run_javascript rejects disabled JavaScript and read-only documents, edit application checks document permissions, and ordinary field-script output filters Save As and panel requests before queuing them. The PR changes capture and serialization rather than these controls.
Hardening Proposals
- proposed — For integrations that execute returned navigation, submission, printing, or similar requests, define them as untrusted document output requiring separate client authorization. This is an integration safeguard, not an observed missing control in this repository.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely describes the main change: XFA clicks and form edits now return only the script output they produced. Description check The description is detailed and covers the behavior change, verification commands, regression tests, safety checks, provenance, and reviewer focus. It omits the template's separate Security and Parity… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description is detailed and covers the behavior change, verification commands, regression tests, safety checks, provenance, and reviewer focus. It omits the template's separate Security and Parity sections, but the available information is sufficient and the relevant parity command is documented under verification.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 29.41% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 17 functions across 7 files. (1 skipped: 1 too large.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
FredApps
left a comment
FredApps
left a comment
There was a problem hiding this comment.
Approving. XFA clicks and form edits now return only the script output they produced: the automation tools take a mark of the document's script output before each edit and take out only what was added after it (errors become a warnings list on the result, merged into an existing one), the JS output type gets mark/split_off helpers that clamp to the current length, and the click path and js_run use the same mark so output from opening or earlier edits stays queued. I read the whole non-test diff; the gate and SonarCloud pass at 8370c8a. I read the code and the CI results; I did not run the app.
|
Merged The branch merges cleanly with
The full |
Taken through storytold#4559's merge of main at 20eebc1. The page_enhance_camera change moves to dispatch.rs, where main moved Automation::call.
main now reports each js_run alert as an object with its message, title, icon and buttons, so the XFA click test compares against that object. engine/src/tests.rs keeps main's bytes, CRLF lines included, with the XFA click test added.
|
Merged |
What this changes
An XFA button click through
run_javascript(js_runwithfield) and theform_set_scripttool returnedtake_js_output, the document's whole waiting script output, as their own. In automation nothing else takes that output, so the next click or script change returned the alerts, console lines and errors of opening and of earlier edits as its own: afterform_fillset a value that fails validation, a click on a button that shows "Hello 500" returned["Quantity must be 100 or less", "Hello 500"]. Several tools that apply an edit,js_runamong them when its script changes fields, also left their edit's output, such as the warnings for XFA data they could not write, in that output for the next click orform_set_scriptto report.This pull request makes these changes:
Session::js_output_markandSession::take_js_output_sincetake only what scripts added after a mark. What was waiting at the mark stays fortake_js_output.form_set_scriptreturn only the output of their own edit.js_set_document_script,form_set_actions,pdfa_convert,ocr_recognize,doc_compare_markandform_detect_fieldsadd the errors their edit produced, such as XFA values that could not be written to the form's data, to the result'swarnings.js_runwithout an XFA click adds the errors of applying its script's field changes towarnings, and returns the alerts, console lines and requests of the field scripts that those changes ran with the script's own. Its tool description says so.This branch and #4551 change different lines and merge without conflicts. #4551's
take_js_errorsindoc_openandapplytakes the errors that were waiting, and the tools here no longer leave their own behind for it. On the earlier head8370c8a, with #4551 and #4166 merged in,cargo test -p pdfcraft-engine -p pdfcraft-automationpassed, including #4166'sform_fill_cannot_print_but_js_run_can, which reads the session's output afterform_fill.How you verified it
cargo fmt --check-D warningson the crates I changedcargo xtask layersandcargo xtask assets(if you touched crate boundaries or any asset)Tests. Each one fails on
main:an_xfa_click_returns_only_its_own_script_output(engine): a click after an open whose initialize script shows "opened" and throws returns only "Hello 2" and no error; the open's output stays. Onmainthe alerts are["opened", "Hello 2"].an_xfa_click_through_js_run_does_not_return_an_earlier_fills_alerts(automation): afterform_fillqty 500, a click returns["Hello 500"]only.form_set_script_does_not_return_an_earlier_fills_console_lines(automation): onmainthe result holds["validated 7"]from the earlier fill.edits_report_the_xfa_data_they_could_not_write_once(automation):js_set_document_script,form_set_actionsand ajs_runthat sets a field each report the structured-content warning once inwarnings, andform_set_scriptlists it once inerrors. Onmainwarningsis missing.js_run_returns_the_output_of_the_calculations_its_changes_ran(automation): ajs_runthat setsareturns its own console line and the one thatb's calculate script printed. Onmainonly its own line is returned.On Windows 11 (x86-64) with rustc 1.99.1, on
b0a18a1, which hasmainat20eebc1merged in:cargo fmt --all -- --checkcargo clippy -p pdfcraft-engine -p pdfcraft-automation --all-targets -- -D warningscargo test -p pdfcraft-engine -p pdfcraft-automation -p pdfcraft-ui-egui -p pdfcraft -p pdfcraft-cli: 11 test binaries, 2,622 passed, 15 ignored, and 1 failed:control::screenshots_of_window_and_region(pdfcraft-ui-egui) timed out waiting for the window to draw, and passed when run again on its owncargo xtask layers,assetsandparityThe full
cargo xtask ciresult is added once this pull request's gate check passes.cargo xtask cipassed its 10 steps on the earlier head8370c8aon top ofmainata7801dc(4,953 tests passed, 0 failed, 17 ignored).Merge with
mainatdc943eeb(4b982964, test follow-upb4b3d4ec) checked withcargo fmt --check, clippy onpdfcraft-engineandpdfcraft-automation, and the five XFA script-output tests (5 passed).The branch merges cleanly with
mainatdc943eeb.Never-crash
unwrap/expect/panic!/unreachable!/todo!outside tests, unless it isprovably infallible with a comment saying why
get, and strings only at charboundaries); checked or saturating arithmetic; allocations sized by input are capped;
recursion is bounded
unsafeThis change fixes no crash.
Provenance
This is the part that cannot be undone after a merge, and the part a new contributor is
least likely to know about. Please read
AGENTS.md§1 before adding any asset.storytold/craft-fonts)
run as external oracle processes
another implementation's source
ATTRIBUTION.tomlentry andcargo xtask assetspassesAnything a reviewer should look at first
The direct edit tools now return
warningswhen an XFA form's data could not be written. On such a form, the click's own edit writes the data too, so a click still reports that warning as itserror.Summary by CodeRabbit