Repository navigation
blog: harden /blog routes (close #65, #66, #67, #68, #69, #70, #71) #73
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
8b39d1a
a8f7bf1
d82a219
636c2f3
2b2d525
bb3e2c8
3dc0749
dc58266
5086cb0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { getPost, parseSlug } from "./blog"; | ||
|
|
||
| describe("parseSlug", () => { | ||
| it.each(["a", "a-b", "a-b-c", "abc123", "a1-b2"])("accepts %j", (input) => { | ||
| expect(parseSlug(input)).not.toBeNull(); | ||
| }); | ||
|
|
||
| it.each(["", "-a", "a-", "a--b", "A", "a_b", "a b", "a.b", "<script>"])( | ||
| "rejects %j", | ||
| (input) => { | ||
| expect(parseSlug(input)).toBeNull(); | ||
| }, | ||
| ); | ||
|
|
||
| it("rejects non-string input", () => { | ||
| expect(parseSlug(42)).toBeNull(); | ||
| expect(parseSlug(null)).toBeNull(); | ||
| expect(parseSlug(undefined)).toBeNull(); | ||
| expect(parseSlug({})).toBeNull(); | ||
| expect(parseSlug([])).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe("getPost", () => { | ||
| // Invalid slugs short-circuit on parseSlug before readEntries runs, | ||
| // so these assertions also exercise the "no filesystem read" path. | ||
| it.each(["A", "a--b", "-a", "a-", "", "a_b", "<script>"])( | ||
| "returns null for invalid slug %j without touching the filesystem", | ||
| (input) => { | ||
| expect(getPost(input)).toBeNull(); | ||
| }, | ||
| ); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,26 +4,34 @@ import matter from "gray-matter"; | |
|
|
||
| const POSTS_DIR = path.join(process.cwd(), "content", "blog"); | ||
|
|
||
| // Constrain slug shape so file-system-sourced strings can never carry | ||
| // HTML or URL-significant characters into hrefs. This is the central | ||
| // guard against the stored-XSS class — see CodeQL js/stored-xss. | ||
| // Also rejects empty strings, leading/trailing hyphens, and consecutive | ||
| // hyphens so canonical kebab-case is enforced at the boundary. | ||
| // Central guard against stored-XSS via slug interpolation. Also enforces | ||
| // canonical kebab-case (no empty, leading/trailing/consecutive hyphens). | ||
| const SLUG_RE = /^[a-z0-9]+(?:-[a-z0-9]+)*$/; | ||
| const ISO_DATE_RE = /^\d{4}-\d{2}-\d{2}$/; | ||
|
|
||
| // Branded slug: once a `Slug` is produced, callers can rely on | ||
| // SLUG_RE having held. The only constructor sites are this module's | ||
| // own `parseEntry` (throws on failure) and `parseSlug` (returns null). | ||
| export type Slug = string & { readonly __brand: "Slug" }; | ||
| // SLUG_RE having held. `unique symbol` makes the brand unfakeable | ||
| // across modules — no external caller can write `"foo" as Slug` | ||
| // because they can't reference this module-local symbol. | ||
| declare const __slug: unique symbol; | ||
| export type Slug = string & { readonly [__slug]: true }; | ||
|
|
||
| export const parseSlug = (s: unknown): Slug | null => | ||
| typeof s === "string" && SLUG_RE.test(s) ? (s as Slug) : null; | ||
|
|
||
| // IsoDate mirrors Slug — parseEntry validates against ISO_DATE_RE, and | ||
| // downstream consumers (RSS feed, archive page) can rely on the brand | ||
| // rather than re-testing the regex. | ||
| declare const __isoDate: unique symbol; | ||
| export type IsoDate = string & { readonly [__isoDate]: true }; | ||
|
|
||
| const parseIsoDate = (s: string): IsoDate | null => | ||
| ISO_DATE_RE.test(s) ? (s as IsoDate) : null; | ||
|
|
||
| export type PostMeta = { | ||
| slug: Slug; | ||
| title: string; | ||
| date: string; | ||
| date: IsoDate; | ||
| summary: string; | ||
| bucket?: string; | ||
| }; | ||
|
|
@@ -86,8 +94,8 @@ const parseEntry = (filename: string): PostSource | null => { | |
| } | ||
| const title = data.title.trim(); | ||
|
|
||
| const date = formatDate(data.date); | ||
| if (!ISO_DATE_RE.test(date)) { | ||
| const date = parseIsoDate(formatDate(data.date)); | ||
| if (date === null) { | ||
| throw new Error( | ||
| data.date === undefined | ||
| ? `[blog] ${filename}: frontmatter \`date\` is required` | ||
|
|
@@ -112,33 +120,32 @@ const parseEntry = (filename: string): PostSource | null => { | |
| return { slug, title, date, summary, bucket, body: content }; | ||
| }; | ||
|
|
||
| // Memoise: the build is single-process and source files are immutable | ||
| // mid-build. `readEntries` is hit from `listPosts` (BlogIndex + sitemap + | ||
| // generateStaticParams = 3 sites) and `getPost` (generateMetadata + the | ||
| // page render, 2× per slug), so without the cache we'd parse the | ||
| // directory 2N+3 times. Errors are thrown before `cache` is assigned, | ||
| // so a malformed post fails the build loud rather than poisoning the | ||
| // cache with a partial result. | ||
| // Single-process build with immutable source files; cache lets every | ||
| // call site share one parse. Errors throw before assignment so a | ||
| // malformed post can't poison the cache with a partial result. | ||
| let cache: PostSource[] | undefined; | ||
|
|
||
| const readEntries = (): PostSource[] => { | ||
| if (cache) return cache; | ||
| // The directory's presence is part of the project's contract — every | ||
| // statically wired blog route depends on it. Falling back to `[]` | ||
| // would silently regress to "nothing yet." in prod. If you really | ||
| // want zero posts, leave a `.gitkeep` in `content/blog/`. | ||
| if (!fs.existsSync(POSTS_DIR)) { | ||
| cache = []; | ||
| return cache; | ||
| throw new Error( | ||
| `[blog] posts directory not found at ${POSTS_DIR}. ` + | ||
| `If you intentionally have zero posts, create the directory with a .gitkeep.`, | ||
| ); | ||
| } | ||
| cache = fs | ||
| .readdirSync(POSTS_DIR) | ||
| .filter((f) => /\.mdx?$/.test(f)) | ||
| .map(parseEntry) | ||
| .filter((e): e is PostSource => e !== null) | ||
| .sort((a, b) => { | ||
| // Newest first; ISO-8601 dates compare lexically. Tie-break on | ||
| // slug because `fs.readdirSync` order isn't portable across | ||
| // filesystems (Linux returns inode order, macOS returns lex | ||
| // order on APFS but not on case-insensitive HFS+). Without the | ||
| // tie-break, ES stable sort would preserve readdir order for | ||
| // same-date posts and output would flip between machines. | ||
| // Newest first; tie-break on slug because readdir order isn't | ||
| // portable across filesystems and stable sort would otherwise | ||
| // flip same-date post order between machines. | ||
| if (a.date !== b.date) return b.date.localeCompare(a.date); | ||
| return a.slug.localeCompare(b.slug); | ||
|
Comment on lines
149
to
150
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The addition of a tie-break on References
|
||
| }); | ||
|
|
@@ -158,5 +165,7 @@ export function listPosts(): PostMeta[] { | |
| } | ||
|
|
||
| export function getPost(slug: string): PostSource | null { | ||
| return readEntries().find((e) => e.slug === slug) ?? null; | ||
| const valid = parseSlug(slug); | ||
| if (valid === null) return null; | ||
| return readEntries().find((e) => e.slug === valid) ?? null; | ||
|
alpergundogdu marked this conversation as resolved.
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validation for
SITE_URLis a good security measure to prevent XML injection. However, the error message includes the rawSITE_URL. If this error is ever displayed in an environment that renders HTML (like some CI logs or error monitoring tools), it could potentially lead to a secondary injection ifSITE_URLitself was maliciously crafted. Consider sanitizing the output in the error message or just stating that the URL is invalid.