Skip to content

Commit 6d6b3b9

Browse files
committed
review round 2: errorId-before-log, least-priv default, optional schema, eager imports
- mapErrorToResponse: appendKeys({ errorId }) now runs before logger.error so the ERROR line itself carries the errorId the client sees. - GiantsFunction tableAccess default flipped from 'readwrite' to 'read'. Least privilege by default; handlers that write opt into 'readwrite'. - ConfigProvider.getConfig and SecretsProvider.getSecret accept an optional structural Schema<T> validator (matches Zod/Valibot/ArkType parse shape). Cements the runtime-validation counterpart of the JSON schema CDK already registers with AppConfig. - Config and Secrets powertools calls are now static imports, matching Telemetry. No more dynamic-import inconsistency across the three.
1 parent 9a45845 commit 6d6b3b9

11 files changed

Lines changed: 198 additions & 62 deletions

File tree

infra/stack.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,9 @@ export class Example extends Stack {
7373
path: 'hello',
7474
description: 'Hello world',
7575
memory: 128,
76-
tableAccess: 'read',
7776
timeout: 10,
77+
// Defaults to read-only DynamoDB access. Handlers that need to
78+
// write should set `tableAccess: 'readwrite'`.
7879
},
7980
];
8081

packages/cdk/GiantsFunction.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -112,11 +112,14 @@ export class GiantsFunction extends NodejsFunction {
112112
}
113113

114114
if (props.table) {
115-
const access = props.tableAccess ?? 'readwrite';
116-
if (access === 'read') {
117-
props.table.grantReadData(this);
118-
} else {
115+
// Default to read-only — least privilege wins unless the handler
116+
// actively needs to write. Opt in to 'readwrite' per-route when it
117+
// does.
118+
const access = props.tableAccess ?? 'read';
119+
if (access === 'readwrite') {
119120
props.table.grantReadWriteData(this);
121+
} else {
122+
props.table.grantReadData(this);
120123
}
121124
}
122125
}

packages/cdk/tests/GiantsFunction.test.ts

Lines changed: 41 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,28 @@ describe('GiantsFunction', () => {
131131
});
132132
});
133133

134-
test('grants DynamoDB read/write by default when table prop is set', () => {
134+
const collectActions = (template: Template): Set<string> => {
135+
const policies = template.findResources('AWS::IAM::Policy');
136+
const actions = new Set<string>();
137+
for (const policy of Object.values(policies)) {
138+
const statements = (
139+
policy as {
140+
Properties: {
141+
PolicyDocument: { Statement: Array<{ Action: string | string[] }> };
142+
};
143+
}
144+
).Properties.PolicyDocument.Statement;
145+
for (const statement of statements) {
146+
const raw = statement.Action;
147+
for (const action of Array.isArray(raw) ? raw : [raw]) {
148+
actions.add(action);
149+
}
150+
}
151+
}
152+
return actions;
153+
};
154+
155+
test('grants read-only DynamoDB by default when a table is set', () => {
135156
const app = new App();
136157
const stack = new Stack(app, 'TestStack');
137158
const table = new Table(stack, 'TestTable', {
@@ -143,23 +164,14 @@ describe('GiantsFunction', () => {
143164
table,
144165
});
145166
const template = Template.fromStack(stack);
167+
const actions = collectActions(template);
146168

147-
template.hasResourceProperties('AWS::IAM::Policy', {
148-
PolicyDocument: {
149-
Statement: Match.arrayWith([
150-
Match.objectLike({
151-
Action: Match.arrayWith([
152-
'dynamodb:BatchGetItem',
153-
'dynamodb:GetItem',
154-
'dynamodb:PutItem',
155-
]),
156-
}),
157-
]),
158-
},
159-
});
169+
expect(actions.has('dynamodb:GetItem')).toBe(true);
170+
expect(actions.has('dynamodb:PutItem')).toBe(false);
171+
expect(actions.has('dynamodb:DeleteItem')).toBe(false);
160172
});
161173

162-
test('grants read-only DynamoDB access when tableAccess is "read"', () => {
174+
test('grants DynamoDB read/write when tableAccess is "readwrite"', () => {
163175
const app = new App();
164176
const stack = new Stack(app, 'TestStack');
165177
const table = new Table(stack, 'TestTable', {
@@ -169,30 +181,23 @@ describe('GiantsFunction', () => {
169181
appConfigApplication: 'example',
170182
entry: new URL('fixtures/handler.ts', import.meta.url).pathname,
171183
table,
172-
tableAccess: 'read',
184+
tableAccess: 'readwrite',
173185
});
174186
const template = Template.fromStack(stack);
175187

176-
const policies = template.findResources('AWS::IAM::Policy');
177-
const actions = new Set<string>();
178-
for (const policy of Object.values(policies)) {
179-
const doc = (
180-
policy as {
181-
Properties: {
182-
PolicyDocument: { Statement: Array<{ Action: string | string[] }> };
183-
};
184-
}
185-
).Properties.PolicyDocument.Statement;
186-
for (const statement of doc) {
187-
const raw = statement.Action;
188-
for (const action of Array.isArray(raw) ? raw : [raw]) {
189-
actions.add(action);
190-
}
191-
}
192-
}
193-
expect(actions.has('dynamodb:GetItem')).toBe(true);
194-
expect(actions.has('dynamodb:PutItem')).toBe(false);
195-
expect(actions.has('dynamodb:DeleteItem')).toBe(false);
188+
template.hasResourceProperties('AWS::IAM::Policy', {
189+
PolicyDocument: {
190+
Statement: Match.arrayWith([
191+
Match.objectLike({
192+
Action: Match.arrayWith([
193+
'dynamodb:BatchGetItem',
194+
'dynamodb:GetItem',
195+
'dynamodb:PutItem',
196+
]),
197+
}),
198+
]),
199+
},
200+
});
196201
});
197202

198203
test('does not hardcode a functionName (avoids cross-stack collisions)', () => {

packages/config/config.ts

Lines changed: 24 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,34 @@
1+
import { getAppConfig } from '@aws-lambda-powertools/parameters/appconfig';
2+
3+
/**
4+
* Structural schema type — matches Zod, Valibot, ArkType, or any validator
5+
* that exposes a `parse(unknown): T` method.
6+
*/
7+
export interface Schema<T> {
8+
parse(input: unknown): T;
9+
}
10+
111
export interface ConfigProvider {
2-
getConfig<Configuration>(profileName: string): Promise<Configuration>;
12+
getConfig<Configuration>(
13+
profileName: string,
14+
schema?: Schema<Configuration>,
15+
): Promise<Configuration>;
316
}
417

518
export interface PowertoolsConfigOptions {
619
environment?: string;
720
maxAge?: number;
821
}
922

23+
const validate = <Configuration>(
24+
raw: unknown,
25+
schema: Schema<Configuration> | undefined,
26+
): Configuration => (schema ? schema.parse(raw) : (raw as Configuration));
27+
1028
export const localConfig = (): ConfigProvider => ({
1129
getConfig: async <Configuration>(
1230
profileName: string,
31+
schema?: Schema<Configuration>,
1332
): Promise<Configuration> => {
1433
const key = `CONFIG_${profileName.toUpperCase().replaceAll('-', '_')}`;
1534
const value = process.env[key];
@@ -18,7 +37,7 @@ export const localConfig = (): ConfigProvider => ({
1837
throw new Error(`Missing environment variable: ${key}`);
1938
}
2039

21-
return JSON.parse(value) as Configuration;
40+
return validate(JSON.parse(value), schema);
2241
},
2342
});
2443

@@ -28,21 +47,18 @@ export const powertoolsConfig = (
2847
): ConfigProvider => ({
2948
getConfig: async <Configuration>(
3049
profileName: string,
50+
schema?: Schema<Configuration>,
3151
): Promise<Configuration> => {
32-
const { getAppConfig } = await import(
33-
'@aws-lambda-powertools/parameters/appconfig'
34-
);
35-
3652
const environment =
3753
options.environment ?? process.env.APPCONFIG_ENVIRONMENT ?? 'default';
3854

39-
const config = await getAppConfig(profileName, {
55+
const raw = await getAppConfig(profileName, {
4056
application,
4157
environment,
4258
maxAge: options.maxAge ?? 300,
4359
transform: 'json',
4460
});
4561

46-
return config as Configuration;
62+
return validate(raw, schema);
4763
},
4864
});

packages/config/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,5 @@ export {
33
localConfig,
44
type PowertoolsConfigOptions,
55
powertoolsConfig,
6+
type Schema,
67
} from './config.ts';

packages/config/tests/config.test.ts

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,4 +107,52 @@ describe('powertoolsConfig', () => {
107107
expect.objectContaining({ maxAge: 30 }),
108108
);
109109
});
110+
111+
test('runs the raw value through an optional schema', async () => {
112+
const raw = { greeting: 'hi' };
113+
mockGetAppConfig.mockResolvedValueOnce(raw);
114+
const schema = {
115+
parse: mock((input: unknown) => input as typeof raw),
116+
};
117+
const result = await powertoolsConfig('example').getConfig(
118+
'profile',
119+
schema,
120+
);
121+
expect(schema.parse).toHaveBeenCalledWith(raw);
122+
expect(result).toEqual(raw);
123+
});
124+
125+
test('propagates schema parse errors', async () => {
126+
mockGetAppConfig.mockResolvedValueOnce({ greeting: 42 });
127+
const schema = {
128+
parse: () => {
129+
throw new Error('bad shape');
130+
},
131+
};
132+
await expect(
133+
powertoolsConfig('example').getConfig('profile', schema),
134+
).rejects.toThrow('bad shape');
135+
});
136+
});
137+
138+
describe('localConfig with schema', () => {
139+
const original = process.env.CONFIG_EXAMPLE;
140+
141+
afterEach(() => {
142+
if (original === undefined) {
143+
delete process.env.CONFIG_EXAMPLE;
144+
} else {
145+
process.env.CONFIG_EXAMPLE = original;
146+
}
147+
});
148+
149+
test('validates via schema.parse when supplied', async () => {
150+
process.env.CONFIG_EXAMPLE = JSON.stringify({ greeting: 'hi' });
151+
const schema = {
152+
parse: mock((input: unknown) => input as { greeting: string }),
153+
};
154+
const result = await localConfig().getConfig('example', schema);
155+
expect(schema.parse).toHaveBeenCalledWith({ greeting: 'hi' });
156+
expect(result).toEqual({ greeting: 'hi' });
157+
});
110158
});

packages/errors/map-error-to-response.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,11 +23,11 @@ export const mapErrorToResponse = (
2323
): ErrorResponse => {
2424
const errorId = randomUUID();
2525

26-
logger.error('mapErrorToResponse', error as Error);
27-
// Append errorId so the wide-event emitted at end-of-invocation carries
28-
// the same id that's in the response body — lets clients quote errorId
29-
// when asking for diagnostics.
26+
// Append errorId before logging so the ERROR line itself carries the id
27+
// the client will see in the response body — plus every subsequent log
28+
// in the invocation, including the end-of-invocation wide-event.
3029
logger.appendKeys({ errorId });
30+
logger.error('mapErrorToResponse', error as Error);
3131

3232
return respond(500, {
3333
errorId,

packages/errors/tests/errors.test.ts

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,24 @@ describe('mapErrorToResponse', () => {
3434
const body = JSON.parse(res.body);
3535
expect(logger.appendKeys).toHaveBeenCalledWith({ errorId: body.errorId });
3636
});
37+
38+
test('appends errorId BEFORE logging so the ERROR line carries it too', () => {
39+
const order: string[] = [];
40+
const logger: TelemetryLogger = {
41+
debug: mock(() => {}),
42+
info: mock(() => {}),
43+
warn: mock(() => {}),
44+
resetKeys: mock(() => {}),
45+
appendKeys: mock(() => {
46+
order.push('appendKeys');
47+
}),
48+
error: mock(() => {
49+
order.push('error');
50+
}),
51+
};
52+
mapErrorToResponse(new Error('boom'), logger);
53+
expect(order).toEqual(['appendKeys', 'error']);
54+
});
3755
});
3856

3957
describe('validationErrorResponse', () => {

packages/secrets/index.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1,5 @@
1-
export { powertoolsSecrets, type SecretsProvider } from './secrets.ts';
1+
export {
2+
powertoolsSecrets,
3+
type Schema,
4+
type SecretsProvider,
5+
} from './secrets.ts';

packages/secrets/secrets.ts

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,30 @@
1+
import { getSecret } from '@aws-lambda-powertools/parameters/secrets';
2+
3+
/**
4+
* Structural schema type — matches Zod, Valibot, ArkType, or any validator
5+
* that exposes a `parse(unknown): T` method.
6+
*/
7+
export interface Schema<T> {
8+
parse(input: unknown): T;
9+
}
10+
111
export interface SecretsProvider {
2-
getSecret<Credentials>(secretName: string): Promise<Credentials>;
12+
getSecret<Credentials>(
13+
secretName: string,
14+
schema?: Schema<Credentials>,
15+
): Promise<Credentials>;
316
}
417

518
export const powertoolsSecrets = (maxAge = 300): SecretsProvider => ({
6-
getSecret: async <Credentials>(secretName: string): Promise<Credentials> => {
7-
const { getSecret } = await import(
8-
'@aws-lambda-powertools/parameters/secrets'
9-
);
10-
11-
const secret = await getSecret(secretName, {
19+
getSecret: async <Credentials>(
20+
secretName: string,
21+
schema?: Schema<Credentials>,
22+
): Promise<Credentials> => {
23+
const raw = await getSecret(secretName, {
1224
maxAge,
1325
transform: 'json',
1426
});
1527

16-
return secret as Credentials;
28+
return schema ? schema.parse(raw) : (raw as Credentials);
1729
},
1830
});

0 commit comments

Comments
 (0)