diff --git a/CLAUDE.md b/CLAUDE.md index fd0572f..c4936bb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -48,8 +48,8 @@ Module `github.com/mozilla/markfluence` (`go 1.25`). `main.go` is a shim to `cmd - `cmd/root.go` — the cobra root: `--url`/`--username`/`--debug`/`--no-color` persistent flags, version from `internal/buildinfo`, and registration of the four subcommands. `Execute()` prints cobra-generated errors (bad args/flags) but not `ui.ErrSilent`, which marks a failure a command already reported. - `cmd/{update,create,fix,info}/` — one package per command (each exports `Cmd`), orchestrating the `internal` packages and `internal/ui` output. `create` is two-phase and transactional (validate all, then create parents-first in topological order); `fix` is read-only on the server. -- `internal/client` — `ConfluenceClient` over `net/http` with basic auth. Built from a `Config` (site URL, cloud ID, username, token) via `New`; it carries **two bases**: `BaseURL()` is where requests go (the gateway when a cloud ID is set) and `SiteURL()` is always the site. Anything a reader sees uses `SiteURL()` — printed page URLs and, critically, the `baseURL` handed to `convert.MdToConfluence`, since rewritten links are published *into* the page. Pages are Confluence **v2**; attachment writes and the user lookup are **v1** (`/wiki/rest/api/...`). Typed `HTTPError`, per-attempt context timeouts, centralized retry/backoff in `send` (429 for any method honoring `Retry-After`, plus 502/503/504 and network errors for idempotent methods only; exponential backoff capped), `SetContentProperty` retry-once on top (recovers a lost create-POST response), `SyncAttachments` (SHA-256-in-comment skip/update), `_links.next` pagination. `config.go` holds `Resolve` and the `.env` reader. -- `internal/convert` — the converter (the crux). `MdToConfluence(md *frontmatter.MarkdownFile, baseURL, spaceKey, version string) (*ConfluencePage, error)`. It parses with goldmark (GFM) and renders through a custom `storageRenderer` registered at priority 100 (below the default HTML=1000 and table=500 renderers) that emits Confluence storage format. `shield.go` renames raw `ac:`/`ri:` tags to colon-free sentinels around the goldmark step so pasted storage passes through; `callouts.go` is an AST transformer + blockquote renderer for GitHub alerts; `images.go`, `links.go` (sibling-file scans, GitHub/Confluence slugs, doc-link + anchor rewriting), `tables.go` (the `` tag, stamped with Confluence's `data-layout="align-start"` so tables auto-size to their content and left-align, plus cells: an AST transformer consumes a leading `` comment in a cell and `renderTableCell` emits it as `data-highlight-colour` — rows still fall through to the GFM renderer), and `renderer.go` (code macros, text soft-break→space, images, links) do the rest. The `` and `` token substitutions happen **inside** `MdToConfluence`. +- `internal/client` — `ConfluenceClient` over `net/http` with basic auth. Built from a `Config` (site URL, cloud ID, username, token) via `New`; it carries **two bases**: `BaseURL()` is where requests go (the gateway when a cloud ID is set) and `SiteURL()` is always the site. Anything a reader sees uses `SiteURL()` — printed page URLs and, critically, the `baseURL` handed to `convert.MdToConfluence`, since rewritten links are published *into* the page. Pages are Confluence **v2**; attachment writes and the user lookup are **v1** (`/wiki/rest/api/...`). Typed `HTTPError`, per-attempt context timeouts, centralized retry/backoff in `send` (429 for any method honoring `Retry-After`, plus 502/503/504 and network errors for idempotent methods only; exponential backoff capped), `SetContentProperty` retry-once on top (recovers a lost create-POST response), `SyncAttachments` (skip/update by a SHA-256 recorded in the attachment's comment, alongside the source path so `read` recovers image paths exactly; the comment is `markfluence: sha256=… path=…`, and the legacy `mzcld:checksum: …` form is still parsed — the skip test compares the *parsed* checksum, so the format change doesn't force a re-upload), `_links.next` pagination. `config.go` holds `Resolve` and the `.env` reader. +- `internal/convert` — the converter (the crux). `MdToConfluence(md *frontmatter.MarkdownFile, baseURL, spaceKey, version string) (*ConfluencePage, error)`. It parses with goldmark (GFM) and renders through a custom `storageRenderer` registered at priority 100 (below the default HTML=1000 and table=500 renderers) that emits Confluence storage format. `shield.go` renames raw `ac:`/`ri:` tags to colon-free sentinels around the goldmark step so pasted storage passes through; `callouts.go` is an AST transformer + blockquote renderer for GitHub alerts; `attachname.go` owns the source-path↔attachment-name mapping (percent-encoding `%`→`%25` then `/`→`%2F`, which is **bijective** — that is what makes the dedupe collision-free and lets `read` recover an image's original path; decode refuses an absolute result, which markfluence never produces); `images.go` (resolution stays page-relative like GitHub; the documentation root — cwd — bounds what may be published, and an image above it is `IMAGE BROKEN`), `links.go` (sibling-file scans, GitHub/Confluence slugs, doc-link + anchor rewriting), `tables.go` (the `
` tag, stamped with Confluence's `data-layout="align-start"` so tables auto-size to their content and left-align, plus cells: an AST transformer consumes a leading `` comment in a cell and `renderTableCell` emits it as `data-highlight-colour` — rows still fall through to the GFM renderer), and `renderer.go` (code macros, text soft-break→space, images, links) do the rest. The `` and `` token substitutions happen **inside** `MdToConfluence`. - `internal/frontmatter` — flat YAML frontmatter parse/quote/`UpdateField`, and the `MarkdownFile` type (`Parse`/`ParseFile`, exported `Filename`/`Content`/`Frontmatter`/`Body`, and `Title`/`PageID`/`Space`/`Parent` accessors that normalize missing/blank/`"null"`). - `internal/pagewidth` — the `page_width` `Width` enum (`narrow`/`wide`/`max`, default `max`), `Declared`, the vocab↔content-property maps, `WidthFromProperties`, and `Apply`/`Read` against the client. - `internal/buildinfo` — `Version` (set via ldflags), `CommitDate` (from the `vcs.time` build setting), and `Stamp`. diff --git a/README.md b/README.md index a2be604..30cdf89 100644 --- a/README.md +++ b/README.md @@ -502,6 +502,34 @@ Example: references a remote URL); a missing/unsupported image becomes `IMAGE BROKEN: …` text. +Image paths resolve relative to the Markdown file, the same way they do when you +view the file on GitHub, so a page in a subdirectory can share an asset +directory above it: + +``` +docs/ ← run markfluence from here + assets/logo.png + guide/page.md → ![logo](../assets/logo.png) +``` + +**Run markfluence from the root of your documentation tree.** That root bounds +which images may be published: an image resolving outside it (`../../secrets/x.png`) +is reported as `IMAGE BROKEN: … (outside the documentation root)` rather than +uploaded. + +Confluence attachment names cannot contain `/`, so the path is percent-encoded +into the attachment name — `assets/logo.png` is attached as `assets%2Flogo.png`, +and `../assets/logo.png` as `..%2Fassets%2Flogo.png`. The encoding is reversible, +so `markfluence read` restores an image's original path instead of a flattened +one. markfluence also records the source path in the attachment's comment, which +it prefers over decoding the name. + +> [!NOTE] +> Pages published before this encoding existed used `/` → `_`. Republishing such +> a page uploads the image under its new name and updates the page to match, but +> the old attachment stays behind, unreferenced — markfluence never deletes. +> Remove those manually if the clutter bothers you. + Extra properties ride in the title as JSON: ```markdown diff --git a/_plans/018_attachment-name-encoding.md b/_plans/018_attachment-name-encoding.md new file mode 100644 index 0000000..a55e4d9 --- /dev/null +++ b/_plans/018_attachment-name-encoding.md @@ -0,0 +1,275 @@ +# Plan: attachment names that round-trip + +Make the mapping between a markdown image's source path and its Confluence +attachment name **bijective**, and record the source path on the attachment, so +that publishing a page and reading it back recovers the image's original +location instead of a flattened approximation. + +Confluence attachment names cannot contain `/`, so a path has to be flattened +into a name. Today that is one line (`internal/convert/images.go:147`): + +```go +// attachmentFilename derives a stable, collision-free attachment name from an +// image path: a leading "./" is dropped and "/" becomes "_" so images from +// different directories don't collide. +func attachmentFilename(src string) string { + name := strings.TrimPrefix(src, "./") + return strings.ReplaceAll(name, "/", "_") +} +``` + +There is no inverse: `renderImage` (`internal/convert/storage_to_md.go:377`) +uses `ri:filename` verbatim as the markdown `src`. That produces two defects. + +**The "collision-free" claim is false.** `a/b.png` and `a_b.png` both flatten to +`a_b.png`. Because the encoded name is also the dedupe key +(`internal/convert/images.go:59`), the *second* file is silently skipped: one set +of bytes is uploaded and both images on the page resolve to it. `_` is a bad +separator precisely because `_` is common in filenames. + +**Directory structure is unrecoverable.** The publish → `read` → publish cycle is +a stable fixed point, but a lossy one: + +``` +docs/img.png → publish → docs_img.png → read → ![](docs_img.png) → publish → docs_img.png +``` + +The directory is gone and the read-back `src` names a file that does not exist on +disk. This is what makes #37 (`export`) incoherent — export cannot lay +attachments out so the markdown's image links resolve. #37 calls this +reconciliation its crux. + +## Out of scope (deliberately) + +- **Clamping `..` when writing files.** A decoded `../assets/logo.png` is + legitimate (see the base-directory decision below), and `read` only prints + text, so an accurate path is the right output. Refusing to write outside a + destination directory belongs to #37, at the point export actually creates + files — which it must do for hand-uploaded attachments regardless. +- **Warning about orphaned attachments.** Detecting one means plumbing a legacy + name through `convert.Attachment` → `client.LocalAttachment` → + `planAttachments` for a one-time cosmetic concern. #9's `attachment-list` will + let users see every attachment on a page and remove orphans directly. +- **#9 (attachment subcommands).** Paused for this: a standalone + `attachment-upload` needs settled guidance on what remote name to use. + +## Decisions locked + +### Percent-encode, because the encoding must escape its own escape character + +`%` → `%25`, then `/` → `%2F`. Decode reverses it: `%2F` → `/` first, `%25` → `%` +last, so the `%25` output is not rescanned — which is exactly how a literal +`%2F` in a source path (encoded `%252F`) round-trips instead of collapsing into a +separator. + +| source path | `_` (today) | `%2F` | +|---|---|---| +| `assets/x.png` | `assets_x.png` | `assets%2Fx.png` | +| `a/b.png` | `a_b.png` ⚠️ | `a%2Fb.png` | +| `a_b.png` | `a_b.png` ⚠️ collides with the above | `a_b.png` | +| `__a.png` | `__a.png` | `__a.png` | +| `a_/b.png` | `a__b.png` | `a_%2Fb.png` | +| literal `a%2Fb.png` | `a%2Fb.png` | `a%252Fb.png` | + +Because the encoding is injective, **the collision defect is fixed by +construction**: two distinct sources can no longer produce one name, the dedupe +becomes sound, and the doc comment's claim becomes true. + +**Rejected: `/` → `__`.** The obvious fix, and wrong. It is a substitution, not +an escaping scheme — it never escapes its own delimiter, so it cannot be +bijective: + +| source | `__` encoding | decodes to | +|---|---|---| +| `__a.png` | `__a.png` | `/a.png` — **absolute path at filesystem root** | +| `a_/b.png` | `a___b.png` | `a/_b.png` — wrong | +| literal `a__b.png` | `a__b.png` | `a/b.png` — wrong | + +The first is disqualifying: #37's export would write to `/a.png`. + +**Rejected: escaped underscore** (`_` → `__`, then `/` → `_s`). Equally bijective +and certain to be accepted by any API, but produces names like +`docs_sguide_simg.png`. Kept as the fallback if percent-encoding had failed the +live probe. + +**Rejected: fullwidth solidus `／`** (U+FF0F). Renders almost identically to a +slash in Confluence's UI, which is genuinely attractive, but it is a +confusable-character hazard — users copy the name and get a character that is not +a slash — plus non-ASCII filename risk on upload and on export to disk. + +### Names are page-anchored; the documentation root bounds what may be published + +Image *resolution* stays page-relative — `filepath.Join(baseDir, src)` where +`baseDir` is the markdown file's directory — unchanged, and the same thing GitHub +does rendering a repo's markdown. + +Two separable questions follow: what bounds the "too far out" check, and what the +name is anchored to. With cwd `docs/` and page `docs/guide/foo.md`: + +| src | resolves to | A: root-anchored | **B: page-anchored, root-bounded** | C: page-anchored, page-bounded | +|---|---|---|---|---| +| `image1.png` | `docs/guide/image1.png` | `guide%2Fimage1.png` | `image1.png` | `image1.png` | +| `sub/deep.png` | `docs/guide/sub/deep.png` | `guide%2Fsub%2Fdeep.png` | `sub%2Fdeep.png` | `sub%2Fdeep.png` | +| `../assets/logo.png` | `docs/assets/logo.png` | `assets%2Flogo.png` | `..%2Fassets%2Flogo.png` | ❌ BROKEN | +| `../../outside.png` | `outside.png` | ❌ BROKEN | ❌ BROKEN | ❌ BROKEN | + +**B is chosen**, because it is how these files behave viewed on GitHub: the path +the author wrote is the truth, and a shared asset directory above the page works. + +- The name derives from the `src`, so it **never depends on which directory + markfluence was invoked from** — no `--root` flag is needed, and cwd only + affects the boundary check, not naming. +- A is the only model where `..` can never appear in a name, which would let + decode refuse both absolute and escaping paths. Its price is that the name + depends on the invocation directory: run from `docs/guide/` instead of `docs/` + and the same image gets a different name, silently orphaning the old + attachment — the exact migration pain this work exists to stop repeating. +- C keeps stable names *and* the clean invariant, but bans shared asset + directories outright, forcing assets to be duplicated under each page's + directory. + +markfluence is expected to be run from the root of the documentation tree. An +image resolving outside it is `IMAGE BROKEN: … (outside the documentation root)`, +consistent with the existing broken-image handling. The check fails open when cwd +is unknown: it is an authoring guard, not a security boundary. + +### Decode refuses absolute paths only + +The encode side normalizes (`path.Clean`, strip a leading `/`) so an absolute path +is never produced. On decode, an absolute result therefore proves the attachment +did not come from markfluence — refuse it and fall back to the raw attachment +name. + +`..` is deliberately **not** refused: under model B we produce it legitimately. + +No encoding can fix the underlying ambiguity for foreign attachments — a +hand-uploaded `%2Fetc%2Fpasswd.png` decodes to `/etc/passwd.png` under *any* +scheme, because there is no way to distinguish a name we wrote from one a human +typed. That is why the guard lives on the decode side rather than in the +encoding. + +### The comment carries the source path + +Decoding a name is inference; the comment is truth, and markfluence already +writes one on every attachment it uploads. New format, retiring the dead `mzcld` +name (markfluence has not been that tool for a long time, and #9's +`attachment-list` would surface the string to users): + +``` +legacy: mzcld:checksum: ab12cd… +new: markfluence: sha256=ab12cd… path=assets/x.png +``` + +`path=` is written last and unquoted so a source path may contain spaces. + +**Both forms are parsed, and skip/update compares the parsed `sha256` rather than +the raw comment string.** Without that, changing the format would force a +needless re-upload of every attachment whose name did not change; with it, an +unchanged root-level image keeps its legacy comment and is still correctly +skipped. The path is stamped the next time the file actually changes. + +This also gives #9's `attachment-list` a real signal for whether an attachment is +markfluence-managed. + +### `StorageToMarkdown` takes a sources map + +It is a pure function over a storage string — no client, no page id — so it +cannot look up comments itself: + +```go +func StorageToMarkdown(storage string, sources map[string]string) (string, error) +// sources maps attachment name → source path; nil falls back to decoding names. +``` + +`read` builds the map from `ListAttachments`, but **only when the body actually +contains `ri:attachment`**, and a failed listing passes `nil` rather than failing +the read — the same tolerance `read` already applies to page width +(`cmd/read/read.go:142`). There is exactly one production caller +(`cmd/read/read.go:85`), so the signature change is cheap. + +Threading the map requires converting the render chain (`blockStrings`, +`renderBlock`, `renderList`, `renderListItem`, `renderTable`, `cellTexts`, +`renderMacro`, `renderCallout`, `renderInlineChildren`, `renderInline`, +`renderLink`, `renderImage`, `renderRawBlock`) into methods on a small +`mdRenderer`, mirroring the forward direction's `storageRenderer`. + +**Rejected: rewriting `ri:filename` in the parsed tree** after `parseStorage`, +which would need no signature changes at all. It would corrupt attachment +references inside unknown macros, which pass through as raw storage and are +re-serialized verbatim — writing a decoded path back into published content. + +## Migration: orphaned attachments + +markfluence never deletes. For every already-published page with a subdirectory +image, the next `update` uploads under the new name, rewrites the body to +reference it, and leaves the old `assets_x.png` attached but unreferenced. +Nothing breaks and no data is lost, but the cruft is permanent and accumulates +with every page published under the old scheme before this lands — which is an +argument for doing it sooner rather than later. + +## Verified against the live API + +Atlassian documents none of the attachment-name rules, so percent-encoding was +**probed before any code was written**; the fallback was escaped-underscore. + +- **`%` is stored verbatim.** Uploaded as `probe%2Fsub%2Fx.png`; both the create + response and a later `GET .../child/attachment` return the title unchanged — + not rejected, not stripped, not normalized back to a slash. +- **`ri:filename` matching is literal.** Rendered non-destructively via + `POST /wiki/rest/api/contentbody/convert/view?contentIdContext=`: the + percent-encoded name resolves to a real `confluence-embedded-image`, + structurally identical to an underscore-named control, while a nonexistent + attachment renders the `unknown-attachment` placeholder — so the test + discriminates. The rendered `src` is **double-encoded** (`%252F`), i.e. + Confluence treats `%2F` as literal filename characters and escapes them + correctly. +- **The comment format survives** verbatim under `metadata.comment`. +- **Download works**: `{base}/wiki` + `_links.download` → 200, bytes identical. + +End-to-end after implementation, against a real page: `assets/markfluence-test.png` +published as `assets%2Fmarkfluence-test.png`, the page renders 3 embedded images +with 0 placeholders, `read` returned `![…](assets/markfluence-test.png)` with the +directory recovered, and a re-run skips. + +Incidental findings, recorded for #9: `expand=extensions` yields `fileSize` and +`mediaType`; `version.number` is available; the collection carries +`size`/`limit`/`start` and **omits `_links.next` when results fit one page**, so +`start`/`limit` offset pagination is the reliable approach; `_links.download` has +the form `/rest/api/content/{pageId}/child/attachment/{attId}/download`. + +## Testing + +- `internal/convert/attachname_test.go`: round-trip over every edge case above — + the three `__` failures, the escape character itself, spaces, `../` — plus an + injectivity test (the property the dedupe depends on), normalization of + equivalent spellings, and refusal of names decoding to absolute paths. +- Regression cases: `images-shared-parent` (a page in a subdirectory using + `../assets`, proving the supported layout publishes as `..%2F…`) and a third + entry in `images-broken` for an image above the root. +- `internal/convert/storage_to_md_test.go`: a recorded source overrides a decoded + name; an absolute recorded source is refused. +- `internal/client/client_test.go`: comment construction and parsing of both + forms, that an upload stamps `path=`, and skip-on-match for the new format. The + **existing** skip/update tests are repointed at `legacyChecksumPrefix`, so they + become the legacy-tolerance coverage. +- Goldens: `make regen-regressions`, plus the `storage2md/images` fixtures, whose + `input.storage` moves to the new scheme so `output.md` demonstrates the decode. + +## Docs + +- `README.md`: images section — resolution is page-relative like GitHub, run from + the documentation root, the encoding itself with worked examples, and a note + that republishing leaves the old underscore-named attachment behind. +- `CLAUDE.md`: `attachname.go` in the converter breakdown, the root-bounding rule + in `images.go`, and the comment format plus tolerant-parsing rationale on + `SyncAttachments`. + +## Commits + +1. `feat(convert): percent-encode image paths into attachment names` — encoding, + normalization, root bounding, `Attachment.Source`, tests. +2. `feat(convert): recover an image's original path when reading a page` — decode, + `sources` param, `mdRenderer`, `read` plumbing, goldens. +3. `feat(client): record an attachment's source path in its comment` — comment + format, tolerant parsing, compare on parsed sha. +4. `docs: document attachment name encoding and the documentation root`. diff --git a/cmd/create/create.go b/cmd/create/create.go index 696033f..fde0f3a 100644 --- a/cmd/create/create.go +++ b/cmd/create/create.go @@ -499,7 +499,7 @@ func pageURL(c *client.ConfluenceClient, page *client.Page, pageID string) strin func toLocalAttachments(atts []convert.Attachment) []client.LocalAttachment { out := make([]client.LocalAttachment, len(atts)) for i, a := range atts { - out[i] = client.LocalAttachment{Path: a.Path, Filename: a.Filename} + out[i] = client.LocalAttachment{Path: a.Path, Filename: a.Filename, Source: a.Source} } return out } diff --git a/cmd/read/read.go b/cmd/read/read.go index b9fade9..73ccc3d 100644 --- a/cmd/read/read.go +++ b/cmd/read/read.go @@ -82,7 +82,7 @@ func run(cmd *cobra.Command, args []string) error { body := page.Body.Storage.Value if formatFlag == formatMarkdown { - body, err = convert.StorageToMarkdown(page.Body.Storage.Value) + body, err = convert.StorageToMarkdown(page.Body.Storage.Value, attachmentSources(c, page)) if err != nil { return operationalFail(pageID, err, jsonout.CodeConvert) } @@ -102,6 +102,32 @@ func run(cmd *cobra.Command, args []string) error { return nil } +// attachmentSources maps each attachment name on the page to the markdown image +// path it was published from, letting the converter restore an image's original +// location exactly rather than inferring it from the attachment name. +// +// It is an optimization, not a requirement: a page with no attachment references +// skips the lookup entirely, and a failed lookup returns nil so the converter +// falls back to decoding names -- a read is worth completing without it, the +// same way a failed page-width read is tolerated below. +func attachmentSources(c *client.ConfluenceClient, page *client.Page) map[string]string { + if !strings.Contains(page.Body.Storage.Value, " +// path=") and the legacy checksum-only form, so an attachment written by +// an older markfluence is still recognized as unchanged. +func parseAttachmentComment(comment string) AttachmentMeta { + if sum, ok := strings.CutPrefix(comment, legacyChecksumPrefix); ok { + return AttachmentMeta{SHA256: strings.TrimSpace(sum), Managed: true} + } + rest, ok := strings.CutPrefix(comment, attachmentCommentPrefix) + if !ok { + return AttachmentMeta{} + } + m := AttachmentMeta{Managed: true} + if sum, after, found := strings.Cut(strings.TrimPrefix(rest, "sha256="), " "); found { + m.SHA256, rest = sum, after + } else { + m.SHA256, rest = sum, "" + } + // path= is last, so its value is the remainder verbatim. + if src, ok := strings.CutPrefix(rest, "path="); ok { + m.Source = src + } + return m } // SyncAction reports what sync_attachments did for one file. @@ -539,7 +597,7 @@ func (c *ConfluenceClient) planAttachments(pageID string, attachments []LocalAtt if err != nil { return nil, err } - comment := attachmentChecksumPrefix + sum + comment := attachmentComment(sum, att.Source) contentType := mime.TypeByExtension(filepath.Ext(att.Filename)) if contentType == "" { contentType = "application/octet-stream" @@ -549,7 +607,10 @@ func (c *ConfluenceClient) planAttachments(pageID string, attachments []LocalAtt switch { case !ok: p.action = "created" - case cur.Metadata.Comment == comment: + case cur.Meta().SHA256 == sum: + // Compare the checksum, not the whole comment: an attachment stamped + // by an older markfluence is unchanged and must not be re-uploaded + // merely because the comment format has moved on. p.action = "skipped" default: p.action = "updated" diff --git a/internal/client/client_test.go b/internal/client/client_test.go index 7435afd..39d8f76 100644 --- a/internal/client/client_test.go +++ b/internal/client/client_test.go @@ -1,10 +1,12 @@ package client import ( + "io" "net/http" "net/http/httptest" "os" "path/filepath" + "strings" "sync/atomic" "testing" "time" @@ -20,9 +22,19 @@ func TestMain(m *testing.M) { type scripted struct { responses []resp calls []string + bodies []string idx int } +// lastBody returns the body of the most recent request, for assertions about +// what was actually sent. +func (s *scripted) lastBody() string { + if len(s.bodies) == 0 { + return "" + } + return s.bodies[len(s.bodies)-1] +} + type resp struct { status int body string @@ -33,6 +45,8 @@ func newServer(t *testing.T, responses ...resp) (*ConfluenceClient, *scripted) { s := &scripted{responses: responses} srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { s.calls = append(s.calls, r.Method) + body, _ := io.ReadAll(r.Body) + s.bodies = append(s.bodies, string(body)) if s.idx >= len(s.responses) { t.Errorf("unexpected extra request: %s %s", r.Method, r.URL.Path) w.WriteHeader(500) @@ -345,10 +359,12 @@ func TestSyncAttachmentsCreatesWhenAbsent(t *testing.T) { } } -func TestSyncAttachmentsSkipsWhenChecksumMatches(t *testing.T) { +// A legacy comment still identifies an unchanged file, so a format change does +// not force a re-upload of every attachment. +func TestSyncAttachmentsSkipsWhenLegacyChecksumMatches(t *testing.T) { path, sum := writeTempImage(t) list := `{"results":[{"id":"a1","title":"x.png","metadata":{"comment":"` + - attachmentChecksumPrefix + sum + `"}}]}` + legacyChecksumPrefix + sum + `"}}]}` c, s := newServer(t, resp{200, list}) actions, err := c.SyncAttachments("1", []LocalAttachment{{Path: path, Filename: "x.png"}}) if err != nil { @@ -365,7 +381,7 @@ func TestSyncAttachmentsSkipsWhenChecksumMatches(t *testing.T) { func TestSyncAttachmentsUpdatesWhenChecksumDiffers(t *testing.T) { path, _ := writeTempImage(t) list := `{"results":[{"id":"a1","title":"x.png","metadata":{"comment":"` + - attachmentChecksumPrefix + `stale"}}]}` + legacyChecksumPrefix + `stale"}}]}` c, s := newServer(t, resp{200, list}, resp{200, `{}`}) actions, err := c.SyncAttachments("1", []LocalAttachment{{Path: path, Filename: "x.png"}}) if err != nil { @@ -383,8 +399,8 @@ func TestPlanAttachmentsClassifiesWithoutUploading(t *testing.T) { path, sum := writeTempImage(t) // same.png matches (skip), stale.png differs (update), new.png is absent (create). list := `{"results":[` + - `{"id":"a1","title":"same.png","metadata":{"comment":"` + attachmentChecksumPrefix + sum + `"}},` + - `{"id":"a2","title":"stale.png","metadata":{"comment":"` + attachmentChecksumPrefix + `stale"}}` + + `{"id":"a1","title":"same.png","metadata":{"comment":"` + legacyChecksumPrefix + sum + `"}},` + + `{"id":"a2","title":"stale.png","metadata":{"comment":"` + legacyChecksumPrefix + `stale"}}` + `]}` c, s := newServer(t, resp{200, list}) actions, err := c.PlanAttachments("1", []LocalAttachment{ @@ -573,3 +589,80 @@ func TestResolveNext(t *testing.T) { }) } } + +// TestAttachmentCommentRecordsSource checks the comment an upload stamps: the +// checksum used to skip unchanged files, plus the path the image was published +// from so a later read recovers it exactly. +func TestAttachmentCommentRecordsSource(t *testing.T) { + got := attachmentComment("abc123", "../assets/logo.png") + if want := "markfluence: sha256=abc123 path=../assets/logo.png"; got != want { + t.Errorf("attachmentComment = %q, want %q", got, want) + } + if got := attachmentComment("abc123", ""); got != "markfluence: sha256=abc123" { + t.Errorf("attachmentComment without a source = %q", got) + } +} + +func TestParseAttachmentComment(t *testing.T) { + cases := []struct { + name string + comment string + want AttachmentMeta + }{ + {"current form", "markfluence: sha256=abc123 path=assets/x.png", + AttachmentMeta{SHA256: "abc123", Source: "assets/x.png", Managed: true}}, + {"no source recorded", "markfluence: sha256=abc123", + AttachmentMeta{SHA256: "abc123", Managed: true}}, + // The path is written last and unquoted, so it may contain spaces. + {"source with spaces", "markfluence: sha256=abc123 path=my docs/a b.png", + AttachmentMeta{SHA256: "abc123", Source: "my docs/a b.png", Managed: true}}, + {"legacy form", legacyChecksumPrefix + "abc123", + AttachmentMeta{SHA256: "abc123", Managed: true}}, + {"hand-uploaded", "a note from a human", AttachmentMeta{}}, + {"empty", "", AttachmentMeta{}}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := parseAttachmentComment(c.comment); got != c.want { + t.Errorf("parseAttachmentComment(%q) = %+v, want %+v", c.comment, got, c.want) + } + }) + } +} + +// TestSyncAttachmentsStampsSource checks that an upload records the source path, +// so reading the page back can recover the image's original location. +func TestSyncAttachmentsStampsSource(t *testing.T) { + path, _ := writeTempImage(t) + c, s := newServer(t, resp{200, `{"results":[]}`}, resp{200, `{}`}) + _, err := c.SyncAttachments("1", []LocalAttachment{ + {Path: path, Filename: "assets%2Fx.png", Source: "assets/x.png"}, + }) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(s.lastBody(), "path=assets/x.png") { + t.Errorf("upload body did not record the source path:\n%s", s.lastBody()) + } +} + +// TestSyncAttachmentsSkipsWhenCurrentChecksumMatches is the new-format twin of the +// legacy skip test. +func TestSyncAttachmentsSkipsWhenCurrentChecksumMatches(t *testing.T) { + path, sum := writeTempImage(t) + list := `{"results":[{"id":"a1","title":"x.png","metadata":{"comment":"` + + attachmentComment(sum, "x.png") + `"}}]}` + c, s := newServer(t, resp{200, list}) + actions, err := c.SyncAttachments("1", []LocalAttachment{ + {Path: path, Filename: "x.png", Source: "x.png"}, + }) + if err != nil { + t.Fatal(err) + } + if len(actions) != 1 || actions[0].Action != "skipped" { + t.Fatalf("actions = %v, want [skipped]", actions) + } + if !eqStrings(s.calls, []string{"GET"}) { + t.Errorf("calls = %v, want [GET] (no upload)", s.calls) + } +} diff --git a/internal/convert/attachname.go b/internal/convert/attachname.go new file mode 100644 index 0000000..ec398ff --- /dev/null +++ b/internal/convert/attachname.go @@ -0,0 +1,73 @@ +package convert + +// attachname.go owns the mapping between a markdown image's source path and the +// Confluence attachment name it is published under. +// +// "/" is not legal in an attachment name, so the path is flattened. The encoding +// escapes its own escape character, which makes it bijective: distinct source +// paths always produce distinct names, and every name markfluence produces +// decodes back to the exact path it came from. That is what lets `read` +// reconstruct an image's original location, and it is why two images can never +// silently collide on one attachment name. +// +// Confluence stores these names verbatim and matches ri:filename literally -- +// verified against Cloud, where "a%2Fb.png" resolves and renders with the name +// re-escaped as "a%252Fb.png" in the image URL. + +import ( + "path" + "strings" +) + +const ( + pctEscape = "%25" // a literal "%" in the source path + pctSlash = "%2F" // a "/" path separator +) + +// attachmentFilename derives the Confluence attachment name for a markdown image +// src. The src is normalized first so a name can never decode to an absolute +// path, then "%" is encoded before "/" -- in that order, so the escapes +// introduced for "/" are not themselves escaped. +func attachmentFilename(src string) string { + rel := normalizeSrc(src) + rel = strings.ReplaceAll(rel, "%", pctEscape) + return strings.ReplaceAll(rel, "/", pctSlash) +} + +// attachmentSource inverts attachmentFilename, recovering the source path an +// attachment was published from. It reports false when the name could not have +// come from markfluence -- currently when it decodes to an empty or absolute +// path, which attachmentFilename never produces -- so callers fall back to +// treating the attachment name as the path. +// +// A name markfluence did not create is decoded on a best-effort basis: there is +// no way to tell a hand-uploaded "a%2Fb.png" from one we published. +func attachmentSource(filename string) (string, bool) { + // Decode "%2F" first and "%25" last. Replacing "%2F" only ever removes text + // and cannot spell a new "%25", while "%25" must be replaced last precisely + // so its output is not rescanned -- that is how a literal "%2F" in the source + // path (encoded "%252F") round-trips instead of collapsing to a separator. + s := strings.ReplaceAll(filename, pctSlash, "/") + s = strings.ReplaceAll(s, pctEscape, "%") + if s == "" || path.IsAbs(s) { + return "", false + } + return s, true +} + +// normalizeSrc reduces a markdown image src to a clean relative path: "./a/x.png" +// and "a/./x.png" both become "a/x.png". A leading "/" is dropped because image +// resolution joins src onto the page's directory anyway, so an absolute-looking +// src was never actually absolute -- and dropping it keeps attachmentFilename +// from ever producing a name that decodes to an absolute path. +// +// A ".." prefix is preserved: an image in a shared directory above the page +// ("../assets/logo.png") is a supported layout, the same as it would be viewing +// the file on GitHub. +func normalizeSrc(src string) string { + s := strings.TrimPrefix(path.Clean(src), "/") + if s == "." { + return "" + } + return s +} diff --git a/internal/convert/attachname_test.go b/internal/convert/attachname_test.go new file mode 100644 index 0000000..219ef7b --- /dev/null +++ b/internal/convert/attachname_test.go @@ -0,0 +1,114 @@ +package convert + +import "testing" + +// TestAttachmentNameRoundTrip is the core guarantee: every source path encodes to +// an attachment name that decodes back to exactly that path. The cases include +// the ones a naive "/" -> "__" substitution gets wrong. +func TestAttachmentNameRoundTrip(t *testing.T) { + cases := []struct { + src string + name string + }{ + {"x.png", "x.png"}, + {"assets/x.png", "assets%2Fx.png"}, + {"a/b/c/deep.png", "a%2Fb%2Fc%2Fdeep.png"}, + + // "_" is ordinary text, so a path and a name that merely looks flattened + // stay distinct -- the collision the old "/" -> "_" encoding produced. + {"a/b.png", "a%2Fb.png"}, + {"a_b.png", "a_b.png"}, + + // A leading "__" must not decode to an absolute path. + {"__a.png", "__a.png"}, + // A component ending in "_" must not shift the separator. + {"a_/b.png", "a_%2Fb.png"}, + // A literal "%2F" in the filename must not decode to a separator. + {"a%2Fb.png", "a%252Fb.png"}, + // The escape character itself. + {"100%.png", "100%25.png"}, + {"%25.png", "%2525.png"}, + + // A shared asset above the page is a supported layout. + {"../assets/logo.png", "..%2Fassets%2Flogo.png"}, + + // Spaces and other characters are left alone -- only "/" is illegal. + {"my docs/a b.png", "my docs%2Fa b.png"}, + } + for _, c := range cases { + if got := attachmentFilename(c.src); got != c.name { + t.Errorf("attachmentFilename(%q) = %q, want %q", c.src, got, c.name) + } + got, ok := attachmentSource(c.name) + if !ok { + t.Errorf("attachmentSource(%q) refused a name we produced", c.name) + continue + } + if got != c.src { + t.Errorf("attachmentSource(%q) = %q, want %q (round trip)", c.name, got, c.src) + } + } +} + +// TestAttachmentFilenameIsInjective is what makes the dedupe in renderImage sound: +// no two distinct sources may share one attachment name. +func TestAttachmentFilenameIsInjective(t *testing.T) { + srcs := []string{ + "a/b.png", "a_b.png", "a__b.png", "a%2Fb.png", "__a.png", "a_/b.png", + "x.png", "assets/x.png", "../x.png", "100%.png", + } + seen := map[string]string{} + for _, src := range srcs { + name := attachmentFilename(src) + if prev, dup := seen[name]; dup { + t.Errorf("%q and %q both encode to %q", prev, src, name) + } + seen[name] = src + } +} + +// TestAttachmentFilenameNormalizes folds equivalent spellings of one path onto a +// single name, so the same file is not uploaded twice under different names. +func TestAttachmentFilenameNormalizes(t *testing.T) { + cases := []struct{ src, want string }{ + {"./x.png", "x.png"}, + {"./assets/x.png", "assets%2Fx.png"}, + {"assets/./x.png", "assets%2Fx.png"}, + {"assets/../assets/x.png", "assets%2Fx.png"}, + // Resolution joins src onto the page directory, so a leading "/" was never + // really absolute; dropping it keeps names from decoding to absolute paths. + {"/assets/x.png", "assets%2Fx.png"}, + } + for _, c := range cases { + if got := attachmentFilename(c.src); got != c.want { + t.Errorf("attachmentFilename(%q) = %q, want %q", c.src, got, c.want) + } + } +} + +// TestAttachmentSourceRefusesAbsolute covers names markfluence never produces: +// a hand-uploaded attachment must not be able to steer a reader at an absolute +// path (which is what #37's export would then write to). +func TestAttachmentSourceRefusesAbsolute(t *testing.T) { + for _, name := range []string{"%2Fetc%2Fpasswd.png", "%2F.png", ""} { + if got, ok := attachmentSource(name); ok { + t.Errorf("attachmentSource(%q) = %q, true; want refusal", name, got) + } + } +} + +// TestAttachmentSourceDecodesForeignNames documents best-effort behavior for +// attachments markfluence did not upload: they decode like any other name, since +// there is no way to tell them apart. +func TestAttachmentSourceDecodesForeignNames(t *testing.T) { + for _, c := range []struct{ name, want string }{ + {"hand-uploaded.png", "hand-uploaded.png"}, + {"screenshot 2026.png", "screenshot 2026.png"}, + {"..%2Fup.png", "../up.png"}, + } { + got, ok := attachmentSource(c.name) + if !ok || got != c.want { + t.Errorf("attachmentSource(%q) = %q, %v; want %q, true", c.name, got, ok, c.want) + } + } +} diff --git a/internal/convert/convert.go b/internal/convert/convert.go index 5faa4c8..c84221f 100644 --- a/internal/convert/convert.go +++ b/internal/convert/convert.go @@ -7,6 +7,7 @@ package convert import ( "bytes" + "os" "path/filepath" "strings" @@ -59,8 +60,16 @@ func MdToConfluence(md *frontmatter.MarkdownFile, baseURL, spaceKey, version str // escaping them; restore them after rendering. shielded, unshield := shieldStorage(md.Body) dir := filepath.Dir(md.Filename) + // The documentation root is the working directory: markfluence is run from + // the root of a documentation tree. An unresolvable cwd disables the check + // rather than failing the conversion. + root, err := os.Getwd() + if err != nil { + root = "" + } r := &storageRenderer{ baseDir: dir, + root: root, currentBasename: filepath.Base(md.Filename), baseURL: baseURL, spaceKey: spaceKey, diff --git a/internal/convert/images.go b/internal/convert/images.go index 707ef57..0f04189 100644 --- a/internal/convert/images.go +++ b/internal/convert/images.go @@ -49,6 +49,11 @@ func (r *storageRenderer) renderImage( r.broken = append(r.broken, msg) _, _ = w.WriteString(html.EscapeString(msg)) + case !r.withinRoot(filepath.Join(r.baseDir, src)): + msg := fmt.Sprintf("IMAGE BROKEN: %s (outside the documentation root)", src) + r.broken = append(r.broken, msg) + _, _ = w.WriteString(html.EscapeString(msg)) + case !isFile(filepath.Join(r.baseDir, src)): msg := fmt.Sprintf("IMAGE BROKEN: %s (not found)", src) r.broken = append(r.broken, msg) @@ -65,7 +70,9 @@ func (r *storageRenderer) renderImage( if err != nil { abs = filepath.Join(r.baseDir, src) } - r.attachments = append(r.attachments, Attachment{Filename: filename, Path: abs}) + r.attachments = append(r.attachments, Attachment{ + Filename: filename, Path: abs, Source: normalizeSrc(src), + }) } _, _ = w.WriteString(acImage(alt, attrs, filename, "")) } @@ -141,14 +148,6 @@ func acImage(alt string, attrs map[string]string, riFilename, riURL string) stri return fmt.Sprintf("%s", leading, resource) } -// attachmentFilename derives a stable, collision-free attachment name from an -// image path: a leading "./" is dropped and "/" becomes "_" so images from -// different directories don't collide. -func attachmentFilename(src string) string { - name := strings.TrimPrefix(src, "./") - return strings.ReplaceAll(name, "/", "_") -} - // nodeText returns the concatenated plain text of a node's descendants. func nodeText(n ast.Node, source []byte) string { var b strings.Builder @@ -178,6 +177,31 @@ func isFile(path string) bool { return err == nil && !info.IsDir() } +// withinRoot reports whether an image path resolves inside the documentation +// root. markfluence is meant to be run from the root of a documentation tree, so +// an image above it -- "../../../secrets/x.png" -- is a mistake rather than a +// shared asset, and is reported broken instead of published. +// +// A path at or below the root is fine, including one reached via ".." from a +// page in a subdirectory: "../assets/logo.png" from docs/guide/foo.md is the +// ordinary shared-assets layout. The check fails open when the root is unknown +// or a path cannot be resolved -- it is an authoring guard, not a security +// boundary. +func (r *storageRenderer) withinRoot(p string) bool { + if r.root == "" { + return true + } + abs, err := filepath.Abs(p) + if err != nil { + return true + } + rel, err := filepath.Rel(r.root, abs) + if err != nil { + return true + } + return rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) +} + func isDigits(s string) bool { if s == "" { return false diff --git a/internal/convert/page.go b/internal/convert/page.go index aacc16e..9efc2cc 100644 --- a/internal/convert/page.go +++ b/internal/convert/page.go @@ -15,8 +15,12 @@ type ConfluencePage struct { } // Attachment is a local image the body references, to be uploaded to the page. -// Path is absolute; Filename is the stable, collision-free attachment name. +// Path is absolute. Filename is the attachment name, a bijective encoding of +// Source, so distinct images can never collide on one name. Source is the +// normalized page-relative path the image was written as, recorded on the +// attachment so a later read recovers it exactly rather than inferring it. type Attachment struct { Filename string `json:"filename"` Path string `json:"path"` + Source string `json:"source"` } diff --git a/internal/convert/renderer.go b/internal/convert/renderer.go index 0fa4607..3188376 100644 --- a/internal/convert/renderer.go +++ b/internal/convert/renderer.go @@ -21,6 +21,10 @@ import ( type storageRenderer struct { baseDir string + // root bounds which images may be published: the documentation root, which + // markfluence is expected to be run from. Empty disables the check. + root string + // Link/anchor rewriting context, populated per conversion. currentBasename string baseURL string diff --git a/internal/convert/storage_to_md.go b/internal/convert/storage_to_md.go index 32d1b4f..a4057ce 100644 --- a/internal/convert/storage_to_md.go +++ b/internal/convert/storage_to_md.go @@ -14,6 +14,7 @@ import ( "encoding/xml" "fmt" "io" + "path" "sort" "strings" ) @@ -30,12 +31,19 @@ var calloutMacroInverse = map[string]string{ } // StorageToMarkdown converts a Confluence storage-format body to Markdown. -func StorageToMarkdown(storage string) (string, error) { +// +// sources maps an attachment name to the markdown image path it was published +// from, as recorded on the attachment when markfluence uploaded it. It is +// optional: a nil map (or a name missing from it) falls back to decoding the +// attachment name, which is exact for names markfluence created and a +// best-effort guess for hand-uploaded ones. +func StorageToMarkdown(storage string, sources map[string]string) (string, error) { root, err := parseStorage(storage) if err != nil { return "", err } - blocks := blockStrings(root.kids, "") + r := &mdRenderer{sources: sources} + blocks := r.blockStrings(root.kids, "") out := strings.Join(blocks, "\n\n") out = strings.Trim(out, "\n") if out == "" { @@ -44,6 +52,28 @@ func StorageToMarkdown(storage string) (string, error) { return out + "\n", nil } +// mdRenderer carries the per-conversion context the storage->markdown walk needs. +// A fresh one is used per conversion, so nothing leaks between documents. +type mdRenderer struct { + // sources maps attachment name -> the image path it was published from. May + // be nil, in which case paths are recovered by decoding attachment names. + sources map[string]string +} + +// sourceFor resolves an attachment name back to the markdown image path to write. +// The path recorded on the attachment wins because it is exact; otherwise the +// name is decoded. An absolute path is never something markfluence published, so +// it is refused in both cases and the raw attachment name is used instead. +func (r *mdRenderer) sourceFor(filename string) string { + if src, ok := r.sources[filename]; ok && src != "" && !path.IsAbs(src) { + return src + } + if src, ok := attachmentSource(filename); ok { + return src + } + return filename +} + // snode is a minimal parsed storage node: an element (name + attrs + children) or // a text node (name == ""). type snode struct { @@ -104,7 +134,7 @@ func qname(n xml.Name) string { // blockStrings renders block-level children to a slice of block strings (joined // by callers with blank lines). listIndent is the continuation indent applied to // nested lists. -func blockStrings(kids []*snode, listIndent string) []string { +func (r *mdRenderer) blockStrings(kids []*snode, listIndent string) []string { var out []string for _, k := range kids { if k.name == "" { @@ -113,7 +143,7 @@ func blockStrings(kids []*snode, listIndent string) []string { } continue } - if s := renderBlock(k, listIndent); s != "" { + if s := r.renderBlock(k, listIndent); s != "" { out = append(out, s) } } @@ -121,44 +151,44 @@ func blockStrings(kids []*snode, listIndent string) []string { } // renderBlock renders a single block-level element. -func renderBlock(n *snode, listIndent string) string { +func (r *mdRenderer) renderBlock(n *snode, listIndent string) string { switch n.name { case "h1", "h2", "h3", "h4", "h5", "h6": level := int(n.name[1] - '0') - return strings.Repeat("#", level) + " " + renderInlineChildren(n) + return strings.Repeat("#", level) + " " + r.renderInlineChildren(n) case "p": - return renderInlineChildren(n) + return r.renderInlineChildren(n) case "ul": - return renderList(n, false, listIndent) + return r.renderList(n, false, listIndent) case "ol": - return renderList(n, true, listIndent) + return r.renderList(n, true, listIndent) case "blockquote": - return prefixLines(strings.Join(blockStrings(n.kids, ""), "\n\n"), "> ") + return prefixLines(strings.Join(r.blockStrings(n.kids, ""), "\n\n"), "> ") case "hr": return "---" case "pre": return "```\n" + textContent(n) + "\n```" case "table": - return renderTable(n) + return r.renderTable(n) case "ac:structured-macro": - return renderMacro(n, true) + return r.renderMacro(n, true) case "ac:image", "a", "strong", "b", "em", "i", "code", "del", "s", "strike", "br": // An inline element sitting at block level (Confluence often emits a bare // not wrapped in

) is rendered as its own paragraph. - return renderInline(n) + return r.renderInline(n) case "ac:layout", "ac:layout-section", "ac:layout-cell": - return renderRawBlock(n) + return r.renderRawBlock(n) case "div": - return strings.Join(blockStrings(n.kids, listIndent), "\n\n") + return strings.Join(r.blockStrings(n.kids, listIndent), "\n\n") default: // Unknown element: render its children as blocks (transparent wrapper). - return strings.Join(blockStrings(n.kids, listIndent), "\n\n") + return strings.Join(r.blockStrings(n.kids, listIndent), "\n\n") } } // renderList renders a ul/ol, incrementing ordered markers and indenting nested // lists to align under their item text. -func renderList(n *snode, ordered bool, indent string) string { +func (r *mdRenderer) renderList(n *snode, ordered bool, indent string) string { var lines []string i := 0 for _, k := range n.kids { @@ -171,31 +201,31 @@ func renderList(n *snode, ordered bool, indent string) string { marker = fmt.Sprintf("%d. ", i) } cont := indent + strings.Repeat(" ", len(marker)) - lines = append(lines, indent+marker+renderListItem(k, cont)) + lines = append(lines, indent+marker+r.renderListItem(k, cont)) } return strings.Join(lines, "\n") } // renderListItem renders an

  • : its inline/paragraph content on the first line, // with any nested lists indented beneath it. -func renderListItem(li *snode, cont string) string { +func (r *mdRenderer) renderListItem(li *snode, cont string) string { var head strings.Builder var tail []string for _, k := range li.kids { switch k.name { case "ul": - tail = append(tail, renderList(k, false, cont)) + tail = append(tail, r.renderList(k, false, cont)) case "ol": - tail = append(tail, renderList(k, true, cont)) + tail = append(tail, r.renderList(k, true, cont)) case "p": - if s := renderInlineChildren(k); s != "" { + if s := r.renderInlineChildren(k); s != "" { if head.Len() > 0 { head.WriteString(" ") } head.WriteString(s) } default: - head.WriteString(renderInline(k)) + head.WriteString(r.renderInline(k)) } } item := strings.TrimSpace(head.String()) @@ -206,7 +236,7 @@ func renderListItem(li *snode, cont string) string { } // renderTable renders a table as a GFM pipe table. Alignment is not preserved. -func renderTable(n *snode) string { +func (r *mdRenderer) renderTable(n *snode) string { var rows []*snode var header *snode var walk func(*snode) @@ -232,7 +262,7 @@ func renderTable(n *snode) string { header, rows = rows[0], rows[1:] } - head := cellTexts(header) + head := r.cellTexts(header) var b strings.Builder b.WriteString("| " + strings.Join(head, " | ") + " |\n") seps := make([]string, len(head)) @@ -240,8 +270,8 @@ func renderTable(n *snode) string { seps[i] = "---" } b.WriteString("| " + strings.Join(seps, " | ") + " |") - for _, r := range rows { - b.WriteString("\n| " + strings.Join(cellTexts(r), " | ") + " |") + for _, row := range rows { + b.WriteString("\n| " + strings.Join(r.cellTexts(row), " | ") + " |") } return b.String() } @@ -257,11 +287,11 @@ func rowHasHeaderCell(tr *snode) bool { } // cellTexts renders a row's cells to inline strings with pipes escaped. -func cellTexts(tr *snode) []string { +func (r *mdRenderer) cellTexts(tr *snode) []string { var cells []string for _, c := range tr.kids { if c.name == "th" || c.name == "td" { - text := strings.ReplaceAll(renderInlineChildren(c), "|", `\|`) + text := strings.ReplaceAll(r.renderInlineChildren(c), "|", `\|`) cells = append(cells, text) } } @@ -273,16 +303,16 @@ func cellTexts(tr *snode) []string { // through as raw storage. A block-context unknown macro uses the round-trip-safe // multi-line form (renderRawBlock, markdown body); an inline one stays raw on a // single line so it does not break out of its paragraph. -func renderMacro(n *snode, block bool) string { +func (r *mdRenderer) renderMacro(n *snode, block bool) string { switch name := n.attrs["ac:name"]; { case name == "code": return renderCodeMacro(n) case name == "toc": return tocToken case calloutMacroInverse[name] != "": - return renderCallout(n, name) + return r.renderCallout(n, name) case block: - return renderRawBlock(n) + return r.renderRawBlock(n) default: return serialize(n) } @@ -303,10 +333,10 @@ func renderCodeMacro(n *snode) string { } // renderCallout renders a callout macro as a GitHub alert blockquote. -func renderCallout(n *snode, macro string) string { +func (r *mdRenderer) renderCallout(n *snode, macro string) string { content := "[!" + calloutMacroInverse[macro] + "]" if body := findChild(n, "ac:rich-text-body"); body != nil { - if inner := strings.Join(blockStrings(body.kids, ""), "\n\n"); inner != "" { + if inner := strings.Join(r.blockStrings(body.kids, ""), "\n\n"); inner != "" { content += "\n" + inner } } @@ -316,47 +346,47 @@ func renderCallout(n *snode, macro string) string { // --- inline rendering -------------------------------------------------------- // renderInlineChildren renders a node's children as a single inline string. -func renderInlineChildren(n *snode) string { +func (r *mdRenderer) renderInlineChildren(n *snode) string { var b strings.Builder for _, k := range n.kids { - b.WriteString(renderInline(k)) + b.WriteString(r.renderInline(k)) } return strings.TrimSpace(b.String()) } // renderInline renders one inline node. -func renderInline(n *snode) string { +func (r *mdRenderer) renderInline(n *snode) string { if n.name == "" { return collapse(n.text) } switch n.name { case "strong", "b": - return "**" + renderInlineChildren(n) + "**" + return "**" + r.renderInlineChildren(n) + "**" case "em", "i": - return "*" + renderInlineChildren(n) + "*" + return "*" + r.renderInlineChildren(n) + "*" case "code": return "`" + textContent(n) + "`" case "del", "s", "strike": - return "~~" + renderInlineChildren(n) + "~~" + return "~~" + r.renderInlineChildren(n) + "~~" case "br": return " \n" case "a": - return renderLink(n) + return r.renderLink(n) case "ac:image": - return renderImage(n) + return r.renderImage(n) case "ac:structured-macro": // An inline macro (e.g. status/emoticon) stays raw on one line so it does // not break out of its paragraph. - return renderMacro(n, false) + return r.renderMacro(n, false) default: - return renderInlineChildren(n) + return r.renderInlineChildren(n) } } // renderLink renders an as a markdown link, falling back to the href as text. -func renderLink(n *snode) string { +func (r *mdRenderer) renderLink(n *snode) string { href := n.attrs["href"] - text := renderInlineChildren(n) + text := r.renderInlineChildren(n) if text == "" { text = href } @@ -368,13 +398,13 @@ func renderLink(n *snode) string { // renderImage renders an as a markdown image, reconstructing the // title/width/height/align attributes into a plain title or a JSON title. -func renderImage(n *snode) string { +func (r *mdRenderer) renderImage(n *snode) string { alt := n.attrs["ac:alt"] src := "" for _, k := range n.kids { switch k.name { case "ri:attachment": - src = k.attrs["ri:filename"] + src = r.sourceFor(k.attrs["ri:filename"]) case "ri:url": src = k.attrs["ri:value"] } @@ -519,13 +549,13 @@ func attrString(attrs map[string]string) string { // raw on a single line. This covers both column layouts and bodied macros // (expand, panel, …), keeping their bodies readable while the structure and // parameters survive verbatim. -func renderRawBlock(n *snode) string { +func (r *mdRenderer) renderRawBlock(n *snode) string { open := "<" + n.name + attrString(n.attrs) + ">" closeTag := "" // Content container: raw tags around a markdown body. if n.name == "ac:rich-text-body" || n.name == "ac:layout-cell" { - if md := strings.Join(blockStrings(n.kids, ""), "\n\n"); md != "" { + if md := strings.Join(r.blockStrings(n.kids, ""), "\n\n"); md != "" { return open + "\n\n" + md + "\n\n" + closeTag } return open + closeTag @@ -542,7 +572,7 @@ func renderRawBlock(n *snode) string { continue // drop inter-tag whitespace } if k.name == "ac:rich-text-body" || k.name == "ac:layout-cell" || hasElementChild(k) { - parts = append(parts, renderRawBlock(k)) + parts = append(parts, r.renderRawBlock(k)) } else { parts = append(parts, serialize(k)) } diff --git a/internal/convert/storage_to_md_test.go b/internal/convert/storage_to_md_test.go index b67d697..76d9706 100644 --- a/internal/convert/storage_to_md_test.go +++ b/internal/convert/storage_to_md_test.go @@ -32,7 +32,7 @@ func TestStorageToMarkdown(t *testing.T) { if err != nil { t.Fatalf("reading input: %v", err) } - md, err := convert.StorageToMarkdown(string(in)) + md, err := convert.StorageToMarkdown(string(in), nil) if err != nil { t.Fatalf("StorageToMarkdown: %v", err) } @@ -78,7 +78,7 @@ func TestStorageToMarkdownAcceptsForwardCorpus(t *testing.T) { if err := json.Unmarshal(data, &page); err != nil { t.Fatalf("parsing golden: %v", err) } - if _, err := convert.StorageToMarkdown(page.HTML); err != nil { + if _, err := convert.StorageToMarkdown(page.HTML, nil); err != nil { t.Errorf("StorageToMarkdown(%q storage): %v", name, err) } }) @@ -109,7 +109,7 @@ func TestRoundTripStableCallouts(t *testing.T) { if err != nil { t.Fatalf("MdToConfluence: %v", err) } - got, err := convert.StorageToMarkdown(page.HTML) + got, err := convert.StorageToMarkdown(page.HTML, nil) if err != nil { t.Fatalf("StorageToMarkdown: %v", err) } @@ -123,7 +123,7 @@ func TestRoundTripStableCallouts(t *testing.T) { func TestStorageToMarkdownStripsGeneratedIDs(t *testing.T) { in := `` + `DONE` - got, err := convert.StorageToMarkdown(in) + got, err := convert.StorageToMarkdown(in, nil) if err != nil { t.Fatal(err) } @@ -151,7 +151,7 @@ func TestRoundTripPassthrough(t *testing.T) { if err != nil { t.Fatalf("MdToConfluence: %v", err) } - back, err := convert.StorageToMarkdown(page.HTML) + back, err := convert.StorageToMarkdown(page.HTML, nil) if err != nil { t.Fatalf("StorageToMarkdown: %v", err) } @@ -161,3 +161,46 @@ func TestRoundTripPassthrough(t *testing.T) { }) } } + +// TestStorageToMarkdownPrefersRecordedSource checks the two ways an image path is +// recovered. The path recorded on the attachment wins because it is exact; with +// no record, the attachment name is decoded, which is equally exact for a name +// markfluence produced. +func TestStorageToMarkdownPrefersRecordedSource(t *testing.T) { + const in = `

    ` + + got, err := convert.StorageToMarkdown(in, nil) + if err != nil { + t.Fatal(err) + } + if want := "![d](assets/x.png)\n"; got != want { + t.Errorf("decoded from name: got %q, want %q", got, want) + } + + // A recorded source overrides the name -- this is what makes an attachment + // whose name cannot be decoded faithfully (one a human uploaded, or one from + // an older markfluence) still resolve to the right path. + sources := map[string]string{"assets%2Fx.png": "images/original name.png"} + got, err = convert.StorageToMarkdown(in, sources) + if err != nil { + t.Fatal(err) + } + if want := "![d](images/original name.png)\n"; got != want { + t.Errorf("from recorded source: got %q, want %q", got, want) + } +} + +// TestStorageToMarkdownRefusesAbsoluteSource covers a tampered or foreign +// attachment: neither a decoded name nor a recorded path may point a reader -- +// or a later export writing files -- at an absolute location. +func TestStorageToMarkdownRefusesAbsoluteSource(t *testing.T) { + const in = `

    ` + got, err := convert.StorageToMarkdown(in, map[string]string{"%2Fetc%2Fpasswd.png": "/etc/passwd.png"}) + if err != nil { + t.Fatal(err) + } + // Falls back to the raw attachment name rather than an absolute path. + if want := "![d](%2Fetc%2Fpasswd.png)\n"; got != want { + t.Errorf("got %q, want %q", got, want) + } +} diff --git a/internal/convert/testdata/regression/image-properties/test.output b/internal/convert/testdata/regression/image-properties/test.output index 2bbcc4d..af041dc 100644 --- a/internal/convert/testdata/regression/image-properties/test.output +++ b/internal/convert/testdata/regression/image-properties/test.output @@ -1,12 +1,13 @@ { "attachments": [ { - "filename": "assets_shot.png", - "path": "/assets/shot.png" + "filename": "assets%2Fshot.png", + "path": "/assets/shot.png", + "source": "assets/shot.png" } ], "broken": [], - "html": "

    Image Properties

    \n

    A JSON title sets title/width/height/align attributes:

    \n

    \n

    A plain-string title becomes a tooltip (ac:title):

    \n

    \n

    Invalid width/align values are dropped with warnings:

    \n

    \n", + "html": "

    Image Properties

    \n

    A JSON title sets title/width/height/align attributes:

    \n

    \n

    A plain-string title becomes a tooltip (ac:title):

    \n

    \n

    Invalid width/align values are dropped with warnings:

    \n

    \n", "warnings": [ "assets/shot.png: ignoring width='wide' (must be a number)", "assets/shot.png: ignoring align='middle' (must be left, center, or right)" diff --git a/internal/convert/testdata/regression/images-broken/main.md b/internal/convert/testdata/regression/images-broken/main.md index 1c6f85c..4877b14 100644 --- a/internal/convert/testdata/regression/images-broken/main.md +++ b/internal/convert/testdata/regression/images-broken/main.md @@ -7,3 +7,7 @@ A reference to a file that isn't there: A reference to a file with an unsupported extension: ![a pdf](notes.pdf) + +A reference to a file above the documentation root: + +![escaped](../../../../../../../../etc/passwd.png) diff --git a/internal/convert/testdata/regression/images-broken/test.output b/internal/convert/testdata/regression/images-broken/test.output index fb6dd3f..b012a92 100644 --- a/internal/convert/testdata/regression/images-broken/test.output +++ b/internal/convert/testdata/regression/images-broken/test.output @@ -2,8 +2,9 @@ "attachments": [], "broken": [ "IMAGE BROKEN: assets/missing.png (not found)", - "IMAGE BROKEN: notes.pdf (unsupported type)" + "IMAGE BROKEN: notes.pdf (unsupported type)", + "IMAGE BROKEN: ../../../../../../../../etc/passwd.png (outside the documentation root)" ], - "html": "

    Broken Images

    \n

    A reference to a file that isn't there:

    \n

    IMAGE BROKEN: assets/missing.png (not found)

    \n

    A reference to a file with an unsupported extension:

    \n

    IMAGE BROKEN: notes.pdf (unsupported type)

    \n", + "html": "

    Broken Images

    \n

    A reference to a file that isn't there:

    \n

    IMAGE BROKEN: assets/missing.png (not found)

    \n

    A reference to a file with an unsupported extension:

    \n

    IMAGE BROKEN: notes.pdf (unsupported type)

    \n

    A reference to a file above the documentation root:

    \n

    IMAGE BROKEN: ../../../../../../../../etc/passwd.png (outside the documentation root)

    \n", "warnings": [] } diff --git a/internal/convert/testdata/regression/images-local/test.output b/internal/convert/testdata/regression/images-local/test.output index eb8238f..cc5d07a 100644 --- a/internal/convert/testdata/regression/images-local/test.output +++ b/internal/convert/testdata/regression/images-local/test.output @@ -1,11 +1,12 @@ { "attachments": [ { - "filename": "assets_diagram.png", - "path": "/assets/diagram.png" + "filename": "assets%2Fdiagram.png", + "path": "/assets/diagram.png", + "source": "assets/diagram.png" } ], "broken": [], - "html": "

    Local Image

    \n

    A local image becomes an ri:attachment and is collected for upload:

    \n

    \n", + "html": "

    Local Image

    \n

    A local image becomes an ri:attachment and is collected for upload:

    \n

    \n", "warnings": [] } diff --git a/internal/convert/testdata/regression/images-shared-parent/assets/logo.png b/internal/convert/testdata/regression/images-shared-parent/assets/logo.png new file mode 100644 index 0000000..9b9a92c --- /dev/null +++ b/internal/convert/testdata/regression/images-shared-parent/assets/logo.png @@ -0,0 +1 @@ +stub image content (never read by md_to_confluence) diff --git a/internal/convert/testdata/regression/images-shared-parent/sub/main.md b/internal/convert/testdata/regression/images-shared-parent/sub/main.md new file mode 100644 index 0000000..f8fdcc2 --- /dev/null +++ b/internal/convert/testdata/regression/images-shared-parent/sub/main.md @@ -0,0 +1,7 @@ +# Shared Assets + +A page in a subdirectory referencing an asset directory above it -- the layout +GitHub renders too -- is published, with the path preserved in the attachment +name: + +![the logo](../assets/logo.png) diff --git a/internal/convert/testdata/regression/images-shared-parent/test.input b/internal/convert/testdata/regression/images-shared-parent/test.input new file mode 100644 index 0000000..e72fb1d --- /dev/null +++ b/internal/convert/testdata/regression/images-shared-parent/test.input @@ -0,0 +1,4 @@ +{ + "filename": "sub/main.md", + "files": ["sub/main.md", "assets/logo.png"] +} diff --git a/internal/convert/testdata/regression/images-shared-parent/test.output b/internal/convert/testdata/regression/images-shared-parent/test.output new file mode 100644 index 0000000..11c8486 --- /dev/null +++ b/internal/convert/testdata/regression/images-shared-parent/test.output @@ -0,0 +1,12 @@ +{ + "attachments": [ + { + "filename": "..%2Fassets%2Flogo.png", + "path": "/assets/logo.png", + "source": "../assets/logo.png" + } + ], + "broken": [], + "html": "

    Shared Assets

    \n

    A page in a subdirectory referencing an asset directory above it -- the layout GitHub renders too -- is published, with the path preserved in the attachment name:

    \n

    \n", + "warnings": [] +} diff --git a/internal/convert/testdata/regression/kitchen-sink/test.output b/internal/convert/testdata/regression/kitchen-sink/test.output index a289b1f..69cfa6c 100644 --- a/internal/convert/testdata/regression/kitchen-sink/test.output +++ b/internal/convert/testdata/regression/kitchen-sink/test.output @@ -1,11 +1,12 @@ { "attachments": [ { - "filename": "assets_logo.png", - "path": "/assets/logo.png" + "filename": "assets%2Flogo.png", + "path": "/assets/logo.png", + "source": "assets/logo.png" } ], "broken": [], - "html": "

    Release Notes

    \n\n

    See the upgrade guide before starting.

    \n

    Back up your data before upgrading.

    \n

    What's New

    \n

    A soft-wrapped paragraph describing the release across multiple source lines that collapse into one.

    \n

    \n

    Configuration

    \n
    • \n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n
    SettingDefault
    retries3
    timeout30s
    \npython

    A pasted status macro: Stable

    \n", + "html": "

    Release Notes

    \n\n

    See the upgrade guide before starting.

    \n

    Back up your data before upgrading.

    \n

    What's New

    \n

    A soft-wrapped paragraph describing the release across multiple source lines that collapse into one.

    \n

    \n

    Configuration

    \n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n
    SettingDefault
    retries3
    timeout30s
    \npython

    A pasted status macro: Stable

    \n", "warnings": [] } diff --git a/internal/convert/testdata/storage2md/images/input.storage b/internal/convert/testdata/storage2md/images/input.storage index 1139c4f..f3f5e7b 100644 --- a/internal/convert/testdata/storage2md/images/input.storage +++ b/internal/convert/testdata/storage2md/images/input.storage @@ -1,5 +1,5 @@ -

    +

    -

    +

    diff --git a/internal/convert/testdata/storage2md/images/output.md b/internal/convert/testdata/storage2md/images/output.md index 0579601..c3006f9 100644 --- a/internal/convert/testdata/storage2md/images/output.md +++ b/internal/convert/testdata/storage2md/images/output.md @@ -1,4 +1,4 @@ -![an architecture diagram](assets_diagram.png) +![an architecture diagram](assets/diagram.png) ![a bare block-level image](bare.png) @@ -6,4 +6,4 @@ ![tooltipped](shot.png "A Tooltip") -![sized shot](assets_shot.png '{"title":"A Tooltip","width":300,"height":150,"align":"center"}') +![sized shot](assets/shot.png '{"title":"A Tooltip","width":300,"height":150,"align":"center"}')