~bigbes/sr-ht-spec

ref: 5b2fa22879bf5fe9b36c2bbc633de4e69f6ffa75 sr-ht-spec/doc/review.go -rw-r--r-- 12.9 KiB
5b2fa228 — Eugene Blikh doc: say which of the three guards on the review renderer is load-bearing a day ago
                                                                                
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
205
206
207
208
209
210
211
212
213
214
215
216
217
218
219
220
221
222
223
224
225
226
227
228
229
230
231
232
233
234
235
236
237
238
239
240
241
242
243
244
245
246
247
248
249
250
251
252
253
254
255
256
257
258
259
260
261
262
263
264
265
266
267
268
269
270
271
272
273
274
275
276
277
278
279
280
281
282
283
284
285
286
287
288
289
290
291
292
293
294
295
296
297
298
299
300
301
302
303
304
305
306
307
308
309
310
311
312
313
314
315
316
317
318
package doc

import (
	"strings"

	"github.com/yuin/goldmark"
	"github.com/yuin/goldmark/ast"
	"github.com/yuin/goldmark/extension"
	"github.com/yuin/goldmark/parser"
	"github.com/yuin/goldmark/renderer"
	"github.com/yuin/goldmark/renderer/html"
	"github.com/yuin/goldmark/util"
)

// This file is the REVIEW plane's markdown renderer: the one that renders a
// document nobody has approved yet.
//
// SECURITY, AND WHY IT DIFFERS FROM NewRenderer. The read plane turns goldmark's
// unsafe mode on, and the reason it states — "documents here are first-party and
// reviewed" — is exactly inverted on the review page: the source is
// agent-authored and approving it is what the page is for. A <script> reaching
// this page runs in the reviewer's session, with the reviewer's cookie, while
// they decide whether that document is acceptable.
//
// THREE THINGS HOLD THAT, and it is worth knowing which does what, because two
// of them are load-bearing and the famous one is not:
//
//  1. reviewHTMLRenderer, below, escapes raw HTML instead of emitting it. This is
//     the mechanism for the <script> case. Removing it turns six tests red.
//  2. reviewResolver refuses a dangerous destination, which is the mechanism for
//     [[javascript:…|click]]: wikilink.go writes its own anchor and takes the
//     href from the Target without checking it. Removing it turns three tests
//     red, one of them exactly that wikilink.
//  3. Unsafe stays OFF. Measured by mutation: putting html.WithUnsafe() back
//     turns NOTHING red today, because (1) intercepts the node kinds it governs
//     and (2) strips the destinations before goldmark's own check would. It stays
//     off anyway — it is the backstop for a goldmark version that renders
//     something neither (1) nor (2) covers, and for the day someone simplifies
//     one of them away. Do not read the green suite as licence to turn it on.
//
// Unsafe off costs one thing this plane cannot afford, which is why (1) is not
// simply "leave it to goldmark": goldmark's own answer to raw HTML with unsafe
// off is to REPLACE it with "<!-- raw HTML omitted -->", and a review page that
// silently drops part of the document under review is worse than one that shows
// markup. Here raw HTML is escaped and shown as the text it is.
//
// It is a separate goldmark instance rather than a flag on the read one because
// the two planes have opposite trust in their input, and a shared instance would
// be one WithUnsafe() away from making the review page unsafe as a side effect
// of a change to the read page. The cost is that the extension list is written
// twice: TestReviewRendererMatchesTheReadPlaneExceptForRawHTML is what notices
// when the two drift.

// PieceOp says what the diff found for one run of a block's text.
type PieceOp int

const (
	// PieceEqual is text present in both revisions.
	PieceEqual PieceOp = iota
	// PieceInsert is text the proposal adds.
	PieceInsert
	// PieceDelete is text it removes.
	PieceDelete
)

// Piece is one run of a block's markdown source with the change it carries. It
// is prosediff.Span's shape without the dependency: doc renders markdown and
// knows nothing about how a diff was computed.
type Piece struct {
	Text string
	Op   PieceOp
	// Space says a separator precedes this piece in the combined rendering. It
	// is a flag rather than a leading space in Text so that the separator stays
	// OUTSIDE the <ins>/<del> the piece becomes — an underlined or tinted space
	// hanging off the front of a replacement word reads as part of the word.
	Space bool
}

// Sentinel runes for the change marks, from the Unicode Private Use Area.
//
// The problem they solve: a word-level diff knows which WORDS changed, and the
// reviewer has to read a rendered document rather than its markdown source, so
// the marks have to survive a markdown rendering. Wrapping them in <ins>/<del>
// before rendering does not work (the tags are text to goldmark, and with unsafe
// off they are escaped into visible angle brackets); walking the AST afterwards
// to map byte offsets back onto spans is a lot of machinery to get wrong.
//
// A rune goldmark has no opinion about is neither: it passes through the parser
// and the renderer as ordinary text, and it cannot be confused with document
// content, because any such rune the document itself carries is stripped before
// the marks go in (see strip). The swap back happens after rendering, and the
// only strings it can produce are the four fixed tags below — a document cannot
// smuggle markup through it.
//
// The four literals are the runes U+E000, U+E001, U+E002 and U+E003, in that
// order. They are invisible in most editors, so this paragraph is how you know
// which is which; a hex dump of the four lines below is how you check.
const (
	insOpen  = ""
	insClose = ""
	delOpen  = ""
	delClose = ""
)

// strip removes any sentinel the document itself contains, so that the marks in
// the rendered output are exactly the ones this package put there. Without it a
// document carrying U+E000 would grow an <ins> nobody asked for — and, worse,
// would unbalance the pairs and cost its block the rendering (see Inline).
var strip = strings.NewReplacer(insOpen, "", insClose, "", delOpen, "", delClose, "")

// unmark swaps the sentinels for the tags they stand for. It runs on rendered
// HTML, so its replacements are markup by intent; nothing else in this file
// writes a tag from document content.
var unmark = strings.NewReplacer(
	insOpen, "<ins>", insClose, "</ins>",
	delOpen, "<del>", delClose, "</del>",
)

// ReviewRenderer renders one block of a proposed document as inline HTML, with
// the diff's word-level marks in place. It is reusable and concurrency-safe, as
// Renderer is.
type ReviewRenderer struct {
	// inner wraps the goldmark instance in a Renderer so that link resolution,
	// the wikilink extension and the paragraph unwrapping are the read plane's
	// code and not a second copy of it. Only the configuration differs.
	inner *Renderer
	res   Resolver
}

// NewReviewRenderer builds the renderer the review page uses: the read plane's
// markdown dialect, with raw HTML shown as text instead of executed and with no
// document set to resolve links against.
func NewReviewRenderer() *ReviewRenderer {
	md := goldmark.New(
		goldmark.WithExtensions(
			extension.GFM, // tables, strikethrough, autolinks, task lists
			extension.DefinitionList,
			extension.Footnote,
			wikilinkExtension{},
		),
		goldmark.WithParserOptions(
			parser.WithAutoHeadingID(),
		),
		goldmark.WithRendererOptions(
			// No html.WithUnsafe(). See the file comment: this is the difference
			// from NewRenderer, and it is the point of the file.
			renderer.WithNodeRenderers(
				util.Prioritized(tableRenderer{}, tableRendererPriority),
				util.Prioritized(reviewHTMLRenderer{}, reviewHTMLPriority),
			),
		),
	)
	return &ReviewRenderer{inner: &Renderer{md: md}, res: reviewResolver{}}
}

// Inline renders one block's pieces as inline HTML — no wrapping paragraph —
// with the inserted and deleted runs wrapped in <ins> and <del>.
//
// ok is false when the marks did not survive the rendering, and then the HTML is
// discarded rather than served: a mark that landed inside a link destination
// comes back percent-encoded, and a mark whose opener and closer ended up in
// different elements comes back inverted. Either way the caller is told, so it
// can show the block's source instead of a rendering that has quietly lost the
// diff. Guessing which half to trust is what this refuses to do.
func (r *ReviewRenderer) Inline(pieces []Piece) (string, bool) {
	var src strings.Builder
	pairs := 0
	emitted := false
	for _, p := range pieces {
		text := strip.Replace(p.Text)
		if p.Space && emitted {
			src.WriteByte(' ')
		}
		emitted = true
		switch p.Op {
		case PieceInsert:
			src.WriteString(insOpen + text + insClose)
			pairs++
		case PieceDelete:
			src.WriteString(delOpen + text + delClose)
			pairs++
		default:
			src.WriteString(text)
		}
	}

	out := r.inner.RenderInline([]byte(src.String()), "", r.res)
	if !marksIntact(out, pairs) {
		return "", false
	}
	return unmark.Replace(out), true
}

// marksIntact reports whether rendered output carries exactly the pairs of
// sentinels that went into it, each closed by its own closer and none nested
// inside another. That is the whole check: it cannot prove the tags will nest
// correctly against the markup around them, but every way a mark is LOST —
// percent-encoded into an href, dropped with an omitted node, duplicated into
// both a link's target and its label — shows up here as a count that does not
// match.
func marksIntact(out string, pairs int) bool {
	open := ""
	found := 0
	for _, r := range out {
		switch string(r) {
		case insOpen, delOpen:
			if open != "" {
				return false
			}
			open = string(r)
		case insClose:
			if open != insOpen {
				return false
			}
			open = ""
			found++
		case delClose:
			if open != delOpen {
				return false
			}
			open = ""
			found++
		}
	}
	return open == "" && found == pairs
}

// reviewResolver is the review plane's link resolution: there is none.
//
// A proposal is read off its branch, and no Archive is built for a revision
// nobody has approved — so this resolver has no document set to look a
// destination up in. A destination is therefore handed back as written, which is
// what Target documents for a link that resolved to nothing. It is deliberately
// NOT reported Missing: "missing" is a claim about the corpus, the renderer
// draws it as a visibly broken link, and this resolver cannot tell a broken
// wikilink from a perfectly good one — marking every internal link in the
// proposal broken would be a fabrication repeated once per link.
//
// The consequence is a known limitation rather than a defect to paper over: an
// internal link on the review page points at the destination as typed, so
// following one is unlikely to land anywhere. The review page is for reading the
// proposed text; navigating the corpus is the read plane's job, and giving these
// links real hrefs needs the space and revision the diff does not carry.
type reviewResolver struct{}

func (reviewResolver) Resolve(_, dest string) Target {
	dest = strings.TrimSpace(dest)
	// A dangerous destination gets no href at all. goldmark applies this check
	// itself with unsafe off, but only to the links IT renders: wikilink.go
	// writes its own anchor and takes the href from this Target, so
	// [[javascript:…|click]] would otherwise reach the page unchecked. Reported
	// Missing because that is the one state the wikilink renderer draws without
	// an href — and here the claim is true: the destination resolved to nothing
	// that may be linked to.
	if html.IsDangerousURL([]byte(dest)) {
		return Target{Missing: true}
	}
	if dest == "" || strings.HasPrefix(dest, "#") {
		return Target{Href: dest}
	}
	if schemeRe.MatchString(dest) || strings.HasPrefix(dest, "//") {
		return Target{Href: dest, IsExternal: true}
	}
	return Target{Href: dest}
}

// reviewHTMLRenderer shows raw HTML as the text it is, rather than executing it
// (unsafe on) or omitting it (unsafe off, goldmark's default).
//
// This page can afford neither behaviour. Executing it is the hole this whole
// file exists to close. Omitting it silently removes part of the document under
// review: the reviewer sees a gap where the agent wrote markup, approves what
// they read, and merges what they did not. So the bytes are escaped and shown —
// the reviewer reads exactly what is in the file, and a <script> is a visible
// sentence instead of a running script.
type reviewHTMLRenderer struct{}

// reviewHTMLPriority beats goldmark's own html renderer at 1000. goldmark
// registers node renderers from the lowest priority number last, so the smaller
// number is the one that wins a node kind — the same rule tableRenderer relies
// on.
const reviewHTMLPriority = 100

func (reviewHTMLRenderer) RegisterFuncs(reg renderer.NodeRendererFuncRegisterer) {
	reg.Register(ast.KindRawHTML, renderRawHTMLAsText)
	reg.Register(ast.KindHTMLBlock, renderHTMLBlockAsText)
}

func renderRawHTMLAsText(w util.BufWriter, source []byte, node ast.Node, entering bool) (ast.WalkStatus, error) {
	if !entering {
		return ast.WalkSkipChildren, nil
	}
	n := node.(*ast.RawHTML)
	for i := 0; i < n.Segments.Len(); i++ {
		seg := n.Segments.At(i)
		_, _ = w.WriteString(escapeText(string(seg.Value(source))))
	}
	return ast.WalkSkipChildren, nil
}

func renderHTMLBlockAsText(w util.BufWriter, source []byte, node ast.Node, entering bool) (ast.WalkStatus, error) {
	n := node.(*ast.HTMLBlock)
	if !entering {
		if n.HasClosure() {
			_, _ = w.WriteString(escapeText(string(n.ClosureLine.Value(source))))
		}
		_, _ = w.WriteString("</pre>\n")
		return ast.WalkContinue, nil
	}
	// A block of raw HTML is read line by line, exactly as a code fence is, so
	// it is presented as one: its own lines, its own whitespace, no reflow.
	_, _ = w.WriteString(`<pre class="raw-html">`)
	for i := 0; i < n.Lines().Len(); i++ {
		line := n.Lines().At(i)
		_, _ = w.WriteString(escapeText(string(line.Value(source))))
	}
	return ast.WalkContinue, nil
}