malpercio.dev
WIP! A BB-style forum, on the ATmosphere!
We're still working... we'll be back soon when we have something to show off!
node
typescript
hono
htmx
atproto
fix(css-sanitizer): address PR review security and quality issues
Critical:
- Strip </style> sequences from generated output to prevent HTML parser
breakout when CSS is injected via dangerouslySetInnerHTML (XSS regression)
- Fail closed on css-tree onParseError: Raw nodes from error recovery bypass
walker checks, so discard entire output when any parse error occurs
- Wrap sanitizeCssOverrides calls in dedicated try-catch in POST and PUT
theme handlers (separate from PDS write block per CLAUDE.md granularity rule)
- Add try-catch around sanitizeCss calls in BaseLayout with empty fallback
so a css-tree bug doesn't 500 every page for all users
Security:
- Sanitize cssOverrides in POST /api/admin/themes/:rkey/duplicate so
pre-sanitization records don't propagate dangerous CSS via duplication
- Move warning push after list.remove() so audit log only says "Stripped X"
when the node was actually removed (not before the null-check)
- Fix onParseError type signature: (error: SyntaxError) => void
Quality:
- Replace JSON.stringify(warnings) with warnings in structured logger calls
- Update Bruno Create Theme.bru: remove stale ATB-62 placeholder text
- Add integration tests: dangerous CSS stripped in POST and PUT theme handlers
- Fix duplicate test expectation: sanitizer now runs on duplication (compact form)
- Fix </style> test: split into fail-closed test and string-literal stripping test
+37
-1
apps/appview/src/routes/__tests__/admin.test.ts
+37
-1
apps/appview/src/routes/__tests__/admin.test.ts
···
2542
2542
expect(call.record.fontUrls).toEqual(["https://fonts.googleapis.com/css2?family=Space+Grotesk"]);
2543
2543
});
2544
2544
2545
+
it("strips dangerous CSS constructs from cssOverrides before PDS write", async () => {
2546
+
const res = await app.request("/api/admin/themes", {
2547
+
method: "POST",
2548
+
headers: { "Content-Type": "application/json" },
2549
+
body: JSON.stringify({
2550
+
name: "Dangerous Theme",
2551
+
colorScheme: "light",
2552
+
tokens: { "color-bg": "#ffffff" },
2553
+
cssOverrides: '@import "https://evil.com/steal.css"; .ok { color: red; }',
2554
+
}),
2555
+
});
2556
+
expect(res.status).toBe(201);
2557
+
const call = mockPutRecord.mock.calls[0][0];
2558
+
expect(call.record.cssOverrides).not.toContain("@import");
2559
+
expect(call.record.cssOverrides).not.toContain("evil.com");
2560
+
expect(call.record.cssOverrides).toContain("color:red");
2561
+
});
2562
+
2545
2563
it("returns 400 when name is missing", async () => {
2546
2564
const res = await app.request("/api/admin/themes", {
2547
2565
method: "POST",
···
2754
2772
const call = mockPutRecord.mock.calls[0][0];
2755
2773
// Sanitizer reformats CSS to compact form (no extra spaces)
2756
2774
expect(call.record.cssOverrides).toBe(".existing{color:red}");
2775
+
});
2776
+
2777
+
it("strips dangerous CSS constructs from cssOverrides before PDS write on update", async () => {
2778
+
const res = await app.request(`/api/admin/themes/${TEST_RKEY}`, {
2779
+
method: "PUT",
2780
+
headers: { "Content-Type": "application/json" },
2781
+
body: JSON.stringify({
2782
+
name: "Updated Theme",
2783
+
colorScheme: "light",
2784
+
tokens: { "color-bg": "#f0f0f0" },
2785
+
cssOverrides: 'body { background: url("https://evil.com/track.gif"); color: blue; }',
2786
+
}),
2787
+
});
2788
+
expect(res.status).toBe(200);
2789
+
const call = mockPutRecord.mock.calls[0][0];
2790
+
expect(call.record.cssOverrides).not.toContain("evil.com");
2791
+
expect(call.record.cssOverrides).toContain("color:blue");
2757
2792
});
2758
2793
2759
2794
it("preserves existing fontUrls when not provided in request body", async () => {
···
3129
3164
expect(res.status).toBe(201);
3130
3165
expect(mockPutRecord).toHaveBeenCalledOnce();
3131
3166
const putCall = mockPutRecord.mock.calls[0][0];
3132
-
expect(putCall.record.cssOverrides).toBe("body { font-size: 18px; }");
3167
+
// Sanitizer reformats CSS to compact form on duplication
3168
+
expect(putCall.record.cssOverrides).toBe("body{font-size:18px}");
3133
3169
expect(putCall.record.fontUrls).toEqual(["https://fonts.googleapis.com/css2?family=Roboto"]);
3134
3170
expect(putCall.record.name).toBe("Custom Theme (Copy)");
3135
3171
});
+66
-23
apps/appview/src/routes/admin.ts
+66
-23
apps/appview/src/routes/admin.ts
···
1067
1067
}
1068
1068
}
1069
1069
1070
-
// Sanitize cssOverrides before writing to PDS
1071
-
const sanitizedCssOverrides =
1072
-
typeof cssOverrides === "string"
1073
-
? (() => {
1074
-
const { css, warnings } = sanitizeCssOverrides(cssOverrides);
1075
-
if (warnings.length > 0) {
1076
-
ctx.logger.warn("Stripped dangerous CSS constructs from theme on create", {
1077
-
operation: "POST /api/admin/themes",
1078
-
warnings: JSON.stringify(warnings),
1079
-
});
1080
-
}
1081
-
return css;
1082
-
})()
1083
-
: undefined;
1070
+
// Sanitize cssOverrides before writing to PDS. In its own try-catch
1071
+
// because sanitization failure has different semantics than a PDS write failure.
1072
+
let sanitizedCssOverrides: string | undefined;
1073
+
if (typeof cssOverrides === "string") {
1074
+
try {
1075
+
const { css, warnings } = sanitizeCssOverrides(cssOverrides);
1076
+
if (warnings.length > 0) {
1077
+
ctx.logger.warn("Stripped dangerous CSS constructs from theme on create", {
1078
+
operation: "POST /api/admin/themes",
1079
+
warnings,
1080
+
});
1081
+
}
1082
+
sanitizedCssOverrides = css;
1083
+
} catch (error) {
1084
+
if (isProgrammingError(error)) throw error;
1085
+
ctx.logger.error("CSS sanitization failed unexpectedly on create", {
1086
+
operation: "POST /api/admin/themes",
1087
+
error: error instanceof Error ? error.message : String(error),
1088
+
});
1089
+
return c.json({ error: "Failed to process CSS overrides" }, 500);
1090
+
}
1091
+
}
1084
1092
1085
1093
const { agent, error: agentError } = getForumAgentOrError(ctx, c, "POST /api/admin/themes");
1086
1094
if (agentError) return agentError;
···
1188
1196
// optional fields not provided in the request body, to avoid data loss.
1189
1197
const rawCssOverrides =
1190
1198
typeof cssOverrides === "string" ? cssOverrides : theme.cssOverrides;
1191
-
const resolvedCssOverrides = (() => {
1192
-
if (rawCssOverrides == null) return rawCssOverrides;
1193
-
const { css, warnings } = sanitizeCssOverrides(rawCssOverrides);
1194
-
if (warnings.length > 0) {
1195
-
ctx.logger.warn("Stripped dangerous CSS constructs from theme on update", {
1199
+
let resolvedCssOverrides: string | null | undefined = rawCssOverrides;
1200
+
if (rawCssOverrides != null) {
1201
+
try {
1202
+
const { css, warnings } = sanitizeCssOverrides(rawCssOverrides);
1203
+
if (warnings.length > 0) {
1204
+
ctx.logger.warn("Stripped dangerous CSS constructs from theme on update", {
1205
+
operation: "PUT /api/admin/themes/:rkey",
1206
+
themeRkey,
1207
+
warnings,
1208
+
});
1209
+
}
1210
+
resolvedCssOverrides = css;
1211
+
} catch (error) {
1212
+
if (isProgrammingError(error)) throw error;
1213
+
ctx.logger.error("CSS sanitization failed unexpectedly on update", {
1196
1214
operation: "PUT /api/admin/themes/:rkey",
1197
1215
themeRkey,
1198
-
warnings: JSON.stringify(warnings),
1216
+
error: error instanceof Error ? error.message : String(error),
1199
1217
});
1218
+
return c.json({ error: "Failed to process CSS overrides" }, 500);
1200
1219
}
1201
-
return css;
1202
-
})();
1220
+
}
1203
1221
const resolvedFontUrls = Array.isArray(fontUrls) ? fontUrls : (theme.fontUrls as string[] | null);
1204
1222
1205
1223
try {
···
1361
1379
const newName = `${source.name} (Copy)`;
1362
1380
const now = new Date().toISOString();
1363
1381
1382
+
// Sanitize cssOverrides from source before writing to PDS so any
1383
+
// pre-sanitization records don't propagate dangerous CSS via duplication.
1384
+
let duplicateCssOverrides: string | null = null;
1385
+
if (source.cssOverrides != null) {
1386
+
try {
1387
+
const { css, warnings } = sanitizeCssOverrides(source.cssOverrides);
1388
+
if (warnings.length > 0) {
1389
+
ctx.logger.warn("Stripped dangerous CSS constructs from theme on duplicate", {
1390
+
operation: "POST /api/admin/themes/:rkey/duplicate",
1391
+
sourceRkey,
1392
+
warnings,
1393
+
});
1394
+
}
1395
+
duplicateCssOverrides = css;
1396
+
} catch (error) {
1397
+
if (isProgrammingError(error)) throw error;
1398
+
ctx.logger.error("CSS sanitization failed unexpectedly on duplicate", {
1399
+
operation: "POST /api/admin/themes/:rkey/duplicate",
1400
+
sourceRkey,
1401
+
error: error instanceof Error ? error.message : String(error),
1402
+
});
1403
+
return c.json({ error: "Failed to process CSS overrides" }, 500);
1404
+
}
1405
+
}
1406
+
1364
1407
try {
1365
1408
const result = await agent.com.atproto.repo.putRecord({
1366
1409
repo: ctx.config.forumDid,
···
1371
1414
name: newName,
1372
1415
colorScheme: source.colorScheme,
1373
1416
tokens: source.tokens,
1374
-
...(source.cssOverrides != null && { cssOverrides: source.cssOverrides }),
1417
+
...(duplicateCssOverrides != null && { cssOverrides: duplicateCssOverrides }),
1375
1418
...(source.fontUrls != null && { fontUrls: source.fontUrls }),
1376
1419
createdAt: now,
1377
1420
},
+24
-9
apps/web/src/layouts/base.tsx
+24
-9
apps/web/src/layouts/base.tsx
···
31
31
}>
32
32
> = (props) => {
33
33
const { auth, resolvedTheme } = props;
34
+
35
+
let rootCss = "";
36
+
try {
37
+
rootCss = sanitizeCss(`:root { ${tokensToCss(resolvedTheme.tokens)} }`);
38
+
} catch (err) {
39
+
console.error("Failed to sanitize root CSS tokens — rendering without tokens", {
40
+
error: String(err),
41
+
});
42
+
}
43
+
44
+
let overridesCss: string | null = null;
45
+
if (resolvedTheme.cssOverrides) {
46
+
try {
47
+
overridesCss = sanitizeCss(resolvedTheme.cssOverrides);
48
+
} catch (err) {
49
+
console.error("Failed to sanitize CSS overrides — rendering without overrides", {
50
+
error: String(err),
51
+
});
52
+
}
53
+
}
54
+
34
55
return (
35
56
<html lang="en">
36
57
<head>
···
38
59
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
39
60
<meta http-equiv="Accept-CH" content="Sec-CH-Prefers-Color-Scheme" />
40
61
<title>{props.title ?? "atBB Forum"}</title>
41
-
<style
42
-
dangerouslySetInnerHTML={{
43
-
__html: sanitizeCss(`:root { ${tokensToCss(resolvedTheme.tokens)} }`),
44
-
}}
45
-
/>
46
-
{resolvedTheme.cssOverrides && (
47
-
<style
48
-
dangerouslySetInnerHTML={{ __html: sanitizeCss(resolvedTheme.cssOverrides) }}
49
-
/>
62
+
<style dangerouslySetInnerHTML={{ __html: rootCss }} />
63
+
{overridesCss && (
64
+
<style dangerouslySetInnerHTML={{ __html: overridesCss }} />
50
65
)}
51
66
{resolvedTheme.fontUrls && resolvedTheme.fontUrls.length > 0 && (() => {
52
67
const safeFontUrls = resolvedTheme.fontUrls!.filter((url) => url.startsWith("https://"));
+1
-1
bruno/AppView API/Admin Themes/Create Theme.bru
+1
-1
bruno/AppView API/Admin Themes/Create Theme.bru
···
37
37
- name (required): Theme display name, non-empty
38
38
- colorScheme (required): "light" or "dark"
39
39
- tokens (required): Plain object of CSS design token key-value pairs. Values must be strings.
40
-
- cssOverrides (optional): Raw CSS string for structural overrides (not rendered until ATB-62 sanitization ships)
40
+
- cssOverrides (optional): Raw CSS string for structural overrides. Dangerous constructs (@import, external URLs, expression()) are stripped automatically on save.
41
41
- fontUrls (optional): Array of HTTPS URLs for font stylesheets
42
42
43
43
Returns (201):
+17
-6
packages/css-sanitizer/src/__tests__/index.test.ts
+17
-6
packages/css-sanitizer/src/__tests__/index.test.ts
···
276
276
expect(result).toContain("color:red");
277
277
});
278
278
279
-
it("prevents </style> tag injection (HTML escape vector)", () => {
280
-
// An attacker who controls cssOverrides might try to break out of the <style> block
281
-
// This is primarily handled by Hono's JSX escaping, but sanitizing is belt-and-suspenders
282
-
const result = sanitizeCss("body { color: red; }");
283
-
// Safe CSS should pass through and not contain injection attempts
284
-
expect(result).toContain("color:red");
279
+
it("discards malformed CSS containing raw </style> (parse error → fail closed)", () => {
280
+
// Raw </style> is invalid CSS syntax — css-tree's onParseError fires,
281
+
// and the fail-closed policy discards the entire input for security.
282
+
const result = sanitizeCss('body { color: red; } </style><script>alert(1)</script>');
283
+
expect(result).not.toContain("</style");
284
+
expect(result).not.toContain("<script>");
285
+
expect(result).toBe("");
286
+
});
287
+
288
+
it("strips </style> from CSS string literal values (HTML parser injection vector)", () => {
289
+
// A CSS string literal containing </style> is valid CSS that passes parsing.
290
+
// Without post-processing, generate() reproduces it verbatim and the HTML
291
+
// parser would end the <style> block when injected via dangerouslySetInnerHTML.
292
+
// Stripping </style is sufficient: <script> that follows it stays as harmless
293
+
// CSS text inside the style block and cannot execute.
294
+
const result = sanitizeCss('.foo::before { content: "</style><script>alert(1)</script>"; }');
295
+
expect(result).not.toContain("</style");
285
296
});
286
297
});
+1
-1
packages/css-sanitizer/src/css-tree.d.ts
+1
-1
packages/css-sanitizer/src/css-tree.d.ts
+72
-17
packages/css-sanitizer/src/index.ts
+72
-17
packages/css-sanitizer/src/index.ts
···
61
61
}
62
62
63
63
/**
64
+
* Removes a node from its parent list and logs a warning on success.
65
+
* If list/item is null the node cannot be removed; logs a distinct failure
66
+
* message so the audit trail stays accurate.
67
+
*/
68
+
function removeNode(
69
+
list: List | null,
70
+
item: ListItem | null,
71
+
successMessage: string,
72
+
failureMessage: string,
73
+
warnings: string[]
74
+
): void {
75
+
if (list !== null && item !== null) {
76
+
list.remove(item);
77
+
warnings.push(successMessage);
78
+
} else {
79
+
warnings.push(failureMessage);
80
+
}
81
+
}
82
+
83
+
/**
64
84
* Sanitizes a CSS string intended for use as theme `cssOverrides`.
65
85
*
66
86
* Strips all constructs that can trigger network requests or execute code:
···
72
92
*
73
93
* Returns the sanitized CSS string and a list of warning messages
74
94
* describing what was stripped, suitable for structured logging.
95
+
*
96
+
* Returns empty CSS on any parse or generation error — fail closed.
75
97
*/
76
98
export function sanitizeCssOverrides(input: string): SanitizeResult {
77
99
const warnings: string[] = [];
···
81
103
}
82
104
83
105
let ast: CssNode;
106
+
let parseErrorOccurred = false;
84
107
try {
85
108
ast = csstree.parse(input, {
86
109
parseValue: true,
87
-
onParseError: () => {
88
-
// Continue with a fallback node on parse errors
110
+
onParseError: (_error: SyntaxError) => {
111
+
// When css-tree recovers from a parse error it inserts Raw nodes whose
112
+
// type is "Raw" — not "Declaration" or "Atrule" — so they bypass every
113
+
// walker check below. Discard the entire output to stay fail-closed.
114
+
parseErrorOccurred = true;
89
115
},
90
116
});
91
117
} catch {
···
93
119
return { css: "", warnings };
94
120
}
95
121
122
+
if (parseErrorOccurred) {
123
+
warnings.push("CSS parse error encountered — content discarded for security");
124
+
return { css: "", warnings };
125
+
}
126
+
96
127
csstree.walk(ast, {
97
128
enter(node: CssNode, item: ListItem | null, list: List | null) {
98
129
if (node.type === "Atrule") {
···
100
131
typeof node.name === "string" ? node.name.toLowerCase() : "";
101
132
102
133
if (name === "import") {
103
-
warnings.push("Stripped @import rule");
104
-
if (list !== null && item !== null) list.remove(item);
134
+
removeNode(
135
+
list, item,
136
+
"Stripped @import rule",
137
+
"Warning: @import rule detected but could not be removed",
138
+
warnings
139
+
);
105
140
return;
106
141
}
107
142
108
143
if (name === "font-face" && subtreeContainsExternalUrl(node)) {
109
-
warnings.push("Stripped @font-face with external source URL");
110
-
if (list !== null && item !== null) list.remove(item);
144
+
removeNode(
145
+
list, item,
146
+
"Stripped @font-face with external source URL",
147
+
"Warning: @font-face with external URL detected but could not be removed",
148
+
warnings
149
+
);
111
150
return;
112
151
}
113
152
}
···
119
158
: "";
120
159
121
160
if (DANGEROUS_PROPERTIES.has(property)) {
122
-
warnings.push(
123
-
`Stripped dangerous property: ${String(node.property)}`
161
+
removeNode(
162
+
list, item,
163
+
`Stripped dangerous property: ${String(node.property)}`,
164
+
`Warning: dangerous property ${String(node.property)} detected but could not be removed`,
165
+
warnings
124
166
);
125
-
if (list !== null && item !== null) list.remove(item);
126
167
return;
127
168
}
128
169
129
170
if (subtreeContainsExternalUrl(node)) {
130
-
warnings.push(
131
-
`Stripped declaration with external URL: ${String(node.property)}`
171
+
removeNode(
172
+
list, item,
173
+
`Stripped declaration with external URL: ${String(node.property)}`,
174
+
`Warning: declaration with external URL in ${String(node.property)} detected but could not be removed`,
175
+
warnings
132
176
);
133
-
if (list !== null && item !== null) list.remove(item);
134
177
return;
135
178
}
136
179
137
180
if (subtreeContainsExpression(node)) {
138
-
warnings.push(
139
-
`Stripped expression() in: ${String(node.property)}`
181
+
removeNode(
182
+
list, item,
183
+
`Stripped expression() in: ${String(node.property)}`,
184
+
`Warning: expression() in ${String(node.property)} detected but could not be removed`,
185
+
warnings
140
186
);
141
-
if (list !== null && item !== null) list.remove(item);
142
187
return;
143
188
}
144
189
}
145
190
},
146
191
});
147
192
148
-
const sanitized = csstree.generate(ast);
149
-
return { css: sanitized, warnings };
193
+
let generated: string;
194
+
try {
195
+
generated = csstree.generate(ast);
196
+
} catch {
197
+
warnings.push("CSS generation failed after sanitization — content discarded for security");
198
+
return { css: "", warnings };
199
+
}
200
+
201
+
// Strip </style> sequences that could break out of the <style> block when
202
+
// CSS is injected via dangerouslySetInnerHTML (which bypasses JSX escaping).
203
+
const htmlSafe = generated.replace(/<\/style/gi, "");
204
+
return { css: htmlSafe, warnings };
150
205
}
151
206
152
207
/**