fix(e2e): allow host mocks through UFW - #8708
Conversation
📝 WalkthroughWalkthroughAdds a host firewall fixture for live E2E mocks. It discovers Docker bridge topology, applies and cleans up an exact UFW rule, reports remediation details, and integrates concurrent setup failure handling with the gateway upgrade test. ChangesHost mock firewall
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant UpgradeTest
participant FakeOpenAIServer
participant HostMockFirewall
participant Docker
participant UFW
participant CleanupRegistry
UpgradeTest->>FakeOpenAIServer: start fake endpoint
UpgradeTest->>HostMockFirewall: register endpoint port
HostMockFirewall->>Docker: inspect bridge network
Docker-->>HostMockFirewall: return subnet and gateway
HostMockFirewall->>UFW: apply exact rule
UFW-->>HostMockFirewall: return mutation result
UpgradeTest->>UpgradeTest: run installation and firewall setup concurrently
UpgradeTest->>FakeOpenAIServer: close server on registration failure
CleanupRegistry->>UFW: restore baseline rule state
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 6044319 in the TypeScript / code-coverage/cliThe overall coverage in commit 6044319 in the Show a code coverage summary of the most impacted files.
Updated |
senthilr-nv
added
chore
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/e2e/fixtures/host-mock-firewall.ts (1)
263-467: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftSplit the firewall lifecycle into smaller phases.
registerOpenShellHostMockFirewallcombines validation, topology discovery, UFW inspection, mutation, and cleanup coordination. The shared state flags make lifecycle ownership difficult to verify. Extract focused phases and isolate the cleanup state coordinator.As per coding guidelines, keep function complexity low.
🤖 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 `@test/e2e/fixtures/host-mock-firewall.ts` around lines 263 - 467, The registerOpenShellHostMockFirewall function is too complex and mixes validation, topology discovery, UFW inspection, mutation, and cleanup state. Split these responsibilities into focused helper functions, including a dedicated cleanup-state coordinator that owns applyInFlight, baselineRules, mutationStarted, closed, topology, and rule access; keep registerOpenShellHostMockFirewall as orchestration and preserve all existing results, errors, authorization checks, and cleanup behavior.Source: Coding guidelines
🤖 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 `@test/e2e/fixtures/host-mock-firewall.ts`:
- Around line 416-425: In the setup flow around mutationStarted and
applyInFlight, check closed immediately before invoking options.host.command to
apply the UFW rule; if cleanup has started, abort without setting
mutationStarted or issuing the mutation. Preserve the existing post-apply closed
check for interruptions that occur during the command.
In `@test/e2e/live/openshell-gateway-upgrade.test.ts`:
- Around line 1191-1201: Update the firewall setup handling around
registerOpenShellHostMockFirewall to await its returned promise inside the
existing try/catch, so asynchronous topology, authorization, and UFW failures
reach the catch. Ensure fake.close() runs before rethrowing any rejected
firewall setup, including the corresponding setup block noted by the review.
In `@test/e2e/support/host-mock-firewall.test.ts`:
- Around line 91-95: Update the test helper method containing this
call-recording and response-shifting logic to remove the if statement. Replace
the exhausted-queue check with a nullish fallback that throws the same
unexpected-command error, while preserving function-response invocation and
direct response returns.
---
Nitpick comments:
In `@test/e2e/fixtures/host-mock-firewall.ts`:
- Around line 263-467: The registerOpenShellHostMockFirewall function is too
complex and mixes validation, topology discovery, UFW inspection, mutation, and
cleanup state. Split these responsibilities into focused helper functions,
including a dedicated cleanup-state coordinator that owns applyInFlight,
baselineRules, mutationStarted, closed, topology, and rule access; keep
registerOpenShellHostMockFirewall as orchestration and preserve all existing
results, errors, authorization checks, and cleanup behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8192ea87-b18b-4e0e-bd1c-ef5aa36d2dc4
📒 Files selected for processing (3)
test/e2e/fixtures/host-mock-firewall.tstest/e2e/live/openshell-gateway-upgrade.test.tstest/e2e/support/host-mock-firewall.test.ts
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
8 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: 1 warning · 0 suggestionsWarningsWarnings do not block.
|
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Blocking security finding on commit a578b188d9:
test/e2e/fixtures/host-mock-firewall.ts:416-424 can apply the UFW rule after cleanup has completed. If cleanup starts while the baseline ufw show added command is pending, cleanup sets closed, sees mutationStarted === false, and returns. Setup then resumes without checking closed, starts the UFW mutation, and throws only after the rule was applied. No cleanup remains to delete that rule. This violates #8696's interruption requirement and can leave the host firewall more permissive after the test exits.
Check closed immediately before setting mutationStarted and invoking ufw allow. Add a regression test that pauses the baseline-rule snapshot, starts cleanup, resolves the snapshot, and proves that no UFW apply command runs.
Security review result: FAIL for error recovery, security testing, and whole-operation state safety until this race is fixed. I found no credential, input-injection, authorization, dependency, cryptography, or unrelated configuration blocker.
|
@cv The blocking cleanup-race finding from your |
2 participants
Summary
Live E2E host mocks now add one temporary UFW rule after the OpenShell Docker bridge appears. The fixture limits the rule to the detected bridge interface, subnet, gateway, and mock port, then restores the original UFW rule set during cleanup.
Related Issue
Fixes #8696
Changes
openshell-gateway-upgradelive E2E test is the current consumer, andhost-mock-firewall.test.tsprotects the contract.Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run test:changedpassed 10 tests in 1 file for the review repair.npm run test:e2e-phases:checkvalidated 125 tests across 81 files.npm run test-conditionals:scan -- --top 25passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Tests