nextcloud / nextcloud/desktop

fix(redirect): Allow credentials for HTTPS redirects in different domains

Open
#10,258 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
C++
Stars
3.9k
Forks
1k
Avg merge
1d 17h
Merged PRs (30d)
123

Description

## Solution

This patch improves the redirect credential handling to support legitimate SSO scenarios while maintaining security.

### Changes

Replace the overly strict host/port matching with a smarter check:
1. **Allow HTTPS-to-HTTPS redirects** regardless of domain (they're encrypted end-to-end)
2. **Only strip credentials for HTTP redirects** to prevent leaking auth tokens in plaintext
3. **Still prevent HTTPS→HTTP downgrades** (this was already checked at line 251)

### Proposed Fix

```cpp
// Line 279-288: Replace with:
bool isSafeRedirect = (redirectUrl.scheme() == QLatin1String("https"))
&& (requestedUrl.host() == redirectUrl.host()
&& requestedUrl.port() == redirectUrl.port());

if (!isSafeRedirect) {
qCWarning(lcNetworkJob).nospace() << "redirect target mismatches origin or uses HTTP, removing credentials"
<< " origin=" << requestedUrl.scheme() << "://" << requestedUrl.host() << ":" << requestedUrl.port()
<< " target=" << redirectUrl.scheme() << "://" << redirectUrl.host() << ":" << redirectUrl.port();

auto headers = request.headers();
headers.removeAll(QHttpHeaders::WellKnownHeader::Authorization);
request.setHeaders(headers);
request.setAttribute(AbstractCredentials::DontAddCredentialsAttribute, true);
}
```

### Why This Works

- **HTTPS redirects to different domains**: Safe for SSO (transport-layer encryption protects credentials)
- **HTTP redirects**: Credentials removed (plaintext exposure prevented)
- **HTTPS→HTTP downgrades**: Caught by existing check at line 251, request won't reach this point
- **Same-origin redirects**: Keep credentials as before

### Testing

Should test with:
1. OAuth2 redirect to different HTTPS domain ✅ (now works)
2. HTTPS→HTTP redirect ✅ (still blocked)
3. Same-origin redirect ✅ (still works)
4. HTTP→HTTP cross-domain ✅ (credentials removed as before)

Fixes: Regression from commit 585b6ca32dc97c51ede2dad1c002b8a2625e12fc

Contributor guide

Open the contributing guide

Research direction

Start at the network job's redirect credential handling around lines 251 and 279-288. Reconcile the stated HTTPS cross-domain requirement with the proposed host-matching condition, then exercise the four listed redirect scenarios; done means HTTPS SSO redirects retain credentials while HTTP and downgrade cases do not.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
desktop, networking, security
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.