Closed
Conversation
Reviewer's GuideRefactors contest problem list retrieval into a reusable function and wires it into the problem switcher so that the list is persisted in localStorage and can be refreshed via a new UI control, fixing stale problem listings. Sequence diagram for refreshed contest problem list via ProblemSwitchersequenceDiagram
actor User
participant Browser
participant ProblemSwitcherUI
participant GetContestProblemList
participant XMOJServer
participant LocalStorage
User->>ProblemSwitcherUI: Click refresh link
ProblemSwitcherUI->>GetContestProblemList: GetContestProblemList(true)
GetContestProblemList->>XMOJServer: HTTP GET contest.php?cid
XMOJServer-->>GetContestProblemList: HTML contest page
GetContestProblemList->>GetContestProblemList: Parse HTML and build problemList
GetContestProblemList->>LocalStorage: setItem(UserScript_Contest_cid_ProblemList, problemList)
alt RefreshList is true
GetContestProblemList->>Browser: location.reload()
Browser->>Browser: Reload page
Browser->>LocalStorage: getItem(UserScript_Contest_cid_ProblemList)
Browser-->>Browser: ContestProblemList JSON
Browser->>ProblemSwitcherUI: Build buttons from ContestProblemList
end
Flow diagram for GetContestProblemList functionflowchart TD
A[Start GetContestProblemList RefreshList] --> B[Build contest URL with cid from SearchParams]
B --> C[fetch contest.php?cid]
C --> D{Request succeeded and status 200?}
D -->|No| J[End]
D -->|Yes| E[Read response text]
E --> F{Response contains forbidden message?}
F -->|Yes| J
F -->|No| G[Parse HTML with DOMParser]
G --> H[Select #problemset tbody rows]
H --> I[Iterate rows and build problemList with title and url]
I --> K[localStorage.setItem UserScript_Contest_cid_ProblemList]
K --> L{RefreshList is true?}
L -->|No| J
L -->|Yes| M[location.reload]
M --> J[End]
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
请向 |
hendragon-bot
bot
added
the
user-script
Contributor
|
请向 |
1 similar comment
Contributor
|
请向 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 security issues, 2 other issues, and left some high level feedback:
Security issues:
- User controlled data in methods like
innerHTML,outerHTMLordocument.writeis an anti-pattern that can lead to XSS vulnerabilities (link) - User controlled data in a
problemSwitcher.innerHTMLis an anti-pattern that can lead to XSS vulnerabilities (link)
General comments:
- In
main(),unsafeWindow.GetContestProblemList(false)is async but not awaited before immediately reading fromlocalStorage, which can lead to a race whereContestProblemListis still null; consider awaiting the call or refactoring to use the returned data directly. - The inline
onclick="GetContestProblemList(true)"handler couples HTML to a global function; it would be more robust to attach the click listener via JavaScript to avoid relying on a global name and to make the async handling explicit. GetContestProblemListsilently fails when the contest is private or not started and may leaveContestProblemListnull, which will causeJSON.parse(ContestProblemList)to throw; consider guarding the parse or handling the no-data case more gracefully.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `main()`, `unsafeWindow.GetContestProblemList(false)` is async but not awaited before immediately reading from `localStorage`, which can lead to a race where `ContestProblemList` is still null; consider awaiting the call or refactoring to use the returned data directly.
- The inline `onclick="GetContestProblemList(true)"` handler couples HTML to a global function; it would be more robust to attach the click listener via JavaScript to avoid relying on a global name and to make the async handling explicit.
- `GetContestProblemList` silently fails when the contest is private or not started and may leave `ContestProblemList` null, which will cause `JSON.parse(ContestProblemList)` to throw; consider guarding the parse or handling the no-data case more gracefully.
## Individual Comments
### Comment 1
<location path="XMOJ.user.js" line_range="2296" />
<code_context>
- localStorage.setItem("UserScript-Contest-" + SearchParams.get("cid") + "-ProblemList", JSON.stringify(problemList));
- ContestProblemList = JSON.stringify(problemList);
- }
+ unsafeWindow.GetContestProblemList(false);
+ ContestProblemList = localStorage.getItem("UserScript-Contest-" + SearchParams.get("cid") + "-ProblemList");
}
</code_context>
<issue_to_address>
**issue (bug_risk):** The async `GetContestProblemList` call is not awaited before reading from `localStorage`, which can lead to a race condition.
Previously the fetch and `localStorage.setItem` completed inline before `ContestProblemList` was used. Now `GetContestProblemList` is `async`, but `unsafeWindow.GetContestProblemList(false);` is called without `await` and `localStorage` is read immediately afterward, so `ContestProblemList` may still be `null`, causing `JSON.parse(ContestProblemList)` to throw. Please either `await unsafeWindow.GetContestProblemList(false);` (marking the caller `async` if needed) before reading from `localStorage`, or refactor `GetContestProblemList` to return the list directly instead of going through `localStorage`.
</issue_to_address>
### Comment 2
<location path="XMOJ.user.js" line_range="2294-2300" />
<code_context>
}
let ContestProblemList = localStorage.getItem("UserScript-Contest-" + SearchParams.get("cid") + "-ProblemList");
if (ContestProblemList == null) {
- const contestReq = await fetch("https://www.xmoj.tech/contest.php?cid=" + SearchParams.get("cid"));
</code_context>
<issue_to_address>
**issue (bug_risk):** `JSON.parse` is called even when `ContestProblemList` may still be `null`, which can throw at runtime.
On failure of `GetContestProblemList` (network error, non-200, contest not visible), the `localStorage` key isn’t set and `ContestProblemList` stays `null`. Since `JSON.parse(null)` throws `SyntaxError`, this refactor makes that failure path more likely. Please guard before parsing (e.g., handle the `null` case explicitly or default to an empty array) instead of parsing `null`.
</issue_to_address>
### Comment 3
<location path="XMOJ.user.js" line_range="2320" />
<code_context>
problemSwitcher.innerHTML += `<a href="javascript:void(0)" onclick="GetContestProblemList(true)" title="刷新列表" class="mb-2" style="text-align: center;" active>刷新</a>`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-document-method):** User controlled data in methods like `innerHTML`, `outerHTML` or `document.write` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="XMOJ.user.js" line_range="2320" />
<code_context>
problemSwitcher.innerHTML += `<a href="javascript:void(0)" onclick="GetContestProblemList(true)" title="刷新列表" class="mb-2" style="text-align: center;" active>刷新</a>`;
</code_context>
<issue_to_address>
**security (javascript.browser.security.insecure-innerhtml):** User controlled data in a `problemSwitcher.innerHTML` is an anti-pattern that can lead to XSS vulnerabilities
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| localStorage.setItem("UserScript-Contest-" + SearchParams.get("cid") + "-ProblemList", JSON.stringify(problemList)); | ||
| ContestProblemList = JSON.stringify(problemList); | ||
| } | ||
| unsafeWindow.GetContestProblemList(false); |
There was a problem hiding this comment.
issue (bug_risk): The async GetContestProblemList call is not awaited before reading from localStorage, which can lead to a race condition.
Previously the fetch and localStorage.setItem completed inline before ContestProblemList was used. Now GetContestProblemList is async, but unsafeWindow.GetContestProblemList(false); is called without await and localStorage is read immediately afterward, so ContestProblemList may still be null, causing JSON.parse(ContestProblemList) to throw. Please either await unsafeWindow.GetContestProblemList(false); (marking the caller async if needed) before reading from localStorage, or refactor GetContestProblemList to return the list directly instead of going through localStorage.
Comment on lines
2294
to
2300
| let ContestProblemList = localStorage.getItem("UserScript-Contest-" + SearchParams.get("cid") + "-ProblemList"); | ||
| if (ContestProblemList == null) { | ||
| const contestReq = await fetch("https://www.xmoj.tech/contest.php?cid=" + SearchParams.get("cid")); | ||
| const res = await contestReq.text(); | ||
| if (contestReq.status === 200 && res.indexOf("比赛尚未开始或私有，不能查看题目。") === -1) { | ||
| const parser = new DOMParser(); | ||
| const dom = parser.parseFromString(res, "text/html"); | ||
| const rows = (dom.querySelector("#problemset > tbody")).rows; | ||
| let problemList = []; | ||
| for (let i = 0; i < rows.length; i++) { | ||
| problemList.push({ | ||
| "title": rows[i].children[2].innerText, | ||
| "url": rows[i].children[2].children[0].href | ||
| }); | ||
| } | ||
| localStorage.setItem("UserScript-Contest-" + SearchParams.get("cid") + "-ProblemList", JSON.stringify(problemList)); | ||
| ContestProblemList = JSON.stringify(problemList); | ||
| } | ||
| unsafeWindow.GetContestProblemList(false); | ||
| ContestProblemList = localStorage.getItem("UserScript-Contest-" + SearchParams.get("cid") + "-ProblemList"); | ||
| } | ||
|
|
||
| let problemSwitcher = document.createElement("div"); |
There was a problem hiding this comment.
issue (bug_risk): JSON.parse is called even when ContestProblemList may still be null, which can throw at runtime.
On failure of GetContestProblemList (network error, non-200, contest not visible), the localStorage key isn’t set and ContestProblemList stays null. Since JSON.parse(null) throws SyntaxError, this refactor makes that failure path more likely. Please guard before parsing (e.g., handle the null case explicitly or default to an empty array) instead of parsing null.
| problemSwitcher.style.flexDirection = "column"; | ||
|
|
||
| let problemList = JSON.parse(ContestProblemList); | ||
| problemSwitcher.innerHTML += `<a href="javascript:void(0)" onclick="GetContestProblemList(true)" title="刷新列表" class="mb-2" style="text-align: center;" active>刷新</a>`; |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-document-method): User controlled data in methods like innerHTML, outerHTML or document.write is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
| problemSwitcher.style.flexDirection = "column"; | ||
|
|
||
| let problemList = JSON.parse(ContestProblemList); | ||
| problemSwitcher.innerHTML += `<a href="javascript:void(0)" onclick="GetContestProblemList(true)" title="刷新列表" class="mb-2" style="text-align: center;" active>刷新</a>`; |
There was a problem hiding this comment.
security (javascript.browser.security.insecure-innerhtml): User controlled data in a problemSwitcher.innerHTML is an anti-pattern that can lead to XSS vulnerabilities
Source: opengrep
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue 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:2296">
P1: Await the async cache refresh before reading `ContestProblemList`, otherwise first-load problem switcher rendering still races the fetch and can crash on `problemList.length`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| localStorage.setItem("UserScript-Contest-" + SearchParams.get("cid") + "-ProblemList", JSON.stringify(problemList)); | ||
| ContestProblemList = JSON.stringify(problemList); | ||
| } | ||
| unsafeWindow.GetContestProblemList(false); |
There was a problem hiding this comment.
P1: Await the async cache refresh before reading ContestProblemList, otherwise first-load problem switcher rendering still races the fetch and can crash on problemList.length.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At XMOJ.user.js, line 2296:
<comment>Await the async cache refresh before reading `ContestProblemList`, otherwise first-load problem switcher rendering still races the fetch and can crash on `problemList.length`.</comment>
<file context>
@@ -2270,22 +2293,8 @@ async function main() {
- localStorage.setItem("UserScript-Contest-" + SearchParams.get("cid") + "-ProblemList", JSON.stringify(problemList));
- ContestProblemList = JSON.stringify(problemList);
- }
+ unsafeWindow.GetContestProblemList(false);
+ ContestProblemList = localStorage.getItem("UserScript-Contest-" + SearchParams.get("cid") + "-ProblemList");
}
</file context>
Suggested change
| unsafeWindow.GetContestProblemList(false); | |
| await unsafeWindow.GetContestProblemList(false); |
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
1 participant
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 *EX problems not display.
How does this PR accomplish the above?
Update list when click the refresh button. (The refresh button is located at the top of the problem switcher)
By submitting this pull request, I confirm the following:
git rebase)Summary by Sourcery
Bug Fixes:
Summary by cubic
Fixes the Problem Switcher not updating so new and EX problems show up after a manual refresh. Adds a refresh control that fetches the latest contest problem list and reloads the view.
GetContestProblemListto parse the contest page, cache results inlocalStorage, and optionally reload.Written for commit cb969d2. Summary will update on new commits.