-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Don't crash on invalid inline attachments #817
Don't crash on invalid inline attachments #817
Conversation
src/couch/src/couch_att.erl
Outdated
?_assertEqual(ResultStub, stub_from_json(Att, Props)), | ||
?_assertEqual(ResultFollows, follow_from_json(Att, Props)), | ||
?_assertEqual(ResultInline, inline_from_json(Att, PropsInline)), | ||
?_assertThrow({bad_request, _}, inline_from_json(Att, Props)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case the data would be undefined so the case when data is not available at all in the request. Maybe have another clause where data is present but is invalid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This covers the original's issue exception, but sure, I'll add another test.
{att_len, Length} | ||
], Att) | ||
catch | ||
_:_ -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if we should be more conservative and explicitly match on the knows exception tags and error message
error:function_clause
error:{badmatch, _}
the function_clause
happens when is an invalid input data type such as undefined or list.
badmatch
happens if type is correct but data is not a valid B64 encoding.
But I guess it's not clear that there couldn't be other errors thrown so maybe catching all is ok there. Docs don't say what exactly should be thrown.
So up to you here. I am 50/50 on this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about it, but then decided to opt out. We don't particularly interested in how base64
can't decode our data, only in fact that it can't be decoded, so there are no merit to try and match all possible ways base64
can break, if at the end we are raising the same invalid request exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yap got it, that makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JS
make javascript suites=attachments.js
test/javascript/tests/attachments.js pass
=======================================================
JavaScript tests complete.
EUnit
Compiled test/couch_changes_tests.erl
Compiled test/chttpd_endpoints_tests.erl
======================== EUnit ========================
module 'couch_att'
Lazy record upgrade tests
couch_att: attachment_upgrade_test_ (Existing record fields don't upgrade)...ok
couch_att: attachment_upgrade_test_ (New fields upgrade)...ok
[done in 0.006 s]
Attachment defaults tests
couch_att: attachment_defaults_test_ (Records retain old default values)...ok
couch_att: attachment_defaults_test_ (Upgraded records inherit defaults)...ok
couch_att: attachment_defaults_test_ (Undefined entries are elided on upgrade)...ok
[done in 0.009 s]
Basic attachment field api
couch_att: attachment_field_api_test_...ok
couch_att: attachment_field_api_test_...ok
couch_att: attachment_field_api_test_...ok
[done in 0.009 s]
Disk term tests
couch_att:789: attachment_disk_term_test_...ok
couch_att:790: attachment_disk_term_test_...ok
couch_att:791: attachment_disk_term_test_...ok
couch_att:792: attachment_disk_term_test_...ok
[done in 0.012 s]
JSON term tests
couch_att:822: attachment_json_term_test_...[0.009 s] ok
couch_att:823: attachment_json_term_test_...ok
couch_att:824: attachment_json_term_test_...ok
couch_att:825: attachment_json_term_test_...ok
couch_att:826: attachment_json_term_test_...ok
[done in 0.024 s]
Attachment stub merging tests
[done in 0.060 s]
=======================================================
All 17 tests passed.
==> rel (eunit)
==> asf4 (eunit)
25a3292
to
369b442
Compare
@nickva Had to revert spec restoration commit, apparently R16 still doesn't know about |
Ah ok, cool. Still approved, merge it when ready. |
Overview
We trust inline attachment to be base64 encoded, so in case it's not provided or invalidly encoded the upload request crashes with
function_clause
.The change catches decode attempt and re-throws it as 400 error.
Testing recommendations
Standard eunit and javascript tests should pass.
GitHub issue number
Fixes #784
Checklist