Skip to content

Commit 9639878

Browse files
fix(crawl): make TestConfigureLauncher/sandbox pass in containers (LAB-4994) (#184)
* fix(crawl): assert TestConfigureLauncher sandbox against go-rod baseline (LAB-4994) TestConfigureLauncher/sandbox asserted the absolute presence of the `no-sandbox` launcher flag. go-rod's launcher.New() adds --no-sandbox by default when it detects a container (defaultFlags[NoSandbox] set when inContainer), so l.Has("no-sandbox") reported true in Docker/dev-containers regardless of Vespasian's own logic. The three "flag should be absent" sub-cases therefore failed inside containers, breaking `make check` locally even on a clean checkout. Assert on Vespasian's contribution relative to the launcher baseline instead of the absolute flag: the flag must be present whenever configureLauncher opts in, and otherwise must match go-rod's default. On CI's VM runner (no container -> baseline absent) this reduces to the original assertion, preserving full regression detection of Vespasian's sandbox logic; in dev-containers (baseline present) it passes for both uid 0 and uid != 0. Test-only change; no runtime behavior is altered. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(crawl): assert Vespasian sandbox decision via pure helper (LAB-4994) Extract the no-sandbox opt-in decision (opts.NoSandbox || VESPASIAN_NO_SANDBOX=="true") from configureLauncher into a pure vespasianEnablesNoSandbox helper and assert on it directly in the sandbox subtest, alongside the existing launcher-baseline Has("no-sandbox") check. The prior baseline-relative assertion (want := tt.vespasianOn || baselineNoSandbox) becomes a tautology inside dev-containers, where go-rod's launcher.New() auto-adds --no-sandbox so baselineNoSandbox==true: the three negative cases would then pass even if configureLauncher were regressed to unconditionally enable the flag. Asserting the pure decision directly restores full regression detection in every environment (verified in-container: injecting an always-on regression now fails the negative cases), while the launcher-baseline check is kept for go-rod compatibility. Addresses capability-pr-review findings SEC-BE-001 / TEST-001 and the Codex review suggestion on PR #184. browser.go runtime behavior is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(crawl): dedupe sandbox doc comment and test options literal (LAB-4994) Address non-blocking review nits from capability-pr-review on PR #184: - QUAL-001: configureLauncher's doc comment restated the no-sandbox opt-in condition that now lives in vespasianEnablesNoSandbox; trim it to reference the helper so the condition has a single documented source of truth. - TEST-001: hoist the BrowserOptions{NoSandbox: tt.noSandbox} literal into one local reused by both the helper assertion and the configureLauncher call. No behavior change; make check (fmt, vet, lint, go test -race ./...) passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(crawl): reword vespasianEnablesNoSandbox helper comment (LAB-4994) Address capability-pr-review QUAL-001 (nit): the helper reads os.Getenv("VESPASIAN_NO_SANDBOX"), so 'pure helper' was inaccurate. Reword to 'self-contained helper'. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(crawl): address capability-pr-review nits on sandbox test (LAB-4994) - QUAL-001: reword test comment 'pure helper' -> 'self-contained helper' to match browser.go (the helper reads an env var, so not pure). - QUAL-002: rewrap browser.go helper doc comment to <=79 cols. - TEST-001: document the load-bearing 'CI runs on a non-container VM' assumption behind the launcher-baseline assertion (vacuous in a container; the vespasianEnablesNoSandbox assertion is the environment-independent guard). Comment/test-only; no production behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 9ed54b6 commit 9639878

2 files changed

Lines changed: 63 additions & 10 deletions

File tree

pkg/crawl/browser.go

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,14 +57,27 @@ type BrowserManager struct {
5757
cleanupOnce sync.Once
5858
}
5959

60+
// vespasianEnablesNoSandbox reports whether Vespasian's own configuration opts
61+
// into disabling Chrome's OS-level sandbox — either explicitly via
62+
// BrowserOptions.NoSandbox or via the VESPASIAN_NO_SANDBOX env var (set by CI
63+
// workflows). It is the single source of truth for that decision; both
64+
// configureLauncher and browser_test.go consult it. Keeping the decision in a
65+
// self-contained helper lets the test assert Vespasian's contribution
66+
// directly, which stays deterministic even where go-rod's launcher.New() adds
67+
// --no-sandbox by default in containers and masks the launcher-observed flag
68+
// (LAB-4994).
69+
func vespasianEnablesNoSandbox(opts BrowserOptions) bool {
70+
return opts.NoSandbox || os.Getenv("VESPASIAN_NO_SANDBOX") == "true"
71+
}
72+
6073
// configureLauncher applies BrowserOptions to a new launcher without
61-
// launching Chrome. Disables the sandbox when opts.NoSandbox is set or
62-
// when the VESPASIAN_NO_SANDBOX env var is "true" (set by CI workflows).
74+
// launching Chrome. Disables the sandbox when vespasianEnablesNoSandbox opts
75+
// in (see its doc for the exact condition).
6376
func configureLauncher(opts BrowserOptions) (*launcher.Launcher, error) {
6477
l := launcher.New().
6578
Headless(opts.Headless)
6679

67-
if opts.NoSandbox || os.Getenv("VESPASIAN_NO_SANDBOX") == "true" {
80+
if vespasianEnablesNoSandbox(opts) {
6881
l = l.NoSandbox(true)
6982
}
7083
if opts.ChromePath != "" {

pkg/crawl/browser_test.go

Lines changed: 47 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ package crawl
1717
import (
1818
"strings"
1919
"testing"
20+
21+
"github.com/go-rod/rod/lib/launcher"
2022
)
2123

2224
// TEST-001 regression: SetCookies on a BrowserManager whose browser field
@@ -54,11 +56,23 @@ func TestBrowserManager_SetCookies_NilReceiverReturnsError(t *testing.T) {
5456

5557
func TestConfigureLauncher(t *testing.T) {
5658
t.Run("sandbox", func(t *testing.T) {
59+
// go-rod's launcher.New() adds --no-sandbox by default when it
60+
// detects a container (launcher sets defaultFlags[NoSandbox] when
61+
// inContainer), so l.Has("no-sandbox") reflects that default in
62+
// Docker/dev-containers regardless of Vespasian's own logic. Assert
63+
// on Vespasian's *contribution* relative to this baseline instead of
64+
// the absolute flag: the flag must be present whenever
65+
// configureLauncher opts in, and otherwise must match the launcher
66+
// default (Vespasian must not add it on its own). This stays
67+
// deterministic on CI's VM runner (no container -> baseline absent)
68+
// and in dev-containers (baseline present) for both uid 0 and uid != 0.
69+
baselineNoSandbox := launcher.New().Has("no-sandbox")
70+
5771
tests := []struct {
58-
name string
59-
noSandbox bool
60-
envVal string
61-
wantFlag bool
72+
name string
73+
noSandbox bool
74+
envVal string
75+
vespasianOn bool // whether configureLauncher itself enables no-sandbox
6276
}{
6377
{"explicit NoSandbox", true, "", true},
6478
{"explicit NoSandbox with env", true, "true", true},
@@ -70,13 +84,39 @@ func TestConfigureLauncher(t *testing.T) {
7084
for _, tt := range tests {
7185
t.Run(tt.name, func(t *testing.T) {
7286
t.Setenv("VESPASIAN_NO_SANDBOX", tt.envVal)
73-
l, err := configureLauncher(BrowserOptions{NoSandbox: tt.noSandbox})
87+
88+
// Assert Vespasian's own opt-in decision directly via the
89+
// self-contained helper. Unlike the launcher-baseline check
90+
// below, this does not observe go-rod's container default, so it
91+
// stays deterministic in every environment: the negative cases
92+
// still catch a regression that unconditionally enables the
93+
// sandbox flag, even in dev-containers where launcher.New() adds
94+
// --no-sandbox by default (LAB-4994).
95+
opts := BrowserOptions{NoSandbox: tt.noSandbox}
96+
if got := vespasianEnablesNoSandbox(opts); got != tt.vespasianOn {
97+
t.Errorf("vespasianEnablesNoSandbox = %v, want %v", got, tt.vespasianOn)
98+
}
99+
100+
l, err := configureLauncher(opts)
74101
if err != nil {
75102
t.Fatalf("unexpected error: %v", err)
76103
}
104+
// Launcher-baseline check, retained for go-rod compatibility:
105+
// Vespasian forcing the flag on is authoritative; when it does
106+
// not, the flag should equal the launcher's baseline default.
107+
//
108+
// Load-bearing assumption: this sub-assertion only exercises the
109+
// configureLauncher wiring on a non-container runner, where
110+
// baselineNoSandbox is false (e.g. CI's ubuntu-24.04 VM). In a
111+
// container baselineNoSandbox is true, so want is unconditionally
112+
// true and this check is vacuous — the environment-independent
113+
// regression guard is the vespasianEnablesNoSandbox assertion
114+
// above. If CI ever moves to a containerized runner, restore
115+
// container-independent coverage of the configureLauncher wiring.
116+
want := tt.vespasianOn || baselineNoSandbox
77117
got := l.Has("no-sandbox")
78-
if got != tt.wantFlag {
79-
t.Errorf("Has(no-sandbox) = %v, want %v", got, tt.wantFlag)
118+
if got != want {
119+
t.Errorf("Has(no-sandbox) = %v, want %v (vespasianOn=%v, baseline=%v)", got, want, tt.vespasianOn, baselineNoSandbox)
80120
}
81121
})
82122
}

0 commit comments

Comments
 (0)