Skip to content

Commit 5ec5f6f

Browse files
Copilotmekarpeles
andcommitted
Implement signed verification cookies and code quality improvements
- Add create_verification_cookie_value() and verify_verification_cookie() to accounts/model.py - Update is_suspicious_visitor() to verify signed cookies instead of simple vf=1 - Update account_verify_human endpoint to create signed cookies - Remove inline styles from challenge.html template - Run black formatter on all modified Python files - Update tests to work with signed cookie verification - Add test for referer header check Co-authored-by: mekarpeles <978325+mekarpeles@users.noreply.github.com>
1 parent dbc6b2f commit 5ec5f6f

File tree

5 files changed

+141
-25
lines changed

5 files changed

+141
-25
lines changed

openlibrary/accounts/model.py

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,60 @@ def get_secret_key():
9595
return config.infobase['secret_key']
9696

9797

98+
def create_verification_cookie_value() -> str:
99+
"""Create a signed verification cookie value.
100+
101+
Returns:
102+
str: Cookie value in format "vf/session_id,signature"
103+
"""
104+
# Generate random session ID (server-side only)
105+
session_id = secrets.token_urlsafe(32)
106+
107+
# Create the data portion
108+
data = f"vf/{session_id}"
109+
110+
# Sign it (creates: "salt$hash")
111+
signature = generate_hash(get_secret_key(), data)
112+
113+
# Combine into single cookie value
114+
cookie_value = f"{data},{signature}"
115+
# Result: "vf/kJ8x3mP...,a1b2c$d3e4f5"
116+
117+
return cookie_value
118+
119+
120+
def verify_verification_cookie(cookie_value: str) -> bool:
121+
"""Verify a verification cookie value.
122+
123+
Args:
124+
cookie_value: The cookie value to verify
125+
126+
Returns:
127+
bool: True if cookie is valid and properly signed, False otherwise
128+
"""
129+
if not cookie_value:
130+
return False
131+
132+
# Parse the single cookie
133+
parts = cookie_value.split(",")
134+
if len(parts) != 2:
135+
return False # Malformed cookie
136+
137+
data, signature = parts
138+
# data = "vf/kJ8x3mP..."
139+
# signature = "a1b2c$d3e4f5"
140+
141+
# Verify the signature (NO database lookup needed!)
142+
if not verify_hash(get_secret_key(), data, signature):
143+
return False # Cookie was tampered with or forged
144+
145+
# Extract session ID (we can trust it because signature is valid)
146+
if not data.startswith("vf/"):
147+
return False
148+
149+
return True
150+
151+
98152
def generate_login_code_for_user(username: str) -> str:
99153
"""
100154
Args:

openlibrary/plugins/openlibrary/code.py

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1239,7 +1239,7 @@ def is_suspicious_visitor():
12391239
A suspicious visitor is someone who is NOT:
12401240
1. a recognized bot
12411241
2. coming from a referer
1242-
3. carrying a verification cookie (vf=1)
1242+
3. carrying a valid signed verification cookie
12431243
4. logged in
12441244
12451245
Returns:
@@ -1253,8 +1253,11 @@ def is_suspicious_visitor():
12531253
if web.ctx.env.get('HTTP_REFERER'):
12541254
return False
12551255

1256-
# Check if visitor has already been verified (has vf=1 cookie)
1257-
if web.cookies().get('vf') == '1':
1256+
# Check if visitor has a valid signed verification cookie
1257+
from openlibrary.accounts.model import verify_verification_cookie
1258+
1259+
cookie_value = web.cookies().get('vf')
1260+
if cookie_value and verify_verification_cookie(cookie_value):
12581261
return False
12591262

12601263
# Check if user is logged in

openlibrary/plugins/openlibrary/tests/test_verification.py

Lines changed: 53 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,11 @@ def test_is_suspicious_visitor_when_logged_in(self, monkeypatch):
2828
mock_user = web.storage(key='/people/test_user')
2929
web.ctx.site.get_user = lambda: mock_user
3030

31-
# Mock is_bot to return False
32-
monkeypatch.setattr(code, 'is_bot', lambda: False)
31+
# Mock is_recognized_bot to return False
32+
monkeypatch.setattr(code, 'is_recognized_bot', lambda: False)
33+
34+
# Mock no referer
35+
web.ctx.env = {}
3336

3437
# Mock no verification cookie
3538
def mock_cookies():
@@ -44,8 +47,8 @@ def test_is_suspicious_visitor_when_bot(self, monkeypatch):
4447
# Mock no logged in user
4548
web.ctx.site.get_user = lambda: None
4649

47-
# Mock is_bot to return True
48-
monkeypatch.setattr(code, 'is_bot', lambda: True)
50+
# Mock is_recognized_bot to return True
51+
monkeypatch.setattr(code, 'is_recognized_bot', lambda: True)
4952

5053
# Mock no verification cookie
5154
def mock_cookies():
@@ -55,17 +58,44 @@ def mock_cookies():
5558

5659
assert code.is_suspicious_visitor() is False
5760

58-
def test_is_suspicious_visitor_with_cookie(self, monkeypatch):
59-
"""Test that visitors with vf=1 cookie are not suspicious."""
61+
def test_is_suspicious_visitor_with_valid_cookie(self, monkeypatch):
62+
"""Test that visitors with valid signed cookie are not suspicious."""
63+
from openlibrary.accounts import model
64+
6065
# Mock no logged in user
6166
web.ctx.site.get_user = lambda: None
6267

63-
# Mock is_bot to return False
64-
monkeypatch.setattr(code, 'is_bot', lambda: False)
68+
# Mock is_recognized_bot to return False
69+
monkeypatch.setattr(code, 'is_recognized_bot', lambda: False)
70+
71+
# Mock no referer
72+
web.ctx.env = {}
73+
74+
# Create a valid signed cookie
75+
valid_cookie = model.create_verification_cookie_value()
6576

6677
# Mock verification cookie present
6778
def mock_cookies():
68-
return {'vf': '1'}
79+
return {'vf': valid_cookie}
80+
81+
monkeypatch.setattr(web, 'cookies', mock_cookies)
82+
83+
assert code.is_suspicious_visitor() is False
84+
85+
def test_is_suspicious_visitor_with_referer(self, monkeypatch):
86+
"""Test that visitors with referer are not suspicious."""
87+
# Mock no logged in user
88+
web.ctx.site.get_user = lambda: None
89+
90+
# Mock is_recognized_bot to return False
91+
monkeypatch.setattr(code, 'is_recognized_bot', lambda: False)
92+
93+
# Mock referer present
94+
web.ctx.env = {'HTTP_REFERER': 'https://example.com'}
95+
96+
# Mock no verification cookie
97+
def mock_cookies():
98+
return {}
6999

70100
monkeypatch.setattr(web, 'cookies', mock_cookies)
71101

@@ -76,8 +106,11 @@ def test_is_suspicious_visitor_when_all_checks_fail(self, monkeypatch):
76106
# Mock no logged in user
77107
web.ctx.site.get_user = lambda: None
78108

79-
# Mock is_bot to return False
80-
monkeypatch.setattr(code, 'is_bot', lambda: False)
109+
# Mock is_recognized_bot to return False
110+
monkeypatch.setattr(code, 'is_recognized_bot', lambda: False)
111+
112+
# Mock no referer
113+
web.ctx.env = {}
81114

82115
# Mock no verification cookie
83116
def mock_cookies():
@@ -87,17 +120,20 @@ def mock_cookies():
87120

88121
assert code.is_suspicious_visitor() is True
89122

90-
def test_is_suspicious_visitor_with_wrong_cookie_value(self, monkeypatch):
91-
"""Test that visitors with wrong cookie value are still suspicious."""
123+
def test_is_suspicious_visitor_with_invalid_cookie(self, monkeypatch):
124+
"""Test that visitors with invalid cookie value are still suspicious."""
92125
# Mock no logged in user
93126
web.ctx.site.get_user = lambda: None
94127

95-
# Mock is_bot to return False
96-
monkeypatch.setattr(code, 'is_bot', lambda: False)
128+
# Mock is_recognized_bot to return False
129+
monkeypatch.setattr(code, 'is_recognized_bot', lambda: False)
130+
131+
# Mock no referer
132+
web.ctx.env = {}
97133

98-
# Mock verification cookie with wrong value
134+
# Mock verification cookie with invalid value
99135
def mock_cookies():
100-
return {'vf': '0'}
136+
return {'vf': 'invalid_value'}
101137

102138
monkeypatch.setattr(web, 'cookies', mock_cookies)
103139

openlibrary/plugins/upstream/account.py

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1317,8 +1317,15 @@ class account_verify_human(delegate.page):
13171317

13181318
def POST(self):
13191319
"""Handle verification request."""
1320-
# Set the verification cookie (vf=1)
1321-
web.setcookie('vf', '1', expires=self.VERIFICATION_COOKIE_EXPIRES_SECONDS)
1320+
from openlibrary.accounts.model import create_verification_cookie_value
1321+
1322+
# Create a signed verification cookie
1323+
cookie_value = create_verification_cookie_value()
1324+
1325+
# Set the verification cookie
1326+
web.setcookie(
1327+
'vf', cookie_value, expires=self.VERIFICATION_COOKIE_EXPIRES_SECONDS
1328+
)
13221329

13231330
# Track verification success
13241331
stats.increment('ol.stats.verify_human.verified')

openlibrary/templates/lib/challenge.html

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,29 @@
44
<h1>$_("Human Verification")</h1>
55
</div>
66

7-
<div id="contentBody" style="text-align: center; padding: 2rem;">
8-
<p style="margin-bottom: 2rem;">$_("Please verify you are human to continue.")</p>
9-
<button id="verify-human-btn" class="cta-btn cta-btn--shell" style="padding: 1rem 2rem; font-size: 1.1rem;">
7+
<div id="contentBody" class="verification-challenge">
8+
<p class="verification-message">$_("Please verify you are human to continue.")</p>
9+
<button id="verify-human-btn" class="cta-btn cta-btn--shell">
1010
$_("Verify you are human")
1111
</button>
1212
</div>
1313

14+
<style>
15+
.verification-challenge {
16+
text-align: center;
17+
padding: 2rem;
18+
}
19+
20+
.verification-message {
21+
margin-bottom: 2rem;
22+
}
23+
24+
#verify-human-btn {
25+
padding: 1rem 2rem;
26+
font-size: 1.1rem;
27+
}
28+
</style>
29+
1430
<script>
1531
(function() {
1632
var button = document.getElementById('verify-human-btn');

0 commit comments

Comments
 (0)