Skip to content

fix: panic when caling api with root key for disabled workspace#4083

Merged
mcstepp merged 3 commits intomainfrom
eng-2112-fix-nilpointer-exception-when-workspace-is-disabled
Oct 7, 2025
Merged

fix: panic when caling api with root key for disabled workspace#4083
mcstepp merged 3 commits intomainfrom
eng-2112-fix-nilpointer-exception-when-workspace-is-disabled

Conversation

@Flo4604
Copy link
Member

@Flo4604 Flo4604 commented Oct 7, 2025

What does this PR do?

Fixes #4082

When using an API key for a disabled workspace we shall not panic anymore, + adds tests for all root key statuses

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • Enhancement (small improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How should this be tested?

Create Root Key, in your local db set enabled to 0 for your workspace

Call any API you should get a nice error message saying the workspace is disabled.

Checklist

Required

  • Filled out the "How to test" section in this PR
  • Read Contributing Guide
  • Self-reviewed my own code
  • Commented on my code in hard-to-understand areas
  • Ran pnpm build
  • Ran pnpm fmt
  • Checked for warnings, there are none
  • Removed all console.logs
  • Merged the latest changes from main onto my branch with git pull origin main
  • My changes don't cause any responsiveness issues

Appreciated

  • If a UI change was made: Added a screen recording or screenshots to this PR
  • Updated the Unkey Docs if changes were necessary

@linear
Copy link

linear bot commented Oct 7, 2025

@changeset-bot
Copy link

changeset-bot bot commented Oct 7, 2025

⚠️ No Changeset found

Latest commit: 78df3ba

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Oct 7, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
dashboard Ready Ready Preview Comment Oct 7, 2025 4:07pm
1 Skipped Deployment
Project Deployment Preview Comments Updated (UTC)
engineering Ignored Ignored Preview Oct 7, 2025 4:07pm

@github-actions
Copy link
Contributor

github-actions bot commented Oct 7, 2025

Thank you for following the naming conventions for pull request titles! 🙏

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 7, 2025

📝 Walkthrough

Walkthrough

Adds integration tests for API v2 root-key verification, updates Keys service to construct KeyVerifier inline (fixing nil-pointer when workspace is disabled), adds ForWorkspaceID to seed utilities to target keys to workspaces, and introduces a minor formatting change in status handling.

Changes

Cohort / File(s) Summary
Integration tests: key verification
go/apps/api/integration/root_keys/root_keys_test.go
New end-to-end tests for POST /v2/keys.verifyKey covering valid, missing, disabled, expired, soft-deleted, workspace-mismatch, and insufficient-permission cases; seeds state and asserts status codes and responses.
Key service construction tweak
go/internal/services/keys/get.go
Replaces reused KeyVerifier with locally constructed &KeyVerifier{...} instances in the workspace-disabled and subsequent paths, passing richer context (session, rBAC, region, logger, rate/usage limiters, AuthorizedWorkspaceID, Key) to avoid nil-pointer issues.
Status formatting
go/internal/services/keys/status.go
Minor formatting: adds a blank line in ToFault for StatusInsufficientPermissions. No behavioral change.
Seed utilities: workspace targeting
go/pkg/testutil/seed/seed.go
Adds ForWorkspaceID *string to key creation structs and persists it to DB via sql.NullString (using ptr.SafeDeref), allowing seeding of workspace-bound keys.
CLI flags
go/cmd/ctrl/main.go
Removes the database-hydra CLI flag from the control-plane command flags.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant API as API v2 (/v2/keys.verifyKey)
  participant KeysSvc as Keys Service
  participant DB

  Client->>API: POST /v2/keys.verifyKey (Authorization: RootKey ...)
  API->>KeysSvc: VerifyKey(request)
  KeysSvc->>DB: Lookup key (+ForWorkspaceID) and workspace
  DB-->>KeysSvc: Key, Workspace
  alt Workspace disabled
    note right of KeysSvc #D9EDF7: Construct KeyVerifier inline\n(with full context)
    KeysSvc-->>API: 403 DisabledWorkspace
  else Key missing / soft-deleted
    KeysSvc-->>API: 401 Unauthorized
  else Key disabled / expired
    KeysSvc-->>API: 403 Forbidden
  else Workspace not found
    KeysSvc-->>API: 404 NotFound
  else Insufficient permissions
    KeysSvc-->>API: 403 Forbidden
  else Valid
    KeysSvc-->>API: 200 OK (+key data)
  end
  API-->>Client: HTTP response
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

Core Team

Suggested reviewers

  • imeyer
  • chronark
  • perkinsjr

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Out of Scope Changes Check ⚠️ Warning The PR includes a removal of the “database-hydra” CLI flag in go/cmd/ctrl/main.go which is unrelated to fixing nil pointer panics for disabled workspaces and does not align with any objectives from issue #4082. Please revert or isolate the removal of the “database-hydra” flag in a separate PR, ensuring this change set only contains fixes for the disabled-workspace panic and its associated tests.
✅ Passed checks (4 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly summarizes the primary bug fix—preventing a panic when using a root key on a disabled workspace—even though there is a minor typo in “caling” and casing inconsistency with “api.”
Linked Issues Check ✅ Passed The PR implements the fix for #4082 by updating the KeyVerifier initialization to avoid nil pointer dereferences on disabled workspaces and adding integration tests that assert a 403 response when using root keys on disabled workspaces, fulfilling the issue’s requirement to prevent panics and return an appropriate error.
Description Check ✅ Passed The PR description follows the repository template with a clear summary including “Fixes #4082,” a marked bug-fix type, detailed testing instructions, and a checklist section, even though the checklist items remain unticked.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch eng-2112-fix-nilpointer-exception-when-workspace-is-disabled

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d82b58b and 78df3ba.

📒 Files selected for processing (2)
  • go/cmd/ctrl/main.go (0 hunks)
  • go/internal/services/keys/get.go (2 hunks)
💤 Files with no reviewable changes (1)
  • go/cmd/ctrl/main.go
🧰 Additional context used
🧬 Code graph analysis (1)
go/internal/services/keys/get.go (2)
go/internal/services/keys/verifier.go (1)
  • KeyVerifier (32-56)
go/internal/services/keys/status.go (1)
  • StatusWorkspaceDisabled (23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Build / Build
  • GitHub Check: Test API / API Test Local
  • GitHub Check: Test Go API Local / Test
🔇 Additional comments (2)
go/internal/services/keys/get.go (2)

112-127: Well done fixing the nil pointer panic!

This properly addresses the issue from the past review. By populating Key, AuthorizedWorkspaceID, and isRootKey, you prevent the nil pointer dereference in GetRootKey (line 39) that was causing the panic. Additionally, including session, logger, clickhouse, and other services enables proper telemetry via kv.log for disabled workspace access attempts, preserving the workspace context that was previously lost.


182-201: LGTM—consistent initialization pattern.

The inline construction of KeyVerifier with all necessary fields matches the pattern used in the disabled workspace path (lines 112-127), improving code consistency. All required context (Key, services, session, logger) is properly initialized.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 48fc43e and d82b58b.

📒 Files selected for processing (4)
  • go/apps/api/integration/root_keys/root_keys_test.go (1 hunks)
  • go/internal/services/keys/get.go (2 hunks)
  • go/internal/services/keys/status.go (1 hunks)
  • go/pkg/testutil/seed/seed.go (2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-21T12:37:40.996Z
Learnt from: Flo4604
PR: unkeyed/unkey#3821
File: apps/dashboard/lib/trpc/routers/key/updateRootKeyPermissions.ts:74-74
Timestamp: 2025-08-21T12:37:40.996Z
Learning: Root keys in Unkey have two workspace fields: `workspaceId` (always set to env().UNKEY_WORKSPACE_ID for the Unkey workspace that owns the key) and `forWorkspaceId` (set to ctx.workspace.id for the user's workspace that the key is for). When querying root keys, the system filters by forWorkspaceId to get keys for the current user's workspace, but the returned rootKey.workspaceId is always the Unkey workspace ID.

Applied to files:

  • go/pkg/testutil/seed/seed.go
🧬 Code graph analysis (3)
go/pkg/testutil/seed/seed.go (2)
go/pkg/db/types/null_string.go (1)
  • NullString (10-10)
go/pkg/ptr/deref.go (1)
  • SafeDeref (35-44)
go/apps/api/integration/root_keys/root_keys_test.go (5)
go/pkg/testutil/seed/seed.go (3)
  • Resources (23-28)
  • CreateApiRequest (84-92)
  • CreateKeyRequest (188-208)
go/apps/api/integration/http.go (1)
  • CallRandomNode (104-111)
go/pkg/db/key_update.sql_generated.go (1)
  • UpdateKeyParams (51-70)
go/pkg/db/workspace_update_enabled.sql_generated.go (1)
  • UpdateWorkspaceEnabledParams (19-22)
go/pkg/db/key_soft_delete_by_id.sql_generated.go (1)
  • SoftDeleteKeyByIDParams (17-20)
go/internal/services/keys/get.go (2)
go/internal/services/keys/verifier.go (1)
  • KeyVerifier (32-56)
go/internal/services/keys/status.go (1)
  • StatusWorkspaceDisabled (23-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: Test Go API Local / Test
  • GitHub Check: Test API / API Test Local
  • GitHub Check: Build / Build

@coderabbitai coderabbitai bot requested a review from chronark October 7, 2025 16:05
@Flo4604
Copy link
Member Author

Flo4604 commented Oct 7, 2025

I changed it, that we now log the workspace id of the disabled workspace.

Copy link
Collaborator

@mcstepp mcstepp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks pretty solid and address the root issue, and there's test coverage. nothing major jumping out

@graphite-app
Copy link

graphite-app bot commented Oct 7, 2025

Celebrity gif. A young Keanu Reeves stands in the rain smiling. He raises up his arm and gives an enthusiastic thumbs up. (Added via Giphy)

@mcstepp mcstepp added this pull request to the merge queue Oct 7, 2025
@graphite-app
Copy link

graphite-app bot commented Oct 7, 2025

Graphite Automations

"Post a GIF when PR approved" took an action on this PR • (10/07/25)

1 gif was posted to this PR based on Andreas Thomas's automation.

Merged via the queue into main with commit 3dff665 Oct 7, 2025
17 checks passed
@mcstepp mcstepp deleted the eng-2112-fix-nilpointer-exception-when-workspace-is-disabled branch October 7, 2025 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix nilpointer exception when workspace is disabled

3 participants