Skip to content

Commit eb0e633

Browse files
bert-ebert-e
committed
Merge branch 'bugfix/ARSN-553/add_missing_xml_escapes' into tmp/octopus/w/8.3/bugfix/ARSN-553/add_missing_xml_escapes
2 parents b143ef7 + 2ff8e72 commit eb0e633

File tree

6 files changed

+188
-9
lines changed

6 files changed

+188
-9
lines changed

‎lib/models/BucketLoggingStatus.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { parseString } from 'xml2js';
22
import errors, { ArsenalError, errorInstances } from '../errors';
3+
import escapeForXml from '../s3middleware/escapeForXml';
34

45
/** BucketLoggingStatus constants, not documented by AWS but found via testing */
56
const TARGET_BUCKET_MIN_LENGTH = 3;
@@ -10,7 +11,7 @@ const TARGET_PREFIX_MAX_LENGTH = 800;
1011
* Format of xml request:
1112
* https://docs.aws.amazon.com/AmazonS3/latest/API/API_LoggingEnabled.html
1213
* https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutBucketLogging.html
13-
*
14+
*
1415
<?xml version="1.0" encoding="UTF-8"?>
1516
<BucketLoggingStatus xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
1617
<LoggingEnabled>
@@ -61,7 +62,7 @@ export default class BucketLoggingStatus {
6162
if (this._loggingEnabled) {
6263
loggingEnabledXML = `<LoggingEnabled>
6364
<TargetBucket>${this._loggingEnabled.TargetBucket}</TargetBucket>
64-
<TargetPrefix>${this._loggingEnabled.TargetPrefix}</TargetPrefix>
65+
<TargetPrefix>${escapeForXml(this._loggingEnabled.TargetPrefix)}</TargetPrefix>
6566
</LoggingEnabled>
6667
`;
6768
}

‎lib/models/LifecycleConfiguration.ts‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1250,13 +1250,13 @@ export default class LifecycleConfiguration {
12501250
}
12511251
const tags = filter && filter.tags;
12521252
const Prefix = rulePrefix !== undefined ?
1253-
`<Prefix>${rulePrefix}</Prefix>` : '';
1253+
`<Prefix>${escapeForXml(rulePrefix)}</Prefix>` : '';
12541254
let tagXML = '';
12551255
if (tags) {
12561256
tagXML = tags.map(t => {
12571257
const { key, val } = t;
1258-
const Tag = `<Tag><Key>${key}</Key>` +
1259-
`<Value>${val}</Value></Tag>`;
1258+
const Tag = `<Tag><Key>${escapeForXml(key)}</Key>` +
1259+
`<Value>${escapeForXml(val)}</Value></Tag>`;
12601260
return Tag;
12611261
}).join('');
12621262
}

‎lib/models/NotificationConfiguration.ts‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import {
77
} from '../constants';
88
import { errorInstances } from '../errors';
99
import type { ArsenalError } from '../errors';
10+
import escapeForXml from '../s3middleware/escapeForXml';
1011

1112
/**
1213
* Format of xml request:
@@ -332,16 +333,16 @@ export default class NotificationConfiguration {
332333
if (config && config.queueConfig) {
333334
config.queueConfig.forEach(c => {
334335
xmlArray.push('<QueueConfiguration>');
335-
xmlArray.push(`<Id>${c.id}</Id>`);
336-
xmlArray.push(`<Queue>${c.queueArn}</Queue>`);
336+
xmlArray.push(`<Id>${escapeForXml(c.id)}</Id>`);
337+
xmlArray.push(`<Queue>${escapeForXml(c.queueArn)}</Queue>`);
337338
c.events.forEach(e => {
338339
xmlArray.push(`<Event>${e}</Event>`);
339340
});
340341
if (c.filterRules) {
341342
xmlArray.push('<Filter><S3Key>');
342343
c.filterRules.forEach(r => {
343-
xmlArray.push(`<FilterRule><Name>${r.name}</Name>` +
344-
`<Value>${r.value}</Value></FilterRule>`);
344+
xmlArray.push(`<FilterRule><Name>${escapeForXml(r.name)}</Name>` +
345+
`<Value>${escapeForXml(r.value)}</Value></FilterRule>`);
345346
});
346347
xmlArray.push('</S3Key></Filter>');
347348
}

‎tests/unit/models/BucketLoggingStatus.spec.js‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -306,5 +306,29 @@ describe('BucketLoggingStatus', () => {
306306
assert.strictEqual(result.res.getLoggingEnabled(), undefined);
307307
});
308308
});
309+
310+
describe('XML escaping for special characters', () => {
311+
const specialCharacters = ['&', '<', '>', '"', "'"];
312+
313+
specialCharacters.forEach(char =>
314+
it(`should escape \`${char}\` in TargetPrefix and generate valid XML`, done => {
315+
const loggingEnabled = {
316+
TargetBucket: 'test-bucket',
317+
TargetPrefix: `logs/app${char}env/`,
318+
};
319+
const config = new BucketLoggingStatus(loggingEnabled);
320+
const xml = config.toXML();
321+
322+
// Verify the XML is valid and the character roundtrips by parsing it
323+
parseString(xml, { explicitArray: false }, (err, result) => {
324+
assert.ifError(err);
325+
assert(result.BucketLoggingStatus);
326+
const logging = result.BucketLoggingStatus.LoggingEnabled;
327+
assert.strictEqual(logging.TargetPrefix, `logs/app${char}env/`);
328+
done();
329+
});
330+
})
331+
);
332+
});
309333
});
310334
});

‎tests/unit/models/LifecycleConfiguration.spec.js‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1396,3 +1396,85 @@ describe('LifecycleConfiguration::getConfigJson', () => {
13961396
);
13971397
}));
13981398
});
1399+
1400+
describe('LifecycleConfiguration.getConfigXml - XML escaping for special characters', () => {
1401+
const specialCharacters = ['&', '<', '>', '"', "'"];
1402+
1403+
specialCharacters.forEach(char => {
1404+
it(`should escape \`${char}\` in rule ID and generate valid XML`, done => {
1405+
const config = {
1406+
rules: [{
1407+
ruleID: `test-id${char}value`,
1408+
ruleStatus: 'Enabled',
1409+
prefix: 'logs/',
1410+
actions: [{
1411+
actionName: 'Expiration',
1412+
days: 90,
1413+
}],
1414+
}],
1415+
};
1416+
1417+
const xml = LifecycleConfiguration.getConfigXml(config);
1418+
1419+
parseString(xml, (err, result) => {
1420+
assert.ifError(err);
1421+
const rule = result.LifecycleConfiguration.Rule[0];
1422+
assert.strictEqual(rule.ID[0], `test-id${char}value`);
1423+
done();
1424+
});
1425+
});
1426+
1427+
it(`should escape \`${char}\` in prefix`, done => {
1428+
const config = {
1429+
rules: [{
1430+
ruleID: 'test-id',
1431+
ruleStatus: 'Enabled',
1432+
prefix: `logs/${char}path/`,
1433+
actions: [{
1434+
actionName: 'Expiration',
1435+
days: 90,
1436+
}],
1437+
}],
1438+
};
1439+
1440+
const xml = LifecycleConfiguration.getConfigXml(config);
1441+
1442+
parseString(xml, (err, result) => {
1443+
assert.ifError(err);
1444+
const rule = result.LifecycleConfiguration.Rule[0];
1445+
assert.strictEqual(rule.Prefix[0], `logs/${char}path/`);
1446+
done();
1447+
});
1448+
});
1449+
1450+
it(`should escape \`${char}\` in tag key and value`, done => {
1451+
const config = {
1452+
rules: [{
1453+
ruleID: 'test-id',
1454+
ruleStatus: 'Enabled',
1455+
filter: {
1456+
tags: [{
1457+
key: `env${char}key`,
1458+
val: `value${char}data`,
1459+
}],
1460+
},
1461+
actions: [{
1462+
actionName: 'Expiration',
1463+
days: 90,
1464+
}],
1465+
}],
1466+
};
1467+
1468+
const xml = LifecycleConfiguration.getConfigXml(config);
1469+
1470+
parseString(xml, (err, result) => {
1471+
assert.ifError(err);
1472+
const rule = result.LifecycleConfiguration.Rule[0];
1473+
const tag = rule.Filter[0].Tag[0];
1474+
assert.strictEqual(tag.Key[0], `env${char}key`);
1475+
assert.strictEqual(tag.Value[0], `value${char}data`);
1476+
done();
1477+
});
1478+
});
1479+
});
1480+
});

‎tests/unit/models/NotificationConfiguration.spec.js‎

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,3 +290,74 @@ describe('NotificationConfiguration.restrictSupportedNotificationBasedOnLifecycl
290290
expect(supportedNotificationEvents.has('s3:ObjectCreated:*')).toBeTruthy();
291291
});
292292
});
293+
294+
describe('NotificationConfiguration.getConfigXML - XML escaping for special characters', () => {
295+
const specialCharacters = ['&', '<', '>', '"', "'"];
296+
297+
specialCharacters.forEach(char => {
298+
it(`should escape \`${char}\` in notification ID and generate valid XML`, done => {
299+
const config = {
300+
queueConfig: [{
301+
id: `test-id${char}value`,
302+
queueArn: 'arn:scality:bucketnotif:::target',
303+
events: ['s3:ObjectCreated:*'],
304+
filterRules: [],
305+
}],
306+
};
307+
308+
const xml = NotificationConfiguration.getConfigXML(config);
309+
310+
parseString(xml, (err, result) => {
311+
assert.ifError(err);
312+
const queueConfig = result.NotificationConfiguration.QueueConfiguration[0];
313+
assert.strictEqual(queueConfig.Id[0], `test-id${char}value`);
314+
done();
315+
});
316+
});
317+
318+
it(`should escape \`${char}\` in queue ARN`, done => {
319+
const config = {
320+
queueConfig: [{
321+
id: 'test-id',
322+
queueArn: `arn:scality:bucketnotif:::queue${char}name`,
323+
events: ['s3:ObjectCreated:*'],
324+
filterRules: [],
325+
}],
326+
};
327+
328+
const xml = NotificationConfiguration.getConfigXML(config);
329+
330+
parseString(xml, (err, result) => {
331+
assert.ifError(err);
332+
const queueConfig = result.NotificationConfiguration.QueueConfiguration[0];
333+
assert.strictEqual(queueConfig.Queue[0], `arn:scality:bucketnotif:::queue${char}name`);
334+
done();
335+
});
336+
});
337+
338+
it(`should escape \`${char}\` in filter rule name and value`, done => {
339+
const config = {
340+
queueConfig: [{
341+
id: 'test-id',
342+
queueArn: 'arn:scality:bucketnotif:::target',
343+
events: ['s3:ObjectCreated:*'],
344+
filterRules: [{
345+
name: `Prefix${char}Name`,
346+
value: `logs/${char}path`,
347+
}],
348+
}],
349+
};
350+
351+
const xml = NotificationConfiguration.getConfigXML(config);
352+
353+
parseString(xml, (err, result) => {
354+
assert.ifError(err);
355+
const queueConfig = result.NotificationConfiguration.QueueConfiguration[0];
356+
const filterRule = queueConfig.Filter[0].S3Key[0].FilterRule[0];
357+
assert.strictEqual(filterRule.Name[0], `Prefix${char}Name`);
358+
assert.strictEqual(filterRule.Value[0], `logs/${char}path`);
359+
done();
360+
});
361+
});
362+
});
363+
});

0 commit comments

Comments
 (0)