When looking at options to mitigate
https://github.com/github/github/issues/174690, my initial approach
there failed because we can't drop some Rails components because we use
gems that express all of Rails as a dependency instead of the parts they
need.
The chatops controller is one of those. It looks like though it only
needs ActiveSupport & ActionPack so I reduced the dependencies here to
just those parts.
In a number of places the controller was sending plaintext responses
back when it encountered an error in a before_action. Those responses
were being "parsed" by Hubot, assuming they were JSON. That parsing
would fail, and return a generic error back to the user.
This sends back valid JSON using the already written jsonrpc_error
method. I followed the pattern I saw, and invented a new set of error
numbers starting at -32800 (there was already -32700 and -32600).
This increases the security of chatops endpoins by only allowing
permitted parameters to be passed. It goes further and validates the
JSON RPC params are also explicitly named in the regular expression
matchers as well.
The whole `scrubbed_params` has seems a little fishy to me - it is
copying the JSON RPC params up into the main params hash. But it cannot
copy all of them because you could then invoke somebody else's chatop
command by providing a `chatop` argument to your own command.
It is better to separate concerns and just leave the JSON RPC params
where they are.
The reference hubot chatops-rpc implementation has the generic arguments
(--param value) overwriting the parameter values extracted via the
regular expression parsing. The change to the test here emulates that
behavior.
refs d32d185f63/src/chatops-rpc.coffee (L250-L253)
This adds support for a new option, which allows servers to hint
to clients that they should format the response as a separate
attachment instead of returning it inline as a message.
Because this is a hint, and because not all clients will support
an attachment mode, clients are allowed to ignore this field for
any reason.
For example, Slack supports "snippets", which are arbitrary text
documents sent as attachments. These don't use Slack's normal message
formatting and have a larger filesize limit.
Dirkjan Bussink
Lee Quarella
Nina Kaufman
Raffaele Di Fazio
Misty De Meo
Kristján Oddsson
Misty De Meo
Georgi Knox
Jared McFarland
Brian Anglin
Tim Pease
Tim Pease
John Hawthorn