Skip to content

Commit c901cb8

Browse files
lokeshclaude
andcommitted
Improve return-loan error handling and redirect encoding
- Replace contextlib.suppress with explicit try/except for clearer intent - Consolidate redirect URL encoding into a single urllib.parse.quote call - Use urlunsplit for redirect sanitization (preserves fragment, simpler code) - Move stats.increment into success branch so failed returns aren't counted Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent e9af63a commit c901cb8

1 file changed

Lines changed: 17 additions & 24 deletions

File tree

openlibrary/plugins/upstream/borrow.py

Lines changed: 17 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
"""Handlers for borrowing books"""
22

3-
import contextlib
43
import copy
54
import hashlib
65
import hmac
@@ -163,18 +162,14 @@ def POST(self, key): # noqa: PLR0915
163162

164163
error_redirect = archive_url
165164

166-
# Sanitize i.redirect to prevent open redirect attacks by stripping
167-
# any host/scheme from the URL, preserving only the path and query string.
168-
parsed_redirect = urllib.parse.urlsplit(i.redirect) if i.redirect else None
169-
redirect_path = (
170-
(
171-
parsed_redirect.path
172-
+ ('?' + parsed_redirect.query if parsed_redirect.query else '')
165+
# Strip scheme/host to prevent open-redirect attacks.
166+
if i.redirect:
167+
parsed = urllib.parse.urlsplit(i.redirect)
168+
edition_redirect = urllib.parse.urlunsplit(
169+
('', '', parsed.path, parsed.query, parsed.fragment)
173170
)
174-
if parsed_redirect
175-
else ''
176-
)
177-
edition_redirect = redirect_path or edition.url()
171+
else:
172+
edition_redirect = edition.url()
178173

179174
user = accounts.get_current_user()
180175

@@ -187,35 +182,33 @@ def POST(self, key): # noqa: PLR0915
187182
) # invalidate cache for user loans
188183
if not user or not ia_itemname or not s3_keys:
189184
web.setcookie(config.login_cookie_name, "", expires=-1)
190-
redirect_url = f"/account/login?redirect={urllib.parse.quote(edition_redirect, safe='')}%2Fborrow%3Faction%3D{action}"
185+
return_path = f"{edition_redirect}/borrow?action={action}"
186+
redirect_url = f"/account/login?redirect={urllib.parse.quote(return_path, safe='')}"
191187
if i._autoReadAloud is not None:
192188
redirect_url += '&_autoReadAloud=' + i._autoReadAloud
193189
raise web.seeother(redirect_url)
194190

195191
if action == 'return':
196-
# Try to return the loan, catching PatronAccessException explicitly so we can verify if the loan is actually gone.
197-
with contextlib.suppress(lending.PatronAccessException):
192+
try:
198193
lending.s3_loan_api(s3_keys, ocaid=edition.ocaid, action='return_loan')
199-
stats.increment('ol.loans.return')
194+
except lending.PatronAccessException:
195+
pass # 400/409 — loan may already be gone; verified below
196+
200197
edition.update_loan_status()
201198
user.update_loan_status()
199+
title = edition.title or _('this book')
202200

203201
if user.has_borrowed(edition):
204-
# The return failed and they still have the loan
205202
add_flash_message(
206203
'error',
207204
_(
208205
'Unable to return %s. Please try again later or contact info@archive.org.'
209206
)
210-
% (edition.title or _('this book')),
207+
% title,
211208
)
212209
else:
213-
# The return succeeded or the loan was already expired/gone
214-
title = edition.title or _('this book')
215-
add_flash_message(
216-
'success',
217-
_('%s has been returned.') % title,
218-
)
210+
stats.increment('ol.loans.return')
211+
add_flash_message('success', _('%s has been returned.') % title)
219212
raise web.seeother(edition_redirect)
220213
elif action == 'join-waitinglist':
221214
lending.get_cached_user_waiting_loans.memcache_delete(

0 commit comments

Comments
 (0)