fix: harden docs checker heading lines, definitions and docs symlinks
- N1: never call Lines() on inline nodes; locate empty ATX headings after the end of all preceding blocks (no panic, correct line after `***`) - N2: take reference definition lines from ast.LinkReferenceDefinition and drop refCtx/defRe - N3: reject docs/adr and docs/plans that resolve outside the repository Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
9eec1d1016
commit
694c43051e
6 files changed
+55
-58
No files matched your search
+1
-1
@@ -7,7 +7,7 @@ The ADR body uses the [MADR 4](https://adr.github.io/madr/) layout; `Consequence
|
|||||||
## 1. Files and directories
|
## 1. Files and directories
|
||||||
|
|
||||||
- ADRs live in `docs/adr/`, plans in `docs/plans/`, relative to the repository root. Both are flat: subdirectories are not scanned and are errors.
|
- ADRs live in `docs/adr/`, plans in `docs/plans/`, relative to the repository root. Both are flat: subdirectories are not scanned and are errors.
|
||||||
- A missing directory is treated as empty. `docs/adr` and `docs/plans` MUST be real directories: a symlink in their place is an error.
|
- A missing directory is treated as empty. `docs/adr` and `docs/plans` MUST be real directories: a symlink in their place is an error, and neither may resolve outside the repository through a symlinked parent such as `docs`.
|
||||||
- Entries whose name starts with `.` (such as `.DS_Store` or editor swap files) are ignored. `README.md` in either directory is skipped. Every other entry MUST be a regular file whose name matches section 2 and ends in `.md`. Any other file, directory or name is an error. There are no other exceptions: no templates and no companion files; every `.md` file is a record.
|
- Entries whose name starts with `.` (such as `.DS_Store` or editor swap files) are ignored. `README.md` in either directory is skipped. Every other entry MUST be a regular file whose name matches section 2 and ends in `.md`. Any other file, directory or name is an error. There are no other exceptions: no templates and no companion files; every `.md` file is a record.
|
||||||
- Files are UTF-8 without a byte order mark. LF and CRLF line endings are both accepted.
|
- Files are UTF-8 without a byte order mark. LF and CRLF line endings are both accepted.
|
||||||
|
|
||||||
|
|||||||
+28
-56
@@ -3,12 +3,10 @@ package docs
|
|||||||
import (
|
import (
|
||||||
"bytes"
|
"bytes"
|
||||||
"regexp"
|
"regexp"
|
||||||
"slices"
|
|
||||||
"strings"
|
"strings"
|
||||||
|
|
||||||
"github.com/yuin/goldmark"
|
"github.com/yuin/goldmark"
|
||||||
"github.com/yuin/goldmark/ast"
|
"github.com/yuin/goldmark/ast"
|
||||||
"github.com/yuin/goldmark/parser"
|
|
||||||
"github.com/yuin/goldmark/text"
|
"github.com/yuin/goldmark/text"
|
||||||
"github.com/yuin/goldmark/util"
|
"github.com/yuin/goldmark/util"
|
||||||
)
|
)
|
||||||
@@ -40,30 +38,26 @@ func (d *doc) lineAt(off int) int {
|
|||||||
return d.startLine + bytes.Count(d.src[:min(off, len(d.src))], []byte("\n"))
|
return d.startLine + bytes.Count(d.src[:min(off, len(d.src))], []byte("\n"))
|
||||||
}
|
}
|
||||||
|
|
||||||
// headingLine is the line of h. An empty ATX heading has no lines, so it is the first
|
// blockEnd is the end offset of the last line under n. Only block nodes have lines.
|
||||||
// `#` line after the end of the previous top-level block.
|
func blockEnd(n ast.Node) (end int) {
|
||||||
func (d *doc) headingLine(h *ast.Heading, prev ast.Node) int {
|
_ = ast.Walk(n, func(c ast.Node, enter bool) (ast.WalkStatus, error) {
|
||||||
if h.Lines().Len() > 0 {
|
if enter && c.Type() == ast.TypeBlock {
|
||||||
return d.lineAt(h.Lines().At(0).Start)
|
|
||||||
}
|
|
||||||
from := 0
|
|
||||||
if prev != nil {
|
|
||||||
_ = ast.Walk(prev, func(c ast.Node, enter bool) (ast.WalkStatus, error) {
|
|
||||||
if enter {
|
|
||||||
for i := range c.Lines().Len() {
|
for i := range c.Lines().Len() {
|
||||||
from = max(from, c.Lines().At(i).Stop)
|
end = max(end, c.Lines().At(i).Stop)
|
||||||
}
|
|
||||||
if t, ok := c.(*ast.Text); ok {
|
|
||||||
from = max(from, t.Segment.Stop)
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
return ast.WalkContinue, nil
|
return ast.WalkContinue, nil
|
||||||
})
|
})
|
||||||
}
|
return end
|
||||||
|
}
|
||||||
|
|
||||||
|
// emptyHeadingAt is the offset of an empty ATX heading, which has no lines: the first `#`
|
||||||
|
// line at or after from, the end of everything before it.
|
||||||
|
func (d *doc) emptyHeadingAt(from int) int {
|
||||||
if m := atxRe.FindIndex(d.src[from:]); m != nil {
|
if m := atxRe.FindIndex(d.src[from:]); m != nil {
|
||||||
return d.lineAt(from + m[0])
|
return from + m[0]
|
||||||
}
|
}
|
||||||
return d.startLine
|
return from
|
||||||
}
|
}
|
||||||
|
|
||||||
// lineOf is the line of the first text under n. Without text (an autolink, an image with
|
// lineOf is the line of the first text under n. Without text (an autolink, an image with
|
||||||
@@ -93,18 +87,25 @@ func (d *doc) lineOf(n ast.Node, needle string) int {
|
|||||||
|
|
||||||
func (c *checker) body(r *record, src string, startLine int, title string, titleOK bool) {
|
func (c *checker) body(r *record, src string, startLine int, title string, titleOK bool) {
|
||||||
b := []byte(src)
|
b := []byte(src)
|
||||||
ctx := &refCtx{Context: parser.NewContext()}
|
root := goldmark.DefaultParser().Parse(text.NewReader(b))
|
||||||
root := goldmark.DefaultParser().Parse(text.NewReader(b), parser.WithContext(ctx))
|
|
||||||
d := &doc{c: c, r: r, src: b, startLine: startLine}
|
d := &doc{c: c, r: r, src: b, startLine: startLine}
|
||||||
|
from := 0 // end of the blocks seen so far
|
||||||
for n := root.FirstChild(); n != nil; n = n.NextSibling() {
|
for n := root.FirstChild(); n != nil; n = n.NextSibling() {
|
||||||
if h, ok := n.(*ast.Heading); ok {
|
if h, ok := n.(*ast.Heading); ok {
|
||||||
line := d.headingLine(h, n.PreviousSibling())
|
at := from
|
||||||
d.heads = append(d.heads, heading{h.Level, strings.TrimSpace(string(h.Lines().Value(b))), line, len(d.kids)})
|
if h.Lines().Len() > 0 {
|
||||||
|
at = h.Lines().At(0).Start
|
||||||
|
} else {
|
||||||
|
at = d.emptyHeadingAt(from)
|
||||||
|
from = at + bytes.IndexByte(d.src[at:], '\n') + 1 // the next line; ^ would match mid-line
|
||||||
}
|
}
|
||||||
|
d.heads = append(d.heads, heading{h.Level, strings.TrimSpace(string(h.Lines().Value(b))), d.lineAt(at), len(d.kids)})
|
||||||
|
}
|
||||||
|
from = max(from, blockEnd(n))
|
||||||
d.kids = append(d.kids, n)
|
d.kids = append(d.kids, n)
|
||||||
}
|
}
|
||||||
d.placeholders(root)
|
d.placeholders(root)
|
||||||
d.links(root, ctx)
|
d.links(root)
|
||||||
d.title(title, titleOK)
|
d.title(title, titleOK)
|
||||||
if r.kind == "adr" {
|
if r.kind == "adr" {
|
||||||
for _, name := range adrSections {
|
for _, name := range adrSections {
|
||||||
@@ -295,27 +296,13 @@ func (d *doc) placeholders(root ast.Node) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// refCtx records every link reference definition; the parser keeps only the first of a label.
|
|
||||||
type refCtx struct {
|
|
||||||
parser.Context
|
|
||||||
all []parser.Reference
|
|
||||||
}
|
|
||||||
|
|
||||||
func (c *refCtx) AddReference(r parser.Reference) {
|
|
||||||
c.all = append(c.all, r)
|
|
||||||
c.Context.AddReference(r)
|
|
||||||
}
|
|
||||||
|
|
||||||
// defRe finds a definition's label at the start of a line, allowing blockquote and list prefixes.
|
|
||||||
var defRe = regexp.MustCompile(`(?m)^[ \t>]*(?:(?:[*+-]|\d+[.)])[ \t]+)*\[([^\]\n]+)\]:`)
|
|
||||||
|
|
||||||
// dest decodes a link destination as CommonMark does before it is used.
|
// dest decodes a link destination as CommonMark does before it is used.
|
||||||
func dest(b []byte) string {
|
func dest(b []byte) string {
|
||||||
return string(util.ResolveEntityNames(util.ResolveNumericReferences(util.UnescapePunctuations(b))))
|
return string(util.ResolveEntityNames(util.ResolveNumericReferences(util.UnescapePunctuations(b))))
|
||||||
}
|
}
|
||||||
|
|
||||||
// links checks every link, image and autolink destination and every reference definition.
|
// links checks every link, image and autolink destination and every reference definition.
|
||||||
func (d *doc) links(root ast.Node, ctx *refCtx) {
|
func (d *doc) links(root ast.Node) {
|
||||||
_ = ast.Walk(root, func(n ast.Node, enter bool) (ast.WalkStatus, error) {
|
_ = ast.Walk(root, func(n ast.Node, enter bool) (ast.WalkStatus, error) {
|
||||||
if !enter {
|
if !enter {
|
||||||
return ast.WalkContinue, nil
|
return ast.WalkContinue, nil
|
||||||
@@ -327,24 +314,9 @@ func (d *doc) links(root ast.Node, ctx *refCtx) {
|
|||||||
d.c.checkLink(d.r, d.lineOf(n, "]("+string(t.Destination)), dest(t.Destination))
|
d.c.checkLink(d.r, d.lineOf(n, "]("+string(t.Destination)), dest(t.Destination))
|
||||||
case *ast.AutoLink:
|
case *ast.AutoLink:
|
||||||
d.c.checkLink(d.r, d.lineOf(n, "<"+string(t.Label(d.src))), string(t.URL(d.src)))
|
d.c.checkLink(d.r, d.lineOf(n, "<"+string(t.Label(d.src))), string(t.URL(d.src)))
|
||||||
|
case *ast.LinkReferenceDefinition:
|
||||||
|
d.c.checkLink(d.r, d.lineAt(t.Lines().At(0).Start), dest(t.Destination))
|
||||||
}
|
}
|
||||||
return ast.WalkContinue, nil
|
return ast.WalkContinue, nil
|
||||||
})
|
})
|
||||||
// shortcut: a definition's line is its label's nth match outside code; a multi-line
|
|
||||||
// definition inside a container may be located by label only. Upgrade by tracking offsets.
|
|
||||||
code := codeRanges(root)
|
|
||||||
lines := map[string][]int{}
|
|
||||||
for _, m := range defRe.FindAllSubmatchIndex(d.src, -1) {
|
|
||||||
if !slices.ContainsFunc(code, func(r [2]int) bool { return m[0] >= r[0] && m[0] < r[1] }) {
|
|
||||||
k := util.ToLinkReference(d.src[m[2]:m[3]])
|
|
||||||
lines[k] = append(lines[k], d.lineAt(m[0]))
|
|
||||||
}
|
|
||||||
}
|
|
||||||
for _, ref := range ctx.all {
|
|
||||||
line, k := d.startLine, util.ToLinkReference(ref.Label())
|
|
||||||
if len(lines[k]) > 0 {
|
|
||||||
line, lines[k] = lines[k][0], lines[k][1:]
|
|
||||||
}
|
|
||||||
d.c.checkLink(d.r, line, dest(ref.Destination()))
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
@@ -62,6 +62,9 @@ func Check(root string) (Result, error) {
|
|||||||
} else if err == nil && !st.IsDir() {
|
} else if err == nil && !st.IsDir() {
|
||||||
c.add(d.dir, 1, "must be a real directory (symlinks are not allowed)")
|
c.add(d.dir, 1, "must be a real directory (symlinks are not allowed)")
|
||||||
continue
|
continue
|
||||||
|
} else if r, err := filepath.EvalSymlinks(filepath.Join(root, d.dir)); err == nil && !within(real, r) {
|
||||||
|
c.add(d.dir, 1, "must be inside the repository (a symlinked parent directory is not allowed)")
|
||||||
|
continue
|
||||||
}
|
}
|
||||||
entries, err := os.ReadDir(filepath.Join(root, d.dir))
|
entries, err := os.ReadDir(filepath.Join(root, d.dir))
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
@@ -337,6 +337,10 @@ func TestCheck(t *testing.T) {
|
|||||||
{name: "plan missing section", files: map[string]string{pPath: sub(planBase, "## Steps\n\n1. Do it.\n\n", "")}, want: []string{pPath + ":1: missing required section \"Steps\""}},
|
{name: "plan missing section", files: map[string]string{pPath: sub(planBase, "## Steps\n\n1. Do it.\n\n", "")}, want: []string{pPath + ":1: missing required section \"Steps\""}},
|
||||||
{name: "plan empty H1", files: map[string]string{pPath: sub(planBase, "# Adopt Queue", "#")}, want: []string{pPath + ":8: H1"}},
|
{name: "plan empty H1", files: map[string]string{pPath: sub(planBase, "# Adopt Queue", "#")}, want: []string{pPath + ":8: H1"}},
|
||||||
{name: "empty ADR H1 is on its own line", files: map[string]string{aPath: sub(adrBase, "# Use Queue", "#")}, want: []string{aPath + ":16: must equal title"}},
|
{name: "empty ADR H1 is on its own line", files: map[string]string{aPath: sub(adrBase, "# Use Queue", "#")}, want: []string{aPath + ":16: must equal title"}},
|
||||||
|
{name: "empty H2 after a paragraph", files: map[string]string{aPath: adrBase + "\nText.\n\n##\n"}},
|
||||||
|
{name: "H1 after an empty H2 after a paragraph", files: map[string]string{aPath: adrBase + "\nText.\n\n##\n#\n"}, want: []string{aPath + ":38: more than one H1"}},
|
||||||
|
{name: "empty H1 after a thematic break", files: map[string]string{aPath: adrBase + "\n***\n#\n"}, want: []string{aPath + ":36: more than one H1"}},
|
||||||
|
{name: "two empty H1s", files: map[string]string{aPath: adrBase + "\n#\n#\n"}, want: []string{aPath + ":35: more than one H1", aPath + ":36: more than one H1"}},
|
||||||
{name: "headings inside blockquotes and lists do not count", files: map[string]string{aPath: adrBase + "\n> ## Consequences\n\n* ## Decision Outcome\n"}},
|
{name: "headings inside blockquotes and lists do not count", files: map[string]string{aPath: adrBase + "\n> ## Consequences\n\n* ## Decision Outcome\n"}},
|
||||||
{name: "H4 does not end the Decision Outcome prose", files: map[string]string{aPath: sub(adrBase, "Chosen option: Queue.", "#### Detail\n\nChosen.")}},
|
{name: "H4 does not end the Decision Outcome prose", files: map[string]string{aPath: sub(adrBase, "Chosen option: Queue.", "#### Detail\n\nChosen.")}},
|
||||||
{name: "plan second H1", files: map[string]string{pPath: planBase + "\n# Again\n"}, want: []string{pPath + ":22: more than one H1"}},
|
{name: "plan second H1", files: map[string]string{pPath: planBase + "\n# Again\n"}, want: []string{pPath + ":22: more than one H1"}},
|
||||||
@@ -367,6 +371,16 @@ func TestCheckSymlinkedDocsDir(t *testing.T) {
|
|||||||
expect(t, res.Problems, []string{"docs/plans:1: real directory"})
|
expect(t, res.Problems, []string{"docs/plans:1: real directory"})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func TestCheckSymlinkedDocsParent(t *testing.T) {
|
||||||
|
root := tree(t, nil, map[string]string{"docs": "@outside"})
|
||||||
|
out, err := os.Readlink(filepath.Join(root, "docs"))
|
||||||
|
must(t, err)
|
||||||
|
must(t, os.MkdirAll(filepath.Join(out, "adr"), 0o755))
|
||||||
|
res, err := Check(root)
|
||||||
|
must(t, err)
|
||||||
|
expect(t, res.Problems, []string{"docs/adr:1: inside the repository"})
|
||||||
|
}
|
||||||
|
|
||||||
func TestCheckEmptyRoot(t *testing.T) {
|
func TestCheckEmptyRoot(t *testing.T) {
|
||||||
res, err := Check(t.TempDir())
|
res, err := Check(t.TempDir())
|
||||||
must(t, err)
|
must(t, err)
|
||||||
|
|||||||
@@ -73,6 +73,8 @@ func TestLinkForms(t *testing.T) {
|
|||||||
{"duplicate definition is checked", "[a]: /docs/spec.txt\n\n[a]: ../bad.md", []string{":37: relative"}},
|
{"duplicate definition is checked", "[a]: /docs/spec.txt\n\n[a]: ../bad.md", []string{":37: relative"}},
|
||||||
{"definition in a blockquote", "> [Zed]: ../bad.md", []string{":35: relative"}},
|
{"definition in a blockquote", "> [Zed]: ../bad.md", []string{":35: relative"}},
|
||||||
{"definition-like line in code", "```\n[a]: x\n```\n\n[a]: ../bad.md", []string{":39: relative"}},
|
{"definition-like line in code", "```\n[a]: x\n```\n\n[a]: ../bad.md", []string{":39: relative"}},
|
||||||
|
{"definition-like paragraph continuation", "Some text\n[a]: ../x.md\n\n[a]: ../bad.md", []string{":36: relative", ":38: relative"}}, // [a] in the text is a reference link
|
||||||
|
{"definition-like line in an HTML comment", "<!--\n[a]: x\n-->\n\n[a]: ../bad.md", []string{":39: relative"}},
|
||||||
{"autolink on a later line", "Para one\nline two\n<ftp://example.com/x>", []string{":37: scheme"}},
|
{"autolink on a later line", "Para one\nline two\n<ftp://example.com/x>", []string{":37: scheme"}},
|
||||||
{"image without alt on a later line", "Para one\nline two\n", []string{":37: relative"}},
|
{"image without alt on a later line", "Para one\nline two\n", []string{":37: relative"}},
|
||||||
{"unused definition", "[r]: foo.md", []string{":35: relative"}},
|
{"unused definition", "[r]: foo.md", []string{":35: relative"}},
|
||||||
|
|||||||
@@ -108,7 +108,7 @@ func (c *checker) exists(p string) string {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
return "target " + p + " does not exist"
|
return "target " + p + " does not exist"
|
||||||
}
|
}
|
||||||
if rel, err := filepath.Rel(c.realRoot, real); err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
|
if !within(c.realRoot, real) {
|
||||||
return "target " + p + " resolves outside the repository"
|
return "target " + p + " resolves outside the repository"
|
||||||
}
|
}
|
||||||
if st, err := os.Stat(real); err != nil || !st.Mode().IsRegular() {
|
if st, err := os.Stat(real); err != nil || !st.Mode().IsRegular() {
|
||||||
@@ -186,3 +186,9 @@ func (k kinds) String() string {
|
|||||||
}
|
}
|
||||||
return strings.Join(s, ", ")
|
return strings.Join(s, ", ")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// within reports whether the real path p is root or lies under it.
|
||||||
|
func within(root, p string) bool {
|
||||||
|
rel, err := filepath.Rel(root, p)
|
||||||
|
return err == nil && rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator))
|
||||||
|
}
|
||||||
Reference in new issue
Block a user