fix(server): answer the generic forbidden error with 403

[geovannewashington]

Summary

services.ErrForbidden was declared with the not-found code, so every refusal that used it answered 404 with {"message":"forbidden"}. It now uses the forbidden code, like every other forbidden error in the file.

-ErrForbidden = errors.New("forbidden", ErrLayer, ErrCodeNotFound)
+ErrForbidden = errors.New("forbidden", ErrLayer, ErrCodeForbidden)

The refusals that move from 404 to 403:

• POST /api/ssh-approvals/:code/confirm on a reauth approval, the issue's example
• a user or API key acting on an SSH identity that is not theirs
• BoundTo with no tenant, a backstop behind the tenant guard
• in cloud, without a code change: the SAML re-auth step-up and the community-edition SAML configuration refusal

The web terminal re-auth step-up is the exception. The console reads a 403 from POST /api/web-terminal/reauth as a wrong password or code, so its other refusals now use not-found errors and keep answering 404:

 StampWebReauth
   key is not one of the caller's identities
-    ErrForbidden                 (would be 403: "Incorrect password.")
+    ErrSSHIdentityNotFound       (404)
   approval is another tenant's, another key's, or not a re-auth
-    ErrForbidden                 (would be 403, and shows a live code exists)
+    ErrSSHApprovalCodeNotFound   (404, same as an unknown code)

This goes against the spec's line that the step-up's ownership checks answer 403. Keeping 403 there would show "Incorrect password." to a user who typed the correct one. It also applies to cloud's TOTP and SAML step-ups, which call StampWebReauth.

OpenAPI: confirm and reject now list 403, and web terminal re-auth now lists 404. Each description says which refusals answer which status. Reject already returned 403 from its role check, and re-auth already returned 404 for an unknown code, without documenting either.

Evidence

• Before: the new error handler case fails.

  handle(services.ErrForbidden)
    expected: 403
    actual  : 404
  

  The re-auth service cases for another member's key, an unknown key, and an approval for another key, of another kind, or from another namespace fail with ErrForbidden.

  After: the handler answers 403 with {"message":"forbidden"}, and those re-auth cases return ErrSSHIdentityNotFound or ErrSSHApprovalCodeNotFound. The full server suite passes. redocly lint reports the community, cloud and enterprise specs as valid.

Merge Danger

Door: two-way

It changes the status code of existing errors, with no migration, no wire format and no agent contract. Reverting restores the previous statuses.

Blast Radius: API clients

A client that treated these 404s as "not found" now gets 403. On the re-auth step-up, the console keeps "Incorrect password." and "Invalid code" for a failed factor only, and shows "Re-authentication failed." for the other refusals, as before. Cloud's OpenAPI entries for the SAML flows are a follow-up.

Closes #7277

[github-actions]

Claude finished @geovannewashington's task in 5m 14s —— View job

---

Code Review Complete

Reviewed 4 files. 2 inline issues posted. Standards: 0 violations, 0 judgement calls. Spec: 0. Correctness: 2.

Standards

No findings. The one-line sentinel fix, the new row in the handler test table and the two OpenAPI edits all follow the repo rules, go-conventions and tdd. The fix commit carries Fixes: shellhub-io/shellhub#7277.

Correctness

The worst issue is at server/api/services/errors.go:78. The web re-auth fingerprint ownership check and the approval mismatch check both use ErrForbidden, so they now answer 403. That is the same status as a wrong password. SSHApproval.tsx:490-495 chooses its message from the status alone, so a user who typed the right password now sees "Incorrect password." The PR's Blast Radius section says the console is unaffected, which is not true for this path.

The second, lower-impact issue: releaseSSHApproval now answers 403 instead of 404 for a live approval code from another tenant. That tells the caller the code exists, which contradicts the new OpenAPI text saying non-member codes return 404.

Spec

Every requirement of #7277 is met:

• ErrForbidden now uses the forbidden code.
• Confirm and reject now list 403, and their descriptions match decideSSHApproval.
• The error handler test has the planned new row.
• No cloud code needed to change.

I checked every other caller (web-reauth.go, ssh-identity.go, scope.go) and none reaches a route that doesn't already list 403. Nothing outside the spec was changed.

---

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

[gustavosbarreto]

/review

[github-actions]

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

---

Code Review Complete

Reviewed 7 files. 1 inline issue posted. Standards: 0 violations, 1 judgement call. Spec: 0. Correctness: 0.

Standards

Judgement call at server/api/services/web-reauth.go:50-51: the new StampWebReauth doc comment says "a 403 from the step-up only ever means a failed factor". That isn't true for the empty-tenant scope.NewBounded path at line 55, which still returns ErrForbidden and now answers 403. That path is reachable from a session with no tenant, and the console then shows "Incorrect password." for a correct password. The new OpenAPI sentence in api@web-terminal@reauth.yaml:18 has the same gap.

Spec

The PR does everything #7277 asks for:

• ErrForbidden now uses ErrCodeForbidden.
• The issue's example, confirming a reauth approval, now answers 403.
• Confirm and reject both document 403.

The issue also wanted the StampWebReauth ownership check to answer "not allowed". The PR keeps it at 404 on purpose, so the console doesn't show "Incorrect password." to a user who typed the right one. The PR body explains this and the earlier review thread settled it, so I'm not raising it again.

Additional notes

• I did not repeat the two findings from the previous round. Both are fixed: the ownership check and the approval mismatch now return not-found errors.
• Cross-repo: I checked cloud's SAML step-up, the TOTP step-up, ConfigureSAMLAuthentication and the cloud tests. Nothing breaks. The SAML handler turns every error into ?status=error, and no cloud test asserts a status for these errors.
• Store failures: wrapping the store error in NewErrSSHIdentityNotFound(req.Fingerprint, err) does not hide a database failure behind a 404. store.ErrInternal is checked first and still answers 500. That's an improvement on the old code, which dropped the error.

---

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

[otavio]

/review

[github-actions]

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

---

Code Review Complete

Reviewed 7 files. 1 inline issue posted. Standards: 0 violations, 1 judgement call. Spec: 0. Correctness: 0.

Standards

Judgement call at server/api/services/web-reauth_test.go:35-36: the test tables now record failure twice, as expectedErr bool and wantErr error. Because of that, the "user does not exist" case still passes on any error. Pin ErrUserNotFound and drop the bool.

Spec

The PR does everything #7277 asks for:

• ErrForbidden is declared with ErrCodeForbidden.
• Confirming a reauth approval now answers 403.
• The new handler test row checks the 403 and its message.

The StampWebReauth refusals are kept at 404 on purpose. That was settled in earlier rounds, so I'm not raising it again.

Additional notes

• Previous round: the empty-tenant finding is fixed. scope.NewBounded("") now returns ErrSSHIdentityNotFound, and the new "no tenant" test case covers it.
• Wider 404 → 403 effect: the change also reaches BoundTo and the ssh-identity API-key, rename and delete ownership checks. Their OpenAPI files already list 403.
  • One side effect: on GET/POST /api/namespaces/api-key/:name/ssh-identities, an API key caller can now tell another key's name in its own namespace (403) from a name that doesn't exist (404).
  • The issue asks for ownership checks to say "not allowed", and this only happens inside the caller's own namespace, so I'm not filing it as a finding. If you'd rather hide key names, return NewErrAPIKeyNotFound there instead.
• Cross-repo: the cloud SAML step-up turns every error into a status=error redirect, and ConfigureSAMLAuthentication's tests compare error values rather than statuses. Nothing in cloud breaks.

---

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

[otavio]

/review

[github-actions]

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

---

Code Review Complete

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

Standards

No findings. The new StampWebReauth doc comment states the contract (which sentinel is returned for which refusal), so it qualifies for the code-style.md exemption. The test tables now pin each refusal with require.ErrorIs against a sentinel. Each commit message gives its reason, and the fix commit carries Fixes: shellhub-io/shellhub#7277.

Correctness

The new description in openapi/spec/paths/api@ssh-approvals@{code}@confirm.yaml:10-13 says already-decided codes return 404, but an already-decided reauth code returns 403. decideSSHApproval refuses the reauth kind before the claim step that produces the already-decided 404. The role checks run in the same order, so reject's new sentence is imprecise for the same reason.

Spec

The PR does everything #7277 asks for:

• "Fix: declare ErrForbidden with ErrCodeForbidden" (errors.go:78), pinned by the new handler test row (403, {"message":"forbidden"}).
• The issue's example, confirm on a kind=reauth approval, now answers 403.

The StampWebReauth refusals stay at 404 on purpose. That was settled in earlier rounds, so I'm not raising it again.

Additional notes

• Previous round: the duplicated expectedErr/wantErr finding is fixed in cf4548c. Both tables decide failure by wantErr alone, and the user-not-found case pins ErrUserNotFound.
• Cross-repo: I checked cloud again. SAMLStepUp turns every error into a status=error redirect. TOTP failure still returns ErrUnauthorizedMFA (403), which matches the "403 means a failed factor" contract. ConfigureSAMLAuthentication's tests compare sentinels. Nothing in cloud breaks.

---

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

[otavio]

/review

[github-actions]

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

---

Code Review Complete

Reviewed 7 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.

If you push additional changes and want a new review, tag @shellhub-io/admin and a team member can trigger it.