Fix loginpage.php Loop#973
Merged
Merged
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideIntroduces a guard against infinite redirects on the login page by tracking when the site is already in a 'logged-in but must log out first' state and short‑circuiting the AutoLogin redirection logic to return the user to their last page instead of re-opening loginpage.php. Sequence diagram for AutoLogin redirect with logined loop guardsequenceDiagram
actor User
participant Browser
participant UserScript
participant XMOJ_Server
User->>Browser: Open protected_page
Browser->>XMOJ_Server: GET protected_page
XMOJ_Server-->>Browser: HTML with login_required_message
Browser->>UserScript: Execute XMOJ.user.js
UserScript->>Browser: Read body_a_1_innerText
alt AutoLogin_enabled and login_required_message
UserScript->>Browser: Store last_page in localStorage
UserScript->>Browser: Redirect to loginpage_php
Browser->>XMOJ_Server: GET loginpage_php
XMOJ_Server-->>Browser: HTML with Please_logout_First_message
Browser->>UserScript: Execute XMOJ.user.js on loginpage
UserScript->>Browser: Read first_link_innerText
UserScript->>UserScript: Set logined = (first_link_innerText == Please_logout_First_message)
alt Profile_missing or login_required_on_other_pages
alt logined is true
UserScript->>Browser: Read last_page from localStorage
UserScript->>Browser: Redirect back to last_page
else logined is false
UserScript->>Browser: Redirect to loginpage_php (no loop guard)
end
else Already_on_profile_page
UserScript->>Browser: Do nothing
end
else AutoLogin_disabled or no_login_required_message
UserScript->>Browser: Do nothing
end
Flow diagram for AutoLogin logic with logined loop protectionflowchart TD
A[Start page_load] --> B[Check UtilityEnabled AutoLogin]
B -->|false| Z[End no_autologin]
B -->|true| C[Read first_link_innerText]
C --> D{first_link_is_please_login_then_continue}
D -->|true| E[Set last_page = pathname + search in localStorage]
E --> F[Set logined based on whether first_anchor_innerText equals Please_logout_First]
F --> G{logined is true}
G -->|true| H[Redirect to last_page_or_root]
G -->|false| I[Redirect to loginpage_php]
D -->|false| J[Check profile_element]
J --> K{profile_element is null}
K -->|true| L[Set logined based on whether first_anchor_innerText equals Please_logout_First]
L --> M{logined is true}
M -->|true| H
M -->|false| N[Redirect to loginpage_php]
K -->|false| O{profile_innerHTML is 登录 and pathname not login_pages}
O -->|true| P[Set logined based on whether first_anchor_innerText equals Please_logout_First]
P --> Q{logined is true}
Q -->|true| H
Q -->|false| R[Store last_page and redirect to loginpage_php]
O -->|false| Z
I --> Z
N --> Z
R --> Z
H --> Z[End]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
hendragon-bot
Bot
added
the
user-script
Deploying xmoj-script-dev-channel with
|
| Latest commit: |
e23fdd6
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b510b6e1.xmoj-script-dev-channel.pages.dev |
| Branch Preview URL: | https://fix-loginpage-loop-nqpm.xmoj-script-dev-channel.pages.dev |
2 tasks
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
loginedvariable assumesdocument.querySelector('a')is non-null and that the first<a>always contains the"Please logout First!"text; consider adding a null check and using a more specific selector tied to the logout element to avoid runtime errors and brittle behavior. - The redirect-back logic using
localStorage.getItem("UserScript-LastPage") ?? "/"is duplicated in several places; extracting this into a small helper function would make the flow easier to maintain and less error-prone. - The login-state checks mix English (
"Please logout First!") and Chinese UI text ("请登录后继续操作"), which may diverge across locales or future UI changes; consider centralizing these string constants or using a more robust way to detect login state than matching literal innerText.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `logined` variable assumes `document.querySelector('a')` is non-null and that the first `<a>` always contains the `"Please logout First!"` text; consider adding a null check and using a more specific selector tied to the logout element to avoid runtime errors and brittle behavior.
- The redirect-back logic using `localStorage.getItem("UserScript-LastPage") ?? "/"` is duplicated in several places; extracting this into a small helper function would make the flow easier to maintain and less error-prone.
- The login-state checks mix English (`"Please logout First!"`) and Chinese UI text (`"请登录后继续操作"`), which may diverge across locales or future UI changes; consider centralizing these string constants or using a more robust way to detect login state than matching literal innerText.
## Individual Comments
### Comment 1
<location path="XMOJ.user.js" line_range="914" />
<code_context>
});
//otherwise CurrentUsername might be undefined
+let logined = (document.querySelector('a').innerText == "Please logout First!");
if (UtilityEnabled("AutoLogin") && document.querySelector("body > a:nth-child(1)") != null && document.querySelector("body > a:nth-child(1)").innerText == "请登录后继续操作") {
+ if (logined) location.href = (localStorage.getItem("UserScript-LastPage") == null ? "/" : localStorage.getItem("UserScript-LastPage"));
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against `document.querySelector('a')` returning null before accessing `.innerText`.
If the page has no `<a>` tags or the first anchor is added later, `document.querySelector('a')` will be `null` and this will throw. Add a null check, for example:
```js
const firstAnchor = document.querySelector('a');
const logined = !!firstAnchor && firstAnchor.innerText === 'Please logout First!';
```
This avoids a runtime error and keeps `logined` as a boolean.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="XMOJ.user.js">
<violation number="1" location="XMOJ.user.js:3">
P3: Do not manually bump `@version`; this repository's workflow updates script versions automatically.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // ==UserScript== | ||
| // @name XMOJ | ||
| // @version 3.4.3 | ||
| // @version 3.4.4 |
Contributor
There was a problem hiding this comment.
P3: Do not manually bump @version; this repository's workflow updates script versions automatically.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At XMOJ.user.js, line 3:
<comment>Do not manually bump `@version`; this repository's workflow updates script versions automatically.</comment>
<file context>
@@ -1,6 +1,6 @@
// ==UserScript==
// @name XMOJ
-// @version 3.4.3
+// @version 3.4.4
// @description XMOJ增强脚本
// @author @XMOJ-Script-dev, @langningchen and the community
</file context>
Suggested change
| // @version 3.4.4 | |
| // @version 3.4.3 |
Signed-off-by: zsTree <wa2025666@gmail.com>
Contributor
There was a problem hiding this comment.
3 issues found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="XMOJ.user.js">
<violation number="1" location="XMOJ.user.js:914">
P1: Guard `document.querySelector('a')` before reading `.innerText` to avoid a startup crash on pages without anchor tags.</violation>
<violation number="2" location="XMOJ.user.js:914">
P1: Guard against `document.querySelector('a')` returning `null` before accessing `.innerText`. If the page has no `<a>` tags, this will throw a `TypeError` at runtime, potentially breaking the entire userscript.</violation>
<violation number="3" location="XMOJ.user.js:916">
P1: The new `logined` redirect is immediately overwritten by unconditional `location.href = loginpage.php` in the same blocks; add early exit/else so the back-redirect actually works.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Member
|
The version number needs to be bumped |
Signed-off-by: zsTree <wa2025666@gmail.com>
Signed-off-by: zsTree <wa2025666@gmail.com>
Signed-off-by: zsTree <wa2025666@gmail.com>
Member
Author
|
@boomzero Fixed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR aim to accomplish?
Fix loginpage.php loop.
How does this PR accomplish the above?
Check if
Please logout First!is displayed on loginpage.phpBy submitting this pull request, I confirm the following:
git rebase)Summary by Sourcery
Bug Fixes:
Summary by cubic
Fix the redirect loop on
loginpage.phpwhen AutoLogin is enabled by prefetching the page, detecting the “Please logout First!” state, and skipping redirects if already logged in. Bumps to3.4.5and updates release metadata inUpdate.json.loginpage.phpand setloginedwhen “Please logout First!” is found.!loginedto stop infinite loops.UserScript-LastPagebefore redirecting.Written for commit e23fdd6. Summary will update on new commits.