935524 - Captain should not log or accept invalid commands

Status: RESOLVED WONTFIX

(Infrastructure & Operations :: IT-Managed Tools, task)

Description

[Frederik Braun [:freddy]]

The log of previous commands contains invalid commands. Even though they are never executed, Captain shouldn't accept or log those. Maybe rendering a 403 or redirecting to the Run Command view saying something like "This is not a valid command".

Not really a security bug, but making it block the secreview because I didn't know any other implementation bug to block. Feel free to change this.

Comment 1

[Osmose [:osmose, :mkelly]]

We already have an issue tracking this, actually: https://github.com/mozilla/captain/issues/15

When Captain runs a command, it just sends a job to RabbitMQ, which eventually finds it's way to the shove instance for that project's cluster. Captain doesn't actually wait for a response from shove, so there's no opportunity for shove to say "Hey, this command is invalid." We could make Captain wait for shove, but this would mean that any long-running commands would block people from executing any more commands (whereas now it just blocks the commands from executing until they're done, but users can go about their business).

Captain could download the whitelist itself and validate that way, but IMO that isn't a blocker for v1.

If you're okay with us launching initially without this feature, than we can close this bug and use the issue linked above to track the feature whenever we start working on post-v1 features.

Comment 2

[Frederik Braun [:freddy]]

(In reply to Michael Kelly [:mkelly] from comment #1)
> If you're okay with us launching initially without this feature, than we can
> close this bug and use the issue linked above to track the feature whenever
> we start working on post-v1 features.

Totally! As I said, this was more something that I spotted and found noteworthy. Not a security issue.
And you're also right. There's of course no way Captain could or should validate commands. It is wise to do the check within shove ;) But from a usability perspective it might be nice to have three stats within Captain (waiting, done, failed).

Anyhow, completely your call on how to approach this.

Comment 3

[Osmose [:osmose, :mkelly]]

I choose the easy route! We'll handle this after the initial launch. Thanks for spotting it, though!