Skip to content

Commit f590278

Browse files
geroplona-agent
geropl
and
ona-agent
committed
Add CLC-1592 OAuth token leak fix design document
- Add comprehensive design document for OAuth state validation security fix - Include detailed vulnerability analysis and attack vector description - Implement feature flag strategy for safe rollout (enforceOAuthStateValidation) - Add 4-step implementation plan with progress tracking - Include comprehensive testing strategy for security and regression scenarios - Provide complete code implementation with feature flag guards Co-authored-by: Ona <[email protected]>
1 parent b2a1279 commit f590278

File tree

1 file changed

+387
-0
lines changed

1 file changed

+387
-0
lines changed

‎CLC-1592-OAuth-Token-Leak-Fix.md‎

Lines changed: 387 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,387 @@
1+
# Design Document: Fix for CLC-1592 - Bitbucket OAuth Access Token Leak
2+
3+
## Overview
4+
5+
This document outlines the solution for CLC-1592, a critical security vulnerability that allows attackers to steal OAuth access tokens through a redirect URI validation bypass in Gitpod's authorization flow.
6+
7+
## Problem Statement
8+
9+
### Vulnerability Description
10+
The `/api/authorize` endpoint accepts direct requests without validating that they originated from legitimate OAuth callbacks. This allows malicious OAuth applications to redirect users to the endpoint with crafted parameters, enabling access token theft.
11+
12+
### Attack Vector
13+
1. Attacker creates malicious OAuth application with Gitpod as redirect URI
14+
2. Malicious app redirects to: `https://api.gitpod.io/api/authorize?returnTo=https://attacker.com&host=malicious-provider`
15+
3. `/api/authorize` processes request without validation
16+
4. User gets redirected to attacker-controlled domain
17+
5. Attacker captures OAuth tokens and completes authentication on their system
18+
19+
### Impact
20+
- **OAuth token theft**: Full access to victim's GitHub/GitLab/Bitbucket accounts
21+
- **Account takeover**: Complete access to repositories and data
22+
- **Session hijacking**: Attackers can authenticate as the victim
23+
24+
## Current Architecture
25+
26+
### Legitimate OAuth Flow
27+
```mermaid
28+
sequenceDiagram
29+
participant U as User
30+
participant D as Dashboard
31+
participant GP as Git Provider
32+
participant CB as /auth/callback
33+
participant AZ as /api/authorize
34+
35+
U->>D: Click authorize
36+
D->>GP: Redirect with client_id
37+
GP->>CB: Redirect with code + state JWT
38+
CB->>CB: Validate JWT state
39+
CB->>AZ: Redirect with validated state
40+
AZ->>AZ: Process authorization
41+
```
42+
43+
### Attack Flow
44+
```mermaid
45+
sequenceDiagram
46+
participant A as Attacker
47+
participant MA as Malicious App
48+
participant AZ as /api/authorize
49+
participant V as Victim
50+
51+
A->>MA: Create malicious OAuth app
52+
MA->>AZ: Direct redirect (bypass callback)
53+
AZ->>AZ: Process without validation ❌
54+
AZ->>A: Redirect to attacker domain
55+
A->>A: Capture tokens ❌
56+
```
57+
58+
## Solution Design
59+
60+
### Core Principle
61+
Require that all `/api/authorize` requests include a valid JWT state parameter that proves the request originated from a legitimate OAuth callback.
62+
63+
### Implementation Strategy
64+
Leverage the existing JWT state validation system already used in OAuth callbacks, rather than creating new validation infrastructure. The implementation will be guarded by a feature flag to enable safe rollout and quick rollback if needed.
65+
66+
### Key Components
67+
68+
#### 1. Feature Flag Guard
69+
```typescript
70+
// Check feature flag before applying validation
71+
const enforceStateValidation = this.config.featureFlags?.enforceOAuthStateValidation ?? false;
72+
73+
if (!enforceStateValidation) {
74+
// Continue with existing logic for backward compatibility
75+
log.info(`OAuth state validation disabled by feature flag`, { "authorize-flow": true });
76+
// ... existing authorization logic
77+
return;
78+
}
79+
```
80+
81+
#### 2. State Parameter Validation
82+
```typescript
83+
// In authenticator.ts authorize() method
84+
const stateParam = req.query.state?.toString();
85+
86+
if (!stateParam) {
87+
log.warn(`Missing state parameter in authorize request`, { host, returnTo });
88+
res.redirect(this.getSorryUrl(`Invalid authorization request.`));
89+
return;
90+
}
91+
```
92+
93+
#### 3. JWT State Verification
94+
```typescript
95+
try {
96+
// Reuse existing JWT state validation logic
97+
const flowState = await this.parseState(stateParam);
98+
99+
// Validate that the state authorizes this specific request
100+
if (flowState.host !== host || flowState.returnTo !== returnTo) {
101+
log.warn(`State parameter mismatch`, {
102+
stateHost: flowState.host,
103+
requestHost: host
104+
});
105+
res.redirect(this.getSorryUrl(`Invalid authorization request.`));
106+
return;
107+
}
108+
} catch (error) {
109+
log.warn(`Invalid state parameter`, error);
110+
res.redirect(this.getSorryUrl(`Invalid authorization request.`));
111+
return;
112+
}
113+
```
114+
115+
#### 4. Parameter Matching
116+
Ensure the JWT state explicitly authorizes the requested host and returnTo parameters to prevent parameter tampering.
117+
118+
## Implementation Details
119+
120+
### File Changes
121+
**Primary Change**: `components/server/src/auth/authenticator.ts`
122+
- Modify `authorize()` method to conditionally require and validate JWT state parameter
123+
- Add feature flag check (`enforceOAuthStateValidation`) before validation logic
124+
- Add state validation before existing authorization logic (when feature flag enabled)
125+
- Reuse existing `parseState()` method for JWT validation
126+
- Maintain backward compatibility when feature flag is disabled
127+
128+
**Feature Flag Configuration**:
129+
- Feature flag name: `enforceOAuthStateValidation`
130+
- Default value: `false` (disabled for safe rollout)
131+
- Type: `boolean`
132+
- Location: Server configuration/feature flags system
133+
134+
### Code Implementation
135+
```typescript
136+
async authorize(req: express.Request, res: express.Response, next: express.NextFunction) {
137+
const user = req.user;
138+
if (!req.isAuthenticated() || !User.is(user)) {
139+
log.info(`User is not authenticated.`, { "authorize-flow": true });
140+
res.redirect(this.getSorryUrl(`Not authenticated. Please login.`));
141+
return;
142+
}
143+
144+
const returnTo: string | undefined = req.query.returnTo?.toString();
145+
const host: string | undefined = req.query.host?.toString();
146+
const scopes: string = req.query.scopes?.toString() || "";
147+
const override = req.query.override === "true";
148+
const stateParam = req.query.state?.toString();
149+
150+
// NEW: Feature flag guard for OAuth state validation
151+
const enforceStateValidation = this.config.featureFlags?.enforceOAuthStateValidation ?? false;
152+
153+
if (enforceStateValidation) {
154+
// NEW: Validate JWT state parameter when feature flag is enabled
155+
if (!stateParam) {
156+
log.warn(`Missing state parameter in authorize request`, {
157+
host, returnTo, "authorize-flow": true
158+
});
159+
res.redirect(this.getSorryUrl(`Invalid authorization request.`));
160+
return;
161+
}
162+
163+
try {
164+
const flowState = await this.parseState(stateParam);
165+
166+
if (flowState.host !== host || flowState.returnTo !== returnTo) {
167+
log.warn(`State parameter mismatch in authorize request`, {
168+
stateHost: flowState.host,
169+
requestHost: host,
170+
stateReturnTo: flowState.returnTo,
171+
requestReturnTo: returnTo,
172+
"authorize-flow": true
173+
});
174+
res.redirect(this.getSorryUrl(`Invalid authorization request.`));
175+
return;
176+
}
177+
178+
log.info(`Valid state parameter in authorize request`, { host, "authorize-flow": true });
179+
180+
} catch (error) {
181+
log.warn(`Invalid state parameter in authorize request`, error, {
182+
host, returnTo, "authorize-flow": true
183+
});
184+
res.redirect(this.getSorryUrl(`Invalid authorization request.`));
185+
return;
186+
}
187+
} else {
188+
log.info(`OAuth state validation disabled by feature flag`, {
189+
host, returnTo, "authorize-flow": true
190+
});
191+
}
192+
193+
// Continue with existing authorization logic...
194+
const authProvider = host && (await this.getAuthProviderForHost(host));
195+
if (!returnTo || !host || !authProvider) {
196+
log.info(`Bad request: missing parameters.`, { "authorize-flow": true });
197+
res.redirect(this.getSorryUrl(`Bad request: missing parameters.`));
198+
return;
199+
}
200+
201+
// ... rest of existing validation logic ...
202+
203+
// Create new state for OAuth provider
204+
const newState = await this.signInJWT.sign({ host, returnTo, overrideScopes: override });
205+
authProvider.authorize(req, res, next, this.deriveAuthState(newState), wantedScopes);
206+
}
207+
```
208+
209+
## Security Analysis
210+
211+
### Attack Prevention
212+
| Attack Vector | Mitigation | Result |
213+
|---------------|------------|---------|
214+
| **Direct URL manipulation** | Requires valid JWT state | ❌ Blocked |
215+
| **Malicious OAuth redirects** | State validation fails | ❌ Blocked |
216+
| **Parameter tampering** | State must match parameters | ❌ Blocked |
217+
| **Replay attacks** | JWT expiration (5 minutes) | ❌ Blocked |
218+
219+
### Legitimate Flow Preservation
220+
| Scenario | Impact | Result |
221+
|----------|--------|---------|
222+
| **Dashboard authorization** | OAuth callback provides state | ✅ Works |
223+
| **Existing integrations** | No changes required | ✅ Works |
224+
| **User workflows** | Transparent to users | ✅ Works |
225+
226+
## Testing Strategy
227+
228+
### Security Testing
229+
1. **Feature Flag Enabled Testing**:
230+
- Attempt the exact attack described in CLC-1592 (should be blocked)
231+
- Test with malformed/missing state parameters (should be blocked)
232+
- Test with invalid/expired JWT tokens (should be blocked)
233+
- Test various malicious parameter combinations (should be blocked)
234+
235+
2. **Feature Flag Disabled Testing**:
236+
- Verify attack scenarios work as before (backward compatibility)
237+
- Ensure no validation is performed when flag is disabled
238+
239+
### Regression Testing
240+
1. **Feature Flag Enabled**:
241+
- Verify all legitimate authorization workflows continue working
242+
- Test GitHub, GitLab, Bitbucket integrations with valid state
243+
- Ensure no impact on legitimate user flows
244+
245+
2. **Feature Flag Disabled**:
246+
- Verify all authorization workflows work exactly as before
247+
- Test all OAuth providers without state parameter requirements
248+
- Ensure complete backward compatibility
249+
250+
3. **Feature Flag Toggle Testing**:
251+
- Test enabling/disabling flag during runtime
252+
- Verify immediate effect of flag changes
253+
- Test rollback scenarios
254+
255+
### Test Cases
256+
```typescript
257+
describe('CLC-1592 Fix', () => {
258+
describe('with feature flag enabled', () => {
259+
beforeEach(() => {
260+
// Enable feature flag for these tests
261+
config.featureFlags.enforceOAuthStateValidation = true;
262+
});
263+
264+
it('should block requests without state parameter', async () => {
265+
const response = await request('/api/authorize?host=github.com&returnTo=...');
266+
expect(response.status).toBe(302);
267+
expect(response.headers.location).toContain('sorry');
268+
});
269+
270+
it('should block requests with invalid state', async () => {
271+
const response = await request('/api/authorize?state=invalid&host=github.com');
272+
expect(response.status).toBe(302);
273+
expect(response.headers.location).toContain('sorry');
274+
});
275+
276+
it('should allow requests with valid state', async () => {
277+
const validState = await signInJWT.sign({ host: 'github.com', returnTo: '...' });
278+
const response = await request(`/api/authorize?state=${validState}&host=github.com`);
279+
expect(response.status).toBe(302);
280+
expect(response.headers.location).toContain('github.com');
281+
});
282+
});
283+
284+
describe('with feature flag disabled', () => {
285+
beforeEach(() => {
286+
// Disable feature flag for these tests
287+
config.featureFlags.enforceOAuthStateValidation = false;
288+
});
289+
290+
it('should allow requests without state parameter (backward compatibility)', async () => {
291+
const response = await request('/api/authorize?host=github.com&returnTo=...');
292+
expect(response.status).toBe(302);
293+
expect(response.headers.location).toContain('github.com');
294+
});
295+
});
296+
});
297+
```
298+
299+
## Deployment Considerations
300+
301+
### Feature Flag Strategy
302+
1. **Initial Deployment**: Deploy with feature flag disabled (default: false)
303+
2. **Gradual Rollout**: Enable feature flag for testing/staging environments
304+
3. **Production Enablement**: Enable feature flag in production after validation
305+
4. **Quick Rollback**: Disable feature flag immediately if issues arise
306+
307+
### Monitoring
308+
- Track authorization request failures when feature flag is enabled
309+
- Monitor for unusual patterns in state validation errors
310+
- Alert on significant increases in blocked requests
311+
- Log feature flag status for debugging
312+
313+
### Rollback Plan
314+
- **Immediate**: Disable feature flag (no code deployment needed)
315+
- **Full Rollback**: Code revert if feature flag system fails
316+
- No database changes required
317+
- No frontend changes to rollback
318+
319+
## Benefits
320+
321+
### Security Improvements
322+
- **Eliminates OAuth redirect attacks**: Direct access to `/api/authorize` blocked (when feature flag enabled)
323+
- **Prevents parameter tampering**: State validation ensures request integrity
324+
- **Maintains defense in depth**: Leverages existing security infrastructure
325+
- **Zero new attack surface**: Reuses proven JWT validation system
326+
- **Safe rollout**: Feature flag enables immediate rollback without code deployment
327+
328+
### Implementation Advantages
329+
- **Feature flag protected**: Safe rollout with immediate rollback capability
330+
- **Minimal code changes**: Single method modification
331+
- **No frontend changes**: Existing dashboard flows unchanged
332+
- **Reuses existing infrastructure**: JWT state system already battle-tested
333+
- **Backward compatible**: No breaking changes to legitimate flows when flag is disabled
334+
- **Simple to understand**: Clear validation logic with feature flag guard
335+
336+
## Implementation Steps
337+
338+
### Step 1: Code Analysis and Understanding
339+
**Deliverable**: Complete understanding of current implementation
340+
- [ ] Read and analyze `components/server/src/auth/authenticator.ts`
341+
- [ ] Document current `authorize()` method flow
342+
- [ ] Identify existing `parseState()` method implementation
343+
- [ ] Map OAuth flow architecture and state handling
344+
- [ ] Understand feature flag system integration
345+
346+
### Step 2: Security Validation Implementation
347+
**Deliverable**: Modified authenticator with feature-flagged state validation
348+
- [ ] Add feature flag check (`enforceOAuthStateValidation`)
349+
- [ ] Add state parameter extraction and null check (when flag enabled)
350+
- [ ] Implement JWT state verification using existing `parseState()`
351+
- [ ] Add parameter matching validation (host vs state.host, returnTo vs state.returnTo)
352+
- [ ] Add comprehensive error handling with security logging
353+
- [ ] Ensure proper redirect to sorry page for invalid requests
354+
- [ ] Maintain backward compatibility when feature flag is disabled
355+
356+
### Step 3: Testing Implementation
357+
**Deliverable**: Comprehensive test suite covering security and regression scenarios
358+
- [ ] Create unit tests for state validation logic
359+
- [ ] Implement attack scenario tests (CLC-1592 reproduction) with feature flag enabled
360+
- [ ] Add regression tests for legitimate OAuth flows
361+
- [ ] Test feature flag disabled scenario (backward compatibility)
362+
- [ ] Test edge cases (malformed JWT, expired tokens, parameter mismatches)
363+
- [ ] Validate all OAuth providers (GitHub, GitLab, Bitbucket)
364+
365+
### Step 4: Security Validation
366+
**Deliverable**: Confirmed attack prevention and flow preservation
367+
- [ ] Reproduce exact CLC-1592 attack scenario with feature flag enabled
368+
- [ ] Verify attack is blocked by new validation
369+
- [ ] Confirm legitimate dashboard OAuth flows work unchanged
370+
- [ ] Test various malicious parameter combinations
371+
- [ ] Validate JWT expiration handling (5-minute timeout)
372+
- [ ] Verify backward compatibility with feature flag disabled
373+
374+
## Progress Tracking
375+
376+
| Step | Status | Notes |
377+
|------|--------|-------|
378+
| Step 1: Code Analysis | ⏳ Pending | |
379+
| Step 2: Implementation | ⏳ Pending | |
380+
| Step 3: Testing | ⏳ Pending | |
381+
| Step 4: Security Validation | ⏳ Pending | |
382+
383+
## Conclusion
384+
385+
This solution provides robust protection against CLC-1592 by requiring JWT state validation for all `/api/authorize` requests. It leverages existing, proven security infrastructure while maintaining full compatibility with legitimate authorization flows.
386+
387+
The fix is minimal, focused, and directly addresses the root cause of the vulnerability without introducing new complexity or potential security issues. The feature flag implementation ensures safe rollout and immediate rollback capability.

0 commit comments

Comments
 (0)