-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Fix #1988 - Validate attachment names on POST /db/doc #1997
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
Fix #1988 - Validate attachment names on POST /db/doc #1997
Conversation
|
Hi @jjrodrig , any desire to get this updated and merged into the |
|
Sure @wohali As soon this PR is accepted, I'll open PRs to merge into the 3.x and 3.0.x branches |
81ab659 to
b5c4daa
Compare
b5c4daa to
a69c401
Compare
src/chttpd/src/chttpd_db.erl
Outdated
| db_attachment_req(#httpd{method='GET',mochi_req=MochiReq}=Req, Db, DocId, FileNameParts) -> | ||
| FileName = list_to_binary(mochiweb_util:join(lists:map(fun binary_to_list/1, | ||
| FileNameParts),"/")), | ||
| validate_attachment_name(FileName), |
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.
why validate on GET?
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.
Just to be consistent in the API response and return 400 when the attachment name is not valid.
We can remove the validation and just return 404
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.
@rnewson Do you think that we should return 404 and not 400 when a not allowed attachment name is requested?
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.
finally I've removed the attachment name validation on GET
src/chttpd/src/chttpd_db.erl
Outdated
| validate_partitioned_db_enabled(Req) -> | ||
| case couch_flags:is_enabled(partitioned, Req) of | ||
| true -> | ||
| true -> |
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.
remove whitespace only change
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.
Done
83847fb to
515368b
Compare
|
@wohali I'll update this and move it to main |
ceea9bf to
2711564
Compare
2711564 to
2bf4f41
Compare
rnewson
left a comment
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.
lgtm
Overview
This PR solves the problem described at the issue #1988
PUT & POST operations for creating documents in a database enforce different validation rules over the attachment names of the new document.
When a document is created with a PUT operation "_foo.txt" is not accepted as a valid attachment name. If the document is created by a POST operation the "_foo.txt" attachment name is accepted.
This is an inconsistent behavoiur in the API as not accepted attachment names can be created.
Testing recommendations
Related Issues or Pull Requests
This PR fixes #1988
Checklist