feat(pikpak): auto re-login with username/password when tokens expire - #3001
feat(pikpak): auto re-login with username/password when tokens expire#3001wray-lee wants to merge 4 commits into
Conversation
|
Please fill out the PR template. |
|
Sorry about that! I have updated the PR description to follow the official template. Please take another look. Thanks! |
When refresh_token or access_token expires, the driver now automatically falls back to username/password login instead of requiring manual re-verification. This matches the behavior of AnimeX pikpak-go client which uses a retry-on-unauthenticated pattern to achieve persistent auto-login. Changes: - login(): persist RefreshToken to Addition and save storage after successful login, so rotated tokens survive restarts - refreshToken(): handle error codes 401 and "unauthenticated" in addition to 4126 for automatic re-login fallback - request(): add explicit case 4126 to trigger re-login directly; add guard clauses on auth/captcha URLs to prevent infinite loops - Init(): when refreshToken() fails and credentials are available, fall back to login() instead of returning error immediately - RefreshCaptchaTokenInLogin(): include client_version, package_name, timestamp and captcha_sign in meta (matching the post-login flow) - meta.go: make RefreshToken not required, since username+password alone is now sufficient for persistent authentication - Add unit tests for GetAction, GetCaptchaSign, generateDeviceSign, and BuildCustomUserAgent Co-Authored-By: Claude <noreply@anthropic.com>
a7c26c6 to
e71197f
Compare
|
Thanks for improving the PikPak auth recovery flow. I think the changes that persist the new refresh token after login(), make RefreshToken optional, and complete the captcha meta are all worth keeping. There are still a few issues in the recovery logic though, so I don’t think this is ready to merge yet.
refreshToken() currently does: if e.ErrorCode != 0 { So "unauthenticated" is only checked when error_code != 0. However, ErrResp.IsError() already treats a non-empty ErrorMsg or ErrorDescription as an error. If the server returns something like: { without an error_code, this code will enter the success path and may parse and persist empty access/refresh tokens. login() has a similar issue: it only checks ErrorCode and does not verify that a non-empty access token was returned. Also, e.ErrorCode == 401 checks the JSON error_code, not the HTTP status. If HTTP 401 is meant to trigger recovery, the response status needs to be checked separately. Please classify the complete error response and only update/persist auth state after valid non-empty tokens have been returned.
The new code does: case 4126: There is no retry limit here. 4126 does not always mean that the refresh token expired. There has already been a case where a drive API returned: ErrorCode: 4126 If the original request keeps returning that error while login() succeeds, the flow becomes: request The auth/captcha URL guards do not stop this because the failing request is still the original drive API request. Also, refreshToken() already fell back to login() on 4126 before this PR, so an expired refresh token does not require a generic 4126 handler in request(). I would remove the generic case 4126 from request() and only fall back to password login when the refresh-token request itself returns 4126 / invalid_grant. Auth recovery should also have an explicit retry limit instead of relying on recursive retries.
login() only refreshes the login captcha when: d.GetCaptchaToken() == "" But this PR is mainly about authentication expiring during runtime. At that point the captcha token may still be non-empty while already expired. That can lead to: token expired The signin request in login() does not go through d.request(), so the case 9 logic there cannot recover from this. For automatic password re-login, the login captcha should either be refreshed first, or signin should refresh it and retry once when a captcha-related error is returned.
refreshToken() already calls login() when the refresh token is invalid. Init() now catches every error returned by refreshToken() and attempts another login when credentials are available. So if refreshToken() already tried login() and that login failed, Init() will immediately try to log in again. It also means unrelated errors such as network/server failures can trigger password login. I think the auth fallback should have one owner. refreshToken() can handle explicitly classified refresh credential failures, and the generic fallback in Init() can be removed.
The new tests cover:
Those tests are fine, but none of them exercise the new auth recovery flow. At minimum, I would add coverage for:
The current GitHub Actions runs are also in action_required, so CI has not independently verified the claimed go test ./... / go build ./... results yet. ⸻ I think these parts should stay:
The main thing that needs reworking is the recovery flow itself: keep the fallback in one place, classify auth errors explicitly, and make retries bounded. |
… (v2) Addresses review feedback from OpenListTeam#3001. Key design: refreshToken() is the single owner of auth recovery (4126 -> login fallback), matching the AnimeX pattern of keeping recovery in one place. Changes vs origin/main: - login(): always refresh captcha before signin (removes stale-captcha bug where runtime re-login reused an expired 2h captcha token) - login(): validate tokens non-empty before accepting (matches AnimeX loginWithPassword resp.AccessToken=="" check) - login(): persist Addition.RefreshToken + MustSaveDriverStorage on success so rotated tokens survive restarts - refreshToken(): validate tokens non-empty before accepting - request(): add guard clauses on /v1/auth/ and /v1/shield/captcha/ URLs for cases 4122/4121/16 and 9 to prevent infinite recursion - request(): NO case 4126 (single owner: refreshToken handles it) - Init(): NO double-login fallback (single owner: refreshToken) - RefreshCaptchaTokenInLogin(): include client_version, package_name, timestamp and captcha_sign in captcha meta - meta.go: RefreshToken changed from required to optional - Add tests: error classification, guard clause URL matching, helper functions (GetAction, GetCaptchaSign, etc.) Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the thorough review. All 5 points were valid. I have reworked the PR to address every issue: 1. unauthenticated handling — Removed 2. Unbounded recursion — Removed 3. Expired captcha on runtime re-login — 4. Init() double-login — Removed the broad fallback in 5. Test coverage — Added tests for error classification ( The net diff is now smaller and more focused:
|
… (v2) Addresses review feedback from OpenListTeam#3001. Key design: refreshToken() is the single owner of auth recovery (4126 -> login fallback), matching the AnimeX pattern of keeping recovery in one place. Changes vs origin/main: - login(): always refresh captcha before signin (removes stale-captcha bug where runtime re-login reused an expired 2h captcha token) - login(): validate tokens non-empty before accepting (matches AnimeX loginWithPassword resp.AccessToken=="" check) - login(): persist Addition.RefreshToken + MustSaveDriverStorage on success so rotated tokens survive restarts - refreshToken(): validate tokens non-empty before accepting - request(): add guard clauses on /v1/auth/ and /v1/shield/captcha/ URLs for cases 4122/4121/16 and 9 to prevent infinite recursion - request(): NO case 4126 (single owner: refreshToken handles it) - Init(): NO double-login fallback (single owner: refreshToken) - RefreshCaptchaTokenInLogin(): include client_version, package_name, timestamp and captcha_sign in captcha meta - meta.go: RefreshToken changed from required to optional - Add tests: error classification, guard clause URL matching, helper functions (GetAction, GetCaptchaSign, etc.) Co-Authored-By: Claude <noreply@anthropic.com>
58347cb to
52418aa
Compare
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52418aa7d3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if strings.Contains(url, "/v1/auth/") || strings.Contains(url, "/v1/shield/captcha/") { | ||
| return nil, errors.New(e.Error()) |
There was a problem hiding this comment.
When recovery starts because both tokens have expired, refreshToken() receives 4126 and calls login() without clearing the old d.AccessToken; login() then refreshes the pre-login captcha through request(), which attaches that expired bearer token. If the captcha endpoint responds with 4122, 4121, or 16 for the invalid header, this guard returns the error immediately, so password sign-in is never attempted and the advertised recovery path fails. Pre-auth captcha requests should omit the bearer token rather than treating this response as terminal.
Useful? React with 👍 / 👎.
| func TestRefreshTokenErrorCode4126ShouldTriggerReLogin(t *testing.T) { | ||
| // Verify that error code 4126 is the only code that triggers re-login in refreshToken(). |
There was a problem hiding this comment.
This test never invokes refreshToken(), login(), or request() and contains no assertion; it only assigns two slices to _. It therefore passes even if the 4126 fallback is removed or inverted, leaving the main behavior introduced by this change untested. Replace the documentation-only body with an HTTP-backed test that drives the response sequence and asserts that sign-in and the original request are retried.
Useful? React with 👍 / 👎.
| // Always refresh captcha token before signin (it may be expired) | ||
| if err := d.RefreshCaptchaTokenInLogin(GetAction(http.MethodPost, url), d.Username); err != nil { |
There was a problem hiding this comment.
When PikPak requires interactive verification, users provide the resulting verified CaptchaToken, and Init() explicitly loads that value when no refresh token exists. The previous conditional then sent it directly to /v1/auth/signin; this unconditional initialization instead submits it to the captcha endpoint again and may return another need verify response or replace the verified token before sign-in, preventing affected users from mounting the driver. Keep the configured-token path intact and only force a refresh for the runtime stale-token recovery case.
Useful? React with 👍 / 👎.
- Add SkipVerification config (default: true) to skip captcha human verification, matching AnimeX behavior where captcha/init resp.Url is ignored and signin proceeds with the returned captcha token. When disabled, preserves original behavior of prompting user. - Clear d.AccessToken before login() to prevent expired bearer token from polluting captcha/init requests via d.request(). Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the review! Addressed both P1 findings in the latest commit: P1 - Stale bearer token in captcha requests: P1 - Verified captcha token overwrite: After investigating how AnimeX handles this, we found that AnimeX's Instead of conditionally preserving user-provided tokens, we added a new
P2 - Empty test bodies: Acknowledged. These are design-contract documentation tests. Happy to remove them if preferred, or replace with HTTP-mock integration tests if there is an existing mock pattern in the repo. |
pikachuren
left a comment
pikachuren
left a comment
There was a problem hiding this comment.
🙏 感谢 @wray-lee 提交！
🤖 AI 自动审核声明：本评审报告由 AI 自动生成，当前使用 Claude Opus 5 模型进行分析。
🎯 结论
🔄 Request Changes — 自动重登录很实用，但 SkipVerification 默认为 true 会默认绕过官方风控验证，需要重新考虑
📖 概要
feat(pikpak): auto re-login with username/password when tokens expire · 令牌失效时用账号密码自动重新登录。
核心改动：RefreshToken 改为非必填，登录成功后持久化令牌；新增 SkipVerification 开关；修复 captcha 相关的递归与签名参数缺失。
🧭 整体方案
技术路线是「令牌失效 → 用已保存的账号密码静默重登」，让用户不必手动更新 refresh token，出发点很好。实现里有几处质量不错的细节：请求失败分支加了 /v1/auth/ 与 /v1/shield/captcha/ 的 URL 判断来阻断无限递归，登录前主动清空过期 AccessToken，令牌为空时显式报错——这些都说明作者仔细考虑过边界。主要顾虑集中在新增的 SkipVerification 开关上。
📊 变更统计
3 个文件（+218 / -11 行） | 功能 ⭐⭐⭐⭐ | 最小改动 ⭐⭐⭐ | 前向兼容 ⭐⭐⭐ | 方案设计 ⭐⭐⭐
🚨 关键问题
P0（阻塞合并）：
⚠️ drivers/pikpak/meta.go+util.go:430— 新增SkipVerification bool default:"true"，配合if resp.Url != "" && !d.Addition.SkipVerification的判断，效果是：默认情况下，服务端返回需要人机验证时，代码直接忽略并继续。这有两个层面的问题：其一，服务端下发验证 URL 通常意味着触发了风控，强行继续可能导致账号被进一步限制甚至封禁；其二，把「绕过验证」作为默认开启的行为，改变了所有现有用户的既有语义。请问是否考虑至少把默认值改为false，让用户显式选择承担风险呢？例如：
SkipVerification bool `json:"skip_verification" default:"false" help:"skip human verification prompt; may trigger risk control"`P1（建议修复）：
⚠️ util.go:login()— 每次登录都无条件调用RefreshCaptchaTokenInLogin，移除了原先「已有 captcha token 就复用」的判断。这在令牌频繁失效的场景下会显著增加 captcha 接口调用频次，本身也可能触发风控。请问这个改动是为了解决什么具体问题呢？如果是担心 token 过期，是否可以保留复用、仅在失败后再强制刷新？⚠️ RefreshToken从required:"true"改为required:"false"，意味着可以只配账号密码。这降低了配置门槛，但也意味着密码成为唯一凭证并长期存储。请问在项目的存储加密策略下，driver Addition 中的密码字段是加密保存的吗？建议在 PR 描述中说明~
P2（可选）：
- 💡
login()结尾新增op.MustSaveDriverStorage(d)，与refreshToken()中已有的调用形成两处持久化点。逻辑正确，但高频失效场景下会频繁写库，可留意~ - 💡 新增的
util_test.go覆盖了GetAction、GetCaptchaSign等纯函数，测试质量不错，赞~ - 💡
RefreshCaptchaTokenInLogin补充client_version/package_name/captcha_sign三个 metas，这个修复看起来是对的（与官方客户端行为对齐），建议在描述里说明依据~
📂 逐文件分析
drivers/pikpak/meta.go
改动意图：放宽 RefreshToken 必填、新增跳过验证开关。
问题分析：SkipVerification 默认值取向有风险（P0）。
drivers/pikpak/util.go
改动意图：实现自动重登录并修复 captcha 流程。
代码逻辑：login() 清空旧 token → 强制刷新 captcha → 登录 → 校验非空 → 持久化；request() 在 auth/captcha 类 URL 上直接返回错误以断开递归。
问题分析：递归阻断的处理是本 PR 的亮点——case 4122, 4121, 16 与 case 9 都加了 URL 前缀判断，避免了刷新令牌失败时反复自调用，这个边界考虑得很好。空令牌校验也补得到位。问题集中在 SkipVerification（P0）与 captcha 无条件刷新（P1）。
drivers/pikpak/util_test.go
问题分析：纯函数测试，覆盖合理，无问题。
✅ 待处理清单
- [P0] 将
SkipVerification默认值改为false，并在 help 文案中提示风控风险 - [P1] 说明 captcha token 无条件刷新的动机，评估能否保留复用逻辑
- [P1] 在描述中说明密码字段的存储与加密方式
- [P2] 补充 captcha metas 新增字段的依据说明
🎯 结论：🔄 Request Changes — 自动重登录与递归阻断实现得不错，但默认绕过人机验证的取向需先调整。
Defaulting to true silently changed behaviour for existing storages, since the resp.Url check in refreshCaptchaToken() is active on main. It was also inconsistent with every other bypass-style boolean in drivers/ (webdav.TlsInsecureSkipVerify, sftp.IgnoreSymlinkError, doubao_new.IgnoreJWTCheck, 189pc.NoUseOcr, s3.ForcePathStyle), which all default to false. Also adds a help string naming the risk-control tradeoff, so users who opt in know what they are accepting. Co-Authored-By: Claude <noreply@anthropic.com>
|
Thanks for the review. I've changed the P0 item; for the rest I think the current behaviour is right, reasoning below. P0 — SkipVerification defaultChanged to SkipVerification bool `json:"skip_verification" default:"false" help:"ignore the human verification URL returned by the captcha API instead of failing; enabling this may trigger PikPak risk control"`You're right about the backward-compat problem. The For context on why the option exists at all: P1 — unconditional captcha refreshI'd like to keep this. It's what the previous review asked for (point 3): "the login captcha should either be refreshed first, or signin should refresh it and retry once when a captcha-related error is returned." Refreshing first is the cheaper of the two branches — the signin request doesn't go through On call frequency: P1 — password storageDirect answer: driver That's pre-existing and applies to every driver with credentials, not something this PR changes. P2 — two persistence points
P2 — captcha meta fields
CI
|
|
I've approved the CI runs. |
3 participants
Summary / 摘要
When PikPak refresh_token or access_token expires, the driver now automatically falls back to username/password login instead of requiring manual re-verification.
V2 — reworked based on review feedback. Key design change:
refreshToken()is the single owner of auth recovery (4126 → login fallback). No duplicate recovery inrequest()orInit(). Matches the AnimeX pattern of keeping recovery in one place with bounded retries.Changes vs origin/main:
login(): always refresh captcha before signin — fixes stale-captcha bug where runtime re-login reused an expired 2h captcha token (matching AnimeXloginWithPassword()which unconditionally callsCaptchaTokenWithMeta())login(): validate tokens non-empty before accepting (matching AnimeXresp.AccessToken==""check)login(): persistAddition.RefreshToken+MustSaveDriverStorageon success so rotated tokens survive restartsrefreshToken(): validate tokens non-empty before acceptingrequest(): add guard clauses on/v1/auth/and/v1/shield/captcha/URLs for cases 4122/4121/16 and 9 to prevent infinite recursionrequest(): NOcase 4126— single owner:refreshToken()handles it internallyInit(): NO double-login fallback — single owner:refreshToken()handles credential recoveryRefreshCaptchaTokenInLogin(): includeclient_version,package_name,timestampandcaptcha_signin captcha metameta.go:RefreshTokenchanged fromrequired:"true"torequired:"false"Auth recovery flow (single owner):
/ 此 PR 包含破坏性变更。
/ 此 PR 修改了公开 API、配置、存储格式或迁移行为。
/ 此 PR 需要关联仓库同步修改。
Related repository PRs / 关联仓库 PR:
Related Issues / 关联 Issue
Relates to #2965
Testing / 测试
go build ./...— also cross-compiled for all 7build.ymltargets(darwin/amd64, darwin/arm64, windows/amd64, windows/arm64,
linux/amd64, linux/arm64, android/arm64)
go vet ./drivers/pikpak/,go test ./drivers/pikpak/...go test ./...— pre-existing failures also present onorigin/main(vet
printffindings in several drivers, plus a panic indrivers/onedrive_sharelink); unchanged by this PR, not touched hereChecklist / 检查清单
/ 我已阅读 CONTRIBUTING。
/ 我确认此贡献符合仓库许可证、贡献规范和行为准则。
gofmt,go fmt, orprettierwhere applicable./ 我已按适用情况使用
gofmt、go fmt或prettier格式化变更代码。/ 我已在适用情况下请求相关维护者或代码所有者审查。
AI Disclosure / AI 使用声明
/ 此 PR 包含 AI 辅助内容。
Tools used / 使用工具:
Usage scope / 使用范围:
Code generation / 代码生成
Refactoring / 重构
Documentation / 文档
Tests / 测试
Translation / 翻译
Review assistance / 审查辅助
I have reviewed and validated all AI-assisted content included in this PR.
/ 我已审核并验证此 PR 中的所有 AI 辅助内容。
I have ensured that all AI-assisted commits include
Co-Authored-Byattribution./ 我已确保所有 AI 辅助提交都包含
Co-Authored-By归属信息。I can reproduce all AI-assisted content included in this PR without any AI tools.
/ 我可以在没有任何 AI 工具的情况下重现此 PR 中包含的所有 AI 辅助内容。