posit-dev / posit-dev/quarto-yaml

FileId/span integrity: findings from the q2 source-span audit (parse_with_parent contract, FileId(0) aliasing, validation-crate context mismatches)

Open
#17 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Rust
Stars
2
Forks
1
Avg merge
2m
Merged PRs (30d)
1

Description

During a systematic audit of FileId/span integrity in Quarto 2 (triggered by a wrong-file diagnostic-span bug, quarto-dev/q2#478), we traced several weaknesses to APIs in this workspace. Recording them here for visibility since the audit itself lives in q2's (non-public) issue tracker. We are addressing these as part of a planned redesign of the quarto-source-map binding API (making the (FileId, offsets, content) pairing unrepresentable-when-wrong); the items below will mostly ride that work rather than being patched piecemeal. Filing so readers of this repo know the problems are known and owned.

quarto-yaml

Y1 — parse_with_parent's contract is unchecked, and the doc example violates it.
The parent SourceInfo must describe exactly the content being parsed (origin at byte 0 of content, length = content.len()), but nothing validates this: make_source_info stores offsets verbatim into SourceInfo::substring, and resolve_byte_range composes parent_start + offset with no clamp to the parent's end. A misaligned parent yields plausible in-file offsets at the wrong location. The doc example on parse_with_parent (parser.rs, the rust,no_run block) narrates "extracted from parent document at offset 10–50" while passing a parent spanning 0..1000 — every resulting span would be off by 10. Suggested minimal fix ahead of the redesign: debug_assert!(parent.length() == content.len()) in parse_impl + correct the example.

Y2 — the FileId(0) dummy aliases real files.
parse() (no filename, no parent) mints Original { FileId(0), … } for every node (make_source_info, make_source_info_at_offset, make_tag_source_info). Because quarto_source_map::SourceContext::get_file falls back to positional indexing for unmapped ids, a dummy span resolves successfully against whatever file was registered first in the consumer's context — a confidently wrong span rather than a missing one. Direction: mint a reserved non-aliasing sentinel (or file_id_for_filename("<anonymous>")) instead of 0; the sentinel arrives with the quarto-source-map redesign.

Y4 — file_id_for_filename truncates to 32 bits on wasm32.
hasher.finish() as usize halves the hash width on wasm32 targets. Combined with SourceContext::add_file_with_id panicking on duplicate ids, a birthday collision (~2^16 registered filenames) becomes a crash in WASM consumers. The redesigned API plans a width-stable id and non-panicking duplicate handling; noting here because the hash recipe is documented as a stability contract.

quarto-yaml-validation

V1 — nothing ties a diagnostic to the SourceContext it is rendered with.
ValidationDiagnostic::from_validation_error(err, ctx_A) computes ranges with one context while to_text(&self, ctx_B) renders with whatever context the caller passes later; ValidationError::with_yaml_node(node, ctx) likewise accepts any ctx for any node. Constructing with one context and rendering with another is fully representable and produces byte offsets rendered against the wrong file's content (the exact bug class the q2 audit chased). Direction: a single owning handle so construction and rendering cannot see different contexts.

V2 — SourceRange mixes coordinate spaces.
In diagnostic.rs (source_range construction): filename comes from the fully-resolved root file, but start_offset/end_offset come from SourceInfo::start_offset()/end_offset(), which are parent-relative for every Substring (i.e. every parsed node). This is coincidentally correct for parse_file (parent spans the whole file from 0) and wrong for parse_with_parent: a frontmatter diagnostic reports the parent document's filename with offsets relative to the frontmatter, while start_line/start_column in the same struct are correct. Any JSON consumer slicing the named file at those offsets reads the wrong text. Fix: derive the offsets from resolve_byte_range() so they share a base with filename. (Also: end_offset() - start_offset() underflow-panics in debug for hand-built spans with end < start; SourceInfo::original never validates ordering.)

V3 — the filename hash is re-implemented inline in diagnostic.rs test helpers instead of calling the public file_id_for_filename, which parser.rs documents as the stability contract. A hash-recipe change would silently desynchronize them.

Non-issues verified while auditing

  • Schema-file spans cannot leak into instance diagnostics through the normal path: Schema discards spans after parsing and all add_error sites pass instance nodes.
  • create_contiguous_span's same-file assert is unreachable today (single builder, fixed parent) — though the Substring arm silently drops end_info's parent and would mint a hybrid span if per-node parents ever appear; an assert there would restore symmetry.

Happy to expand any of these into separate actionable issues when the redesign plan lands; treat this one as the umbrella/context record.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the planned quarto-source-map binding redesign, then read the cited APIs in parser.rs and diagnostic.rs, including parse_with_parent, file_id_for_filename, and SourceRange construction. This umbrella record is considered owned by the redesign rather than a standalone change; done means the redesigned contracts address the listed span, sentinel, hashing, and context-mismatch weaknesses.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend, tooling
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.