feat(middleware): introduce Request ID middleware #3082
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## next #3082 +/- ##
==========================================
+ Coverage 94.75% 96.16% +1.41%
==========================================
Files 136 147 +11
Lines 13339 14824 +1485
Branches 2237 2599 +362
==========================================
+ Hits 12639 14256 +1617
+ Misses 700 568 -132 ☔ View full report in Codecov by Sentry. |
|
Hi @ryuapp I love this middleware since it's simple! I think this has no problem and is ready to merge. @usualoma @nakasyou @MathurAditya724 @fzn0x and others. Do you have any thoughts on this? |
|
I can't decide between However, I realized that the above problem is a font problem and does not affect anything other than the document, so I am thinking of fixing it, but does anyone have any opinions? |
|
I think |
|
Interesting middleware! Can already think about multiple use cases for this. And I am voting for |
|
I prefer |
|
Thank you for all the opinions. |
|
The tests is failing on |
There was a problem hiding this comment.
Thanks for the great pull request!
Performance improvement proposals
If the request ID can be obtained from the request header, it can be assumed that it can be used as is in most cases. String.prototype.replace() is heavy, so it is better not to do anything if it passes the check by the regular expression prepared in advance.
The following is an example implementation.
diff --git a/src/middleware/request-id/request-id.ts b/src/middleware/request-id/request-id.ts
index 0c11cb46..ea66b48b 100644
--- a/src/middleware/request-id/request-id.ts
+++ b/src/middleware/request-id/request-id.ts
@@ -44,13 +44,19 @@ export const requestId = ({
headerName = 'X-Request-Id',
generator = () => crypto.randomUUID(),
}: RequesIdOptions = {}): MiddlewareHandler => {
+ const isValidIncomingHeaderValueRe = new RegExp(
+ `^[\\w\\-]${limitLength ? `{,${limitLength}}` : '+'}$`
+ )
+
return async function requestId(c, next) {
// If `headerName` is empty string, req.header will return the object
let reqId = headerName ? c.req.header(headerName) : undefined
if (reqId) {
- reqId = reqId.replace(/[^\w\-]/g, '')
- reqId = limitLength > 0 ? reqId.substring(0, limitLength) : reqId
+ if (!isValidIncomingHeaderValueRe.test(reqId)) {
+ reqId = reqId.replace(/[^\w\-]/g, '')
+ reqId = limitLength > 0 ? reqId.substring(0, limitLength) : reqId
+ }
} else {
reqId = generator(c)
}
Co-Authored-By: Taku Amano <[email protected]>
|
Hello @usualoma . benchmark time (avg) iter/s (min … max) p75 p99 p995
------------------------------------------------------------------------ -----------------------------
no-check: valid 68.51 ns/iter 14,595,900.4 (60.83 ns … 240.78 ns) 66.26 ns 136.44 ns 158.73 ns
pre check: valid 96.83 ns/iter 10,327,663.8 (85.27 ns … 154 ns) 92.97 ns 148.24 ns 149.35 ns
no-check: invalid 93.97 ns/iter 10,641,957.4 (90.13 ns … 212.15 ns) 92.73 ns 122.85 ns 146.21 ns
pre check: invalid 246.53 ns/iter 4,056,358.6 (235.76 ns … 670.25 ns) 240.52 ns 547.67 ns 621.05 nsSource codes const validId = crypto.randomUUID();
const invalidId = validId + "!";
const limitLength = 255;
const isValidIncomingHeaderValueRe = new RegExp(
`^[\\w\\-]${limitLength ? `{0,${limitLength}}` : "+"}$`,
);
let id;
console.log(isValidIncomingHeaderValueRe);
console.log(!isValidIncomingHeaderValueRe.test(validId));
console.log(!isValidIncomingHeaderValueRe.test(invalidId));
Deno.bench({
name: "no-check: valid",
fn() {
id = validId.replace(/[^\w\-]/g, "");
id = limitLength > 0 ? id.substring(0, limitLength) : id;
},
});
Deno.bench({
name: "pre check: valid",
fn() {
if (!isValidIncomingHeaderValueRe.test(validId)) {
id = validId.replace(/[^\w\-]/g, "");
id = limitLength > 0 ? id.substring(0, limitLength) : id;
}
},
});
Deno.bench({
name: "no-check: invalid",
fn() {
id = invalidId.replace(/[^\w\-]/g, "");
id = limitLength > 0 ? id.substring(0, limitLength) : id;
},
});
Deno.bench({
name: "pre check: invalid",
fn() {
if (!isValidIncomingHeaderValueRe.test(invalidId)) {
id = invalidId.replace(/[^\w\-]/g, "");
id = limitLength > 0 ? id.substring(0, limitLength) : id;
}
},
});However, as you say, I'm thinking of removing this validation, leaving only lengthLimit. What do you think about this? |
|
@ryuapp I also benchmarked and confirmed the following. Checking with a regular expression like scriptsBoth benchmarks are the same. denoconst validId = crypto.randomUUID();
const invalidId = validId + "!";
const limitLength = 255;
const isValidIncomingHeaderValueRe = new RegExp(
`^[\\w\\-]${limitLength ? `{0,${limitLength}}` : "+"}$`,
);
const isValidCharsOnlyRe = /^[\w\-]+$/;
let id;
console.log(isValidIncomingHeaderValueRe);
console.log(!isValidIncomingHeaderValueRe.test(validId));
console.log(!isValidIncomingHeaderValueRe.test(invalidId));
console.log(!isValidCharsOnlyRe.test(validId));
console.log(!isValidCharsOnlyRe.test(invalidId));
Deno.bench({
name: "no-check: valid",
fn() {
id = validId.replace(/[^\w\-]/g, "");
id = limitLength > 0 ? id.substring(0, limitLength) : id;
},
});
Deno.bench({
name: "pre check: valid",
fn() {
if (!isValidIncomingHeaderValueRe.test(validId)) {
id = validId.replace(/[^\w\-]/g, "");
id = limitLength > 0 ? id.substring(0, limitLength) : id;
}
},
});
Deno.bench({
name: "pre check w/ isValidCharsOnlyRe: valid",
fn() {
if (!isValidCharsOnlyRe.test(validId) || (limitLength > 0 && validId.length > limitLength)) {
id = validId.replace(/[^\w\-]/g, "");
id = limitLength > 0 ? id.substring(0, limitLength) : id;
}
},
});
Deno.bench({
name: "no-check: invalid",
fn() {
id = invalidId.replace(/[^\w\-]/g, "");
id = limitLength > 0 ? id.substring(0, limitLength) : id;
},
});
Deno.bench({
name: "pre check: invalid",
fn() {
if (!isValidIncomingHeaderValueRe.test(invalidId)) {
id = invalidId.replace(/[^\w\-]/g, "");
id = limitLength > 0 ? id.substring(0, limitLength) : id;
}
},
});node / bunimport { run, group, bench } from 'mitata'
const validId = crypto.randomUUID()
const invalidId = validId + '!'
const limitLength = 255
const isValidIncomingHeaderValueRe = new RegExp(
`^[\\w\\-]${limitLength ? `{0,${limitLength}}` : '+'}$`
)
const isValidCharsOnlyRe = /^[\w\-]+$/
let id
console.log(isValidIncomingHeaderValueRe)
console.log(!isValidIncomingHeaderValueRe.test(validId))
console.log(!isValidIncomingHeaderValueRe.test(invalidId))
group('uuid validation', () => {
bench('no-check: valid', () => {
id = validId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
})
bench('pre check: valid', () => {
if (!isValidIncomingHeaderValueRe.test(validId)) {
id = validId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
}
})
bench('pre check w/ isValidCharsOnlyRe: valid', () => {
if (!isValidCharsOnlyRe.test(validId) || (limitLength > 0 && validId.length > limitLength)) {
id = validId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
}
})
bench('no-check: invalid', () => {
id = invalidId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
})
bench('pre check: invalid', () => {
if (!isValidIncomingHeaderValueRe.test(invalidId)) {
id = invalidId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
}
})
})
run()denonodebunI'm thinking about this now.
|
|
Ah, using scriptimport { run, group, bench } from 'mitata'
const validId = crypto.randomUUID()
const invalidId = validId + '!'
const limitLength = 255
const isValidIncomingHeaderValueRe = new RegExp(
`^[\\w\\-]${limitLength ? `{0,${limitLength}}` : '+'}$`
)
const isValidCharsOnlyRe = /^[\w\-]+$/
const hasInvalidCharRe = /[^\w\-]/
let id
console.log(isValidIncomingHeaderValueRe)
console.log(!isValidIncomingHeaderValueRe.test(validId))
console.log(!isValidIncomingHeaderValueRe.test(invalidId))
group('uuid validation', () => {
bench('no-check: valid', () => {
id = validId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
})
bench('pre check: valid', () => {
if (!isValidIncomingHeaderValueRe.test(validId)) {
id = validId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
}
})
bench('pre check w/ isValidCharsOnlyRe: valid', () => {
if (!isValidCharsOnlyRe.test(validId) || (limitLength > 0 && validId.length > limitLength)) {
id = validId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
}
})
bench('pre check w/ hasInvalidCharRe: valid', () => {
if (hasInvalidCharRe.test(validId) || (limitLength > 0 && validId.length > limitLength)) {
id = validId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
}
})
bench('no-check: invalid', () => {
id = invalidId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
})
bench('pre check: invalid', () => {
if (!isValidIncomingHeaderValueRe.test(invalidId)) {
id = invalidId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
}
})
})
run()denonodebunHowever... |
|
@ryuapp
With regard to this, the topic is "resistance to DoS attacks that send headers like a few KB with the intention of attacking", isn't it? I think that is certainly true. As another suggestion, how about "checking the length and character type of the header value and re-generating it if the result is invalid"? diff --git a/src/middleware/request-id/request-id.ts b/src/middleware/request-id/request-id.ts
index 0c11cb46..0c2bbd28 100644
--- a/src/middleware/request-id/request-id.ts
+++ b/src/middleware/request-id/request-id.ts
@@ -48,10 +48,7 @@ export const requestId = ({
// If `headerName` is empty string, req.header will return the object
let reqId = headerName ? c.req.header(headerName) : undefined
- if (reqId) {
- reqId = reqId.replace(/[^\w\-]/g, '')
- reqId = limitLength > 0 ? reqId.substring(0, limitLength) : reqId
- } else {
+ if (!reqId || reqId.length > limitLength || /[^\w\-]/.test(reqId)) {
reqId = generator(c)
}In general, if the value of the header is invalid, I think it can be regarded as an 'illegal access'. If the header value is invalid with a proper access (although such use cases are almost never the case), it would be better to modify the system or deal with it independently in the application without using this middleware. If the application wants to accept non-UUID or longer strings, it can do this. app.use(requestId({
generator: (c) => c.req.header('X-Amzn-Trace-Id'), // like "Self=1-67891234-12456789abcdef012345678;Root=1-67891233-abcdef012345678912345678"
}))benchmarkBenchmarking with the following scripts showed the following. I consider this a good result because I think we should focus on two things. 'high performance in the normal case' and 'not so bad in the worst case.'
scriptimport { run, group, bench } from 'mitata'
let id
const limitLength = 255
const data = [
{ label: 'valid', id: crypto.randomUUID() },
{ label: 'short invalid', id: crypto.randomUUID() + '!' },
{ label: 'long invalid', id: '!'.repeat(2048) },
]
data.forEach(({ label, id: testId }) => {
group(label, () => {
bench('replace() and substring()', () => {
id = testId.replace(/[^\w\-]/g, '')
id = limitLength > 0 ? id.substring(0, limitLength) : id
})
bench('regenerate', () => {
if (!testId || testId.length > limitLength || /[^\w\-]/.test(testId)) {
id = crypto.randomUUID()
}
})
})
})
run()denonodebunWhat do you think? |
|
@usualoma Thank you for investigating this. ok. I adopt the proposal. Thanks so much! |
Co-Authored-By: Taku Amano <[email protected]>
|
@ryuapp Thank you! |
Request ID middleware generates a unique id for a request.
The unique request id can be used to trace a request end-to-end.
It is also easily customizable with several options.
The author should do the following, if applicable
bun run format:fix && bun run lint:fixto format the code