Skip to content

Commit b02fe63

Browse files
iqbalgdsiqbalgds
committed
PP-14390 Fix issues with CSP middleware
- Clean up files - Move the `hasSubstr` into the CSP middleware file as that is the only place it is being used. - Also combine the unit tests into the CSP middleware test file.
1 parent f9aebd0 commit b02fe63

3 files changed

Lines changed: 166 additions & 139 deletions

File tree

‎src/utils/index.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,4 +4,3 @@ module.exports.isValidEmail = require('./is-valid-email')
44
module.exports.fieldValidationChecks = require('./field-validation-checks')
55
module.exports.countries = require('./countries')
66
module.exports.currencyFormatter = require('./currency-formatter')
7-
module.exports.csp = require('./middleware/csp')

‎src/utils/middleware/csp.js‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,7 @@ const detectErrorsMiddleware = (logger) => {
3535
}
3636
}
3737
const captureEventMiddleware = (ignoredStrings, logger, Sentry) => {
38-
console.log('== 1 - pay-js-commons - captureEventMiddleware')
3938
return (req, res) => {
40-
console.log('== 2 - pay-js-commons - captureEventMiddleware')
41-
console.log('== 2 - pay-js-commons - logger: ', logger)
4239
let reports = undefined
4340
if (Array.isArray(req.body) && req.body.length > 0) {
4441
reports = req.body.filter(report => report.type === 'csp-violation') // new style reporting-api, can be batched into multiple reports
@@ -57,14 +54,15 @@ const captureEventMiddleware = (ignoredStrings, logger, Sentry) => {
5754
return res.status(400).end()
5855
} else {
5956
if (hasSubstr(ignoredStrings, blockedUri)) return res.status(204).end()
60-
// sentry.captureEvent({
61-
// message: `Blocked ${violatedDirective} from ${blockedUri}`,
62-
// level: 'warning',
63-
// extra: {
64-
// cspReport: body,
65-
// userAgent: userAgent
66-
// }
67-
// })
57+
58+
Sentry.captureEvent({
59+
message: `Blocked ${violatedDirective} from ${blockedUri}`,
60+
level: 'warning',
61+
extra: {
62+
cspReport: body,
63+
userAgent
64+
}
65+
})
6866
}
6967
})
7068
} else {
@@ -76,6 +74,7 @@ const captureEventMiddleware = (ignoredStrings, logger, Sentry) => {
7674
}
7775

7876
module.exports = {
77+
hasSubstr,
7978
rateLimitMiddleware,
8079
captureEventMiddleware,
8180
requestParseMiddleware,

‎src/utils/middleware/csp.test.js‎

Lines changed: 156 additions & 127 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@ const { expect } = require('chai')
77
const loggingSpy = { info: sinon.spy() }
88
const sentrySpy = { captureEvent: sinon.spy() }
99

10-
1110
const {
11+
hasSubstr,
1212
rateLimitMiddleware,
1313
captureEventMiddleware,
1414
requestParseMiddleware,
@@ -26,165 +26,194 @@ const cspMiddlewareStack = [
2626
sentrySpy)
2727
]
2828

29-
describe('CSP report endpoint', () => {
30-
const underTest = express()
31-
underTest.enable('trust proxy')
32-
underTest.set('trust proxy', 3)
33-
underTest.use('/test', cspMiddlewareStack)
29+
describe('CSP middleware', () => {
30+
describe('hasSubstr', () => {
31+
const testCase = 'lorem ipsum dolor sit amet'
32+
33+
it('returns true when substring is detected', () => {
34+
const lookupStrings = ['ipsum']
35+
const result = hasSubstr(lookupStrings, testCase)
36+
expect(result).to.equal(true)
37+
})
38+
39+
it('returns false when substring is not detected', () => {
40+
const lookupStrings = ['consectetur']
41+
const result = hasSubstr(lookupStrings, testCase)
42+
expect(result).to.equal(false)
43+
})
3444

35-
beforeEach(() => {
36-
loggingSpy.info.resetHistory()
37-
sentrySpy.captureEvent.resetHistory()
45+
it('ignores case when comparing', () => {
46+
const lookupStrings = ['DOLOR']
47+
const result = hasSubstr(lookupStrings, testCase)
48+
expect(result).to.equal(true)
49+
})
50+
51+
it('handles multiple lookup strings', () => {
52+
const lookupStrings = ['consectetur', 'LOREM']
53+
const result = hasSubstr(lookupStrings, testCase)
54+
expect(result).to.equal(true)
55+
})
3856
})
3957

40-
const validReportUriPayload = {
41-
'csp-report': {
42-
'document-uri': 'https://example.com/page-with-violation',
43-
'referrer': '',
44-
'violated-directive': 'script-src \'self\'',
45-
'effective-directive': 'script-src',
46-
'original-policy': 'default-src \'none\'; script-src \'self\'; object-src \'self\'; report-to: default',
47-
'disposition': 'enforce',
48-
'blocked-uri': 'https://evil.example.com/malicious.js',
49-
'line-number': 22,
50-
'column-number': 13,
51-
'source-file': 'https://example.com/script.js',
52-
'status-code': 200,
53-
'script-sample': ''
54-
}
55-
}
58+
describe('CSP report endpoint', () => {
59+
const underTest = express()
60+
underTest.enable('trust proxy')
61+
underTest.set('trust proxy', 3)
62+
underTest.use('/test', cspMiddlewareStack)
63+
64+
beforeEach(() => {
65+
loggingSpy.info.resetHistory()
66+
sentrySpy.captureEvent.resetHistory()
67+
})
5668

57-
const validReportingAPIPayload = [
58-
{
59-
'age': 2,
60-
'body': {
61-
'blockedURL': 'https://site2.example/script.js',
69+
const validReportUriPayload = {
70+
'csp-report': {
71+
'document-uri': 'https://example.com/page-with-violation',
72+
'referrer': '',
73+
'violated-directive': 'script-src \'self\'',
74+
'effective-directive': 'script-src',
75+
'original-policy': 'default-src \'none\'; script-src \'self\'; object-src \'self\'; report-to: default',
6276
'disposition': 'enforce',
63-
'documentURL': 'https://site.example',
64-
'effectiveDirective': 'script-src-elem',
65-
'originalPolicy': 'script-src \'self\'; object-src \'none\'; report-to main-endpoint;',
66-
'referrer': 'https://site.example',
67-
'sample': '',
68-
'statusCode': 200
69-
},
70-
'type': 'csp-violation',
71-
'url': 'https://site.example',
72-
'user_agent': 'Mozilla/5.0... Chrome/92.0.4504.0'
77+
'blocked-uri': 'https://evil.example.com/malicious.js',
78+
'line-number': 22,
79+
'column-number': 13,
80+
'source-file': 'https://example.com/script.js',
81+
'status-code': 200,
82+
'script-sample': ''
83+
}
7384
}
74-
]
7585

76-
const validPayloads = [
77-
{ arg: validReportUriPayload, expectedMessage: 'Blocked script-src \'self\' from https://evil.example.com/malicious.js', expectedReport: validReportUriPayload['csp-report'], type: 'report-uri' },
78-
{ arg: validReportingAPIPayload, expectedMessage: 'Blocked script-src-elem from https://site2.example/script.js', expectedReport: validReportingAPIPayload[0]['body'], type: 'reporting api' }
79-
]
86+
const validReportingAPIPayload = [
87+
{
88+
'age': 2,
89+
'body': {
90+
'blockedURL': 'https://site2.example/script.js',
91+
'disposition': 'enforce',
92+
'documentURL': 'https://site.example',
93+
'effectiveDirective': 'script-src-elem',
94+
'originalPolicy': 'script-src \'self\'; object-src \'none\'; report-to main-endpoint;',
95+
'referrer': 'https://site.example',
96+
'sample': '',
97+
'statusCode': 200
98+
},
99+
'type': 'csp-violation',
100+
'url': 'https://site.example',
101+
'user_agent': 'Mozilla/5.0... Chrome/92.0.4504.0'
102+
}
103+
]
104+
105+
const validPayloads = [
106+
{ arg: validReportUriPayload, expectedMessage: 'Blocked script-src \'self\' from https://evil.example.com/malicious.js', expectedReport: validReportUriPayload['csp-report'], type: 'report-uri' },
107+
{ arg: validReportingAPIPayload, expectedMessage: 'Blocked script-src-elem from https://site2.example/script.js', expectedReport: validReportingAPIPayload[0]['body'], type: 'reporting api' }
108+
]
109+
110+
validPayloads.forEach(test => {
111+
it(`should return 204 and send to sentry if a valid ${test.type} csp report is present`, async () => {
112+
await request(underTest)
113+
.post('/test')
114+
.set('user-agent', 'supertest')
115+
.send(test.arg)
116+
.expect(204)
117+
118+
expect(sentrySpy.captureEvent.calledOnce).to.be.true
119+
expect(sentrySpy.captureEvent.calledWith({
120+
message: test.expectedMessage,
121+
level: 'warning',
122+
extra: {
123+
'cspReport': test.expectedReport,
124+
'userAgent': 'supertest'
125+
}
126+
})).to.be.true
127+
})
128+
})
80129

81-
validPayloads.forEach(test => {
82-
it(`should return 204 and send to sentry if a valid ${test.type} csp report is present`, async () => {
130+
it('should return 204 and not send to sentry if a valid csp report is present but the blocked-uri is ignored', async () => {
83131
await request(underTest)
84132
.post('/test')
85133
.set('user-agent', 'supertest')
86-
.send(test.arg)
134+
.send({
135+
'csp-report': {
136+
'blocked-uri': 'https://www.banned.com',
137+
'violated-directive': 'connect-src'
138+
}
139+
})
87140
.expect(204)
88141

89-
expect(sentrySpy.captureEvent.calledOnce).to.be.true
90-
expect(sentrySpy.captureEvent.calledWith({
91-
message: test.expectedMessage,
92-
level: 'warning',
93-
extra: {
94-
'cspReport': test.expectedReport,
95-
'userAgent': 'supertest'
96-
}
97-
})).to.be.true
142+
expect(sentrySpy.captureEvent.notCalled).to.be.true
98143
})
99-
})
100144

101-
it('should return 204 and not send to sentry if a valid csp report is present but the blocked-uri is ignored', async () => {
102-
await request(underTest)
103-
.post('/test')
104-
.set('user-agent', 'supertest')
105-
.send({
106-
'csp-report': {
107-
'blocked-uri': 'https://www.banned.com',
108-
'violated-directive': 'connect-src'
109-
}
110-
})
111-
.expect(204)
145+
it('should return 400 if content-type is unexpected', async () => {
146+
await request(underTest)
147+
.post('/test')
148+
.set('content-type', 'application/x-www-form-urlencoded')
149+
.send(validReportUriPayload)
150+
.expect(400)
151+
})
112152

113-
expect(sentrySpy.captureEvent.notCalled).to.be.true
114-
})
153+
it('should return 400 if payload is too large', async () => {
115154

116-
it('should return 400 if content-type is unexpected', async () => {
117-
await request(underTest)
118-
.post('/test')
119-
.set('content-type', 'application/x-www-form-urlencoded')
120-
.send(validReportUriPayload)
121-
.expect(400)
122-
})
155+
const largePayload = {
156+
...validReportUriPayload,
157+
'large_value': 'some text here x1000'.repeat(1000)
158+
}
123159

124-
it('should return 400 if payload is too large', async () => {
160+
await request(underTest)
161+
.post('/test')
162+
.send(largePayload)
163+
.expect(400)
125164

126-
const largePayload = {
127-
...validReportUriPayload,
128-
'large_value': 'some text here x1000'.repeat(1000)
129-
}
165+
expect(loggingSpy.info.calledOnce).to.be.true
166+
expect(loggingSpy.info.calledWith('CSP violation request payload exceeds maximum size')).to.be.true
167+
})
130168

131-
await request(underTest)
132-
.post('/test')
133-
.send(largePayload)
134-
.expect(400)
169+
it('should return 400 if request is not JSON', async () => {
170+
await request(underTest)
171+
.post('/test')
172+
.set('content-type', 'application/json')
173+
.send('notJSON')
174+
.expect(400)
135175

136-
expect(loggingSpy.info.calledOnce).to.be.true
137-
expect(loggingSpy.info.calledWith('CSP violation request payload exceeds maximum size')).to.be.true
138-
})
176+
expect(loggingSpy.info.calledOnce).to.be.true
177+
expect(loggingSpy.info.calledWith('CSP violation request payload did not match expected content type')).to.be.true
178+
})
139179

140-
it('should return 400 if request is not JSON', async () => {
141-
await request(underTest)
142-
.post('/test')
143-
.set('content-type', 'application/json')
144-
.send('notJSON')
145-
.expect(400)
180+
it('should return 400 if json does not included expected values', async () => {
181+
await request(underTest)
182+
.post('/test')
183+
.send({ key: 'value' })
184+
.expect(400)
185+
})
146186

147-
expect(loggingSpy.info.calledOnce).to.be.true
148-
expect(loggingSpy.info.calledWith('CSP violation request payload did not match expected content type')).to.be.true
149-
})
187+
it('should return 429 if too many requests are made and respect trust proxy settings', async () => {
150188

151-
it('should return 400 if json does not included expected values', async () => {
152-
await request(underTest)
153-
.post('/test')
154-
.send({ key: 'value' })
155-
.expect(400)
156-
})
189+
// X-Forwarded-For: <client>, <proxy1>, <proxy2>, https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For
190+
// make 10 requests for each 'IP', the 11th should be rate limited
157191

158-
it('should return 429 if too many requests are made and respect trust proxy settings', async () => {
192+
for (let i = 0; i < 10; i++) {
193+
await request(underTest)
194+
.post('/test')
195+
.set('x-forwarded-for', '1.2.3.4, 2.2.2.2, 3.3.3.3')
196+
.send(validReportingAPIPayload)
197+
.expect(204)
159198

160-
// X-Forwarded-For: <client>, <proxy1>, <proxy2>, https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/X-Forwarded-For
161-
// make 10 requests for each 'IP', the 11th should be rate limited
199+
await request(underTest)
200+
.post('/test')
201+
.set('x-forwarded-for', '5.6.7.8, 2.2.2.2, 3.3.3.3')
202+
.send(validReportUriPayload)
203+
.expect(204)
204+
}
162205

163-
for (let i = 0; i < 10; i++) {
164206
await request(underTest)
165207
.post('/test')
166208
.set('x-forwarded-for', '1.2.3.4, 2.2.2.2, 3.3.3.3')
167-
.send(validReportingAPIPayload)
168-
.expect(204)
209+
.send(validReportUriPayload)
210+
.expect(429)
169211

170212
await request(underTest)
171213
.post('/test')
172214
.set('x-forwarded-for', '5.6.7.8, 2.2.2.2, 3.3.3.3')
173-
.send(validReportUriPayload)
174-
.expect(204)
175-
}
176-
177-
await request(underTest)
178-
.post('/test')
179-
.set('x-forwarded-for', '1.2.3.4, 2.2.2.2, 3.3.3.3')
180-
.send(validReportUriPayload)
181-
.expect(429)
182-
183-
await request(underTest)
184-
.post('/test')
185-
.set('x-forwarded-for', '5.6.7.8, 2.2.2.2, 3.3.3.3')
186-
.send(validReportingAPIPayload)
187-
.expect(429)
215+
.send(validReportingAPIPayload)
216+
.expect(429)
217+
})
188218
})
189219
})
190-

0 commit comments

Comments
 (0)