Hi,

Thanks for the detailed review!

>Initially i thought it was a good idea but after noticing that lowdown
>is several times larger than the whole gotwebd codebase I've changed my mind.

Understood — I'll make the dependency optional at build time, default
enabled, so people who don't want the extra dependency can still build
gotwebd without it. Diff against gotwebd/Makefile:

--- gotwebd/Makefile
+++ gotwebd/Makefile
@@ -30,7 +30,10 @@
MAN =		${PROG}.conf.5 ${PROG}.8
CPPFLAGS +=	-I${.CURDIR}/../include -I${.CURDIR}/../lib -I${.CURDIR}
CPPFLAGS +=	-I${.CURDIR}/../template
-LDADD +=	-lz -levent -lutil -lm -lcrypto
+LDADD +=	-lz -levent -lutil -lm -lcrypto
+.if !defined(NO_LOWDOWN)
+CPPFLAGS +=	-DGOTWEBD_WITH_LOWDOWN
+LDADD +=	-llowdown
+.endif
YFLAGS =
DPADD =		${LIBEVENT} ${LIBUTIL} ${LIBM} ${LIBCRYPTO}
#CFLAGS +=	-DGOT_NO_OBJ_CACHE

and in got_operations.c, got_readme_is_markdown() always exists so
pages.tmpl never needs an #ifdef; it just says "no" when lowdown isn't
linked in:

--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -28,7 +28,9 @@
#include <string.h>
#include <unistd.h>
 
+#ifdef GOTWEBD_WITH_LOWDOWN
#include <lowdown.h>
+#endif
 
#include "got_error.h"
#include "got_object.h"
@@ -760,6 +762,7 @@
return 0;
}
 
+#ifdef GOTWEBD_WITH_LOWDOWN
/*
  * Render a README's Markdown content (buf/len) into a sanitized HTML5
  * fragment. Only called for files whose name ends in ".md"; README and
@@ -793,6 +796,13 @@
*html = out;
*htmlsz = outlen;
return 0;
}
+#else
+int
+got_render_readme_markdown(char **html, size_t *htmlsz,
+    const uint8_t *buf, size_t len)
+{
+	return -1;
+}
+#endif
 
/*
  * Return non-zero if the given README file name should be rendered as
@@ -801,6 +811,10 @@
int
got_readme_is_markdown(const char *name)
{
+#ifndef GOTWEBD_WITH_LOWDOWN
+	return 0;
+#else
size_t len = strlen(name);
 
if (len < 3)
return 0;
return strcasecmp(name + len - 3, ".md") == 0;
+#endif
}

I dropped the comparisons to GitHub/GitLab/cgit from the comments per
your comment below — kept the rationale (untrusted repo content, OWASP
sanitizing, .md-only) but nothing about other software.

>It would be nice to have a gotwebd.conf toggle for this feature as well.

Agreed, and this is a separate knob from the Makefile flag above — the
Makefile flag decides whether the capability is compiled in at all, the
conf option decides whether an operator running a lowdown-enabled
gotwebd wants it turned on for a given server. I'll add
render_readme_markdown <bool> (default on) as a per-server directive
in gotwebd.conf, propagated into struct server, and check it in
pages.tmpl next to the got_readme_is_markdown() call:

--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ struct server {
...
int repos_sort;
+	int render_readme_markdown;
...
};

I'll send the full parse.y/gotwebd.conf.5 hunks for this in v2 once the
rendering path itself is settled, since I'd rather not shuffle the
grammar twice.

>Did you check whether CSS specific to the light-theme or the
>dark-theme would be needed here?

Good catch, I hadn't. I'll add matching .readme overrides wherever
gotweb.css keeps its dark-mode rules, rather than only the light-mode
colors I have now — see question below, I couldn't tell from the tree
where that lives.

>Why is it important to point out that unrelated software such as
>GitHub has similar features? [...] The entire above comment states
>the obvious and could be removed.

Both fixed — comments trimmed to describe behavior only, no comparisons
to other software, and the "does not guarantee NUL-termination" comment
is gone since I no longer copy the buffer at all (next point).

>Why copy the buffer instead of modifying the existing *out buffer?
>Just cast away const, or remove use of const entirely by making *out
>an actual output argument of this function.

You're right, the copy was pointless. got_render_readme_markdown() now
hands back lowdown's own buffer and its real length instead of forcing
a NUL-terminated copy:

--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -762,34 +762,29 @@
-int
-got_render_readme_markdown(char **html, const uint8_t *buf, size_t len)
+int
+got_render_readme_markdown(char **html, size_t *htmlsz,
+    const uint8_t *buf, size_t len)
{
struct lowdown_opts opts;
char *out = NULL;
size_t outlen = 0;
 
*html = NULL;
+	*htmlsz = 0;
 
memset(&opts, 0, sizeof(opts));
opts.type = LOWDOWN_HTML;
opts.feat = LOWDOWN_AUTOLINK | LOWDOWN_TABLES | LOWDOWN_FENCED |
    LOWDOWN_STRIKE | LOWDOWN_SUPER | LOWDOWN_COMMONMARK |
    LOWDOWN_DEFLIST | LOWDOWN_ATTRS;
-	/*
-	 * OWASP-sanitize any raw HTML embedded in the README: this content
-	 * comes from repository data which gotwebd must not trust blindly.
-	 */
+	/* OWASP-sanitize embedded raw HTML: README content is untrusted. */
opts.oflags = LOWDOWN_HTML_HEAD_IDS | LOWDOWN_HTML_NUM_ENT |
    LOWDOWN_HTML_OWASP | LOWDOWN_SMARTY;
 
if (!lowdown_buf(&opts, (const char *)buf, len, &out, &outlen, NULL))
return -1;
 
-	/* lowdown_buf() does not guarantee NUL-termination. */
-	*html = malloc(outlen + 1);
-	if (*html == NULL) {
-		free(out);
-		return -1;
-	}
-	memcpy(*html, out, outlen);
-	(*html)[outlen] = '\0';
-	free(out);
+	*html = out;
+	*htmlsz = outlen;
return 0;
}

and in pages.tmpl the write now uses the real length instead of
strlen(readme_html):

-        {! if (tp_write(tp, readme_html, strlen(readme_html)) == -1) {
+        {! if (tp_write(tp, readme_html, readme_htmlsz) == -1) {

>If the blob is read from a loose object file, the first iteration of
>this loop will see the blob object header, which needs to be
>skipped. [...] Look at install_blob() in lib/worktree.c for an
>example.

You're right, the manual slurp loop I added reads straight from
t->blob without skipping the header on the first block, unlike the
existing <pre> path a few lines below it in the same file. I'd rather
not paper over this with a half-remembered fix, though — I want to
match install_blob()'s handling exactly rather than guess at it, so
I'll send that hunk once I've diffed it against what this loop is
doing. Flagging now rather than pretending it's fixed.

>we should enforce a maximum size limit [...] Or maybe we should pass
>another tempfile handle to this process? Then we could use
>got_object_blob_dump_to_file() and lowdown_file() instead of managing
>blob contents in memory [...]

I agree the tempfile approach is better than a size cap on top of the
realloc loop — it gets us the header-skipping for free too, since
got_object_blob_dump_to_file() is already used elsewhere in gotwebd for
blob output and presumably already handles that correctly. Proposed
shape for v2: pages.tmpl opens a spare tempfile (same pattern used for
diffs elsewhere in this file), dumps the blob into it with
got_object_blob_dump_to_file(), then got_render_readme_markdown() takes
that FILE * and calls lowdown_file(3) instead of lowdown_buf(3). If the
file exceeds a size threshold we skip lowdown and fall through to the
existing raw <pre> dump instead of rendering. This removes the
readme_buf/realloc loop from pages.tmpl entirely.

>I just realized the code you are adding isn't part of fcgi.c at all,
>but is called via gotweb.c. So lowdown would run under a fairly large
>set of pledge promises still, with full repository read access [...]
>If we could move lowdown rendering into a dedicated process which
>runs under pledge("stdio") that would be much better.

You're right that my "no promise beyond what's already held" framing
undersold the problem — this patch doesn't shrink gotweb.c's attack
surface at all, lowdown just doesn't need anything gotweb.c doesn't
already have under pledge("stdio rpath recvfd sendfd proc exec
unveil"), and that set still includes full repo read access.

A few things I'm honestly unsure about and would appreciate your
opinion on before I spend time on a full v2:

- For the dark theme, is there already a dark stylesheet or a
  prefers-color-scheme block somewhere in gotweb.css that I'm missing,
  or is that not something gotwebd handles yet and I'd be introducing
  the first instance of it?

- Are you OK with the tempfile + lowdown_file() direction for the size
  problem, or would you rather I keep lowdown_buf() and just add an
  explicit byte-count cap on top of the current realloc loop? I have a
  slight preference for the tempfile approach since it fixes the
  header-skipping issue as a side effect, but it's a bigger change to
  the function signature and I don't want to redo it twice if you'd
  rather keep things simpler for now.

- On the pledge/process-separation point — do you consider forking a
  dedicated pledge("stdio") helper for the render step a hard
  requirement before this can go in, or something that could land as a
  follow-up once the tempfile-based version is in and working? I don't
  have a good feel for how much appetite there is for that kind of
  restructuring versus shipping this with the isolation gotweb.c
  already has.

Thanks!
-- 
 Secured with Tuta Mail: 
 https://tuta.com/free-email


Sep 8, 2026, 09:01 by stsp@stsp.name:

> On Tue, Sep 08, 2026 at 10:48:38AM +0200, Stefan Sperling wrote:
>
>> On Mon, Sep 07, 2026 at 10:47:13AM +0200, akira.sato@keemail.me wrote:
>> > This patch adds got_render_readme_markdown(), a small wrapper aroundlowdown_buf(3) from textproc/lowdown (already a port, ISC/BSDlicensed, no external dependencies, and explicitly designed to workunder pledge(2) per lowdown(3) — it doesn't need any promise beyondwhat gotwebd's fcgi process already has). Only files ending in .mdgo through this path; plain README and README.txt keep being shownexactly as before, escaped inside <pre>.
>>
>> As you point out, the fcgi.c code runs under pledge("stdio") today and
>> lowdown is happy with this. Back in 2023, when the READMe feature was
>> introduced, gotwebd would have been running lowdown under a much longer
>> list of pledge promises, namely:
>> pledge("stdio rpath inet recvfd proc exec sendfd unveil")
>>
>
> I just realized the code you are adding isn't part of fgci.c at all,
> but is called via gotweb.c. So lowdown would run under a fairly large set
> of pledge promises still, with full repository read access:
>
>  if (pledge("stdio rpath recvfd sendfd proc exec unveil"
>
> If we could move lowdown rendering into a dedicated process which runs
> under pledge("stdio") that would be much better. But it won't be easy
> to do this, since we are already running inside gotweb_process_request()
> when we discover that markdown needs to be rendered. Hmm...
>