Skip to content

Commit f9b4f66

Browse files
fix: address release review feedback
1 parent 76ea746 commit f9b4f66

9 files changed

Lines changed: 168 additions & 27 deletions

File tree

.github/workflows/release.yml

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ on:
99
permissions:
1010
contents: write
1111
id-token: write
12-
attestations: write
1312

1413
env:
1514
FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: 'true'
@@ -113,6 +112,11 @@ jobs:
113112
needs: build-native
114113
if: startsWith(github.ref, 'refs/tags/')
115114
runs-on: ubuntu-latest
115+
# Upload release assets and mint GitHub artifact attestations.
116+
permissions:
117+
contents: write
118+
id-token: write
119+
attestations: write
116120
steps:
117121
- uses: actions/checkout@v6
118122
- uses: actions/download-artifact@v8
@@ -126,8 +130,9 @@ jobs:
126130
cp scripts/install.ps1 release-assets/url-sanitize-installer.ps1
127131
chmod +x release-assets/url-sanitize-installer.sh
128132
(cd release-assets && sha256sum * > SHA256SUMS)
133+
# actions/attest@v4 pinned on 2026-06-01.
129134
- name: Attest release assets
130-
uses: actions/attest@v4
135+
uses: actions/attest@281a49d4cbb0a72c9575a50d18f6deb515a11deb
131136
with:
132137
subject-path: release-assets/*
133138
- uses: softprops/action-gh-release@v3

docs/spec.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -293,6 +293,7 @@ serializes to identical JSON via `serde`.
293293
## 8. Versioning
294294

295295
All packages (`@url-sanitize/core`, `@url-sanitize/clearurls`, `@url-sanitize/cli`,
296-
`@url-sanitize/fetch`, `url-sanitize-core` crate, `url-sanitize` crate) version-bump together. The
297-
public API is governed by **semver of the result schema and CLI contract**, not
298-
by any single language binding. Breaking the schema is a major bump everywhere.
296+
`@url-sanitize/fetch`, `url-sanitize-core` crate, `url-sanitize` crate, and the
297+
Python package in `pyproject.toml`) version-bump together. The public API is
298+
governed by **semver of the result schema and CLI contract**, not by any single
299+
language binding. Breaking the schema is a major bump everywhere.

docs/threat-model.md

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,13 @@ For consumers who need stronger guarantees (e.g. security-sensitive SaaS, regula
2323
```ts
2424
import { fetchClearurlsCatalog } from '@url-sanitize/fetch';
2525

26+
const pinnedHash = process.env.CLEARURLS_RULES_HASH;
27+
if (!pinnedHash) {
28+
throw new Error('CLEARURLS_RULES_HASH is required');
29+
}
30+
2631
const { catalog } = await fetchClearurlsCatalog({
27-
pinnedHash: process.env.CLEARURLS_RULES_HASH
32+
pinnedHash
2833
});
2934
```
3035

packages/core/test/catalog.test.ts

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,10 +94,15 @@ describe('JSON Schema exports', () => {
9494
const sanitize = compileSanitizer(mergeCatalogs(customCatalog, secondCatalog), {
9595
domainBlocking: true
9696
});
97+
const redirected = sanitize(
98+
`https://redirect.example/?to=${encodeURIComponent('https://target.example/')}`
99+
);
100+
expect(redirected.kind).toBe('redirected');
101+
97102
const results: SanitizeResult[] = [
98103
sanitize('https://example.com/clean'),
99104
sanitize('https://example.com/?utm_source=newsletter&id=123'),
100-
sanitize(`https://redirect.example/?to=${encodeURIComponent('https://target.example/')}`),
105+
redirected,
101106
sanitize('https://blocked.example/')
102107
];
103108

packages/core/test/fuzz.test.ts

Lines changed: 23 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,17 +15,36 @@ describe('fuzz — deterministic ReDoS guard', () => {
1515
});
1616
const rng = mulberry32(0x5eed_2026);
1717
let maxMs = 0;
18+
let slowestInput = '';
19+
let thrown:
20+
| {
21+
error: unknown;
22+
index: number;
23+
input: string;
24+
}
25+
| undefined;
1826

1927
for (let index = 0; index < CASES; index += 1) {
2028
const input = randomUrl(rng, index);
2129
const started = performance.now();
22-
expect(() => sanitize(input), `fuzz input ${index}: ${input}`).not.toThrow();
30+
try {
31+
sanitize(input);
32+
} catch (error) {
33+
thrown = { error, index, input };
34+
}
2335
const elapsed = performance.now() - started;
24-
maxMs = Math.max(maxMs, elapsed);
25-
expect(elapsed, `fuzz input ${index}: ${input}`).toBeLessThan(MAX_SANITIZE_MS);
36+
if (elapsed > maxMs) {
37+
maxMs = elapsed;
38+
slowestInput = `fuzz input ${index}: ${input}`;
39+
}
40+
if (thrown) break;
2641
}
2742

28-
expect(maxMs).toBeLessThan(MAX_SANITIZE_MS);
43+
if (thrown) {
44+
const message = thrown.error instanceof Error ? thrown.error.message : String(thrown.error);
45+
throw new Error(`fuzz input ${thrown.index}: ${thrown.input}: ${message}`);
46+
}
47+
expect(maxMs, slowestInput).toBeLessThan(MAX_SANITIZE_MS);
2948
}, 20_000);
3049
});
3150

packages/fetch/README.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,8 @@ console.log(sanitize('https://example.com/?utm_source=x'));
2525
```
2626

2727
`pinnedHash` is optional. When set, the downloaded rules must match both the
28-
upstream `rules.minify.hash` and the consumer-provided pin.
28+
upstream `rules.minify.hash` and the consumer-provided pin. `timeoutMs` defaults
29+
to `10000` and bounds each rules/hash request.
2930

3031
## License
3132

packages/fetch/src/index.ts

Lines changed: 48 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ export interface FetchClearurlsCatalogOptions {
1010
rulesUrl?: string;
1111
hashUrl?: string;
1212
fetch?: typeof globalThis.fetch;
13+
timeoutMs?: number;
1314
}
1415

1516
export interface FetchClearurlsCatalogResult {
@@ -23,19 +24,21 @@ export async function fetchClearurlsCatalog(
2324
): Promise<FetchClearurlsCatalogResult> {
2425
const rulesUrl = options.rulesUrl ?? DEFAULT_CLEARURLS_RULES_URL;
2526
const hashUrl = options.hashUrl ?? DEFAULT_CLEARURLS_HASH_URL;
26-
const fetchImpl = options.fetch ?? globalThis.fetch;
27+
const fetchImpl =
28+
options.fetch ?? (globalThis.fetch ? globalThis.fetch.bind(globalThis) : undefined);
29+
const timeoutMs = options.timeoutMs ?? 10_000;
2730

2831
if (!fetchImpl) {
2932
throw new Error('fetchClearurlsCatalog requires a fetch implementation');
3033
}
3134

3235
const [rulesTextRaw, expectedHashRaw] = await Promise.all([
33-
fetchText(fetchImpl, rulesUrl),
34-
fetchText(fetchImpl, hashUrl)
36+
fetchText(fetchImpl, rulesUrl, timeoutMs),
37+
fetchText(fetchImpl, hashUrl, timeoutMs)
3538
]);
3639
const rulesText = rulesTextRaw.trim();
3740
const expectedHash = normalizeSha256(expectedHashRaw, 'published hash');
38-
const actualHash = await sha256Hex(rulesText);
41+
const actualHash = await sha256Hex(rulesTextRaw);
3942

4043
if (actualHash !== expectedHash) {
4144
throw new Error(`ClearURLs rules hash mismatch: expected ${expectedHash}, got ${actualHash}`);
@@ -63,14 +66,53 @@ export async function fetchClearurlsCatalog(
6366
};
6467
}
6568

66-
async function fetchText(fetchImpl: typeof globalThis.fetch, url: string): Promise<string> {
67-
const response = await fetchImpl(url);
69+
async function fetchText(
70+
fetchImpl: typeof globalThis.fetch,
71+
url: string,
72+
timeoutMs: number
73+
): Promise<string> {
74+
const { signal, cleanup } = createTimeoutSignal(timeoutMs);
75+
let response: Response;
76+
try {
77+
response = await fetchImpl(url, { signal });
78+
} catch (error) {
79+
if (isAbortError(error)) {
80+
throw new Error(`Fetch ${url} timed out after ${timeoutMs}ms`);
81+
}
82+
throw error;
83+
} finally {
84+
cleanup();
85+
}
86+
6887
if (!response.ok) {
6988
throw new Error(`Fetch ${url} failed: ${response.status} ${response.statusText}`.trim());
7089
}
7190
return response.text();
7291
}
7392

93+
function createTimeoutSignal(timeoutMs: number): { cleanup: () => void; signal: AbortSignal } {
94+
if (!Number.isFinite(timeoutMs) || timeoutMs <= 0) {
95+
throw new Error('fetchClearurlsCatalog timeoutMs must be a positive finite number');
96+
}
97+
98+
if (typeof AbortSignal.timeout === 'function') {
99+
return { signal: AbortSignal.timeout(timeoutMs), cleanup: () => {} };
100+
}
101+
102+
const controller = new AbortController();
103+
const timer = setTimeout(() => controller.abort(), timeoutMs);
104+
return {
105+
signal: controller.signal,
106+
cleanup: () => clearTimeout(timer)
107+
};
108+
}
109+
110+
function isAbortError(error: unknown): boolean {
111+
return (
112+
error instanceof DOMException && (error.name === 'AbortError' || error.name === 'TimeoutError')
113+
);
114+
}
115+
74116
function normalizeSha256(hash: string, label: string): string {
75117
const normalized = hash.trim().toLowerCase();
76118
if (!/^[0-9a-f]{64}$/.test(normalized)) {

packages/fetch/test/fetch.test.ts

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,22 @@ describe('fetchClearurlsCatalog', () => {
3737
});
3838
});
3939

40+
it('hashes the raw rules text before parsing', async () => {
41+
const rulesTextWithNewline = `${rulesText}\n`;
42+
const rulesHashWithNewline = sha256(rulesTextWithNewline);
43+
44+
const result = await fetchClearurlsCatalog({
45+
rulesUrl,
46+
hashUrl,
47+
fetch: fakeFetch({
48+
[rulesUrl]: rulesTextWithNewline,
49+
[hashUrl]: rulesHashWithNewline
50+
})
51+
});
52+
53+
expect(result.metadata.hash).toBe(rulesHashWithNewline);
54+
});
55+
4056
it('uses the injected fetch implementation', async () => {
4157
const seen: string[] = [];
4258
await fetchClearurlsCatalog({
@@ -51,6 +67,49 @@ describe('fetchClearurlsCatalog', () => {
5167
expect(seen.sort()).toEqual([hashUrl, rulesUrl].sort());
5268
});
5369

70+
it('binds the default fetch implementation to globalThis', async () => {
71+
const originalFetch = globalThis.fetch;
72+
const seenThisValues: unknown[] = [];
73+
globalThis.fetch = async function (
74+
this: typeof globalThis,
75+
url: Parameters<typeof globalThis.fetch>[0]
76+
): Promise<Response> {
77+
seenThisValues.push(this);
78+
return textResponse(String(url) === rulesUrl ? rulesText : rulesHash);
79+
} as typeof globalThis.fetch;
80+
81+
try {
82+
await fetchClearurlsCatalog({ rulesUrl, hashUrl });
83+
} finally {
84+
globalThis.fetch = originalFetch;
85+
}
86+
87+
expect(seenThisValues).toEqual([globalThis, globalThis]);
88+
});
89+
90+
it('throws a timeout-specific error when a fetch stalls', async () => {
91+
await expect(
92+
fetchClearurlsCatalog({
93+
rulesUrl,
94+
hashUrl,
95+
timeoutMs: 1,
96+
fetch: (_url, init) =>
97+
new Promise<Response>((_resolve, reject) => {
98+
const signal = init?.signal;
99+
if (!signal) {
100+
reject(new Error('missing timeout signal'));
101+
return;
102+
}
103+
signal.addEventListener(
104+
'abort',
105+
() => reject(new DOMException('timed out', 'TimeoutError')),
106+
{ once: true }
107+
);
108+
})
109+
})
110+
).rejects.toThrow(/timed out after 1ms/);
111+
});
112+
54113
it('throws on HTTP failures', async () => {
55114
await expect(
56115
fetchClearurlsCatalog({
@@ -127,5 +186,5 @@ function textResponse(body: string, status = 200): Response {
127186
}
128187

129188
function sha256(text: string): string {
130-
return createHash('sha256').update(text.trim()).digest('hex');
189+
return createHash('sha256').update(text).digest('hex');
131190
}

scripts/verify-release.mjs

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -56,16 +56,20 @@ if (uniqueVersions.size !== 1) {
5656
}
5757

5858
const version = [...uniqueVersions][0];
59-
if (!/^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/.test(version)) {
60-
errors.push(`invalid release version: ${version}`);
61-
}
59+
if (!version) {
60+
errors.push('failed to resolve a release version from manifests');
61+
} else {
62+
if (!/^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?$/.test(version)) {
63+
errors.push(`invalid release version: ${version}`);
64+
}
6265

63-
if (!isStableReleaseVersion(version)) {
64-
errors.push(`release version must be >= 1.0.0, got ${version}`);
65-
}
66+
if (!isStableReleaseVersion(version)) {
67+
errors.push(`release version must be >= 1.0.0, got ${version}`);
68+
}
6669

67-
if (args.tag && args.tag !== `v${version}`) {
68-
errors.push(`tag ${args.tag} does not match release version v${version}`);
70+
if (args.tag && args.tag !== `v${version}`) {
71+
errors.push(`tag ${args.tag} does not match release version v${version}`);
72+
}
6973
}
7074

7175
const platforms = loadReleasePlatforms();

0 commit comments

Comments
 (0)