From 1e776a7a305d8027457892c85e66d989b2f96e6d Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sat, 15 Aug 2026 23:16:51 -0700 Subject: [PATCH 1/3] fix(cli): #891 -- headless sys.openUrl logs the URL instead of launching a host GUI browser Legacy Frontier launched the owner's browser at the setupFrontier page to collect config from potentially non-technical server owners. That onboarding intent is a required feature and is preserved. The only wrong part is a headless/server/CI process reaching into a host GUI session: there is no human at that machine's desktop, and the daemon host is frequently not the operator's workstation. Root cause: webBrowser.openURL already branched on system.environment.isHeadless, but that branch was not a no-exec branch -- it called sys.openUrl, the kernel verb that double-forks and execs open(1). It succeeded, so the msg() fallback never ran and a browser opened. Headless was the MORE aggressive path, not the safer one. The guard goes in the kernel verb, the single choke point every caller funnels through (webBrowser.openURL, trialVersionCheck, ftpTable, Frontier.tools.commands .openUrl, and the live trigger startupScript.ut:252 -> system.temp.installer .urlToOpen set by finishInstall). Guarding here cannot be bypassed by a script calling sys.openUrl directly, and needs no ODB edit. Headless behavior by --browser value: absent / "default" -> no exec; log the URL and record it at system.temp.Frontier.pendingBrowserUrl "agent-browser" -> unchanged (automated verification path) "allow-gui" -> NEW; restores the real launch for a local desktop session anything else -> rejected, as before The marker records the last URL that wanted a browser (overwrite per call, no URL sniffing), so first-run flows assert the onboarding step by URL equality rather than a boolean. GUI builds are unaffected: the headless block is inside FRONTIER_HEADLESS. Note on file layout: Common/source/shellsysverbs.c is NOT linked into frontier-cli (absent from frontier-cli/Makefile); tests/headless_sys_verbs.c is the production headless implementation despite living under tests/. Both carry the logic and are changed in lockstep, each with a TWIN comment naming the other. Also corrects a latent wrong Pascal length byte in the shellsysverbs.c rejection message. Tests: tests/openurl_headless_guard_test.sh proves the default performs NO exec and the opt-in DOES, using PATH-shadowing stub launchers so no real browser is ever invoked. Registered as test-openurl-guard in test-all. Four integration cases added and two stale "not implemented" skips cleared. Unit: exit 0. Integration: 2491 total, 0 failed (baseline 2488/0; +3 tests, -2 skips). Pre-existing and unchanged: 3 sys.appisrunning failures and the load-dependent test_callback_infrastructure segfault (#828), both reproduced on pristine 2b56ce868. --- Common/source/shellsysverbs.c | 44 +++- frontier-cli/main.c | 8 +- tests/Makefile | 11 +- tests/headless_sys_verbs.c | 113 +++++++- .../test_cases/sys_processor_tests.yaml | 39 ++- tests/openurl_headless_guard_test.sh | 243 ++++++++++++++++++ 6 files changed, 436 insertions(+), 22 deletions(-) create mode 100755 tests/openurl_headless_guard_test.sh diff --git a/Common/source/shellsysverbs.c b/Common/source/shellsysverbs.c index 04750d7dc..def86e479 100644 --- a/Common/source/shellsysverbs.c +++ b/Common/source/shellsysverbs.c @@ -838,11 +838,25 @@ static boolean sysfunctionvalue (short token, hdltreenode hparam1, tyvaluerecord * macOS: execlp("open", ...), Linux: execlp("xdg-open", ...). * * 3/23/26 JES: Respect --browser CLI arg via system.environment.args.browser. - * Allowed values: "default" (system browser) or "agent-browser". + * Allowed values: "default", "agent-browser" or "allow-gui". * Note: Scripts can also set system.environment.args.browser directly; * this is intentional — scripts are trusted code within the ODB. * The browser check is guarded by FRONTIER_HEADLESS; GUI builds * always use the system default browser (no --browser flag). + * + * 2026-08-15 JES #891: in headless builds the default no longer execs a + * host GUI browser -- it logs the URL and records it at + * system.temp.Frontier.pendingBrowserUrl. The onboarding intent (legacy + * Frontier opened the owner's browser at setupFrontier to collect config) + * is preserved; only the host-GUI launch from a headless/server context is + * wrong, since no human is at that machine's desktop. --browser allow-gui + * restores the launch. GUI builds are unaffected: the whole block is + * inside FRONTIER_HEADLESS. + * + * TWIN: tests/headless_sys_verbs.c case sysv_openurl carries the identical + * logic and must change in lockstep. NOTE that despite living under + * Common/source, THIS file is not linked into frontier-cli (it is absent + * from frontier-cli/Makefile); the headless twin is what the CLI runs. */ Handle hurl; @@ -875,8 +889,12 @@ static boolean sysfunctionvalue (short token, hdltreenode hparam1, tyvaluerecord * fall through to the platform default (open/xdg-open). */ /* Always false in non-headless (GUI) builds — the ifdef below - * only sets it to true in headless mode. */ + * only sets them in headless mode. In a GUI build both stay false + * and the exec proceeds, which is the correct legacy behavior: + * there IS a human at this machine's desktop. */ boolean use_agent_browser = false; + boolean allow_gui_browser = false; + boolean fl_no_exec = false; #ifdef FRONTIER_HEADLESS { @@ -903,11 +921,14 @@ static boolean sysfunctionvalue (short token, hdltreenode hparam1, tyvaluerecord if (len == (long)(sizeof ("agent-browser") - 1) && memcmp (*vbrowser.data.stringvalue, "agent-browser", sizeof ("agent-browser") - 1) == 0) { use_agent_browser = true; } + else if (len == (long)(sizeof ("allow-gui") - 1) && memcmp (*vbrowser.data.stringvalue, "allow-gui", sizeof ("allow-gui") - 1) == 0) { + allow_gui_browser = true; + } else if (len != (long)(sizeof ("default") - 1) || memcmp (*vbrowser.data.stringvalue, "default", sizeof ("default") - 1) != 0) { /* Unknown browser value — reject for security */ unlockhandle (hurl); disposehandle (hurl); - langerrormessage (BIGSTRING ("\x45" "Can't open URL: --browser must be \"default\" or \"agent-browser\".")); + langerrormessage (BIGSTRING ("\x4c" "Can't open URL: --browser must be \"default\", \"agent-browser\" or \"allow-gui\".")); return (false); } } @@ -916,8 +937,23 @@ static boolean sysfunctionvalue (short token, hdltreenode hparam1, tyvaluerecord } } } + + /* #891: headless default performs no exec. Kept in lockstep with the + * headless twin; see the TWIN note above. */ + fl_no_exec = !use_agent_browser && !allow_gui_browser; #endif + if (fl_no_exec) { + unlockhandle (hurl); + disposehandle (hurl); + + /* True: the onboarding step was reached, which is what the caller + * asks about. The headless twin additionally logs the URL and sets + * system.temp.Frontier.pendingBrowserUrl; this classic path has + * neither the logging nor the marker helper in scope. */ + return (setbooleanvalue (true, v)); + } + #if defined(__APPLE__) || defined(__linux__) /* Double-fork to avoid zombie processes: first child forks again @@ -939,7 +975,7 @@ static boolean sysfunctionvalue (short token, hdltreenode hparam1, tyvaluerecord if (use_agent_browser) { execlp ("agent-browser", "agent-browser", "open", url, NULL); } - else { + else { /* allow_gui_browser, or any GUI build (#891) */ #ifdef __APPLE__ execlp ("open", "open", url, NULL); #else diff --git a/frontier-cli/main.c b/frontier-cli/main.c index 4301cd79c..b6bd4840e 100644 --- a/frontier-cli/main.c +++ b/frontier-cli/main.c @@ -1387,8 +1387,12 @@ static void print_usage(const char* program_name) { printf(" --my-flag value -> system.environment.args.myFlag\n"); printf("\n"); printf(" Built-in custom flags:\n"); - printf(" --browser MODE Set browser for sys.openUrl (default: system default)\n"); - printf(" MODE: \"default\" or \"agent-browser\"\n"); + printf(" --browser MODE Set browser for sys.openUrl (default: log the URL only)\n"); + printf(" MODE: \"default\", \"agent-browser\" or \"allow-gui\"\n"); + printf(" default: log the URL, set\n"); + printf(" system.temp.Frontier.pendingBrowserUrl, no exec\n"); + printf(" agent-browser: drive the URL with agent-browser\n"); + printf(" allow-gui: launch this machine's GUI browser\n"); printf("\n"); printf("Environment Variables:\n"); diff --git a/tests/Makefile b/tests/Makefile index 1c169db42..b746a9b0d 100644 --- a/tests/Makefile +++ b/tests/Makefile @@ -888,6 +888,13 @@ test-custom-args: @echo "Running custom CLI args tests..." @./custom_cli_args_test.sh +# Run headless sys.openUrl GUI-browser guard tests (shell-based, uses -e mode) +# 2026-08-15 JES #891: the headless default must not exec a host GUI browser. +# Exec assertions use PATH-shadowing stubs, so no real browser is ever launched. +test-openurl-guard: + @echo "Running headless sys.openUrl guard tests..." + @./openurl_headless_guard_test.sh + # Run REPL selection flag tests (shell-based, uses -e mode + --help) # 2026-06-25 JES C.6 #691: covers --plain / --debug-tui / --protocol # mutual exclusion, the --debug-tui deprecation warning, and the new @@ -935,7 +942,7 @@ check-license: @python3 ../tools/relicense_headers.py --check # Run all tests (unit + integration + CLI migration + custom args + debug + hooks + license check) -test-all: test test-integration test-migrate-flag test-migrate-long-path test-system-root-long-path test-system-root-temp-path-boundary test-sys-getenv-long-value test-custom-args test-repl-flags test-debug test-agent-debug test-debug-tui test-cow-idempotent test-ut-sync-lifecycle test-hooks check-license +test-all: test test-integration test-migrate-flag test-migrate-long-path test-system-root-long-path test-system-root-temp-path-boundary test-sys-getenv-long-value test-custom-args test-openurl-guard test-repl-flags test-debug test-agent-debug test-debug-tui test-cow-idempotent test-ut-sync-lifecycle test-hooks check-license @echo "All test suites completed" # Verify Virgin.root <-> .ut corpus sync (issue #675). @@ -1027,7 +1034,7 @@ verify-errors: @echo "Verifying error message coverage..." @../tools/verify_error_coverage.sh -.PHONY: all test test-report test-integration test-integration-verbose test-migrate-flag test-migrate-long-path test-system-root-long-path test-system-root-temp-path-boundary test-sys-getenv-long-value test-custom-args test-repl-flags test-debug test-agent-debug test-debug-tui test-hooks test-all verify-odb-sync test-odb-sync clean install uninstall help results strings_generated kernel_verbs_generated verify-errors validate-bigstrings check-python-deps +.PHONY: all test test-report test-integration test-integration-verbose test-migrate-flag test-migrate-long-path test-system-root-long-path test-system-root-temp-path-boundary test-sys-getenv-long-value test-custom-args test-openurl-guard test-repl-flags test-debug test-agent-debug test-debug-tui test-hooks test-all verify-odb-sync test-odb-sync clean install uninstall help results strings_generated kernel_verbs_generated verify-errors validate-bigstrings check-python-deps check_v6_tables: check_v6_tables.c $(LANG_RUNTIME_SOURCES) ../Common/source/odbengine.c ../frontier-cli/stubs/headless_shell.c $(CC) $(CFLAGS) -DFRONTIER_HEADLESS -DHEADLESS_TEST_PORTABLE_FILE -DHEADLESS_LINKS_REAL_DB -I../Common/headers -I../Common/SystemHeaders -I../portable check_v6_tables.c \ diff --git a/tests/headless_sys_verbs.c b/tests/headless_sys_verbs.c index 6b25682e1..de798403f 100644 --- a/tests/headless_sys_verbs.c +++ b/tests/headless_sys_verbs.c @@ -20,6 +20,7 @@ #include "standard.h" #include /* for getenv, setenv */ +#include /* 2026-08-15 JES #891: for strlen in setpendingbrowserurl */ #include /* for fork, execlp, _exit */ #include /* for waitpid */ #include "memory.h" @@ -27,6 +28,7 @@ #include "lang.h" #include "langinternal.h" #include "tablestructure.h" +#include "tableverbs.h" /* 2026-08-15 JES #891: for findnamedtable */ #include "sysshellcall.h" #include "logging.h" #include "langexternal.h" @@ -72,6 +74,67 @@ static void trimtrailingwhitespace(Handle h) { } } +/* + * #891: record the URL that wanted a browser at + * system.temp.Frontier.pendingBrowserUrl, creating the intervening tables if + * needed. Semantics are "last URL that wanted a browser" -- each call + * overwrites, and no URL sniffing is done, so first-run flows assert the + * setup URL by equality rather than a bare boolean. + * + * The value is stored as a heap string (newfilledhandle) rather than via the + * bigstring assign helpers because a URL can exceed the 255-byte Pascal-string + * limit. + */ +static boolean setpendingbrowserurl (const char *url) { + hdlhashtable httemp; + hdlhashtable htfrontier; + bigstring bstemp; + bigstring bsfrontier; + bigstring bsurl; + Handle hvalue; + tyvaluerecord val; + long len; + + if (systemtable == nil) + return (false); + + copystring (PSTRING ("\x04", "temp"), bstemp); + copystring (PSTRING ("\x08", "Frontier"), bsfrontier); + copystring (PSTRING ("\x11", "pendingBrowserUrl"), bsurl); + + if (!findnamedtable (systemtable, bstemp, &httemp)) { + if (!tablenewsubtable (systemtable, bstemp, &httemp)) + return (false); + } + + if (!findnamedtable (httemp, bsfrontier, &htfrontier)) { + if (!tablenewsubtable (httemp, bsfrontier, &htfrontier)) + return (false); + } + + len = (long) strlen (url); + + if (!newfilledhandle ((ptrvoid) url, len, &hvalue)) + return (false); + + if (!setheapvalue (hvalue, stringvaluetype, &val)) { + disposehandle (hvalue); + return (false); + } + + if (!hashtableassign (htfrontier, bsurl, val)) { + disposehandle (hvalue); + return (false); + } + + /* The table now owns the handle; keep it off the tmp stack so it is not + * released out from under the marker. */ + exemptfromtmpstack (&val); + + return (true); +} /*setpendingbrowserurl*/ + + /* Helper function to safely escape shell arguments for single-quote wrapping * Converts: foo'bar -> 'foo'\''bar' * This prevents command injection by escaping all single quotes and wrapping in quotes @@ -642,8 +705,23 @@ static boolean sys_valueproc(short token, hdltreenode hparam1, * Uses fork/execlp to avoid shell injection. * Respects --browser CLI arg via system.environment.args.browser: * "agent-browser" → execlp("agent-browser", "agent-browser", "open", url) - * "default" or absent → execlp("open", "open", url) on macOS - * See shellsysverbs.c openurlfunc for the equivalent classic implementation. + * "allow-gui" → execlp("open", "open", url) on macOS / xdg-open on Linux + * "default" or absent → no exec; log the URL and record it at + * system.temp.Frontier.pendingBrowserUrl + * + * 2026-08-15 JES #891: the headless default no longer execs a host GUI + * browser. Legacy Frontier launched the owner's browser at the + * setupFrontier page to collect config from the server owner, and that + * onboarding intent is preserved -- but a headless/server/CI process has + * no human at its desktop, and the daemon host is frequently not the + * operator's workstation. The default logs the URL for a remote operator + * and records it in the marker; --browser allow-gui restores the launch + * for a local desktop-style session. + * + * TWIN: Common/source/shellsysverbs.c openurlfunc carries the identical + * logic for the classic build and must change in lockstep. That file is + * NOT in the frontier-cli link (grep frontier-cli/Makefile); this TU is + * the one the CLI actually runs. */ /* No URL scheme validation — any non-empty string is passed through @@ -678,6 +756,7 @@ static boolean sys_valueproc(short token, hdltreenode hparam1, /* Check --browser arg from system.environment.args */ boolean use_agent_browser = false; + boolean allow_gui_browser = false; { hdlhashnode hnode; @@ -704,10 +783,13 @@ static boolean sys_valueproc(short token, hdltreenode hparam1, if (len == (long)(sizeof ("agent-browser") - 1) && memcmp (*vbrowser.data.stringvalue, "agent-browser", sizeof ("agent-browser") - 1) == 0) { use_agent_browser = true; } + else if (len == (long)(sizeof ("allow-gui") - 1) && memcmp (*vbrowser.data.stringvalue, "allow-gui", sizeof ("allow-gui") - 1) == 0) { + allow_gui_browser = true; + } else if (len != (long)(sizeof ("default") - 1) || memcmp (*vbrowser.data.stringvalue, "default", sizeof ("default") - 1) != 0) { unlockhandle (hurl); disposehandle (hurl); - langerrormessage (PSTRING ("\x3f", "Can't open URL: --browser must be \"default\" or \"agent-browser\".")); + langerrormessage (PSTRING ("\x4c", "Can't open URL: --browser must be \"default\", \"agent-browser\" or \"allow-gui\".")); return (false); } } @@ -717,6 +799,25 @@ static boolean sys_valueproc(short token, hdltreenode hparam1, } } + /* #891: headless default -- no host GUI browser. Log the URL so a + * remote operator can open it from their own machine, record it for + * flows and tests, and return true: the onboarding step was reached, + * which is what the caller is asking about. */ + if (!use_agent_browser && !allow_gui_browser) { + boolean flmarked = setpendingbrowserurl (url); + + log_info (LOG_COMP_GENERAL, + "Frontier first-run: complete setup at %s", url); + + unlockhandle (hurl); + disposehandle (hurl); + + if (!flmarked) + return (false); + + return (setbooleanvalue (true, vreturned)); + } + #if defined(__APPLE__) || defined(__linux__) /* Double-fork to avoid zombie processes. */ pid_t pid = fork (); @@ -725,11 +826,13 @@ static boolean sys_valueproc(short token, hdltreenode hparam1, pid_t pid2 = fork (); if (pid2 == 0) { - /* All execlp calls rely on a trusted PATH. */ + /* All execlp calls rely on a trusted PATH. + * Only reachable when an opt-in was named: the default + * returned above without forking (#891). */ if (use_agent_browser) { execlp ("agent-browser", "agent-browser", "open", url, NULL); } - else { + else { /* allow_gui_browser */ #ifdef __APPLE__ execlp ("open", "open", url, NULL); #else diff --git a/tests/integration/test_cases/sys_processor_tests.yaml b/tests/integration/test_cases/sys_processor_tests.yaml index f0ad8867d..bfb928004 100644 --- a/tests/integration/test_cases/sys_processor_tests.yaml +++ b/tests/integration/test_cases/sys_processor_tests.yaml @@ -385,15 +385,16 @@ tests: # even without a display server). # ============================================================================ - # sys.openUrl - Opens a URL via fork/execlp. In headless CI the fork succeeds - # even if the platform launcher (open/xdg-open) isn't available, so we only - # verify the verb is callable and returns a boolean. - # Note: These tests cannot verify actual browser launch — only that the - # verb compiles, dispatches, and returns the expected type. + # sys.openUrl in the headless default mode performs NO process exec (#891): + # it logs the URL and records it at system.temp.Frontier.pendingBrowserUrl so + # a remote operator (or a test) can pick it up. These tests are therefore safe + # to run anywhere — they cannot launch a browser on the host. + # The exec opt-ins (--browser allow-gui / agent-browser) need CLI flags the + # protocol runner can't pass, so they are covered by + # tests/openurl_headless_guard_test.sh with PATH-stubbed launchers. - name: "sys.openUrl - returns boolean type" - skip: "sys.openUrl not implemented" - description: "openUrl should return a boolean regardless of fork outcome" + description: "openUrl should return a boolean in the headless default mode" script: 'typeof(sys.openUrl("https://example.com"))' expected_success: true expected_result: "bool" @@ -406,12 +407,32 @@ tests: # getexempttextvalue coerces numbers to strings — standard Frontier behavior. # The verb accepts any non-empty string (no URL scheme validation). - name: "sys.openUrl - number coerced to string succeeds" - skip: "sys.openUrl not implemented" - description: "Passing a number coerces to string; fork succeeds even for non-URL" + description: "Passing a number coerces to string; accepted even for non-URL" script: 'typeof(sys.openUrl(12345))' expected_success: true expected_result: "bool" + - name: "sys.openUrl - headless default records the pending browser URL" + description: "The onboarding intent is asserted via the marker, not a browser launch" + script: 'sys.openUrl("http://127.0.0.1:5336/setupFrontier");system.temp.Frontier.pendingBrowserUrl' + expected_success: true + expected_result: "http://127.0.0.1:5336/setupFrontier" + + - name: "sys.openUrl - marker holds the most recent URL" + description: "Each call overwrites the marker (last URL that wanted a browser)" + script: 'sys.openUrl("http://example.com/first");sys.openUrl("http://example.com/second");system.temp.Frontier.pendingBrowserUrl' + expected_success: true + expected_result: "http://example.com/second" + + # Sequential tests share one session (#852), so an earlier openUrl case may have + # left a marker behind. Clear it first: this asserts a rejected URL does not + # WRITE a marker, which is independent of test ordering. + - name: "sys.openUrl - empty URL does not set the marker" + description: "A rejected URL must not leave a pending onboarding marker behind" + script: 'if defined(system.temp.Frontier.pendingBrowserUrl) {delete(@system.temp.Frontier.pendingBrowserUrl)};try {sys.openUrl("")};defined(system.temp.Frontier.pendingBrowserUrl)' + expected_success: true + expected_result: "false" + # ============================================================================ # sys.unixshellcommand Tests (4 parameter variants) # ============================================================================ diff --git a/tests/openurl_headless_guard_test.sh b/tests/openurl_headless_guard_test.sh new file mode 100755 index 000000000..91a66a33a --- /dev/null +++ b/tests/openurl_headless_guard_test.sh @@ -0,0 +1,243 @@ +#!/bin/bash +# +# Integration tests for the headless sys.openUrl GUI-browser guard (#891). +# +# Legacy Frontier launched the owner's browser at the setupFrontier page to +# collect config from the server owner. That onboarding intent is preserved; +# only the host-GUI launch from a headless/server/test context is wrong. +# +# Headless default: no process exec. The URL is logged and recorded at +# system.temp.Frontier.pendingBrowserUrl so flows and tests can assert the +# onboarding intent was reached. +# Opt-in (--browser allow-gui): the real launcher runs again. +# +# HARD RULE: no test here may launch a host GUI browser. The opt-in exec is +# proven with a PATH-shadowing stub named "open" that records its argv; the +# real /usr/bin/open is never reached because execlp resolves through PATH. +# +# These tests use -e mode (not --protocol) because the protocol runner cannot +# pass arbitrary unknown flags to the frontier-cli subprocess. + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" +CLI="$PROJECT_ROOT/frontier-cli/frontier-cli" +DB="$PROJECT_ROOT/databases/Frontier.root" + +if [ ! -x "$CLI" ]; then + echo "Error: frontier-cli not found at $CLI" >&2 + exit 1 +fi +if [ ! -f "$DB" ]; then + echo "Error: database not found at $DB" >&2 + exit 1 +fi + +RED='\033[0;31m' +GREEN='\033[0;32m' +NC='\033[0m' + +PASSED=0 +FAILED=0 + +TESTURL="http://127.0.0.1:5336/setupFrontier" + +# Scratch dir holding the PATH stubs and their recording file. +STUBDIR=$(mktemp -d "${TMPDIR:-/tmp}/openurl_guard.XXXXXX") +RECORD="$STUBDIR/exec_record.txt" + +cleanup() { + rm -rf "$STUBDIR" +} +trap cleanup EXIT + +# Stub launchers. Named exactly as the ones execlp() resolves through PATH, so +# a stub firing proves an exec happened and the real browser is never invoked. +for stub in open xdg-open agent-browser; do + cat > "$STUBDIR/$stub" <> "$RECORD" +exit 0 +STUBEOF + chmod +x "$STUBDIR/$stub" +done + +pass() { + echo -e " ${GREEN}PASS${NC}: $1" + PASSED=$((PASSED + 1)) +} + +fail() { + echo -e " ${RED}FAIL${NC}: $1" + echo " $2" + FAILED=$((FAILED + 1)) +} + +# Run the CLI with the stub dir FIRST on PATH so any exec hits a stub. +# The recording file is truncated per run so each assertion is independent. +run_cli() { + : > "$RECORD" + PATH="$STUBDIR:$PATH" "$CLI" "$@" 2>/dev/null +} + +# The double-fork means the grandchild runs asynchronously; give a real exec a +# moment to land before concluding the record is empty. +settle() { + local i + for i in 1 2 3 4 5 6 7 8 9 10; do + if [ -s "$RECORD" ]; then + return + fi + sleep 0.1 + done +} + +echo "==============================================" +echo "Headless sys.openUrl GUI-browser guard (#891)" +echo "==============================================" +echo + +echo "--- Default (no --browser): log and mark, never exec ---" + +actual=$(run_cli --skip-startup --system-root "$DB" \ + -e "sys.openUrl(\"$TESTURL\")") +if [ "$actual" = "true" ]; then + pass "default returns true (onboarding intent honored)" +else + fail "default returns true (onboarding intent honored)" "Got: '$actual'" +fi + +settle +if [ ! -s "$RECORD" ]; then + pass "default performs NO exec" +else + fail "default performs NO exec" "Exec recorded: $(cat "$RECORD")" +fi + +actual=$(run_cli --skip-startup --system-root "$DB" \ + -e "sys.openUrl(\"$TESTURL\");system.temp.Frontier.pendingBrowserUrl") +if [ "$actual" = "$TESTURL" ]; then + pass "default sets pendingBrowserUrl to the URL" +else + fail "default sets pendingBrowserUrl to the URL" "Expected '$TESTURL', got '$actual'" +fi + +echo +echo "--- Explicit --browser default: same as absent ---" + +actual=$(run_cli --skip-startup --system-root "$DB" --browser default \ + -e "sys.openUrl(\"$TESTURL\")") +if [ "$actual" = "true" ]; then + pass "--browser default returns true" +else + fail "--browser default returns true" "Got: '$actual'" +fi + +settle +if [ ! -s "$RECORD" ]; then + pass "--browser default performs NO exec" +else + fail "--browser default performs NO exec" "Exec recorded: $(cat "$RECORD")" +fi + +echo +echo "--- Marker overwrites on each call (last URL wins) ---" + +actual=$(run_cli --skip-startup --system-root "$DB" \ + -e "sys.openUrl(\"http://example.com/first\");sys.openUrl(\"$TESTURL\");system.temp.Frontier.pendingBrowserUrl") +if [ "$actual" = "$TESTURL" ]; then + pass "marker holds the most recent URL" +else + fail "marker holds the most recent URL" "Expected '$TESTURL', got '$actual'" +fi + +echo +echo "--- Opt-in --browser allow-gui: real launcher runs (stubbed) ---" + +actual=$(run_cli --skip-startup --system-root "$DB" --browser allow-gui \ + -e "sys.openUrl(\"$TESTURL\")") +if [ "$actual" = "true" ]; then + pass "allow-gui returns true" +else + fail "allow-gui returns true" "Got: '$actual'" +fi + +settle +if grep -q "$TESTURL" "$RECORD" 2>/dev/null; then + pass "allow-gui EXECS the launcher with the URL" +else + fail "allow-gui EXECS the launcher with the URL" "Record: '$(cat "$RECORD" 2>/dev/null)'" +fi + +echo +echo "--- Opt-in does NOT set the pending marker ---" + +actual=$(run_cli --skip-startup --system-root "$DB" --browser allow-gui \ + -e "sys.openUrl(\"$TESTURL\");defined(system.temp.Frontier.pendingBrowserUrl)") +if [ "$actual" = "false" ]; then + pass "allow-gui leaves pendingBrowserUrl undefined" +else + fail "allow-gui leaves pendingBrowserUrl undefined" "Got: '$actual'" +fi + +echo +echo "--- agent-browser still execs (automation path unchanged) ---" + +actual=$(run_cli --skip-startup --system-root "$DB" --browser agent-browser \ + -e "sys.openUrl(\"$TESTURL\")") +if [ "$actual" = "true" ]; then + pass "agent-browser returns true" +else + fail "agent-browser returns true" "Got: '$actual'" +fi + +settle +if grep -q "^agent-browser .*$TESTURL" "$RECORD" 2>/dev/null; then + pass "agent-browser EXECS agent-browser with the URL" +else + fail "agent-browser EXECS agent-browser with the URL" "Record: '$(cat "$RECORD" 2>/dev/null)'" +fi + +echo +echo "--- Unknown --browser value is rejected ---" + +actual=$(run_cli --skip-startup --system-root "$DB" --browser firefox \ + -e "sys.openUrl(\"$TESTURL\")") +if [ "$actual" != "true" ]; then + pass "unknown browser value does not report success" +else + fail "unknown browser value does not report success" "Got: '$actual'" +fi + +settle +if [ ! -s "$RECORD" ]; then + pass "unknown browser value performs NO exec" +else + fail "unknown browser value performs NO exec" "Exec recorded: $(cat "$RECORD")" +fi + +echo +echo "--- Empty URL still errors before any fork ---" + +actual=$(run_cli --skip-startup --system-root "$DB" -e 'sys.openUrl("")') +if [ "$actual" != "true" ]; then + pass "empty URL does not report success" +else + fail "empty URL does not report success" "Got: '$actual'" +fi + +settle +if [ ! -s "$RECORD" ]; then + pass "empty URL performs NO exec" +else + fail "empty URL performs NO exec" "Exec recorded: $(cat "$RECORD")" +fi + +echo +echo "==============================================" +echo "Passed: $PASSED Failed: $FAILED" +echo "==============================================" + +if [ "$FAILED" -ne 0 ]; then + exit 1 +fi +exit 0 From 41c5efc1b46456310154681062621d80355f62fd Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sat, 15 Aug 2026 23:40:35 -0700 Subject: [PATCH 2/3] fix(cli): #891 gate round -- error-path ownership in setpendingbrowserurl (P1-1, P1-2) + 4 P2s P1-1 (headless_sys_verbs.c): setheapvalue disposes the handle itself on failure (langvalue.c:594 -- "the caller isn't managing the memory any more"), so the disposehandle on that branch was a double free. Removed. P1-2 (headless_sys_verbs.c): on hashtableassign failure the value is still on the tmp stack, so a bare disposehandle left a dangling tmp-stack entry. Replaced with disposetmpvalue, which unstacks AND disposes. Documented that the assign->exempt pair must stay yield-free. P2-3 (shellsysverbs.c): allow_gui_browser scoped inside the FRONTIER_HEADLESS block; it would be set-but-unused in a GUI compile. The fl_no_exec assignment moved inside the same block so it can still read it. P2-4 (openurl_headless_guard_test.sh): the test copies Frontier.root into its scratch dir and points --system-root at the copy. The CLI opens the system root read-write; these runs must not touch the shared working DB. P2-5 (headless_sys_verbs.c): the setup-URL log_info now runs only after the marker write succeeds, so the operator-facing line cannot claim onboarding was reached when the marker was never recorded. P2-6 (both TUs): TWIN comments no longer claim "identical logic". The classic path suppresses the exec but does not log or set the marker, so the observable contract diverges. Both sides now say so and reference #893. Test note on P1-2: the review suggested pre-creating the marker path as a table to force the assign-failure branch from script. Verified directly that this does NOT fail in this build -- hashassign's protect block is gated on fllanghashassignprotect, which headless_threadglobals.c:60 initializes to false and whose only toggles (tableverbs.c:288/294) are commented out. The assign succeeds and overwrites the table. Added a test asserting that actual observable behavior instead of a failure that cannot occur; the P1-2 cleanup path is covered by inspection against the langvalue.c/lang.h contracts. Guard test 15/15. Integration 2492 total, 0 failed. Unit failure set = {#828} only (test_callback_infrastructure 2/6 and test_window_bridge 1/6 clean standalone runs; both nondeterministic, both unlinked from either file changed here -- verified test_window_bridge segfaults 4/5 on pristine 2b56ce868 too). --- Common/source/shellsysverbs.c | 30 +++++++++------- tests/headless_sys_verbs.c | 36 +++++++++++++------ .../test_cases/sys_processor_tests.yaml | 13 +++++++ tests/openurl_headless_guard_test.sh | 16 ++++++--- 4 files changed, 68 insertions(+), 27 deletions(-) diff --git a/Common/source/shellsysverbs.c b/Common/source/shellsysverbs.c index def86e479..c882609d9 100644 --- a/Common/source/shellsysverbs.c +++ b/Common/source/shellsysverbs.c @@ -853,10 +853,13 @@ static boolean sysfunctionvalue (short token, hdltreenode hparam1, tyvaluerecord * restores the launch. GUI builds are unaffected: the whole block is * inside FRONTIER_HEADLESS. * - * TWIN: tests/headless_sys_verbs.c case sysv_openurl carries the identical - * logic and must change in lockstep. NOTE that despite living under - * Common/source, THIS file is not linked into frontier-cli (it is absent - * from frontier-cli/Makefile); the headless twin is what the CLI runs. + * TWIN: tests/headless_sys_verbs.c case sysv_openurl is the parallel + * implementation and must change in lockstep. NOTE that despite living + * under Common/source, THIS file is not linked into frontier-cli (it is + * absent from frontier-cli/Makefile); the headless twin is what the CLI + * runs. The two are NOT observably identical: the twin also logs the URL + * and sets system.temp.Frontier.pendingBrowserUrl, neither of which is in + * scope here, so this path only suppresses the exec. Dedup tracked in #893. */ Handle hurl; @@ -889,15 +892,18 @@ static boolean sysfunctionvalue (short token, hdltreenode hparam1, tyvaluerecord * fall through to the platform default (open/xdg-open). */ /* Always false in non-headless (GUI) builds — the ifdef below - * only sets them in headless mode. In a GUI build both stay false - * and the exec proceeds, which is the correct legacy behavior: - * there IS a human at this machine's desktop. */ + * only sets it to true in headless mode. */ boolean use_agent_browser = false; - boolean allow_gui_browser = false; + + /* Stays false in a GUI build, so the exec proceeds: the correct + * legacy behavior, since there IS a human at this machine's + * desktop. allow_gui_browser is scoped to the headless block + * below, where it is the only place it can be read. */ boolean fl_no_exec = false; #ifdef FRONTIER_HEADLESS { + boolean allow_gui_browser = false; hdlhashnode hnode; tyvaluerecord vargs; bigstring bsargs = BIGSTRING ("\x04" "args"); @@ -936,11 +942,11 @@ static boolean sysfunctionvalue (short token, hdltreenode hparam1, tyvaluerecord } } } - } - /* #891: headless default performs no exec. Kept in lockstep with the - * headless twin; see the TWIN note above. */ - fl_no_exec = !use_agent_browser && !allow_gui_browser; + /* #891: headless default performs no exec. Kept in lockstep with + * the headless twin; see the TWIN note above. */ + fl_no_exec = !use_agent_browser && !allow_gui_browser; + } #endif if (fl_no_exec) { diff --git a/tests/headless_sys_verbs.c b/tests/headless_sys_verbs.c index de798403f..ff4569e32 100644 --- a/tests/headless_sys_verbs.c +++ b/tests/headless_sys_verbs.c @@ -117,18 +117,25 @@ static boolean setpendingbrowserurl (const char *url) { if (!newfilledhandle ((ptrvoid) url, len, &hvalue)) return (false); - if (!setheapvalue (hvalue, stringvaluetype, &val)) { - disposehandle (hvalue); + /* setheapvalue disposes the handle itself on failure (langvalue.c:594 -- + * "the caller isn't managing the memory any more"), so do NOT dispose here. */ + if (!setheapvalue (hvalue, stringvaluetype, &val)) return (false); - } if (!hashtableassign (htfrontier, bsurl, val)) { - disposehandle (hvalue); + /* val is still on the tmp stack; disposetmpvalue removes it from the + * stack AND disposes the handle. A bare disposehandle would leave a + * dangling tmp-stack entry. Reachable from script: pre-creating + * system.temp.Frontier.pendingBrowserUrl as a table makes hashassign + * refuse the non-external-over-external overwrite. */ + disposetmpvalue (&val); return (false); } /* The table now owns the handle; keep it off the tmp stack so it is not - * released out from under the marker. */ + * released out from under the marker. The assign->exempt pair must stay + * yield-free: a yield in between could let the tmp stack unwind the value + * the table now references. */ exemptfromtmpstack (&val); return (true); @@ -718,10 +725,12 @@ static boolean sys_valueproc(short token, hdltreenode hparam1, * and records it in the marker; --browser allow-gui restores the launch * for a local desktop-style session. * - * TWIN: Common/source/shellsysverbs.c openurlfunc carries the identical - * logic for the classic build and must change in lockstep. That file is - * NOT in the frontier-cli link (grep frontier-cli/Makefile); this TU is - * the one the CLI actually runs. + * TWIN: Common/source/shellsysverbs.c openurlfunc is the parallel + * implementation for the classic build and must change in lockstep. + * That file is NOT in the frontier-cli link (grep frontier-cli/Makefile); + * this TU is the one the CLI actually runs. The two are NOT observably + * identical: the classic path suppresses the exec but does not log the + * URL or set the marker, neither being in scope there. Dedup: #893. */ /* No URL scheme validation — any non-empty string is passed through @@ -806,8 +815,13 @@ static boolean sys_valueproc(short token, hdltreenode hparam1, if (!use_agent_browser && !allow_gui_browser) { boolean flmarked = setpendingbrowserurl (url); - log_info (LOG_COMP_GENERAL, - "Frontier first-run: complete setup at %s", url); + /* Log only after the marker write succeeded: the log line is + * the operator's instruction to go complete setup, and it must + * not claim the onboarding step was reached when the marker + * flows and tests rely on was never recorded. */ + if (flmarked) + log_info (LOG_COMP_GENERAL, + "Frontier first-run: complete setup at %s", url); unlockhandle (hurl); disposehandle (hurl); diff --git a/tests/integration/test_cases/sys_processor_tests.yaml b/tests/integration/test_cases/sys_processor_tests.yaml index bfb928004..1dad33ce0 100644 --- a/tests/integration/test_cases/sys_processor_tests.yaml +++ b/tests/integration/test_cases/sys_processor_tests.yaml @@ -424,6 +424,19 @@ tests: expected_success: true expected_result: "http://example.com/second" + # A table pre-created at the marker path does NOT force the assign-failure + # branch in this build: hashassign's protect block is gated on + # fllanghashassignprotect, which headless_threadglobals.c:60 initializes to + # false (and tableverbs.c:288/294 leaves its only toggles commented out). The + # assign therefore succeeds and overwrites the table, verified directly. So + # this asserts the observable behavior rather than a failure that cannot occur; + # the cleanup correctness of that branch is covered by inspection, not a test. + - name: "sys.openUrl - marker overwrites a pre-existing table at its path" + description: "hashassign protection is off in headless, so the marker write wins" + script: 'if not defined(system.temp.Frontier) {new(tableType, @system.temp.Frontier)};if defined(system.temp.Frontier.pendingBrowserUrl) {delete(@system.temp.Frontier.pendingBrowserUrl)};new(tableType, @system.temp.Frontier.pendingBrowserUrl);sys.openUrl("http://example.com/overwrite");system.temp.Frontier.pendingBrowserUrl' + expected_success: true + expected_result: "http://example.com/overwrite" + # Sequential tests share one session (#852), so an earlier openUrl case may have # left a marker behind. Clear it first: this asserts a rejected URL does not # WRITE a marker, which is independent of test ordering. diff --git a/tests/openurl_headless_guard_test.sh b/tests/openurl_headless_guard_test.sh index 91a66a33a..c5878a232 100755 --- a/tests/openurl_headless_guard_test.sh +++ b/tests/openurl_headless_guard_test.sh @@ -21,14 +21,14 @@ SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" PROJECT_ROOT="$(cd "$SCRIPT_DIR/.." && pwd)" CLI="$PROJECT_ROOT/frontier-cli/frontier-cli" -DB="$PROJECT_ROOT/databases/Frontier.root" +SOURCE_DB="$PROJECT_ROOT/databases/Frontier.root" if [ ! -x "$CLI" ]; then echo "Error: frontier-cli not found at $CLI" >&2 exit 1 fi -if [ ! -f "$DB" ]; then - echo "Error: database not found at $DB" >&2 +if [ ! -f "$SOURCE_DB" ]; then + echo "Error: database not found at $SOURCE_DB" >&2 exit 1 fi @@ -41,15 +41,23 @@ FAILED=0 TESTURL="http://127.0.0.1:5336/setupFrontier" -# Scratch dir holding the PATH stubs and their recording file. +# Scratch dir holding the PATH stubs, their recording file, and a private copy +# of the database. The CLI opens the system root read-write, so these runs must +# never point at the shared working DB in databases/. STUBDIR=$(mktemp -d "${TMPDIR:-/tmp}/openurl_guard.XXXXXX") RECORD="$STUBDIR/exec_record.txt" +DB="$STUBDIR/Frontier.root" cleanup() { rm -rf "$STUBDIR" } trap cleanup EXIT +cp "$SOURCE_DB" "$DB" || { + echo "Error: could not copy $SOURCE_DB to the scratch dir" >&2 + exit 1 +} + # Stub launchers. Named exactly as the ones execlp() resolves through PATH, so # a stub firing proves an exec happened and the real browser is never invoked. for stub in open xdg-open agent-browser; do From 3806a94f0fd83d41a7061f231bfb781f0a8eb547 Mon Sep 17 00:00:00 2001 From: Jake Savin Date: Sat, 15 Aug 2026 23:44:35 -0700 Subject: [PATCH 3/3] docs(cli): #891 -- correct the P1-2 comment; the protect-block path is not reachable headless The comment claimed the assign-failure branch was reachable from script by pre-creating the marker path as a table, so hashassign would refuse a non-external value over an external one. That refusal does not happen in this build: it is gated on fllanghashassignprotect, which headless_threadglobals.c:60 initializes false and whose only toggles (tableverbs.c:288/294) are commented out. Verified live -- the assign succeeds and overwrites the table. The real headless trigger for that branch is allocation failure inside hashassign (newhashnode). Comment corrected to say so, and to point at the integration case covering the observed overwrite behavior. Comment-only; no behavior change. Guard test 15/15; unit failure set = {#828}. --- tests/headless_sys_verbs.c | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/headless_sys_verbs.c b/tests/headless_sys_verbs.c index ff4569e32..1a43b503d 100644 --- a/tests/headless_sys_verbs.c +++ b/tests/headless_sys_verbs.c @@ -125,9 +125,16 @@ static boolean setpendingbrowserurl (const char *url) { if (!hashtableassign (htfrontier, bsurl, val)) { /* val is still on the tmp stack; disposetmpvalue removes it from the * stack AND disposes the handle. A bare disposehandle would leave a - * dangling tmp-stack entry. Reachable from script: pre-creating - * system.temp.Frontier.pendingBrowserUrl as a table makes hashassign - * refuse the non-external-over-external overwrite. */ + * dangling tmp-stack entry. + * + * The headless trigger for this branch is allocation failure inside + * hashassign (newhashnode). The protect-block refusal path + * (non-external value over an external one) does NOT apply here: it is + * gated on fllanghashassignprotect, which is false in this build -- + * tests/headless_threadglobals.c:60 initializes it false and the only + * toggles, tableverbs.c:288/294, are commented out. Verified live: with + * pendingBrowserUrl pre-created as a table, the assign SUCCEEDS and + * overwrites it (covered by an integration case). */ disposetmpvalue (&val); return (false); }