[batonac]

Summary

Brings esign up to the app standards quality gates: registry semgrep clean with an empty baseline, ty clean with no ignores, strict TypeScript for the compiled bundles and JSDoc-checked desk JS, and tests from zero (90 tests, coverage 97.9%, floor 0 → 80) with a complete testmap and passing composition check. The v15 override path is gone.

Writing the tests turned up four access-control holes in the signing endpoints, and a few plain bugs. They are fixed here in separate fix: commits, each called out below. Please read the Review guide before merging: these are deliberate behaviour changes on a security-sensitive app.

What changed

One commit per concern:

• chore: ignore the app standards formatting commit in blame: adds style: adopt the app standards (managed config and formatting) #7's squash 0f2bacb (a squash, one parent) to .git-blame-ignore-revs with frappe-nix compat --add-blame-ignore.

• refactor: drop the Frappe v15 controller override path: hooks.py always registers extend_doctype_class and no longer imports frappe. EsignWebForm subclasses the stock WebForm only, and the PaymentWebForm branch is gone.

• fix: check access before rendering, updating or sharing a document:

  • get_print_html set frappe.flags.ignore_print_permissions before validate_esign_key. Without a key, the fallback print-permission check was then skipped, so any visitor could fetch the print HTML of any document of any doctype. Validation now runs first.
  • accept updated an existing document with ignore_permissions whenever no key was sent. Updates now need the share key, web form write permission (has_web_form_permission: owner, website permission, apply_document_permissions) or DocType write permission (frappe.has_permission). Those are the three cases in which frappe's own accept saves; it saves the last one with permission checks on.
  • send_esign_email and get_esign_link (when given a document reference from a request) mint a share key, so they now need the document's email permission. That is the same check frappe's email dialog makes. Server-side callers (Jinja, Python) pass a Document and are unchanged.

• fix: clear the registry's blocking semgrep findings:

  • frappe-manual-commit ×2:
    • attach_print_to_document committed in the middle of the job. It now attaches the PDF and queues a second job, record_signature, with enqueue_after_commit, so the Communication is still written after the File has been committed.
    • download_signed_pdf now sets frappe.local.flags.commit instead of committing itself. That code path also had a bug: it redirected to the raw UUID while storing uuid.int >> 64, so a file's first download always missed. It now stores and redirects with the same key, in cloud_storage's own format.
  • frappe-ssti ×2: the notification subject and message come from a System Manager-authored Notification, which frappe's own Notification renders the same way. Each call carries # nosemgrep with that reason.
  • Also in this commit:
    • The notification context is now built as Notification.send builds it. Before, get_context(doc) was computed and then overwritten.
    • The audit-trail values appended to the notification email are HTML-escaped. The user agent and the email parameter come from the signer's request. (The timeline card had the same problem; see escape the signer's audit values below.)
    • The PDF copied to the Communication is read as raw bytes. File.get_content() decodes binary PDFs as text and would corrupt them.

• fix: make ty clean:

  • WebFormField.fieldname and Notification.set_property_after_alert are read through get(). frappe types both as Literal[None], so ty treated whole branches as unreachable.
  • esign_update_field is typed DF.Data.
  • The request snapshot is built key by key.
  • The upload check "data:" and "base64" in value (where "data:" was always true) now tests both substrings.
  • Three type: ignore comments are gone; no ty: ignore remains.

• fix: validate the share key before marking a document permission-free: get_context, get_print_html and download_signed_pdf set doc.flags.ignore_permissions before validating. frappe's website permission check (part of the no-key fallback) passes any document that carries that flag. So without a key, a Guest could open the signing page of any document of an eSign-enabled doctype, and a logged-in user could download any signed PDF. The flag is now set only after validation.

• fix: build working eSign links from a document reference:

  • get_esign_link was annotated doc: Document, so frappe v16's argument validation rejected the JSON the desk sends, and the link preview in "Send for eSign" never loaded.
  • The link also joined get_url() and the route without a slash (http://host:8000esign-form/…). It now always has exactly one.

• Review fixes (fix: commits on top):

  • fix: let owners and users with write permission through the signing page and accept: the accept guard above now also admits DocType write permission, so a desk user who may write the document but is not its owner can sign without ?key=, as before this PR. The signing page without a key also admits whoever the web form's own rules admit (owner, website permission, document permissions), so a portal owner can open their own record from the web form list, as accept already allowed. New tests: a non-owner System Manager and an owning Website User updating without a key, the owner opening the page, a user with no permission refused, and download_signed_pdf refusing a logged-in user without print permission (that last fix had no regression test before) while a user with print permission gets the file.
  • fix: escape the signer's audit values on the timeline card: the desk timeline template prints its data with {{ }}, which is raw output in frappe's microtemplates. A Guest with a share key could sign with a crafted User-Agent or email parameter and store markup that ran for every desk user opening the timeline (this predates the PR). get_timeline_content now HTML-escapes every value it hands the card, and the email parameter is recorded only when it is a single valid address (otherwise the form's email field or the logged-in user is used, as when the parameter is absent).
  • fix: name the bundle entries .js so prebuilt assets resolve them: see the asset names item under the TypeScript commit.
  • fix: apply the keyless web form rule to the preview and signed-PDF download (review round 2): the page admitted a keyless owner, but get_print_html and download_signed_pdf still required print permission, so a Website User owner got the page with "Failed to load document preview." and a refused signed-PDF link. One helper, validate_esign_access, now decides for all three: without a key, a logged-in user whom the named eSign web form (same doctype) admits by its own rules is let in; everyone else gets the share-key / print-permission check as before, and Guests still need a key. Both endpoints take an optional web_form argument, which the page passes (frappe.esign_context.web_form and the signed-PDF URL). New tests call both endpoints as the keyless owner, a non-owner, with a missing or non-eSign web form, and as Guest.

• test: cover the signing flow, its key checks and the desk hooks: 78 tests on a test-created submittable doctype and an eSign web form. They use requests built the way frappe builds them (set_request, get_response_content). Coverage:

  • the signing page;
  • every key path, including valid, wrong and missing keys and the key taken from the referrer;
  • accept, including the anonymous form's switch to Guest and back, also when saving fails;
  • both background jobs, including the cloud storage branch and the notification;
  • send_esign_email, can_esign and its hooks, the timeline cards, the Email Template extension, and the Jinja and install/migrate/uninstall hooks.

  fail_under goes from 0 to 80, the profile target the ratchet asks for.

• refactor: type the bundles in strict TypeScript and build the fonts as a bundle:

  • The desk, control and web bundles and the controls (about 2.4k lines) are now .ts, checked by the managed tsconfig.browser.json with the frappe-types strict preset. web_form.js (the Web Form doctype_js) stays JS with JSDoc, checked by tsconfig.desk.json against types/doctypes.d.ts (frappe-types gen-registry).
  • controls/base.ts types ControlData as the ControlInput subclass it is at runtime. Instance fields set while frappe's control constructor runs are declared, so the compiled classes never reset them. I checked the compiled output.
  • types/esign.augment.d.ts holds the app-owned members (// app-owned:). types/frappe-modules.d.ts declares frappe's web form modules, now imported as frappe/public/js/… (esbuild resolves these from the apps directory) instead of ../../../../../frappe/….
  • Asset names stay esign.*.bundle.js. The esbuild entries are small .js files (esign.desk.bundle.js, esign.control.bundle.js, web/esign.web.bundle.js) that import the TypeScript modules (esign_desk.ts, esign_controls.ts, web/esign_web.ts). bench build keys assets.json by the entry's source name, while bench build --using-cached keys it by the built file's name with the hash stripped, which is always .js; only a .js entry gets the same key from both. (The first revision named the entries .ts, which 404s on benches that install prebuilt assets.)
  • Fonts: esign/public/css/esign-fonts.entry.css → Vite → dist/css/esign-fonts.bundle.<hash>.css with a manifest. scripts/vite-register.mjs registers it as esign-fonts.bundle.css, which replaces the unhashed /assets/esign/dist/esign-fonts.css path, so a release now busts the cache.
  • @types/hast is declared.

Verification

Everything below ran inside the nix dev shell at the head of this branch unless noted:

• frappe-test --ci --recreate-site at the head → 90 tests OK, coverage 97.9%, fail_under 80; ty and nix-lint stages pass. Testmap: "15 target(s), every one tested or exempt (0 exempt)". Composition: "2 controller(s) compose in both orders". Exit 0.
• frappe-nix sync --check → clean. frappe-nix compat → exit 0.
• yarn -s typecheck:py (ty, error-on-warning) → All checks passed!. The app has no ty: ignore or type: ignore comments.
• yarn run check (oxfmt, oxlint, stylelint, tsc --build over the browser and desk projects) → exit 0, with no oxlint warnings.
• frappe-listing check → every check passes, including L7 registry semgrep with an empty baseline (18 advisory notes remain: missing type hints on whitelisted args, and guest-whitelisted endpoints flagged for review) and all pilot L9 checks.
  • Workaround: ImportCheck needs the MariaDB client headers, which the dev shell lacks, so I ran it with MYSQLCLIENT_CFLAGS/MYSQLCLIENT_LDFLAGS from nixpkgs' mariadb-connector-c. A frappe-nix fix is in flight.
• bench build --app esign → esign.{desk,control,web}.bundle.js registered under their .js keys, pointing at dist/js/esign.*.bundle.<hash>.js (the names --using-cached keys the same way); vite-register: esign: esign-fonts.bundle.css -> /assets/esign/dist/css/esign-fonts.bundle.<hash>.css.
• prek:
  • On a fresh clone of head 623a73a, outside the dev shell (uv run --python 3.14 --frozen --project tools prek run --all-files, yarn from nixpkgs): every hook passes, ssort and static_analysis included, exit 0. (At 5c0da14 the clean-clone ssort wanted _get_esign_web_form above its caller; 623a73a applies exactly that order, which ssort also accepts inside the dev shell.)
  • Inside the dev shell at 623a73a: passes with SKIP=static_analysis. That hook scans the in-repo bench there; a frappe-nix fix is in flight.
• Mutation check of the review fixes: with the round-1 source changes reverted (and download_signed_pdf's old flag order put back), the new tests fail: test_user_with_doctype_write_permission_updates_without_key, test_owner_without_key_gets_the_signing_page, test_user_without_key_needs_print_permission, test_signer_email_resolution and test_signer_controlled_values_are_escaped. With the fixes, all pass.
• git grep and the commit-message check for the internal codename print nothing.
• CI: this repo has only GitGuardian (and the docs check, which doesn't trigger here) until the reusable CI lands.

Review guide

1. Start with the three fix: commits that change access (check access…, validate the share key…, and the accept guard in the first one). Things that worked before and are now refused, on purpose:

   • a visitor with no key, or a wrong key, opening /<route>/<name>/edit or calling get_print_html / download_signed_pdf;
   • updating a document through accept without its key, unless the user has web form write permission (owner, website permission or apply_document_permissions) or DocType write permission;
   • opening the signing page without a key, unless the user has read/print permission or the web form's own rules admit them (owner, website permission, apply_document_permissions);
   • a user without email permission on the document sending an eSign request or fetching a link for it.

   Signing with the emailed link, desk users with write (for accept) or read/print (for the page) permission, and owners are unaffected: an owner without a key gets the page, its preview and the signed-PDF download (through the page's web_form argument; calling the endpoints without it still needs print permission). One exception for keyless owners with no print permission: the page's "Download PDF" button (before signing) uses frappe's own download_pdf, which this app does not control; it refused them on develop too. The timeline card now shows escaped text, so any markup a signer put in the user agent or email shows as text. Before deploying, check that nobody relies on opening eSign pages without the ?key= link.

2. The background job split (attach_print_to_document → record_signature). The timeline entry and notification now arrive in a second short job, after the first job's commit. If the worker died between the two, the PDF would be attached without its Communication. The old code committed at the same point, so this window existed before too.

3. Asset names. The JS bundles keep their esign.*.bundle.js names; the fonts are now looked up as esign-fonts.bundle.css. A bench must run bench build (which runs the app's yarn build and vite-register) after deploying, or the fonts don't load.

4. Manual check worth doing, which I did not do in a browser:

   • "Send for eSign" from a document: the link preview should now render.
   • Open the emailed link as a Guest, sign in each mode (draw, type, upload), and confirm the PDF is attached and the timeline card appears.
   • On the desk, confirm the signature fonts load (the font CSS is now hashed).

5. Smaller deliberate changes:

   • The upload control's button reads "Change" after a file is picked.
   • The eSign page's console.debug lines are gone.
   • web_form.js calls its helpers directly instead of through frm.events.
   • Signature fonts are now emitted with hashed file names.

Not done / follow-ups

• Rename to esign_webforms: separate PR, as planned.
• The web_form.py refactor (its own accept copy, the Guest session switch, and the request mock for jobs): its own reviewed PR. The tests here pin the current behaviour, so that PR can be checked against them.
• Left alone, worth a decision:
  • get_esign_email_templates uses frappe.get_all, so any logged-in user can list eSign email templates. Nothing in the app calls it.
  • An expired key raises LinkExpired (a 500) from has_web_form_permission instead of a 403.
  • accept and get_print_html set frappe.local.message_log = None, which breaks any later msgprint in the same request.
  • An Attach field submitted through esign's accept leaves two File rows (esign's own, plus the one frappe v16 records for attach fields on save).
• Registry advisories: whitelisted arguments without type hints (send_esign_email, accept, get_esign_web_forms). Adding hints switches on frappe's argument validation, so it needs a reviewed change; get_esign_link shows what happens when a hint is wrong.
• Central, not app issues:
  • the managed static_analysis hook scans the dev bench inside the shell;
  • the dev shell lacks the MariaDB client for ImportCheck;
  • the planned typecheck job calls frappe-types gen-doctypes, while frappe-types 16.5.0 ships the generator as gen-registry.

🤖 Generated with Claude Code

https://claude.ai/code/session_01SvdEeMiewirZ3ifFoUix5F