feat(ogar-render-askama): T3 — HtmlDetailView (Redmine show.html.erb on ClassView)#84
Conversation
![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: bafb27e03c
ℹ️ 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".
| // ── Spine binding struct ───────────────────────────────────────────── | ||
|
|
||
| #[derive(Template)] | ||
| #[template(path = "dispatch/html_list_view.askama", escape = "none")] |
There was a problem hiding this comment.
Because this is an HTML view but the spine is compiled with escape = "none", every interpolation in html_list_view.askama is raw unless manually filtered. In the normal row-rendering path, values such as the saved/list title, column captions, and especially grouped row labels can come from data or user-visible configuration; a label like <script>…</script> will be emitted directly. Keep the template on HTML escaping and mark only the pre-rendered cell.body_html as safe.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2b3fb88. Both spines now compile with escape = "html"; only the intentionally-pre-rendered fields (headline_html, cell.body_html, section.body_html) bypass via |safe. Two XSS regression tests added — list + detail variants — pinning that every data-derived slot (title, subtitle, labels, captions, css_classes, group headers) escapes to <...> while the intentional HTML (<a>...</a> headline, rich-text section body) survives. escape = "none" retained only on rust_struct.askama (emits Rust source, not HTML) and cell/rich_text.askama (trusted pre-rendered prose). 25/25 tests green; thanks for the catch.
| // ── Spine binding struct ───────────────────────────────────────────── | ||
|
|
||
| #[derive(Template)] | ||
| #[template(path = "dispatch/html_detail_view.askama", escape = "none")] |
There was a problem hiding this comment.
This detail view also emits browser-facing HTML while disabling Askama escaping globally, so ordinary fields like subtitle, section labels, and inline field labels are rendered raw in html_detail_view.askama. When those values are derived from record data or late-resolved labels, malicious markup is injected into the page; only the intentionally pre-rendered headline_html/cell bodies should be marked safe while the rest stays HTML-escaped.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2b3fb88. Both spines now compile with escape = "html"; only the intentionally-pre-rendered fields (headline_html, cell.body_html, section.body_html) bypass via |safe. Two XSS regression tests added — list + detail variants — pinning that every data-derived slot (title, subtitle, labels, captions, css_classes, group headers) escapes to <...> while the intentional HTML (<a>...</a> headline, rich-text section body) survives. escape = "none" retained only on rust_struct.askama (emits Rust source, not HTML) and cell/rich_text.askama (trusted pre-rendered prose). 25/25 tests green; thanks for the catch.
…on ClassView) Third real emitter per Northstar plan §3, the detail-page sibling of T2's HtmlListView. Mirrors Redmine's `app/views/issues/show.html.erb` shape: definition-list (dt/dd) for inline typed fields + full-width <section> blocks for prose / family-edge collections. Reuses every piece T2 built — RenderColumn, ColumnKind, the 10 cell sub- templates, the two-stage cells-pre-rendered-in-Rust pattern. T3 is just a different spine template; the substrate-agnostic property holds. Surface: - ArtifactKind::HtmlDetailView variant (slot 2, append-only). - templates/dispatch/html_detail_view.askama — spine (<article> + header + <dl class="detail-fields"> + per-block <section>s). - artifact_kinds::html_detail_view::render_detail(class_id, concept, record_id, headline_html, subtitle, columns, cells) — the real entry point. - HtmlDetailViewEmitter — codebook-only proof-of-shape for the for_kind dispatch path. Arity contract: `columns.len() == cells.len()`; the emitter returns an askama::Error on mismatch (caller-side bug, pinned by test). Tests (+3 T3 specific, 23/23 total): - html_detail_view_proof_of_shape_renders_dl_with_class_meta — codebook emit on project_work_item; surfaces data-class-id + data-concept + detail-field-<name> per attribute. - html_detail_view_renders_inline_dl_and_block_sections — substantive render with PrimaryLink + ProgressBar inline + RichText block; pins record-id, headline, subtitle, inline cells via per-kind sub-templates, block <section> for description, and a NEGATIVE assert that the block didn't leak into the inline <dl>. - html_detail_view_column_cell_arity_mismatch_returns_error — caller- side contract. Workspace check + test green. Cell-renderer dispatch duplicated between list_view and detail_view emitters today; once T4 (HtmlForm) lands we factor it into one helper. Both pipelines feed the same per-kind sub-templates from artifact_kinds::cells.
…templates Codex P1 on PR #83 (list spine) + PR #84 (detail spine), same root issue: both spine templates were compiled with `escape = "none"`, so EVERY interpolation was raw — a malicious group-header label, title, caption, subtitle, or css_class would inject raw HTML into the page (XSS hazard). Fix: `escape = "html"` on the spine binding-struct attribute. The templates already use `|safe` correctly on the intentionally-pre-rendered HTML fields (`cell.body_html`, `headline_html`, `section.body_html`), so only those bypass escaping. Every other interpolation (title, captions, labels, css_classes, group header labels, subtitle) now gets HTML-escaped by askama. Two-layer escaping for cell content is consistent: - The cell sub-template (e.g. PrimaryLinkCell) escapes its variables (label, href, …) under its own `escape = "html"`. - The spine receives the already-escaped HTML as `body_html` and passes it through with `|safe` (no double-escape). `escape = "none"` retained only on: - `rust_struct.askama` — emits Rust source where `<` etc. must NOT be HTML-escaped. - `cell/rich_text.askama` — wraps trusted pre-rendered prose (Markdown/Textile already expanded upstream). Tests (+2 XSS regressions, 25/25 total): - html_list_view_escapes_data_derived_strings_xss_regression — pokes every untrusted slot (title, caption, group label, css_classes) with XSS payloads (`<script>`, `<img src=x onerror=…>`) + a `<malicious-class>` tag; asserts none survive raw, all escape to `<...>`. Also pins that the PrimaryLink cell's label `<safe>label</safe>` becomes `<safe>label</safe>` (per-cell escape) and that the intentional cell HTML (`<a href="/i/1">`) DOES survive (cell body |safe). - html_detail_view_escapes_data_derived_strings_xss_regression — parallel set: subtitle, inline + block labels, cell + section css. Pins that headline_html (`<a>...</a>`) and rich-text section body (`<p>...</p>`) pass through (marked safe), while every data-derived string is escaped. Workspace check + test green. T3 (HtmlDetailView from #84) folded into this commit since the cherry-picked T2 commit was dropped on rebase against the merged #83 main; this PR delivers both the T3 feature AND the codex P1 fix for the now-merged T2 spine.
AdaWorldAPI
force-pushed
the
claude/ogar-render-askama-t3-html-detail-view
branch
from
bafb27e to
2b3fb88
Compare
2 participants
What
T3 per Northstar plan §3 — the detail-page sibling of T2 (#83). Mirrors Redmine's
app/views/issues/show.html.erbshape:<dl>for inline typed fields + full-width<section>blocks for prose / family-edge collections.Stacks on top of #83. T3 reuses every piece T2 built —
RenderColumn,ColumnKind, the 10 cell sub-templates, the two-stage cells-pre-rendered-in-Rust pattern. T3 is just a different spine template; the substrate-agnostic property holds.Surface
ArtifactKind::HtmlDetailViewdispatch/html_detail_view.askama<article>+ header +<dl>+ per-block<section>s)artifact_kinds::html_detail_view::render_detail(...)HtmlDetailViewEmitterfor_kinddispatchContract
columns.len() == cells.len()— paired by index. The emitter returnsaskama::Erroron mismatch (caller-side bug, pinned byhtml_detail_view_column_cell_arity_mismatch_returns_error).Block columns (
inline=false) get full-width<section>s below the<dl>; inline columns stay in the<dl>. The render-time split is bycolumn.inline.Tests (23/23, +3 T3-specific)
html_detail_view_proof_of_shape_renders_dl_with_class_meta— codebook emit onproject_work_item; pinsdata-class-id/data-concept/detail-field-<name>per attribute.html_detail_view_renders_inline_dl_and_block_sections— substantive render withPrimaryLink+ProgressBarinline +RichTextblock. Pins record id, headline, subtitle, per-kind cell sub-templates, block<section>for description, and a negative assert that block content didn't leak into the inline<dl>.html_detail_view_column_cell_arity_mismatch_returns_error— caller-side contract.Workspace check + workspace test green.
What this collapses (Redmine show.html.erb · 179 LOC vs ours)
show.html.erbper controller (~26 of them, mostly 100–200 LOC each)<dl>+ sectionsRenderColumnplumbing as the list view_attributes.html.erbpartialNotes
Cell-renderer dispatch is currently duplicated between
html_list_view::render_cellandhtml_detail_view::render_cell_body. Once T4 (HtmlForm) lands, that becomes the third call site and warrants factoring into one helper inartifact_kinds::cells. Not factoring prematurely — three points form a line.Follow-on
T4 (
HtmlForm) — same plumbing, different artifact (create/edit form with typed<input>per attribute keyed offColumnKind). The factoring of cell-dispatch goes with T4.