Repository navigation
test: cover licensing, the firewall and session recording end to end - #7390
Merged
Merged
Conversation
otavio
added this pull request to stack #7380
October 7, 2026 21:19
otavio
marked this pull request as ready for review
October 7, 2026 21:26
|
Claude finished @otavio's task in 5m 11s —— View job Code Review CompleteReviewed 10 files. 2 inline issues posted. Standards: 1 violation, 1 judgement call. Spec: 0 (did not run). Correctness: 0. StandardsThe worst violation is in SpecThe Spec axis did not run. The PR closes no issue in this repo, and the only spec it cites, shellhub-io/team#243 Domain 15, is in a repo this run cannot read. Additional notesCorrectness found no defects. These are the parts it checked, against the
The PR touches no To request another review round, comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
otavio
force-pushed
the
test/e2e-firewall
branch
from
October 8, 2026 14:45
c1eab01 to
49d3fa6
Compare
otavio
force-pushed
the
test/e2e-recording
branch
2 times, most recently
from
October 8, 2026 15:57
3a7d5d3 to
6a664c2
Compare
otavio
removed this pull request from stack #7380
October 8, 2026 15:58
otavio
changed the title
This was referenced Oct 8, 2026
Member
Author
|
/review |
otavio
force-pushed
the
test/e2e-recording
branch
from
October 8, 2026 16:56
6a664c2 to
50b815a
Compare
Member
Author
|
/review |
|
Claude finished @otavio's task in 6m 6s —— View job Code Review CompleteReviewed 45 files and posted 2 inline issues:
Standards
CorrectnessIn SpecThe Spec axis did not run. The PR closes no issue in this repo, and the spec it cites, shellhub-io/team#243, is in a repo this run cannot read. Additional notesI checked the production changes and found no defects:
I left out a few low-value judgement calls:
To request another review round, comment |
otavio
force-pushed
the
test/e2e-recording
branch
from
October 8, 2026 17:04
50b815a to
152a662
Compare
Member
Author
|
/review |
|
Claude finished @otavio's task in 6m 11s —— View job Code Review CompleteReviewed 45 files and posted 3 inline issues:
StandardsThe worst violation is the SpecThe Spec axis did not run. The PR closes no issue in this repo, and the spec it cites, shellhub-io/team#243, is in a repo this run cannot read. Additional notesCorrectness found no defects. These are the parts it checked:
Two findings were dropped after validation:
To request another review round, comment |
An enterprise stack verified licenses against the production key compiled into cloud's pkg/lic, so a test could only run under the one real license a developer or CI secret supplied. Expired, limited, region-bound or feature-less licenses were out of reach, and so was most of the licensing domain. A run started with a LicenseIssuer now owns a test RSA key. The harness builds the server image with -X github.com/shellhub-io/cloud/pkg/lic.PublicKey set to its public half, through a new GO_LDFLAGS build argument that the production build leaves empty. The key lives in tests/.stack/license-issuer.pem and runs reuse it. A fresh key per run would change the build argument and recompile the enterprise server every time. GO_LDFLAGS is declared right before the build step so the module download layers stay cached, and the harness sets it only for enterprise and cloud builds. Each enterprise stack then writes its license to a file of its own and mounts it. The enterprise test overlay maps an unset SHELLHUB_LICENSE_FILE to /dev/null and to an empty ADMIN_API_LICENSE_FILE, which is how WithoutLicense starts a server that loads no license at all. Runs without an issuer, the Playwright suite among them, keep reading SHELLHUB_LICENSE_FILE as before. WithEveryAddressIn mounts GeoLite2 country and city databases, written with mmdbwriter, that place ::/0 in one country. They include reserved networks and turn IPv4 aliasing off because a test connects from a private address, which a stock database leaves unlocated. The locator needs both files to start, and the coordinates do not matter, so they stay at zero. MAXMIND_MIRROR points at an unreachable host only to make the server enable the locator. The fetcher skips every database already present, so it never contacts the host. RestartFailingServer waits for the container's on-failure policy to give up rather than for the API, since a server refusing its license file exits. UseLicense, RestartServer and DeleteDevice outlive the test's context, because Go cancels that context before cleanups run and these helpers restore state in a t.Cleanup. A paid edition now fails early, naming CloudDir's go.mod, when the cloud source is missing, instead of deep inside the image build.
TestEnterpriseLicensing runs one enterprise stack under licenses the run signs, and swaps the installed license per case through the admin API. TestEnterpriseWithoutLicenseFile starts a second stack with no license file. The tests pin two behaviours that read like accidents and are not. - The server logs a refusal for a region the license does not allow as a firewall block. Only the firewall evaluator checks allowed_regions, so the region cases run in a legacy-mode namespace with the firewall feature licensed. - A license that has not started refuses device acceptance but lets a connection through. CanConnectDevice skips IsStarted on purpose, so that a future start date cannot lock an instance out of the devices it already has. Each acceptance case deletes the pending device it enrolled when it ends, because startAcceptedAgent expects exactly one pending device. Their MAC addresses use 02:00:00:00:21:xx, a locally administered prefix numbered after the domain, so they never merge with another case's device. Covers shellhub-io/team#243 Domain 21 Feature Gating, Device Acceptance, Device Connection, License Regions, License Signature and License File Loading, except the billing gating and fail-open cases.
The testcontainers suite ran on the community stack only, so the licensing tests, and the firewall and recording tests that follow them, never ran in CI. A new job checks out the cloud source the way ui-e2e.yml does. The paired-branch action matches the PR's branch name and falls back to master, and the job runs ^TestEnterprise. It needs no license secret, because the run signs its own licenses. A pull request from a fork cannot mint the cloud token, so the job skips its steps and leaves a notice instead of failing every outside contribution. The community leg skips ^TestEnterprise so a run without the cloud source does not try to build an enterprise stack. The flag goes in a test_args matrix field of its own, because extra_args also reaches go build, which has no -skip.
…ansaction Accepting a device asks the license evaluator whether the instance may hold one more, and allows the device when the evaluator fails. That fail-open never held against the database. The check ran inside UpdateDeviceStatus's transaction, and the enterprise store reads the license and counts devices through GetConnection(ctx), which joins it. A failed read aborted the PostgreSQL transaction. The service logged the warning and went on, the next DeviceUpdate failed on the aborted transaction, and the request answered 500. Mocked unit tests could not see it, because the mock store has no transaction to abort. The decision is now taken before WithTransaction opens and handed to the transaction, which still refuses only a device that does not merge into an accepted one. A merge therefore costs one license read it did not before, and the merge-path unit test now runs against an evaluator that refuses, to pin that a merge ignores the answer.
BreakLicenseStore renames the licenses table for the rest of the test, so every license read fails with a database error rather than finding no license. The case installs a license that would refuse the device first, so a 200 proves the acceptance failed open instead of passing the limit. Covers shellhub-io/team#243 Domain 21 "Accept device fails open on store error".
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
finish() discards a recording that captured no events, but it counted resizes as events. A web terminal that signs in with a browser key registers its resize handler on the signature challenge, and the terminal refits as soon as the window settles. So a login the user then rejected left a recording behind holding only a resize line: an entry in the recordings list, and a "Session recorded" notice, for a cast that replays a blank screen. Only output now marks a recording worth keeping; resizes are still written so a real recording reflows on replay. The eventCount getter had no caller and goes with the counter it exposed.
An enterprise stack now hands the test its MinIO container, and tests/environment/recording.go reads and drives recordings through it and the API: turning a namespace's recording on or off, fetching and deleting a seat's recording, counting a session's events of one type, reading the converted mark the archive job sets, listing and removing the objects a session holds in the bucket, and breaking or repairing a session's terminal request. Listing goes through mc inside the MinIO container, since the bucket is not published to the host. A line mc prints that is not an entry it listed successfully is an error, so an empty listing cannot come from a failure. Breaking the terminal request stands in for a session whose archive fails: the archive job builds the cast header from that event, so prefixing its data makes the header unreadable until the prefix is taken off again. Nothing in the product can be asked to fail one session's archive. RunCron now enqueues every job scheduled on the spec instead of refusing an ambiguous one. The recording archive job and the active-session reaper are both scheduled on "* * * * *", and asynq names a cron task with a random UUID, so the spec is all that tells jobs apart. Both jobs are safe to run at any time.
TestEnterpriseSessionRecording runs one enterprise stack and covers which sessions are recorded, playback by seat, the archive job, deleting a recording, and retention taking the archived recording with the session. A count of a session's events read once the session is inactive is complete: Session.Finish drains the session's event writer before it deactivates the session. So the negative cases read the terminal output count once after close. Only cases that read events while the session is still open wait for the batched writes to land. In the skip-and-retry case the broken session is opened inside the healthy session's connection. The archive job lists sessions newest first and the server's own every-minute tick keeps running, so the healthy session must never be archivable while the broken one is intact or missing: any run that archives the healthy session has tried the broken one first. Asking for a recording a session does not have expects 404. Cloud answered 500 until shellhub-io/cloud's test/e2e-recording branch made ErrSessionNotRecorded a not-found error. The 404 wait TestSessionRetention used moves to awaitSessionDeleted, shared with the retention case here.
The empty session is a browser-key login that the re-authentication policy holds and the user rejects: the WebSocket stays open while the approval dialog waits, so the recorder is certainly created before the session ends, and the session prints nothing. A wrong password does not work for this. The server closes the socket within milliseconds, so the recorder may not exist yet when the tab closes, and the test passes whether or not empty recordings are dropped. No UI shows that the recorder exists, so the test waits for its .cast file in the browser's origin private file system before rejecting, then requires the file gone and the preferences page to list no recording.
Stack.SQL returns what psql prints, a table with headers and a row count, so
every helper that read a value selected it behind a label ('events=',
'licenses=', 'converted=', 'digest=') and matched the label with a regex of
its own. Five helpers carried five patterns for the same parsing.
Stack.SQLValue runs the query with --tuples-only --no-align, so psql prints
the bare value, and fails when the query selects no row or more than one
line. The counts, the converted mark and the password digest read through
it, and the session event counts no longer share one query that took an
empty event type to mean every type.
finish() promised that a recording whose file fails to close comes back null with its partial file removed, but the catch only logged and returned null. The .cast file create() made stayed in the origin-private file system with no metadata beside it, so the recordings list never showed it and nothing ever deleted it. The catch now removes the files the way an empty recording does.
otavio
force-pushed
the
test/e2e-recording
branch
from
October 8, 2026 17:12
152a662 to
8566c78
Compare
Member
Author
|
/review |
|
Claude finished @otavio's task in 2m 27s —— View job Code Review CompleteI reviewed all 45 files for Standards (repo rules, Go and React conventions, tests, code smells) and Correctness (security and runtime defects), and found no new issues. The Spec axis did not run. The PR closes no issue in this repo, and the spec its commits cite, shellhub-io/team#243, is in a repo this run cannot read. |
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
2 participants
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
Covers three domains of shellhub-io/team#243 end to end and runs them in a new CI job: Licensing (D21), the firewall items of SSH Legacy Mode (D12) and Session Recording (D15). The tests found four product bugs, and each fix is its own commit next to the case that caught it.
This PR consolidates what was the stack #7377, #7379, #7384 and #7390 so the four review together. The commits keep the stack's order: licensing harness and tests, the license accept fix, firewall, recording.
Enterprise harness and CI
An enterprise stack could only run under the one real license
SHELLHUB_LICENSE_FILEnamed, so expired, limited or feature-less licenses were out of reach. A run now signs its own:The production build leaves
GO_LDFLAGSempty. Runs without an issuer, the Playwright suite among them, still readSHELLHUB_LICENSE_FILE.The firewall and recording layers add their own helpers:
RunCronhad to change because the recording archive job and the active-session reaper both run on* * * * *. asynq names a cron task with a random UUID, so the spec is all that tells them apart.Product fixes
Accepting a device answered 500 when the license could not be read. The fail-open contract never held against the database.
validateDeviceAcceptanceran insideUpdateDeviceStatus's transaction, and the enterprise store reads the license through it, so a failed read aborted the transaction and the followingDeviceUpdatefailed. The license is now evaluated beforeWithTransactionopens and passed in as a bool. All three accept paths go throughupdateDeviceStatusOwnedBy:PATCH /api/devices/:uid/accept, enrollment auto-accept and pairing. A merge now costs one license read whose answer it ignores.The firewall rule form told users the opposite order. Its priority hint said "Higher values are evaluated first", while the store lists rules
ORDER BY priority ASCand the first match decides. It now says "Lower values are evaluated first", and aRuleModalunit test pins it.Cloud answered 500 for a session recording that does not exist.
ErrSessionNotRecordedcarriedErrCodeStore. It answers 404 now, as the OpenAPI spec documents. The change is shellhub-io/cloud#2634, ontest/e2e-recording, the same branch name, so the enterprise job checks it out. It must merge with or before this PR, or the playback and deletion cases fail against cloud master.A browser recording whose file failed to close stayed on disk.
finish()promised to remove the partial file but only logged. It removes it now.The browser kept a recording of a session that printed nothing.
finish()counted resizes as events. A browser-key login registers the resize handler on the signature challenge and the terminal refits, so a rejected login left a "Session recorded" notice and a cast that replays a blank screen. Only output counts now.Issue lines covered
D21, Licensing
TestEnterpriseLicensing/feature gating/firewall routes answer 402 without the featureTestEnterpriseLicensing/feature gating/firewall routes answer with the featureTestEnterpriseLicensing/feature gating/session recording routes answer 402 without the featureTestEnterpriseLicensing/feature gating/session recording routes answer with the featureTestEnterpriseLicensing/feature gating/an expired license answers 402 on every gated routeTestEnterpriseLicensing/feature gating/the license endpoint answers under an expired license,.../the license endpoint installs a license when none is installedTestEnterpriseLicensing/device acceptance/accepting a device within the license limit succeedsTestEnterpriseLicensing/device acceptance/accepting a device with the fleet at the license limit is refusedTestEnterpriseLicensing/device acceptance/accepting a device under an unlimited license succeedsTestEnterpriseLicensing/device acceptance/accepting a device with no license installed is refusedTestEnterpriseLicensing/device acceptance/accepting a device under an expired license is refusedTestEnterpriseLicensing/device acceptance/accepting a device under a license not yet started is refusedTestEnterpriseLicensing/device acceptance/accepting a device succeeds when the license cannot be read.BreakLicenseStorerenames thelicensestable after installing a license that would refuse the device, so only fail-open answers 200TestEnterpriseLicensing/device connection/connecting with the fleet within the license limit succeedsTestEnterpriseLicensing/device connection/connecting with the fleet over the license limit is refusedTestEnterpriseLicensing/device connection/connecting under an unlimited license succeedsTestEnterpriseLicensing/device connection/connecting with no license installed is refusedTestEnterpriseLicensing/device connection/connecting under an expired license is refusedTestEnterpriseLicensing/device connection/connecting under a license not yet started succeedsTestSSHConnectionBehaviour/each cipher the server offers is the one negotiatedTestEnterpriseLicensing/device connection/connecting from a region the license allows succeedsTestEnterpriseLicensing/device connection/connecting from a region the license does not allow is refusedallowed_regionsmeans globalTestEnterpriseLicensing/device connection/connecting under a license that names no region succeeds from anywhereTestEnterpriseLicensing/signature/a license the issuer signed is installedTestEnterpriseLicensing/signature/a license whose body changed after signing is refusedTestEnterpriseLicensing/signature/a license shorter than its signature is refusedTestEnterpriseLicensing/signature/a license that is not base64 is refusedTestEnterpriseLicensing/license file/the server installs the license file on startupTestEnterpriseLicensing/license file/a restart with another license file replaces the installed licenseTestEnterpriseLicensing/license file/a restart with the same license file installs nothingTestEnterpriseWithoutLicenseFileTestEnterpriseLicensing/license file/an invalid license file stops the server from startingD12, firewall rule evaluation
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, over a deny-all rule so only a full match lets the client inTestEnterpriseFirewall/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 rulesterminal.spec.ts› Firewall Rules ›a deny rule created in the console keeps the ssh client outD15, Session Recording
terminal.spec.ts› Session Recording ›the server keeps the recording after the session ends.TestEnterpriseSessionRecording/which sessions are recorded/an interactive shell is recordedadds the native SSH client and is the positive control for the negative cases below.../which sessions are recorded/a command run without a terminal is not recorded. An exec with a pty is recorded: the server decides by pty, not by request type.../which sessions are recorded/a shell is not recorded while the namespace records nothing.../which sessions are recorded/an SFTP transfer is not recorded,.../an scp copy is not recordedterminal.spec.ts› Session Recording ›a session that printed nothing leaves no browser recording, plusrecordings.test.ts.../retention/a session past the retention window takes its archived recording with it.../archive/the archive job moves a closed session's terminal output to object storage.../archive/a session that fails to archive is skipped and retried on the next run.../playback/each seat of a connection plays back only its own terminal,.../archive/each seat is archived to an object of its ownGetSessionAsciinemaon unrecorded session returns error.../playback/a session that was not recorded has no recording to play, 404GetSessionAsciinemaon no-PTY session returns error.../playback/a session without a terminal has no recording to play, 404DeleteSessionAsciinemaclearsRecordedflag.../deletion/deleting a recording removes its object and clears the recorded flagDeleteSessionAsciinemawhen object already gone.../deletion/deleting a recording whose object is already gone clears the recorded flagNot covered here:
license.spec.ts.services/firewall_test.gocovers the limit at the unit level./api/billingwithout the license middlewares ininternal/billing/routes/routes.go, and has since the monolith conversion in cloud a0681806. The billing branch ofLicenseCheckFeatureis dead code.HasLockreadslocks.exclusive_acquired, which nothing in shellhub or cloud ever writes, so a test could only prove the row it seeded.Evidence
testrunner and thee2eimage:Each product fix was red first:
Each mutation below made the cases it targets fail: deny rules created inactive, the allow rule moved to priority 300, the inactive rule made active, the identity-mode switch removed, the console rule saved as Allow, and
finish()keeping empty recordings.golangci-lint v2.14.0 reports 0 issues on
tests/with--max-issues-per-linter=0 --max-same-issues=0, and onserver/and cloud'sinternal/cloud/...for the layers that touch them. ESLint, Prettier, tsc and knip are clean on the console, and actionlint passesqa.yml.Review
Each layer went through
/code-reviewbefore it was opened, and the bot review after. The last bot findings were all doc and test accuracy, fixed in the commits that introduced them:Up's contract now coversUnlicensedstacks and the cloud edition'sdocker-compose.ymlcheck, andWithEditionsays whenUpfails.FirewallRuleFiltersays a device matches when it carries any one of the tags, as cloud's evaluator does.SessionEventCountandSessionEventCountOfTypeshare one query and parser.recordings.test.tsparses the cast instead of matching a substring of it.A
/code-reviewof the consolidated branch then found:finish()'s doc promised a removal the code skipped, fixed above..github/actions/paired-branch. It uses the action now, and the community leg's-skipmoved to atest_argsfieldgo buildnever sees.Stack.SQLValuereturns the bare value instead.StartRuntakes the license issuer directly instead of a one-option variadic,licenseFileEnvsbecamelicensingEnvs, andremoveArtifactsmoved tostack.gosince it also removes GeoIP files.Merge Danger
Door: two-way
The production-facing changes are the license read moving out of the accept transaction, one line of console copy, the browser recorder dropping empty casts, and the
GO_LDFLAGSbuild argument inserver/Dockerfile, which is empty unless a build passes it. No schema, API or wire format changes.Blast Radius: enterprise and CI
Only an enterprise or cloud server runs a license evaluator. Concurrent accepts at the license limit were already racy, because the count was never locked instance-wide, and reading it before the transaction leaves that as it was. The new CI job adds one enterprise build and the stacks of four enterprise tests to QA, using the
CLOUD_DISPATCH_APP_*secretsui-e2e.ymlalready uses.RunCronnow also runs the active-session reaper wherever a test triggers* * * * *.Merge order: shellhub-io/cloud#2634 first or together.
Part of shellhub-io/team#243.