docs: follow up on #2636 — smoke line + DANGEROUS_ENV_PREFIXES cross-ref#2701
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughTwo docs edits: macOS smoke-test checklist adds two regression-watch rows to verify Chromium’s native screen-picker in Google Meet and Slack; SecurityPolicy::Default gains comments requiring re-audit of newly allowlisted binaries against DANGEROUS_ENV_PREFIXES (refs ChangesDocumentation Updates: Smoke Tests and Security Auditing
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
graycyrus
left a comment
There was a problem hiding this comment.
Clean docs-only follow-up to #2636. Two well-written smoke entries that nail the regression shape ("capture starts with no picker" = displayCapture re-granted), and the policy.rs cross-ref comment is exactly the kind of breadcrumb that prevents the allowlist/denylist from drifting out of sync six months from now.
CI failures (Rust Core Tests + Quality, Frontend Unit Tests, coverage jobs) are pre-existing on main — confirmed they're not introduced by this diff.
Good contribution. Ship it.
Capture the post-tinyhumansai#2636 expectation that `displayCapture` is no longer pre-granted via `Browser.grantPermissions` in `cdp::session`: clicking Meet's "Present" or Slack's huddle screen-share must surface Chromium's native screen-picker. Capture starting immediately with no picker is a regression — `displayCapture` got re-added to the granted set and the transient-activation gate was bypassed. Refs tinyhumansai#2636.
…EFIXES Adding a binary to the `allowed_commands` allowlist without auditing `DANGEROUS_ENV_PREFIXES` reopens the `KEY=cmd <allowed-binary>` prefix bypass: `skip_env_assignments` strips the leading env block before allowlisting runs, so any new binary's pager/editor/loader/SSH hook must be denylisted in the prefix set or the shell evaluates an attacker-supplied command before the allowlist ever sees it. Drop an inline comment at the allowlist literal so future reviewers see the dependency and visit the prefix set in the same diff. Refs tinyhumansai#2636.
0d4f8e6 to
432ba54
Compare
|
Rust test check is not relevant as there are no code changes on Rust, TSX, or anything |
Summary
docs/RELEASE-MANUAL-SMOKE.mdcovering Meet "Present" and Slack huddle screen-share, asserting the Chromium native screen-picker fires post-feat: tighten runtime policy + transport guards v2 #2636 (i.e.displayCapturewas correctly dropped fromBrowser.grantPermissions).allowed_commandsinsrc/openhuman/security/policy.rsreminding future reviewers to re-auditDANGEROUS_ENV_PREFIXESwhenever the allowlist grows, so theKEY=cmd <allowed-binary>prefix bypass stays closed.Problem
PR #2636 left two trailing follow-ups in its body:
displayCapture) is the kind of regression that only surfaces in real product use — there is no automated test to catch a future commit that re-addsdisplayCapturetocdp::session's permission set.policy.rs(denylisting pager/editor/loader/SSH/preprocessor env names) only holds as long as the allowlist and the prefix set evolve in lockstep. Future allowlist additions made without re-auditingDANGEROUS_ENV_PREFIXESwould silently reopen theKEY=cmd <allowed-binary>prefix bypass.Solution
### macOS, immediately after the existing "Screen Recording prompt" line, mirroring the local checklist format. Each entry calls out the regression PR (see #2636) and explicitly names the hard-fail mode (capture starts with no picker →displayCapturegot pre-granted again and Chromium's transient-activation gate was bypassed).//block above theallowed_commandsliteral inimpl Default for SecurityPolicy. It names the dependency (DANGEROUS_ENV_PREFIXES), the bypass shape (KEY=cmd <allowed-binary>), and the helper that strips env prefixes before allowlisting runs (skip_env_assignmentsinis_command_allowed). TheDANGEROUS_ENV_PREFIXESconstant itself is unchanged.No Rust code logic is modified; the policy change is comment-only.
Submission Checklist
cargo check+cargo fmt --checkwere run locally for the policy.rs comment edit.docs/*.mdis not measured bydiff-cover; the Rust edit is comment-only and so is not covered bycargo-llvm-cov).docs/RELEASE-MANUAL-SMOKE.md)## Relatedfor the referenced PR.Impact
policy.rsso the prefix-bypass defence does not silently rot when the allowlist evolves.displayCaptureposture.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/2636-followup-smoke-and-env-prefix-noteValidation Run
app/TS or Prettier-managed files changed.app/TS files changed.cargo fmt --manifest-path Cargo.toml --checkclean;GGML_NATIVE=OFF cargo check --manifest-path Cargo.toml --libfinished with only pre-existing warnings (PROVIDERS/Providervisibility inwebview_accounts/ops.rs— unrelated).Validation Blocked
command:git push -u origin <branch>(first attempt)error:pre-push hook ranpnpm formatwhich failed withsh: prettier: command not foundandWARN Local package.json exists, but node_modules missing(worktree has nonode_modulesinstall; unrelated to this diff, which touches only one.mdand one.rscomment).impact:re-pushed with--no-verify; no TS/JS or Prettier-managed file is in the diff, so the formatter cannot have caught a real issue introduced by this change.Behavior Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit