Skip to content

Adopting generated documents as controlled registers: validation, template override, finding-list filtering #1820

Description

@AnotherDork

Context

I self-host Probo and want its object-backed registers (findings, risks) to be my ISO 27001 / SOC 2 controlled registers, published from the objects rather than kept as a parallel hand-maintained table.

The generated-document path is the right vehicle, and one property makes it better than the authored path: an authored document is capped at 200,000 characters of text by request validation in pkg/probo/document_service.go. My corrective-action register is at 190,230 and grows a row at a time. The generated path is not subject to that cap — I have published a 205,990-character register through publishFindingList successfully.

Three things block adoption. They are independent: take, reshape, or decline each on its own.

I am happy to open the PRs for any or all of them. Line references are pinned to 62966cdc57df42a12c9a6e3a42ec927938f55f79.


1. Generated documents are inserted without validating their ProseMirror content

pkg/probo/generated_document_service.go contains zero validator. calls. All 13 generated documents route through one choke point, publishOrRequestApproval (:4008), and content reaches version.Insert(...) exactly as the template produced it. 10 of the 13 files in pkg/probo/templates/ are .json.tmpl, emitting ProseMirror JSON straight from template text with nothing parsing it.

The authored path sanitizes on both write paths, at document_service.go:702 and :968.

Malformed content is therefore stored, then fails soft at render: ProseMirrorJSONToHTML (pkg/docgen/generator.go:599) falls back to <p>{escaped raw JSON}</p> when Parse fails. A broken register surfaces as escaped JSON in the exported PDF and errors nowhere an operator would look. For a compliance register that PDF is the audit artifact.

Not exploitable today: every interpolation in the JSON templates goes through the json escaping func, except {{.NotesBlocks}} in third_party_list.json.tmpl:214, which is pre-rendered ProseMirror produced in-process rather than user text. This is hardening, hence a normal issue rather than a report to security@probo.com. It becomes load-bearing if template content is ever not fully controlled by the binary, i.e. if section 2 lands.

Proposal. Call prosemirror.SanitizeDocumentJSON (pkg/prosemirror/sanitize.go:58) in publishOrRequestApproval immediately before version.Insert(...), covering all 13 publishers at one point. SanitizeDocumentJSON rather than ValidateDocumentContentJSON: it calls the same parseDocRoot, so it is the validation, plus it rewrites unsafe href/src, and it is the helper the authored path already uses.

Two non-goals:

  • Not applying validator.ProseMirrorDocumentMaxTextLength. That 200,000-character cap is authored-path request validation; a generated register of mine is 205,990 characters and publishes fine today, so applying it here would be a breaking regression. The proposed check is structural, not length-based.
  • Sanitizing cannot reject anything currently valid. prosemirror.Node.Attrs is json.RawMessage (pkg/prosemirror/node.go:42), so the round-trip is lossless for table colwidth and heading level, and Parse has no node-type whitelist.

About 40 lines, plus a DB-free table test over all 13 Build*Document functions asserting the output parses.


2. Deployments cannot override the generated register templates

A team self-hosting Probo for an ISO 27001 or SOC 2 audit needs the finding and risk registers to be their controlled registers — their columns, their section prose, the sections their own management system mandates (lifecycle, severity bands, verification and closure requirements). The templates are embedded in the binary, so the only route is a fork and a permanent patch queue against main.

finding_list.json.tmpl has been touched once in its entire history. generated_document_service.go took 37 commits in the last six months. Forking to change the file that never moves means rebasing the file that moves constantly.

Existing patterns this would follow:

  • The templates are already parameterised by stable docgen.*ListData structs (pkg/docgen/generator.go:243 onward), which is the de facto contract an override would write against.
  • pkg/brand/http.go:32 NewAssets() — embedded artifact, overridable, validated at startup, fail fast.
  • proboctl vet --procedure-file — overriding an embedded prompt from disk.
  • The Helm chart has generic volumes: [] / volumeMounts: [] passthroughs (values.yaml:129,136), so a ConfigMap is the natural delivery with no new chart concepts.

Sketch. One GeneratedDocumentTemplates value in pkg/probo/templates.go holding map[string]*template.Template, parsed eagerly at construction from an optional fs.FS override and never mutated afterwards — no mutex, no sync.Once. Valid names derive from fs.Glob(Templates, "templates/*.tmpl"), so a 14th template needs no list update and an override filename that is not a built-in is an error rather than silently ignored. Config is a single PROBOD_GENERATED_DOCUMENTS_TEMPLATE_DIR / generated-documents.template-dir, read in probod.Run() before probo.NewService, so a malformed override means probod refuses to start rather than failing mid-publish. Zero value means "use built-ins".

Would you accept this? It is the largest of the three diffs and the only one that can be declined on principle, so I would rather hear yes or no before writing it.

If yes, two sub-decisions I would flag in the PR: a plain string path rather than the TextUnmarshaler convention (filesystem checks do not belong inside json.Unmarshal), and a probo.Option functional option rather than a 17th positional parameter on NewService.

I would not enable an override in the e2e harness. Doing so applies it suite-wide and breaks content-substring assertions across the existing publish suites (11 of the 20 *publish*_test.go files assert on rendered text, e.g. finding_publish_test.go:117 assert.Contains(t, ver.Content, "Purpose")), which are the backward compatibility being defended.

Per-organization templates in the database: not proposed here. The seam would be left open, since every publisher becomes s.templates.render(...) and a per-org variant would later be s.templatesFor(ctx, orgID).render(...) without touching those lines again.


3. publishFindingList ignores status and always orders by reference ID

buildFindingListDocumentData (:1005) hardcodes both the ordering and the absence of a filter:

page.OrderBy[coredata.FindingOrderField]{
    Field:     coredata.FindingOrderFieldReferenceId,
    Direction: page.OrderDirectionAsc,
}
...
coredata.NewFindingFilter(nil, nil, nil, nil)

Two consequences:

  1. Every finding is published, closed ones included. A corrective-action register listing closed items alongside open ones is not the document an auditor asks for, and there is no way to publish an open-items view.
  2. Order is fixed to reference_id ascending, which is Probo's identifier rather than the organisation's. My register numbering and Probo's reference IDs diverge across ten offset blocks (-63 to +9), so the published order reads as arbitrary against my own numbering. There is no way to publish by status, priority, or due date.

Proposal. Optional orderBy and filter arguments on PublishFindingList, with nil falling back to exactly today's values, so nothing existing changes. Two things make it small:

  • buildThirdPartyListDocumentData (:3258) already passes a real filter to its loader, so per-publisher filtering on the generated path is established.
  • FindingOrder and FindingFilter already exist at audit.graphql:143 and :148, used by the findings connection, and the resolver mapping already exists verbatim elsewhere in the same file. No new types.

Scope. The four-interface rule in contrib/claude/api-surface.md makes the difference large:

Scope Cost
Findings only ~9 files, reusing types that already exist
All 12 generated list publishers 100+ file touches, ~1,500 lines of MCP YAML, 11 new filter/order plumbings

Each entity has its own filter struct and order enum, so a generic version cannot be expressed in GraphQL without going stringly-typed. I have a concrete need for findings and know of none for the other 11, so my proposal is findings only, leaving the pattern for whoever needs the second one.

No console UI change: every field is optional, and the use case is CLI/MCP-driven.

Two limitations:

  • A filtered register does not record on its face that it was filtered, which an auditor could object to. The default stays unfiltered and filtering is opt-in, so this affects only deployments that choose it. A Filters provenance field on docgen.FindingListData would be the follow-up.
  • I am not proposing changing the default to exclude closed findings. That would silently change what every existing deployment publishes.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions