fix: Relax inbound firewall and add tamper protection#29
Open
fix: Relax inbound firewall and add tamper protection#29
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a configurable inbound firewall mode for the devcontainer. The firewall now defaults to allowing inbound traffic (permissive mode) while providing an opt-in strict mode. Firewall command patterns are added to the permission deny list to prevent tampering through configuration files and scripts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/hooks/devcontainer-policy-blocker.sh:
- Around line 105-118: The current BLOCKED_FIREWALL array and matching logic
(uses BLOCKED_FIREWALL, COMMAND, and block) miss variants like iptables-legacy;
update the firewall check to also match common variants (e.g., iptables-legacy,
ip6tables-legacy, iptables-nft, etc.) or switch the grep test to detect the
iptables prefix with optional hyphen/word characters so commands like
"iptables-legacy -L" are caught; ensure you adjust the BLOCKED_FIREWALL entries
or the matching expression consistently and still avoid false positives like
iptables-save/restore before calling block.
In @.claude/settings.json:
- Around line 34-36: The settings.json intentionally omits the "Bash(*sudo *)"
deny pattern (while tier1/2/3 include it) to allow sudo on bare-metal; add a
short clarifying note in the project permissions documentation
(DEVCONTAINER_PERMISSIONS.md) describing this design choice, stating that
settings.json is for bare-metal installs and therefore does not block sudo
whereas the tier files enforce sudo denial for containerized/devcontainer
environments, so readers understand why "Bash(*sudo *)" is present in tiers but
absent from settings.json.
In `@tests/test_hooks.py`:
- Around line 236-242: The test
TestPolicyBlockerFirewallPatterns::test_policy_blocker_blocks_firewall_commands
should also assert that the devcontainer-policy-blocker.sh contains the "sudo "
pattern; update the pattern list used in that test (the for loop over ["iptables
", "ip6tables ", "ipset ", "nft ", "init-firewall"]) to include "sudo " so the
test verifies the Bash(*sudo *) tamper protection as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fed3a26a-84cf-4508-ac09-545b673b4ebc
📒 Files selected for processing (12)
.claude/hooks/devcontainer-policy-blocker.sh.claude/hooks/firewall-edit-blocker.sh.claude/settings.json.devcontainer/devcontainer.json.devcontainer/init-firewall.sh.devcontainer/permissions/tier1-assisted.json.devcontainer/permissions/tier2-autonomous.json.devcontainer/permissions/tier3-full-trust.jsonCLAUDE.mddocs/DECISIONS.mddocs/DEVCONTAINER_PERMISSIONS.mdtests/test_hooks.py
tests/test_hooks.py
Outdated
Comment on lines
+236
to
+242
| class TestPolicyBlockerFirewallPatterns: | ||
| """Verify policy blocker blocks firewall commands.""" | ||
|
|
||
| def test_policy_blocker_blocks_firewall_commands(self) -> None: | ||
| content = (HOOKS_DIR / "devcontainer-policy-blocker.sh").read_text(encoding="utf-8") | ||
| for pattern in ["iptables ", "ip6tables ", "ipset ", "nft ", "init-firewall"]: | ||
| assert pattern in content, f"devcontainer-policy-blocker missing firewall pattern: {pattern}" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding sudo to the firewall pattern test.
The test verifies iptables, ip6tables, ipset, nft, and init-firewall patterns, but the tier JSON files also add Bash(*sudo *) as part of the firewall tamper protection. Consider including "sudo " in the pattern list for completeness.
Suggested addition
def test_policy_blocker_blocks_firewall_commands(self) -> None:
content = (HOOKS_DIR / "devcontainer-policy-blocker.sh").read_text(encoding="utf-8")
- for pattern in ["iptables ", "ip6tables ", "ipset ", "nft ", "init-firewall"]:
+ for pattern in ["iptables ", "ip6tables ", "ipset ", "nft ", "init-firewall", "sudo "]:
assert pattern in content, f"devcontainer-policy-blocker missing firewall pattern: {pattern}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| class TestPolicyBlockerFirewallPatterns: | |
| """Verify policy blocker blocks firewall commands.""" | |
| def test_policy_blocker_blocks_firewall_commands(self) -> None: | |
| content = (HOOKS_DIR / "devcontainer-policy-blocker.sh").read_text(encoding="utf-8") | |
| for pattern in ["iptables ", "ip6tables ", "ipset ", "nft ", "init-firewall"]: | |
| assert pattern in content, f"devcontainer-policy-blocker missing firewall pattern: {pattern}" | |
| class TestPolicyBlockerFirewallPatterns: | |
| """Verify policy blocker blocks firewall commands.""" | |
| def test_policy_blocker_blocks_firewall_commands(self) -> None: | |
| content = (HOOKS_DIR / "devcontainer-policy-blocker.sh").read_text(encoding="utf-8") | |
| for pattern in ["iptables ", "ip6tables ", "ipset ", "nft ", "init-firewall", "sudo "]: | |
| assert pattern in content, f"devcontainer-policy-blocker missing firewall pattern: {pattern}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_hooks.py` around lines 236 - 242, The test
TestPolicyBlockerFirewallPatterns::test_policy_blocker_blocks_firewall_commands
should also assert that the devcontainer-policy-blocker.sh contains the "sudo "
pattern; update the pattern list used in that test (the for loop over ["iptables
", "ip6tables ", "ipset ", "nft ", "init-firewall"]) to include "sudo " so the
test verifies the Bash(*sudo *) tamper protection as well.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/DEVCONTAINER_PERMISSIONS.md`:
- Line 101: The sentence claiming "Docker's own network stack already provides
inbound isolation" is too absolute—update that phrasing to qualify that inbound
isolation depends on configuration; replace it with wording such as: "Docker's
network stack can provide inbound isolation depending on port publishing,
network mode, and the host firewall context" and/or add a short parenthetical
mentioning published ports and bridge/host/network modes so the doc accurately
reflects that isolation is conditional rather than guaranteed.
In `@tests/test_hooks.py`:
- Around line 239-242: Extract the inline firewall pattern list in
test_policy_blocker_blocks_firewall_commands into a module-level constant (e.g.,
FIREWALL_PATTERNS) declared near the top of tests/test_hooks.py, then replace
the literal list in the test with that constant; keep using HOOKS_DIR and the
existing loop/assert logic to read devcontainer-policy-blocker.sh and assert
each pattern is present so readability and maintainability are improved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 71f385f0-e43b-4f11-98db-647f5297e982
📒 Files selected for processing (3)
.claude/hooks/devcontainer-policy-blocker.shdocs/DEVCONTAINER_PERMISSIONS.mdtests/test_hooks.py
docs/DEVCONTAINER_PERMISSIONS.md
Outdated
|
|
||
| ## Inbound Firewall | ||
|
|
||
| By default, the devcontainer firewall only restricts **egress** (outbound) traffic. Inbound traffic is left permissive because Docker's own network stack already provides inbound isolation, and strict inbound rules prevent legitimate use cases like exposing dev servers to LAN devices or other containers. |
There was a problem hiding this comment.
Avoid absolute inbound-isolation wording.
Line 101 says Docker “already provides inbound isolation,” which can be interpreted too broadly. Please qualify this to avoid giving a false security guarantee (e.g., isolation depends on port publishing/network mode and host firewall context).
Suggested wording tweak
-By default, the devcontainer firewall only restricts **egress** (outbound) traffic. Inbound traffic is left permissive because Docker's own network stack already provides inbound isolation, and strict inbound rules prevent legitimate use cases like exposing dev servers to LAN devices or other containers.
+By default, the devcontainer firewall only restricts **egress** (outbound) traffic. Inbound traffic is left permissive because Docker networking typically limits direct inbound reachability unless ports are published, and strict inbound rules can prevent legitimate use cases like exposing dev servers to LAN devices or other containers.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| By default, the devcontainer firewall only restricts **egress** (outbound) traffic. Inbound traffic is left permissive because Docker's own network stack already provides inbound isolation, and strict inbound rules prevent legitimate use cases like exposing dev servers to LAN devices or other containers. | |
| By default, the devcontainer firewall only restricts **egress** (outbound) traffic. Inbound traffic is left permissive because Docker networking typically limits direct inbound reachability unless ports are published, and strict inbound rules can prevent legitimate use cases like exposing dev servers to LAN devices or other containers. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/DEVCONTAINER_PERMISSIONS.md` at line 101, The sentence claiming
"Docker's own network stack already provides inbound isolation" is too
absolute—update that phrasing to qualify that inbound isolation depends on
configuration; replace it with wording such as: "Docker's network stack can
provide inbound isolation depending on port publishing, network mode, and the
host firewall context" and/or add a short parenthetical mentioning published
ports and bridge/host/network modes so the doc accurately reflects that
isolation is conditional rather than guaranteed.
tests/test_hooks.py
Outdated
Comment on lines
+239
to
+242
| def test_policy_blocker_blocks_firewall_commands(self) -> None: | ||
| content = (HOOKS_DIR / "devcontainer-policy-blocker.sh").read_text(encoding="utf-8") | ||
| for pattern in ["iptables ", "iptables-legacy ", "iptables-nft ", "ip6tables ", "ip6tables-legacy ", "ip6tables-nft ", "ipset ", "nft ", "init-firewall"]: | ||
| assert pattern in content, f"devcontainer-policy-blocker missing firewall pattern: {pattern}" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting patterns to a constant for readability.
The inline pattern list is lengthy. Extracting to a module-level constant would improve readability and make future updates easier.
♻️ Suggested refactor
+FIREWALL_PATTERNS = [
+ "iptables ",
+ "iptables-legacy ",
+ "iptables-nft ",
+ "ip6tables ",
+ "ip6tables-legacy ",
+ "ip6tables-nft ",
+ "ipset ",
+ "nft ",
+ "init-firewall",
+]
+
+
class TestPolicyBlockerFirewallPatterns:
"""Verify policy blocker blocks firewall commands."""
def test_policy_blocker_blocks_firewall_commands(self) -> None:
content = (HOOKS_DIR / "devcontainer-policy-blocker.sh").read_text(encoding="utf-8")
- for pattern in ["iptables ", "iptables-legacy ", "iptables-nft ", "ip6tables ", "ip6tables-legacy ", "ip6tables-nft ", "ipset ", "nft ", "init-firewall"]:
+ for pattern in FIREWALL_PATTERNS:
assert pattern in content, f"devcontainer-policy-blocker missing firewall pattern: {pattern}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_hooks.py` around lines 239 - 242, Extract the inline firewall
pattern list in test_policy_blocker_blocks_firewall_commands into a module-level
constant (e.g., FIREWALL_PATTERNS) declared near the top of tests/test_hooks.py,
then replace the literal list in the test with that constant; keep using
HOOKS_DIR and the existing loop/assert logic to read
devcontainer-policy-blocker.sh and assert each pattern is present so readability
and maintainability are improved.
stranma
force-pushed
the
fix/firewall-inbound-tamper-protection
branch
from
dc78f40 to
f7c9b9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/init-firewall.sh:
- Around line 202-204: The current init-firewall.sh branch only sets iptables -P
INPUT DROP when ALLOW_INBOUND != "true", leaving INPUT policy unchanged when
ALLOW_INBOUND=="true" which breaks idempotency; update the conditional so the
permissive branch explicitly runs iptables -P INPUT ACCEPT (i.e., when
ALLOW_INBOUND == "true" set the INPUT policy to ACCEPT) so each run
deterministically enforces the intended mode and prevents leftover DROP policies
from previous runs.
In `@tests/test_hooks.py`:
- Around line 175-179: The test_settings_denies_firewall_commands currently
reads settings.json as a raw string and asserts substrings; instead parse the
JSON (use settings_path.read_text + json.loads) and locate the permissions.deny
array, then assert that each expected pattern (iptables, ip6tables, ipset, nft,
init-firewall) is present as an element in that deny list (e.g., compare
membership or set equality) so the test fails if those patterns were moved out
of permissions.deny; update the test to raise a clear assertion if permissions
or deny are missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78151549-aeff-4aa7-ab5e-e0c0d2fff950
📒 Files selected for processing (5)
.claude/settings.json.devcontainer/devcontainer.json.devcontainer/init-firewall.shdocs/DECISIONS.mdtests/test_hooks.py
Comment on lines
+202
to
+204
| if [ "$ALLOW_INBOUND" != "true" ]; then | ||
| iptables -P INPUT DROP | ||
| fi |
There was a problem hiding this comment.
Permissive mode is not explicitly enforced (idempotency bug).
When ALLOW_INBOUND=true, INPUT policy is left unchanged. If a previous run set INPUT DROP, permissive mode can still behave as strict. Set INPUT ACCEPT explicitly in the permissive branch.
Proposed fix
-if [ "$ALLOW_INBOUND" != "true" ]; then
- iptables -P INPUT DROP
-fi
+if [ "$ALLOW_INBOUND" != "true" ]; then
+ iptables -P INPUT DROP
+else
+ iptables -P INPUT ACCEPT
+fiBased on learnings: "Implement two-layer defense against data exfiltration: firewall (iptables whitelist in devcontainer) and exfiltration guard (dangerous-actions-blocker.sh hook)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if [ "$ALLOW_INBOUND" != "true" ]; then | |
| iptables -P INPUT DROP | |
| fi | |
| if [ "$ALLOW_INBOUND" != "true" ]; then | |
| iptables -P INPUT DROP | |
| else | |
| iptables -P INPUT ACCEPT | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/init-firewall.sh around lines 202 - 204, The current
init-firewall.sh branch only sets iptables -P INPUT DROP when ALLOW_INBOUND !=
"true", leaving INPUT policy unchanged when ALLOW_INBOUND=="true" which breaks
idempotency; update the conditional so the permissive branch explicitly runs
iptables -P INPUT ACCEPT (i.e., when ALLOW_INBOUND == "true" set the INPUT
policy to ACCEPT) so each run deterministically enforces the intended mode and
prevents leftover DROP policies from previous runs.
Comment on lines
+175
to
+179
| def test_settings_denies_firewall_commands(self) -> None: | ||
| settings_path = Path(__file__).parent.parent / ".claude" / "settings.json" | ||
| content = settings_path.read_text(encoding="utf-8") | ||
| for pattern in ["iptables", "ip6tables", "ipset", "nft", "init-firewall"]: | ||
| assert pattern in content, f"settings.json missing firewall deny pattern: {pattern}" |
There was a problem hiding this comment.
Test is too weak: it checks substrings, not actual deny-list entries.
This can pass even if firewall patterns are moved out of permissions.deny. Parse JSON and assert exact deny patterns in the deny array.
Proposed fix
+import json
@@
class TestFirewallDenyRules:
@@
def test_settings_denies_firewall_commands(self) -> None:
settings_path = Path(__file__).parent.parent / ".claude" / "settings.json"
- content = settings_path.read_text(encoding="utf-8")
- for pattern in ["iptables", "ip6tables", "ipset", "nft", "init-firewall"]:
- assert pattern in content, f"settings.json missing firewall deny pattern: {pattern}"
+ data = json.loads(settings_path.read_text(encoding="utf-8"))
+ deny_rules = set(data.get("permissions", {}).get("deny", []))
+ expected = {
+ "Bash(*iptables *)",
+ "Bash(*ip6tables *)",
+ "Bash(*ipset *)",
+ "Bash(*nft *)",
+ "Bash(*init-firewall*)",
+ }
+ for pattern in expected:
+ assert pattern in deny_rules, f"settings.json missing firewall deny pattern: {pattern}"Based on learnings: "Implement two-layer defense against data exfiltration: firewall (iptables whitelist in devcontainer) and exfiltration guard (dangerous-actions-blocker.sh hook)".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_hooks.py` around lines 175 - 179, The
test_settings_denies_firewall_commands currently reads settings.json as a raw
string and asserts substrings; instead parse the JSON (use
settings_path.read_text + json.loads) and locate the permissions.deny array,
then assert that each expected pattern (iptables, ip6tables, ipset, nft,
init-firewall) is present as an element in that deny list (e.g., compare
membership or set equality) so the test fails if those patterns were moved out
of permissions.deny; update the test to raise a clear assertion if permissions
or deny are missing.
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
1 participant
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
Test plan
Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Chores
Documentation