fix(server): let a web terminal token open only one terminal by geovannewashington · Pull Request #7370 · shellhub-io/shellhub

geovannewashington · 2026-10-07T14:27:50Z

Summary

A web terminal token now opens one terminal. Before, the bridge read the token without removing it, so it kept working until its 30 second timer deleted it.

 POST /ws/ssh/session
-  creds.encryptPassword()              error ignored, password parked as plaintext
+  if creds.encryptPassword() fails
+    400 if the password is over 190 bytes, 500 otherwise
   manager.save(T, creds)

 WebSocket /ws/ssh?token=T
+  conn := NewConn(wsconn)              error frames now take Conn's write lock
   validate token, dimensions, IP
-  creds := manager.Load(T)             T stays usable for 30s
+  creds := manager.LoadAndDelete(T)    first caller wins, T is gone
   go conn.KeepAlive()
-  creds.decryptPassword()              error ignored
+  if creds.decryptPassword() fails
+    exit with ErrBridgeDecryptPassword and the cause
   newSession(creds)

Key and identity logins carry no password. Encrypting or decrypting an empty password now does nothing, so they keep working. The ErrCreditialsNoPassword sentinel only existed for callers to filter out, so it is gone.

The encrypt check was not in the spec. The magic key is RSA-2048 with OAEP and SHA-256, which fits at most 190 bytes. A longer password was parked as plaintext, and the new decrypt check would end the bridge on it. Refusing it when the token is created gives the browser a clear error up front.

The bridge test exposed a race that was already there. exit wrote the error frame straight to the WebSocket, while KeepAlive pinged through Conn's lock, so the two frames could interleave. exit now writes through Conn. That fix has its own commit.

TestManagerSave polled with get, which now spends the token on the first poll, so it passed even if tokens never expired. It now waits past the TTL and reads once.

Closes shellhub-io/team#292

Evidence

Before: TestManagerGetRedeemsTheTokenOnce fails because the second get still finds the token. TestManagerConcurrentGetsYieldOneWinner gets 50 winners out of 50. TestNewSSHServerBridge_TokenOpensOneBridge lets the second dial reach GetDevice again, and -race reports the exit/KeepAlive race. TestWebSessionRouteRefusesAPasswordItCannotEncrypt gets 200 for a 191-byte password.
After: all four pass. go test -race ./ssh/web/ passes on every commit, and the full server suite passes except store/pg, a testcontainers suite that cannot run under exec server. This branch does not touch it. With the TTL delete removed from save, TestManagerSave now fails.

token minted with a fingerprint login
first dial   -> ErrGetAuth                    key login got past the decrypt step
second dial  -> ErrBridgeCredentialsNotFound  token already spent

session request with a 191-byte password -> 400, rsa.ErrMessageTooLong

Merge Danger

Door: two-way

There is no wire format or database change. Reverting restores the old behaviour.

Blast Radius: web-terminal

A browser that reused a token now gets "failed to find the credentials". The console mints a new token on every connect, including reconnects, so normal use does not change.

A device password over 190 bytes logged in on master, by accident, through the plaintext fallback. POST /ws/ssh/session now refuses it with 400. Those users cannot open a web terminal with password login until the password is shorter or the encryption changes.

github-actions · 2026-10-07T14:28:15Z

Claude encountered an error after 0s —— View job

I'll analyze this and get back to you.

github-actions · 2026-10-07T16:37:31Z

Claude encountered an error after 1s —— View job

I'll analyze this and get back to you.

github-actions · 2026-10-07T16:44:43Z

Claude finished @geovannewashington's task in 4m 26s —— View job

Code Review Complete

Reviewed 5 files and posted 2 inline issues: no Standards violations, 1 Standards judgement call, and 1 Correctness issue. The Spec axis did not run.

Standards

There are no hard violations. The one judgement call is on web_test.go:124-128. The new TestExitLogLevel entry tests nothing the existing ones don't, and no bridge test covers the new decrypt-failure exit. Deleting the check at web.go:191 would leave every test passing.

Correctness

web.go:191-195 breaks web terminal logins with passwords over 190 bytes, which work on master. RSA-2048 with OAEP and SHA-256 can't encrypt them, and web.go:132 ignores that encrypt error, so the password is parked in plaintext. On master the decrypt failure was ignored and the plaintext login went through. This branch now ends the bridge with ErrBridgeDecryptPassword. The fix belongs at the encrypt call: reject the session request there.

Spec

The Spec axis did not run. The PR's only issue reference is Fixes: shellhub-io/team#292, and this run can't read issues in that repository.

Additional notes

TestNewSSHServerBridge_TokenOpensOneBridge gets ErrGetAuth only because getAuth calls GetDevice exactly once more. I dropped this as below the bar, but the test will break if that lookup changes.
The KeepAlive goroutine keeps running after Close on both the new decrypt exit and the existing newSession exit, because Ticker.Stop doesn't close the channel. That leak predates this PR, so I didn't flag it. Starting go conn.KeepAlive() after the decrypt check would at least keep the new path from leaking.
Nothing under cloud/ uses server/ssh/web, so there is no cross-repo impact.

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

gustavosbarreto · 2026-10-07T17:16:55Z

/review

github-actions · 2026-10-07T17:17:19Z

Claude finished @gustavosbarreto's task in 3m 28s —— View job

Code Review Complete

Reviewed 6 files and posted 1 inline issue. Standards: no violations and 1 judgement call. Spec: did not run. Correctness: no findings.

Standards

The one judgement call is in errors.go:49-50. The ErrBridgeDecryptPassword doc comment was not updated when d979b8a joined the cause onto the error. The sibling ErrCredentialsEncryptPassword comment does say "joined with the cause", so the two now disagree.

Spec

The Spec axis did not run. The only issue reference is Fixes: shellhub-io/team#292 (in f76c6a6), and this run cannot read issues in that repository.

Additional notes

Both fixes from the previous round hold. The session route now returns 400 on rsa.ErrMessageTooLong and 500 on any other encrypt error. errors.Is matches rsa.ErrMessageTooLong through the errors.Join, and TestWebSessionRouteRefusesAPasswordItCannotEncrypt pins the 400.
Correctness found nothing new. Specifically:
- LoadAndDelete leaves exactly one winner per token.
- The token is spent only after the token, dimensions and IP checks pass.
- Opening conn earlier removes the old double close of wsconn.
- Key logins still pass the decrypt step.
- Making the token single-use also removes an older race, where two bridges on one token both decrypted the same *Credentials.
Four judgement calls fell below the bar and were dropped:
- helper duplication with the existing permission test, which predates this branch
- the sequential manager test overlapping the bridge test
- an assertion on the stdlib error text
- no 190-byte boundary case
Nothing under cloud/ imports server/ssh/web.

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

gustavosbarreto · 2026-10-07T17:56:07Z

/review

github-actions · 2026-10-07T17:56:29Z

Claude finished @gustavosbarreto's task in 4m 5s —— View job

Code Review Complete

Reviewed 6 files and posted 2 inline issues. Standards: no violations and 1 judgement call. Spec: did not run. Correctness: 1 finding.

Standards

The one judgement call is at web.go:201 and web.go:133. Both call sites now repeat err != nil && !errors.Is(err, ErrCreditialsNoPassword), and the sentinel exists only to be filtered back out. If encryptPassword and decryptPassword returned nil when there is no password, both checks and the sentinel could go.

Correctness

Moving get to LoadAndDelete (manager.go:29) left the existing TestManagerSave unable to fail. Its poll consumes the token itself, so the test passes even if tokens never expire. The comment is on manager.go:29 because the test's lines are not in the diff. The fix is to poll with manager.credentials.Load at manager_test.go:46.

Spec

The Spec axis did not run. The only issue reference is Fixes: shellhub-io/team#292 (in 18fcc52), and this run cannot read issues in that repository.

Additional notes

The production code had no new correctness findings. Specifically:
- The expiry timer's late Delete on a token that was already used does nothing, and token IDs are UUIDs, so a late timer can't remove a newer token.
- exit writes through conn before KeepAlive starts, and Close runs once.
- No password material reaches the HTTP body or the WebSocket error frame.
- The session route still requires DeviceConnect.
- newSession still gets the request context.
Nothing under cloud/ imports server/ssh/web.

If you've addressed the feedback and want a new review, tag @shellhub-io/admin and a team member can trigger it.

The token store read a token without removing it, so the token kept opening terminals until its 30 second timer deleted it. Reading and deleting in one atomic step leaves exactly one caller with it. TestManagerSave now waits past the TTL and reads once. Polling with get spent the token on the first poll, so the test passed even if tokens never expired.

The bridge wrote its error frame straight to the WebSocket while the keep-alive goroutine pinged through Conn's lock, so the two frames could interleave. Writing the error through Conn serializes them.

The bridge ignored the decrypt error and logged in with whatever the password field held. Key and identity logins carry no password, so encrypting or decrypting an empty password now does nothing. The ErrCreditialsNoPassword sentinel existed only for callers to filter out, so it is gone. The session route also ignored the encrypt error, so a password over the 190 bytes RSA-OAEP fits was parked as plaintext and then failed to decrypt. The route now refuses it with 400, and answers any other encrypt failure, a server fault, with 500. Fixes: shellhub-io/team#292

…rypt The log could not tell a hex decode failure from an RSA one, because the bridge reported only its own sentinel.

The first redemption only asserted it was not "credentials not found", which any other error also satisfied. Expecting ErrGetAuth proves the bridge reached authentication. The not-found test now shares the dial helper. The second GetDevice stub is not for the second dial. getAuth looks the device up again for a key login, and that miss is what the first dial reports as ErrGetAuth. The second dial never reaches GetDevice.

gustavosbarreto · 2026-10-07T18:24:05Z

/review

github-actions · 2026-10-07T18:24:35Z

Code Review Complete

The automated review ran but did not post an updated summary — this usually means no new issues were found since the previous review. If you've pushed changes and want a fresh pass, comment /review.

View job

geovannewashington requested a review from a team as a code owner October 7, 2026 14:27

geovannewashington self-assigned this Oct 7, 2026