Skip to content

Commit 618de20

Browse files
ddecrulleddecrulle
authored
fix(s3-policy): correctly remove empty s3:prefix condition (#947)
1 parent 0bfe159 commit 618de20

File tree

2 files changed

+322
-22
lines changed

2 files changed

+322
-22
lines changed

‎web/src/core/adapters/s3Client/utils/bucketPolicy.test.ts

Lines changed: 261 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
1+
import { it, expect, describe } from "vitest";
2+
import { symToStr } from "tsafe/symToStr";
3+
import { removeObjectNameFromListBucketCondition } from "./bucketPolicy";
4+
import { S3BucketPolicy } from "core/ports/S3Client";
5+
6+
describe(symToStr({ removeObjectNameFromListBucketCondition }), () => {
7+
it("One object", () => {
8+
const bucketArn = "bucketArn";
9+
const objectName = "objectName";
10+
const statements: S3BucketPolicy["Statement"] = [
11+
{
12+
Action: ["s3:ListBucket"],
13+
Condition: {
14+
StringEquals: {
15+
"s3:prefix": [objectName]
16+
}
17+
},
18+
Effect: "Allow",
19+
Principal: {
20+
AWS: ["*"]
21+
},
22+
Resource: [bucketArn]
23+
}
24+
];
25+
26+
const expected: S3BucketPolicy["Statement"] = [];
27+
28+
expect(
29+
removeObjectNameFromListBucketCondition(statements, bucketArn, objectName)
30+
).toStrictEqual(expected);
31+
});
32+
33+
it("Two objects", () => {
34+
const bucketArn = "bucketArn";
35+
const objectName = "objectName";
36+
const statements: S3BucketPolicy["Statement"] = [
37+
{
38+
Action: ["s3:ListBucket"],
39+
Condition: {
40+
StringEquals: {
41+
"s3:prefix": [objectName, "objectName2"]
42+
}
43+
},
44+
Effect: "Allow",
45+
Principal: {
46+
AWS: ["*"]
47+
},
48+
Resource: [bucketArn]
49+
}
50+
];
51+
52+
const expected: S3BucketPolicy["Statement"] = [
53+
{
54+
Action: ["s3:ListBucket"],
55+
Condition: {
56+
StringEquals: {
57+
"s3:prefix": ["objectName2"]
58+
}
59+
},
60+
Effect: "Allow",
61+
Principal: {
62+
AWS: ["*"]
63+
},
64+
Resource: [bucketArn]
65+
}
66+
];
67+
68+
expect(
69+
removeObjectNameFromListBucketCondition(statements, bucketArn, objectName)
70+
).toStrictEqual(expected);
71+
});
72+
73+
it("No matching objectName in s3:prefix, return unchanged", () => {
74+
const bucketArn = "bucketArn";
75+
const objectName = "nonexistent";
76+
const statements: S3BucketPolicy["Statement"] = [
77+
{
78+
Action: ["s3:ListBucket"],
79+
Condition: {
80+
StringEquals: {
81+
"s3:prefix": ["objectName1", "objectName2"]
82+
}
83+
},
84+
Effect: "Allow",
85+
Principal: {
86+
AWS: ["*"]
87+
},
88+
Resource: [bucketArn]
89+
}
90+
];
91+
92+
expect(
93+
removeObjectNameFromListBucketCondition(statements, bucketArn, objectName)
94+
).toStrictEqual(statements);
95+
});
96+
97+
it("Multiple statements, only one affected", () => {
98+
const bucketArn = "bucketArn";
99+
const objectName = "objectName";
100+
const statements: S3BucketPolicy["Statement"] = [
101+
{
102+
Action: ["s3:ListBucket"],
103+
Condition: {
104+
StringEquals: {
105+
"s3:prefix": [objectName, "objectName2"]
106+
}
107+
},
108+
Effect: "Allow",
109+
Principal: {
110+
AWS: ["*"]
111+
},
112+
Resource: [bucketArn]
113+
},
114+
{
115+
Action: ["s3:ListBucket"],
116+
Condition: {
117+
StringEquals: {
118+
"s3:prefix": ["otherDir"]
119+
}
120+
},
121+
Effect: "Allow",
122+
Principal: {
123+
AWS: ["*"]
124+
},
125+
Resource: [bucketArn]
126+
}
127+
];
128+
129+
const expected: S3BucketPolicy["Statement"] = [
130+
{
131+
Action: ["s3:ListBucket"],
132+
Condition: {
133+
StringEquals: {
134+
"s3:prefix": ["objectName2"]
135+
}
136+
},
137+
Effect: "Allow",
138+
Principal: {
139+
AWS: ["*"]
140+
},
141+
Resource: [bucketArn]
142+
},
143+
{
144+
Action: ["s3:ListBucket"],
145+
Condition: {
146+
StringEquals: {
147+
"s3:prefix": ["otherDir"]
148+
}
149+
},
150+
Effect: "Allow",
151+
Principal: {
152+
AWS: ["*"]
153+
},
154+
Resource: [bucketArn]
155+
}
156+
];
157+
158+
expect(
159+
removeObjectNameFromListBucketCondition(statements, bucketArn, objectName)
160+
).toStrictEqual(expected);
161+
});
162+
163+
it("Statement without StringEquals remains unchanged", () => {
164+
const bucketArn = "bucketArn";
165+
const objectName = "objectName";
166+
const statements: S3BucketPolicy["Statement"] = [
167+
{
168+
Action: ["s3:ListBucket"],
169+
Condition: {
170+
StringLike: {
171+
"s3:prefix": ["objectName"]
172+
}
173+
},
174+
Effect: "Allow",
175+
Principal: {
176+
AWS: ["*"]
177+
},
178+
Resource: [bucketArn]
179+
}
180+
];
181+
182+
expect(
183+
removeObjectNameFromListBucketCondition(statements, bucketArn, objectName)
184+
).toStrictEqual(statements);
185+
});
186+
187+
it("Condition is removed but other conditions exist, keep statement", () => {
188+
const bucketArn = "bucketArn";
189+
const objectName = "objectName";
190+
const statements: S3BucketPolicy["Statement"] = [
191+
{
192+
Action: ["s3:ListBucket"],
193+
Condition: {
194+
StringEquals: {
195+
"s3:prefix": [objectName]
196+
},
197+
NumericEquals: {
198+
"aws:MultiFactorAuthAge": 300
199+
}
200+
},
201+
Effect: "Allow",
202+
Principal: {
203+
AWS: ["*"]
204+
},
205+
Resource: [bucketArn]
206+
}
207+
];
208+
209+
const expected: S3BucketPolicy["Statement"] = [
210+
{
211+
Action: ["s3:ListBucket"],
212+
Condition: {
213+
NumericEquals: {
214+
"aws:MultiFactorAuthAge": 300
215+
}
216+
},
217+
Effect: "Allow",
218+
Principal: {
219+
AWS: ["*"]
220+
},
221+
Resource: [bucketArn]
222+
}
223+
];
224+
225+
expect(
226+
removeObjectNameFromListBucketCondition(statements, bucketArn, objectName)
227+
).toStrictEqual(expected);
228+
});
229+
230+
it("Empty statements array, should return empty array", () => {
231+
expect(
232+
removeObjectNameFromListBucketCondition([], "bucketArn", "objectName")
233+
).toStrictEqual([]);
234+
});
235+
236+
it("Bucket ARN does not match, should return unchanged", () => {
237+
const statements: S3BucketPolicy["Statement"] = [
238+
{
239+
Action: ["s3:ListBucket"],
240+
Condition: {
241+
StringEquals: {
242+
"s3:prefix": ["objectName"]
243+
}
244+
},
245+
Effect: "Allow",
246+
Principal: {
247+
AWS: ["*"]
248+
},
249+
Resource: ["otherBucketArn"]
250+
}
251+
];
252+
253+
expect(
254+
removeObjectNameFromListBucketCondition(
255+
statements,
256+
"bucketArn",
257+
"object@Name"
258+
)
259+
).toStrictEqual(statements);
260+
});
261+
});

‎web/src/core/adapters/s3Client/utils/bucketPolicy.ts

Lines changed: 61 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -88,30 +88,57 @@ export const removeObjectNameFromListBucketCondition = (
8888
return null;
8989
}
9090

91-
return statements.map(statement => {
92-
if (
93-
statement.Action.includes("s3:ListBucket") &&
94-
statement.Resource.includes(bucketArn)
95-
) {
96-
const updatedPrefixCondition = statement.Condition?.StringEquals?.[
97-
"s3:prefix"
98-
]?.filter((prefix: string) => prefix !== objectName);
91+
return statements
92+
.map(statement => {
93+
if (
94+
statement.Action.includes("s3:ListBucket") &&
95+
statement.Resource.includes(bucketArn)
96+
) {
97+
const updatedPrefixCondition: string[] =
98+
statement.Condition?.StringEquals?.["s3:prefix"]?.filter(
99+
(prefix: string) => prefix !== objectName
100+
) ?? [];
101+
102+
if (updatedPrefixCondition.length > 0) {
103+
return {
104+
...statement,
105+
Condition: {
106+
...statement.Condition,
107+
StringEquals: {
108+
...statement.Condition?.StringEquals,
109+
"s3:prefix": updatedPrefixCondition
110+
}
111+
}
112+
};
113+
}
99114

100-
return {
101-
...statement,
102-
Condition: {
103-
...statement.Condition,
104-
StringEquals: {
105-
...statement.Condition?.StringEquals,
106-
...(updatedPrefixCondition?.length
107-
? { "s3:prefix": updatedPrefixCondition }
108-
: {})
109-
}
115+
const updatedStringEquals = removeKey(
116+
statement.Condition?.StringEquals,
117+
"s3:prefix"
118+
);
119+
120+
// If "StringEquals" is still valid, return updated statement
121+
if (updatedStringEquals) {
122+
return {
123+
...statement,
124+
Condition: {
125+
...statement.Condition,
126+
StringEquals: updatedStringEquals
127+
}
128+
};
110129
}
111-
};
112-
}
113-
return statement;
114-
});
130+
131+
// Remove "StringEquals" from Condition
132+
const updatedCondition = removeKey(statement.Condition, "StringEquals");
133+
134+
// If Condition is still valid, return updated statement
135+
return updatedCondition
136+
? { ...statement, Condition: updatedCondition }
137+
: undefined;
138+
}
139+
return statement;
140+
})
141+
.filter(statement => statement !== undefined);
115142
};
116143

117144
// Adds a new `s3:GetObject` statement for `resourceArn`
@@ -190,3 +217,15 @@ export const removeResourceArnInGetObjectStatement = (
190217
)
191218
: statements.filter((_, index) => index !== existingStatementIndex);
192219
};
220+
221+
const removeKey = <T extends Record<string, any>, K extends keyof T>(
222+
obj: T | undefined,
223+
key: K
224+
): Omit<T, K> | undefined => {
225+
if (!obj || !(key in obj)) {
226+
return obj;
227+
}
228+
229+
const { [key]: _, ...rest } = obj;
230+
return Object.keys(rest).length > 0 ? rest : undefined;
231+
};

0 commit comments

Comments
 (0)