Skip to content

Commit 8a71d07

Browse files
committed
fix: read rel as an attribute, not a substring of the tag
The scan looked ahead for rel=...stylesheet anywhere in the tag, which matched the string inside ANOTHER attribute's value. That flagged the two shapes the check most needs to leave alone: the canonical async-CSS <link rel="preload" ... onload="this.rel='stylesheet'">, where the advised asset() fix would actively break the preload because the versioned hint could never match the unversioned request, and a data-rel="stylesheet" sitting on a genuine rel="icon". Parse the tag's attributes instead. Each quoted value is consumed as one unit, so it can never be re-scanned as if it held an attribute of its own, and rel now means the rel attribute. The fast bail was also case-sensitive while the scanner was /i, so a file whose only tag was <LINK> was skipped before the scanner could see it. Regression tests cover the onload swap, the data-rel near-miss, the uppercase tag, and a > inside a quoted value.
1 parent e555ee4 commit 8a71d07

2 files changed

Lines changed: 118 additions & 17 deletions

File tree

packages/cli/lib/doctor.js

Lines changed: 80 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -978,23 +978,80 @@ const ROUTE_WALK_IGNORE = new Set(['node_modules', '.git', '.webjs', 'dist', '.n
978978
const ROUTE_MODULE_RE = /^(?:page|layout)\.(?:js|ts|mjs|mts)$/;
979979

980980
/**
981-
* A `<link rel="stylesheet">` whose `href` is a STATIC, quoted, root-absolute
982-
* `/public/…` literal, i.e. one nobody wrapped in `asset()`.
981+
* One whole `<link …>` tag. QUOTE-AWARE (`(?:[^>"']|"[^"]*"|'[^']*')*`), the
982+
* same shape `ssr.js`'s hoist scanner uses, so a `>` inside a quoted attribute
983+
* value cannot terminate the tag early.
984+
* @type {RegExp}
985+
*/
986+
const LINK_TAG_RE = /<link\b(?:[^>"']|"[^"]*"|'[^']*')*>/gi;
987+
988+
/**
989+
* One attribute inside a tag: a name, then optionally `=` and a double-quoted,
990+
* single-quoted, or unquoted value. Matching attributes as WHOLE units is what
991+
* makes the scan correct, because each quoted value is consumed in one step and
992+
* can therefore never be re-scanned as if it contained an attribute of its own.
993+
* @type {RegExp}
994+
*/
995+
const ATTR_RE = /([a-zA-Z_:][-\w:.]*)(?:\s*=\s*(?:"([^"]*)"|'([^']*)'|([^\s>]+)))?/g;
996+
997+
/**
998+
* Parse a tag's attributes into a lowercased-name map. The value is `null` for a
999+
* valueless attribute and carries a `quoted` flag, since this check treats an
1000+
* UNQUOTED href (a template hole) as undecidable rather than as a path.
1001+
* @param {string} tag
1002+
* @returns {Map<string, { value: string | null, quoted: boolean }>}
1003+
*/
1004+
function parseTagAttrs(tag) {
1005+
/** @type {Map<string, { value: string | null, quoted: boolean }>} */
1006+
const attrs = new Map();
1007+
// Skip the tag name itself so `link` is not read as an attribute.
1008+
const body = tag.replace(/^<[a-zA-Z_:][-\w:.]*/, '');
1009+
ATTR_RE.lastIndex = 0;
1010+
for (const m of body.matchAll(ATTR_RE)) {
1011+
const name = m[1].toLowerCase();
1012+
if (attrs.has(name)) continue; // first wins, as in HTML parsing
1013+
const quoted = m[2] !== undefined || m[3] !== undefined;
1014+
const value = m[2] ?? m[3] ?? m[4] ?? null;
1015+
attrs.set(name, { value, quoted });
1016+
}
1017+
return attrs;
1018+
}
1019+
1020+
/**
1021+
* Whether a parsed `<link>` is an unmarked stylesheet, and if so its href.
1022+
*
1023+
* Attribute PARSING rather than a lookahead over the raw tag is load-bearing,
1024+
* not tidiness. A scan that merely looks ahead for `rel=…stylesheet` anywhere in
1025+
* the tag matches the string inside ANOTHER attribute's value, which flags the
1026+
* two shapes this check most needs to leave alone: the canonical async-CSS
1027+
* `<link rel="preload" as="style" href="/public/app.css" onload="this.rel='stylesheet'">`
1028+
* (where the advised `asset()` fix would actively BREAK the preload, since the
1029+
* versioned hint could then never match the unversioned request), and a
1030+
* `data-rel="stylesheet"` sitting on a `rel="icon"`. Reading real attributes
1031+
* makes `rel` mean the `rel` attribute and nothing else.
9831032
*
984-
* Deliberately narrow, so a flag is never a guess:
985-
* - `rel` must be `stylesheet` (see the icon note on the check itself).
986-
* - the href must be QUOTED. A hole (`href=${asset('/public/app.css')}`, or
987-
* any other expression) is left alone, because its value is not decidable
988-
* here and the marked form is exactly the shape that uses one.
989-
* - the path must be under `/public/`, the only prefix `asset()` fingerprints.
990-
* A root remap (`/favicon.ico`, `/sw.js`) is returned unchanged by
1033+
* Returns the href only when every condition holds:
1034+
* - `rel` is a token list CONTAINING `stylesheet` (so `rel="preload"` with an
1035+
* onload swap, and `rel="icon"`, are both out).
1036+
* - `href` is QUOTED. An unquoted value is a template hole
1037+
* (`href=${asset('/public/app.css')}`), undecidable from source, and is
1038+
* exactly the shape the marked form uses.
1039+
* - the path is under `/public/`, the only prefix `asset()` fingerprints. A
1040+
* root remap (`/favicon.ico`, `/sw.js`) is returned unchanged by
9911041
* `resolveAssetUrl`, so advising on one would advise a non-fix.
9921042
*
993-
* `rel` is matched on either side of `href`, since attribute order is free.
994-
* @type {RegExp}
1043+
* @param {string} tag
1044+
* @returns {string | null}
9951045
*/
996-
const UNMARKED_STYLESHEET_RE =
997-
/<link\b(?=[^>]*\brel\s*=\s*["']?stylesheet\b)[^>]*\bhref\s*=\s*["'](\/public\/[^"']*)["'][^>]*>/gi;
1046+
function unmarkedStylesheetHref(tag) {
1047+
const attrs = parseTagAttrs(tag);
1048+
const rel = attrs.get('rel');
1049+
if (!rel || !rel.value) return null;
1050+
if (!rel.value.toLowerCase().split(/\s+/).includes('stylesheet')) return null;
1051+
const href = attrs.get('href');
1052+
if (!href || !href.quoted || !href.value) return null;
1053+
return href.value.startsWith('/public/') ? href.value : null;
1054+
}
9981055

9991056
/**
10001057
* Collect every `app/**` page + layout module path, depth-first.
@@ -1065,12 +1122,18 @@ async function checkUnmarkedAssetLinks(appDir) {
10651122
for (const file of collectRouteModules(routeDir)) {
10661123
let src;
10671124
try { src = await readFile(file, 'utf8'); } catch { continue; }
1068-
if (!src.includes('<link')) continue; // cheap bail before any regex work
1069-
UNMARKED_STYLESHEET_RE.lastIndex = 0;
1070-
for (const m of src.matchAll(UNMARKED_STYLESHEET_RE)) {
1125+
// Cheap bail before any tag scanning. Case-INSENSITIVE to match the tag
1126+
// regex: a file whose only link tag is written `<LINK …>` must still be
1127+
// scanned, or the scanner's own case-insensitivity is unreachable exactly
1128+
// where it is needed.
1129+
if (!/<link/i.test(src)) continue;
1130+
LINK_TAG_RE.lastIndex = 0;
1131+
for (const m of src.matchAll(LINK_TAG_RE)) {
1132+
const href = unmarkedStylesheetHref(m[0]);
1133+
if (!href) continue;
10711134
// 1-indexed line of the match, for a jump-to reference.
10721135
const line = src.slice(0, m.index).split('\n').length;
1073-
findings.push({ file, line, href: m[1] });
1136+
findings.push({ file, line, href });
10741137
}
10751138
}
10761139
if (findings.length === 0) {

test/cli/doctor.test.mjs

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -948,3 +948,41 @@ test('asset-link advisory reports every occurrence across pages and layouts', as
948948
assert.match(r.message, /b\.css/);
949949
assert.match(r.message, /^2 stylesheet link/);
950950
});
951+
952+
test('asset-link advisory does not flag rel="stylesheet" appearing inside another attribute value', async () => {
953+
const dir = tmpDir();
954+
// The canonical async-CSS idiom. `rel` is `preload` here; the `stylesheet`
955+
// string lives in the onload swap. Flagging it would be actively harmful:
956+
// wrapping this href in asset() versions the HINT, which then can never match
957+
// the unversioned request the browser makes, so the file downloads twice.
958+
// A `data-rel` is the same class of near-miss on a genuine icon.
959+
write(dir, 'app/layout.ts', [
960+
"import { html } from '@webjsdev/core';",
961+
'export default function Layout({ children }) {',
962+
' return html`',
963+
' <link rel="preload" as="style" href="/public/app.css" onload="this.rel=\'stylesheet\'">',
964+
' <link data-rel="stylesheet" rel="icon" href="/public/favicon.svg">',
965+
' ${children}`;',
966+
'}',
967+
].join('\n'));
968+
const r = byName(await runDoctorChecks(dir, baseOpts()), ASSET_LINK_CHECK);
969+
assert.equal(r.status, 'pass', 'rel must mean the rel ATTRIBUTE, not the string anywhere in the tag');
970+
});
971+
972+
test('asset-link advisory still flags an unmarked sheet written in uppercase', async () => {
973+
const dir = tmpDir();
974+
write(dir, 'app/layout.ts', 'export default () => `<LINK REL="stylesheet" HREF="/public/up.css">`;');
975+
const r = byName(await runDoctorChecks(dir, baseOpts()), ASSET_LINK_CHECK);
976+
assert.equal(r.status, 'warn', 'HTML tag and attribute names are case-insensitive');
977+
assert.match(r.message, /up\.css/);
978+
});
979+
980+
test('asset-link advisory tolerates a > inside a quoted attribute value', async () => {
981+
const dir = tmpDir();
982+
// A quote-unaware tag scan would end the tag at the `>` in the title and miss
983+
// the href entirely (the #406 class of bug in the SSR hoist scanner).
984+
write(dir, 'app/layout.ts', 'export default () => `<link title="a > b" rel="stylesheet" href="/public/q.css">`;');
985+
const r = byName(await runDoctorChecks(dir, baseOpts()), ASSET_LINK_CHECK);
986+
assert.equal(r.status, 'warn');
987+
assert.match(r.message, /q\.css/);
988+
});

0 commit comments

Comments
 (0)