Skip to content

[#138] Resolve "BUG: Allow broadcast messages beginning with command keywords"

Closes #138 (closed)

Context

Users reported sending normal broadcast messages that got unexpected responses because they started with reserved command words.

Changes

This MR establishes the following (sensible) desired behavior for messages starting with "no-payload" commands:

  • GIVEN an admin on a channel
  • WHEN I send any of the following:
    • hello world
    • join us tomorrow
    • goodbye world
    • leave that one to me
    • info is on the way
    • help us get out of this kettle!
    • responses on now!
    • responses off now!
    • vouching on now
    • vouching off now
  • THEN that message is broadcast to all members of the channel

Implementation Notes

  • to accomplish the above, we introduce a validatePayload helper function into commands.parse.findCommandMatch. we use to squash the bug we want to eliminate, but also to clean up code by moving all payload validation (including phone numbers) into commands.parse, which eliminates some duplication, and -- we hope -- will help out @mari with #137 (closed)
  • for the correct handling of broadcast messages begining with "no payload commands", we:
    • add a branch for all no payload commands to a case statement in varlidatePayload and delegate to validateNoPayload helper
    • use that helper to inspect whether a no-payload command is followed by text, if so, we return NOOP to signal a broadcast message, if not, we pass the originally parsed command through
  • for phone number validation, we:
    • introduce a ParseError type and specify that parse may return either an Executable or a ParseError
    • add a series of case statements to validatePayload that fall through to validatePhoneNumber
    • validatePhoneNumber then tries to parse a valid e164 phone number (see: https://www.twilio.com/docs/glossary/what-e164) from the payload.
      • it returns a CommandMatch with the parsed phone number substituted into the matches field if it can
      • else it returns a ParseError which bubbles up to the return value of parse
  • more generally, parse and execute now have a slightly different contract:
    • parse.parseExecutable now returns a ParseError if a payload cannot be correctly parsed
    • execute.processCommand now returns early and notifies the command sender if a correctly parsed command does not have a correctly parsed payload (ie: if it is given a ParseError as its first argument
    • the hope is that @mari can use this new contract to add a validateIntengerBelow(10) helper to the switch statements and use it to return early if a user ever sends a command like VOUCH LEVEL foobar or VOUCH LEVEL 1000

Left out of Scope

@margot i considered rolling #124 (closed) into this but ultimately opted against it. (#directaction)

reasoning:

  • the likelihood that a subscriber or admin wants to send a one word message that is actually a message seems moderately high
  • if we always block those, we make the most likely person the most sad (they don't really have a way to do what they want to do).
  • the likelihood that an admin might send a malformed message is also moderately high (maybe somewhat lower? particularly after repeated use?)
  • however, the consqequences of the failure mode are relatively low: if they send out a message that gets broadcast, it's embarassing maybe but not the end of the world and they can still recover to do what they wanted, unlike the user in the above case, who does not have a way to do the thing they actually wanted to do (send a one word message), and is less likely to be well-versed on how signalboost works (and hence more confused and less likely to try again via the circuitous route of sending another message)
  • for all those reasons leaving as fix if squeaky for now. but if we hear squeaks, we now have a nice little helper function in validatePayload that would be the perfect place to implement it! as well as a new implementation of excecute.processCommand that will handle the error we were talking about using in this case!

Merge request reports

Loading