feat: add CAPTCHA-gated shared remote shell demo - #35
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (41)
📝 WalkthroughWalkthroughAdds a hardened remote shell demo with a native sandbox, anonymous admission leases, terminal quotas, secure WebSocket protocols, Pages integration, browser coverage, and CI deployment checks. ChangesRemote terminal controls
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/fixtures/browser/pages-demo-fixture.ts (1)
28-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winName the missing directory in the ENOENT message.
The fixture now resolves a configurable directory. The error text still names only
npm run pages:check.tests/browser/pages-remote-demo.spec.tsline 21 requests_site-enabled, whichnpm run pages:checkdoes not build. A contributor who hits this failure runs the suggested command and sees the same failure again.🐛 Proposed fix
+ const siteDirectory = options.siteDirectory ?? '_site'; let siteRoot: string; try { - siteRoot = await realpath( - resolve(process.cwd(), options.siteDirectory ?? '_site'), - ); + siteRoot = await realpath(resolve(process.cwd(), siteDirectory)); } catch (error) { if (error instanceof Error && 'code' in error && error.code === 'ENOENT') { throw new Error( - 'Pages artifact is missing; run `npm run pages:check` before browser tests', + `Pages artifact ${siteDirectory} is missing; run \`npm run ${ + siteDirectory === '_site' ? 'pages:check' : 'pages:check:enabled' + }\` before browser tests`, { cause: error }, ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/fixtures/browser/pages-demo-fixture.ts` around lines 28 - 41, Update the ENOENT error construction in the fixture’s realpath resolution to include the configured site directory, including values such as options.siteDirectory or the default _site, and adjust the remediation text so it does not misleadingly imply npm run pages:check builds every directory.
🧹 Nitpick comments (16)
render.yaml (1)
1-21: 🗄️ Data Integrity & Integration | 🔵 TrivialConsider raising
maxShutdownDelaySecondsfor session drain time.Render sends SIGTERM, then SIGKILL after the shutdown delay. The default shutdown delay is 30 seconds. The public gateway allows sessions up to 60 seconds, and the deployment README requires a sandbox self-test to prove cleanup before granting a new admission. A redeploy during an active session can hit SIGKILL before the session drains and the self-test completes, skipping the documented graceful-cleanup guarantee.
Set
maxShutdownDelaySecondsto a value that comfortably covers the maximum session length plus self-test time (up to 300 seconds is allowed).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@render.yaml` around lines 1 - 21, The Render web service configuration should allow enough time for active sessions to drain before forced termination. Add maxShutdownDelaySeconds to the service definition, setting it to a value up to the 300-second platform limit that comfortably covers the 60-second session maximum and sandbox self-test.tests/browser/pages-remote-demo.spec.ts (1)
351-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the article into the case data.
The test title picks the article with a ternary on
invalidAdmission.name. A new case requires an edit to the ternary. Store the article with each case.♻️ Proposed change
for (const invalidAdmission of [ { name: 'expired', + article: 'an', value: { expiresAt: new Date(0).toISOString() }, message: 'expired before use', }, { name: 'malformed', + article: 'a', value: { token: 'short' }, message: 'failed validation', }, ] as const) { - test(`rejects ${invalidAdmission.name === 'expired' ? 'an' : 'a'} ${invalidAdmission.name} admission response before WebSocket use`, async ({ + test(`rejects ${invalidAdmission.article} ${invalidAdmission.name} admission response before WebSocket use`, async ({🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/browser/pages-remote-demo.spec.ts` around lines 351 - 363, Update the invalidAdmission case data to include the appropriate article for each case, then use that case field in the test title instead of branching on invalidAdmission.name. Keep the existing case names and rejection behavior unchanged.demo/remote/app.ts (1)
70-96: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueBound the per-attempt timeout by the wake deadline.
Each attempt allows 30 seconds. The loop checks the deadline only before an attempt starts. An attempt that starts at 89 seconds can run until about 119 seconds. The final error then reports 90 seconds, which does not match the observed wait.
♻️ Proposed change
- const response = await fetch(new URL('/health/ready', origin), { + const remainingMs = deadline - Date.now(); + const response = await fetch(new URL('/health/ready', origin), { cache: 'no-store', credentials: 'omit', mode: 'cors', redirect: 'error', referrerPolicy: 'no-referrer', - signal: AbortSignal.timeout(30_000), + signal: AbortSignal.timeout(Math.min(30_000, Math.max(1, remainingMs))), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@demo/remote/app.ts` around lines 70 - 96, Update wakeService so each fetch attempt’s timeout is capped by the remaining wake deadline, preventing an attempt from extending beyond the overall deadline. Preserve the existing 30-second maximum per attempt and ensure the final timeout behavior remains consistent with the actual elapsed wait.scripts/check-pages.js (1)
72-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the forbidden-token lists for the two bundles.
The local bundle list rejects
child_processin addition tonode:child_process. The remote bundle list omits the barechild_processentry. The remote bundle is the artifact that reaches a real backend, so it should carry at least the same restrictions as the local bundle.♻️ Proposed change
+const forbiddenBundleTokens = [ + 'node:child_process', + 'child_process', + 'node-pty', + 'WebSocketServer', +]; + const bundle = await readFile(resolve(siteRoot, 'assets/demo.js'), 'utf8'); -for (const forbidden of [ - 'node:child_process', - 'node-pty', - 'child_process', - 'WebSocketServer', -]) { +for (const forbidden of forbiddenBundleTokens) { assert( !bundle.includes(forbidden), `browser bundle must not contain ${forbidden}`, ); }-for (const forbidden of [ - 'node:child_process', - 'node-pty', - 'WebSocketServer', - 'localStorage', - 'sessionStorage', -]) { +for (const forbidden of [ + ...forbiddenBundleTokens, + 'localStorage', + 'sessionStorage', +]) { assert( !remoteBundle.includes(forbidden), `remote browser bundle must not contain ${forbidden}`, ); }Also applies to: 124-143
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-pages.js` around lines 72 - 83, Update the forbidden-token list used for the remote bundle validation to include the bare `child_process` token, matching the local bundle list. Keep the existing remote restrictions and assertions unchanged so both bundle checks reject the same backend-sensitive content.scripts/build-pages.js (1)
102-108: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake
replaceRequiredsafe against$patterns and reuse it for the local page.
source.replace(placeholder, value)treats$sequences invalueas replacement patterns. The current values cannot contain$, becausebuildRevisionmatches/^[0-9a-f]{40}$/uor islocal, and the origin values come from a validated HTTPS origin. A replacement function removes the hazard for future values.Line 27 still renders the local page with a plain
.replace, so a missing__LIT_SHELL_BUILD__placeholder there is caught only later byscripts/check-pages.js. Use the same helper for both pages.♻️ Proposed change
function replaceRequired(source, placeholder, value) { - const rendered = source.replace(placeholder, value); + const rendered = source.replace(placeholder, () => value); if (rendered === source) { throw new Error(`Pages template is missing placeholder ${placeholder}`); } return rendered; }Apply the helper to the local page as well:
-const renderedHtml = sourceHtml.replace('__LIT_SHELL_BUILD__', buildRevision); +const renderedHtml = replaceRequired( + sourceHtml, + '__LIT_SHELL_BUILD__', + buildRevision, +);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/build-pages.js` around lines 102 - 108, Update replaceRequired to pass value through a replacement function so literal dollar signs are not interpreted as replacement patterns, then replace the local page’s direct .replace call with replaceRequired using the same __LIT_SHELL_BUILD__ placeholder validation.deploy/remote-shell/sandbox-launcher.c (4)
56-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the prefix length instead of hardcoding 18.
Line 58 passes a literal length of
18for"sandbox launcher: ". The value is correct today. If the prefix text changes, the length becomes wrong andwrite_all_best_effortreads past the literal in a setuid-root binary.
sizeofon the literal is computed at compile time, so there is no runtime cost.♻️ Proposed change to derive the length
static void fail(const char *message) { const int saved_errno = errno; - write_all_best_effort(STDERR_FILENO, "sandbox launcher: ", 18); + static const char prefix[] = "sandbox launcher: "; + write_all_best_effort(STDERR_FILENO, prefix, sizeof(prefix) - 1);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/remote-shell/sandbox-launcher.c` around lines 56 - 67, Update the prefix write in fail to derive the length of the "sandbox launcher: " string from the string literal using a compile-time sizeof-based expression, removing the hardcoded 18 while preserving the existing output.
549-566: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMark
fail,run_self_test, andrun_shellasnoreturn.The child branch at lines 553-564 has no explicit exit. It is correct today because
run_shellends infail, andfailcalls_exit(126). The compiler cannot verify that invariant. If a future edit adds a path whererun_shellreturns, the child falls through to line 566 and both the parent and the child runsupervise.
_Noreturnlets the compiler enforce the invariant and warn on the fallthrough.♻️ Proposed change to add noreturn attributes
-static void fail(const char *message) { +_Noreturn static void fail(const char *message) {-static void fail_unresolved_seccomp_syscall(const char *name) { +_Noreturn static void fail_unresolved_seccomp_syscall(const char *name) {-static void run_self_test(void) { +_Noreturn static void run_self_test(void) {-static void run_shell(void) { +_Noreturn static void run_shell(void) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/remote-shell/sandbox-launcher.c` around lines 549 - 566, Mark the declarations or definitions of fail, run_self_test, and run_shell with the C noreturn attribute so the compiler knows these functions never return. Preserve the child branch in the fork flow and ensure the compiler can diagnose any future fallthrough into the parent’s supervise path.
168-243: 🔒 Security & Privacy | 🔵 Trivial | 🏗️ Heavy liftConsider an allowlist default for the seccomp policy.
The filter uses
SCMP_ACT_ALLOWas the default action with an enumerated denylist. A denylist permits every syscall that is not named, including syscalls added by future kernels. An allowlist default ofSCMP_ACT_ERRNO(EPERM)with the small set that/bin/shand the listed demo commands need would close that gap.The unresolved-name hard fail at lines 205-208 is a good control and prevents a typo from weakening the policy silently. The surrounding chroot, uid drop, no-new-privs, capability drop, and rlimits also limit the impact. Treat this as a posture improvement rather than an active exploit.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/remote-shell/sandbox-launcher.c` around lines 168 - 243, Update install_seccomp_filter to use SCMP_ACT_ERRNO(EPERM) as the default action and replace the denied_syscalls denylist with an explicit allowlist containing only the syscalls required by /bin/sh and the supported demo commands. Preserve the existing unresolved-name failure behavior and clone namespace restrictions, ensuring every allowlisted syscall is resolved and added before loading the policy.
397-448: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExtend the self-test to cover the namespace and ptrace denials.
The self-test proves identity, chroot, read-only filesystem, network denial, SysV IPC denial, and every rlimit. The
setuid(0)andseteuid(0)calls at line 408 are negative assertions that must fail, so the static analysis privilege-escalation hint is a false positive.The filter's highest-value entries are not asserted:
ptrace,unshare, andclonewith a namespace flag. Theclonerule in particular depends on the flags argument being argument 0. A self-test assertion would catch a mismatch at build time rather than in production.🛡️ Proposed additional assertions in `verify_kernel_isolation`
+ errno = 0; + if (unshare(CLONE_NEWNS) == 0 || errno != EPERM) { + errno = EPERM; + fail("namespace isolation self-test failed"); + } + + errno = 0; + if (ptrace(PTRACE_TRACEME, 0, NULL, NULL) == 0 || errno != EPERM) { + errno = EPERM; + fail("ptrace isolation self-test failed"); + } + `#ifdef` SYS_clone3Add
#include <sys/ptrace.h>for theptraceassertion.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/remote-shell/sandbox-launcher.c` around lines 397 - 448, Extend run_self_test through verify_kernel_isolation to assert denials for ptrace, unshare, and clone when argument 0 contains a namespace flag, ensuring the clone check validates the filter’s flags-argument position. Add the required sys/ptrace.h include and make each assertion fail the self-test if the operation is permitted, while preserving the existing isolation checks.Source: Linters/SAST tools
src/ui/lit-shell-terminal.ts (1)
1145-1149: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winClear protocols on every tab client, not only the active one.
clearProtocols()clears the component field and the activethis.client. WhenshowTabsis enabled, each tab owns a separateTerminalClientintab.client. Those clients keep the configured capability in their own config.The remote demo does not enable tabs, so no current path leaks. Clearing all tab clients completes the one-use capability control and prevents a leak if a future host enables tabs.
♻️ Proposed change to clear all tab clients
/** Forget WebSocket subprotocols after a one-use capability is consumed. */ clearProtocols(): void { this.protocols = []; this.client?.clearProtocols(); + for (const tab of this.tabs) tab.client?.clearProtocols(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/lit-shell-terminal.ts` around lines 1145 - 1149, Update clearProtocols() to clear protocols on every tab.client in addition to the component field and active client, ensuring each TerminalClient releases its one-use capability when tabs are enabled. Reuse the existing client clearProtocols behavior and preserve the current handling for the active client.tests/remote-shell/server.e2e.test.ts (2)
142-143: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed 50 ms sleep with a polled wait.
Line 142 sleeps for a fixed 50 ms, then line 143 asserts
preflightCalls > 1. The earlierwaitForStatuscall does not guarantee a retry has run, becausesandboxHealthyis set to false synchronously before the first recovery attempt. The assertion therefore depends only on the sleep.The margin over
sandboxRecoveryTimeoutMs: 20is small. A loaded runner can make this test flaky. A polled wait removes the timing dependency and also runs faster in the normal case.♻️ Proposed change to poll for the retry
- await new Promise((resolve) => setTimeout(resolve, 50)); - expect(preflightCalls).toBeGreaterThan(1); + await vi.waitFor(() => { + expect(preflightCalls).toBeGreaterThan(1); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/remote-shell/server.e2e.test.ts` around lines 142 - 143, Replace the fixed 50 ms delay before the preflightCalls assertion with a polling wait that repeatedly checks until preflightCalls is greater than 1, using the test’s existing wait helper or polling pattern and an appropriate timeout. Keep the assertion as the final verification while allowing the test to proceed immediately once the retry occurs.
362-418: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared wait helpers into a reusable module.
waitForMessageandwaitForOutputshare the same listener, timeout, and cleanup structure. Near-identical helpers also exist intests/fixtures/remote-demo-container-smoke.mjs, which defines its ownexpectUpgradeRejectedand connect logic against the same gateway protocol.A shared helper module for the remote-shell protocol waits would keep the timeout values and cleanup logic consistent as the protocol grows.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/remote-shell/server.e2e.test.ts` around lines 362 - 418, Extract the shared WebSocket wait and connection logic from waitForMessage, waitForOutput, and the analogous remote-demo smoke-test helpers into a reusable remote-shell protocol helper module. Update both tests to import and use the shared helpers, preserving message predicates, output accumulation, timeout defaults, rejection behavior, and listener cleanup; keep protocol-specific assertions such as expectUpgradeRejected layered on the shared connection logic.deploy/remote-shell/Dockerfile (1)
97-98: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winProbe
/health/livefrom the container HEALTHCHECK.
/health/readyreturns 503 while the sandbox recovers between sessions (deploy/remote-shell/server.tslines 231-238). A recovery window that spans three probe intervals marks the container unhealthy and can trigger a restart during normal operation. Use/health/livefor container liveness and keep/health/readyfor admission gating.♻️ Proposed change
HEALTHCHECK --interval=30s --timeout=5s --start-period=15s --retries=3 \ - CMD ["node", "-e", "const p=process.env.PORT||10000;fetch('http://127.0.0.1:'+p+'/health/ready').then(r=>{if(!r.ok)process.exit(1)}).catch(()=>process.exit(1))"] + CMD ["node", "-e", "const p=process.env.PORT||10000;fetch('http://127.0.0.1:'+p+'/health/live').then(r=>{if(!r.ok)process.exit(1)}).catch(()=>process.exit(1))"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/remote-shell/Dockerfile` around lines 97 - 98, Update the Dockerfile HEALTHCHECK command to probe the `/health/live` endpoint instead of `/health/ready`, while preserving the existing port selection, timing options, and failure handling.tests/remote-shell/admission.test.ts (1)
22-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the retry hint through
toThrowErrorinstead of a bare try/catch.The
catchblock holds the only assertion forretryAfterSeconds. Ifissue()stops throwing, the block never runs and the test still passes. A single matcher covers both the throw and the payload.♻️ Proposed change
- expect(() => admissions.issue()).toThrow(AdmissionUnavailableError); - try { - admissions.issue(); - } catch (error) { - expect(error).toMatchObject({ retryAfterSeconds: 30 }); - } + expect(() => admissions.issue()).toThrow(AdmissionUnavailableError); + expect(() => admissions.issue()).toThrowError( + expect.objectContaining({ retryAfterSeconds: 30 }), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/remote-shell/admission.test.ts` around lines 22 - 27, Update the admissions.issue() assertion to use a single toThrowError matcher that verifies both AdmissionUnavailableError and retryAfterSeconds: 30. Remove the separate try/catch and redundant bare throw assertion so the test fails when issue() does not throw.deploy/remote-shell/build-rootfs.sh (1)
37-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winResolve the README source relative to the script.
Line 37 hardcodes
/build/deploy/remote-shell/rootfs/README.txt, which only exists inside the Docker build stage.deploy/remote-shell/build-launcher.shderives its source directory from$0. Use the same approach so the script also runs locally and during test builds.♻️ Proposed change
+source_root=$(CDPATH='' cd -- "$(dirname -- "$0")" && pwd) + -install -m 0444 /build/deploy/remote-shell/rootfs/README.txt "$rootfs/README.txt" +install -m 0444 "$source_root/rootfs/README.txt" "$rootfs/README.txt"Apply the assignment near the top of the script, next to the
rootfsvalidation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/remote-shell/build-rootfs.sh` around lines 37 - 40, Update build-rootfs.sh to derive the README source directory from the script path, matching build-launcher.sh’s $0-based approach. Define the resolved source path near the rootfs validation, then use it instead of the hardcoded /build/deploy/remote-shell/rootfs/README.txt path in the README install command.deploy/remote-shell/build-launcher.sh (1)
26-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReport the failure reason for the position-independent check.
Line 26 exits silently through
set -ewhen the ELF type is notDYN. The interpreter check below prints a diagnostic. Align both checks so build failures are self-explanatory.♻️ Proposed change
-readelf --file-header "$output" | grep --quiet 'Type:.*DYN' +if ! readelf --file-header "$output" | grep --quiet 'Type:.*DYN'; then + echo 'sandbox launcher must be a position-independent executable' >&2 + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/remote-shell/build-launcher.sh` around lines 26 - 31, Update the ELF type validation in build-launcher.sh so a non-DYN result from readelf --file-header is handled explicitly and prints a clear failure diagnostic before exiting, matching the interpreter check’s behavior. Preserve the existing successful path and dynamic-interpreter validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/pages.yml:
- Around line 113-129: Replace the gawk-dependent header parsing in the redirect
verification at .github/workflows/pages.yml lines 113-129 and 186-204 with a
single curl call using --write-out '%{http_code} %{redirect_url}', then split
the result into redirect_status and redirect_location; preserve the existing
validation in the first site and the PAGE_URL="$redirect_location" reassignment
in the HTTP case branch.
In @.github/workflows/release.yml:
- Around line 43-46: Update the check-run selection pipeline in the release
workflow to sort matching runs by their monotonically increasing .id field
before selecting the last run, replacing the current .started_at ordering. Keep
the existing pending fallback and other filtering unchanged.
In `@demo/remote/app.ts`:
- Around line 98-117: Update requestAdmission to pass an
AbortSignal.timeout-based signal to its fetch request, matching the timeout
pattern used by wakeService. Preserve the existing admission response and error
handling while ensuring the promise rejects when the backend does not respond.
- Around line 253-261: In the terminal startup flow, add an immediate liveness
check after both awaited calls, terminal.connect() and terminal.spawn(...),
using the existing sessionEnded/sessionFailed state used by the session cleanup
handlers. Return or otherwise stop startup when either check indicates the
session is no longer active, so endButton, startCountdown, and the connected
status update only run for a live terminal.
In `@deploy/remote-shell/build-rootfs.sh`:
- Around line 27-36: Update the rootfs assembly loop to retain the resolved path
for the dash command, then create the /bin/sh symlink from that path rather than
hard-coding ../usr/bin/dash. Ensure the target is converted to the correct
relative path under rootfs so both /bin/dash and /usr/bin/dash layouts produce a
valid symlink.
- Around line 14-15: Add a usable /dev/null device to the rootfs during the
build flow around install and enter_sandbox, ensuring it exists inside the
chroot before dash runs and supports shell/command redirections. Prefer
including the device node in the image; otherwise create or mount it as part of
sandbox setup before launching commands.
In `@deploy/remote-shell/config.ts`:
- Line 6: Replace the zero value of maxConnectionMessages in the remote-shell
configuration with a finite positive message limit, consistent with the other
connection quotas and preserving the server’s existing option forwarding.
In `@deploy/remote-shell/sandbox-launcher.c`:
- Around line 216-237: Constrain the sandbox builder around the clone namespace
filtering logic to architectures where clone’s flags are passed in SCMP_A0,
since s390x and cris place the arguments differently. Add an explicit
supported-architecture build constraint or compile-time/manifest assertion,
using the existing clone_syscall and namespace_flags setup, and ensure
unsupported architectures cannot produce or run this builder.
In `@deploy/remote-shell/server.ts`:
- Around line 98-111: Update the rejection handler in
releaseAfterSandboxRecovery so a failed waitForSandboxRecovery does not leave
the process permanently unready: after logging the failure, terminate the
process to allow the platform to replace the instance, rather than returning
with sandboxHealthy set to false.
In `@tests/browser/pages-remote-demo.spec.ts`:
- Around line 174-175: In the no-reconnect assertion in
tests/browser/pages-remote-demo.spec.ts, update the page.waitForTimeout call
before expect(routedWebSocketUrls).toHaveLength(1) from 250 ms to 1250 ms,
matching the existing waits in the related tests and allowing the default 1000
ms reconnect attempt to occur.
In `@tests/e2e/server-owned-spawn-policy.e2e.test.ts`:
- Around line 47-89: Extend the try/finally scope in the test case around the
complete server connection, spawn, output capture, and environment assertions so
process.env[secretName] remains set while the PTY is inspected. Restore or
delete the secret only after the assertions complete, while preserving the
existing cleanup behavior for both previously defined and undefined values.
In `@tests/fixtures/remote-demo-container-smoke.mjs`:
- Around line 223-292: Apply the existing waitUntil-style timeout pattern to the
socket event waits in expectUpgradeRejected, connect, and the returned close
method: bound unexpected-response, open, and close waits with a clear timeout so
failures reject promptly. Preserve the current event handling and assertions,
but stop discarding socket errors by allowing relevant error failures to surface
rather than hanging silently.
In `@tests/pages-remote-config.test.js`:
- Around line 25-39: Add 'https://demo.example.test/' to the unsafe candidate
list in the pagesRemoteConfig rejection tests, keeping the existing TypeError
assertion so the trailing-slash origin behavior remains covered.
In `@tests/remote-shell/server.e2e.test.ts`:
- Around line 251-261: Update the replacement admission assertion after
expectUpgradeRejected to poll with waitForStatus around requestAdmission until
the gateway releases the lease and returns the expected successful status.
Preserve the existing allowedOrigin request parameters and assert the eventual
201 response.
---
Outside diff comments:
In `@tests/fixtures/browser/pages-demo-fixture.ts`:
- Around line 28-41: Update the ENOENT error construction in the fixture’s
realpath resolution to include the configured site directory, including values
such as options.siteDirectory or the default _site, and adjust the remediation
text so it does not misleadingly imply npm run pages:check builds every
directory.
---
Nitpick comments:
In `@demo/remote/app.ts`:
- Around line 70-96: Update wakeService so each fetch attempt’s timeout is
capped by the remaining wake deadline, preventing an attempt from extending
beyond the overall deadline. Preserve the existing 30-second maximum per attempt
and ensure the final timeout behavior remains consistent with the actual elapsed
wait.
In `@deploy/remote-shell/build-launcher.sh`:
- Around line 26-31: Update the ELF type validation in build-launcher.sh so a
non-DYN result from readelf --file-header is handled explicitly and prints a
clear failure diagnostic before exiting, matching the interpreter check’s
behavior. Preserve the existing successful path and dynamic-interpreter
validation.
In `@deploy/remote-shell/build-rootfs.sh`:
- Around line 37-40: Update build-rootfs.sh to derive the README source
directory from the script path, matching build-launcher.sh’s $0-based approach.
Define the resolved source path near the rootfs validation, then use it instead
of the hardcoded /build/deploy/remote-shell/rootfs/README.txt path in the README
install command.
In `@deploy/remote-shell/Dockerfile`:
- Around line 97-98: Update the Dockerfile HEALTHCHECK command to probe the
`/health/live` endpoint instead of `/health/ready`, while preserving the
existing port selection, timing options, and failure handling.
In `@deploy/remote-shell/sandbox-launcher.c`:
- Around line 56-67: Update the prefix write in fail to derive the length of the
"sandbox launcher: " string from the string literal using a compile-time
sizeof-based expression, removing the hardcoded 18 while preserving the existing
output.
- Around line 549-566: Mark the declarations or definitions of fail,
run_self_test, and run_shell with the C noreturn attribute so the compiler knows
these functions never return. Preserve the child branch in the fork flow and
ensure the compiler can diagnose any future fallthrough into the parent’s
supervise path.
- Around line 168-243: Update install_seccomp_filter to use
SCMP_ACT_ERRNO(EPERM) as the default action and replace the denied_syscalls
denylist with an explicit allowlist containing only the syscalls required by
/bin/sh and the supported demo commands. Preserve the existing unresolved-name
failure behavior and clone namespace restrictions, ensuring every allowlisted
syscall is resolved and added before loading the policy.
- Around line 397-448: Extend run_self_test through verify_kernel_isolation to
assert denials for ptrace, unshare, and clone when argument 0 contains a
namespace flag, ensuring the clone check validates the filter’s flags-argument
position. Add the required sys/ptrace.h include and make each assertion fail the
self-test if the operation is permitted, while preserving the existing isolation
checks.
In `@render.yaml`:
- Around line 1-21: The Render web service configuration should allow enough
time for active sessions to drain before forced termination. Add
maxShutdownDelaySeconds to the service definition, setting it to a value up to
the 300-second platform limit that comfortably covers the 60-second session
maximum and sandbox self-test.
In `@scripts/build-pages.js`:
- Around line 102-108: Update replaceRequired to pass value through a
replacement function so literal dollar signs are not interpreted as replacement
patterns, then replace the local page’s direct .replace call with
replaceRequired using the same __LIT_SHELL_BUILD__ placeholder validation.
In `@scripts/check-pages.js`:
- Around line 72-83: Update the forbidden-token list used for the remote bundle
validation to include the bare `child_process` token, matching the local bundle
list. Keep the existing remote restrictions and assertions unchanged so both
bundle checks reject the same backend-sensitive content.
In `@src/ui/lit-shell-terminal.ts`:
- Around line 1145-1149: Update clearProtocols() to clear protocols on every
tab.client in addition to the component field and active client, ensuring each
TerminalClient releases its one-use capability when tabs are enabled. Reuse the
existing client clearProtocols behavior and preserve the current handling for
the active client.
In `@tests/browser/pages-remote-demo.spec.ts`:
- Around line 351-363: Update the invalidAdmission case data to include the
appropriate article for each case, then use that case field in the test title
instead of branching on invalidAdmission.name. Keep the existing case names and
rejection behavior unchanged.
In `@tests/remote-shell/admission.test.ts`:
- Around line 22-27: Update the admissions.issue() assertion to use a single
toThrowError matcher that verifies both AdmissionUnavailableError and
retryAfterSeconds: 30. Remove the separate try/catch and redundant bare throw
assertion so the test fails when issue() does not throw.
In `@tests/remote-shell/server.e2e.test.ts`:
- Around line 142-143: Replace the fixed 50 ms delay before the preflightCalls
assertion with a polling wait that repeatedly checks until preflightCalls is
greater than 1, using the test’s existing wait helper or polling pattern and an
appropriate timeout. Keep the assertion as the final verification while allowing
the test to proceed immediately once the retry occurs.
- Around line 362-418: Extract the shared WebSocket wait and connection logic
from waitForMessage, waitForOutput, and the analogous remote-demo smoke-test
helpers into a reusable remote-shell protocol helper module. Update both tests
to import and use the shared helpers, preserving message predicates, output
accumulation, timeout defaults, rejection behavior, and listener cleanup; keep
protocol-specific assertions such as expectUpgradeRejected layered on the shared
connection logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15f354e2-0318-4e18-a5b6-7771d97582ff
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (52)
.changeset/harden-remote-demo.md.github/dependabot.yml.github/workflows/ci.yml.github/workflows/codeql.yml.github/workflows/pages.yml.github/workflows/release.yml.gitignore.quality/crap-ratchet.jsonCONTRIBUTING.mdREADME.mddemo/index.htmldemo/remote/app.tsdemo/remote/index.htmldemo/remote/style.cssdeploy/remote-shell/Dockerfiledeploy/remote-shell/README.mddeploy/remote-shell/admission.tsdeploy/remote-shell/build-launcher.shdeploy/remote-shell/build-rootfs.shdeploy/remote-shell/config.tsdeploy/remote-shell/rootfs/README.txtdeploy/remote-shell/sandbox-launcher.cdeploy/remote-shell/server.tseslint.config.mjsknip.jsonpackage.jsonrender.yamlscripts/build-pages.jsscripts/check-pages-enabled.jsscripts/check-pages.jsscripts/pages-remote-config.jssrc/client/terminal-client.tssrc/server/terminal-server.tssrc/shared/types.tssrc/ui/lit-shell-terminal.tstests/browser/lit-shell-terminal.spec.tstests/browser/pages-remote-demo.spec.tstests/e2e/connection-limits.e2e.test.tstests/e2e/docker-validation.e2e.test.tstests/e2e/server-owned-spawn-policy.e2e.test.tstests/e2e/session-resource-quotas.e2e.test.tstests/e2e/session-sharing-policy.e2e.test.tstests/e2e/validation-isolation.e2e.test.tstests/fixtures/browser/pages-demo-fixture.tstests/fixtures/remote-demo-container-smoke.mjstests/pages-remote-config.test.jstests/remote-shell/admission.test.tstests/remote-shell/config.test.tstests/remote-shell/server.e2e.test.tstests/server-config-validation.test.tstests/terminal-client.test.tstsconfig.json
lsadehaan
changed the title
![github-advanced-security[bot]](https://avatars.github.com/in/57789?s=60&v=4){kind=small}
2 participants
Summary
constrained-container coverage
container with a mandatory server-verified Cloudflare Turnstile gate
local UID/GID wiring, and one-use WebSocket admission capabilities
dependency review and audits, CodeQL, SBOMs, image scans, exact-revision
deployment, and fail-closed Pages publication
releases while retaining documented compatibility holds
Remote demo model
All visitors deliberately share one disposable container, workspace, UID/GID,
and four-session capacity. There is no per-visitor sandbox inside that
container. The image contains no credentials and must not be connected to
private services. The whole guest epoch resets on one fixed global five-minute
schedule; processes, files, writable host paths, and guest IPC resources are
cleared together.
Cloudflare Turnstile is a hard server-side gate. Siteverify must return the
expected hostname, action, and fresh challenge timestamp before the gateway
issues a short-lived, one-use WebSocket capability. Verification is bounded by
four concurrent calls and a 12-attempt global token bucket that refills once per
second. Proxy-supplied client IP headers are neither trusted nor forwarded.
The root lifecycle gateway runs with a minimal capability bounding set. Every
PTY is created as fixed UID/GID 65532 with a fixed shell, directory, environment,
and resource limits. Runtime package managers and setuid/setgid files are
removed from the final image.
CI/CD and rollout
Render auto-deploy is disabled. Only a successful
masterCI run can access theenvironment-scoped Render credential, request deployment of that exact commit,
and verify the exact live revision. Pull requests never receive the credential.
The Pages artifact remains network-disabled unless the backend origin and
public site key are configured together. Publication also fails closed unless
the exact backend revision is live and the custom domain permanently redirects
HTTP to the expected HTTPS URL.
Verification
npm run validate— 505 passing tests, one intentional skip, coverage policy,CRAP ratchet, formatting, lint, types, docs, dependency, and Pages checks
npm run package:check— build, publint, Are the Types Wrong, packed consumershared PTYs, quotas, identity/environment isolation, process/file/IPC reset,
token replay, and automatic/manual epoch behavior
persistence, limiter, client cleanup, and CI credential-boundary issues fixed
The live Pages integration intentionally remains disabled until the reviewed
commit is merged, the exact-revision Render deployment is healthy, and the
custom-domain HTTPS prerequisite passes.
Interest and feedback: #34