[Test Coverage] Improve host-iptables.ts branch coverage from 83% to 99%#1691
Open
github-actions[bot] wants to merge 1 commit intomainfrom
Open
[Test Coverage] Improve host-iptables.ts branch coverage from 83% to 99%#1691github-actions[bot] wants to merge 1 commit intomainfrom
github-actions[bot] wants to merge 1 commit intomainfrom
Conversation
Add 17 new tests to host-iptables.test.ts covering previously uncovered security-critical branches: - IPv6 DNS handling: FW_WRAPPER_V6 chain creation, existing chain cleanup, ip6tables unavailable fallback, IPv6 chain cleanup error recovery - Docker bridge gateway validation: empty stdout returns null, non-IPv4 gateway returns null (prevents invalid IPs in iptables rules) - Default DNS servers: empty dnsServers array falls back to DEFAULT_DNS_SERVERS - Port validation: empty entries and invalid ports in allowHostServicePorts are silently skipped with warning - Error paths: sysctl failure when disabling/re-enabling IPv6, failed DOCKER-USER chain creation, FW_WRAPPER chain cleanup errors - DOCKER-USER line parsing: header lines containing chain name but no leading line number are correctly skipped Coverage improvements for host-iptables.ts: Statements: 91.6% → 100% Branches: 83.3% → 98.95% Functions: 100% → 100% Lines: 91.9% → 100% Overall project coverage improvements: Statements: 84.34% → 85.00% Branches: 77.43% → 78.24% Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Mossaka
approved these changes
Apr 6, 2026
Contributor
Author
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (2 files)
Coverage comparison generated by |
Contributor
Author
Smoke Test Results
Overall: PASS
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds additional Jest unit tests to increase branch/line coverage for the security-critical host firewall setup/cleanup logic in host-iptables.ts, focusing on IPv6 handling, DOCKER-USER parsing edge cases, DNS defaults, host-access gateway validation, and error paths.
Changes:
- Expanded
setupHostIptablestests to cover IPv6 sysctl failure paths, DOCKER-USER chain creation failures, and IPv6 DNS chain setup/cleanup branches. - Added
cleanupHostIptablestests for DOCKER-USER parsing edge cases, IPv6 cleanup error paths, and sysctl re-enable failure handling. - Added host-access tests for Docker bridge gateway edge cases (empty/invalid gateway) and port list parsing (empty/invalid entries).
Show a summary per file
| File | Description |
|---|---|
| src/host-iptables.test.ts | Adds new tests targeting previously uncovered branches in host iptables setup/cleanup logic (IPv6, DNS defaults, DOCKER-USER parsing, host-access gateways/ports, error paths). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (6)
src/host-iptables.test.ts:596
- This test uses
expect(promise).resolves.not.toThrow(), but.toThrow()requires a function and will fail when the promise resolves tovoid. Use.resolves.toBeUndefined()(or justawait setupHostIptables(...)) to assert the call completes without throwing.
// Should not throw - error during cleanup is caught and logged
await expect(setupHostIptables('172.30.0.10', 3128, ['8.8.8.8'])).resolves.not.toThrow();
});
src/host-iptables.test.ts:1128
expect(promise).resolves.not.toThrow()is invalid here for the same reason elsewhere:.toThrow()expects a function, not a resolvedvoidvalue. Replace with.resolves.toBeUndefined()or justawait cleanupHostIptables().
// Should not throw - header lines with no numbers are silently skipped
await expect(cleanupHostIptables()).resolves.not.toThrow();
src/host-iptables.test.ts:1170
expect(promise).resolves.not.toThrow()will fail because the promise resolves tovoidand.toThrow()requires a function. Use.resolves.toBeUndefined()or justawait cleanupHostIptables()to assert no throw.
// Should not throw - enableIpv6ViaSysctl catches sysctl errors
await expect(cleanupHostIptables()).resolves.not.toThrow();
});
src/host-iptables.test.ts:1195
- Same issue:
expect(promise).resolves.not.toThrow()is not a valid Promise assertion in Jest (resolved value isvoid). Replace with.resolves.toBeUndefined()/await cleanupHostIptables().
// Should not throw - error during IPv6 chain cleanup is caught and logged
await expect(cleanupHostIptables()).resolves.not.toThrow();
});
src/host-iptables.test.ts:1435
expect(promise).resolves.not.toThrow()is not a valid Jest assertion here (the promise resolves tovoid, but.toThrow()expects a function). Use.resolves.toBeUndefined()or justawait setupHostIptables(...)to assert success.
// Should not throw - the error during IPv6 chain cleanup is caught and logged
await expect(
setupHostIptables('172.30.0.10', 3128, ['8.8.8.8', '2001:4860:4860::8888'])
).resolves.not.toThrow();
src/host-iptables.test.ts:1428
- This test simulates
ip6tables -F FW_WRAPPER_V6throwing while the chain “exists” (v6ChainExists === 0), then expects setup to continue successfully. InsetupHostIptables,ip6tables -N FW_WRAPPER_V6is executed after the cleanup try/catch; if the flush throws before-Xruns, the chain may still exist and-Nwould typically fail. To keep the test aligned with production behavior, either simulate a cleanup failure that doesn’t imply the chain still exists, or assert the expected rejection.
if (cmd === 'ip6tables' && args[2] === '-L' && args[3] === 'FW_WRAPPER_V6') {
// Chain exists
return Promise.resolve({ exitCode: 0 } as any);
}
if (cmd === 'ip6tables' && args[2] === '-F') {
// Flush throws an actual error (not just non-zero exit)
return Promise.reject(new Error('ip6tables: internal error during flush'));
}
- Files reviewed: 1/1 changed files
- Comments generated: 2
| }) as any); | ||
|
|
||
| // Should not throw - disableIpv6ViaSysctl catches sysctl errors | ||
| await expect(setupHostIptables('172.30.0.10', 3128, ['8.8.8.8'])).resolves.not.toThrow(); |
There was a problem hiding this comment.
expect(promise).resolves.not.toThrow() is not a valid Jest assertion because .toThrow() expects a function, not the resolved value of a Promise (this Promise resolves to void). Replace with an assertion on the resolved value (e.g., .resolves.toBeUndefined()), or simply await setupHostIptables(...) with no expect if the goal is just “does not throw”.
This issue also appears in the following locations of the same file:
- line 594
- line 1126
- line 1168
- line 1193
- line 1432
Suggested change
| await expect(setupHostIptables('172.30.0.10', 3128, ['8.8.8.8'])).resolves.not.toThrow(); | |
| await setupHostIptables('172.30.0.10', 3128, ['8.8.8.8']); |
Comment on lines
+587
to
+595
| // Flush FW_WRAPPER - throws! | ||
| .mockRejectedValueOnce(new Error('iptables: Chain flush failed')); | ||
|
|
||
| // After the error in chain cleanup, the outer try-catch should handle it; | ||
| // but then creating FW_WRAPPER chain will be attempted and also succeed | ||
| mockedExeca.mockResolvedValue({ stdout: '', stderr: '', exitCode: 0 } as any); | ||
|
|
||
| // Should not throw - error during cleanup is caught and logged | ||
| await expect(setupHostIptables('172.30.0.10', 3128, ['8.8.8.8'])).resolves.not.toThrow(); |
There was a problem hiding this comment.
The mock simulates iptables -F FW_WRAPPER throwing, then asserts setup succeeds. In setupHostIptables, the chain is re-created unconditionally with iptables -N FW_WRAPPER outside the cleanup try/catch; if the flush throws before the chain is deleted, the chain likely still exists and -N would fail in a real run. Consider adjusting the simulated failure point (or the expected outcome) so the test matches the production control flow.
This issue also appears on line 1421 of the same file.
Contributor
Author
🔥 Smoke Test Results — PASS
PR: [Test Coverage] Improve host-iptables.ts branch coverage from 83% to 99%
|
Contributor
Author
|
Smoke Test Results (Codex)
|
Contributor
Author
Chroot Version Comparison Results
Overall: ❌ NOT all tests passed — Python and Node.js versions differ between host and chroot environments.
|
Contributor
Author
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds 17 new tests to
src/host-iptables.test.tstargeting previously uncovered branches in the security-criticalhost-iptables.tsmodule.Coverage Improvements
host-iptables.tsOverall Project
Security-Critical Paths Now Covered
IPv6 Handling (Bypass Prevention)
FW_WRAPPER_V6chain creation when IPv6 DNS servers are provided andip6tablesis availableFW_WRAPPER_V6chain before recreating (prevents stale rules)ip6tablesis unavailable even with IPv6 DNS servers configuredDocker Bridge Gateway Validation
getDockerBridgeGatewayreturnsnullwhen Docker bridge returns empty stdout — prevents empty string from being used in iptables rulesgetDockerBridgeGatewayreturnsnullwhen Docker bridge returns non-IPv4 (e.g., hostname) — prevents invalid IPs from entering firewall rulesDNS Server Fallback
dnsServersarray correctly falls back toDEFAULT_DNS_SERVERS(Google DNS), preventing an open DNS channelPort Validation
allowHostServicePortssplits (e.g.,"5432,,6379") are silently skippedallowHostServicePortsare skipped with a warning (mirrors existing behavior forallowHostPorts)Error Paths
sysctlfailure when disabling IPv6 (doesn't throw, logs warning)sysctlfailure when re-enabling IPv6 on cleanup (doesn't throw, logs debug)iptables -N DOCKER-USERfailure after chain not found (propagates error with clear message)FW_WRAPPERchain flush failure during cleanup (caught, doesn't throw)Line Parsing Edge Cases
No Existing Tests Modified
All 3 pre-existing test failures in
docker-manager.test.tsremain unchanged (unrelated to this PR).