Skip to content

Commit

Permalink
Fix spicedb throwing on invalid arguments (#20269)
Browse files Browse the repository at this point in the history
  • Loading branch information
@filiptronicek
filiptronicek authored Oct 7, 2024
1 parent dae1529 commit 3f0fc73
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 24 deletions.
2 changes: 1 addition & 1 deletion components/gitpod-protocol/src/util/gitpod-host-url.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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}\\.`);
Expand Down
53 changes: 31 additions & 22 deletions components/server/src/api/workspace-service-api.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Expand All @@ -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();
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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({
Expand All @@ -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") {
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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");
Expand Down Expand Up @@ -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();
Expand All @@ -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,
Expand All @@ -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");
Expand Down
4 changes: 4 additions & 0 deletions components/server/src/authorization/spicedb-authorizer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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),
Expand Down
2 changes: 1 addition & 1 deletion components/server/src/workspace/workspace-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
});

Expand Down

0 comments on commit 3f0fc73

Please sign in to comment.