Fix spicedb throwing on invalid arguments (#20269)

Author: filiptronicek

components/gitpod-protocol/src/util/gitpod-host-url.ts

@@ -16,7 +16,7 @@ const baseWorkspaceIDRegex =
     "(([a-f][0-9a-f]{7}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})|([0-9a-z]{2,16}-[0-9a-z]{2,16}-[0-9a-z]{8,11}))";
 
 // this pattern matches v4 UUIDs as well as the new generated workspace ids (e.g. pink-panda-ns35kd21)
-const workspaceIDRegex = RegExp(`^(?:debug-)?${baseWorkspaceIDRegex}$`);
+export const workspaceIDRegex = RegExp(`^(?:debug-)?${baseWorkspaceIDRegex}$`);
 
 // this pattern matches URL prefixes of workspaces
 const workspaceUrlPrefixRegex = RegExp(`^(([0-9]{4,6}|debug)-)?${baseWorkspaceIDRegex}\\.`);

components/server/src/api/workspace-service-api.ts

@@ -58,6 +58,15 @@ import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messag
 import { ContextService } from "../workspace/context-service";
 import { UserService } from "../user/user-service";
 import { ContextParser } from "../workspace/context-parser-service";
+import { workspaceIDRegex } from "@gitpod/gitpod-protocol/lib/util/gitpod-host-url";
+
+const isWorkspaceId = (workspaceId?: string) => {
+    if (!workspaceId) {
+        return false;
+    }
+
+    return workspaceIDRegex.test(workspaceId);
+};
 
 @injectable()
 export class WorkspaceServiceAPI implements ServiceImpl<typeof WorkspaceServiceInterface> {
@@ -68,8 +77,8 @@ export class WorkspaceServiceAPI implements ServiceImpl<typeof WorkspaceServiceI
     @inject(ContextParser) private contextParser: ContextParser;
 
     async getWorkspace(req: GetWorkspaceRequest, _: HandlerContext): Promise<GetWorkspaceResponse> {
-        if (!req.workspaceId) {
-            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required");
+        if (!isWorkspaceId(req.workspaceId)) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "a valid workspaceId is required");
         }
         const info = await this.workspaceService.getWorkspace(ctxUserId(), req.workspaceId);
         const response = new GetWorkspaceResponse();
@@ -198,8 +207,8 @@ export class WorkspaceServiceAPI implements ServiceImpl<typeof WorkspaceServiceI
 
     async startWorkspace(req: StartWorkspaceRequest): Promise<StartWorkspaceResponse> {
         // We rely on FGA to do the permission checking
-        if (!req.workspaceId) {
-            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required");
+        if (!isWorkspaceId(req.workspaceId)) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "a valid workspaceId is required");
         }
         const user = await this.userService.findUserById(ctxUserId(), ctxUserId());
         const { workspace, latestInstance: instance } = await this.workspaceService.getWorkspace(
@@ -227,8 +236,8 @@ export class WorkspaceServiceAPI implements ServiceImpl<typeof WorkspaceServiceI
         req: GetWorkspaceDefaultImageRequest,
         _: HandlerContext,
     ): Promise<GetWorkspaceDefaultImageResponse> {
-        if (!req.workspaceId) {
-            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required");
+        if (!isWorkspaceId(req.workspaceId)) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "a valid workspaceId is required");
         }
         const result = await this.workspaceService.getWorkspaceDefaultImage(ctxUserId(), req.workspaceId);
         const response = new GetWorkspaceDefaultImageResponse({
@@ -246,8 +255,8 @@ export class WorkspaceServiceAPI implements ServiceImpl<typeof WorkspaceServiceI
     }
 
     async sendHeartBeat(req: SendHeartBeatRequest, _: HandlerContext): Promise<SendHeartBeatResponse> {
-        if (!req.workspaceId) {
-            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required");
+        if (!isWorkspaceId(req.workspaceId)) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "a valid workspaceId is required");
         }
         const info = await this.workspaceService.getWorkspace(ctxUserId(), req.workspaceId);
         if (!info.latestInstance?.id || info.latestInstance.status.phase !== "running") {
@@ -265,8 +274,8 @@ export class WorkspaceServiceAPI implements ServiceImpl<typeof WorkspaceServiceI
         req: GetWorkspaceOwnerTokenRequest,
         _: HandlerContext,
     ): Promise<GetWorkspaceOwnerTokenResponse> {
-        if (!req.workspaceId) {
-            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required");
+        if (!isWorkspaceId(req.workspaceId)) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "a valid workspaceId is required");
         }
         const ownerToken = await this.workspaceService.getOwnerToken(ctxUserId(), req.workspaceId);
         const response = new GetWorkspaceOwnerTokenResponse();
@@ -278,8 +287,8 @@ export class WorkspaceServiceAPI implements ServiceImpl<typeof WorkspaceServiceI
         req: GetWorkspaceEditorCredentialsRequest,
         _: HandlerContext,
     ): Promise<GetWorkspaceEditorCredentialsResponse> {
-        if (!req.workspaceId) {
-            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required");
+        if (!isWorkspaceId(req.workspaceId)) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "a valid workspaceId is required");
         }
         const credentials = await this.workspaceService.getIDECredentials(ctxUserId(), req.workspaceId);
         const response = new GetWorkspaceEditorCredentialsResponse();
@@ -288,8 +297,8 @@ export class WorkspaceServiceAPI implements ServiceImpl<typeof WorkspaceServiceI
     }
 
     async updateWorkspace(req: UpdateWorkspaceRequest): Promise<UpdateWorkspaceResponse> {
-        if (!req.workspaceId) {
-            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required");
+        if (!isWorkspaceId(req.workspaceId)) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "a valid workspaceId is required");
         }
         if (req.spec?.timeout?.inactivity?.seconds || (req.spec?.sshPublicKeys && req.spec?.sshPublicKeys.length > 0)) {
             throw new ApplicationError(ErrorCodes.UNIMPLEMENTED, "not implemented");
@@ -363,17 +372,17 @@ export class WorkspaceServiceAPI implements ServiceImpl<typeof WorkspaceServiceI
     }
 
     async stopWorkspace(req: StopWorkspaceRequest): Promise<StopWorkspaceResponse> {
-        if (!req.workspaceId) {
-            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required");
+        if (!isWorkspaceId(req.workspaceId)) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "a valid workspaceId is required");
         }
         await this.workspaceService.stopWorkspace(ctxUserId(), req.workspaceId, "stopped via API");
         const response = new StopWorkspaceResponse();
         return response;
     }
 
     async deleteWorkspace(req: DeleteWorkspaceRequest): Promise<DeleteWorkspaceResponse> {
-        if (!req.workspaceId) {
-            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required");
+        if (!isWorkspaceId(req.workspaceId)) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "a valid workspaceId is required");
         }
         await this.workspaceService.deleteWorkspace(ctxUserId(), req.workspaceId, "user");
         const response = new DeleteWorkspaceResponse();
@@ -389,8 +398,8 @@ export class WorkspaceServiceAPI implements ServiceImpl<typeof WorkspaceServiceI
     }
 
     async createWorkspaceSnapshot(req: CreateWorkspaceSnapshotRequest): Promise<CreateWorkspaceSnapshotResponse> {
-        if (!req.workspaceId) {
-            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required");
+        if (!isWorkspaceId(req.workspaceId)) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "a valid workspaceId is required");
         }
         const snapshot = await this.workspaceService.takeSnapshot(ctxUserId(), {
             workspaceId: req.workspaceId,
@@ -410,8 +419,8 @@ export class WorkspaceServiceAPI implements ServiceImpl<typeof WorkspaceServiceI
     }
 
     async updateWorkspacePort(req: UpdateWorkspacePortRequest): Promise<UpdateWorkspacePortResponse> {
-        if (!req.workspaceId) {
-            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "workspaceId is required");
+        if (!isWorkspaceId(req.workspaceId)) {
+            throw new ApplicationError(ErrorCodes.BAD_REQUEST, "a valid workspaceId is required");
         }
         if (!req.port) {
             throw new ApplicationError(ErrorCodes.BAD_REQUEST, "port is required");

components/server/src/authorization/spicedb-authorizer.ts

@@ -15,6 +15,7 @@ import { base64decode } from "@jmondi/oauth2-server";
 import { DecodedZedToken } from "@gitpod/spicedb-impl/lib/impl/v1/impl.pb";
 import { ctxTryGetCache, ctxTrySetCache } from "../util/request-context";
 import { ApplicationError, ErrorCodes } from "@gitpod/gitpod-protocol/lib/messaging/error";
+import { isGrpcError } from "@gitpod/gitpod-protocol/lib/util/grpc";
 
 async function tryThree<T>(errMessage: string, code: (attempt: number) => Promise<T>): Promise<T> {
     let attempt = 0;
@@ -104,6 +105,9 @@ export class SpiceDBAuthorizer {
                 const permitted = response.permissionship === v1.CheckPermissionResponse_Permissionship.HAS_PERMISSION;
                 return { permitted, checkedAt: response.checkedAt?.token };
             } catch (err) {
+                if (isGrpcError(err) && err.code === grpc.status.INVALID_ARGUMENT) {
+                    throw new ApplicationError(ErrorCodes.BAD_REQUEST, `Invalid request for permission check: ${err}`);
+                }
                 error = err;
                 log.error("[spicedb] Failed to perform authorization check.", err, {
                     request: new TrustedValue(req),

components/server/src/workspace/workspace-service.ts

@@ -248,7 +248,7 @@ export class WorkspaceService {
         const res = await this.db.find({
             limit: 20,
             ...options,
-            userId, // gpl: We probably want to removed this limitation in the future, butkeeping the old behavior for now due to focus on FGA
+            userId, // gpl: We probably want to removed this limitation in the future, but keeping the old behavior for now due to focus on FGA
             includeHeadless: false,
         });