fix(bin): send the Relay bearer to curl on stdin instead of a temp file - #2892
Open
cedmos wants to merge 2 commits into
Open
fix(bin): send the Relay bearer to curl on stdin instead of a temp file#2892cedmos wants to merge 2 commits into
cedmos wants to merge 2 commits into
Conversation
Process arguments are world readable via ps for the life of a call, so a bearer passed as `-H "Authorization: Bearer $tok"` exposes the credential to any local process. The fm-x-* relay clients did not do that - they already routed the header through a 0600 temp file - but that file is still a credential on disk kept alive by a cleanup trap, and a SIGKILL skips traps. Replace the temp file with a single shared owner, fmx_curl_config, that emits curl's url and header directives for `curl -K -` to read on stdin. The config is built with the printf builtin and piped in, so the token never becomes any process's argv and never touches disk at all. Values use curl's quoted-value syntax with backslash and double quote escaped; a token containing a newline is still refused outright. Converts all three request sites: fmx_post_json (answer, followup, request-context), fm-x-poll.sh, and fm-x-dismiss.sh. Tests assert the guarantee behaviourally: the fake curl records the command line it reads back from the OS, and each case additionally asserts the bearer still arrived in the auth header so a client that stopped authenticating could not pass. Reintroducing the argv form makes them fail. The interrupted-post test now proves the stronger property - no credential file is written at all - instead of that a trap removed it.
Confidence Score: 5/5The PR appears safe to merge, with no unaddressed blocking or independently publishable findings. The converted request paths preserve curl failure handling and keep request bodies separate from configuration stdin, while the remaining URL-validation and multi-call test concerns have already been communicated in the existing review. Reviews (1): Last reviewed commit: "no-mistakes(document): document Relay be..." | Re-trigger Greptile |
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
1 participant
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.
Intent
Keep the Bearer key out of curl argv in the Relay scripts.
THE DEFECT AS BRIEFED: process arguments are world-readable via ps, so -H "Authorization: Bearer $key" exposes the credential to any local process for the life of the call. A standing captain rule from today makes the ONLY allowed shape 'curl -K -', reading url and header from stdin.
IMPORTANT PREMISE CORRECTION FOUND WHILE WORKING (deliberate, not an oversight): the fm-x-* scripts were NOT passing the bearer in argv. All three request sites already routed the header through -H "@tempfile" (a mode-600 temp file written by fmx_auth_header_file). There is no -H "Authorization: Bearer $key" anywhere in this family. This was verified by tracing every FMX_TOKEN reference and every curl call site. The work was still done because the captain's rule mandates the 'curl -K -' shape regardless, and it is a genuine improvement: it removes a credential from disk whose cleanup depended on traps, and traps do not survive SIGKILL. Reviewers should not expect to find a removed argv leak in this diff.
SCOPE FENCE (non-negotiable, explicitly imposed): fix ONLY the bin/fm-x-*.sh family. Do NOT touch, for any reason: bin/flowy-fleet-watch.sh and bin/flowy-fleet-nudge.sh (another owner runs these), bin/fm-completion-outbox.sh (a live crew is fixing exactly this defect there right now), AGENTS.md, data/captain.md, .agents/skills/flowy-collab/ (firstmate is editing those). If a fix seems to need one of those, STOP and report instead of editing. Any finding that proposes editing those files must be escalated, not applied.
REQUIREMENTS AS ACCEPTED:
WHAT WAS BUILT AND WHY: a single shared owner, fmx_curl_config, emits curl's url and header directives for 'curl -K -' to read on stdin. Callers build it with the printf BUILTIN and pipe it in, so the token never becomes any process's argv and never touches disk. Converted all three request sites: fmx_post_json (answer/followup/request-context), fm-x-poll.sh, fm-x-dismiss.sh. Values use curl's quoted-value syntax with backslash and double-quote escaped; a newline-bearing token is still refused outright (kept from the old helper) because the config is line-oriented.
DELIBERATE DESIGN DECISIONS a reviewer might otherwise flag:
TESTING APPROACH (deliberate): tests/fm-x-mode.test.sh's fake curl now records psargv - the command line it reads back FROM THE OS via ps - and the new tests assert the token is absent from it. This measures the actual exposure rather than restating the source, satisfying the explicit 'behavioural, not source bytes' requirement. Each case ALSO asserts the bearer still arrived in the auth header, so a client that simply stopped authenticating could not pass vacuously. This was mutation-tested: reintroducing the argv form makes the tests fail with the real leaked command line. An awkward-token case pins the quote/backslash escaping round trip. The pre-existing interrupted-post test previously asserted that a trap REMOVED the auth temp file; since no such file is created any more, it was rewritten to prove the stronger property - no credential file is written at all, checked against a private TMPDIR. That rewrite is intentional, not a weakened test.
KNOWN PRE-EXISTING FAILURES, NOT CAUSED BY THIS CHANGE: tests/fm-public-followup.test.sh ('could not register the public commitment') and tests/fm-gotmp.test.sh ('teardown did not remove the tasktmp dir') each fail one test. Both were baselined by stashing this change and re-running on the clean base, where they fail identically. They are broken on main and are out of scope for this task.
What Changed
fmx_auth_header_filewithfmx_curl_config <url>inbin/fm-x-lib.sh: it emitsurlandheaderdirectives in curl's quoted-value syntax (escaping\and", still rejecting a token containing a newline or carriage return) and reads${FMX_TOKEN-}so it is safe underset -u. The 0600 auth temp file and itsEXIT/HUP/INT/TERMcleanup traps are gone — there is no longer a credential file to clean up.fmx_post_json,bin/fm-x-poll.sh, andbin/fm-x-dismiss.sh— to pipe that config intocurl -K -via builtinprintf, dropping the-H "@file"argument and the inline URL argument. Payloads still travel via--data/--data-binary @file, so nothing else contends for stdin.docs/architecture.mdnow namesfmx_curl_configas the single owner of how the token reaches curl.tests/fm-x-mode.test.sh: the fake curl parses-Kconfig from stdin or a file and recordspsargv, its own command line read back from the OS viaps. New poll/reply/dismiss cases assert the token is absent from that command line while the bearer still arrives in the auth header, plus a case pinning the quote/backslash escaping round trip. The interrupted-post test was rewritten to assert no credential file is written at all, checked against a privateTMPDIR.Risk Assessment
✅ Low: A well-bounded, single-purpose hardening of three curl call sites behind one shared helper: the escaping round-trips correctly against curl's actual config-value unescaping, exit-status and stdin semantics are unchanged (no pipefail, stdin consumed before the request), no stale references to the removed temp-file helper remain, the scope fence and commit-metadata constraints are honored, and both findings are minor robustness improvements rather than reachable defects.
Testing
Ran the colocated suite
tests/fm-x-mode.test.sh(110 ok, 0 not ok), then went past unit-level evidence: I drove the shippedfm-x-poll.shandfm-x-reply.shwith real curl against a real local HTTP listener while samplingpsfrom a separate process, and the bearer never appeared in curl's command line on either the GET or the POST path while the listener still received the correctAuthorization: Bearerheader and an intact JSON body. A control run of the briefed defective-H "Authorization: Bearer $tok"shape against the same listener did show the credential inps, so the check is measuring the real exposure. A private TMPDIR watched throughout the call held no credential file, whereas the same harness against the base commit caught the old mode-0600fm-x-auth.*file on disk — the concrete improvement this change makes. I also mutation-tested the new tests by reintroducing the argv form, which made them fail with the actual leaked command line, then reverted. The two test files the author flagged as broken on main fail identically after reverting this change's files to the base commit, so they are pre-existing and unrelated. No visual artifacts apply: this is a shell CLI and credential-handling change with no rendered surface. The transient harness was removed and the worktree is clean at the target commit.Evidence: End-to-end: bearer absent from curl argv, present in the relay's auth header (real curl, real listener, real ps)
Source: End-to-end: bearer absent from curl argv, present in the relay's auth header (real curl, real listener, real ps)
=== 1. SHIPPED CLIENT: bin/fm-x-poll.sh against http://127.0.0.1 (a real listener) === curl processes sampled from the OS (ps -A -o comm=,args=) for the life of the call: curl -K - -m 5 -s -o /…/fm-x-poll.BD4hjY -w %{http_code} -H Accept: application/json grep -c "s3cr3t-relay-bearer-DO-NOT-LEAK" <ps sample> -> 0 => the bearer is NOT in curl argv. === 2. ...and the request still authenticated (relay-side view) === GET /connector/poll authorization: Bearer s3cr3t-relay-bearer-DO-NOT-LEAK === 3. CONTROL: the briefed defective shape, same listener, same sampler === curl -m 5 -s -o /dev/null -w control http_code=%{http_code}\n -H Authorization: Bearer s3cr3t-relay-bearer-DO-NOT-LEAK -H Accept: application/json http://127.0.0.1:60222/connector/poll => ps DOES expose the credential for that shape. The measurement is real. === 4. Nothing on disk: private TMPDIR polled every 100ms for the whole call === files containing the token: (none) === 5. CONTRAST at base commit 822a990 (same harness) === -rw-------@ /…/tmp-base/fm-x-auth.al9NLA => the base client wrote a mode-0600 credential file to disk for the call. The target commit writes none at all. === 6. THE POST PATH: bin/fm-x-reply.sh -> POST /connector/answer (real socket) === $ fm-x-reply.sh req-e2e-001 "Aye captain - relay reply over a real socket." req-e2e-001 (exit 0) curl -K - -m 10 -s -o /…/fm-x-reply.CMTOcO -w %{http_code} -X POST -H Content-Type: application/json --data-binary @/…/fm-x-reply.FnBaiI token occurrences in that ps sample: 0 relay-side: POST /connector/answer authorization: Bearer s3cr3t-relay-bearer-DO-NOT-LEAK body: {"request_id":"req-e2e-001","text":"Aye captain - relay reply over a real socket."} => bearer absent from argv, bearer present in the header, JSON body intact - 'curl -K -' reading stdin does not contend with --data-binary @file.Evidence: Mutation check + pre-existing-failure baseline
Source: Mutation check + pre-existing-failure baseline
MUTATION CHECK - do the new tests actually catch the leak? Reintroduced-H "Authorization: Bearer $FMX_TOKEN"in bin/fm-x-poll.sh, re-ran the suite: not ok - poll leaked the bearer token into curl argv (visible via ps): psargv=bash /var/.../argv-poll/fakebin/curl -m 5 -s -o /var/.../fm-x-poll.fGk3d4 -w %{http_code} -H Authorization: Bearer tok-argv-poll -H Accept: application/json https://relay.test/connector/poll The failure carries the REAL leaked command line read back from the OS, not a source-text assertion. Mutation reverted; worktree restored to the target commit. TARGETED SUITE AT THE TARGET COMMIT: 110 ok, 0 not ok ok - fm-x-poll keeps the bearer token out of curl argv ok - fm-x-reply keeps the bearer token out of curl argv ok - fm-x-dismiss keeps the bearer token out of curl argv ok - the curl stdin config escapes quotes and backslashes in a token ok - fm-x-reply writes no credential temp file, even on an interrupted post PRE-EXISTING FAILURES, VERIFIED NOT CAUSED BY THIS CHANGE (the four changed files reverted to 822a990, same two test files re-run) tests/fm-gotmp.test.sh target: not ok - teardown did not remove the tasktmp dir base: not ok - teardown did not remove the tasktmp dir tests/fm-public-followup.test.sh target: PF_REGISTRY_LOCK_IDS[@]: unbound variable / not ok - could not register the public commitment base: PF_REGISTRY_LOCK_IDS[@]: unbound variable / not ok - could not register the public commitmentPipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
bin/fm-x-lib.sh:758- fmx_curl_config rejects a newline/CR in the token (line 753) but applies no such guard to the URL, which is emitted into the same line-orientedcurl -K -config. A newline in FMX_RELAY_URL splits the config into extra lines that curl parses as additional directives (e.g.output = ...,proxy = ..., a secondheader = ...). Reachability is limited: the .env path (fmx_env_get, bin/fm-x-lib.sh:56) is line-based and cannot yield a newline, and"is already escaped, so it requires an operator-set env var containing a literal newline - not a privilege-boundary crossing. Since this function is documented as the single owner of how the request reaches curl, extend the existingcase "$token" in *$'\n'*|*$'\r'*) return 1guard to cover $url as well.tests/fm-x-mode.test.sh:2905- assert_token_off_argv takestail -1of the^psargv=lines, so the OS-derived command line - the evidence the intent designates as the real behavioural check - is inspected for only the final relay call, while the weaker stub-reconstructed^argv=check (line 2910) scans every call. This is currently harmless: all four new cases issue exactly one relay call, because fmx_resolve_reply_context only reaches fmx_request_relay_context when allow_relay=1, which requires --followup, and none of these tests pass it. But the helper reads as general-purpose, so a follow-up-path case added later (request-context POST followed by the followup POST) would have its first call covered only by the$*-reconstructed string. Drop thetail -1and assert that no^psargv=line contains the token.✅ **Test** - passed
✅ No issues found.
bash tests/fm-x-mode.test.sh— 110 ok, 0 not ok, includingfm-x-poll/fm-x-reply/fm-x-dismiss keeps the bearer token out of curl argv,the curl stdin config escapes quotes and backslashes in a token, andfm-x-reply writes no credential temp file, even on an interrupted postMutation check: reintroduced-H "Authorization: Bearer $FMX_TOKEN"inbin/fm-x-poll.sh, re-ranbash tests/fm-x-mode.test.sh→not ok - poll leaked the bearer token into curl argv (visible via ps)carrying the real leaked command line; mutation reverted viagit checkout -- bin/fm-x-poll.shManual E2E (GET path): ran the shippedbin/fm-x-poll.shwith realcurlagainst a python3 HTTP listener on 127.0.0.1 that holds the response 2s, while samplingps -A -o comm=,args=every 100ms — sampled argv wascurl -K - -m 5 -s -o … -w %{http_code} -H Accept: application/json, zero token occurrences, and the listener receivedAuthorization: Bearer <token>Manual E2E (POST path): ranbin/fm-x-reply.sh req-e2e-001 "…"against the same listener — argv containedcurl -K - … -X POST … --data-binary @filewith zero token occurrences, listener received the bearer header and the intact JSON body (proves-K -stdin does not contend with the payload file)Control run:curl -m 5 -s -o /dev/null -w '%{http_code}' -H "Authorization: Bearer $TOKEN" http://127.0.0.1:$PORT/connector/pollunder the same sampler —psDOES expose the full credential, confirming the measurement is realOn-disk check: polled a privateTMPDIRwithgrep -rl "$TOKEN"every 100ms for the whole call — no credential file at any point, and the directory was empty afterwardBase contrast: ran base-commit822a990copies ofbin/fm-x-{lib,poll}.shunder the same harness — a-rw-------fm-x-auth.*credential file appeared on disk during the callBaseline of known failures:bash tests/fm-gotmp.test.shandbash tests/fm-public-followup.test.shat the target commit, then again after revertingbin/fm-x-{lib,poll,dismiss}.shandtests/fm-x-mode.test.shto822a990— identical single-test failures both times✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.