Repository navigation
Conversation
otavio
changed the title
|
Claude finished @otavio's task in 7m 8s —— View job Code Review CompleteReviewed 10 files. 1 inline issue posted. Standards: 0 violations, 1 judgement call. Spec: did not run. Correctness: 0. StandardsThe SpecThe Spec axis did not run. The PR closes no issue in this repo, and the only reference in its commits is Additional notesThe correctness pass checked these claims against the source and found them sound:
To request another review round, comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Two tests created a tag with the same inline request, and the licensing tests spelled out the access-denied check (log mark, the banner, the server's log line) that the firewall tests need too. createTag now sits next to tagDevice, and requireAccessDenied next to dialForBanner.
The rule form told users that higher priority values are evaluated first. The server lists a namespace's rules by priority ascending and lets the first match decide, so a user following the hint would order their rules backwards and a deny meant to win would lose to the allow it was meant to override.
The firewall rule types live in the private cloud module, which the tests module cannot import, so the harness carries its own request shape. A rule's response holds tag objects where the request holds tag names, which is why CreateFirewallRule reads back only the ID. EditSSHAccessMode lets one stack serve a case in each access mode: a legacy namespace can switch to identity and back, where building a second stack would cost minutes.
Each case asserts the wrapped error text next to the server's log line, because the server logs "blocked by a firewall rule" both for a rule that denies and for an evaluation that failed (a bad pattern, a missing license). Only the error text tells a rule's deny apart. The source address a rule matches is read from the session the client opens first, not hardcoded: Docker's port publishing decides what the server sees, and it differs between rootful and rootless daemons. Every match case has a miss case beside it, and the two priority cases add their rules in opposite orders, so neither a matcher that ignores its field nor evaluation in insertion order passes. Refs: shellhub-io/team#243
The native client logs in once before the rule exists, so the refusal that follows can only come from the rule the console created. Killing the native clients moves to the file's afterEach: a third describe needed the same cleanup the other two each repeated. Refs: shellhub-io/team#243
c1eab01 to
49d3fa6
Compare
7955e5b to
fe74a86
Compare
|
Consolidated into #7390, which carries this PR's commits so the whole stack reviews and merges together. |
Summary
Covers the Firewall Rule Evaluation items and the firewall end-to-end item of shellhub-io/team#243 Domain 12 (SSH Legacy Mode). This layer sits on #7379 in the per-domain e2e stack and runs in the
validate (tests - enterprise)job #7377 added.The firewall runs in the banner phase, before authentication. A rule that blocks a connection sends the client the Access Denied banner and leaves this line in the server's log:
Each refusal checks for the wrapped error text
a firewall rule block your connectionas well as the log line, because the server writesblocked by a firewall rulefor a deny and for a failed evaluation alike. Each match case has a miss case next to it. The two priority cases create their rules in opposite orders.Product fix, its own commit: the rule form's priority hint said "Higher values are evaluated first". The store lists rules
ORDER BY priority ASC, and the first match decides. A user who followed the hint would put their rules in the reverse order. The hint now says "Lower values are evaluated first", and aRuleModalunit test pins it.Issue lines covered
TestEnterpriseFirewall/a connection is let through when the namespace has no rulesTestEnterpriseFirewall/an allow rule matching the source address, the user and the hostname lets the connection through(sits over a deny-all rule, so only a full match lets the client in)TestEnterpriseFirewall/a deny rule matching the source address blocks the connection,.../a deny rule for another source address lets the connection throughTestEnterpriseFirewall/a deny rule matching the user blocks the connection,.../a deny rule for another user lets the connection throughTestEnterpriseFirewall/a deny rule matching the hostname blocks the connection,.../a deny rule for another hostname lets the connection throughTestEnterpriseFirewall/a deny rule matching the tag blocks the connection,.../a deny rule for a tag the device does not carry lets the connection throughTestEnterpriseFirewall/an allow rule with a lower priority value wins over a matching deny rule,.../a deny rule with a lower priority value wins over a matching allow ruleTestEnterpriseFirewall/an inactive deny rule is skippedTestEnterpriseFirewall/a namespace in the identity mode is not held to its firewall rulesTestEnterpriseLicensing/device connection/connecting with the fleet over the license limit is refused. The license check runs before the firewall, so the firewall's own copy of the device limit check never decides an enterprise connectionterminal.spec.ts› Firewall Rules ›a deny rule created in the console keeps the ssh client out(withTestEnterpriseFirewallabove for the testcontainers side)Not covered here:
Evidence
testrunner and thee2eimage:Mutation checks, one run each, then reverted. Each one made the cases it targets fail:
golangci-lint v2.14.0 reports 0 issues with
--max-issues-per-linter=0 --max-same-issues=0ontests,server,gateway,pkg, andagentwith and without--build-tags docker. ESLint andtsc --noEmitare clean on the console.Review
/code-reviewran four rounds before this PR was opened:clientAddressdepended on an earlier subtest's session, so it now logs in itself. Standards: 3 hard, 8 judgement calls.EditSSHAccessModecontract was wrong.FirewallRuleCountread only the first page, so it now readsX-Total-Count.requireAccessDenied,createTag, and the native client cleanup in the file-levelafterEach.matching/allow/deny.EditSSHAccessModecontract was still incomplete (JWT bearer 403,SSHLegacyAllowed, the seeded policy's logins). The "lower priority" names were ambiguous, and two doc comments had gaps.CreateFirewallRulecontract listed some reasons for a non-200 response and missed others. Two rejected:DeleteFirewallRule's "fails unless 200" already covers every case, and the shared helpers sit next to their siblings, astagDeviceanddialForBanneralready do.Merge Danger
Door: two-way
Tests and one line of console copy. No migration, API or wire change.
Blast Radius: tests
TestEnterpriseFirewalladds about 80 seconds tovalidate (tests - enterprise). The only change users see is the corrected priority hint in the firewall rule form.