diff --git a/CHANGELOG.md b/CHANGELOG.md index b6761fe..b116b50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,11 +2,29 @@ ### Breaking +- `--text` is no longer `mandatory` on `send`, `reply`, `dm`, and `edit`. + Exactly one of `--text`, `--text-file` and `--text-stdin` is now required, + and giving none or more than one is a usage error (exit 64). Every existing + command line that passes `--text` keeps working. - OAuth flow now validates the `state` parameter (CSRF protection). Callbacks without a valid state are rejected. - HTML characters in OAuth callback responses are now escaped. ### Added +- `send`, `reply`, `dm`, and `edit` take the message text from a file with + `--text-file `, or from standard input with `--text-stdin` + (`--text-file -` is the same thing). Those bytes never pass through a shell, + so they are sent unchanged: no unescaping, no line-ending rewrite, no + control-character stripping. Inline `--text` keeps the normalization it has + today, because a shell-quoted argument is the case that normalization + repairs. This closes the case where a backtick span or a `$( )` span in a + double-quoted `--text` argument is run by the shell before the CLI starts, + which changed the message while the send still reported success. + `--file` / `-f` is unchanged: it attaches a file, it does not supply text. +- Inline `--text` warns when its value contains `` ` ``, `$(` or `${`. The + warning names the habit, not a detected fault: a marker that reaches the CLI + proves the shell did not expand it that time, and a span the shell did expand + leaves no trace to detect. The message is sent either way. - `search` command: full-text message search across every conversation the authenticated user can see, backed by Slack's `search.messages` method. Takes `-q`/`--query` (required), `-l`/`--limit` (default 20), and diff --git a/README.md b/README.md index 8e4ea63..639c3fc 100644 --- a/README.md +++ b/README.md @@ -309,17 +309,26 @@ dart run bin/dart_slack.dart search -q "in:#incidents from:@lee after:2026-08-01 dart run bin/dart_slack.dart search -q "deploy failed" -c C0123ABCDEF dart run bin/dart_slack.dart search -q "deploy failed" -c incidents -# Send a message +# Send a message. Write the text to a file first, then pass the path. +# A shell runs backticks and $( ) inside a double-quoted argument, so +# inline -t can change the message before the CLI starts. See "Message text". +dart run bin/dart_slack.dart send -c --text-file ./message.md + +# The same text from standard input +printf 'Hello from the CLI\n' | + dart run bin/dart_slack.dart send -c --text-stdin + +# Short text with no backtick, no $( and no newline is safe inline dart run bin/dart_slack.dart send -c -t "Hello from the CLI" -# Send a message with a file attached -dart run bin/dart_slack.dart send -c -t "See attached" -f ./report.pdf +# Send a message with a file attached. --file attaches, --text-file is the text. +dart run bin/dart_slack.dart send -c --text-file ./comment.md -f ./report.pdf # Reply to a thread -dart run bin/dart_slack.dart reply -c -r -t "Thread reply" +dart run bin/dart_slack.dart reply -c -r --text-file ./reply.md # Edit a message -dart run bin/dart_slack.dart edit -c --ts -t "Updated text" +dart run bin/dart_slack.dart edit -c --ts --text-file ./new.md # Delete a message dart run bin/dart_slack.dart delete -c --ts @@ -343,10 +352,10 @@ dart run bin/dart_slack.dart canvas edit --canvas --mode append -m " dart run bin/dart_slack.dart canvas delete --canvas # Send a DM -dart run bin/dart_slack.dart dm -u -t "Hey there" +dart run bin/dart_slack.dart dm -u --text-file ./message.md # Send a DM with a file attached -dart run bin/dart_slack.dart dm -u -t "Here you go" -f ./notes.txt +dart run bin/dart_slack.dart dm -u --text-file ./comment.md -f ./notes.txt # List workspace users dart run bin/dart_slack.dart users @@ -398,6 +407,62 @@ stored token does not gain a scope on its own. Without it, Slack answers ### Message text +`send`, `reply`, `dm`, and `edit` take the message text from exactly one of +three sources. Giving none of them, or more than one, is a usage error. + +| Source | Passes through a shell | Bytes sent | +| --- | --- | --- | +| `--text-file ` | no | exactly the file contents | +| `--text-stdin` (or `--text-file -`) | no | exactly the standard input | +| `-t` / `--text` | YES | normalized, see below | + +`--file` / `-f` is a different option. It attaches a file to the message. +`--text-file` supplies the text of the message. + +#### Why the inline option is the unsafe one + +A shell expands a backtick span and a `$( )` span inside a double-quoted +argument. It runs them BEFORE this CLI starts, so the CLI never sees the text +the author wrote: + +```sh +python3 -c 'import sys; print(repr(sys.argv[1]))' "field is `printf changed`" +# 'field is changed' +``` + +The marked-up span is gone and the command output is spliced in. The send then +succeeds, so nothing reports the change. The CLI cannot detect this, because +the shell destroys the evidence before the process starts. The only fix is to +keep the text out of the argument list. + +`--text` therefore prints a warning when its value contains `` ` ``, `$(` or +`${`. Read that warning as "this command line is a risky habit", not as "this +message was damaged": a marker that arrives intact proves the shell did NOT +run it that time. + +```sh +# Safe. The bytes never meet a shell. +cat > message.md <<'EOF' +Run `dart test` and check $(pwd). +EOF +dart run bin/dart_slack.dart send -c C0123ABCDEF --text-file message.md + +# Also safe, and no temporary file. +dart run bin/dart_slack.dart send -c C0123ABCDEF --text-stdin <<'EOF' +Run `dart test` and check $(pwd). +EOF +``` + +File and standard-input bytes are sent unchanged. They are NOT normalized: no +unescaping, no line-ending rewrite, no control-character stripping. There is no +shell between the author and those bytes, so there is nothing to repair, and +repairing would corrupt a file that deliberately holds the two characters `\` +and `n`. + +#### Normalization of inline `--text` + +The rest of this section applies to `-t` / `--text` only. + `send`, `reply`, `dm`, and `edit` normalize the `--text` argument before they call the Slack API. A plain double-quoted shell string does not interpret backslash escapes, so `-t "line1\nline2"` sends the two literal characters `\` diff --git a/lib/src/cli/commands/dm_command.dart b/lib/src/cli/commands/dm_command.dart index e29522c..d371e43 100644 --- a/lib/src/cli/commands/dm_command.dart +++ b/lib/src/cli/commands/dm_command.dart @@ -1,20 +1,19 @@ import 'dart:io'; import 'package:dart_slack/src/cli/commands/authenticated_command.dart'; -import 'package:dart_slack/src/cli/message_text.dart'; +import 'package:dart_slack/src/cli/message_text_input.dart'; import 'package:dart_slack/src/slack.dart'; import 'package:mason_logger/mason_logger.dart'; -/// `dart_slack dm --user --text "message" [--file ]` +/// `dart_slack dm --user --text-file [--file ]` +/// +/// The message text comes from exactly one of `--text`, `--text-file` or +/// `--text-stdin`. `--file` attaches a file and is a different option. /// /// Sends a direct message to a Slack user. -class DmCommand extends AuthenticatedCommand { +class DmCommand extends AuthenticatedCommand with MessageTextCommand { /// Creates a [DmCommand]. - DmCommand({ - required super.logger, - super.credentialsStore, - super.httpClient, - }) { + DmCommand({required super.logger, super.credentialsStore, super.httpClient}) { argParser ..addOption( 'user', @@ -22,17 +21,8 @@ class DmCommand extends AuthenticatedCommand { help: 'The user ID to message.', mandatory: true, ) - ..addOption( - 'text', - abbr: 't', - help: 'The message text.', - mandatory: true, - ) - ..addOption( - 'file', - abbr: 'f', - help: 'Path to a local file to attach.', - ); + ..addOption('file', abbr: 'f', help: 'Path to a local file to attach.'); + addMessageTextOptions(argParser, help: 'The message text.'); } @override @@ -44,7 +34,6 @@ class DmCommand extends AuthenticatedCommand { @override Future runAuthenticated(Slack slack) async { final user = argResults!['user'] as String; - final text = normalizeMessageText(argResults!['text'] as String); final filePath = argResults!['file'] as String?; if (filePath != null) { @@ -56,13 +45,13 @@ class DmCommand extends AuthenticatedCommand { final filename = await slack.uploadFile( channel: channel, path: filePath, - comment: text, + comment: messageText, ); logger.success('File "$filename" sent to $user.'); return ExitCode.success.code; } - final message = await slack.postMessage(channel: user, text: text); + final message = await slack.postMessage(channel: user, text: messageText); logger.success('DM sent to $user (ts: ${message.ts}).'); return ExitCode.success.code; } diff --git a/lib/src/cli/commands/edit_command.dart b/lib/src/cli/commands/edit_command.dart index d100020..ce4b661 100644 --- a/lib/src/cli/commands/edit_command.dart +++ b/lib/src/cli/commands/edit_command.dart @@ -1,12 +1,15 @@ import 'package:dart_slack/src/cli/commands/authenticated_command.dart'; -import 'package:dart_slack/src/cli/message_text.dart'; +import 'package:dart_slack/src/cli/message_text_input.dart'; import 'package:dart_slack/src/slack.dart'; import 'package:mason_logger/mason_logger.dart'; -/// `dart_slack edit --channel --ts --text "new text"` +/// `dart_slack edit --channel --ts --text-file ` +/// +/// The new text comes from exactly one of `--text`, `--text-file` or +/// `--text-stdin`. /// /// Edits an existing Slack message. -class EditCommand extends AuthenticatedCommand { +class EditCommand extends AuthenticatedCommand with MessageTextCommand { /// Creates an [EditCommand]. EditCommand({ required super.logger, @@ -24,13 +27,8 @@ class EditCommand extends AuthenticatedCommand { 'ts', help: 'The message timestamp to edit.', mandatory: true, - ) - ..addOption( - 'text', - abbr: 't', - help: 'The new message text.', - mandatory: true, ); + addMessageTextOptions(argParser, help: 'The new message text.'); } @override @@ -43,9 +41,8 @@ class EditCommand extends AuthenticatedCommand { Future runAuthenticated(Slack slack) async { final channel = argResults!['channel'] as String; final ts = argResults!['ts'] as String; - final text = normalizeMessageText(argResults!['text'] as String); - await slack.updateMessage(channel: channel, ts: ts, text: text); + await slack.updateMessage(channel: channel, ts: ts, text: messageText); logger.success('Message $ts in $channel updated.'); return ExitCode.success.code; } diff --git a/lib/src/cli/commands/reply_command.dart b/lib/src/cli/commands/reply_command.dart index e4712f2..a22234e 100644 --- a/lib/src/cli/commands/reply_command.dart +++ b/lib/src/cli/commands/reply_command.dart @@ -1,15 +1,18 @@ import 'dart:io'; import 'package:dart_slack/src/cli/commands/authenticated_command.dart'; -import 'package:dart_slack/src/cli/message_text.dart'; +import 'package:dart_slack/src/cli/message_text_input.dart'; import 'package:dart_slack/src/slack.dart'; import 'package:mason_logger/mason_logger.dart'; -/// `dart_slack reply --channel --thread --text "message"` +/// `dart_slack reply --channel --thread --text-file ` /// `[--file ]` /// +/// The reply text comes from exactly one of `--text`, `--text-file` or +/// `--text-stdin`. `--file` attaches a file and is a different option. +/// /// Replies to a message thread in a Slack channel. -class ReplyCommand extends AuthenticatedCommand { +class ReplyCommand extends AuthenticatedCommand with MessageTextCommand { /// Creates a [ReplyCommand]. ReplyCommand({ required super.logger, @@ -29,17 +32,8 @@ class ReplyCommand extends AuthenticatedCommand { help: 'The thread timestamp (thread_ts) to reply to.', mandatory: true, ) - ..addOption( - 'text', - abbr: 't', - help: 'The reply text.', - mandatory: true, - ) - ..addOption( - 'file', - abbr: 'f', - help: 'Path to a local file to attach.', - ); + ..addOption('file', abbr: 'f', help: 'Path to a local file to attach.'); + addMessageTextOptions(argParser, help: 'The reply text.'); } @override @@ -52,7 +46,6 @@ class ReplyCommand extends AuthenticatedCommand { Future runAuthenticated(Slack slack) async { final channel = argResults!['channel'] as String; final thread = argResults!['thread'] as String; - final text = normalizeMessageText(argResults!['text'] as String); final filePath = argResults!['file'] as String?; if (filePath != null) { @@ -64,7 +57,7 @@ class ReplyCommand extends AuthenticatedCommand { channel: channel, path: filePath, threadTs: thread, - comment: text, + comment: messageText, ); logger.success('File "$filename" sent to thread $thread in $channel.'); return ExitCode.success.code; @@ -72,7 +65,7 @@ class ReplyCommand extends AuthenticatedCommand { final message = await slack.postMessage( channel: channel, - text: text, + text: messageText, threadTs: thread, ); logger.success( diff --git a/lib/src/cli/commands/send_command.dart b/lib/src/cli/commands/send_command.dart index f0a805f..ef8f2e7 100644 --- a/lib/src/cli/commands/send_command.dart +++ b/lib/src/cli/commands/send_command.dart @@ -1,14 +1,17 @@ import 'dart:io'; import 'package:dart_slack/src/cli/commands/authenticated_command.dart'; -import 'package:dart_slack/src/cli/message_text.dart'; +import 'package:dart_slack/src/cli/message_text_input.dart'; import 'package:dart_slack/src/slack.dart'; import 'package:mason_logger/mason_logger.dart'; -/// `dart_slack send --channel --text "message" [--file ]` +/// `dart_slack send --channel --text-file [--file ]` +/// +/// The message text comes from exactly one of `--text`, `--text-file` or +/// `--text-stdin`. `--file` attaches a file and is a different option. /// /// Posts a message to a Slack channel. -class SendCommand extends AuthenticatedCommand { +class SendCommand extends AuthenticatedCommand with MessageTextCommand { /// Creates a [SendCommand]. SendCommand({ required super.logger, @@ -22,17 +25,8 @@ class SendCommand extends AuthenticatedCommand { help: 'The channel ID to post to.', mandatory: true, ) - ..addOption( - 'text', - abbr: 't', - help: 'The message text.', - mandatory: true, - ) - ..addOption( - 'file', - abbr: 'f', - help: 'Path to a local file to attach.', - ); + ..addOption('file', abbr: 'f', help: 'Path to a local file to attach.'); + addMessageTextOptions(argParser, help: 'The message text.'); } @override @@ -44,7 +38,6 @@ class SendCommand extends AuthenticatedCommand { @override Future runAuthenticated(Slack slack) async { final channel = argResults!['channel'] as String; - final text = normalizeMessageText(argResults!['text'] as String); final filePath = argResults!['file'] as String?; if (filePath != null) { @@ -55,13 +48,16 @@ class SendCommand extends AuthenticatedCommand { final filename = await slack.uploadFile( channel: channel, path: filePath, - comment: text, + comment: messageText, ); logger.success('File "$filename" sent to $channel.'); return ExitCode.success.code; } - final message = await slack.postMessage(channel: channel, text: text); + final message = await slack.postMessage( + channel: channel, + text: messageText, + ); logger.success('Message sent to $channel (ts: ${message.ts}).'); return ExitCode.success.code; } diff --git a/lib/src/cli/message_text_input.dart b/lib/src/cli/message_text_input.dart new file mode 100644 index 0000000..cb70dda --- /dev/null +++ b/lib/src/cli/message_text_input.dart @@ -0,0 +1,219 @@ +import 'dart:convert'; +import 'dart:io'; +import 'dart:typed_data'; + +import 'package:args/args.dart'; +import 'package:dart_slack/src/cli/commands/authenticated_command.dart'; +import 'package:dart_slack/src/cli/message_text.dart'; +import 'package:mason_logger/mason_logger.dart'; + +/// Option that carries the message text on the command line. +const String textOption = 'text'; + +/// Option that names a file holding the message text. +const String textFileOption = 'text-file'; + +/// Flag that reads the message text from standard input. +const String textStdinFlag = 'text-stdin'; + +/// The value of [textFileOption] that means standard input. +const String stdinPathMarker = '-'; + +/// Spellings a POSIX shell expands inside a double-quoted argument. +/// +/// Each one turns message text into a command the shell runs before this +/// process starts. +const List shellSubstitutionMarkers = ['`', r'$(', r'${']; + +/// The UTF-8 encoding of U+FEFF, the byte-order mark. +const List utf8ByteOrderMark = [0xEF, 0xBB, 0xBF]; + +/// Decodes [bytes] as UTF-8 without losing a leading U+FEFF. +/// +/// Dart's UTF-8 decoder drops a leading byte-order mark, and +/// `File.readAsStringSync` uses that decoder. U+FEFF is a valid character, +/// so dropping it changes the message. The byte-for-byte promise of +/// `--text-file` and `--text-stdin` has to hold for it too, so this puts it +/// back when the bytes carried it. +String decodeMessageBytes(List bytes) { + final text = utf8.decode(bytes); + final hadMark = + bytes.length >= 3 && + bytes[0] == utf8ByteOrderMark[0] && + bytes[1] == utf8ByteOrderMark[1] && + bytes[2] == utf8ByteOrderMark[2]; + // UNCONDITIONAL when the bytes carried a mark. The decoder drops EXACTLY + // ONE leading mark, so one must go back, whatever the decoded text starts + // with. Testing `!text.startsWith('\uFEFF')` first was wrong and lost a + // character: two leading marks decode to one, that one satisfied the test, + // and the repair was skipped (kumar-waaf, review of #53). + if (hadMark) return '\uFEFF$text'; + return text; +} + +/// Reads every byte of standard input. +/// +/// The read is synchronous because [AuthenticatedCommand.validateArguments] +/// is synchronous, and the message text must be resolved before any +/// credential is touched. +List readStandardInputBytes() { + final buffer = BytesBuilder(copy: false); + while (true) { + final byte = stdin.readByteSync(); + if (byte < 0) break; + buffer.addByte(byte); + } + return buffer.takeBytes(); +} + +/// Reads standard input and decodes it as UTF-8. +/// +/// [readBytes] is the byte source, and a test replaces it. The split is +/// deliberate: the byte loop needs a real process to exercise, but every +/// way this function can change the text lives in the decode, so a test +/// that supplies bytes runs the whole of the part that can be wrong. +String readStandardInput({List Function()? readBytes}) => + decodeMessageBytes((readBytes ?? readStandardInputBytes)()); + +/// Gives a command three ways to receive message text, only one of which +/// passes that text through a shell. +/// +/// The inline `--text` option is the unsafe one. A backtick span or a `$(` +/// span inside a double-quoted shell argument runs as a command *before* +/// this process starts, so the text the tool receives is not the text the +/// author wrote, and the send still reports success. The shell leaves no +/// trace of what it removed, so the tool cannot detect that case: the only +/// fix is to keep the text out of the argument list. +/// +/// `--text-file ` and `--text-stdin` do that. Their bytes never reach +/// a shell, so they are sent unchanged. Both decode through +/// [decodeMessageBytes], which keeps a leading U+FEFF that the UTF-8 +/// decoder would otherwise drop. +/// +/// Two deliberate differences between the sources: +/// +/// * Inline text is passed through [normalizeMessageText], which repairs +/// shell-quoting damage such as a literal `\n`. File and standard-input +/// text is sent byte-for-byte. There is no shell between the author and +/// those bytes, so there is nothing to repair, and repairing would corrupt +/// a file that legitimately contains the two characters `\` and `n`. +/// * The backtick warning applies to inline text only, for the same reason. +mixin MessageTextCommand on AuthenticatedCommand { + late final String _messageText; + + /// Reads standard input. Replaced in tests. + String Function() readMessageStdin = readStandardInput; + + /// Adds `--text`, `--text-file` and `--text-stdin` to [parser]. + /// + /// [help] describes the text, for example `'The message text.'`. + void addMessageTextOptions(ArgParser parser, {required String help}) { + parser + ..addOption( + textOption, + abbr: 't', + help: + '$help Unsafe for text that contains a backtick or "\$(": a ' + 'shell runs those before this tool starts. Use --$textFileOption ' + 'or --$textStdinFlag for that text.', + ) + ..addOption( + textFileOption, + help: + 'Read the message text from this file, byte for byte. Use ' + '"$stdinPathMarker" for standard input. This is the safe way to ' + 'pass text that contains backticks, quotes or newlines. It is ' + 'not --file, which attaches a file to the message.', + ) + ..addFlag( + textStdinFlag, + negatable: false, + help: 'Read the message text from standard input, byte for byte.', + ); + } + + /// The resolved message text. Valid only after [validateMessageText] + /// returns `null`. + String get messageText => _messageText; + + @override + int? validateArguments() => validateMessageText(); + + /// Resolves the message text from exactly one of the three sources. + /// + /// Returns the exit code to stop with, or `null` to continue. + int? validateMessageText() { + final inline = argResults![textOption] as String?; + final textFile = argResults![textFileOption] as String?; + final stdinFlag = argResults![textStdinFlag] as bool? ?? false; + + final fromStdin = stdinFlag || textFile == stdinPathMarker; + final fromFile = textFile != null && textFile != stdinPathMarker; + final sources = [inline != null, fromFile, fromStdin].where((e) => e); + + if (sources.isEmpty) { + return _messageTextUsageError( + 'Missing the message text. Give it with exactly one of ' + '--$textOption, --$textFileOption or --$textStdinFlag.', + ); + } + if (sources.length > 1) { + return _messageTextUsageError( + 'Conflicting message text. Give exactly one of --$textOption, ' + '--$textFileOption or --$textStdinFlag.', + ); + } + + if (fromStdin) { + _messageText = readMessageStdin(); + return null; + } + + if (fromFile) { + final file = File(textFile); + if (!file.existsSync()) { + logger.err('Message text file not found: $textFile'); + return ExitCode.noInput.code; + } + try { + _messageText = decodeMessageBytes(file.readAsBytesSync()); + } on FileSystemException catch (e) { + logger.err('Cannot read the message text file $textFile: ${e.message}'); + return ExitCode.noInput.code; + } + return null; + } + + _warnAboutShellSubstitution(inline!); + _messageText = normalizeMessageText(inline); + return null; + } + + /// Warns when inline text carries a shell substitution spelling. + /// + /// The warning is not a detector of damage already done. A span the shell + /// ran is gone before this process starts, so a marker that arrives here + /// proves the shell did *not* run it this time. The warning names the + /// habit, because the same command line under different quoting silently + /// sends a different message. + void _warnAboutShellSubstitution(String text) { + final found = shellSubstitutionMarkers.where(text.contains).toList(); + if (found.isEmpty) return; + final markers = found.map((m) => '"$m"').join(' and '); + logger.warn( + 'The --$textOption value contains $markers. A shell runs that ' + 'spelling as a command before this tool starts. It arrived here ' + 'intact, so the shell did not run it this time. A span the shell ' + 'does run leaves no trace and the send still reports success. Use ' + '--$textFileOption or --$textStdinFlag for text like this.', + ); + } + + int _messageTextUsageError(String message) { + logger + ..err(message) + ..info('') + ..info(usage); + return ExitCode.usage.code; + } +} diff --git a/test/src/cli/message_text_input_test.dart b/test/src/cli/message_text_input_test.dart new file mode 100644 index 0000000..5b003f7 --- /dev/null +++ b/test/src/cli/message_text_input_test.dart @@ -0,0 +1,485 @@ +import 'dart:convert'; +import 'dart:io'; + +import 'package:args/command_runner.dart'; +import 'package:dart_slack/src/auth/credentials.dart'; +import 'package:dart_slack/src/auth/credentials_store.dart'; +import 'package:dart_slack/src/cli/commands/authenticated_command.dart'; +import 'package:dart_slack/src/cli/commands/dm_command.dart'; +import 'package:dart_slack/src/cli/commands/edit_command.dart'; +import 'package:dart_slack/src/cli/commands/reply_command.dart'; +import 'package:dart_slack/src/cli/commands/send_command.dart'; +import 'package:dart_slack/src/cli/message_text_input.dart'; +import 'package:http/http.dart' as http; +import 'package:mason_logger/mason_logger.dart'; +import 'package:mocktail/mocktail.dart'; +import 'package:test/test.dart'; + +class _MockLogger extends Mock implements Logger {} + +class _MockCredentialsStore extends Mock implements CredentialsStore {} + +class _MockHttpClient extends Mock implements http.Client {} + +/// One message-writing command, with the arguments that address it. +class _Subject { + const _Subject(this.name, this.build, this.address); + + final String name; + final AuthenticatedCommand Function({ + required Logger logger, + CredentialsStore? credentialsStore, + http.Client? httpClient, + }) + build; + + /// The arguments before the text options, `send` excluded. + final List address; +} + +const _subjects = <_Subject>[ + _Subject('send', SendCommand.new, ['-c', 'C1']), + _Subject('reply', ReplyCommand.new, ['-c', 'C1', '-r', '1.0']), + _Subject('dm', DmCommand.new, ['-u', 'U1']), + _Subject('edit', EditCommand.new, ['-c', 'C1', '--ts', '1.0']), +]; + +const _credentials = Credentials(accessToken: 'xoxp-test', userId: 'U123'); + +void main() { + setUpAll(() { + registerFallbackValue(Uri.parse('https://example.com')); + }); + + for (final subject in _subjects) { + group('${subject.name} message text input', () { + late _MockLogger logger; + late _MockCredentialsStore credentialsStore; + late _MockHttpClient httpClient; + late AuthenticatedCommand command; + late CommandRunner runner; + late List sentBodies; + + setUp(() { + logger = _MockLogger(); + credentialsStore = _MockCredentialsStore(); + httpClient = _MockHttpClient(); + sentBodies = []; + + when(() => logger.success(any())).thenReturn(null); + when(() => logger.err(any())).thenReturn(null); + when(() => logger.warn(any())).thenReturn(null); + when(() => logger.info(any())).thenReturn(null); + when(() => httpClient.close()).thenReturn(null); + when(() => credentialsStore.load()).thenReturn(_credentials); + when( + () => httpClient.post( + any(), + headers: any(named: 'headers'), + body: any(named: 'body'), + ), + ).thenAnswer((invocation) async { + sentBodies.add(invocation.namedArguments[#body] as String); + return http.Response( + jsonEncode({ + 'ok': true, + 'channel': 'C1', + 'ts': '1.0', + 'message': {'text': 'x'}, + }), + 200, + ); + }); + + command = subject.build( + logger: logger, + credentialsStore: credentialsStore, + httpClient: httpClient, + ); + runner = CommandRunner('test', 'test')..addCommand(command); + }); + + Future run(List textArgs) => + runner.run([subject.name, ...subject.address, ...textArgs]); + + String sentText() => + (jsonDecode(sentBodies.last) as Map)['text'] + as String; + + void readsStdin(String value) { + (command as MessageTextCommand).readMessageStdin = () => value; + } + + File writeTextBytes(List bytes) { + final file = File( + '${Directory.systemTemp.createTempSync('dart_slack_bytes').path}' + '/message.txt', + )..writeAsBytesSync(bytes); + addTearDown(() => file.parent.deleteSync(recursive: true)); + return file; + } + + /// Installs the REAL `readStandardInput` with a supplied byte source, + /// so the command's standard-input path runs the real decode rather + /// than a string the test handed it. + void readsStdinBytes(List bytes) { + (command as MessageTextCommand).readMessageStdin = () => + readStandardInput(readBytes: () => bytes); + } + + /// The UTF-8 bytes of the message the command actually sent. + List sentTextBytes() => utf8.encode(sentText()); + + File writeTextFile(String contents) { + final file = File( + '${Directory.systemTemp.createTempSync('dart_slack_text').path}' + '/message.txt', + )..writeAsStringSync(contents); + addTearDown(() => file.parent.deleteSync(recursive: true)); + return file; + } + + test('declares the three text sources and no longer forces --text', () { + final options = command.argParser.options; + + // `--text` was `mandatory: true`. That is what forced every caller + // through a shell argument, so it has to be false now. The other two + // options are the safe paths this issue adds. + expect(options[textOption]?.mandatory, isFalse); + expect(options[textOption]?.abbr, equals('t')); + expect(options[textFileOption], isNotNull); + expect(options[textFileOption]?.abbr, isNull); + expect(options[textStdinFlag], isNotNull); + }); + + test('--text-file delivers the bytes unchanged', () async { + // Every character the shell would have eaten, in one file. + const raw = + 'a `printf changed` b \$(id) c \${HOME} d "q" e \'s\'\n' + 'second line\ttab'; + final file = writeTextFile(raw); + + final exitCode = await run(['--text-file', file.path]); + + expect(exitCode, equals(ExitCode.success.code)); + expect(sentText(), equals(raw)); + }); + + test('--text-stdin delivers the bytes unchanged', () async { + // The literal backslash-n and the carriage return are here on + // purpose. Without them this test survives a mutant that normalizes + // the standard-input bytes, and the byte-identity promise is then + // guarded on the file path only. + const raw = + 'x `printf changed` y \$(id) z \${HOME}\n' + r'keep \n and \t literal' + '\r\nafter a carriage return\ttab'; + readsStdin(raw); + + final exitCode = await run(['--text-stdin']); + + expect(exitCode, equals(ExitCode.success.code)); + expect(sentText(), equals(raw)); + }); + + test('--text-file - reads standard input', () async { + readsStdin('from stdin'); + + final exitCode = await run(['--text-file', '-']); + + expect(exitCode, equals(ExitCode.success.code)); + expect(sentText(), equals('from stdin')); + }); + + test('file text keeps a literal backslash-n', () async { + // The inline path unescapes this, because a shell cannot pass a real + // newline in a double-quoted argument. A file can, so unescaping file + // bytes would corrupt a code sample that contains the two characters. + final file = writeTextFile(r'line1\nline2'); + + await run(['--text-file', file.path]); + + expect(sentText(), equals(r'line1\nline2')); + expect(sentText(), isNot(contains('\n'))); + }); + + test('stdin text keeps a literal backslash-n', () async { + // The mirror of the file case. The inline path unescapes this; the + // standard-input path must not, for the same reason. + readsStdin(r'line1\nline2'); + + await run(['--text-stdin']); + + expect(sentText(), equals(r'line1\nline2')); + expect(sentText(), isNot(contains('\n'))); + }); + + test('inline text is still repaired, as before', () async { + await run(['-t', r'line1\nline2']); + + expect(sentText(), equals('line1\nline2')); + }); + + test('rejects --text together with --text-file', () async { + final file = writeTextFile('from file'); + + final exitCode = await run(['-t', 'inline', '--text-file', file.path]); + + expect(exitCode, equals(ExitCode.usage.code)); + expect(sentBodies, isEmpty); + }); + + test('rejects --text together with --text-stdin', () async { + readsStdin('from stdin'); + + final exitCode = await run(['-t', 'inline', '--text-stdin']); + + expect(exitCode, equals(ExitCode.usage.code)); + expect(sentBodies, isEmpty); + }); + + test('rejects --text-file together with --text-stdin', () async { + final file = writeTextFile('from file'); + + final exitCode = await run([ + '--text-file', + file.path, + '--text-stdin', + ]); + + expect(exitCode, equals(ExitCode.usage.code)); + expect(sentBodies, isEmpty); + }); + + test('accepts --text-file - with --text-stdin, one source', () async { + readsStdin('one source'); + + final exitCode = await run(['--text-file', '-', '--text-stdin']); + + expect(exitCode, equals(ExitCode.success.code)); + expect(sentText(), equals('one source')); + }); + + test('rejects no text source at all', () async { + final exitCode = await run([]); + + expect(exitCode, equals(ExitCode.usage.code)); + expect(sentBodies, isEmpty); + }); + + test('reports a missing text file and sends nothing', () async { + final exitCode = await run([ + '--text-file', + '${Directory.systemTemp.path}/dart_slack_absent_message.txt', + ]); + + expect(exitCode, equals(ExitCode.noInput.code)); + expect(sentBodies, isEmpty); + }); + + test('answers a text usage error without reading a credential', () async { + // The same rule `search` follows: a usage error must not depend on + // the auth state, or a logged-out caller is told the wrong thing. + final exitCode = await run([]); + + expect(exitCode, equals(ExitCode.usage.code)); + verifyNever(() => credentialsStore.load()); + }); + + test( + 'warns about a backtick in inline --text, and still sends', + () async { + final exitCode = await run(['-t', 'see `code` here']); + + expect(exitCode, equals(ExitCode.success.code)); + expect(sentText(), equals('see `code` here')); + final warning = verify( + () => logger.warn(captureAny()), + ).captured.single; + expect(warning, contains('--$textFileOption')); + expect(warning, contains('`')); + }, + ); + + test(r'warns about $( in inline --text', () async { + await run(['-t', r'value $(id) here']); + + verify(() => logger.warn(any())).called(1); + }); + + test(r'warns about ${ in inline --text', () async { + await run(['-t', r'value ${HOME} here']); + + verify(() => logger.warn(any())).called(1); + }); + + test('does not warn about ordinary inline text', () async { + await run(['-t', 'ordinary text with a "quote"']); + + verifyNever(() => logger.warn(any())); + }); + + test('does not warn about a backtick that came from a file', () async { + // File bytes never passed through a shell, so the warning would be + // noise on the very path callers are being moved to. + final file = writeTextFile('see `code` here'); + + await run(['--text-file', file.path]); + + verifyNever(() => logger.warn(any())); + }); + + // --- U+FEFF, through the COMMAND, on both safe paths ------------- + // + // The decoder-level tests below prove `decodeMessageBytes`. They do + // not prove the command uses it: restoring `readAsStringSync()` in + // the file branch left the whole file green (kumar-waaf). These + // compare the UTF-8 bytes of the SENT message against the input + // bytes, so only the real command path can satisfy them. + const bom = [0xEF, 0xBB, 0xBF]; + const hello = [0x68, 0x65, 0x6C, 0x6C, 0x6F]; + + for (final shape in <({String name, List bytes})>[ + (name: 'a single leading mark', bytes: [...bom, ...hello]), + ( + name: 'two consecutive leading marks', + bytes: [...bom, ...bom, ...hello], + ), + ( + name: 'three consecutive leading marks', + bytes: [...bom, ...bom, ...bom, ...hello], + ), + (name: 'a non-leading mark only', bytes: [...hello, ...bom, ...hello]), + ( + name: 'a leading mark and a separated one', + bytes: [...bom, ...hello, ...bom, ...hello], + ), + (name: 'a mark and nothing else', bytes: [...bom]), + ]) { + test('--text-file sends ${shape.name} unchanged', () async { + final file = writeTextBytes(shape.bytes); + + final exitCode = await run(['--text-file', file.path]); + + expect(exitCode, equals(ExitCode.success.code)); + expect(sentTextBytes(), equals(shape.bytes)); + }); + + test('--text-stdin sends ${shape.name} unchanged', () async { + readsStdinBytes(shape.bytes); + + final exitCode = await run(['--text-stdin']); + + expect(exitCode, equals(ExitCode.success.code)); + expect(sentTextBytes(), equals(shape.bytes)); + }); + } + + test('does not warn about a backtick that came from stdin', () async { + readsStdin('see `code` here'); + + await run(['--text-stdin']); + + verifyNever(() => logger.warn(any())); + }); + }); + } + + _byteOrderMarkGroup(); +} + +/// Checks the real `readStandardInput()` and the real file reader. +/// +/// A leading U+FEFF is the case the injected-reader tests cannot reach. +/// Dart's UTF-8 decoder drops it, and `File.readAsStringSync` uses that +/// decoder, so both safe paths lost a valid character before this guard. +void _byteOrderMarkGroup() { + group('leading U+FEFF survives the safe input paths', () { + const bomBytes = [0xEF, 0xBB, 0xBF]; + const helloBytes = [0x68, 0x65, 0x6C, 0x6C, 0x6F]; + + test('decodeMessageBytes keeps it', () { + expect(decodeMessageBytes([...bomBytes, ...helloBytes]).codeUnits, [ + 0xFEFF, + ...helloBytes, + ]); + }); + + test('decodeMessageBytes keeps BOTH of two consecutive leading marks', () { + // The decoder drops exactly ONE. A repair conditioned on the decoded + // text not already starting with U+FEFF skips this case and loses a + // character (kumar-waaf, review of #53). + expect( + decodeMessageBytes([...bomBytes, ...bomBytes, ...helloBytes]).codeUnits, + [0xFEFF, 0xFEFF, ...helloBytes], + ); + }); + + test('decodeMessageBytes keeps all three of three leading marks', () { + expect( + decodeMessageBytes([ + ...bomBytes, + ...bomBytes, + ...bomBytes, + ...helloBytes, + ]).codeUnits, + [0xFEFF, 0xFEFF, 0xFEFF, ...helloBytes], + ); + }); + + test('decodeMessageBytes keeps a mark that is the whole input', () { + expect(decodeMessageBytes(bomBytes).codeUnits, [0xFEFF]); + }); + + test('decodeMessageBytes keeps a second U+FEFF too', () { + // Only the leading mark is dropped by the decoder. A mark further in + // must not be duplicated or moved by the repair. + expect( + decodeMessageBytes([...bomBytes, 0x61, ...bomBytes, 0x62]).codeUnits, + [0xFEFF, 0x61, 0xFEFF, 0x62], + ); + }); + + test('decodeMessageBytes leaves text without a mark alone', () { + expect(decodeMessageBytes(helloBytes), equals('hello')); + }); + + test('the real file reader keeps it', () { + final dir = Directory.systemTemp.createTempSync('dart_slack_bom'); + addTearDown(() => dir.deleteSync(recursive: true)); + final file = File('${dir.path}/message.txt') + ..writeAsBytesSync([...bomBytes, ...helloBytes]); + + expect(decodeMessageBytes(file.readAsBytesSync()).codeUnits, [ + 0xFEFF, + ...helloBytes, + ]); + // The reader this replaced, named so the regression is unmistakable. + expect(file.readAsStringSync(), equals('hello')); + }); + + test('the real readStandardInput keeps it', () { + // This drives `readStandardInput` itself, not the test double the + // command-level stdin tests install. Only the byte loop is replaced, + // so the decode that dropped the mark runs for real here. + expect( + readStandardInput( + readBytes: () => [...bomBytes, ...helloBytes], + ).codeUnits, + equals([0xFEFF, ...helloBytes]), + ); + }); + + test( + 'the real readStandardInput does not repair a literal backslash-n', + () { + // The same seam guards the other promise of this path. A mutant that + // normalizes inside `readStandardInput` is caught here. + expect( + readStandardInput(readBytes: () => utf8.encode(r'line1\nline2')), + equals(r'line1\nline2'), + ); + }, + ); + }); +} diff --git a/test/src/commands/dm_command_test.dart b/test/src/commands/dm_command_test.dart index 33efb29..7607707 100644 --- a/test/src/commands/dm_command_test.dart +++ b/test/src/commands/dm_command_test.dart @@ -31,10 +31,16 @@ void main() { expect(command.description, isNotEmpty); }); - test('requires user and text options', () { + test('requires user, and offers three text sources', () { final options = command.argParser.options; expect(options['user']?.mandatory, isTrue); - expect(options['text']?.mandatory, isTrue); + // `--text` is no longer mandatory: `--text-file` and + // `--text-stdin` are the safe sources. Exactly one of the three + // is required at run time, proved in + // test/src/cli/message_text_input_test.dart. + expect(options['text']?.mandatory, isFalse); + expect(options['text-file'], isNotNull); + expect(options['text-stdin'], isNotNull); }); test('has correct abbreviations', () { diff --git a/test/src/commands/reply_command_test.dart b/test/src/commands/reply_command_test.dart index 2b9340d..7c6c37d 100644 --- a/test/src/commands/reply_command_test.dart +++ b/test/src/commands/reply_command_test.dart @@ -35,7 +35,13 @@ void main() { final options = command.argParser.options; expect(options['channel']?.mandatory, isTrue); expect(options['thread']?.mandatory, isTrue); - expect(options['text']?.mandatory, isTrue); + // `--text` is no longer mandatory: `--text-file` and + // `--text-stdin` are the safe sources. Exactly one of the three + // is required at run time, proved in + // test/src/cli/message_text_input_test.dart. + expect(options['text']?.mandatory, isFalse); + expect(options['text-file'], isNotNull); + expect(options['text-stdin'], isNotNull); }); test('has correct abbreviations', () { diff --git a/test/src/commands/send_command_test.dart b/test/src/commands/send_command_test.dart index 0d11eca..afd71c8 100644 --- a/test/src/commands/send_command_test.dart +++ b/test/src/commands/send_command_test.dart @@ -31,10 +31,16 @@ void main() { expect(command.description, isNotEmpty); }); - test('requires channel and text options', () { + test('requires channel, and offers three text sources', () { final options = command.argParser.options; expect(options['channel']?.mandatory, isTrue); - expect(options['text']?.mandatory, isTrue); + // `--text` is no longer mandatory: `--text-file` and + // `--text-stdin` are the safe sources. Exactly one of the three + // is required at run time, proved in + // test/src/cli/message_text_input_test.dart. + expect(options['text']?.mandatory, isFalse); + expect(options['text-file'], isNotNull); + expect(options['text-stdin'], isNotNull); }); test('has correct abbreviations', () {