Repository navigation
Escape control characters in the tong approval prompt - #93
Merged
Merged
Conversation
The privilege summary printed before the y/N question interpolated values straight from a workspace tong definition, which is untrusted, and the YAML reader passes raw control bytes through. A value carrying terminal escape sequences could therefore hide what the reviewer is being asked to approve: cursor movement can rewrite or erase earlier lines, and a mode switch such as concealed text hides every line after it. Either way a grant (a secret, a mount, the docker socket, the host's GPUs) could be approved unseen. The gate runs before schema validation, and secret references are collected from anywhere in the definition, unknown keys included, so a value that is never validated or resolved still reached the terminal. Every rendered line now passes through a new `tongs.printable`, which shows each non-printable character as its Python escape (`\x1b`, `\n`, `\u202e`). Escaping whole lines rather than each interpolated value means a field added to the summary later is covered without anyone having to remember to wrap it, while the summary's own line breaks are added after escaping and stay intact. The cut is `str.isprintable`, so besides C0, DEL, and C1 controls it also escapes bidi overrides, zero-width and other format characters, and non-ASCII spaces: none belongs in an image, mount, network, or secret reference, and each can make a value look like something it is not. Ordinary text, non-ASCII letters included, renders unchanged. A few printable characters are still blank or confusable, so lookalikes are not caught; the guarantee is that no value can move the cursor, restyle, break a line, or override the text direction. The tong name was already shown with %r, which escapes the same set.
When a tong file fails to parse, discovery warns with its path before the approval gate runs, on the same terminal. A workspace file name is untrusted and can carry escape sequences, and unlike the summary's own values these do not need to move the cursor: modes they switch on, such as concealed text or a shrunken scroll region, persist into the privilege summary and question printed right after, hiding them just as effectively. Run the warning through `printable`, the same escaping the summary now uses. The parse error text it includes is already repr-quoted, so only the path changes in practice.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #92 (GPU support for tongs): this PR also escapes the approval prompt's new
gpus:line. Merge #92 first; GitHub will then retarget this PR tomaster.Problem
The first-run approval gate shows what a workspace-sourced tong asks for (image, secret references, mounts, networks, docker socket, GPUs) before asking y/N. Those values come straight from a definition in a repo you cloned. The YAML reader passes raw control bytes through, and the gate runs before schema validation, so even a value validation would later reject reached the terminal unescaped. A crafted value could move the cursor, erase lines, or switch on concealed text, and so hide a grant such as the docker socket or a secret reference from the person approving it. A secret reference placed under an unknown key is enough: validation tolerates unknown keys and the summary scans the whole definition. This was already the case on
master; a review of the GPU work found it.What changes
tongs.printable(text)(swarmforge/tongs/model.py): replaces every characterstr.isprintable()rejects with its Python escape (\x1b,\n,). That covers C0 and C1 controls, DEL, bidi overrides, zero-width/format characters, and non-ASCII spaces. Ordinary text, including non-ASCII letters, prints unchanged, so a normal prompt looks exactly as before.render_privilege_summaryescapes each finished line before joining them, so the summary keeps its own line structure and any line added later is covered without anyone remembering to wrap it. The y/N question and the denial messages already render the tong name with%r, which escapes the same set.printabletoo.docs/tongs/approval.mdsays values are shown with control and formatting characters escaped.What is guaranteed: nothing the prompt prints from a definition can move the cursor, restyle the text, break a line, or reverse the text direction. Lookalike characters Python counts as printable (blank Hangul fillers, combining overlays) pass through unchanged. They can't hide a grant: each grant has its own fixed label line, and approval is keyed to the hash of the exact definition. Backslashes are left alone, so ordinary values look the same.
Testing
make test: 959 tests pass.ruff checkis clean.bin/run-anvil. Nothing reached the terminal unescaped before the y/N answer.Not in this PR
set -xecho, git-guard warnings) is still raw.'%s'; that name comes from a trusted layer or a file name.