internal/mdloader review — 2026-09-28

Reviewed revision: fc2968d9. Scope: internal/mdloader, its extensions, and relevant noteloader, model, template and page-rendering consumers. This is a review, not a refactoring patch.

The biggest problem is ownership of rendering state. A single mutable renderer is shared by all notes, HTML is used as both output and dependency-readiness status, and resolved URL strings double as note identities. Those shortcuts explain several concrete failures below. Splitting loader.go into smaller files alone would not address them.

Highest-priority findings

P1 means fix promptly: concurrency, unsafe output, or silent loss of page content. P2 means a reproducible functional or operational defect to address next. Security impact depends on who can publish markdown and the deployed browser policy; this review does not claim an unauthenticated write path.

Rank	Priority	Problem	Evidence
1	P1	Concurrent partial renders overwrite shared page context	Reproduced with Go race detector
2	P1	Embed retries silently leave entire pages empty	Reproduced: empty target, cycle, long chain
3	P1	Wikilinks allow executable URL schemes	Reproduced generated `javascript:` href
4	P2	Domain-aware rendering breaks for embeds and mixed link/embed targets	Reproduced incorrect hrefs and absent domain variant
5	P2	Rendering performs synchronous external HTTP requests	Reproduced with a local HTTP transport stub
6	P2	Fragment embeds look up a URL fragment as part of note identity	Reproduced missing embed for an existing note
7	P2	Free previews discard supported block types	Reproduced missing GFM table

1. Shared renderer state is unsafe during concurrent requests

Locations: internal/mdloader/partial_renderer.go:31, internal/mdloader/loader.go:631, internal/mdloader/link_resolver.go:17.

Every note's PartialRenderer receives the same ldr.md and ldr.linkResolver. withCurrentPage saves, overwrites and restores resolver.currentPage without synchronization. It is used after loading, during template rendering: internal/defaulttemplate/views.html:393, :439, :445 call Introduce/Sections for cards and footers.

Reproduction: load two notes with ![[pic.png]], mapping that filename to different asset URLs. Concurrently call both notes' PartialRenderer.Introduce() in loops. go test -race reports reads/writes at partial_renderer.go:37–40, including reads from the resolver. This is an actual race, not merely an opportunity to parallelize the loader. Interleaving can resolve an asset or link using another note's context.

There is more shared state than currentPage: the heading renderer has stack/inDocument; the link renderer tracks enter/exit state by AST node. Its sync.Map does not make a whole render operation atomic. Locking each PartialRenderer separately would not protect the shared renderer.

Improve: create a render session with its own renderer instances and explicit page/host context. Keep the parsed document read-only. A single lock shared by all users of the existing renderer can be an interim fix, but serializes template work and must cover the complete operation, including restoration. Add concurrent tests for both different notes and the same note.

2. HTML emptiness is incorrectly used as dependency readiness

Locations: internal/mdloader/link_renderer.go:235, internal/mdloader/loader.go:379, especially :402–415.

renderEmbed treats len(note.HTML) == 0 as “not rendered yet.” The loader makes an initial pass and exactly three retry passes, then returns success even if dependencies remain unresolved. Successful retries are never removed from the retry list. Order comes from a Go map.

Reproductions:

a.md = Before ![[b]] after, with frontmatter-only b.md: Load succeeds; A has empty HTML and no warnings. A valid empty document is permanently considered unfinished.
A embeds B and B embeds A: same silent empty result.
A chain of 40 notes, each embedding the next, ending in End: one observed run returned 35 empty notes. The precise count depends on map order; it is not a stable limit.

The empty-HTML warning at loader.go:440 does not help: rendering returns an error before reaching it, and the caller ultimately swallows that error.

Improve: build an explicit embed dependency graph; render in dependency order with visiting/done/failed states, cycle diagnostics, and an output/depth budget. “Done with empty HTML” must be valid. Preserve surrounding content with a diagnostic placeholder or fail publication explicitly; never report a successful snapshot containing silently discarded pages. Test long chains in multiple source orders, self-embeds, cycles, and empty targets.

3. Wikilink URL escaping does not validate the URL scheme

Location: internal/mdloader/link_renderer.go:131.

Reproduction: [[javascript:alert(1)|click]] generates:

<p><a class="wip" href="javascript:alert(1)">click</a></p>

util.URLEscape is not a scheme allowlist. An unresolved target falls through to the HTML renderer as an href. The Enclave image branch has explicit source filtering, but wikilinks follow a separate path. This bypasses the safe-link behavior a reader might expect from a Markdown renderer.

Impact: generated content contains a click-triggered script URL. A restrictive CSP may prevent execution; no browser/CSP exploit test was performed. The source comments assume admin-authored content, so this finding is not evidence that an anonymous visitor can inject markdown. It remains an inconsistent safety boundary, especially for imported or automated content.

Improve: apply one deliberate URL policy to all custom renderers, covering safe relative URLs, supported schemes, control-character normalization and attribute escaping. Add tests for both ordinary Markdown links and wikilinks, including entity/control-character variants and asset replacements.

4. Domain behavior is attached to target strings instead of each use

Locations: internal/mdloader/domain_render.go:109–114, internal/mdloader/link_renderer.go:245, internal/mdloader/domain_render.go:227–228.

buildDomainResolvedLinks excludes any target that appears in an embed. Because ResolvedLinks is keyed only by target text, this also disables domain rewriting for an ordinary link to the same target. Embedded content is always copied from note.HTML, even if the target has a suitable DomainHTML variant.

Reproduction: A has route: foo.test/a and body [[b]] ![[b]]; B has route: foo.test/target and body [[c]]; C has route: foo.test/c-target. A receives no domain variant. Its ordinary link remains /b, and the embedded link remains /c, instead of /target and /c-target on foo.test.

The same architectural gap exists for free previews: domain rendering explicitly leaves DomainFreeHTML as a TODO, while internal/case/rendernotepage/view.html:204 serves FreeHTML directly. Partial-render APIs also have no host parameter.

Improve: resolve a link to stable note identity plus fragment first; choose its URL for the current host at rendering time. Embedding should consume a note identity and render under the host's context, rather than requiring a canonical URL as a lookup workaround. Use the same context for full HTML, previews, partials and nested embeds. Include host and output mode in any render-cache key.

5. A renderer hides synchronous network I/O

Location: internal/mdloader/image_renderer.go:152 (GetTweetOembedHtml). Dependency inspected locally: github.com/quailyquaily/goldmark-enclave@v0.2.1/object/twitter.go:19.

The Twitter renderer calls an external oEmbed endpoint synchronously. The dependency uses http.DefaultClient with a fresh five-second context rooted in context.Background(). The load/request cannot supply its own cancellation. Each render can fetch again: normal HTML, free HTML, domain HTML, retries and request-time partials.

Reproduction: replace http.DefaultClient temporarily with a counting local RoundTripper returning {"html":"tweet"}. Load a note containing ![](https://twitter.com/user/status/123): one request. Call Introduce() twice: total becomes three requests. No real network request was needed for this check.

Impact: a remote service can delay publishing and template responses; many embeds multiply the delay, and the output can change between rendering modes for identical input.

Improve: use an app-owned provider/cache, with bounded fetching and explicit cancellation outside the renderer. Render cached data or a predictable placeholder. The existing ChartDataProvider separation is a useful local precedent. Test that rendering an already prepared document does not initiate HTTP calls.

6. Fragment embeds are silently dropped

Locations: internal/mdloader/link_resolver.go:53, internal/mdloader/link_renderer.go:216–232.

The resolver appends #fragment to the destination. renderEmbed strips only ?version=..., then looks up the entire result as a note path. Ordinary link attribute lookup already strips fragments (link_renderer.go:263), so the two paths disagree.

Reproduction: A contains Before ![[b#Heading]] after; B contains # Heading and Body. A becomes Before after with embedded note not found: /b#Heading, even though B exists.

Improve: separate note identity from fragment and URL query data. Define section/block embed support explicitly: render the requested section when supported, otherwise issue an accurate unsupported-feature diagnostic. Merely stripping the fragment and embedding the whole note would silently change the author's selection.

7. Free-preview extraction has a closed list of block types

Location: internal/mdloader/free_cut.go:97–118.

renderFreeContent renders only selected core Goldmark node kinds. Other nodes are traversed without rendering their wrappers. A GFM table contains no recognized paragraph nodes, so the entire table disappears. A custom callout can lose its wrapper/title while its paragraphs survive.

Reproduction: with free_paragraphs: 1, place a GFM table first, then a paragraph After. FreeHTML contains only After, rather than a preview of the first visible block.

Improve: select preview blocks first, preserving their AST subtrees, then render them with the same renderer as the full document. Specify whether the limit counts blocks or paragraphs and how nested containers behave. Test tables, callouts, lists, media, charts and embeds.

Separate policy issue: free_cut: true without a separator renders all content. This is explicitly required by current tests (free_cut_test.go:27 and :360); it is not classified here as an accidental implementation bug. The comment claiming a first-paragraph fallback is stale. Because this governs a public preview, the missing-separator behavior should be intentional and documented, ideally with an author-visible warning.

General improvements

Make the pipeline and ownership explicit. Use parse → apply metadata → index/resolve → dependency planning → render variants → publish immutable snapshot. NoteView currently mixes parsed inputs, derived metadata, HTML, warning mutation, and a live renderer. Cache parsed input separately from render output.
Stop sharing mutable ASTs by convention. parseSource reuses cached.Ast() (loader.go:553), while image_renderer.go:102 can remove children during rendering. Move cleanup into parse-time transformation before publishing the cached document. This is an ownership concern found by inspection; the review did not prove a separate cached-AST race in production.
Use structured resolved references. A note pointer/ID, reference kind, fragment, original text and diagnostic status are safer than reverse lookup from a rendered URL. This addresses the domain and fragment bugs together and reduces .html/?version= string surgery.
Give diagnostics a consistent contract. Partial renderers swallow errors after writing into the same buffer (partial_renderer.go:276, :415, :435); some load errors abort the vault, others only log, and unresolved retries disappear entirely. Define fatal load failures versus recoverable note diagnostics. Render a node into a temporary buffer before committing it if failure is recoverable; avoid publishing half-rendered markup.
Optimize only after ownership and correctness are fixed. findAssets walks nvs.Map rather than unique notes (loader.go:282), repeating work for aliases. Domain rendering repeatedly walks ASTs to rediscover embeds, and successful retry notes are rendered again. Collect reference/dependency information once per parsed document; benchmark notes × links × hosts and an embed chain before adding workers. Parallelizing the current shared renderer would amplify finding 1.

There is useful existing work to preserve: deterministic basename indexing, separation into Goldmark extensions, targeted XSS tests for Enclave images, patch-result caching, and substantial rendering fixtures. A gradual refactor with regression tests is preferable to replacing the whole module.

Suggested sequence: first isolate render state, fix URL validation and replace silent embed retries; then unify link identity and render context; then repair preview selection and move external I/O behind a provider. Each step should add a failing regression test before implementation.

Verification and limits

go test ./internal/mdloader/... passed for the existing suite.
The existing suite also passed with the race detector; the new concurrent reproduction failed with WARNING: DATA RACE. Existing sequential fixtures therefore do not cover this request-time access pattern.
Temporary TestReviewProbeOutputs and TestReviewProbeNetwork probes produced the outputs quoted above. Those probes logged observations; their passing status does not mean the observed behavior was correct.
Temporary probes were removed after review. Only this report is retained. No production code changed, no external request was sent by the HTTP reproduction, and no whole-system benchmark or browser exploit test was run.
This is a focused module review, not a complete security or authorization audit. In particular, whether paid content may intentionally be embedded into a public host page needs a separate product/access-policy decision; the current embed implementation copies full target HTML.

To reproduce the race, create a temporary external-package test under internal/mdloader, load two sources with the same asset name and different replacement URLs, then run this pattern with go test -race ./internal/mdloader -run TestReviewRace -count=1:

func TestReviewRace(t *testing.T) {
    sources := []mdloader.SourceFile{
        {Path: "a.md", Content: []byte("![[pic.png]]"),
            Assets: map[string]*model.NoteAssetReplace{"pic.png": {URL: "/a.png"}}},
        {Path: "b.md", Content: []byte("![[pic.png]]"),
            Assets: map[string]*model.NoteAssetReplace{"pic.png": {URL: "/b.png"}}},
    }
    notes, err := mdloader.Load(mdloader.Options{Sources: sources, Log: &logger.TestLogger{}})
    if err != nil { t.Fatal(err) }
    var wg sync.WaitGroup
    for _, note := range notes.PathMap {
        wg.Add(1)
        go func(n *model.NoteView) {
            defer wg.Done()
            for i := 0; i < 100; i++ { n.PartialRenderer.Introduce() }
        }(note)
    }
    wg.Wait()
}

Imports: sync, testing, trip2g/internal/logger, trip2g/internal/mdloader, trip2g/internal/model; package: mdloader_test.