Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/cloud/src/CloudAPI.ts
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ export class CloudAPI {
async shareTask(taskId: string, visibility: ShareVisibility = "organization"): Promise<ShareResponse> {
this.log(`[CloudAPI] Sharing task ${taskId} with visibility: ${visibility}`)

// The server should validate that the authenticated user owns this task
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could this comment be more specific? For example: 'The server MUST validate that the authenticated user (from session token) matches the task's owner_id field'

// by checking the session token's user ID against the task's owner
const response = await this.request("/api/extension/share", {
method: "POST",
body: JSON.stringify({ taskId, visibility }),
Expand Down
1 change: 1 addition & 0 deletions packages/cloud/src/bridge/BridgeOrchestrator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ export class BridgeOrchestrator {
instanceId: this.instanceId,
appProperties: this.appProperties,
gitProperties: this.gitProperties,
userId: this.userId,
})
}

Expand Down
12 changes: 9 additions & 3 deletions packages/cloud/src/bridge/TaskChannel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,9 @@ type TaskEventMapping = {
createPayload: (task: TaskLike, ...args: any[]) => any // eslint-disable-line @typescript-eslint/no-explicit-any
}

// eslint-disable-next-line @typescript-eslint/no-empty-object-type
interface TaskChannelOptions extends BaseChannelOptions {}
interface TaskChannelOptions extends BaseChannelOptions {
userId?: string
}

/**
* Manages task-level communication channels.
Expand All @@ -41,6 +42,7 @@ export class TaskChannel extends BaseChannel<
private subscribedTasks: Map<string, TaskLike> = new Map()
private pendingTasks: Map<string, TaskLike> = new Map()
private taskListeners: Map<string, Map<TaskBridgeEventName, TaskEventListener>> = new Map()
private readonly userId?: string

private readonly eventMapping: readonly TaskEventMapping[] = [
{
Expand Down Expand Up @@ -74,6 +76,7 @@ export class TaskChannel extends BaseChannel<

constructor(options: TaskChannelOptions) {
super(options)
this.userId = options.userId
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we validate that userId is a non-empty string when provided? An empty string could cause issues downstream.

}

protected async handleCommandImplementation(command: TaskBridgeCommand): Promise<void> {
Expand Down Expand Up @@ -160,7 +163,10 @@ export class TaskChannel extends BaseChannel<
public async subscribeToTask(task: TaskLike, _socket: Socket): Promise<void> {
const taskId = task.taskId

await this.publish(TaskSocketEvents.JOIN, { taskId }, (response: JoinResponse) => {
// Include userId in the join request for server-side validation
const joinPayload = this.userId ? { taskId, userId: this.userId } : { taskId }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Critical: This client-side validation is like putting a 'Please Don't Enter' sign on an unlocked door. Without server-side validation, this provides only a false sense of security. The PR description correctly notes this limitation.


await this.publish(TaskSocketEvents.JOIN, joinPayload, (response: JoinResponse) => {
if (response.success) {
console.log(`[TaskChannel#subscribeToTask] subscribed to ${taskId}`)
this.subscribedTasks.set(taskId, task)
Expand Down
126 changes: 126 additions & 0 deletions packages/cloud/src/bridge/__tests__/TaskChannel.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -403,4 +403,130 @@ describe("TaskChannel", () => {
errorSpy.mockRestore()
})
})

describe("User ID Validation", () => {
it("should include userId in JOIN payload when userId is provided", async () => {
const userId = "test-user-123"
const channelWithUserId = new TaskChannel({
instanceId,
appProperties,
userId,
})

// Mock the publish method to capture the payload
let capturedPayload: any = null
const channel = channelWithUserId as any
channel.publish = vi.fn((event: string, data: any, callback?: Function) => {
if (event === TaskSocketEvents.JOIN) {
capturedPayload = data
if (callback) {
callback({ success: true })
}
}
return true
})

await channelWithUserId.onConnect(mockSocket)
await channel.subscribeToTask(mockTask, mockSocket)

// Verify the JOIN payload includes userId
expect(capturedPayload).toEqual({
taskId,
userId,
})
})

it("should not include userId in JOIN payload when userId is not provided", async () => {
// Mock the publish method to capture the payload
let capturedPayload: any = null
const channel = taskChannel as any
channel.publish = vi.fn((event: string, data: any, callback?: Function) => {
if (event === TaskSocketEvents.JOIN) {
capturedPayload = data
if (callback) {
callback({ success: true })
}
}
return true
})

await taskChannel.onConnect(mockSocket)
await channel.subscribeToTask(mockTask, mockSocket)

// Verify the JOIN payload does not include userId
expect(capturedPayload).toEqual({
taskId,
})
})

it("should handle subscription failure when user is not authorized", async () => {
const userId = "unauthorized-user"
const channelWithUserId = new TaskChannel({
instanceId,
appProperties,
userId,
})

// Mock the publish method to simulate authorization failure
const channel = channelWithUserId as any
channel.publish = vi.fn((event: string, data: any, callback?: Function) => {
if (event === TaskSocketEvents.JOIN && callback) {
// Simulate authorization failure
callback({
success: false,
error: "User not authorized to access this task",
})
}
return true
})

const errorSpy = vi.spyOn(console, "error")

await channelWithUserId.onConnect(mockSocket)
await channel.subscribeToTask(mockTask, mockSocket)

// Verify error was logged
expect(errorSpy).toHaveBeenCalledWith(
`[TaskChannel#subscribeToTask] failed to subscribe to ${taskId}: User not authorized to access this task`,
)

// Verify task was not added to subscribedTasks
expect(channel.subscribedTasks.has(taskId)).toBe(false)

errorSpy.mockRestore()
})

it("should successfully subscribe when user is authorized", async () => {
const userId = "authorized-user"
const channelWithUserId = new TaskChannel({
instanceId,
appProperties,
userId,
})

// Mock the publish method to simulate successful authorization
const channel = channelWithUserId as any
channel.publish = vi.fn((event: string, data: any, callback?: Function) => {
if (event === TaskSocketEvents.JOIN && callback) {
// Simulate successful authorization
callback({ success: true })
}
return true
})

const logSpy = vi.spyOn(console, "log")

await channelWithUserId.onConnect(mockSocket)
await channel.subscribeToTask(mockTask, mockSocket)

// Verify success was logged
expect(logSpy).toHaveBeenCalledWith(`[TaskChannel#subscribeToTask] subscribed to ${taskId}`)

// Verify task was added to subscribedTasks
expect(channel.subscribedTasks.has(taskId)).toBe(true)
expect(channel.subscribedTasks.get(taskId)).toBe(mockTask)

logSpy.mockRestore()
})
})
})
Loading