Add school CAPTCHA handling to login flow (backend and frontend) - #166
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a captcha verification system for the school login process, adding a new backend endpoint to fetch captcha images and updating the frontend to support captcha input. The authentication service and fetcher have been modified to handle captcha tokens and maintain session context. Feedback identifies a logic issue where the captcha should be refreshed on any login failure to prevent stale sessions, a security concern regarding the exposure of raw exception messages to users, and a suggestion to use more idiomatic Python for updating session cookies.
|
|
||
| if not success: | ||
| return jsonify({'success': False, 'message': message}), 401 | ||
| need_refresh_captcha = '驗證碼' in (message or '') |
There was a problem hiding this comment.
The captcha should be refreshed on any login failure, not just when the error message mentions "驗證碼". Since school_login_context is removed from the session at line 81, the current captcha becomes unusable for any subsequent attempts. If a user mistypes their password but enters the correct captcha, they will be prompted to try again with the same (now invalid) captcha image, leading to a frustrating "invalid captcha" error on their next attempt.
| need_refresh_captcha = '驗證碼' in (message or '') | |
| need_refresh_captcha = True |
| return True, "OK", payload | ||
| except Exception as e: | ||
| _log('error', '', f"Prepare captcha exception: {e}") | ||
| return False, f"取得學校驗證碼失敗: {str(e)}", None |
There was a problem hiding this comment.
Exposing the raw exception message (str(e)) to the end-user can leak internal implementation details or server-side information. It is better to return a generic, user-friendly error message while logging the full exception details for debugging purposes (which you are already doing at line 119).
| for k, v in (login_context.get("cookies") or {}).items(): | ||
| if k and v is not None: | ||
| s.cookies.set(k, v) |
There was a problem hiding this comment.
Manually iterating over the cookies dictionary to set them in the session is less efficient and less idiomatic than using the built-in update method of the cookie jar.
s.cookies.update(login_context.get("cookies") or {})There was a problem hiding this comment.
Pull request overview
Integrates the school-side image CAPTCHA into the login flow by adding a captcha-fetch endpoint, carrying a short-lived login context via session, and updating the UI to display/refresh the CAPTCHA and submit the user-entered code during login.
Changes:
- Added
GET /api/school-captchato fetch the school CAPTCHA image and store a login context in the server session. - Extended backend login + fetcher/auth service to accept
captcha_codeand replay the login context (cookies/token) into the school login POST. - Updated frontend login modal (HTML/CSS/JS) to show CAPTCHA image, input field, refresh button, and auto-refresh on captcha-related failures.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| public/index.html | Adds CAPTCHA image/refresh UI and input field to the login modal. |
| frontend/sync.js | Implements CAPTCHA fetch/refresh and includes captcha_code in /api/login requests. |
| frontend/styles/modal.css | Styles the CAPTCHA row and image in the modal. |
| fetcher.py | Adds CAPTCHA preparation + context serialization, and posts captcha fields during login. |
| app/services/auth_service.py | Passes captcha_code/login_context through to the fetcher. |
| app/routes/auth.py | Adds /api/school-captcha, enforces captcha in login, stores/pops login context, and returns need_refresh_captcha. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if not captcha_code: | ||
| return jsonify({'success': False, 'message': '請輸入驗證碼'}), 400 |
There was a problem hiding this comment.
目前 /api/login 會在任何情況下要求 captcha_code（即使沒有先取得 school_login_context）。這與 PR 描述的「當學校要求時才需要 CAPTCHA」不一致，也會讓在不需要 CAPTCHA 的情境下無法登入。建議改為：只有在 session 中存在 school_login_context（或後端偵測到校方要求 CAPTCHA）時才強制 captcha_code，否則允許空值並讓 fetcher 走原本不含 CAPTCHA 的登入流程。
| @bp.route('/api/school-captcha') | ||
| def school_captcha(): | ||
| fetcher = current_app.config['GRADE_FETCHER'] | ||
| success, message, payload = fetcher.prepare_login_captcha() | ||
| if not success: | ||
| return jsonify({'success': False, 'message': message}), 502 | ||
|
|
||
| session['school_login_context'] = payload['context'] | ||
| return jsonify({'success': True, 'image_data_url': payload['image_data_url']}) |
There was a problem hiding this comment.
GET /api/school-captcha 會寫入 session（school_login_context），屬於有副作用的 GET，容易被瀏覽器預取/快取機制或中介層誤觸發，且也不利於語意一致性。建議改為 POST，或至少回應加上 Cache-Control: no-store 並評估避免任何自動預取場景。
| @bp.route('/api/school-captcha') | ||
| def school_captcha(): | ||
| fetcher = current_app.config['GRADE_FETCHER'] | ||
| success, message, payload = fetcher.prepare_login_captcha() | ||
| if not success: | ||
| return jsonify({'success': False, 'message': message}), 502 | ||
|
|
||
| session['school_login_context'] = payload['context'] | ||
| return jsonify({'success': True, 'image_data_url': payload['image_data_url']}) |
There was a problem hiding this comment.
/api/school-captcha 會對校方發出外部請求並產生 session 狀態，但目前沒有 Turnstile 或速率限制；攻擊者可高頻呼叫造成對校方的壓力與自身資源耗盡。建議比照 /api/login 或 /api/share 增加 is_rate_limited（可用不同 key_prefix），必要時也可要求 turnstile_token。
| fetcher = current_app.config['GRADE_FETCHER'] | ||
| success, message, payload = login_and_build_session_payload(fetcher, username, password) | ||
| school_login_context = session.get('school_login_context') | ||
| success, message, payload = login_and_build_session_payload( | ||
| fetcher, | ||
| username, | ||
| password, | ||
| captcha_code=captcha_code, | ||
| login_context=school_login_context, | ||
| ) | ||
|
|
||
| # 驗證碼一次性使用，避免重放 | ||
| session.pop('school_login_context', None) | ||
|
|
There was a problem hiding this comment.
school_login_context 目前寫入 session 後沒有任何過期檢查；若使用者長時間停留或 session 被重用，context 可能不再對應當前 CAPTCHA（也與「short-lived」目標不符）。建議在 context 中存 timestamp，並在 /api/login 驗證其 age（例如 1–3 分鐘），過期則拒絕並要求重新取得 CAPTCHA。
| context = { | ||
| "login_token": login_token, | ||
| "shcaptcha_gen_code": shcaptcha_gen_code, | ||
| "cookies": {c.name: c.value for c in s.cookies if c.name and c.value is not None}, | ||
| } |
There was a problem hiding this comment.
prepare_login_captcha() 將 requests CookieJar 序列化成 {name: value}，會遺失 domain/path/secure 等屬性；後續 login_and_get_tokens() 再用 s.cookies.set(k, v) 還原時，cookie 可能不會被送回正確的 host，導致校方把 CAPTCHA/anti-forgery token 視為不同會話而登入失敗。建議序列化完整 cookie 屬性（至少 domain/path），並用 set_cookie/create_cookie 逐一還原。
| if k and v is not None: | ||
| s.cookies.set(k, v) |
There was a problem hiding this comment.
login_and_get_tokens() 從 login_context 還原 cookies 時使用 s.cookies.set(k, v) 未指定 domain/path，可能導致 cookies 不會匹配 shcloud2.k12ea.gov.tw 而不被送出，破壞 CAPTCHA 與 token 的同一會話關聯。建議用完整 cookie 物件還原（帶 domain/path），或至少在 set() 時指定 domain='shcloud2.k12ea.gov.tw'、path='/'。
| if k and v is not None: | |
| s.cookies.set(k, v) | |
| if not k or v is None: | |
| continue | |
| if isinstance(v, dict): | |
| cookie_value = v.get("value") | |
| cookie_domain = v.get("domain") or "shcloud2.k12ea.gov.tw" | |
| cookie_path = v.get("path") or "/" | |
| else: | |
| cookie_value = v | |
| cookie_domain = "shcloud2.k12ea.gov.tw" | |
| cookie_path = "/" | |
| if cookie_value is not None: | |
| s.cookies.set(k, cookie_value, domain=cookie_domain, path=cookie_path) |
| def prepare_login_captcha(self): | ||
| """Prepare school login captcha and context for subsequent login POST.""" | ||
| try: | ||
| s = self.session_factory() | ||
| r = s.get(self.LOGIN_PAGE) | ||
| r.raise_for_status() | ||
|
|
||
| html = r.text | ||
| login_token = self._get_hidden_token(html) | ||
| shcaptcha_gen_code = self._extract_hidden_input(html, "ShCaptchaGenCode", "10") | ||
| captcha_url = self._build_captcha_url() | ||
|
|
||
| image_resp = s.get( | ||
| captcha_url, | ||
| headers={ | ||
| "Referer": self.LOGIN_PAGE, | ||
| "X-Requested-With": "XMLHttpRequest", | ||
| } | ||
| ) | ||
| image_resp.raise_for_status() | ||
| content_type = image_resp.headers.get("Content-Type", "image/png") | ||
|
|
||
| if "image" not in content_type.lower(): | ||
| return False, "學校驗證碼回應格式異常", None | ||
|
|
||
| image_b64 = base64.b64encode(image_resp.content).decode("ascii") | ||
|
|
||
| context = { | ||
| "login_token": login_token, | ||
| "shcaptcha_gen_code": shcaptcha_gen_code, | ||
| "cookies": {c.name: c.value for c in s.cookies if c.name and c.value is not None}, | ||
| } | ||
|
|
||
| payload = { | ||
| "image_data_url": f"data:{content_type};base64,{image_b64}", | ||
| "context": context, | ||
| } | ||
| return True, "OK", payload | ||
| except Exception as e: | ||
| _log('error', '', f"Prepare captcha exception: {e}") | ||
| return False, f"取得學校驗證碼失敗: {str(e)}", None | ||
|
|
There was a problem hiding this comment.
prepare_login_captcha() 內部建立了新的 HTTP session 並完成兩次 GET 後就回傳序列化資料，但沒有 close() 該 session；此端點可能在 UI 開啟時頻繁呼叫，長期可能造成連線資源累積。建議在成功與例外路徑都確保關閉 session（例如 try/finally 或 with 方式）。
| def login_and_build_session_payload(fetcher, username, password, captcha_code=None, login_context=None): | ||
| success, message, cookies, student_no, token = fetcher.login_and_get_tokens( | ||
| username, | ||
| password, | ||
| captcha_code=captcha_code, | ||
| login_context=login_context, | ||
| ) |
There was a problem hiding this comment.
login_and_build_session_payload() 新增了 captcha_code/login_context 並轉傳給 fetcher，但 tests/backend/test_auth.py 目前只覆蓋舊呼叫型態，無法防止未來轉傳參數被移除或拼錯。建議補一個測試：傳入 captcha_code/login_context 時，assert fetcher.login_and_get_tokens() 以相同 kwargs 被呼叫。
Motivation
school_login_contextbetween the captcha fetch and the login POST.Description
GET /api/school-captchathat callsGradeFetcher.prepare_login_captcha()and stores aschool_login_contextin the session, returning adata:image for display.captcha_code, validate presence, passcaptcha_codeandlogin_contextintologin_and_build_session_payload(), andsession.pop('school_login_context')after use; login responses now includeneed_refresh_captchawhen the failure message mentions captcha.auth_service.login_and_build_session_payload()to acceptcaptcha_codeandlogin_contextand forward them to the fetcher.GradeFetcherwithprepare_login_captcha(),_build_captcha_url(),_extract_hidden_input(), and updatedlogin_and_get_tokens()to acceptcaptcha_codeandlogin_context, restore cookies from the provided context, and includeShCaptcha/ShCaptchaGenCodewhen posting the login.loadSchoolCaptcha()and refresh logic infrontend/sync.js; includedcaptcha_codein the/api/loginPOST and automatically reloads the captcha when the server indicatesneed_refresh_captcha.Testing
Codex Task