Skip to main content

max / alloy

Make the polkit agent's prompt end when its conversation does Five defects on the authentication surface, found reviewing the tier-3 privilege work and confirmed against the code. A cancelled authentication left its modal on screen still collecting a password: cancel_authentication was a no-op and nothing withdrew the prompt, so the user finished typing into a conversation polkit had already closed. converse now re-reads the withdrawal after the answer arrives and before anything is written, and the shell takes the question down. The window between that read and the write is left open on purpose, because closing it means the write and the cancellation sharing a lock on zbus's single executor thread, which is the freeze the async handler exists to prevent. Agent::drop could call Unregister while that dispatcher was blocked handing over a prompt. Reproduced: a CancelAuthentication against a live blocking Listener never returns. Fixed at the root by serving the conversation on its own thread, with the prompts receiver dropped first as belt and braces. A duplicate cookie used to overwrite its entry, leaving the first conversation asking a question no cancellation could reach; it is refused now. Tracked withdraws before forgetting, so a conversation dropped mid-await stops rather than outliving the map entry that could have stopped it. polkit's and PAM's own strings, action_id included, are stripped of control and format characters before they reach the modal. The typed answer is cli::Secret from Enter onward; the TextField buffer behind it cannot be scrubbed from this side of the crate boundary and the comment says so rather than implying otherwise. sbin_path.rs lands here because this is the commit that makes its claim true: cli::child_command is now the only Command::new in the crate.
Co-Authored-By
Claude Opus 5 (1M context) <noreply@anthropic.com>
Author: Max Johnson <me@maxj.phd> · 2026-08-19 16:59 UTC
Signed with PGP, not checked
Commit: d3c7c9529e447282421bdd542433003337e3a2d7
Parent: cd9d67a
3 files changed, +1090 insertions, -99 deletions
M crates/alloy/src/polkit.rs +351 -36
@@ -37,16 +37,24 @@
37 37 //! about to run and the children it spawns.
38 38
39 39 use std::collections::HashMap;
40 + use std::collections::hash_map::Entry;
41 + use std::future::Future;
40 42 use std::io::{BufRead, BufReader, Write};
41 43 use std::path::{Path, PathBuf};
42 - use std::process::{Child, Command, Stdio};
44 + use std::pin::Pin;
45 + use std::process::{Child, Stdio};
46 + use std::sync::atomic::{AtomicBool, Ordering};
43 47 use std::sync::mpsc::{Receiver, SyncSender, sync_channel};
48 + use std::sync::{Arc, Mutex, PoisonError};
49 + use std::task::{Context as TaskContext, Poll, Waker};
44 50
45 51 use anyhow::{Context, Result, anyhow, bail};
46 52 use zbus::blocking::Connection;
47 53 use zbus::interface;
48 54 use zbus::zvariant::{OwnedValue, Value};
49 55
56 + use crate::cli::{Secret, child_command};
57 +
50 58 /// Where polkit keeps the setuid helper that runs the PAM conversation.
51 59 ///
52 60 /// Both paths are tried in order. Fedora (and so Alloy) ships `/usr/lib`;
@@ -74,29 +82,54 @@
74 82 /// this one, so a request that cannot be answered is a request nobody should be
75 83 /// able to construct.
76 84 pub(crate) struct Prompt {
77 - /// The polkit action, e.g. `org.freedesktop.NetworkManager.settings.modify.system`.
85 + /// The polkit action, e.g. `org.freedesktop.NetworkManager.settings.modify.system`,
86 + /// stripped of control and format characters by [`printable`].
87 + ///
88 + /// Stripped for the same reason the other two are, and it was the one that
89 + /// was not: the console names the action in the log pane when the question
90 + /// arrives and again when polkit withdraws it (`shell.rs`), and the pane is
91 + /// a ratatui buffer like the modal. An action id is a dotted name in
92 + /// practice, which is an argument about what polkit sends rather than about
93 + /// what reaches the screen.
78 94 pub action_id: String,
79 - /// polkit's own human-readable line for the action.
95 + /// polkit's own human-readable line for the action, stripped of control
96 + /// characters by [`printable`].
80 97 pub message: String,
81 - /// What the helper asked for, usually `Password: `.
98 + /// What the helper asked for, usually `Password: `, stripped of control
99 + /// characters by [`printable`].
82 100 pub question: String,
83 101 /// Whether what is typed should be drawn. False for a password, true for
84 102 /// the one-time-code shape (`PAM_PROMPT_ECHO_ON`), which does exist and
85 103 /// which a masked field would make unusable.
86 104 pub echo: bool,
87 - reply: SyncSender<Option<String>>,
105 + reply: SyncSender<Option<Secret>>,
106 + /// Set by [`cancel_authentication`](Listener::cancel_authentication) when
107 + /// polkit withdraws this conversation. Shared with every other prompt of
108 + /// the same conversation and with [`converse`], which is what lets a
109 + /// withdrawal reach a question already on the screen.
110 + withdrawn: Arc<AtomicBool>,
88 111 }
89 112
90 113 impl Prompt {
91 114 /// Answer the prompt.
92 115 ///
93 - /// The answer is a `String` and not a [`Secret`](crate::cli::Secret),
94 - /// because the helper reads it as a line of text and there is nowhere else
95 - /// for it to go: it crosses one channel, is written to one pipe, and is
96 - /// dropped. What a `Secret` buys elsewhere is a scrubbed buffer inside a
97 - /// long-lived [`Invocation`](crate::cli::Invocation); nothing here is
98 - /// long-lived.
99 - pub(crate) fn answer(self, response: String) {
116 + /// The answer is a [`Secret`] and not a `String`. It used to be the latter,
117 + /// on the argument that nothing here is long-lived: it crosses one channel,
118 + /// is written to one pipe, and is dropped. That holds for the value and not
119 + /// for the buffer behind it, which is the whole of what `Secret` is for:
120 + /// a `Vec<u8>` that is zeroed on drop, where a `String` is handed back to
121 + /// the allocator with the password still in it. This is an authentication
122 + /// surface, and it should not be the one place in the console that opts out
123 + /// of the type written for exactly this.
124 + ///
125 + /// **The caller's obligation: no newline.** The helper's protocol is one
126 + /// line per message, so a newline inside an answer would be read as the end
127 + /// of it and everything after as the next message, desyncing a conversation
128 + /// that is deciding whether to authorize something. Nothing the console
129 + /// offers can produce one, since Enter submits the field rather than
130 + /// reaching it, which is why this is stated rather than assumed. [`converse`] refuses
131 + /// an answer carrying one rather than writing it.
132 + pub(crate) fn answer(self, response: Secret) {
100 133 // A send that fails means the D-Bus thread gave up first — polkit
101 134 // cancelled, or the command died. Nothing to report: the screen is
102 135 // about to be told the same thing by the outcome channel.
@@ -107,6 +140,49 @@
107 140 pub(crate) fn dismiss(self) {
108 141 let _ = self.reply.send(None);
109 142 }
143 +
144 + /// Whether polkit has withdrawn the question since it was asked.
145 + ///
146 + /// Polled by the screen, which owns this value and is the only thing that
147 + /// can take the modal down. A prompt left up after a withdrawal is not
148 + /// merely stale: it still collects, and the password typed into it goes to
149 + /// a conversation that has already ended.
150 + pub(crate) fn withdrawn(&self) -> bool {
151 + self.withdrawn.load(Ordering::Relaxed)
152 + }
153 +
154 + /// A prompt with no conversation behind it, for exercising the screen's
155 + /// half of this: the modal, the key handling, and the take-down on a
156 + /// withdrawal.
157 + ///
158 + /// Here rather than in [`crate::shell`]'s tests because the fields are
159 + /// private and stay so. What the screen is allowed to do with a prompt is
160 + /// [`answer`](Prompt::answer), [`dismiss`](Prompt::dismiss) and
161 + /// [`withdrawn`](Prompt::withdrawn), and a test that reached past those
162 + /// would have stopped testing what the screen can do.
163 + ///
164 + /// Returns the reply channel, so a test can see which of answer and
165 + /// dismissal the screen chose, and the flag, so it can withdraw the
166 + /// question the way [`Listener::cancel_authentication`] does.
167 + #[cfg(test)]
168 + pub(crate) fn for_test(
169 + action_id: &str,
170 + message: &str,
171 + ) -> (Self, Receiver<Option<Secret>>, Arc<AtomicBool>) {
172 + // Buffered, unlike the live path: nothing is receiving here, and a
173 + // rendezvous channel would make a dismissal block the test.
174 + let (reply, answers) = sync_channel(1);
175 + let withdrawn = Arc::new(AtomicBool::new(false));
176 + let prompt = Self {
177 + action_id: printable(action_id),
178 + message: printable(message),
179 + question: printable("Password:"),
180 + echo: false,
181 + reply,
182 + withdrawn: Arc::clone(&withdrawn),
183 + };
184 + (prompt, answers, withdrawn)
185 + }
110 186 }
111 187
112 188 /// A registered authentication agent.
@@ -117,7 +193,9 @@
117 193 pub(crate) struct Agent {
118 194 connection: Connection,
119 195 subject: Subject,
120 - prompts: Receiver<Prompt>,
196 + /// An `Option` so [`drop`](Agent::drop) can close it before anything else
197 + /// happens. See that impl for why the order is load-bearing.
198 + prompts: Option<Receiver<Prompt>>,
121 199 }
122 200
123 201 impl Agent {
@@ -133,7 +211,14 @@
133 211
134 212 let connection = zbus::blocking::connection::Builder::system()
135 213 .context("no system bus")?
136 - .serve_at(AGENT_PATH, Listener { helper, sender })
214 + .serve_at(
215 + AGENT_PATH,
216 + Listener {
217 + helper,
218 + sender,
219 + live: Mutex::new(HashMap::new()),
220 + },
221 + )
137 222 .context("the agent object path is already in use")?
138 223 .build()
139 224 .context("the agent could not be served")?;
@@ -156,7 +241,7 @@
156 241 Ok(Self {
157 242 connection,
158 243 subject,
159 - prompts,
244 + prompts: Some(prompts),
160 245 })
161 246 }
162 247
@@ -167,12 +252,27 @@
167 252 /// reason every other console refresh is a poll: there is one thread that
168 253 /// owns the screen and it is not this one.
169 254 pub(crate) fn pending(&self) -> Option<Prompt> {
170 - self.prompts.try_recv().ok()
255 + self.prompts.as_ref()?.try_recv().ok()
171 256 }
172 257 }
173 258
174 259 impl Drop for Agent {
175 260 fn drop(&mut self) {
261 + // The receiver goes first, and that ordering is the fix for a freeze
262 + // rather than tidiness. A field is dropped *after* the body of the
263 + // `Drop` it belongs to, so leaving this to the compiler means the
264 + // Unregister below runs with the channel still open. Prompts cross a
265 + // rendezvous channel, so a conversation that asked a question nobody
266 + // collected is parked in `send` holding a `Prompt` this receiver keeps
267 + // alive; closing the channel here turns that park into an error the
268 + // conversation ends on, before the console blocks on a bus call.
269 + //
270 + // It is insurance and not the whole story: the conversation itself runs
271 + // on a thread of its own (see [`Conversation`]), so a parked one no
272 + // longer blocks the bus either. Both are cheap and the failure they
273 + // guard against is a console that has to be killed.
274 + drop(self.prompts.take());
275 +
176 276 // Best effort by necessity: a drop cannot report, and the failure modes
177 277 // are a bus that has gone away and a polkit that has restarted, in both
178 278 // of which the registration is already void.
@@ -254,6 +354,110 @@
254 354 struct Listener {
255 355 helper: PathBuf,
256 356 sender: SyncSender<Prompt>,
357 + /// The conversations currently in flight, by the cookie polkit named them
358 + /// with, each holding the flag its prompts watch.
359 + ///
360 + /// A map rather than a single slot because the interface allows more than
361 + /// one question at a time and the cookie is what tells them apart. Only one
362 + /// is ever on the screen, but a withdrawal names a cookie and must reach
363 + /// that conversation rather than whichever was most recent.
364 + live: Mutex<HashMap<String, Arc<AtomicBool>>>,
365 + }
366 +
367 + impl Listener {
368 + /// Start tracking a conversation, returning the flag its prompts carry and
369 + /// the guard that stops tracking it.
370 + ///
371 + /// **`None` when that cookie is already in flight, and the refusal is the
372 + /// point.** polkit names a conversation once, so a second
373 + /// `BeginAuthentication` under a live cookie is not something this can
374 + /// serve. Inserting over the entry would drop the first conversation's
375 + /// flag on the floor: that conversation keeps its own `Arc` and keeps
376 + /// asking, and a later `CancelAuthentication` naming the cookie would set
377 + /// the *second* flag, so the first is left permanently unwithdrawable —
378 + /// the exact state [`cancel_authentication`](Listener::cancel_authentication)
379 + /// exists to make impossible. Refusing leaves whatever is in flight
380 + /// exactly as it was.
381 + ///
382 + /// Split out from [`begin_authentication`](Listener::begin_authentication)
383 + /// so the registry can be exercised without a bus.
384 + fn track(&self, cookie: &str) -> Option<(Tracked<'_>, Arc<AtomicBool>)> {
385 + let withdrawn = Arc::new(AtomicBool::new(false));
386 + match self.lock().entry(cookie.to_string()) {
387 + Entry::Occupied(_) => None,
388 + Entry::Vacant(slot) => {
389 + slot.insert(Arc::clone(&withdrawn));
390 + Some((
391 + Tracked {
392 + listener: self,
393 + cookie: cookie.to_string(),
394 + },
395 + withdrawn,
396 + ))
397 + }
398 + }
399 + }
400 +
401 + /// Stop tracking one, once it is over either way. [`Tracked`] is what calls
402 + /// this on the live path; it stays a method of its own so the registry can
403 + /// be driven directly in tests.
404 + fn forget(&self, cookie: &str) {
405 + self.lock().remove(cookie);
406 + }
407 +
408 + /// Mark a conversation withdrawn. Unknown cookies are ignored: polkit is
409 + /// allowed to cancel something this agent already finished, and a
410 + /// cancellation for a conversation nobody is having is a no-op rather than
411 + /// an error worth reporting back over the bus.
412 + fn withdraw(&self, cookie: &str) {
413 + if let Some(withdrawn) = self.lock().get(cookie) {
414 + withdrawn.store(true, Ordering::Relaxed);
415 + }
416 + }
417 +
418 + /// A poisoned lock is recovered from rather than propagated. The map holds
419 + /// flags and nothing else, so a panic elsewhere cannot have left it
420 + /// inconsistent, and refusing to answer polkit over it would turn a bug in
421 + /// one conversation into an agent that has stopped working.
422 + fn lock(&self) -> std::sync::MutexGuard<'_, HashMap<String, Arc<AtomicBool>>> {
423 + self.live.lock().unwrap_or_else(PoisonError::into_inner)
424 + }
425 + }
426 +
427 + /// Tracking for one conversation, released when the conversation is over.
428 + ///
429 + /// A guard rather than a call at the end of
430 + /// [`begin_authentication`](Listener::begin_authentication). What stood there
431 + /// was that call plus a claim that the only way past it was the task being
432 + /// dropped, which happens when the connection is going away and takes the whole
433 + /// map with it. Nobody measured that, and it is not this code's to know: zbus
434 + /// decides when a handler's future is dropped, and a task cancelled for any
435 + /// reason this code cannot see would leave a map entry behind forever, holding
436 + /// its flag, making its cookie unusable by [`Listener::track`] above, and
437 + /// keeping nothing else alive that would say so. A guard is correct whether or
438 + /// not the claim was, so the claim no longer has to be made.
439 + struct Tracked<'a> {
440 + listener: &'a Listener,
441 + cookie: String,
442 + }
443 +
444 + impl Drop for Tracked<'_> {
445 + /// Withdraw before forgetting, and the order is the whole point.
446 + ///
447 + /// Forgetting alone releases the cookie and leaves the conversation running:
448 + /// on the drop-mid-await path the thread keeps its helper child and its
449 + /// prompt sender, and a later `CancelAuthentication` for that cookie finds
450 + /// no entry and returns Ok having withdrawn nothing. That is the orphan
451 + /// [`Listener::track`]'s duplicate refusal exists to keep out of the map,
452 + /// reintroduced by the guard that was meant to tidy up after it.
453 + ///
454 + /// Setting the flag first means the conversation reads its own withdrawal at
455 + /// the next check and stops on the path it already has, rather than needing
456 + /// a second mechanism to kill it from here.
457 + fn drop(&mut self) {
458 + self.listener.withdraw(&self.cookie);
459 + self.listener.forget(&self.cookie);
460 + }
257 461 }
258 462
259 463 // Every argument below is owned and several are unread, both of which clippy
@@ -276,8 +480,19 @@
276 480 /// claims to implement. Spelled without a leading underscore because the
277 481 /// macro reads these names back out, which makes an underscore a lie about
278 482 /// whether anything uses them.
483 + ///
484 + /// `async` with the conversation itself on a thread of its own, which is
485 + /// not decoration. zbus serves this connection from a single executor
486 + /// thread, so a handler that blocks it blocks everything else the
487 + /// connection does: no further method is dispatched, and no reply to a call
488 + /// this process made is read. Held that way, a conversation waiting for a
489 + /// password made the agent deaf to
490 + /// [`cancel_authentication`](Listener::cancel_authentication) and made
491 + /// [`Agent::drop`]'s Unregister a call whose reply could never arrive,
492 + /// which is a frozen console. Awaiting instead yields the thread for the
493 + /// minutes a person spends typing.
279 494 #[allow(clippy::too_many_arguments)]
280 - fn begin_authentication(
495 + async fn begin_authentication(
281 496 &self,
282 497 action_id: String,
283 498 message: String,
@@ -291,33 +506,126 @@
291 506 let user = choose_identity(&identities)
292 507 .ok_or_else(|| zbus::fdo::Error::Failed("no identity this agent can ask".into()))?;
293 508
294 - converse(
295 - &self.helper,
296 - &user,
297 - &cookie,
298 - &action_id,
299 - &message,
300 - |prompt| {
301 - self.sender
302 - .send(prompt)
303 - .map_err(|_| anyhow!("the console stopped listening"))
304 - },
305 - )
306 - .map_err(|error| zbus::fdo::Error::Failed(error.to_string()))
509 + // Refused rather than served if polkit is already having a conversation
510 + // under this cookie. See [`Listener::track`]: the alternative is the
511 + // conversation already in flight losing the flag its withdrawal would
512 + // arrive on.
513 + let Some((tracked, withdrawn)) = self.track(&cookie) else {
514 + return Err(zbus::fdo::Error::Failed(
515 + "a conversation with that cookie is already in flight".into(),
516 + ));
517 + };
518 + let helper = self.helper.clone();
519 + let sender = self.sender.clone();
520 + let asked_about = cookie.clone();
521 + let outcome = Conversation::spawn(move || {
522 + converse(
523 + &helper,
524 + &user,
525 + &asked_about,
526 + &action_id,
527 + &message,
528 + &withdrawn,
529 + |prompt| {
530 + sender
531 + .send(prompt)
532 + .map_err(|_| anyhow!("the console stopped listening"))
533 + },
534 + )
535 + })
536 + .await;
537 +
538 + // Untracked by dropping the guard, which is also what happens if this
539 + // task is dropped mid-await instead of reaching here. See [`Tracked`].
540 + drop(tracked);
541 + outcome.map_err(|error| zbus::fdo::Error::Failed(error.to_string()))
307 542 }
308 543
309 544 /// polkit withdrew the question — the command it was for went away.
310 545 ///
311 - /// Nothing to do. The helper is a child of the call that is still blocked
312 - /// in [`begin_authentication`](Listener::begin_authentication), and polkit
313 - /// closes that out itself; killing it from here would race the reply.
314 - #[allow(clippy::unused_self)]
546 + /// The prompt is on the screen and the screen is another thread's, so this
547 + /// sets the flag that thread polls rather than reaching into it. Taking the
548 + /// modal down is what makes this more than bookkeeping: without it the
549 + /// console keeps drawing a question polkit has already abandoned, and keeps
550 + /// collecting into it, so the password is typed into a conversation that
551 + /// ended.
552 + ///
553 + /// It races an answer, and the race is already settled. Answer and
554 + /// dismissal both cross the prompt's own reply channel and whichever
555 + /// arrives first is the one the conversation sees, which is how Esc has
556 + /// always worked; the loser sends into a channel nobody is receiving on and
557 + /// is dropped. An answer that wins that race is refused anyway, because
558 + /// [`converse`] reads this flag again between the answer arriving and it
559 + /// being written to the helper. What used to be here instead was a no-op, on the reasoning
560 + /// that killing the helper from this thread would race the reply. That was
561 + /// true of killing the helper and never true of withdrawing the question.
315 562 fn cancel_authentication(&self, cookie: String) -> zbus::fdo::Result<()> {
316 - drop(cookie);
563 + self.withdraw(&cookie);
317 564 Ok(())
318 565 }
319 566 }
320 567
568 + /// A helper conversation running on a thread of its own, awaited by the D-Bus
569 + /// method that started it.
570 + ///
571 + /// Hand-rolled rather than pulled from a runtime, because the console has no
572 + /// runtime: zbus is here for its D-Bus client and the whole point of the
573 + /// `blocking` feature is that async does not spread into a TUI with one thread
574 + /// and one frame at a time. This is the one place that needs a future, it needs
575 + /// exactly one thing from it, which is to resolve when a thread finishes, and
576 + /// that is thirty lines rather than a dependency.
577 + ///
578 + struct Conversation {
579 + state: Arc<Mutex<ConversationState>>,
580 + }
581 +
582 + #[derive(Default)]
583 + struct ConversationState {
584 + finished: Option<Result<()>>,
585 + waker: Option<Waker>,
586 + }
587 +
588 + impl Conversation {
589 + fn spawn(work: impl FnOnce() -> Result<()> + Send + 'static) -> Self {
590 + let state = Arc::new(Mutex::new(ConversationState::default()));
591 + let handle = Arc::clone(&state);
592 + std::thread::spawn(move || {
593 + let finished = work();
594 + // Stored and the waker taken under one lock, so a poll landing in
595 + // the middle either sees the result or leaves a waker that is about
596 + // to be woken. Either order is fine; both at once is what a second
597 + // lock would allow, and a lost wakeup here is an agent that never
598 + // answers polkit again.
599 + let waker = {
600 + let mut state = handle.lock().unwrap_or_else(PoisonError::into_inner);
601 + state.finished = Some(finished);
602 + state.waker.take()
603 + };
604 + if let Some(waker) = waker {
605 + waker.wake();
606 + }
607 + });
608 + Self { state }
609 + }
610 + }
611 +
612 + impl Future for Conversation {
613 + type Output = Result<()>;
614 +
615 + fn poll(self: Pin<&mut Self>, cx: &mut TaskContext<'_>) -> Poll<Self::Output> {
616 + let mut state = self.state.lock().unwrap_or_else(PoisonError::into_inner);
617 + match state.finished.take() {
618 + Some(finished) => Poll::Ready(finished),
619 + None => {
620 + // Replaced rather than kept, since a future can be polled by a
621 + // different waker than the one that saw it last.
622 + state.waker = Some(cx.waker().clone());
623 + Poll::Pending
624 + }
625 + }
626 + }
627 + }
628 +
321 629 /// Which of the identities polkit will accept this agent can actually ask.
322 630 ///
323 631 /// **This machine's own user first, and that ordering is the security-relevant
@@ -397,15 +705,21 @@
397 705 /// been handed over, and the answer comes back through the prompt's own reply
398 706 /// channel. Taking it as a closure is what keeps this function testable against
399 707 /// a scripted helper with no D-Bus and no terminal anywhere near it.
708 + ///
709 + /// `withdrawn` is polkit's cancellation, shared with every prompt this hands
710 + /// out. Checked between messages so a conversation whose command has gone away
711 + /// stops rather than asking the next question in a sequence nobody is waiting
712 + /// on the end of.
400 713 fn converse(
401 714 helper: &Path,
402 715 user: &str,
403 716 cookie: &str,
404 717 action_id: &str,
405 718 message: &str,
719 + withdrawn: &Arc<AtomicBool>,
406 720 ask: impl Fn(Prompt) -> Result<()>,
407 721 ) -> Result<()> {
408 - let mut child = Command::new(helper)
722 + let mut child = child_command(helper)
409 723 .arg(user)
410 724 .stdin(Stdio::piped())
411 725 .stdout(Stdio::piped())
@@ -422,15 +736,28 @@
422 736 writeln!(stdin, "{cookie}").context("the helper closed before the cookie")?;
423 737
424 738 while let Some(line) = lines.next().transpose().context("the helper stopped")? {
739 + if withdrawn.load(Ordering::Relaxed) {
Lines truncated
M crates/alloy/src/shell.rs +315 -63
@@ -19,7 +19,7 @@
19 19 use ratatui::crossterm::event::{self, Event, KeyCode, KeyEvent, KeyEventKind};
20 20 use ratatui::layout::Rect;
21 21
22 - use crate::cli::CommandLog;
22 + use crate::cli::{CommandLog, child_command};
23 23
24 24 /// What a view wants the shell to do after handling a key.
25 25 ///
@@ -539,7 +539,7 @@
539 539 ) -> Result<()> {
540 540 ratatui::restore();
541 541
542 - let agent = Command::new(AGENT)
542 + let agent = child_command(AGENT)
543 543 .args(["--process", &std::process::id().to_string()])
544 544 .spawn();
545 545
@@ -638,69 +638,39 @@
638 638 // not; the invocation is recorded below, once its outcome is known.
639 639 let worker = std::thread::spawn(move || invocation.capture_quiet());
640 640
641 + // The typed answer lives in a `TextField` and not in a
642 + // [`Secret`](crate::cli::Secret), and that is a limit worth naming rather
643 + // than papering over. `Secret` covers a value being carried around: it is
644 + // bytes, and it is zeroed on drop. A field is an editing buffer: a
645 + // `String` inside `alloy_tui`, reallocated as it grows, and drawn into the
646 + // frame every tick, so nothing this side of the crate boundary can promise
647 + // the password exists in one scrubbable place while it is being typed. The
648 + // installer's password fields sit in exactly the same position (`install.rs`,
649 + // `Answers`), and the answer there is the same: the plaintext lives in the
650 + // field until it becomes a value, and from that moment it is a `Secret`.
651 + // That moment is `Enter`, in [`authorization_turn`].
652 + //
653 + // **Where it costs most is the echoed prompt**, which is worth saying here
654 + // rather than leaving to be discovered. `PAM_PROMPT_ECHO_ON` is a one-time
655 + // code, and a value that is being displayed is a value in the frame buffer
656 + // and in the modal's own message: there is no version of drawing it that
657 + // does not copy it, and no scrub that could catch every copy while it is on
658 + // the screen. The masked branch draws bullets and copies nothing. So the
659 + // echoed path keeps a per-tick plaintext copy by construction; what closing
660 + // that would take is not a `Secret` here but a field widget that renders
661 + // from bytes it owns, which is `alloy_tui`'s to have.
641 662 let mut prompt: Option<(crate::polkit::Prompt, alloy_tui::TextField)> = None;
642 663
643 664 let outcome = loop {
644 - // Rebuilt every frame from the prompt and what has been typed, which is
645 - // what lets a `Confirm` — a value with no input in it — carry a text
646 - // field. The modal's own footer already reads `Enter confirm Esc
647 - // cancel`, which is exactly what the two keys do here.
648 - let modal = prompt.as_ref().map(|(prompt, field)| Confirm {
649 - title: prompt.message.clone(),
650 - message: format!(
651 - "{} {}",
652 - prompt.question,
653 - if prompt.echo {
654 - field.value().to_string()
655 - } else {
656 - "•".repeat(field.value().chars().count())
657 - }
658 - ),
659 - severity: Severity::Warn,
660 - });
661 - terminal.draw(|frame| draw(frame, theme, view, log, modal.as_ref(), false))?;
662 -
663 - if prompt.is_none()
664 - && let Some(agent) = &agent
665 - && let Some(pending) = agent.pending()
666 - {
667 - // Recorded as commentary, the way the mock backends mark a line
668 - // that is not a command. Naming the action is the one thing every
669 - // good version of this prompt does — it is what Omarchy's Quattro
670 - // PR added to theirs (wiki `alloy-privilege`) — and the pane is
671 - // where the console says what is happening.
672 - log.record(
673 - format!("# polkit asks about {}", pending.action_id),
674 - Severity::Warn,
675 - );
676 - prompt = Some((pending, alloy_tui::TextField::new()));
677 - }
678 -
679 - if event::poll(AUTHORIZATION_POLL)?
680 - && let Event::Key(key) = event::read()?
681 - && key.kind == KeyEventKind::Press
682 - && let Some((asked, field)) = prompt.take()
683 - {
684 - match key.code {
685 - KeyCode::Enter => asked.answer(field.value().to_string()),
686 - KeyCode::Esc => asked.dismiss(),
687 - code => {
688 - // Not taken after all: put it back with the key applied.
689 - let mut field = field;
690 - match code {
691 - KeyCode::Char(c) => field.insert(c),
692 - KeyCode::Backspace => field.backspace(),
693 - KeyCode::Delete => field.delete(),
694 - KeyCode::Left => field.left(),
695 - KeyCode::Right => field.right(),
696 - KeyCode::Home => field.home(),
697 - KeyCode::End => field.end(),
698 - _ => {}
699 - }
700 - prompt = Some((asked, field));
701 - }
702 - }
703 - }
665 + authorization_turn(
666 + terminal,
667 + theme,
668 + &*view,
669 + log,
670 + &mut prompt,
671 + || agent.as_ref().and_then(crate::polkit::Agent::pending),
672 + || read_key(AUTHORIZATION_POLL),
673 + )?;
704 674
705 675 if worker.is_finished() {
706 676 break worker
@@ -726,6 +696,129 @@
726 696 Ok(())
727 697 }
728 698
699 + /// One turn of [`authorize_inline`]'s loop: take down a question polkit has
700 + /// withdrawn, draw, collect a new question, and apply a key to the one on the
701 + /// screen.
702 + ///
703 + /// Split out of the loop so it can be tested, and the two closures are what a
704 + /// test cannot otherwise supply. `pending` is a registered polkit agent, which
705 + /// needs a system bus and something to ask; `key` is a real terminal, which
706 + /// crossterm reads whether or not one is attached. Everything between them is
707 + /// the same code the console runs, and the backend is generic for the same
708 + /// reason: [`ratatui::backend::TestBackend`] is a frame with no terminal under
709 + /// it.
710 + fn authorization_turn<B>(
711 + terminal: &mut ratatui::Terminal<B>,
712 + theme: &Theme,
713 + view: &dyn View,
714 + log: &mut CommandLog,
715 + prompt: &mut Option<(crate::polkit::Prompt, alloy_tui::TextField)>,
716 + pending: impl FnOnce() -> Option<crate::polkit::Prompt>,
717 + key: impl FnOnce() -> Result<Option<KeyEvent>>,
718 + ) -> Result<()>
719 + where
720 + // The error bound is `?`'s and not this function's: a backend names its own
721 + // failure type, and `anyhow` carries one only if it is an error that can
722 + // cross threads. Both backends in play satisfy it.
723 + B: ratatui::backend::Backend,
724 + B::Error: std::error::Error + Send + Sync + 'static,
725 + {
726 + // polkit can withdraw a question it already asked, when the command it was
727 + // for went away. Taken down here rather than left up: a modal still on the
728 + // screen still collects, and the password typed into it would go to a
729 + // conversation that has ended. First in the turn, so a keypress arriving in
730 + // the same turn as the withdrawal finds no question to answer.
731 + if let Some((withdrawn, _)) = prompt.take_if(|(asked, _)| asked.withdrawn()) {
732 + log.record(
733 + format!(
734 + "# polkit withdrew the question about {}",
735 + withdrawn.action_id
736 + ),
737 + Severity::Warn,
738 + );
739 + withdrawn.dismiss();
740 + }
741 +
742 + // Rebuilt every frame from the prompt and what has been typed, which is
743 + // what lets a `Confirm` — a value with no input in it — carry a text
744 + // field. The modal's own footer already reads `Enter confirm Esc
745 + // cancel`, which is exactly what the two keys do here.
746 + let modal = prompt.as_ref().map(|(prompt, field)| Confirm {
747 + title: prompt.message.clone(),
748 + // Formatted per branch rather than around one `if`, so the echoed value
749 + // is copied into the message and not into a `String` on the way there.
750 + // One copy is the floor for a value being drawn; see the note in
751 + // [`authorize_inline`] about what the other end of this would take.
752 + message: if prompt.echo {
753 + format!("{} {}", prompt.question, field.value())
754 + } else {
755 + format!(
756 + "{} {}",
757 + prompt.question,
758 + "•".repeat(field.value().chars().count())
759 + )
760 + },
761 + severity: Severity::Warn,
762 + });
763 + terminal.draw(|frame| draw(frame, theme, view, log, modal.as_ref(), false))?;
764 +
765 + if prompt.is_none()
766 + && let Some(pending) = pending()
767 + {
768 + // Recorded as commentary, the way the mock backends mark a line
769 + // that is not a command. Naming the action is the one thing every
770 + // good version of this prompt does — it is what Omarchy's Quattro
771 + // PR added to theirs (wiki `alloy-privilege`) — and the pane is
772 + // where the console says what is happening.
773 + log.record(
774 + format!("# polkit asks about {}", pending.action_id),
775 + Severity::Warn,
776 + );
777 + *prompt = Some((pending, alloy_tui::TextField::new()));
778 + }
779 +
780 + if let Some(key) = key()?
781 + && let Some((asked, field)) = prompt.take()
782 + {
783 + match key.code {
784 + KeyCode::Enter => asked.answer(crate::cli::Secret::new(field.value())),
785 + KeyCode::Esc => asked.dismiss(),
786 + code => {
787 + // Not taken after all: put it back with the key applied.
788 + let mut field = field;
789 + match code {
790 + KeyCode::Char(c) => field.insert(c),
791 + KeyCode::Backspace => field.backspace(),
792 + KeyCode::Delete => field.delete(),
793 + KeyCode::Left => field.left(),
794 + KeyCode::Right => field.right(),
795 + KeyCode::Home => field.home(),
796 + KeyCode::End => field.end(),
797 + _ => {}
798 + }
799 + *prompt = Some((asked, field));
800 + }
801 + }
802 + }
803 +
804 + Ok(())
805 + }
806 +
807 + /// The next key press, or nothing within `timeout`.
808 + ///
809 + /// Split out so [`authorization_turn`] takes its input as a closure. Nothing
810 + /// else changed: a non-key event and a key release are both "no key", which is
811 + /// what the loop did when this was inline.
812 + fn read_key(timeout: Duration) -> Result<Option<KeyEvent>> {
813 + if event::poll(timeout)?
814 + && let Event::Key(key) = event::read()?
815 + && key.kind == KeyEventKind::Press
816 + {
817 + return Ok(Some(key));
818 + }
819 + Ok(None)
820 + }
821 +
729 822 fn draw(
730 823 frame: &mut Frame,
731 824 theme: &Theme,
@@ -804,9 +897,15 @@
804 897
805 898 #[cfg(test)]
806 899 mod tests {
807 - use super::*;
900 + use std::sync::atomic::Ordering;
901 +
902 + use ratatui::Terminal;
903 + use ratatui::backend::TestBackend;
808 904 use ratatui::layout::Rect;
809 905
906 + use super::*;
907 + use crate::polkit::Prompt;
908 +
810 909 /// A view that records what the shell called on it. Stands in for the real
811 910 /// views, none of which have destructive actions yet.
812 911 #[derive(Default)]
@@ -835,6 +934,159 @@
835 934 }
836 935 }
837 936
937 + // ---- the authorization loop ----
938 +
939 + /// A real theme rather than a fixture, since the turn draws a frame and a
940 + /// theme that failed to resolve would be a test failing for the wrong
941 + /// reason.
942 + fn theme() -> Theme {
943 + let dir = makeover::bundled_themes_dir().expect("makeover bundles its themes");
944 + let colors = makeover::load_theme(&[(dir, false)], "akari-dawn").expect("akari-dawn ships");
945 + Theme::from_theme(&colors).expect("akari-dawn resolves")
946 + }
947 +
948 + fn offscreen() -> Terminal<TestBackend> {
949 + Terminal::new(TestBackend::new(80, 24)).expect("a terminal with no terminal under it")
950 + }
951 +
952 + /// One turn with no agent behind it and no key pressed, which is the
953 + /// console idling with whatever is in `prompt` on the screen.
954 + fn quiet_turn(
955 + terminal: &mut Terminal<TestBackend>,
956 + log: &mut CommandLog,
957 + prompt: &mut Option<(Prompt, alloy_tui::TextField)>,
958 + ) {
959 + turn(terminal, log, prompt, None, None);
960 + }
961 +
962 + fn turn(
963 + terminal: &mut Terminal<TestBackend>,
964 + log: &mut CommandLog,
965 + prompt: &mut Option<(Prompt, alloy_tui::TextField)>,
966 + pending: Option<Prompt>,
967 + key: Option<KeyCode>,
968 + ) {
969 + authorization_turn(
970 + terminal,
971 + &theme(),
972 + &StubView::default(),
973 + log,
974 + prompt,
975 + || pending,
976 + || Ok(key.map(KeyEvent::from)),
977 + )
978 + .expect("the turn draws and returns");
979 + }
980 +
981 + fn recorded(log: &mut CommandLog) -> Vec<String> {
982 + log.entries()
983 + .iter()
984 + .map(|entry| entry.command.clone())
985 + .collect()
986 + }
987 +
988 + // The headline of task 3f2fb5c8, tested where it actually happens rather
989 + // than by calling `dismiss` by hand. polkit withdraws a question the
990 + // console is still showing; the modal has to come down, and an Enter
991 + // arriving in the same turn must not answer it. Without the take-down the
992 + // Enter below sends the typed value into a conversation that has ended,
993 + // which is the failure in full.
994 + #[test]
995 + fn a_withdrawn_question_comes_off_the_screen_before_a_key_can_answer_it() {
996 + let mut terminal = offscreen();
997 + let mut log = CommandLog::new();
998 + let (prompt, answers, withdrawn) = Prompt::for_test("an.action", "polkit's sentence");
999 + let mut slot = Some((prompt, alloy_tui::TextField::new()));
1000 +
1001 + // Nothing withdrawn: the question stays up and nothing is sent.
1002 + quiet_turn(&mut terminal, &mut log, &mut slot);
1003 + assert!(slot.is_some(), "an ordinary turn leaves the question up");
1004 + assert!(answers.try_recv().is_err(), "and answers nothing");
1005 +
1006 + withdrawn.store(true, Ordering::Relaxed);
1007 + turn(
1008 + &mut terminal,
1009 + &mut log,
1010 + &mut slot,
1011 + None,
1012 + Some(KeyCode::Enter),
1013 + );
1014 +
1015 + assert!(slot.is_none(), "the modal is taken down");
1016 + assert!(
1017 + matches!(answers.try_recv(), Ok(None)),
1018 + "a withdrawn question is refused, and the Enter answers nothing",
1019 + );
1020 + assert!(
1021 + recorded(&mut log)
1022 + .iter()
1023 + .any(|line| line == "# polkit withdrew the question about an.action"),
1024 + "the pane says why the modal vanished: {:?}",
1025 + recorded(&mut log),
1026 + );
1027 + }
1028 +
1029 + // The other half of the same turn: a question that has not been withdrawn
1030 + // is answered by Enter, and the answer is what was typed.
1031 + #[test]
1032 + fn enter_answers_the_question_with_what_was_typed() {
1033 + let mut terminal = offscreen();
1034 + let mut log = CommandLog::new();
1035 + let (prompt, answers, _withdrawn) = Prompt::for_test("an.action", "polkit's sentence");
1036 + let mut slot = Some((prompt, alloy_tui::TextField::new()));
1037 +
1038 + for code in [KeyCode::Char('h'), KeyCode::Char('i')] {
1039 + turn(&mut terminal, &mut log, &mut slot, None, Some(code));
1040 + }
1041 + assert!(slot.is_some(), "typing does not answer");
1042 +
1043 + turn(
1044 + &mut terminal,
1045 + &mut log,
1046 + &mut slot,
1047 + None,
1048 + Some(KeyCode::Enter),
1049 + );
1050 + let answer = answers.try_recv().expect("the question is answered");
1051 + assert_eq!(
1052 + answer.as_ref().map(|secret| secret.expose().to_vec()),
1053 + Some(b"hi".to_vec()),
1054 + );
1055 + assert!(slot.is_none(), "and the modal comes down with it");
1056 + }
1057 +
1058 + // A question arriving from the agent is named in the pane and put on the
1059 + // screen, and the next one waits: only one modal at a time.
1060 + #[test]
1061 + fn a_question_from_the_agent_reaches_the_screen_once() {
1062 + let mut terminal = offscreen();
1063 + let mut log = CommandLog::new();
1064 + let (first, _first_answers, _first_flag) =
1065 + Prompt::for_test("an.action", "polkit's sentence");
1066 + let (second, second_answers, _second_flag) =
1067 + Prompt::for_test("another.action", "polkit's other sentence");
1068 + let mut slot = None;
1069 +
1070 + turn(&mut terminal, &mut log, &mut slot, Some(first), None);
1071 + assert!(slot.is_some());
1072 + assert!(
1073 + recorded(&mut log)
1074 + .iter()
1075 + .any(|line| line == "# polkit asks about an.action"),
1076 + );
1077 +
1078 + turn(&mut terminal, &mut log, &mut slot, Some(second), None);
1079 + assert!(
1080 + second_answers.try_recv().is_err(),
1081 + "the second question is neither drawn nor answered while the first is up",
1082 + );
1083 + assert!(
1084 + !recorded(&mut log)
1085 + .iter()
1086 + .any(|line| line == "# polkit asks about another.action"),
1087 + );
1088 + }
1089 +
838 1090 // `q` reaches the view for the same reason Esc does. A view with unsaved
839 1091 // edits answers it with a question; one without keeps the old meaning,
840 1092 // which is what the default is.
A crates/alloy/tests/sbin_path.rs +424
@@ -1,0 +1,424 @@
1 + //! One mechanism finds the tools in `/usr/sbin`, and it is the only one.
2 + //!
3 + //! **Nothing was failing when this rule was written, and the rule is kept
4 + //! anyway.** GoingsOn problem `d0dc9dad` measured a booted Alloy session on
5 + //! fw12 with a `PATH` of exactly
6 + //! `/var/home/max/.local/bin:/var/home/max/.cargo/bin:/usr/local/bin:/usr/bin:/usr/local/sbin`,
7 + //! carrying neither `/sbin` nor `/usr/sbin`, and then inferred that the console's
8 + //! sbin tools fail to start from such a session. The inference was never run and
9 + //! it is false on the image Alloy builds: Fedora 42 unified `/usr/sbin` into
10 + //! `/usr/bin` and `fedora-bootc:43` inherits it, so all eight of `cryptsetup`,
11 + //! `wipefs`, `useradd`, `chpasswd`, `chroot`, `setfiles`, `udevadm` and `rfkill`
12 + //! resolve under that `PATH`, each to `/usr/bin`. Re-measured 2026-08-19 inside
13 + //! `localhost/alloy:clip-client`; `cli::SBIN_DIRS` carries the same note.
14 + //!
15 + //! What is left is worth keeping for two reasons that do not depend on the
16 + //! inference. A host that never merged, a later base image, or a tool that moves
17 + //! are all cases the merge does not cover, and putting the directories on the
18 + //! child's `PATH` costs one environment entry. And the chokepoint is a shape
19 + //! rather than a fix: **`cli.rs` is the crate's only `Command::new`**, so the
20 + //! environment every child is given is decided in one readable place, and the
21 + //! next change to that environment has somewhere to go. A `Command::new`
22 + //! anywhere else is a spawn that silently skips it.
23 + //!
24 + //! A text check over the sources, in the same spirit as `profile_split.rs` and
25 + //! `polkit_rules.rs`: cheap, runs on every `cargo test`, and catches the edit
26 + //! that a booted image would only catch on the screen nobody opened that day.
27 + //!
28 + //! Two things a text check cannot do, said here rather than left to be found.
29 + //! It reads the sources of this crate only, so a spawn in a dependency is
30 + //! outside it. And it matches text: the negative tests below run each matcher
31 + //! over sources that are wrong on purpose, because a matcher that has stopped
32 + //! matching passes every check in this file in exactly the way a clean tree
33 + //! does.
34 +
35 + use std::path::{Path, PathBuf};
36 +
37 + fn src_dir() -> PathBuf {
38 + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("src")
39 + }
40 +
41 + /// Every `.rs` file under `src/`, at any depth, by path and contents.
42 + ///
43 + /// Recursive rather than one `read_dir`. A flat listing is right for the tree as
44 + /// it stands, and it exempts the first module directory or `src/bin` file
45 + /// anybody adds, without failing: the check would keep passing while the thing
46 + /// it guards stopped being true. The name is the path relative to `src/`, so a
47 + /// failure names `install/disk.rs` rather than a second `disk.rs`.
48 + fn modules() -> Vec<(String, String)> {
49 + let dir = src_dir();
50 + let mut out = Vec::new();
51 + collect(&dir, &dir, &mut out);
52 + out.sort();
53 + assert!(!out.is_empty(), "no modules found in {}", dir.display());
54 + out
55 + }
56 +
57 + fn collect(root: &Path, dir: &Path, out: &mut Vec<(String, String)>) {
58 + let entries =
59 + std::fs::read_dir(dir).unwrap_or_else(|err| panic!("cannot read {}: {err}", dir.display()));
60 + for entry in entries {
61 + let path = entry.expect("a readable directory entry").path();
62 + if path.is_dir() {
63 + collect(root, &path, out);
64 + continue;
65 + }
66 + if path.extension().is_none_or(|ext| ext != "rs") {
67 + continue;
68 + }
69 + let name = path
70 + .strip_prefix(root)
71 + .expect("walked from root")
72 + .to_string_lossy()
73 + .to_string();
74 + let text = std::fs::read_to_string(&path)
75 + .unwrap_or_else(|err| panic!("cannot read {}: {err}", path.display()));
76 + out.push((name, text));
77 + }
78 + }
79 +
80 + /// The part of a module that ships, with its `#[cfg(test)] mod tests` cut off.
81 + ///
82 + /// Test code spawns freely and should: `pkcheck --version` probes whether the
83 + /// machine running the tests has polkit at all, and the `workspace` export test
84 + /// runs the wrapper scripts it just wrote. Neither ships, and neither is what a
85 + /// user's session `PATH` decides.
86 + ///
87 + /// The cut is the first `#[cfg(test)]` in the first column, which every module
88 + /// here places on the file's last item. That shape is asserted rather than
89 + /// assumed: a module that grew code below its tests would otherwise have that
90 + /// code silently exempted, which is the one way this check could go quiet
91 + /// without failing.
92 + fn shipped(name: &str, text: &str) -> String {
93 + let marker = "\n#[cfg(test)]\n";
94 + let Some(at) = text.find(marker) else {
95 + return text.to_string();
96 + };
97 +
98 + let tail = &text[at + 1..];
99 + assert_eq!(
100 + tail.matches(marker).count(),
101 + 0,
102 + "{name} has more than one `#[cfg(test)]` in the first column; \
103 + the tail is no longer a single test module"
104 + );
105 + assert!(
106 + tail.starts_with("#[cfg(test)]\nmod tests {\n"),
107 + "{name}'s `#[cfg(test)]` does not open a `mod tests`"
108 + );
109 + assert!(
110 + tail.trim_end().ends_with('}'),
111 + "{name} does not end with its test module"
112 + );
113 +
114 + text[..at].to_string()
115 + }
116 +
117 + /// Lines that are wholly a comment, which is what a text matcher must not read.
118 + ///
119 + /// Only the whole-line form. Truncating every line at its first `//` was the
120 + /// other option and it is wrong in the direction that matters: a `//` inside a
121 + /// string literal would cut the rest of the line away, and the rest of the line
122 + /// is where the thing being looked for would be.
123 + fn is_comment(line: &str) -> bool {
124 + line.trim_start().starts_with("//")
125 + }
126 +
127 + /// The `Command::new` calls in `text` that are `std::process::Command`.
128 + ///
129 + /// `clap::Command::new` is the parser's builder and names a subcommand rather
130 + /// than a program; `profile.rs` builds a whole command tree out of it.
131 + ///
132 + /// Keyed on the literal, which an aliased import would walk straight past:
133 + /// `use std::process::Command as Proc;` and then `Proc::new`. That hole is
134 + /// closed by [`process_imports`] rather than here, because a matcher for every
135 + /// spelling of a name is a matcher nobody can read.
136 + fn spawns(text: &str) -> Vec<&str> {
137 + text.lines()
138 + .filter(|line| line.contains("Command::new("))
139 + .filter(|line| !line.contains("clap::Command::new("))
140 + .filter(|line| !is_comment(line))
141 + .collect()
142 + }
143 +
144 + /// The lines that rename something out of `std::process` as they import it.
145 + ///
146 + /// The alias is the hole in [`spawns`], and it is the whole hole: `Command` has
147 + /// to arrive under some name, and every other way of writing that name still
148 + /// contains the literal. `use std::process::Command as Proc;` does not, and
149 + /// `Proc::new` walks past a matcher keyed on `Command::new(`. Forbidding the
150 + /// rename is narrower than forbidding the import, which cannot be forbidden:
151 + /// `shell.rs` names `Command` as a type in `Flow::Suspend(Command)` without
152 + /// spawning anything.
153 + fn process_aliases(text: &str) -> Vec<&str> {
154 + text.lines()
155 + .filter(|line| line.trim_start().starts_with("use std::process"))
156 + .filter(|line| !is_comment(line))
157 + .filter(|line| line.contains(" as "))
158 + .collect()
159 + }
160 +
161 + /// The chokepoint is a chokepoint.
162 + ///
163 + /// One `Command::new` in the crate, in `cli.rs`, inside `child_command`. Any
164 + /// other is a child spawned without the sbin directories on its `PATH`.
165 + #[test]
166 + fn cli_is_the_only_place_that_spawns() {
167 + for (name, text) in modules() {
168 + let text = shipped(&name, &text);
169 + let found = spawns(&text);
170 + if name == "cli.rs" {
171 + assert_eq!(
172 + found.len(),
173 + 1,
174 + "cli.rs should hold exactly one `Command::new`, found: {found:?}"
175 + );
176 + assert!(
177 + found[0].contains("Command::new(program)"),
178 + "cli.rs's `Command::new` is not `child_command`'s: {}",
179 + found[0]
180 + );
181 + continue;
182 + }
183 + assert!(
184 + found.is_empty(),
185 + "{name} calls `Command::new` directly, which skips the sbin `PATH` \
186 + `cli::child_command` sets; use `child_command` instead:\n {}",
187 + found.join("\n ")
188 + );
189 + }
190 + }
191 +
192 + /// And the name it is keyed on is the name in the source.
193 + ///
194 + /// `Command::new` is a literal, so a renamed import would pass the check above
195 + /// while spawning exactly what it forbids. Nothing in the crate renames anything
196 + /// out of `std::process`, which is a rule a reader can check by eye and the one
197 + /// thing that makes the literal enough.
198 + #[test]
199 + fn nothing_renames_a_process_type_on_the_way_in() {
200 + for (name, text) in modules() {
201 + let text = shipped(&name, &text);
202 + let aliased = process_aliases(&text);
203 + assert!(
204 + aliased.is_empty(),
205 + "{name} renames something out of `std::process`; a renamed `Command` \
206 + would walk past `cli_is_the_only_place_that_spawns`, which matches the \
207 + literal `Command::new(`:\n {}",
208 + aliased.join("\n ")
209 + );
210 + }
211 + }
212 +
213 + /// The mechanism that was not chosen stays unchosen.
214 + ///
215 + /// `rfkill` was first fixed by trying `rfkill` and then `/usr/sbin/rfkill`, one
216 + /// call site carrying its own answer. Two mechanisms is worse than either: the
217 + /// absolute path is what the pane then shows, it cannot help the installer's
218 + /// `chroot <root> useradd`, and it has to be remembered for every tool added
219 + /// after it. If one comes back, this says so at the call site rather than
220 + /// leaving the reasoning in a comment nobody reads.
221 + #[test]
222 + fn no_tool_is_spawned_by_an_absolute_sbin_path() {
223 + for (name, text) in modules() {
224 + for line in shipped(&name, &text).lines() {
225 + for program in absolute_programs(line) {
226 + assert!(
227 + !program.starts_with("/sbin/") && !program.starts_with("/usr/sbin/"),
228 + "{name} names `{program}` by absolute path; the sbin directories \
229 + are on every child's `PATH` already (`cli::child_command`)"
230 + );
231 + }
232 + }
233 + }
234 + }
235 +
236 + /// Every program name spelled as a literal on one line.
237 + ///
238 + /// Every occurrence, not the first. `split_once` stopped at the first opening on
239 + /// the line, so a second call further along it was never looked at, and one line
240 + /// carrying two invocations is ordinary Rust rather than a stretch.
241 + fn absolute_programs(line: &str) -> Vec<&str> {
242 + if is_comment(line) {
243 + return Vec::new();
244 + }
245 + let mut found = Vec::new();
246 + for opening in ["Invocation::new(\"", "child_command(\""] {
247 + let mut rest = line;
248 + while let Some((_, after)) = rest.split_once(opening) {
249 + let program = after.split('"').next().unwrap_or(after);
250 + found.push(program);
251 + rest = after;
252 + }
253 + }
254 + found
255 + }
256 +
257 + /// The `PATH` the console builds names both directories.
258 + ///
259 + /// Asserted against the literal in `cli.rs` rather than by importing it: the
260 + /// crate is a binary, and this file is a text check over its sources like the
261 + /// rest of it. `/sbin` is a symlink to `/usr/sbin` on the Alloy image, and
262 + /// naming both is what keeps the list from depending on which of the two a
263 + /// given host merged.
264 + #[test]
265 + fn both_sbin_directories_are_named() {
266 + let cli = std::fs::read_to_string(src_dir().join("cli.rs")).expect("cannot read cli.rs");
267 + let dirs = cli
268 + .lines()
269 + .find_map(|line| line.strip_prefix("const SBIN_DIRS: [&str; 2] = ["))
270 + .and_then(|rest| rest.split(']').next())
271 + .expect("no SBIN_DIRS in cli.rs");
272 +
273 + let named: Vec<&str> = dirs
274 + .split(',')
275 + .map(|entry| entry.trim().trim_matches('"'))
276 + .filter(|entry| !entry.is_empty())
277 + .collect();
278 + assert_eq!(named, ["/usr/sbin", "/sbin"]);
279 + }
280 +
281 + /// A guard is only a guard if it can fail.
282 + ///
283 + /// The checks above pass on a tree where nothing is wrong, which is also what a
284 + /// check with a broken matcher does. This runs each matcher over sources that
285 + /// are wrong on purpose.
286 + #[test]
287 + fn the_guard_catches_what_it_is_for() {
288 + let direct = "fn probe() {\n Command::new(\"cryptsetup\").status()\n}\n";
289 + assert_eq!(spawns(direct).len(), 1);
290 +
291 + let parser = " clap::Command::new(\"alloy\")\n";
292 + assert!(spawns(parser).is_empty());
293 +
294 + // The tail is cut, so a spawn below it is not the crate's business.
295 + let tested = format!("fn ship() {{}}\n\n#[cfg(test)]\nmod tests {{\n{direct}}}\n");
296 + assert!(spawns(&shipped("fake.rs", &tested)).is_empty());
297 + assert_eq!(spawns(&tested).len(), 1, "and the cut is what excused it");
298 +
299 + // The alias `spawns` cannot see, and the check that sees it.
300 + let aliased = "use std::process::Command as Proc;\n\n Proc::new(\"wipefs\")\n";
301 + assert!(spawns(aliased).is_empty(), "the literal check cannot");
302 + assert_eq!(process_aliases(aliased).len(), 1);
303 +
304 + // A plain import is not a rename: `shell.rs` names the type without
305 + // spawning, and this must not be the check that stops it.
306 + assert!(process_aliases("use std::process::Command;\n").is_empty());
307 + assert!(process_aliases("use std::process::{Command, Stdio};\n").is_empty());
308 +
309 + // The second call on a line, which `split_once` never reached.
310 + let two = " pick(Invocation::new(\"rfkill\"), Invocation::new(\"/usr/sbin/rfkill\"));";
311 + assert_eq!(absolute_programs(two), ["rfkill", "/usr/sbin/rfkill"]);
312 +
313 + // A `//` inside a string literal is not the start of a comment, and
314 + // truncating there would hide everything after it.
315 + let quoted = r#" child_command("//weird").arg(Invocation::new("/usr/sbin/useradd"));"#;
316 + assert_eq!(absolute_programs(quoted), ["/usr/sbin/useradd", "//weird"]);
317 +
318 + // A whole-line comment is still not code.
319 + assert!(absolute_programs(" // Invocation::new(\"/usr/sbin/rfkill\")").is_empty());
320 + assert!(spawns(" /// Command::new(\"x\")").is_empty());
321 + }
322 +
323 + /// `modules` reads the directory the crate is actually built from, all of it.
324 + ///
325 + /// A path that resolved to nothing would make every check above pass by having
326 + /// nothing to look at, which is the failure mode a text check over a tree has.
327 + /// The recursion has the same failure in miniature, so it is exercised against a
328 + /// tree written for the purpose rather than trusted to be right.
329 + #[test]
330 + fn the_check_reads_the_crate_it_guards() {
331 + let names: Vec<String> = modules().into_iter().map(|(name, _)| name).collect();
332 + for expected in ["cli.rs", "main.rs", "install.rs", "bluetooth.rs"] {
333 + assert!(
334 + names.iter().any(|name| name == expected),
335 + "{expected} is missing from {names:?}"
336 + );
337 + }
338 + assert!(Path::new(&src_dir()).is_dir());
339 +
340 + let root = TempTree::new("modules-are-recursive");
341 + std::fs::create_dir_all(root.path().join("install")).expect("a nested module directory");
342 + std::fs::write(root.path().join("main.rs"), "fn main() {}\n").expect("a top-level module");
343 + std::fs::write(root.path().join("install/disk.rs"), "fn wipe() {}\n").expect("a nested module");
344 + std::fs::write(root.path().join("wordlist.txt"), "not rust\n").expect("a non-module");
345 +
346 + let mut found = Vec::new();
347 + collect(root.path(), root.path(), &mut found);
348 + let mut names: Vec<String> = found.into_iter().map(|(name, _)| name).collect();
349 + names.sort();
350 + assert_eq!(names, ["install/disk.rs", "main.rs"]);
351 + }
352 +
353 + /// The assumption the whole mechanism rests on: a bare program name is resolved
354 + /// against the `PATH` the child was given, not the one this process holds.
355 + ///
356 + /// std does that lookup itself once the environment has been touched rather than
357 + /// handing it to `posix_spawnp`, which searches the caller's `PATH` and would
358 + /// find nothing here. It is a guarantee of the library and not of POSIX, which
359 + /// is exactly why it is worth a test: if it ever stopped holding, `child_command`
360 + /// would go back to setting a `PATH` nothing reads and nothing else would notice.
361 + ///
362 + /// Here rather than in `cli.rs`'s unit tests because it writes and runs a script,
363 + /// and an integration test has `CARGO_TARGET_TMPDIR`. `std::env::temp_dir` was
364 + /// the other option and it is the wrong disk twice over: a `/tmp` mounted
365 + /// `noexec` fails this for a reason that has nothing to do with the claim, and
366 + /// nothing sweeps what it leaves behind.
367 + #[test]
368 + fn a_bare_name_is_found_through_the_child_path() {
369 + let dir = TempTree::new("child-path");
370 + use std::os::unix::fs::PermissionsExt as _;
371 +
372 + let tool = dir.path().join("alloy-not-on-any-path");
373 + std::fs::write(&tool, "#!/bin/sh\necho found\n").expect("the fake tool is written");
374 + std::fs::set_permissions(&tool, std::fs::Permissions::from_mode(0o755))
375 + .expect("the fake tool is executable");
376 +
377 + // Not through `child_command`, which adds the sbin directories: what is
378 + // under test is that `env` decides the lookup, and a directory this
379 + // process's own PATH cannot contain is the only honest way to ask.
380 + let found = std::process::Command::new("alloy-not-on-any-path")
381 + .env("PATH", dir.path())
382 + .output()
383 + .expect("the child PATH is what the lookup reads");
384 + assert_eq!(String::from_utf8_lossy(&found.stdout).trim(), "found");
385 +
386 + // And the same name with the inherited PATH does not resolve, so the
387 + // assertion above is about the environment and not about the directory
388 + // happening to be searched anyway.
389 + assert!(
390 + std::process::Command::new("alloy-not-on-any-path")
391 + .output()
392 + .is_err()
393 + );
394 + }
395 +
396 + /// A directory under `CARGO_TARGET_TMPDIR`, removed on drop.
397 + ///
398 + /// Dropping rather than a call at the end of the test: a failed assertion
399 + /// unwinds past the call and leaves the tree behind, and the tests that want one
400 + /// here are tests whose assertions can fail.
401 + struct TempTree(PathBuf);
402 +
403 + impl TempTree {
404 + fn new(what: &str) -> Self {
405 + let path = Path::new(env!("CARGO_TARGET_TMPDIR")).join(format!(
406 + "alloy-{what}-{}-{:?}",
407 + std::process::id(),
408 + std::thread::current().id()
409 + ));
410 + std::fs::remove_dir_all(&path).ok();
411 + std::fs::create_dir_all(&path).expect("a scratch directory under the target dir");
412 + Self(path)
413 + }
414 +
415 + fn path(&self) -> &Path {
416 + &self.0
417 + }
418 + }
419 +
420 + impl Drop for TempTree {
421 + fn drop(&mut self) {
422 + std::fs::remove_dir_all(&self.0).ok();
423 + }
424 + }