Repository navigation
refactor: pass request params as args to is_spam and Recaptcha (#13538) - #13611
Conversation
…netarchive#13538) Require caller-provided form data in is_spam. Keep Recaptcha.validate as the web.py form shim so signup still works, and move the Google check to validate_response for add/edit book.
|
Thanks for the PR, @csy20 — and welcome, this looks like your first contribution to Open Library! 🎉 🤖 A reviewer has been assigned for an initial pass. PR triage checklist (maintainers / Pierre)
Note This comment was automatically generated by Pierre, Open Library's PR first-touch assistant, on behalf of @mekarpeles. This is NOT a code review or sign-off. |
There was a problem hiding this comment.
🟡 Changes recommended
addbook.py rereads the request body before saving, which can result in submitted fields not being saved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Refactors spam and reCAPTCHA helpers to receive request data explicitly while preserving signup validation behavior.
Changes:
- Requires explicit input for spam checks.
- Passes form data and client IP from edit handlers.
- Adds explicit reCAPTCHA response validation.
File summaries
| File | Summary |
|---|---|
openlibrary/plugins/upstream/spamcheck.py |
Removes the implicit request-input fallback. |
openlibrary/plugins/upstream/code.py |
Passes people-page POST data explicitly. |
openlibrary/plugins/upstream/addbook.py |
Passes form data to spam/reCAPTCHA checks; the save path must reuse parsed form data. |
openlibrary/plugins/recaptcha/recaptcha.py |
Adds explicit validation while preserving the signup form shim. |
Review details
Suppressed comments (1)
openlibrary/plugins/upstream/addbook.py:887
- This parses the full POST body, but the handler still calls
helper.save(web.input())below. A second read can see an empty body because fullweb.input()parsing drainswsgi.input(as documented inopenlibrary/utils/request_context.py), so work edits may pass these checks and then save no submitted fields. Reuseform_datafor the save.
recap = get_recaptcha()
if recap and not recap.validate_response(form_data.get("g-recaptcha-response"), web.ctx.ip):
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
RayBB
left a comment
There was a problem hiding this comment.
Thanks for this it looks great and is a step in the direction we need to go for fastapi.
Tested it locally pretty thoroughly and put it on testing and it it seems solid.
Closes #13538
Part of #13536. Thread spam and captcha request data as explicit arguments so these helpers no longer hide
web.input()/web.ctx.ipreads. Same behavior.This is a second attempt after #13547. That PR renamed
Recaptcha.validateso web.py forms passed the empty field value in as the Google token, which would fail every signup when reCAPTCHA is on. Ray's review asked to keepvalidate(self, value=None)as the form shim.Technical
spamcheck.is_spam(i, ...)now requiresi. Theweb.input()fallback is gone.book_edit.POST/work_edit.POSTpassweb.input()(full POST), not the GET-onlyi.edit.POSTfor people pages passesweb.input().Recaptcha.validate(self, value=None)stays registered as theweb.form.Validatorcallback (signup/account/create).Recaptcha.validate_response(response, remoteip)does the Google call with no web globals. Add/edit book call that.Testing
ruff check/ruff formaton the four files.python3.14 -m py_compileon the four files.make test-py-uv.Validator(..., self.validate), which still readsg-recaptcha-responsefrom the request. I do not have recaptcha keys in local docker, so I could not screenshot a live signup.Screenshot
N/A — no UI change. Recaptcha markup is the same.
Stakeholders
@RayBB