Skip to content

Commit 47ca123

Browse files
Fix OAuth redirect loop and improve session security configuration
OAuth Improvements: - Fix infinite redirect loop by implementing a "Direct Login Hijack" flow for Google, GitHub, and Generic providers. - Move OAuth token storage from session cookies to the database to prevent "Cookie Too Large" errors. - Ensure `SESSION_COOKIE_SECURE` is automatically enforced when OAuth is enabled. Security & Configuration: - Add new "Use via HTTPS" setting to Basic Configuration. This allows administrators to enforce `Secure` and `SameSite=Lax` cookies on standard/LDAP logins when running over HTTPS. - Update application initialization to dynamically configure cookie security based on login type and settings. - Clarify "Allow Reverse Proxy Authentication" setting description in the UI. - Add static warning in OAuth settings emphasizing the HTTPS requirement. Database: -Fix OAuth redirect loop and improve session security configuration OAuth Improvements: - Fix infinite redirect loop by implementing a "Direct Login Hijack" flow for Google, GitHub, and Generic providers. - Move OAuth token storage from session cookies to the database to prevent "Cookie Too Large" errors. - Ensure `SESSION_COOKIE_SECURE` is automatically enforced when OAuth is enabled. Security & Configuration: - Add new "Use via HTTPS" setting to Basic Configuration. This allows administrators to enforce `Secure` and `SameSite=Lax` cookies on standard/LDAP logins when running over HTTPS. - Update application initialization to dynamically configure cookie security based on login type and settings. - Clarify "Allow Reverse Proxy Authentication" setting description in the UI. - Add static warning in OAuth settings emphasizing the HTTPS requirement. Database: -Add config_use_https column to settings table.
1 parent fb0635b commit 47ca123

6 files changed

Lines changed: 165 additions & 10 deletions

File tree

CONTRIBUTORS

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ Copyright (C) 2018-2025 Calibre-Web contributors
1111
Copyright (C) 2024-2025 Calibre-Web Automated contributors
1212
# Upstream Contributors (janeczku/calibre-web)
1313

14-
- OzzieIsaacs (anon) (2769 commits)
14+
- OzzieIsaacs (anon) (2771 commits)
1515
- Ozzie Isaacs (anon) (264 commits)
1616
- cbartondock (96 commits)
1717
- idalin (69 commits)
@@ -38,7 +38,7 @@ Copyright (C) 2024-2025 Calibre-Web Automated contributors
3838
- kyos (anon) (18 commits)
3939
- quarz12 (18 commits)
4040
- Thore Schillmann (anon) (18 commits)
41-
- mapi68 (13 commits)
41+
- mapi68 (14 commits)
4242
- ok11 (13 commits)
4343
- pwr (13 commits)
4444
- Kyosfonica (11 commits)
@@ -52,6 +52,7 @@ Copyright (C) 2024-2025 Calibre-Web Automated contributors
5252
- XZVB12 (10 commits)
5353
- celogeek (9 commits)
5454
- GarckaMan (9 commits)
55+
- IgorKurkov (9 commits)
5556
- Knepherbird (9 commits)
5657
- Yamakuni (anon) (9 commits)
5758
- 89jd (8 commits)

cps/__init__.py

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
from werkzeug.middleware.proxy_fix import ProxyFix
1818

1919
from . import logger
20+
from . import constants
2021
from .cli import CliParameter
2122
from .reverseproxy import ReverseProxied
2223
from .server import WebServer
@@ -74,6 +75,7 @@
7475
app = Flask(__name__)
7576
app.config.update(
7677
SESSION_COOKIE_HTTPONLY=True,
78+
SESSION_COOKIE_SECURE=os.environ.get('SESSION_COOKIE_SECURE', 'False').lower() == 'true',
7779
SESSION_COOKIE_SAMESITE='Lax',
7880
REMEMBER_COOKIE_SAMESITE='Strict',
7981
WTF_CSRF_SSL_STRICT=False,
@@ -125,6 +127,18 @@ def create_app():
125127
config_sql.load_configuration(ub.session, encrypt_key)
126128
config.init_config(ub.session, encrypt_key, cli_param)
127129

130+
# Intelligent Security Configuration
131+
# Force SESSION_COOKIE_SECURE if OAuth is enabled OR if "Use via HTTPS" is checked
132+
# This ensures OAuth works (requires Secure cookies) while allowing HTTP for standard login if desired
133+
if config.config_login_type == constants.LOGIN_OAUTH or getattr(config, 'config_use_https', False):
134+
app.config['SESSION_COOKIE_SECURE'] = True
135+
app.config['SESSION_COOKIE_SAMESITE'] = 'Lax'
136+
log.info("Enforcing SESSION_COOKIE_SECURE=True (OAuth enabled or HTTPS enforced)")
137+
else:
138+
# Fallback to environment variable or False
139+
app.config['SESSION_COOKIE_SECURE'] = os.environ.get('SESSION_COOKIE_SECURE', 'False').lower() == 'true'
140+
log.info(f"SESSION_COOKIE_SECURE set to {app.config['SESSION_COOKIE_SECURE']} (Standard/LDAP login)")
141+
128142
# Set OAuth redirect host consistency
129143
if hasattr(config, 'config_oauth_redirect_host') and config.config_oauth_redirect_host:
130144
from urllib.parse import urlparse

cps/config_sql.py

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ class _Settings(_Base):
8686
config_anonbrowse = Column(SmallInteger, default=0)
8787
config_public_reg = Column(SmallInteger, default=0)
8888
config_remote_login = Column(Boolean, default=False)
89+
config_use_https = Column(Boolean, default=False)
8990
config_kobo_sync = Column(Boolean, default=False)
9091

9192
# Sync read progress to Hardcover - should this be renamed?

cps/oauth.py

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,20 @@
1111
from flask_dance.consumer.storage.sqla import SQLAlchemyStorage as SQLAlchemyBackend
1212
from flask_dance.consumer.storage.sqla import first, _get_real_user
1313
from sqlalchemy.orm.exc import NoResultFound
14-
backend_resultcode = False # prevent storing values with this resultcode
14+
backend_resultcode = True # prevent storing values with this resultcode
1515
except ImportError:
1616
pass
1717

18+
import logging
19+
def debug_log(msg):
20+
try:
21+
# Try writing to a location we know exists and is writable in the container
22+
with open('/tmp/oauth_debug.log', 'a') as f:
23+
f.write(msg + '\n')
24+
except Exception as e:
25+
print(f"Logging failed: {e}")
26+
27+
1828

1929
class OAuthBackend(SQLAlchemyBackend):
2030
"""
@@ -30,8 +40,21 @@ def __init__(self, model, session, provider_id,
3040
super(OAuthBackend, self).__init__(model, session, user, user_id, user_required, anon_user, cache)
3141

3242
def get(self, blueprint, user=None, user_id=None):
33-
if self.provider_id + '_oauth_token' in session and session[self.provider_id + '_oauth_token'] != '':
34-
return session[self.provider_id + '_oauth_token']
43+
debug_log(f"GET called. Provider: {self.provider_id}")
44+
45+
# Debug session contents
46+
try:
47+
debug_log(f"Session keys: {list(session.keys())}")
48+
except:
49+
pass
50+
51+
if self.provider_id + '_oauth_token' in session:
52+
debug_log(f"Found token in session for {self.provider_id}")
53+
if session[self.provider_id + '_oauth_token'] != '':
54+
return session[self.provider_id + '_oauth_token']
55+
else:
56+
debug_log(f"Token NOT in session for {self.provider_id}")
57+
3558
# check cache
3659
cache_key = self.make_cache_key(blueprint=blueprint, user=user, user_id=user_id)
3760
token = self.cache.get(cache_key)
@@ -106,6 +129,14 @@ def set(self, blueprint, token, user=None, user_id=None):
106129
if has_user and u:
107130
existing_query = existing_query.filter_by(user=u)
108131

132+
# Check if token is already saved (e.g. by oauth_update_token) to avoid redundant delete/insert
133+
try:
134+
existing = existing_query.first()
135+
if existing and existing.token == token:
136+
return
137+
except Exception:
138+
pass
139+
109140
# queue up delete query -- won't be run until commit()
110141
existing_query.delete()
111142

cps/oauth_bb.py

Lines changed: 100 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -408,8 +408,38 @@ def logout_oauth_user():
408408

409409

410410
def oauth_update_token(provider_id, token, provider_user_id):
411+
try:
412+
with open('/tmp/oauth_debug.log', 'a') as f:
413+
f.write(f"oauth_update_token called for {provider_id}, user {provider_user_id}\n")
414+
f.write(f"Session before update: {list(session.keys())}\n")
415+
except:
416+
pass
417+
418+
# Aggressively clean up potential duplicate tokens to prevent cookie overflow (4KB limit)
419+
# We remove ALL token data from session and rely on DB storage
420+
keys_to_remove = [
421+
'google_oauth_token', 'github_oauth_token', 'generic_oauth_token',
422+
provider_id + "_oauth_token"
423+
]
424+
for key in keys_to_remove:
425+
if key in session:
426+
try:
427+
session.pop(key)
428+
with open('/tmp/oauth_debug.log', 'a') as f:
429+
f.write(f"Removed {key} from session to save space\n")
430+
except:
431+
pass
432+
411433
session[provider_id + "_oauth_user_id"] = provider_user_id
412-
session[provider_id + "_oauth_token"] = token
434+
# Do NOT store token in session - it's too big and causes cookie drop
435+
# session[provider_id + "_oauth_token"] = token
436+
session.modified = True
437+
438+
try:
439+
with open('/tmp/oauth_debug.log', 'a') as f:
440+
f.write(f"Session after update: {list(session.keys())}\n")
441+
except:
442+
pass
413443

414444
# Find this OAuth token in the database, or create it
415445
query = ub.session.query(ub.OAuth).filter_by(
@@ -725,7 +755,16 @@ def github_logged_in(blueprint, token):
725755

726756
github_info = resp.json()
727757
github_user_id = str(github_info["id"])
728-
return oauth_update_token(str(oauthblueprints[0]['id']), token, github_user_id)
758+
759+
# Save token to DB
760+
oauth_update_token(str(oauthblueprints[0]['id']), token, github_user_id)
761+
762+
# DIRECT LOGIN: Hijack flow to prevent redirect loop
763+
response = bind_oauth_or_register(oauthblueprints[0]['id'], github_user_id, 'github.login', 'github')
764+
if response:
765+
abort(response)
766+
767+
return False
729768

730769

731770
@oauth_authorized.connect_via(oauthblueprints[1]['blueprint'])
@@ -735,6 +774,9 @@ def google_logged_in(blueprint, token):
735774
log.error("Failed to log in with Google")
736775
return False
737776

777+
# We do NOT store token in session["google_oauth_token"] here to avoid duplication/bloat.
778+
# It will be stored in session[provider_id + "_oauth_token"] by oauth_update_token.
779+
738780
resp = blueprint.session.get("/oauth2/v2/userinfo")
739781
if not resp.ok:
740782
flash(_("Failed to fetch user info from Google."), category="error")
@@ -743,7 +785,20 @@ def google_logged_in(blueprint, token):
743785

744786
google_info = resp.json()
745787
google_user_id = str(google_info["id"])
746-
return oauth_update_token(str(oauthblueprints[1]['id']), token, google_user_id)
788+
789+
# Save token to DB
790+
oauth_update_token(str(oauthblueprints[1]['id']), token, google_user_id)
791+
792+
# DIRECT LOGIN: Hijack flow to prevent redirect loop
793+
# We perform the binding/login logic right here
794+
response = bind_oauth_or_register(oauthblueprints[1]['id'], google_user_id, 'google.login', 'google')
795+
796+
# If we got a response (redirect), abort the current request and send it immediately
797+
# This stops Flask-Dance from doing its own redirect to /link/google
798+
if response:
799+
abort(response)
800+
801+
return False
747802

748803

749804
@oauth_authorized.connect_via(oauthblueprints[2]['blueprint'])
@@ -757,7 +812,14 @@ def generic_logged_in(blueprint, token):
757812
# Pass token explicitly to avoid DB race condition
758813
provider_user_id = register_user_from_generic_oauth(token)
759814
if provider_user_id:
760-
return oauth_update_token(str(oauthblueprints[2]['id']), token, provider_user_id)
815+
# Save token to DB
816+
oauth_update_token(str(oauthblueprints[2]['id']), token, provider_user_id)
817+
818+
# DIRECT LOGIN: Hijack flow to prevent redirect loop
819+
response = bind_oauth_or_register(oauthblueprints[2]['id'], provider_user_id, 'generic.login', 'generic')
820+
if response:
821+
abort(response)
822+
return False
761823
else:
762824
# register_user_from_generic_oauth already logged error and flashed message
763825
return False
@@ -830,6 +892,8 @@ def generic_error(blueprint, error, error_description=None, error_uri=None):
830892
@oauth.route('/link/github')
831893
@oauth_required
832894
def github_login():
895+
# This route is now only a fallback if the direct login hijack fails
896+
# or if the user navigates here manually.
833897
if not github.authorized:
834898
return redirect(url_for('github.login'))
835899
try:
@@ -859,13 +923,40 @@ def github_login_unlink():
859923
@oauth.route('/link/google')
860924
@oauth_required
861925
def google_login():
926+
# Try to find token in session using the provider ID key
927+
provider_id = str(oauthblueprints[1]['id'])
928+
user_id_key = provider_id + "_oauth_user_id"
929+
930+
# 1. Try to get User ID from session (Small cookie!)
931+
if user_id_key in session:
932+
provider_user_id = session[user_id_key]
933+
934+
# 2. Fetch the huge token from Database instead of session
935+
oauth_entry = ub.session.query(ub.OAuth).filter_by(
936+
provider=provider_id,
937+
provider_user_id=provider_user_id
938+
).first()
939+
940+
if oauth_entry and oauth_entry.token:
941+
# 3. Manually inject token into blueprint
942+
google.token = oauth_entry.token
943+
944+
# 4. Proceed directly to login
945+
return bind_oauth_or_register(oauthblueprints[1]['id'], provider_user_id, 'google.login', 'google')
946+
862947
if not google.authorized:
863948
return redirect(url_for("google.login"))
949+
864950
try:
951+
# If google.authorized is False but we have a token, google.get might fail
952+
# We can try to use the token directly with requests if needed, but let's try google.get first
953+
# If google.token was set, google.get should use it
954+
865955
resp = google.get("/oauth2/v2/userinfo")
866956
if resp.ok:
867957
account_info_json = resp.json()
868958
return bind_oauth_or_register(oauthblueprints[1]['id'], account_info_json['id'], 'google.login', 'google')
959+
869960
flash(_("Google Oauth error, please retry later."), category="error")
870961
log.error("Google Oauth error, please retry later")
871962
except (InvalidGrantError, TokenExpiredError) as e:
@@ -876,6 +967,9 @@ def google_login():
876967
flash(_("Google Oauth error: {}").format(e), category="error")
877968
log.error(e)
878969
return redirect(url_for("google.login"))
970+
except Exception as e:
971+
log.error(f"Unexpected error: {e}")
972+
879973
return redirect(url_for('web.login'))
880974

881975

@@ -888,6 +982,8 @@ def google_login_unlink():
888982
@oauth.route('/link/generic')
889983
@oauth_required
890984
def generic_login():
985+
# This route is now only a fallback if the direct login hijack fails
986+
# or if the user navigates here manually.
891987
if not oauthblueprints[2]['blueprint'].session.authorized:
892988
return redirect(url_for("generic.login"))
893989
try:

cps/templates/config_edit.html

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -246,6 +246,12 @@ <h4 class="settings-section-header">{{_('Feature Configuration')}}</h4>
246246
<div class="form-group">
247247
<input type="checkbox" id="config_allow_reverse_proxy_header_login" name="config_allow_reverse_proxy_header_login" data-control="reverse-proxy-login-settings" {% if config.config_allow_reverse_proxy_header_login %}checked{% endif %}>
248248
<label for="config_allow_reverse_proxy_header_login">{{_('Allow Reverse Proxy Authentication')}}</label>
249+
<div class="help-block">{{_('Trusts the "Remote-User" header from a reverse proxy for authentication. This is an authentication method, distinct from the HTTPS security setting below.')}}</div>
250+
</div>
251+
<div class="form-group">
252+
<input type="checkbox" id="config_use_https" name="config_use_https" {% if config.config_use_https %}checked{% endif %}>
253+
<label for="config_use_https">{{_('Use via HTTPS')}}</label>
254+
<div class="help-block">{{_('Enables secure cookie settings (Secure, SameSite=Lax). Enable this ONLY if you are accessing the server via HTTPS, otherwise you will be locked out.')}}</div>
249255
</div>
250256
<div data-related="reverse-proxy-login-settings">
251257
<div class="form-group">
@@ -268,7 +274,7 @@ <h4 class="settings-section-header">{{_('Feature Configuration')}}</h4>
268274
<option value="1" {% if config.config_login_type == 1 %}selected{% endif %}>{{_('Use LDAP Authentication')}}</option>
269275
{% endif %}
270276
{% if feature_support['oauth'] %}
271-
<option value="2" {% if config.config_login_type == 2 %}selected{% endif %}>{{_('Use OAuth')}}</option>
277+
<option value="2" {% if config.config_login_type == 2 %}selected{% endif %}>{{_('Use OAuth (requires HTTPS)')}}</option>
272278
{% endif %}
273279
</select>
274280
</div>
@@ -381,6 +387,9 @@ <h4 class="text-center">{{_('Following Settings are Needed For User Import')}}</
381387
{% endif %}
382388
{% if feature_support['oauth'] %}
383389
<div data-related="login-settings-2">
390+
<p class="text-danger" style="margin-top: 2rem; background: #ffe9ec; padding: 1rem; border-left: 4px solid #a94442; padding-inline: 2rem; margin-bottom: 2rem;">
391+
<strong>{{_('Important:')}}</strong> {{_('OAuth authentication requires this server to be accessed via HTTPS. If you are using HTTP, login will fail.')}}
392+
</p>
384393
{% set generic = provider | selectattr('provider_name', 'equalto', 'generic') | first %}
385394
<p>{{ _('Generic OAuth Provider') }}</p>
386395

@@ -628,6 +637,9 @@ <h4 class="settings-section-header">{{_('Security Settings')}}</h4>
628637
}
629638
});
630639

640+
// Trigger change on load to set initial state
641+
$('#config_login_type').trigger('change');
642+
631643
$('#test_oidc_connection').on('click', function() {
632644
var serverUrl = $('#config_generic_oauth_server_url_test').val() || $('#config_generic_oauth_server_url').val();
633645
var resultSpan = $('#oidc_test_result');

0 commit comments

Comments
 (0)