Skip to content

Commit e1f04f4

Browse files
v0.3.26: fix billing, bubble up workflow block errors, credentials security improvements
v0.3.26: fix billing, bubble up workflow block errors, credentials security improvements
2 parents 56ffb53 + fd9e61f commit e1f04f4

File tree

23 files changed

+365
-282
lines changed

23 files changed

+365
-282
lines changed

apps/sim/app/api/auth/oauth/token/route.test.ts

Lines changed: 73 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ describe('OAuth Token API Routes', () => {
1010
const mockGetUserId = vi.fn()
1111
const mockGetCredential = vi.fn()
1212
const mockRefreshTokenIfNeeded = vi.fn()
13+
const mockAuthorizeCredentialUse = vi.fn()
14+
const mockCheckHybridAuth = vi.fn()
1315

1416
const mockLogger = {
1517
info: vi.fn(),
@@ -37,6 +39,14 @@ describe('OAuth Token API Routes', () => {
3739
vi.doMock('@/lib/logs/console/logger', () => ({
3840
createLogger: vi.fn().mockReturnValue(mockLogger),
3941
}))
42+
43+
vi.doMock('@/lib/auth/credential-access', () => ({
44+
authorizeCredentialUse: mockAuthorizeCredentialUse,
45+
}))
46+
47+
vi.doMock('@/lib/auth/hybrid', () => ({
48+
checkHybridAuth: mockCheckHybridAuth,
49+
}))
4050
})
4151

4252
afterEach(() => {
@@ -48,7 +58,12 @@ describe('OAuth Token API Routes', () => {
4858
*/
4959
describe('POST handler', () => {
5060
it('should return access token successfully', async () => {
51-
mockGetUserId.mockResolvedValueOnce('test-user-id')
61+
mockAuthorizeCredentialUse.mockResolvedValueOnce({
62+
ok: true,
63+
authType: 'session',
64+
requesterUserId: 'test-user-id',
65+
credentialOwnerUserId: 'owner-user-id',
66+
})
5267
mockGetCredential.mockResolvedValueOnce({
5368
id: 'credential-id',
5469
accessToken: 'test-token',
@@ -78,14 +93,18 @@ describe('OAuth Token API Routes', () => {
7893
expect(data).toHaveProperty('accessToken', 'fresh-token')
7994

8095
// Verify mocks were called correctly
81-
// POST no longer calls getUserId; token resolution uses credential owner.
82-
expect(mockGetUserId).not.toHaveBeenCalled()
96+
expect(mockAuthorizeCredentialUse).toHaveBeenCalled()
8397
expect(mockGetCredential).toHaveBeenCalled()
8498
expect(mockRefreshTokenIfNeeded).toHaveBeenCalled()
8599
})
86100

87101
it('should handle workflowId for server-side authentication', async () => {
88-
mockGetUserId.mockResolvedValueOnce('workflow-owner-id')
102+
mockAuthorizeCredentialUse.mockResolvedValueOnce({
103+
ok: true,
104+
authType: 'internal_jwt',
105+
requesterUserId: 'workflow-owner-id',
106+
credentialOwnerUserId: 'workflow-owner-id',
107+
})
89108
mockGetCredential.mockResolvedValueOnce({
90109
id: 'credential-id',
91110
accessToken: 'test-token',
@@ -111,8 +130,7 @@ describe('OAuth Token API Routes', () => {
111130
expect(response.status).toBe(200)
112131
expect(data).toHaveProperty('accessToken', 'fresh-token')
113132

114-
// POST no longer calls getUserId; still refreshes successfully
115-
expect(mockGetUserId).not.toHaveBeenCalled()
133+
expect(mockAuthorizeCredentialUse).toHaveBeenCalled()
116134
expect(mockGetCredential).toHaveBeenCalled()
117135
})
118136

@@ -130,8 +148,10 @@ describe('OAuth Token API Routes', () => {
130148
})
131149

132150
it('should handle authentication failure', async () => {
133-
// Authentication failure no longer applies to POST path; treat as refresh failure via missing owner
134-
mockGetUserId.mockResolvedValueOnce(undefined)
151+
mockAuthorizeCredentialUse.mockResolvedValueOnce({
152+
ok: false,
153+
error: 'Authentication required',
154+
})
135155

136156
const req = createMockRequest('POST', {
137157
credentialId: 'credential-id',
@@ -142,12 +162,12 @@ describe('OAuth Token API Routes', () => {
142162
const response = await POST(req)
143163
const data = await response.json()
144164

145-
expect([401, 404]).toContain(response.status)
165+
expect(response.status).toBe(403)
146166
expect(data).toHaveProperty('error')
147167
})
148168

149169
it('should handle workflow not found', async () => {
150-
mockGetUserId.mockResolvedValueOnce(undefined)
170+
mockAuthorizeCredentialUse.mockResolvedValueOnce({ ok: false, error: 'Workflow not found' })
151171

152172
const req = createMockRequest('POST', {
153173
credentialId: 'credential-id',
@@ -159,13 +179,16 @@ describe('OAuth Token API Routes', () => {
159179
const response = await POST(req)
160180
const data = await response.json()
161181

162-
// With owner-based resolution, missing workflowId no longer matters.
163-
// If credential not found via owner lookup, returns 404 accordingly
164-
expect([401, 404]).toContain(response.status)
182+
expect(response.status).toBe(403)
165183
})
166184

167185
it('should handle credential not found', async () => {
168-
mockGetUserId.mockResolvedValueOnce('test-user-id')
186+
mockAuthorizeCredentialUse.mockResolvedValueOnce({
187+
ok: true,
188+
authType: 'session',
189+
requesterUserId: 'test-user-id',
190+
credentialOwnerUserId: 'owner-user-id',
191+
})
169192
mockGetCredential.mockResolvedValueOnce(undefined)
170193

171194
const req = createMockRequest('POST', {
@@ -177,12 +200,17 @@ describe('OAuth Token API Routes', () => {
177200
const response = await POST(req)
178201
const data = await response.json()
179202

180-
expect([401, 404]).toContain(response.status)
203+
expect(response.status).toBe(401)
181204
expect(data).toHaveProperty('error')
182205
})
183206

184207
it('should handle token refresh failure', async () => {
185-
mockGetUserId.mockResolvedValueOnce('test-user-id')
208+
mockAuthorizeCredentialUse.mockResolvedValueOnce({
209+
ok: true,
210+
authType: 'session',
211+
requesterUserId: 'test-user-id',
212+
credentialOwnerUserId: 'owner-user-id',
213+
})
186214
mockGetCredential.mockResolvedValueOnce({
187215
id: 'credential-id',
188216
accessToken: 'test-token',
@@ -211,7 +239,11 @@ describe('OAuth Token API Routes', () => {
211239
*/
212240
describe('GET handler', () => {
213241
it('should return access token successfully', async () => {
214-
mockGetUserId.mockResolvedValueOnce('test-user-id')
242+
mockCheckHybridAuth.mockResolvedValueOnce({
243+
success: true,
244+
authType: 'session',
245+
userId: 'test-user-id',
246+
})
215247
mockGetCredential.mockResolvedValueOnce({
216248
id: 'credential-id',
217249
accessToken: 'test-token',
@@ -236,7 +268,7 @@ describe('OAuth Token API Routes', () => {
236268
expect(response.status).toBe(200)
237269
expect(data).toHaveProperty('accessToken', 'fresh-token')
238270

239-
expect(mockGetUserId).toHaveBeenCalledWith(mockRequestId)
271+
expect(mockCheckHybridAuth).toHaveBeenCalled()
240272
expect(mockGetCredential).toHaveBeenCalledWith(mockRequestId, 'credential-id', 'test-user-id')
241273
expect(mockRefreshTokenIfNeeded).toHaveBeenCalled()
242274
})
@@ -255,7 +287,10 @@ describe('OAuth Token API Routes', () => {
255287
})
256288

257289
it('should handle authentication failure', async () => {
258-
mockGetUserId.mockResolvedValueOnce(undefined)
290+
mockCheckHybridAuth.mockResolvedValueOnce({
291+
success: false,
292+
error: 'Authentication required',
293+
})
259294

260295
const req = new Request(
261296
'http://localhost:3000/api/auth/oauth/token?credentialId=credential-id'
@@ -266,12 +301,16 @@ describe('OAuth Token API Routes', () => {
266301
const response = await GET(req as any)
267302
const data = await response.json()
268303

269-
expect([401, 404]).toContain(response.status)
304+
expect(response.status).toBe(401)
270305
expect(data).toHaveProperty('error')
271306
})
272307

273308
it('should handle credential not found', async () => {
274-
mockGetUserId.mockResolvedValueOnce('test-user-id')
309+
mockCheckHybridAuth.mockResolvedValueOnce({
310+
success: true,
311+
authType: 'session',
312+
userId: 'test-user-id',
313+
})
275314
mockGetCredential.mockResolvedValueOnce(undefined)
276315

277316
const req = new Request(
@@ -283,12 +322,16 @@ describe('OAuth Token API Routes', () => {
283322
const response = await GET(req as any)
284323
const data = await response.json()
285324

286-
expect([401, 404]).toContain(response.status)
325+
expect(response.status).toBe(404)
287326
expect(data).toHaveProperty('error')
288327
})
289328

290329
it('should handle missing access token', async () => {
291-
mockGetUserId.mockResolvedValueOnce('test-user-id')
330+
mockCheckHybridAuth.mockResolvedValueOnce({
331+
success: true,
332+
authType: 'session',
333+
userId: 'test-user-id',
334+
})
292335
mockGetCredential.mockResolvedValueOnce({
293336
id: 'credential-id',
294337
accessToken: null,
@@ -305,12 +348,16 @@ describe('OAuth Token API Routes', () => {
305348
const response = await GET(req as any)
306349
const data = await response.json()
307350

308-
expect([400, 401]).toContain(response.status)
351+
expect(response.status).toBe(400)
309352
expect(data).toHaveProperty('error')
310353
})
311354

312355
it('should handle token refresh failure', async () => {
313-
mockGetUserId.mockResolvedValueOnce('test-user-id')
356+
mockCheckHybridAuth.mockResolvedValueOnce({
357+
success: true,
358+
authType: 'session',
359+
userId: 'test-user-id',
360+
})
314361
mockGetCredential.mockResolvedValueOnce({
315362
id: 'credential-id',
316363
accessToken: 'test-token',
@@ -329,7 +376,7 @@ describe('OAuth Token API Routes', () => {
329376
const response = await GET(req as any)
330377
const data = await response.json()
331378

332-
expect([401, 404]).toContain(response.status)
379+
expect(response.status).toBe(401)
333380
expect(data).toHaveProperty('error')
334381
})
335382
})

apps/sim/app/api/auth/oauth/token/route.ts

Lines changed: 11 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,8 @@
1-
import { eq } from 'drizzle-orm'
21
import { type NextRequest, NextResponse } from 'next/server'
2+
import { authorizeCredentialUse } from '@/lib/auth/credential-access'
3+
import { checkHybridAuth } from '@/lib/auth/hybrid'
34
import { createLogger } from '@/lib/logs/console/logger'
4-
import { getCredential, getUserId, refreshTokenIfNeeded } from '@/app/api/auth/oauth/utils'
5-
import { db } from '@/db'
6-
import { account } from '@/db/schema'
5+
import { getCredential, refreshTokenIfNeeded } from '@/app/api/auth/oauth/utils'
76

87
export const dynamic = 'force-dynamic'
98

@@ -29,18 +28,13 @@ export async function POST(request: NextRequest) {
2928
return NextResponse.json({ error: 'Credential ID is required' }, { status: 400 })
3029
}
3130

32-
// Resolve the credential owner directly by id. This lets API/UI/webhooks run under
33-
// whichever user owns the persisted credential, not necessarily the session user
34-
// or workflow owner.
35-
const creds = await db.select().from(account).where(eq(account.id, credentialId)).limit(1)
36-
if (!creds.length) {
37-
logger.error(`[${requestId}] Credential not found: ${credentialId}`)
38-
return NextResponse.json({ error: 'Credential not found' }, { status: 404 })
31+
const authz = await authorizeCredentialUse(request, { credentialId, workflowId })
32+
if (!authz.ok || !authz.credentialOwnerUserId) {
33+
return NextResponse.json({ error: authz.error || 'Unauthorized' }, { status: 403 })
3934
}
40-
const credentialOwnerUserId = creds[0].userId
4135

42-
// Fetch the credential verifying it belongs to the resolved owner
43-
const credential = await getCredential(requestId, credentialId, credentialOwnerUserId)
36+
// Fetch the credential as the owner to enforce ownership scoping
37+
const credential = await getCredential(requestId, credentialId, authz.credentialOwnerUserId)
4438

4539
try {
4640
// Refresh the token if needed
@@ -73,14 +67,13 @@ export async function GET(request: NextRequest) {
7367
}
7468

7569
// For GET requests, we only support session-based authentication
76-
const userId = await getUserId(requestId)
77-
78-
if (!userId) {
70+
const auth = await checkHybridAuth(request, { requireWorkflowId: false })
71+
if (!auth.success || auth.authType !== 'session' || !auth.userId) {
7972
return NextResponse.json({ error: 'User not authenticated' }, { status: 401 })
8073
}
8174

8275
// Get the credential from the database
83-
const credential = await getCredential(requestId, credentialId, userId)
76+
const credential = await getCredential(requestId, credentialId, auth.userId)
8477

8578
if (!credential) {
8679
return NextResponse.json({ error: 'Credential not found' }, { status: 404 })

apps/sim/app/api/tools/drive/file/route.ts

Lines changed: 10 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,7 @@
1-
import { eq } from 'drizzle-orm'
21
import { type NextRequest, NextResponse } from 'next/server'
3-
import { getSession } from '@/lib/auth'
2+
import { authorizeCredentialUse } from '@/lib/auth/credential-access'
43
import { createLogger } from '@/lib/logs/console/logger'
54
import { refreshAccessTokenIfNeeded } from '@/app/api/auth/oauth/utils'
6-
import { db } from '@/db'
7-
import { account } from '@/db/schema'
85

96
export const dynamic = 'force-dynamic'
107

@@ -18,46 +15,28 @@ export async function GET(request: NextRequest) {
1815
logger.info(`[${requestId}] Google Drive file request received`)
1916

2017
try {
21-
// Get the session
22-
const session = await getSession()
23-
24-
// Check if the user is authenticated
25-
if (!session?.user?.id) {
26-
logger.warn(`[${requestId}] Unauthenticated request rejected`)
27-
return NextResponse.json({ error: 'User not authenticated' }, { status: 401 })
28-
}
29-
3018
// Get the credential ID and file ID from the query params
3119
const { searchParams } = new URL(request.url)
3220
const credentialId = searchParams.get('credentialId')
3321
const fileId = searchParams.get('fileId')
34-
const workflowId = searchParams.get('workflowId')
22+
const workflowId = searchParams.get('workflowId') || undefined
3523

3624
if (!credentialId || !fileId) {
3725
logger.warn(`[${requestId}] Missing required parameters`)
3826
return NextResponse.json({ error: 'Credential ID and File ID are required' }, { status: 400 })
3927
}
4028

41-
// Get the credential from the database
42-
const credentials = await db.select().from(account).where(eq(account.id, credentialId)).limit(1)
43-
44-
if (!credentials.length) {
45-
logger.warn(`[${requestId}] Credential not found`, { credentialId })
46-
return NextResponse.json({ error: 'Credential not found' }, { status: 404 })
47-
}
48-
49-
const credential = credentials[0]
50-
51-
// Credential ownership:
52-
// - If session user owns the credential: allow
53-
// - If not, allow read-only resolution when a workflowId is present (collaboration case)
54-
if (credential.userId !== session.user.id && !workflowId) {
55-
return NextResponse.json({ error: 'Unauthorized' }, { status: 403 })
29+
const authz = await authorizeCredentialUse(request, { credentialId: credentialId, workflowId })
30+
if (!authz.ok || !authz.credentialOwnerUserId) {
31+
return NextResponse.json({ error: authz.error || 'Unauthorized' }, { status: 403 })
5632
}
5733

5834
// Refresh access token if needed using the utility function
59-
const ownerUserId = credential.userId
60-
const accessToken = await refreshAccessTokenIfNeeded(credentialId, ownerUserId, requestId)
35+
const accessToken = await refreshAccessTokenIfNeeded(
36+
credentialId,
37+
authz.credentialOwnerUserId,
38+
requestId
39+
)
6140

6241
if (!accessToken) {
6342
return NextResponse.json({ error: 'Failed to obtain valid access token' }, { status: 401 })

0 commit comments

Comments
 (0)