fix(cli): #1709 guard frontmatter title/label/id decode so a literal % cannot crash the build

[jstockdi]

Related Issue

Resolves #1709

Documentation

N/A — no user-facing documentation changes (bug fix only).

Summary of Changes

1. Added a small safeDecodeURIComponent helper in
   packages/cli/src/lifecycles/graph.js that wraps decodeURIComponent in a
   try/catch and returns the original string on URIError.
2. Used it for the page id, label, and title decodes so valid
   percent-encoded filenames still decode, but a literal % in frontmatter
   (e.g. 100% Complete, Save 20%) can no longer crash the build.
3. Added a regression test case
   packages/cli/test/cases/build.default.frontmatter-percent/ with a fixture
   page whose frontmatter title/label contain %, asserting the build succeeds
   and the title reaches the output verbatim.
4. No breaking changes — valid percent-encoded filenames decode exactly as
   before; only the previously-fatal malformed-input path now falls back to the
   raw string.

[thescientist13]

Awesome!

I wonder if we should try and handle this for the source plugin functionality too? 🤔

• https://github.com/ProjectEvergreen/greenwood/blob/master/packages/cli/src/lifecycles/graph.js#L373
• https://greenwoodjs.dev/docs/reference/plugins-api/#api-5

[jstockdi]

Checked the source-plugin path (graph.js external-sources branch): it never calls decodeURIComponent on node values, so the crash from #1709 can't occur there. External nodes spread ...node directly into the page — title/label/id are taken as provided — and the only URI function applied is encodeURIComponent(route), which doesn't throw on a literal % (it only throws on lone surrogates).

So no change needed for correctness. If you'd like symmetry anyway — e.g. guard the encodeURIComponent(route) call, or run external title/label through safeDecodeURIComponent so both page sources normalize identically — I'm glad to add it here or as a follow-up; it would also cover a source plugin emitting percent-encoded routes.

[thescientist13]

hmm, I think we should probably align the two APIs so the expectation is consistent for users, as opposed to having two different behaviors depending on which API you use.

But then I also think, is there a gap in this for dynamic routing too? Thinking maybe specifically around get static paths handling, in that should id in the example here (which becomes the output route / filename) suffer from the same issue? 🤔
https://greenwoodjs.dev/docs/pages/server-rendering/#dynamic-prerendering

// src/pages/product/[id].js
export async function getStaticPaths() {
  const products = await getProducts();

  return products.map((product) => {
    return {
      // return as many other params as you want
      params: {
        id: product.id,
        name: product.name,
      },
    };
  });
}

That said, I could see the case being made that for Dynamic Routing and Source Plugins, since it's more of a BYOC (bring your own content) strategy, it could be stated that providing properly encoded values is an exercise left to the user, but then I also wonder why not just have Greenwood take care of the things it most needs to care about, like route in both these cases.

So I think I would prefer at least for the things that Greenwood actually cares about / needs to use (e.g. route, label, title) should be handled, and / or we should clearly document what is handled by Greenwood in these cases, and what needs to be handled by the user.

WDYT?

[jstockdi]

Agreed on aligning the two APIs — done in 2bcd855 on this branch: external source-plugin nodes now run id / label / title through the same safeDecodeURIComponent as filesystem pages, so percent-encoded values decode and a literal % passes through verbatim instead of diverging by API. (route was already handled for source plugins — graph.js encodes it via encodeURIComponent.) Extended build.plugins.source with both a literal-% node and a percent-encoded node to pin the behavior.

On your dynamic routing question — good instinct, and it's real, just not where I expected. The params themselves are fine (the decode added in #1714 is guarded), but a getStaticPaths value containing a literal % did crash the build: the raw value gets spliced into the output file:// URL, and fs.mkdir / fs.readFile throw URIError: URI malformed on the invalid percent-sequence — first in prerender.js, then again in bundle.js. Repro: { params: { id: "100%" } }.

Since that's squarely in #1714's territory, I pushed 87957b3 there: a shared getOutputHrefForStaticPath helper that encodes the param at the four output-URL splice sites (prerender + bundle), plus a test asserting a 100% slug builds to a literal 100%/ directory. All existing staticPaths suites (adapter, develop/serve dynamic routing, base-path) still pass.

For docs, I'd propose stating it as: Greenwood normalizes the fields it owns — route, id, label, title — identically across filesystem pages, source plugins, and getStaticPaths params; everything else in a node or params object is passed through as provided. Happy to PR that against greenwoodjs.dev if useful.

[thescientist13]

why do think we need a try catch here? I notice in https://github.com/ProjectEvergreen/greenwood/pull/1714/changes#diff-9433f3990fd9cc31af7c346493e74ea36240f71dbd389a27d2ea659aae74bc6aR56 we didn't make a wrapper functions.

I notice in that other PR we just check for value, so not sure if there is a risk, should we create a helper function (maybe in url utils) that can be used in all these cases? 🤔

[jstockdi]

The try/catch is the actual fix rather than defensive padding — decodeURIComponent throws URIError on any string containing a % that doesn't start a valid escape sequence ("100% Complete"), and that throw is what was killing the build in #1709. A value check can't catch it: the title ? ... : title ternary guards null/undefined, which is a different failure mode.

#1714 has the same guard, just inline instead of named — the Object.fromEntries map in getParamsFromSegment wraps each decodeURIComponent(value) in a try/catch for the same reason (a literal % in a getStaticPaths param).

Agreed a shared helper is the right home — done in 96de833: safeDecodeURIComponent now lives in lib/url-utils.js and graph.js imports it. Once #1714 lands I'll follow up to swap its inline try/catch over to the helper so every decode site goes through the one function.

And 👍 on the docs side — I'll open an issue for the frontmatter-encoding expectations.

[thescientist13]

The try/catch is the actual fix rather than defensive padding

ah, interesting approach. thanks for the explanation

[thescientist13]

Yeah, if you wanted to make an issue / PR on the docs side, that would be great, just better guide developers on what is expected in which scenarios. 👍

[jstockdi]

Docs side is up: ProjectEvergreen/www.greenwoodjs.dev#291 tracks it, with the wording in ProjectEvergreen/www.greenwoodjs.dev#292 — a Special Characters section on the frontmatter page and a param-encoding note under Dynamic Prerendering. Flagged there that it should merge alongside or after the release line carrying this PR and #1714.