diff --git a/CHANGELOG.md b/CHANGELOG.md index ee3543b05..acf7dde9a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -155,6 +155,18 @@ in a channel is cancelled. GitHub links through a GitHub App's user sign-in, set with them and no public URL. Nothing reads the links yet: they are what GitHub alerts and reviews will build on. Migration `0053_identity_links` only adds tables. +### A credential written as the word after a flag is redacted + +`docker login -p secret`, `sshpass -p secret` and `redis-cli -a secret` were recorded with the +password in clear: the rules recognised a flag whose name says what it carries, and mysql's attached +`-psecret`, but not a credential passed as the word after the flag. `redis-cli -ahunter2` was worse — +the attached form reads as a flag called `-asecret`, so the rule for a flag named after a secret +redacted the word *after* it and left the password. A URL with no user, `redis://:secret@host`, was +missed the same way, and so were curl's proxy credentials in `--proxy-user` where `-u` and `--user` +were covered. All of them are now redacted, in the command kept for Action Recording and in the copy +exported to a telemetry collector. A password separated from `-p` by a space is left alone: mysql +asks for it at the prompt and the word after the flag is the database name. + ### An app's account can belong to the team An administrator can make a Composio app Shared: one account, connected once, that every Bot granted diff --git a/server/src/telemetry/scrub.ts b/server/src/telemetry/scrub.ts index 23d9f8d1c..4e2569a34 100644 --- a/server/src/telemetry/scrub.ts +++ b/server/src/telemetry/scrub.ts @@ -42,6 +42,37 @@ const RULES: [RegExp, (match: string, ...groups: string[]) => string][] = [ /(x-api-key\s*[:=]\s*)("[^"]*"|'[^']*'|[^\s'"]+)/gi, (_m, lead) => `${lead}${REDACTED}`, ], + /* + * Credentials passed positionally: the value is the word after the flag, so nothing in the command + * names it and the rules below cannot see it. Tool-anchored, and checked before them because a + * generic rule can mistake an attached short-form password for a flag name — `redis-cli -asecret` + * reads as a flag called `-asecret`, and the rule for a flag named after a secret then redacts the + * word after it instead of the password. + * + * Each of these refuses a value that begins with `-`, so a flag left bare, which asks for the + * password interactively, does not swallow the flag that follows it. + */ + // `-psecret` for mysql and friends. Nothing separates them: a space means the password is asked for + // at the prompt and the word after `-p` is the database, so only the attached form is redacted. + [ + /(\bmysql(?:dump)?\b[^|;&]*?\s-p)("[^"]*"|'[^']*'|[^\s'"-]\S*)/gi, + (_m, lead) => `${lead}${REDACTED}`, + ], + // `docker login -p secret`, where `-p` is the password rather than a port. + [ + /(\bdocker\s+login\b[^|;&]*?\s-p[ \t]*)("[^"]*"|'[^']*'|[^\s'"-]\S*)/gi, + (_m, lead) => `${lead}${REDACTED}`, + ], + // `sshpass -p secret ssh user@host`, where `-p` is the password handed to ssh. + [ + /(\bsshpass\b[^|;&]*?\s-p[ \t]*)("[^"]*"|'[^']*'|[^\s'"-]\S*)/gi, + (_m, lead) => `${lead}${REDACTED}`, + ], + // `redis-cli -a secret`, `-asecret` and `--pass secret`, the same shape under redis-cli's names. + [ + /(\bredis-cli\b[^|;&]*?\s(?:-a[ \t]*|--pass(?:=|[ \t]+)))("[^"]*"|'[^']*'|[^\s'"-]\S*)/gi, + (_m, lead) => `${lead}${REDACTED}`, + ], // A quoted key in a JSON body: `-d '{"password":"x"}'`, or `-d "{\\"password\\":\\"x\\"}"` once the // shell quoting around it has escaped the inner quotes. The rule below stops at the closing quote // of the key, so it never saw these. @@ -68,23 +99,21 @@ const RULES: [RegExp, (match: string, ...groups: string[]) => string][] = [ ), (_m, lead) => `${lead}${REDACTED}`, ], - // `-psecret` for mysql and friends, `-p secret` too. - [ - /(\bmysql(?:dump)?\b[^|;&]*?\s-p)(\S+)/gi, - (_m, lead) => `${lead}${REDACTED}`, - ], // `curl -u user:password`, and the other ways curl takes it: `-uuser:password`, - // `--user=user:password`, and either of them with the pair in quotes. + // `--user=user:password`, `--proxy-user user:password`, and any of them with the pair in quotes. + // curl's short form for the proxy is `-U`, which is deliberately not here: `-U` is a user agent to + // wget and an unrelated flag to ssh, and a pair like `wget -U Mozilla:5` is not a credential. [ - /(\s(?:-u\s*|--user(?:=|\s+)))(?:"([^":]*):[^"]*"|'([^':]*):[^']*'|([^\s:"']+):\S+)/g, + /(\s(?:-u\s*|--user(?:=|\s+)|--proxy-user(?:=|\s+)))(?:"([^":]*):[^"]*"|'([^':]*):[^']*'|([^\s:"']+):\S+)/g, (_m, lead, doubleQuoted, singleQuoted, bare) => { const user = doubleQuoted ?? singleQuoted ?? bare; return `${lead}${user}:${REDACTED}`; }, ], - // Userinfo in any URL: `https://user:pass@host`. + // Userinfo in any URL: `https://user:pass@host`. The user is optional, because + // `redis://:pass@host` is how a password-only scheme is written. [ - /([a-z][a-z0-9+.-]*:\/\/)([^\s/@:]+):([^\s/@]+)@/gi, + /([a-z][a-z0-9+.-]*:\/\/)([^\s/@:]*):([^\s/@]+)@/gi, (_m, scheme, user) => `${scheme}${user}:${REDACTED}@`, ], ]; diff --git a/server/tests/enterprise-controls.test.ts b/server/tests/enterprise-controls.test.ts index f87e61c8b..19360b2c9 100644 --- a/server/tests/enterprise-controls.test.ts +++ b/server/tests/enterprise-controls.test.ts @@ -396,6 +396,20 @@ describe("scrubbing commands", () => { 'curl -u "admin:s3 cret" https://x.test', "curl -u admin:[REDACTED] https://x.test", ], + [ + "curl --proxy-user bob:hunter2 -x http://proxy.test:3128 https://x.test", + "curl --proxy-user bob:[REDACTED] -x http://proxy.test:3128 https://x.test", + ], + // `-p` bare asks mysql for the password and leaves the database name after it alone: the two are + // separated by a space, and the attached form is what carries a password. + ["mysql -h db -p", "mysql -h db -p"], + ["mysql -u root -p mydb", "mysql -u root -p mydb"], + ["docker login -u bob -p hunter2", "docker login -u bob -p [REDACTED]"], + ["sshpass -p hunter2 ssh user@host", "sshpass -p [REDACTED] ssh user@host"], + ["redis-cli -a hunter2 SET k v", "redis-cli -a [REDACTED] SET k v"], + ["redis-cli -ahunter2 SET k v", "redis-cli -a[REDACTED] SET k v"], + ["redis-cli --pass hunter2 SET k v", "redis-cli --pass [REDACTED] SET k v"], + ["redis://:hunter2@cache:6379", "redis://:[REDACTED]@cache:6379"], ]; for (const [input, output] of cases) { test(input, () => expect(scrubCommand(input)).toBe(output)); diff --git a/server/tests/otel-export-scrubbing.test.ts b/server/tests/otel-export-scrubbing.test.ts index 26624daef..be45d30c9 100644 --- a/server/tests/otel-export-scrubbing.test.ts +++ b/server/tests/otel-export-scrubbing.test.ts @@ -33,6 +33,9 @@ test("a command exported to the collector is scrubbed of the secrets in it", asy action: "computer_run_command", command: `curl -H "Authorization: Bearer ${secret}" https://api.example.test`, nested: { command: `export API_TOKEN=${secret}` }, + // A credential written positionally is the shape that used to survive: it is not a `NAME=value` + // and not a named flag, so nothing matched it before it reached the wire. + positional: { command: `redis-cli -a ${secret} SET k v` }, }, }); await exporter?.shutdown();