Skip to content

Commit 45a54dd

Browse files
committed
fix(hosting): address PR review (nitro baseURL scoping, batch tests, doc/comment fixes)
Today's review comments on PR #46: - nitro readBundledBaseURL: brace-scope the read to the `app` block so an ipx.baseURL serialized before app.baseURL can't be picked up and 308 the whole site (fail-closed: no whole-source fallback). + tests (ipx-before-app, ipx-before-root). - kv_keys_handler: export computeDiff + batches and factor the put/delete diff out of applyUpdate; add boundary tests (exactly 50 vs 51 keys, mixed put+delete crossing the 50-key ceiling, 3 MB byte flush, no-op path). - kvs_router: reword the glob comment (this is framework route matching, NOT CloudFront behavior selection — CF '*' crosses '/', ours doesn't) + add a negative test that /api/*/data does NOT match /api/foo/bar/data. - kv_keys_handler header: drop stale NodejsFunction reference (now pre-bundled at build time via bundle-handlers.mjs). - kvs_router.test: comment that new Function() codegen is intentional and would throw (not silently no-op) under --disallow-code-generation-from-strings. - node_runtime: drop the unused DEFAULT_NODE_RUNTIME_STRING export and document why adapters pin nodejs20.x to the framework bundle instead of this constant. - changeset: 'three' -> 'four' bugs (matches the four bullets).
1 parent 676f792 commit 45a54dd

8 files changed

Lines changed: 232 additions & 29 deletions

File tree

.changeset/hosting-phase0-bugs.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
"@aws-blocks/core": patch
44
---
55

6-
Fix three hosting correctness bugs:
6+
Fix four hosting correctness bugs:
77

88
- **Base path is now a first-class `Hosting` prop, and Nuxt `app.baseURL` is modelled.** Added a caller-declared `basePath` option to `Hosting` (e.g. `{ basePath: '/app' }`) — the recommended, framework-agnostic source of truth that CloudFront behaviors are prefixed with (plus a root→`/<basePath>/` 308 redirect). When the prop is omitted, the Nitro adapter now detects Nuxt's `app.baseURL` from the build output and sets `manifest.basePath` (parity with Next `basePath` / Astro `base`); previously it was silently dropped, so a Nuxt app with a base path deployed broken — pages rendered but their hashed `/<base>/_nuxt/*` assets 404'd (no hydration). If a base path is detected in the prerendered output but can't be read, synth fails loud instead of shipping a broken site.
99
- **Per-pattern header rules delegate to the SSR runtime instead of competing for CloudFront behavior slots.** For SSR (compute) deploys, a header rule whose pattern has no dedicated behavior is no longer wired as its own CloudFront behavior — the request falls through to the catch-all SSR Lambda, which already emits the framework's `headers()` / `routeRules` at runtime (CloudFront caches the response including those headers). This removes redundant behaviors that burned the scarce ~25-behavior budget and re-asserted a header the origin already sets, and it means SSR header rules can never trip the behavior cap. For **static-only** deploys (S3 origin, no runtime to emit the header) the cap still applies: a rule that would exceed it throws if it sets a security header (CSP, HSTS, X-Frame-Options, … — a lost CSP otherwise looks like a successful deploy) and is dropped with a warning if it's cosmetic.

packages/hosting/src/adapters/nitro.test.ts

Lines changed: 39 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,13 @@ const writeMinimalNitroOutput = (
3131
* Used to exercise the prerendered-HTML baseURL safety net.
3232
*/
3333
publicFiles?: Record<string, string>;
34+
/**
35+
* When set, inject an `ipx: { baseURL: <value> }` block into the runtime
36+
* config BEFORE the `app` block, reproducing the ordering hazard where a
37+
* bare `/"baseURL"/` scan would wrongly pick up `ipx.baseURL` (default
38+
* `/_ipx`) instead of `app.baseURL`. Exercises the brace-scoped read.
39+
*/
40+
ipxBaseURLBefore?: string;
3441
} = {},
3542
): void => {
3643
const outputDir = path.join(projectDir, '.output');
@@ -53,7 +60,13 @@ const writeMinimalNitroOutput = (
5360
extras.baseURL !== undefined
5461
? ` "baseURL": ${JSON.stringify(extras.baseURL)},`
5562
: '';
56-
const bundleSource = `// nitro server bundle\n_inlineRuntimeConfig = { app: {${baseURLBlob} }, nitro: { "routeRules": ${JSON.stringify(bundledRules)} } };\n`;
63+
// Optional `ipx.baseURL` serialized BEFORE the app block — the ordering that
64+
// would trip a naive whole-source `/"baseURL"/` scan.
65+
const ipxBlob =
66+
extras.ipxBaseURLBefore !== undefined
67+
? `ipx: { "baseURL": ${JSON.stringify(extras.ipxBaseURLBefore)} }, `
68+
: '';
69+
const bundleSource = `// nitro server bundle\n_inlineRuntimeConfig = { ${ipxBlob}app: {${baseURLBlob} }, nitro: { "routeRules": ${JSON.stringify(bundledRules)} } };\n`;
5770
fs.writeFileSync(path.join(chunksDir, 'nitro.mjs'), bundleSource);
5871
// Optional prerendered HTML / public files.
5972
for (const [rel, content] of Object.entries(extras.publicFiles ?? {})) {
@@ -901,6 +914,31 @@ void describe('nitroAdapter — app.baseURL → manifest.basePath (P0.1)', () =>
901914
assert.strictEqual(manifest.basePath, undefined);
902915
});
903916

917+
void it('reads app.baseURL even when ipx.baseURL is serialized first (brace-scoped)', () => {
918+
// Ordering hazard: a bare /"baseURL"/ scan over the whole bundle would pick
919+
// up ipx.baseURL (/_ipx) here and 308 the whole site. The brace-scoped read
920+
// of the `app` block must still yield the real app.baseURL.
921+
writeMinimalNitroOutput(tmpDir, {
922+
ipxBaseURLBefore: '/_ipx',
923+
baseURL: '/myapp/',
924+
});
925+
writePackageJson(tmpDir, { nuxt: '^4.0.0' });
926+
const manifest = nitroAdapter({ projectDir: tmpDir, skipBuild: true });
927+
assert.strictEqual(manifest.basePath, '/myapp');
928+
});
929+
930+
void it('does NOT mistake ipx.baseURL for a base path when app.baseURL is root', () => {
931+
// ipx.baseURL=/_ipx serialized before an app block whose baseURL is "/"
932+
// (root). basePath must be undefined, NOT "/_ipx".
933+
writeMinimalNitroOutput(tmpDir, {
934+
ipxBaseURLBefore: '/_ipx',
935+
baseURL: '/',
936+
});
937+
writePackageJson(tmpDir, { nuxt: '^4.0.0' });
938+
const manifest = nitroAdapter({ projectDir: tmpDir, skipBuild: true });
939+
assert.strictEqual(manifest.basePath, undefined);
940+
});
941+
904942
void it('leaves basePath undefined when no baseURL is present', () => {
905943
writeMinimalNitroOutput(tmpDir);
906944
writePackageJson(tmpDir, { nuxt: '^4.0.0' });

packages/hosting/src/adapters/nitro.ts

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -660,8 +660,24 @@ const readBundledBaseURL = (serverDir: string): string | undefined => {
660660

661661
for (const candidate of candidates) {
662662
const source = fs.readFileSync(candidate, 'utf-8');
663-
// The runtime-config blob embeds it as `"baseURL": "/myapp/"`.
664-
const match = source.match(/"baseURL"\s*:\s*"([^"]*)"/);
663+
// Scope to the `"app":{...}` block, then read ONLY its `baseURL`. The
664+
// runtime config also carries `ipx.baseURL` (default `/_ipx`) under
665+
// `_inlineRuntimeConfig`; a bare `/"baseURL"/` match over the whole source
666+
// is order-dependent and would silently pick up `/_ipx` if the bundle ever
667+
// serialized it before `app.baseURL` — setting basePath to `/_ipx` and
668+
// 308-ing the whole site. Brace-scoping to the `app` object (same approach
669+
// as readBundledRouteRules) removes that ambiguity. Try both `"app":` and
670+
// `app:` (un-quoted key) since the inlined literal isn't always strict JSON.
671+
//
672+
// No whole-source fallback: a wrong prefix is worse than a missed one (the
673+
// HTML safety net catches a MISSED app.baseURL, but not a WRONG one). If the
674+
// app block isn't found, return undefined and let detectBaseURLFromPrerendered
675+
// Html fail loud on a genuinely missed prefix.
676+
const appBlock =
677+
extractJsonObjectAfter(source, '"app":') ??
678+
extractJsonObjectAfter(source, 'app:');
679+
if (!appBlock) continue;
680+
const match = appBlock.match(/"baseURL"\s*:\s*"([^"]*)"/);
665681
if (match) {
666682
const value = match[1];
667683
return value === '/' ? undefined : value;

packages/hosting/src/constructs/kv_keys_handler.test.ts

Lines changed: 81 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// SPDX-License-Identifier: Apache-2.0
33
import { describe, it } from 'node:test';
44
import assert from 'node:assert/strict';
5-
import { deleteDrainSet } from './kv_keys_handler.js';
5+
import { batches, computeDiff, deleteDrainSet } from './kv_keys_handler.js';
66

77
// Regression for the Delete-path drain bug: CloudFormation does not send
88
// OldResourceProperties on Delete, so the keys to drain must come from
@@ -40,3 +40,83 @@ describe('kv_keys_handler — deleteDrainSet', () => {
4040
assert.deepEqual(deleteDrainSet(event), real);
4141
});
4242
});
43+
44+
// The route-table flip is applied via batched UpdateKeys calls. An off-by-one
45+
// at the 50-key boundary would partial-apply the table mid-cutover and surface
46+
// as an opaque deploy-time failure — so the pure diff + batching are unit-tested
47+
// at the boundaries here.
48+
describe('kv_keys_handler — computeDiff', () => {
49+
it('puts new + changed keys, deletes removed keys, skips unchanged', () => {
50+
const desired = { a: '1', b: '2-new', c: '3' }; // a unchanged, b changed, c new
51+
const previous = { a: '1', b: '2-old', d: '4' }; // d removed
52+
const { puts, deletes } = computeDiff(desired, previous);
53+
assert.deepEqual(
54+
puts.sort((x, y) => x.Key.localeCompare(y.Key)),
55+
[
56+
{ Key: 'b', Value: '2-new' },
57+
{ Key: 'c', Value: '3' },
58+
],
59+
);
60+
assert.deepEqual(deletes, [{ Key: 'd' }]);
61+
});
62+
63+
it('is a no-op when desired equals previous', () => {
64+
const same = { a: '1', b: '2' };
65+
const { puts, deletes } = computeDiff(same, { ...same });
66+
assert.equal(puts.length, 0);
67+
assert.equal(deletes.length, 0);
68+
});
69+
});
70+
71+
describe('kv_keys_handler — batches (50-key / 3 MB boundaries)', () => {
72+
const mkPuts = (n: number) =>
73+
Array.from({ length: n }, (_, i) => ({ Key: `k${i}`, Value: 'v' }));
74+
const collect = (
75+
puts: { Key: string; Value: string }[],
76+
deletes: { Key: string }[] = [],
77+
) => [...batches(puts, deletes)];
78+
79+
it('packs exactly 50 puts into a single batch', () => {
80+
const out = collect(mkPuts(50));
81+
assert.equal(out.length, 1);
82+
assert.equal(out[0].puts.length, 50);
83+
});
84+
85+
it('splits 51 puts into 50 + 1', () => {
86+
const out = collect(mkPuts(51));
87+
assert.equal(out.length, 2);
88+
assert.equal(out[0].puts.length, 50);
89+
assert.equal(out[1].puts.length, 1);
90+
});
91+
92+
it('counts puts AND deletes against the same 50-key ceiling (mixed crossing)', () => {
93+
// 30 puts + 30 deletes = 60 keys → must split (50 then 10), not one batch.
94+
const deletes = Array.from({ length: 30 }, (_, i) => ({ Key: `d${i}` }));
95+
const out = collect(mkPuts(30), deletes);
96+
const totalKeys = out.reduce(
97+
(n, b) => n + b.puts.length + b.deletes.length,
98+
0,
99+
);
100+
assert.equal(totalKeys, 60); // nothing dropped
101+
assert.ok(
102+
out.every((b) => b.puts.length + b.deletes.length <= 50),
103+
'no batch exceeds the 50-key ceiling',
104+
);
105+
assert.equal(out.length, 2);
106+
});
107+
108+
it('flushes on the 3 MB byte ceiling before the key ceiling', () => {
109+
// Two ~2 MB puts (4 MB total) must land in separate batches even though
110+
// they are only 2 keys — the byte ceiling trips first.
111+
const big = 'x'.repeat(2 * 1024 * 1024);
112+
const out = collect([
113+
{ Key: 'a', Value: big },
114+
{ Key: 'b', Value: big },
115+
]);
116+
assert.equal(out.length, 2);
117+
});
118+
119+
it('yields nothing for an empty diff (no-op path)', () => {
120+
assert.equal(collect([], []).length, 0);
121+
});
122+
});

packages/hosting/src/constructs/kv_keys_handler.ts

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,13 @@
1414
* only AFTER that build's assets are uploaded to S3 — preserving the
1515
* atomic-deploy cutover guarantee.
1616
*
17-
* Bundled with its SDK dependency via esbuild (NodejsFunction), because
18-
* `@aws-sdk/client-cloudfront-keyvaluestore` is NOT part of the Lambda runtime
19-
* baseline (it requires SigV4a signing).
17+
* Bundling: pre-bundled at BUILD time via `scripts/bundle-handlers.mjs`
18+
* (esbuild → `kv_keys_handler_bundle.mjs`) and shipped through
19+
* `Code.fromAsset`, NOT bundled at synth via `NodejsFunction`. Reason:
20+
* `@aws-sdk/client-cloudfront-keyvaluestore` is NOT in the Lambda runtime
21+
* baseline (it requires SigV4a signing), and a synth-time `NodejsFunction`
22+
* would resolve `projectRoot`/lockfile under `node_modules/` once this package
23+
* is installed from npm and fail. See `kv_keys.ts` for the asset wiring.
2024
*/
2125
import {
2226
CloudFrontKeyValueStoreClient,
@@ -58,14 +62,38 @@ const client = new CloudFrontKeyValueStoreClient({
5862

5963
const byteLen = (s: string): number => Buffer.byteLength(s, 'utf8');
6064

65+
export type Put = { Key: string; Value: string };
66+
export type Delete = { Key: string };
67+
68+
/**
69+
* Pure diff of desired vs. previous entries into the puts/deletes the route-
70+
* table flip needs: a put for every new-or-changed key, a delete for every key
71+
* that disappeared. Exported (with {@link batches}) so the load-bearing
72+
* cutover logic is unit-testable without the SDK.
73+
*/
74+
export function computeDiff(
75+
desired: Entries,
76+
previous: Entries,
77+
): { puts: Put[]; deletes: Delete[] } {
78+
const puts = Object.entries(desired)
79+
.filter(([k, v]) => previous[k] !== v)
80+
.map(([Key, Value]) => ({ Key, Value }));
81+
const deletes = Object.keys(previous)
82+
.filter((k) => !(k in desired))
83+
.map((Key) => ({ Key }));
84+
return { puts, deletes };
85+
}
86+
6187
/**
6288
* Split desired puts + deletes into UpdateKeys batches that respect the
63-
* 50-key / 3 MB-per-request ceiling.
89+
* 50-key / 3 MB-per-request ceiling. Exported for unit testing of the batch
90+
* boundaries (an off-by-one here would partial-apply the route table
91+
* mid-cutover and surface as an opaque deploy-time failure).
6492
*/
65-
function* batches(
66-
puts: { Key: string; Value: string }[],
67-
deletes: { Key: string }[],
68-
): Generator<{ puts: typeof puts; deletes: typeof deletes }> {
93+
export function* batches(
94+
puts: Put[],
95+
deletes: Delete[],
96+
): Generator<{ puts: Put[]; deletes: Delete[] }> {
6997
let curPuts: typeof puts = [];
7098
let curDeletes: typeof deletes = [];
7199
let count = 0;
@@ -115,12 +143,7 @@ async function applyUpdate(
115143
desired: Entries,
116144
previous: Entries,
117145
): Promise<void> {
118-
const puts = Object.entries(desired)
119-
.filter(([k, v]) => previous[k] !== v)
120-
.map(([Key, Value]) => ({ Key, Value }));
121-
const deletes = Object.keys(previous)
122-
.filter((k) => !(k in desired))
123-
.map((Key) => ({ Key }));
146+
const { puts, deletes } = computeDiff(desired, previous);
124147

125148
if (puts.length === 0 && deletes.length === 0) return;
126149

packages/hosting/src/constructs/kvs_router.test.ts

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,12 @@ const baseManifest = (overrides: Partial<DeployManifest> = {}): DeployManifest =
3434
* Evaluate a generated CloudFront Function string against a KVS map. Strips the
3535
* `import cf from 'cloudfront'` ESM line and injects fakes, then returns the
3636
* `handler`'s output. `selectedOrigin` captures cf.selectRequestOriginById.
37+
*
38+
* Uses `new Function(...)` deliberately to execute the generated function
39+
* source end-to-end — the source is fully repo-controlled (produced by
40+
* generateKvsRouterRequestCode), not external input. NOTE: under a hardened
41+
* runner with `--disallow-code-generation-from-strings` these helpers would
42+
* throw rather than silently no-op; the default `node --test` runner allows it.
3743
*/
3844
async function runRequestFn(
3945
code: string,
@@ -211,6 +217,40 @@ void describe('generated request fn — glob matching (regression: mid-segment w
211217
});
212218
assert.equal(selectedOrigin, ORIGIN_ID.s3);
213219
});
220+
221+
void it('does NOT let a single-level * cross a segment: /api/*/data ⊄ /api/foo/bar/data', async () => {
222+
// Locks in the single-segment guarantee. The matched route is STATIC (S3);
223+
// an unmatched path on this compute deploy falls through to the implicit
224+
// default (server). So a single-segment middle resolves to S3, and a
225+
// two-segment middle — if the wildcard wrongly crossed '/' — would ALSO hit
226+
// S3; it must instead miss and fall through to the server origin.
227+
const m = baseManifest({
228+
routes: [
229+
{ pattern: '/api/*/data', target: 'static' },
230+
{ pattern: '/*', target: 'compute' }, // implicit catch-all (server)
231+
],
232+
});
233+
const e = buildKvsEntries({
234+
manifest: m,
235+
buildId: 'b1',
236+
hasServer: true,
237+
hasImage: false,
238+
});
239+
// One middle segment → matches the single-level wildcard → S3.
240+
const hit = await runRequestFn(code, e, {
241+
uri: '/api/foo/data',
242+
headers: { host: { value: 'x.test' } },
243+
cookies: {},
244+
});
245+
assert.equal(hit.selectedOrigin, ORIGIN_ID.s3);
246+
// Two middle segments → must NOT match → falls through to the server origin.
247+
const miss = await runRequestFn(code, e, {
248+
uri: '/api/foo/bar/data',
249+
headers: { host: { value: 'x.test' } },
250+
cookies: {},
251+
});
252+
assert.equal(miss.selectedOrigin, ORIGIN_ID.server);
253+
});
214254
});
215255

216256
void describe('generated request fn — image origin basePath strip', () => {

packages/hosting/src/constructs/kvs_router.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -394,10 +394,13 @@ function matchPattern(uri, pattern) {
394394
return null;
395395
}
396396
// General path: glob with '*' anywhere (incl. mid-segment, e.g.
397-
// /api/*/admin). Each '*' matches a run of any chars EXCEPT '/', mirroring
398-
// CloudFront's path-pattern semantics; a trailing '*' matches the rest
399-
// (including '/'). Implemented as a literal scan (no regex — CloudFront
400-
// Functions JS forbids dynamic RegExp from strings reliably).
397+
// /api/*/admin). This is OUR framework-route matcher, NOT CloudFront behavior
398+
// selection: a non-trailing '*' matches a run of any chars EXCEPT '/' (a
399+
// SINGLE path segment), so '/api/*/data' matches '/api/foo/data' but NOT
400+
// '/api/foo/bar/data'. (CloudFront's own path patterns differ — there '*'
401+
// crosses '/'.) A trailing '*' matches the rest, including '/'. Implemented as
402+
// a literal scan (no regex — CloudFront Functions JS forbids dynamic RegExp
403+
// from strings reliably).
401404
return globMatch(uri, pattern);
402405
}
403406
function globMatch(uri, pattern) {

packages/hosting/src/constructs/node_runtime.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,16 +6,19 @@ import { Runtime } from 'aws-cdk-lib/aws-lambda';
66
/**
77
* Single source of truth for the Node.js Lambda runtime used by the hosting
88
* package's OWN Lambda functions (the KVS-writer custom resource, the ISR
9-
* cache-seed provider, etc.). Set every `Function` / `NodejsFunction` runtime
10-
* to this rather than hardcoding `Runtime.NODEJS_*_X`, so the package moves in
11-
* lockstep when the runtime is bumped.
9+
* cache-seed provider, etc.). Set every such `Function` runtime to this rather
10+
* than hardcoding `Runtime.NODEJS_*_X`, so the package moves in lockstep when
11+
* the runtime is bumped.
12+
*
13+
* Scope: this governs ONLY hosting-owned handlers. It deliberately does NOT
14+
* govern the SSR/edge bundles the adapters emit (Astro/Nuxt/OpenNext), which
15+
* pin `nodejs20.x` to match the Node version the framework compiled the bundle
16+
* against — bumping those independently of the bundle would break them. That is
17+
* why the adapters emit a literal rather than importing this constant.
1218
*
1319
* Mirrors `@aws-blocks/core`'s `DEFAULT_NODE_RUNTIME`; kept local because
1420
* hosting does not depend on core. Bump both together. This controls only the
1521
* AWS-managed runtime that executes deployed handlers — independent of the Node
1622
* version the CLI / CDK synth runs on.
1723
*/
1824
export const DEFAULT_NODE_RUNTIME = Runtime.NODEJS_24_X;
19-
20-
/** String form (`'nodejs24.x'`) for adapters that emit a runtime literal. */
21-
export const DEFAULT_NODE_RUNTIME_STRING = DEFAULT_NODE_RUNTIME.name;

0 commit comments

Comments
 (0)