Skip to content

test: let the Playwright stack sign its own enterprise licenses - #7375

Merged
otavio merged 3 commits into
masterfrom
test/e2e/sign-own-license
Oct 9, 2026
Merged

otavio merged 3 commits into
masterfrom
test/e2e/sign-own-license

Conversation

@luizhf42

@luizhf42 luizhf42 commented Oct 7, 2026 •

Copy link
Copy Markdown
Member

Summary

#7390 made the Go suite sign its own enterprise licenses. The Playwright stack still started its run without an issuer, so the enterprise UI e2e kept reading real licenses from the TEST_LICENSE and TEST_EXPIRED_LICENSE secrets, which break when they expire.

 stack up
+  LoadLicenseIssuer(.stack/license-issuer.pem)    same key as the Go suite, same cached server build
-  StartStackRun(project)
+  StartStackRun(project, issuer)
   Up                                              signs the stack's license with the issuer
   printExports
+    E2E_EXPIRED_LICENSE                           enterprise and cloud only, expired a year ago
 .github/workflows/ui-e2e.yml
-  Write the enterprise license   (secrets.TEST_LICENSE)
-  E2E_EXPIRED_LICENSE: secrets.TEST_EXPIRED_LICENSE
 ui/scripts/e2e.sh
-  SHELLHUB_LICENSE_FILE passthrough

Closes shellhub-io/team#298

Evidence

  • Before: the enterprise UI e2e needs TEST_LICENSE and TEST_EXPIRED_LICENSE, and the expired-license test fails without the second one.
    After: ui/scripts/e2e.sh test --edition=enterprise e2e/license.spec.ts e2e/admin.spec.ts passes 6/6 with neither set, including "with an expired license every admin page sends the admin to the license page".

Merge Danger

Door: two-way

Blast Radius: e2e

Only the e2e harness and the UI e2e workflow change. The cloud edition hasn't been run locally because it needs Stripe keys. After this merges, delete the TEST_LICENSE and TEST_EXPIRED_LICENSE secrets.

@luizhf42
luizhf42 requested review from a team as code owners October 7, 2026 19:34
@luizhf42
luizhf42 added this pull request to stack #7376 October 7, 2026 19:34
@luizhf42 luizhf42 changed the title test/e2e/sign own license test: sign the enterprise e2e licenses with a key from each run Oct 7, 2026
@luizhf42 luizhf42 self-assigned this Oct 7, 2026
@luizhf42
luizhf42 force-pushed the test/e2e/sign-own-license branch from ddaa330 to b7b34d0 Compare October 7, 2026 20:40
@luizhf42

luizhf42 commented Oct 7, 2026

Copy link
Copy Markdown
Member Author

/review

@github-actions

github-actions Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Claude finished @luizhf42's task in 8m 0s —— View job


Code Review Complete

Reviewed 9 files on Standards (repo rules, language conventions, tests, code smells) and Correctness (security and runtime defects). No new issues found, and the code looks good as-is. The Spec axis did not run: the only issue this PR references, shellhub-io/team#298, is in another repository that this run cannot read.

Correctness checks that came back clean:

  • Signing matches the verifier. signLicense produces exactly what cloud/pkg/lic checks: base64 of the payload followed by the signature, with SHA-512 and PKCS#1 v1.5.
  • Public key reaches the binary. lic.PublicKey is a var, so -X can set it. The multi-line PEM survives the -ldflags quoting.
  • Release build is unchanged. With no LICENSE_PUBLIC_KEY, "$@" expands to nothing.
  • License file cleanup. Every error path in Up after the license is written removes it, either through dropLicense or through down.
  • Expired license is signed with the right key. E2E_EXPIRED_LICENSE is signed in the same process as Up, so the image built in that process trusts it.

Additional notes

  • The base branch changed during this run. The review started against a 21-file diff that included the 501 sign-in commit and the admin/license specs. GitHub now lists only 9 files, because those commits are now in test/ui/admin-license-e2e. Findings on those files belong to test(ui): cover licensing and the admin panel end to end #7369, so I dropped them here.
  • One hint is now out of date because of this PR. ui/apps/console/e2e/license.spec.ts (in the base branch) calls requireEnv("E2E_EXPIRED_LICENSE", "set it to a license that expired more than 7 days ago"). stack up now exports that variable itself. A license someone supplies by hand would be signed by a key this run's server doesn't trust, so the server would reject it as invalid rather than expired. The default hint, "run 'stack up' first", is now correct. With the custom hint gone, requireEnv's hint parameter in env.ts has no remaining caller.

@luizhf42
luizhf42 force-pushed the test/e2e/sign-own-license branch from b7b34d0 to 0da0735 Compare October 7, 2026 20:51
@luizhf42
luizhf42 force-pushed the test/e2e/sign-own-license branch from 0da0735 to 491a5c0 Compare October 7, 2026 21:14
@luizhf42
luizhf42 force-pushed the test/e2e/sign-own-license branch from 491a5c0 to 268e393 Compare October 8, 2026 11:35
@luizhf42
luizhf42 force-pushed the test/e2e/sign-own-license branch from 268e393 to fc9d006 Compare October 8, 2026 14:19
Base automatically changed from test/ui/admin-license-e2e to master October 8, 2026 17:40
@gustavosbarreto
gustavosbarreto force-pushed the test/e2e/sign-own-license branch from fc9d006 to 3842123 Compare October 8, 2026 17:40
@luizhf42
luizhf42 force-pushed the test/e2e/sign-own-license branch from 3842123 to 3dde91c Compare October 8, 2026 18:57
@luizhf42 luizhf42 changed the title test: sign the enterprise e2e licenses with a key from each run test: let the Playwright stack sign its own enterprise licenses Oct 8, 2026
@luizhf42
luizhf42 force-pushed the test/e2e/sign-own-license branch 3 times, most recently from 5fe2a32 to 5f58275 Compare October 8, 2026 20:54
@otavio
otavio force-pushed the test/e2e/sign-own-license branch from 5f58275 to f68899a Compare October 8, 2026 23:22
@otavio

otavio commented Oct 8, 2026

Copy link
Copy Markdown
Member

/review

@otavio
otavio force-pushed the test/e2e/sign-own-license branch from f68899a to 223580b Compare October 8, 2026 23:27
@github-actions

github-actions Bot commented Oct 8, 2026 •

Copy link
Copy Markdown

Claude finished @otavio's task in 4m 7s —— View job


Code Review Complete

Reviewed 7 files and posted 2 inline issues. Standards: 0 violations, 1 judgement call. Spec: did not run. Correctness: 1.

Standards

The only finding is a judgement call. licenseIssuerKeyPath at tests/cmd/stack/main.go:28 repeats the literal in tests/main_test.go:12. The Go suite and stack up share cached builds only while those two literals stay equal. No rules are broken. The updated StartStackRun doc comment states its full contract.

Correctness

stack down never removes the signed license file that stack up now writes to tests/.stack/licenses/<project>.dat. teardown reconnects through Attach, which carries no artifacts, so Stack.Down skips removeArtifacts. The comment is anchored at tests/cmd/stack/main.go:263.

These checks came back clean:

  • Shell safety. The eval'd export E2E_EXPIRED_LICENSE=… line is safe because Sign returns base64.
  • Shared key. Both runners use tests/ as the working directory. The key is created when missing, and .stack/ is gitignored.
  • Expired-license path. An expiry one year ago takes the "expired" path, not the 7-day grace period and not "invalid".
  • Community edition. It ignores the issuer.
  • Callers. All three StartStackRun callers are updated. Nothing in cloud/ calls it.

Spec

The Spec axis did not run. The only issue referenced, shellhub-io/team#298 (in the Fixes: trailer), is in another repository this run cannot read.


To request another review round, comment /review.

@otavio
otavio force-pushed the test/e2e/sign-own-license branch from 94e197d to 0579753 Compare October 9, 2026 00:18
@otavio

otavio commented Oct 9, 2026

Copy link
Copy Markdown
Member

/review

1 similar comment
@otavio

otavio commented Oct 9, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Claude finished @otavio's task in 3m 14s —— View job


Code Review Complete

Reviewed 13 files and posted 1 inline issue. Standards: 1 violation, 0 judgement calls. Spec: did not run. Correctness: 0.

Standards

The doc comment on LicenseIssuerKeyPath (tests/environment/license.go:24-26) has a second sentence explaining why the path is shared. That is rationale, not a contract the caller depends on, so the code-style.md doc-comment exemption doesn't cover it. Commit de18a4a already says the same thing.

Spec

The Spec axis did not run. The only issue this PR references is shellhub-io/team#298 (the Fixes: trailer), and this run cannot read issues in that repository.

Additional notes

  • The Attach/Down fix holds.
    • Attach gets its paths from licenseFilePath and geoIPDir, the same helpers Up uses.
    • Both teardown paths pass projectPrefix+name.
    • Go suite stacks have UUID names, so Down cannot remove their files.
    • removeArtifacts uses os.RemoveAll, so tearing down a community stack, which has no license file, does not fail.
  • Correctness checks that came back clean:
    • The eval'd E2E_EXPIRED_LICENSE export is base64 only.
    • A license that expired a year ago is past the 7-day grace period in cloud/pkg/lic, so it is treated as expired.
    • A community stack never reaches licensingEnvs.
    • All StartStackRun callers are updated.
    • Every requireEnv call in ui/ now passes a single argument.
    • Concurrent stack up runs agree on one issuer key, because the key file is created through os.Link and an ErrExist counts as success.

To request another review round, comment /review.

Comment thread tests/environment/license.go Outdated
@otavio
otavio force-pushed the test/e2e/sign-own-license branch from 0579753 to fe23233 Compare October 9, 2026 00:45
@otavio

otavio commented Oct 9, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Claude finished @otavio's task in 1m 59s —— View job


Code Review Complete

I reviewed 13 files on Standards (repo rules, language conventions, tests, code smells) and Correctness (security and runtime defects). I found no new issues, and the code looks good as it is.

The Spec axis did not run. The only issue this PR references is shellhub-io/team#298 (the Fixes: trailer), and this run can't read issues in another repository.

@otavio

otavio commented Oct 9, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Claude finished @otavio's task in 4m 59s —— View job


Code Review Complete

Reviewed 13 files and posted 1 inline issue. Standards: 0 violations, 0 judgement calls. Spec: 3. Correctness: 0.

Standards

No violations. All four doc comments that remain (LicenseIssuerKeyPath, StartStackRun, Attach, Down) state their contracts. The reasons behind the change are in the commit messages.

Spec

The Spec axis ran against shellhub-io/team#298 this time. User stories 1, 3, 4 and 5 are met. Three requirements are missing or only partly done:

  • Partial: "and the path to the private key." stack up exports E2E_EXPIRED_LICENSE but no path to the issuer key, and ui/scripts/e2e.sh forwards no key variable. Playwright tests therefore can't sign a license in any other state, so user story 2 holds only for the Go suite. Posted inline at tests/cmd/stack/main.go:106-108.
  • Partial: "SHELLHUB_LICENSE_FILE and errNoLicense go away." The CI and e2e.sh plumbing is gone. The Go fallback is still there: errNoLicense and licenseFileEnv (tests/environment/license.go:29,40), licenseEnvs (license.go:219-244), the case issuer == nil: branch (licensefile.go:32-35) and TestLicenseEnvs. After this PR, every caller that reaches Up passes an issuer, so the fallback can't be reached outside its own test. The doc comments in run.go:48,99 and stack.go:88 still describe it. These lines are outside the diff, so there's no inline comment. Fix this →
  • Missing: "The RUN that compiles the server gets a --mount=type=cache on the Go build cache." The compile RUN in server/Dockerfile:45-56 has only the cloud-src bind mount. The issuer key is saved under tests/.stack/, so a local run reuses the key and the layer stays cached. CI starts fresh on every run, but the spec already says CI gains nothing from the mount. The mount is cheap to add. If you'd rather not, record on the issue that the persisted key replaces it. server/Dockerfile isn't in this diff, so there's no inline comment.

Additional notes

  • Correctness checks that came back clean:
    • The same issuer signs E2E_EXPIRED_LICENSE and builds the server that trusts it.
    • Teardown's nil-issuer run never reaches Up.
    • Attach and Up agree on the license and GeoIP paths.
    • The ordering in Down is correct for both attached and started stacks.
    • The cloud edition gets the issuer's ldflags.
    • The eval'd exports are base64 only.
    • No routes, pkg/ or server/api/services symbols change, and nothing in cloud/ references the symbols that changed.
  • Dropped judgement calls (low, below the bar):
    • Move expiredLicense into environment.
    • Replace the nil sentinel on printExports.
    • The two-place artifact list, which an earlier round already dropped.

To request another review round, comment /review.

Comment thread tests/cmd/stack/main.go
@otavio
otavio force-pushed the test/e2e/sign-own-license branch from fe23233 to 031ac32 Compare October 9, 2026 11:01
@otavio

otavio commented Oct 9, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Claude finished @otavio's task in 6m 30s —— View job


Code Review Complete

Reviewed 15 files. 2 inline issues posted. Standards: 0 violations, 1 judgement call. Spec: 1 partial (plus 1 already reported). Correctness: 1.

Standards

The one judgement call is Dead Code. In WriteLicenseFile (tests/environment/license_admin.go:128), the require.NotNil on the run's issuer can no longer fail on its own now that the SHELLHUB_LICENSE_FILE fallback is gone. Its message is also wrong in the only cases that reach it. Every remaining doc comment (Up, Attach, Down, StartRun, StartStackRun, LicenseIssuer, LicenseIssuerKeyPath) matches the code.

Correctness

printExports exports E2E_LICENSE_ISSUER_KEY as an unquoted absolute path, and ui/scripts/e2e.sh:82 runs it through eval. A checkout path with a space aborts e2e.sh after the stack is up (tests/cmd/stack/main.go:106-112).

Spec (shellhub-io/team#298)

User stories 1–5 are all met. The CI workflow references neither secret, the cloud edition gets the expired license too, and the key path resolves in the test container, the e2e container and on the host. Two requirements are still open:

  • Partial: "The key lives per process … the process generates one RSA-2048 key pair before the first build" and "The key lives only for that run." stack up calls LoadLicenseIssuer, which creates tests/.stack/license-issuer.pem only when the file is missing and reuses it after that. CI starts clean every time, so each CI run still gets a new key, but on a developer machine the key never changes. This is probably deliberate, because the persisted key is what lets the Go suite and stack up share cached builds. If it is, record on the issue that it replaces the per-process key.
  • Missing (already reported in the previous round): "The RUN that compiles the server gets a --mount=type=cache on the Go build cache." server/Dockerfile hasn't changed.

Additional notes

  • Checks that came back clean:
    • No caller reaches Up for an enterprise or cloud stack with a nil issuer. teardownRun and the run_test.go helpers pass nil but never call Up, and nothing in cloud/ imports tests/environment.
    • Attach/Down can't remove files that belong to a Go suite stack, because those stacks have UUID names.
    • The expired license is signed by the same issuer the server was built with, and it is past the 7-day grace period.
    • Key generation uses crypto/rand. No pkg/ or server/api/services symbols change.
  • Dropped judgement calls (below the bar):
    • That nothing reads E2E_LICENSE_ISSUER_KEY yet. The spec asks for it to be exported, so that one is dropped.
    • That TestLicensingEnvsWithoutIssuer calls the unexported licensingEnvs instead of Up. An earlier round dropped the same point.
  • Spec points left out because test: cover licensing, the firewall and session recording end to end #7390 introduced them, not this branch:
    • The build arg is a general GO_LDFLAGS rather than LICENSE_PUBLIC_KEY.
    • SHELLHUB_LICENSE_FILE is still used internally as the compose variable name.
  • Housekeeping: a validation sub-agent wrote a scratch pr7375.diff at the repo root on the CI runner, and this run wasn't allowed to delete it. It is untracked and only exists on the runner, so it never reaches the branch.

To request another review round, comment /review.

@otavio
otavio force-pushed the test/e2e/sign-own-license branch from 031ac32 to d096e54 Compare October 9, 2026 11:05
Comment thread tests/cmd/stack/main.go Outdated
Comment thread tests/environment/license_admin.go Outdated
@otavio
otavio force-pushed the test/e2e/sign-own-license branch from d096e54 to c5bde4e Compare October 9, 2026 11:11
@otavio

otavio commented Oct 9, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Claude finished @otavio's task in 3m 20s —— View job


Code Review Complete

Reviewed 16 files. 1 inline issue posted. Standards: 1 violation, 0 judgement calls. Spec: 0 new. Correctness: 0.

Standards

This branch rewrote the doc comment on Run.LicenseIssuer (tests/environment/run.go:99-100), but the comment still says only [StartRun] supplies the issuer. The same branch adds an issuer parameter to StartStackRun, so the comment now leaves out the case stack up relies on.

Spec

The Spec axis ran against shellhub-io/team#298.

  • Met: user stories 1–5, and the decisions on the signer, CI plumbing and stack up exports. E2E_EXPIRED_LICENSE expired a year ago, well past the 7-day grace period, and the key path is exported.
  • Still open from earlier rounds:
    • Missing: the server/Dockerfile Go build cache mount.
    • Partial: the key is persisted rather than generated per process.

Additional notes

  • Dropped findings:
    • Spec: the valid license comes from FullLicense() with no ExpiresAt, where the spec says it "expires in a year". That code is from test: cover licensing, the firewall and session recording end to end #7390, not this branch. If the license is meant never to expire, note that on the issue.
    • Judgement calls, below the bar:
      • expiredLicense is signed after the stack is already up.
      • TestPrintExportsSurvivesEval doesn't read back E2E_LICENSE_ISSUER_KEY.
      • The Config doc comment's wording about which stacks need an issuer.
    • Already settled in earlier rounds: nothing consumes E2E_LICENSE_ISSUER_KEY yet.
  • Correctness checks that came back clean:
    • shellQuote is correct for both eval paths (e2e.sh test and up).
    • Creating the issuer key is safe when two runs start at once (temp file, then os.Link, tolerating ErrExist).
    • Attach/Down can't remove files belonging to Go-suite stacks, which have UUID names.
    • Every nil-issuer caller avoids Up.
    • Nothing under pkg/ or server/api/services changes, and nothing in cloud/ references the changed symbols.

To request another review round, comment /review.

Comment thread tests/environment/run.go Outdated
luizhf42 and others added 3 commits October 9, 2026 08:17
The Go suite already runs enterprise stacks under licenses a run issuer signs, but `stack up`
started its run without one, so the Playwright stack still loaded the license
SHELLHUB_LICENSE_FILE named and the license spec read its expired license from the environment.
Both had to be real licenses signed with the production key, and both break when they expire.

`stack up` now loads the issuer from `.stack/license-issuer.pem`, the key the Go suite uses, so
both build the same server and share its cached layers. For an enterprise or cloud stack it also
signs `E2E_EXPIRED_LICENSE`, a license that expired a year ago, well past the 7-day grace
period, and exports it with the rest. `stack down` attaches to the existing stack and builds
nothing, so its run takes no issuer.

It exports `E2E_LICENSE_ISSUER_KEY` too, the key's absolute path, so a spec that needs a license
of its own, past a device limit or bound to another region, can sign one against the stack's
server. The test and e2e containers mount the workspace at the same path, so the path `stack up`
resolves is the one Playwright reads. It is also the first exported value that holds the
checkout's path, and `e2e.sh` evals the export lines, so a space in that path would stop the
script after the stack was up. `printExports` now quotes every value for the shell.

Both runners name the key through `environment.LicenseIssuerKeyPath`. Two literals could drift
apart without failing anything; the two builds would just stop sharing their cache.

The signed license `Up` writes to `.stack/licenses/<project>.dat` is new for these stacks, and
`stack down` left it behind. `Down` removed a stack's license and GeoIP files only when `Up` had
started the stack in the same process, and `stack down` runs in another, reaching the stack
through `Attach`. `Attach` now derives those paths from the project name, the way `Up` does, and
`Down` removes them for an attached stack too.

With `stack up` exporting it, the license spec's own hint for a missing E2E_EXPIRED_LICENSE, to
supply an expired license by hand, now points at a license this run's server rejects as invalid.
The spec falls back to `requireEnv`'s default hint, which leaves `hint` with no caller, so it goes.

Fixes: shellhub-io/team#298
`stack up` signs the stack's license and `E2E_EXPIRED_LICENSE` with the key its server trusts,
so nothing reads `TEST_LICENSE` or `TEST_EXPIRED_LICENSE` any more. Delete both secrets once
this lands.
`stack up` and the Go suite now both start their runs with an issuer, so no enterprise or cloud
stack reaches the branch that loaded the license SHELLHUB_LICENSE_FILE named in the shell or
.env.override. Left in place, it would let a run without an issuer start an enterprise stack
under a license signed with the production key, which is what team#298 removes: such a license
has to be supplied by hand and breaks when it expires.

An enterprise or cloud stack on a run that issues no licenses now fails with
errRunIssuesNoLicense, unless it asks for no license through Unlicensed. `stack down` still
starts its run without an issuer; it attaches to the stack and never signs a license.

`WriteLicenseFile` drops its own check for a run without an issuer. A stack on such a run is now
community or Unlicensed, so the check for a license file right after it already fails.

The variable stays. The compose files mount the license `Up` signs through it, and the dev stack
reads it from .env.override.
@otavio
otavio force-pushed the test/e2e/sign-own-license branch from c5bde4e to dc28332 Compare October 9, 2026 11:53
@otavio

otavio commented Oct 9, 2026

Copy link
Copy Markdown
Member

/review

@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Claude finished @otavio's task in 2m 11s —— View job


Code Review Complete

Reviewed 16 files on all three axes: Standards (repo rules, language conventions, tests, code smells), Spec (does it implement the issue?) and Correctness (security and runtime defects). No new issues found. The code looks good as-is.

@otavio
otavio merged commit 28d4f09 into master Oct 9, 2026
36 checks passed
@otavio
otavio deleted the test/e2e/sign-own-license branch October 9, 2026 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants