Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return 413 when payload too large. #48

Merged
merged 1 commit into from Jun 30, 2017

Conversation

@aheinz-fe
Copy link
Contributor

aheinz-fe commented Jun 28, 2017

@AdriVanHoudt

This comment has been minimized.

Copy link

AdriVanHoudt commented Jun 29, 2017

If this passes does this not mean we are missing a test case?

@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Jun 29, 2017

I definitely expected a breakage. My guess is that the test is just permissive. I'll try to dig it up.

Edit

subtext/test/index.js

Lines 193 to 208 in 041b9be

it('errors when content-length header greater than maxBytes', (done) => {
const payload = '{"x":"1","y":"2","z":"3"}';
const request = Wreck.toReadableStream(payload);
request.headers = {
'content-length': '50',
'content-type': 'application/json'
};
Subtext.parse(request, null, { parse: false, output: 'data', maxBytes: 10 }, (err, parsed) => {
expect(err).to.exist();
expect(err.message).to.equal('Payload content length greater than maximum allowed: 10');
done();
});
});

Here it is. It is permissive. hapi does the work of getting HTTP-y and checking the status code, so we'd see a breakage there.

@AdriVanHoudt

This comment has been minimized.

Copy link

AdriVanHoudt commented Jun 29, 2017

But err.statusCode in that test has changed no?

@devinivy

This comment has been minimized.

Copy link
Member

devinivy commented Jun 29, 2017

That test doesn't check err.output.statusCode! It certainly could, though.

@AdriVanHoudt

This comment has been minimized.

Copy link

AdriVanHoudt commented Jun 29, 2017

Imo it should ^^

@aheinz-fe aheinz-fe force-pushed the aheinz-fe:entity-too-large branch from 0090280 to af17ce0 Jun 29, 2017
@aheinz-fe

This comment has been minimized.

Copy link
Contributor Author

aheinz-fe commented Jun 29, 2017

Commit amended.

@johnbrett johnbrett merged commit af17ce0 into hapijs:master Jun 30, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@johnbrett

This comment has been minimized.

Copy link
Contributor

johnbrett commented Jun 30, 2017

Great work @aheinz-fe! Merged, and well spotted on that test case, it should have checked the statusCode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.