REST module should be more robust when invalid data is send to create new resources. We should make sure that the received data is not NULL after deserialization and return the appropriate response.

CommentFileSizeAuthor
#25 robust-post-1865594-25.patch9.45 KBklausi
#25 robust-post-1865594-25-interdiff.txt743 bytesklausi
#23 robust-post-1865594-23.patch9.45 KBklausi
#23 robust-post-1865594-23-interdiff.txt655 bytesklausi
#15 robust-post-1865594-15.patch9.46 KBklausi
#13 robust-post-1865594-13.patch9.64 KBklausi
#13 robust-post-1865594-13-interdiff.txt4.46 KBklausi
#10 robust-post-1865594-10.patch7.24 KBklausi
#10 robust-post-1865594-10-interdiff.txt5.02 KBklausi
#5 robust-post-1865594-5.patch3.98 KBklausi
#5 robust-post-1865594-5-interdiff.txt1.45 KBklausi
#1 rest-robust-post-1865594-1.patch4.3 KBklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Active » Needs review
FileSize
rest-robust-post-1865594-1.patch4.3 KB

Patch attached.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

IMHO, the deserializer should throw exceptions when the data doesn't meet expectations. The newest patch in #1852812: Use cache to get entity type/bundle metadata from JSON-LD @type URI throws exceptions on denormalize, so that needs to be wrapped in a try/catch. I also provide informative messages, so those should be passed through.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs review » Postponed

ok, postponing this on #1852812: Use cache to get entity type/bundle metadata from JSON-LD @type URI.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Postponed » Needs work

Guess this needs work

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
robust-post-1865594-5-interdiff.txt1.45 KB
robust-post-1865594-5.patch3.98 KB

Rerolled for exceptions.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Title: Handle invalid POST request content » Better error messages in case of invalid POST request content

The scope of this patch seems to have reduced dramatically. Retitling.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

The title might not have been clear, but I'm fairly certain the scope hasn't been changed at all (though klausi would know better).

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

This does not improve only error messages, it also improves HTTP status codes (400 instead of 500). But yeah, we sort of handled invalid POST stuff already by simply blowing up a 500 fatal error.

While we are at it here are some more points for discussing REST error messages:
* Should we translate error messages with t()? I would say no, since they are not intended to be shown to end users.
* Should we protect error messages with error_displayable()? Since resource plugins return HttpExceptions I think it is clear that those are considered public, so they should never contain sensitive information. What about the serializer component? When looking through it I saw some UnexpectedValueExceptions that throw up libxml errors for example. Do we think that there could be ever sensitive data in it?

Crell’s picture

Crell CreditAttribution: Crell commented

My knee-jerk thought is that error messages SHOULD be returned in the language and format specified in the request headers. However, I don't know how far we should push that. It depends on what the convention is around the web. Let's research that a bit and then let that guide our decision.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Title: Better error messages in case of invalid POST request content » Better REST error messages in case of exceptions
FileSize
robust-post-1865594-10-interdiff.txt5.02 KB
robust-post-1865594-10.patch7.24 KB

Thinking more about it I guess it makes sense to just use t() around the exception error messages. We can simply leverage the language system and t() to figure out what the client wants and what is available.

I don't fully trust all levels of the serialization stack, so I kept the generic error message for serialization errors.

For now I hard-coded drupal_jsonld as content-type on error responses. text/plain might lead to parsing errors on primitive clients, so we should return our error message in the appropriate format (handling different formats should be done in other issues like #1850734: Make serialization formats configurable). An error response body in drupal_jsonld looks like this:

{
   "error": "Entity with ID 5 not found"
}

Status: Needs review » Needs work

The last submitted patch, robust-post-1865594-10.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

So you're saying that no matter what exception is thrown in the serialization process, consumers will always see the same error message?

If that's correct, what's the reasoning for this? What does it protect us against?

There are a lot of things that consumers can get wrong when running a request against a service. I think it is waaaaay better to give them informative messages about what they got wrong.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
robust-post-1865594-13-interdiff.txt4.46 KB
robust-post-1865594-13.patch9.64 KB

Fixed the test cases.

My reasoning for swallowing the error messages was that they could reveal sensitive information, like path information such as /var/www/example.com/core/whatever. However, since that is only a minor security risk and the error messages are important for consumers I'm passing through the messages now. If we encounter any problems with that we can fix that later.

Crell’s picture

Crell CreditAttribution: Crell commented

Related: http://stackoverflow.com/questions/14508048/error-messages-from-rest-req...

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Issue tags: +WSCCI
FileSize
robust-post-1865594-15.patch9.46 KB

linclark recommended https://www.youtube.com/watch?v=5WXYw4J4QOU , which talks about errors at 1:09:27. So I think we are doing it quite right here with wrapping the error in the requested format. The video does not mention translation of error messages. Not sure we should invent error number codes to identify particular errors, I think we can leave that open for a possible follow-up.

Patch did not apply anymore, rerolled.

Crell’s picture

Crell CreditAttribution: Crell commented

One of the speakers here at SunshinePHP mentioned https://github.com/blongden/vnd.error, which looks very simple and just what we're looking for.

hpbruna’s picture

hpbruna CreditAttribution: hpbruna commented

#5: robust-post-1865594-5.patch queued for re-testing.

Crell’s picture

Crell CreditAttribution: Crell commented 
+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/DBLogResource.php
@@ -58,6 +58,6 @@ public function get($id = NULL) {
+    throw new NotFoundHttpException(t('Log enttry with ID @id was not found', array('@id' => $id)));

Only one "t" in entry.

Thinking about it, we shouldn't do the exception handling in RequestHandler. We should allow the exception to propagate to an exception listener, which will turn it into a response following the vnd.error spec depending on the base type of the request. (xml or json)

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

What's the reason for using vnd.error, instead of returning the response in the requested media type? Both would use the HTTP error code, so consumers will already know that it was an error.

Some consuming tools do not understand vendor prefixed media types, and it also limits us to XML and JSON responses, whereas our system is format agnostic. Does using the vendor prefixed media type give us an advantage?

Crell’s picture

Crell CreditAttribution: Crell commented

The requested media type may not have a reasonable way to represent errors. JSON-LD may fold naturally to plain JSON, but then do we use a key "message", "messages", "errors", or...? SVG doesn't fall bak to anything sane. Atom's envelope is frickin' huge. :-)

After talking with folks at Sunshine PHP I checked the RFC: http://www.w3.org/Protocols/rfc2616/rfc2616-sec12.html

It's filled with lots of MAY, so I'm comfortable saying that asking for atom+xml and getting back vnd.error+xml is acceptable. It also means that then someone writing a new format doesn't need to think about the format and envelope for error messages, as we'll fold it to vnd.error for the appropriate parent type. (If someone is asking for something in non-XML or JSON, sure, they need to do something custom, but there aren't many of those.)

I'm assuming that a client wouldn't be checking the media type directly, vendor or otherwise. If you ask for Atom, you can expect to get back atom+xml on a 200 or vnd.error+xml on a 4xx/5xx. You can just build that assumption into your code fairly reliably.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I may be mistaken, but we don't actually have the ability to discern a media type tree at this point, do we? That is to say, within the system we don't really have a way to say that JSON is the parent of JSON-LD.

The format structure that we work with is as follows (from Request::initializeFormats):

static::$formats = array(
            'html' => array('text/html', 'application/xhtml+xml'),
            'txt'  => array('text/plain'),
            'js'   => array('application/javascript', 'application/x-javascript', 'text/javascript'),
            'css'  => array('text/css'),
            'json' => array('application/json', 'application/x-json'),
            'xml'  => array('text/xml', 'application/xml', 'application/x-xml'),
            'rdf'  => array('application/rdf+xml'),
            'atom' => array('application/atom+xml'),
            'rss'  => array('application/rss+xml'),
        );

If we were add "application/ld+json" to the "json" array there, then we wouldn't be able to switch Normalizers as we currently do.

Maybe there's a way to distinguish in Symfony which I just haven't seen?

Crell’s picture

Crell CreditAttribution: Crell commented

I don't know. I haven't dug that far into it. :-)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
FileSize
robust-post-1865594-23-interdiff.txt655 bytes
robust-post-1865594-23.patch9.45 KB

Fixed the typo mentioned in #18.

Using vnd.error might be interesting, but I think it is a bit out of scope for this issue. Let's fix the messages and the translation and the default media type here and leave it for another issue. Same for moving out the exception handling to a dedicated exception handler, not within the scope of this patch.

Status: Needs review » Needs work

The last submitted patch, robust-post-1865594-23.patch, failed testing.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Needs work » Needs review
FileSize
robust-post-1865594-25-interdiff.txt743 bytes
robust-post-1865594-25.patch9.45 KB

Fixed the typo in the test case.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I just saw a blog post that talks about an alternative to vnd.error from the IETF. Since that's out of scope for this issue, I posted #1916302: RFC 7807: "Problem Details for HTTP APIs" — serve REST error responses as application/problem+json.

Crell’s picture

Crell CreditAttribution: Crell commented
Status: Needs review » Reviewed & tested by the community

OK, let's do this, and we can discuss refactoring the error message itself in Lin's linked issue.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Automatically closed -- issue fixed for 2 weeks with no activity.