Add data type validation for WebHook payloads.

Review Request #9906 — Created May 2, 2018 and updated

chipx86
Review Board
release-3.0.x
b0ac81e...
reviewboard

Early in the development in the WebHook support, we'd have certain
objects slipping in (like model instances) that broke encoding, and this
made it unsafe to support custom payloads in a non-controlled
environment (such as RBCommons). We attempted to serialize everything to
a set of safe data types, and have now done so for a while.

This change ensures this going forward by taking the payload being
dispatched and normalizing it first, converting everything to a simple
list of allowed primitives (and datetime objects, which are useful in
templates and are safe). The normalization process traverses the payload
in much the same way that the API serialization code does, but in a way
that's a bit more safe and specific to the needs of WebHooks. In the
process, this checks for any types that aren't expected and, if found,
logs the failure and aborts the dispatch.

While we're not going to trigger these exceptions on our in production
today, this does ensure that we don't accidentally introduce something
that will trigger it in the future, and that extension authors don't
either.

Unit tests were updated to check the dispatch process and payload
content a bit more closely, ensuring that normalization and type
validation works correctly and testing more of the data sent in the
request. Much of this code was moved into a common helper to help
simplify tests. Other tests in the same file were also updated with
small fixes.

All unit tests pass.

Manually tested events with a WebHook destination, checking the payloads
to make sure they contained what I expected and were received.

chipx86
  1. Hold off, found an issue.

  2. 
      
chipx86
Review request changed

Change Summary:

Changed the approach. Validation wasn't enough. We now explicitly normalize
the payload coming in, converting it to something safe, and use that for all
serialization.

Description:

   

Early in the development in the WebHook support, we'd have certain

    objects slipping in (like model instances) that broke encoding, and this
    made it unsafe to support custom payloads in a non-controlled
    environment (such as RBCommons). We attempted to serialize everything to
    a set of safe data types, and have now done so for a while.

   
~  

This change ensures this going forward by adding validation before

~   dispatching the payloads. We go through all the keys and values in the
~   payload, looking for anything that doesn't match a strict whitelist of
~   types, and raise an exception if found. This is then logged and the
~   dispatch is aborted with another exception.

  ~

This change ensures this going forward by taking the payload being

  ~ dispatched and normalizing it first, converting everything to a simple
  ~ list of allowed primitives (and datetime objects, which are useful in
  ~ templates and are safe). The normalization process traverses the payload
  ~ in much the same way that the API serialization code does, but in a way
  + that's a bit more safe and specific to the needs of WebHooks. In the
  + process, this checks for any types that aren't expected and, if found,
  + logs the failure and aborts the dispatch.

   
   

While we're not going to trigger these exceptions on our in production

    today, this does ensure that we don't accidentally introduce something
    that will trigger it in the future, and that extension authors don't
    either.

   
   

Unit tests were updated to check the dispatch process and payload

~   content a bit more closely, ensuring that validation passes and testing
~   more of the data sent in the request. Much of this code was moved into a
~   common helper to help simplify tests. Other tests in the same file were
~   also updated with small fixes.

  ~ content a bit more closely, ensuring that normalization and type
  ~ validation works correctly and testing more of the data sent in the
  ~ request. Much of this code was moved into a common helper to help
  ~ simplify tests. Other tests in the same file were also updated with
  + small fixes.

Commit:

-b2948e0b1d1014ba7d3d7188b923ec1178417edf
+b0ac81e5e3a4405fe2182c00d2862e6828ea3819

Diff:

Revision 2 (+410 -211)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...