Enforce quota limit on response data (#3492) #3494
Conversation
|
Since this came out of a Twitter conversation between you and @mbleigh I've assigned him as a reviewer. |
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| res.end = function (chunk: any, encoding?: any, cb?: () => void) { | ||
| if (chunk) { |
There was a problem hiding this comment.
Another thing to consider, apart from the missing unit tests, is that the data limit is not imposed on a direct call to res.end() but only on a stream (using res.write). This is somewhat deliberate, as I would not expect anyone to directly flush 10MB+ on the response stream, but if the Google Cloud Functions production code does check the single chunk it might be useful to include it here as well.
There was a problem hiding this comment.
I am not sure about the prod behavior with res.end() but this is definitely an improvement either way!
There was a problem hiding this comment.
@remie I can see why you were hesitant to add unit tests but I think one test would be pretty helpful. We can add one where the function just generates random bytes until it gets to > 10MB and ensure that the middleware works.
You don't need to worry much about the test speed, the functions runtime tests are in a separate "integration" suite which won't slow down our normal unit tests.
Let me know if you have any trouble writing the test.
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| res.end = function (chunk: any, encoding?: any, cb?: () => void) { | ||
| if (chunk) { |
There was a problem hiding this comment.
I am not sure about the prod behavior with res.end() but this is definitely an improvement either way!
|
@remie let me know if you don't have time to add a unit test, I could helpl! |
|
@samtstern I've been busy with some of our product releases, I was planning to circle back to this early next week. However, feel free to start / take over, I won't feel offended 😄 |
|
@remie no rush at all just checking in! Next week sounds great. |
Description
Proposed fix for "Firebase emulators should impose HTTP/2 request/response size limits" (#3492)
This PR currently does not include any tests, as I'm not entirely confident that it is a good idea to emulate / test processing of a 10MB+ response stream during test execution. Please advice if you prefer me to include such test.
Scenarios Tested
Sample Commands
N/A