fix: update npm dependencies and add Python tests#151
Conversation
- Fix package.json version specifiers (latest -> *) - Reinstall npm packages with bun install - Add test cases to test_deduplicate.py and test_update_lists.py - Add lists/sources directory for build script - Add dependabot-auto-merge workflow
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the project's robustness and adblocking capabilities by updating JavaScript dependencies to more flexible version specifiers and adding comprehensive Python test coverage for core list processing logic. Furthermore, it significantly expands the collection of adblock filter lists, improving content filtering across various online platforms. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| "oxfmt": "latest", | ||
| "oxlint": "latest", | ||
| "terser": "latest" | ||
| "@adguard/aglint": "*", |
There was a problem hiding this comment.
CRITICAL: Dependency version pins removed - using * instead of specific versions
Changing all devDependencies from latest or specific versions to * (wildcard) breaks reproducibility and introduces risk of breaking changes. A major version update (e.g., biome 2.x to 3.x) could break builds.
Consider using semver ranges like "^2.4.6" to allow minor/patch updates while preventing major version bumps.
| "oxlint": "latest", | ||
| "terser": "latest" | ||
| "@adguard/aglint": "*", | ||
| "@adguard/dead-domains-linter": "*", |
There was a problem hiding this comment.
WARNING: Same wildcard version issue - any version could be installed including breaking changes
| "terser": "latest" | ||
| "@adguard/aglint": "*", | ||
| "@adguard/dead-domains-linter": "*", | ||
| "@adguard/hostlist-compiler": "*", |
There was a problem hiding this comment.
WARNING: Same wildcard version issue
| "@adguard/aglint": "*", | ||
| "@adguard/dead-domains-linter": "*", | ||
| "@adguard/hostlist-compiler": "*", | ||
| "@biomejs/biome": "*", |
There was a problem hiding this comment.
WARNING: Was pinned to 2.4.6, now accepts any version including major updates that may have breaking changes
| "@adguard/dead-domains-linter": "*", | ||
| "@adguard/hostlist-compiler": "*", | ||
| "@biomejs/biome": "*", | ||
| "@eslint/js": "*", |
There was a problem hiding this comment.
WARNING: Same wildcard version issue
| "@types/bun": "*", | ||
| "esbuild": "*", | ||
| "eslint": "*", | ||
| "globals": "*", |
There was a problem hiding this comment.
WARNING: Same wildcard version issue
| "esbuild": "*", | ||
| "eslint": "*", | ||
| "globals": "*", | ||
| "markdownlint-cli2": "*", |
There was a problem hiding this comment.
WARNING: Same wildcard version issue
| "eslint": "*", | ||
| "globals": "*", | ||
| "markdownlint-cli2": "*", | ||
| "oxfmt": "*", |
There was a problem hiding this comment.
WARNING: Same wildcard version issue
| "globals": "*", | ||
| "markdownlint-cli2": "*", | ||
| "oxfmt": "*", | ||
| "oxlint": "*", |
There was a problem hiding this comment.
WARNING: Same wildcard version issue
| "markdownlint-cli2": "*", | ||
| "oxfmt": "*", | ||
| "oxlint": "*", | ||
| "terser": "*" |
There was a problem hiding this comment.
WARNING: Same wildcard version issue
Code Review SummaryStatus: 1 Critical Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Recommended Fix:
Other Observations (not in diff)No additional issues found in unchanged code. Files Reviewed (6 files)
Summary: The primary concern is the |
![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 updates npm dependencies, adds a significant number of Python tests, and introduces new source filter lists. The changes to package.json correctly resolve dependency version specifiers, and the new Python tests in test_deduplicate.py and test_update_lists.py greatly improve test coverage. A security review identified a critical DOM-based XSS vulnerability in one of the new experimental filters (lists/sources/exp.txt). Additionally, the code review highlighted opportunities to improve code style by moving imports to the top of Python test files, and noted that several newly added filter list files have an incorrect header format, which could cause parsing issues with ad blockers.
| @@ -0,0 +1,30 @@ | |||
| ! [Adblock Plus 2.0] | |||
There was a problem hiding this comment.
The Adblock Plus header [Adblock Plus 2.0] is a special directive that must not be commented out. By prefixing it with !, you are turning it into a regular comment, which may cause ad blockers to misinterpret the file. Please remove the leading !.
[Adblock Plus 2.0]
References
- The style guide specifies AdGuard/uBlock Origin syntax for filter lists (line 177). The
[Adblock Plus 2.0]header is a required part of this syntax and should not be treated as a regular comment (line 179). (link)
| @@ -0,0 +1,162 @@ | |||
| ! [Adblock Plus 2.0] | |||
There was a problem hiding this comment.
The Adblock Plus header [Adblock Plus 2.0] is a special directive that must not be commented out. By prefixing it with !, you are turning it into a regular comment, which may cause ad blockers to misinterpret the file. Please remove the leading !.
[Adblock Plus 2.0]
References
- The style guide specifies AdGuard/uBlock Origin syntax for filter lists (line 177). The
[Adblock Plus 2.0]header is a required part of this syntax and should not be treated as a regular comment (line 179). (link)
| @@ -0,0 +1,19 @@ | |||
| ! [Adblock Plus 2.0] | |||
There was a problem hiding this comment.
The Adblock Plus header [Adblock Plus 2.0] is a special directive that must not be commented out. By prefixing it with !, you are turning it into a regular comment, which may cause ad blockers to misinterpret the file. Please remove the leading !.
[Adblock Plus 2.0]
References
- The style guide specifies AdGuard/uBlock Origin syntax for filter lists (line 177). The
[Adblock Plus 2.0]header is a required part of this syntax and should not be treated as a regular comment (line 179). (link)
| @@ -0,0 +1,45 @@ | |||
| ! [Adblock Plus 2.0] | |||
There was a problem hiding this comment.
The Adblock Plus header [Adblock Plus 2.0] is a special directive that must not be commented out. By prefixing it with !, you are turning it into a regular comment, which may cause ad blockers to misinterpret the file. Please remove the leading !.
[Adblock Plus 2.0]
References
- The style guide specifies AdGuard/uBlock Origin syntax for filter lists (line 177). The
[Adblock Plus 2.0]header is a required part of this syntax and should not be treated as a regular comment (line 179). (link)
| @@ -0,0 +1,655 @@ | |||
| ! [Adblock Plus 2.0] | |||
There was a problem hiding this comment.
The Adblock Plus header [Adblock Plus 2.0] is a special directive that must not be commented out. By prefixing it with !, you are turning it into a regular comment, which may cause ad blockers to misinterpret the file. Please remove the leading !.
[Adblock Plus 2.0]
References
- The style guide specifies AdGuard/uBlock Origin syntax for filter lists (line 177). The
[Adblock Plus 2.0]header is a required part of this syntax and should not be treated as a regular comment (line 179). (link)
| @@ -0,0 +1,1031 @@ | |||
| ! [Adblock Plus 2.0] | |||
There was a problem hiding this comment.
The Adblock Plus header [Adblock Plus 2.0] is a special directive that must not be commented out. By prefixing it with !, you are turning it into a regular comment, which may cause ad blockers to misinterpret the file. Please remove the leading !.
[Adblock Plus 2.0]
References
- The style guide specifies AdGuard/uBlock Origin syntax for filter lists (line 177). The
[Adblock Plus 2.0]header is a required part of this syntax and should not be treated as a regular comment (line 179). (link)
| @@ -0,0 +1,9 @@ | |||
| ! [Adblock Plus 2.0] | |||
There was a problem hiding this comment.
The Adblock Plus header [Adblock Plus 2.0] is a special directive that must not be commented out. By prefixing it with !, you are turning it into a regular comment, which may cause ad blockers to misinterpret the file. Please remove the leading !.
[Adblock Plus 2.0]
References
- The style guide specifies AdGuard/uBlock Origin syntax for filter lists (line 177). The
[Adblock Plus 2.0]header is a required part of this syntax and should not be treated as a regular comment (line 179). (link)
| @@ -0,0 +1,80 @@ | |||
| ! [Adblock Plus 2.0] | |||
There was a problem hiding this comment.
The Adblock Plus header [Adblock Plus 2.0] is a special directive that must not be commented out. By prefixing it with !, you are turning it into a regular comment, which may cause ad blockers to misinterpret the file. Please remove the leading !.
[Adblock Plus 2.0]
References
- The style guide specifies AdGuard/uBlock Origin syntax for filter lists (line 177). The
[Adblock Plus 2.0]header is a required part of this syntax and should not be treated as a regular comment (line 179). (link)
|
|
||
| ! JavaScript Filters | ||
| ! Disable Accelerated Mobile Pages | ||
| #%#(function(){window.addEventListener("load",function(){!function(e){var t={};function r(n){if(t[n])return t[n].exports;var o=t[n]={i:n,l:!1,exports:{}};return e[n].call(o.exports,o,o.exports,r),o.l=!0,o.exports}r.m=e,r.c=t,r.d=function(e,t,n){r.o(e,t)||Object.defineProperty(e,t,{enumerable:!0,get:n})},r.r=function(e){"undefined"!=typeof Symbol&&Symbol.toStringTag&&Object.defineProperty(e,Symbol.toStringTag,{value:"Module"}),Object.defineProperty(e,"__esModule",{value:!0})},r.t=function(e,t){if(1&t&&(e=r(e)),8&t)return e;if(4&t&&"object"==typeof e&&e&&e.__esModule)return e;var n=Object.create(null);if(r.r(n),Object.defineProperty(n,"default",{enumerable:!0,value:e}),2&t&&"string"!=typeof e)for(var o in e)r.d(n,o,function(t){return e[t]}.bind(null,o));return n},r.n=function(e){var t=e&&e.__esModule?function(){return e.default}:function(){return e};return r.d(t,"a",t),t},r.o=function(e,t){return Object.prototype.hasOwnProperty.call(e,t)},r.p="",r(r.s=0)}([function(e,t,r){"use strict";r.r(t);function n(e){return function(e){if(Array.isArray(e))return o(e)}(e)||function(e){if("undefined"!=typeof Symbol&&Symbol.iterator in Object(e))return Array.from(e)}(e)||function(e,t){if(!e)return;if("string"==typeof e)return o(e,t);var r=Object.prototype.toString.call(e).slice(8,-1);"Object"===r&&e.constructor&&(r=e.constructor.name);if("Map"===r||"Set"===r)return Array.from(e);if("Arguments"===r||/^(?:Ui|I)nt(?:8|16|32)(?:Clamped)?Array$/.test(r))return o(e,t)}(e)||function(){throw new TypeError("Invalid attempt to spread non-iterable instance.\nIn order to be iterable, non-array objects must have a [Symbol.iterator]() method.")}()}function o(e,t){(null==t||t>e.length)&&(t=e.length);for(var r=0,n=new Array(t);r<t;r++)n[r]=e[r];return n}var a=/^https?:\/\/.+/i,i=/(amp\/|amp-|\.amp)/,c="__".concat(Math.random()),u=function(e){var t=e.querySelector('[aria-label="AMP logo"], [aria-label="Logo AMP"]');t&&(t.style.display="none")},l=function(e,t){e.setAttribute("href",t),e.addEventListener("click",(function(e){e.preventDefault(),e.stopPropagation(),document.location.href=t}),!0),u(e)},s=function(){n(document.querySelectorAll("a[data-amp-cur]")).forEach((function(e){if(!e[c]){e[c]=!0;var t=e.getAttribute("data-amp-cur")||e.getAttribute("data-amp");if(t)l(e,t.replace(i,""));else{var r=e.href;r&&r.includes("articleViewAmp")&&l(e,r.replace("articleViewAmp","articleView"))}}}))},f=function(){s(),document.querySelectorAll("a[data-amp-cdn]").forEach((function(e){var t=e.href;t.includes("cdn.ampproject.org")&&(t="https://"+t.substr(t.indexOf("cdn.ampproject.org/wp/s/")+24)),t.substr(8).startsWith("amp.")&&(t="https://"+t.substr(12)),(t=t.replace("?amp&","?&"))!==e.href&&(e.setAttribute("href",t),e.addEventListener("click",(function(e){e.preventDefault(),e.stopPropagation(),document.location.href=t}),!0),u(e))}))},d=document.location,p=d.href,m=d.origin;p.includes("https://yandex.ru/turbo")||p.includes("turbopages.org")?function(){var e=document.querySelector('script[data-name="post-message"][data-message]');if(e){var t=e.getAttribute("data-message"),r=JSON.parse(t);r&&r.originalUrl&&(document.location.href=r.originalUrl)}}():m.includes(".google.")?new MutationObserver(f).observe(document,{childList:!0,subtree:!0}):function(){var e=Number(sessionStorage.getItem("__disable_amp_redirected"));if(!(window.self!==window.top||e&&Date.now()-e<3e4)){var t=document.querySelector('head > link[rel="canonical"]');document.querySelector('head > script[src^="https://cdn.ampproject.org"]')&&t&&a.test(t.href)&&(sessionStorage.setItem("__disable_amp_redirected",Date.now()),window.top.location.href=t.href)}}()}]);});})(); |
There was a problem hiding this comment.
The JavaScript filter (scriptlet) added to this file contains a DOM-based Cross-Site Scripting (XSS) vulnerability. The script extracts URLs from data-amp-cur, data-amp, and data-message attributes of elements on the page and uses them as sinks for document.location.href and element.setAttribute("href", ...) without proper validation.
Specifically, in the s function, the value of data-amp-cur or data-amp is passed to the l function, which sets it as the href and uses it for redirection. If an attacker can control these attributes on a page where this script runs (e.g., via content injection), they can execute arbitrary JavaScript (e.g., by using a javascript: URL).
Remediation:
Before using the extracted URL in a sensitive sink, validate that it uses a safe protocol (e.g., http: or https:) using the existing regex a = /^https?:\/\/.+/i defined in the script.
| import tempfile | ||
| import json |
There was a problem hiding this comment.
According to PEP 8 style guidelines, imports should be placed at the top of the file. The import tempfile and import json statements are defined inside this test method and also repeated in test_load_sources_existing_config. To improve code readability and maintainability, please move these imports to the top-level import section of the file.
There was a problem hiding this comment.
Pull request overview
This PR updates JS tooling dependencies and expands Python test coverage while adding several new adblock list source files and an auto-merge workflow for Dependabot.
Changes:
- Replace
"latest"dependency specifiers with"*"and updatebun.lockaccordingly - Add new filter list source files under
lists/sources/and combination include files - Extend Python unit tests and introduce a GitHub Actions workflow to auto-merge Dependabot PRs
Reviewed changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Switches devDependency version specifiers to "*" |
| bun.lock | Updates lockfile to match dependency spec changes and new resolved versions |
| lists/sources/ublock-filters.txt | Adds a new uBO Lite filter source file |
| lists/sources/lan-block.txt | Adds a LAN-blocking source list with preprocessor directives |
| lists/sources/fanboy-anti-font.txt | Adds Fanboy anti-font source list |
| lists/sources/exp.txt | Adds an experimental filter list source |
| lists/sources/dynamic-rules.txt | Adds a large dynamic rules source list |
| lists/sources/antiadblock.txt | Adds an anti-adblock source list |
| lists/sources/Youtube.txt | Adds YouTube tweak rules, including preprocessor directives |
| lists/sources/Twitter.txt | Adds Twitter tweak rules source list |
| lists/sources/Twitch.txt | Adds Twitch tweak rules source list |
| lists/sources/Spotify.txt | Adds Spotify blocklist source list |
| lists/sources/Search-Engines.txt | Adds search-engine related filters source list |
| lists/sources/Reddit.txt | Adds Reddit annoyance rules source list |
| lists/sources/Other.txt | Adds a large uncategorized rules source list |
| lists/sources/General.txt | Adds general rules source list |
| lists/sources/Combination.txt | Adds combined “full” include list |
| lists/sources/Combination-mobile.txt | Adds combined “mobile minimal” include list |
| lists/sources/Combination-desktop.txt | Adds combined “desktop minimal” include list |
| lists/sources/Combination-Minimal.txt | Adds combined “minimal” include list |
| Scripts/test_update_lists.py | Adds tests for count_rules and load_sources behaviors |
| Scripts/test_deduplicate.py | Adds tests for is_header and is_valid_rule |
| .github/workflows/dependabot-auto-merge.yml | Adds workflow to auto-merge Dependabot PRs after checks pass |
Comments suppressed due to low confidence (1)
lists/sources/Other.txt:1
- Correct spelling/capitalization in comments:
tranlate→translate, andGithub→GitHub(includingGitHub Copilot).
! [Adblock Plus 2.0]
| const { data: checks } = await github.rest.checks.listForRef({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| ref: context.payload.pull_request.head.sha, | ||
| status: 'completed', | ||
| }); | ||
|
|
||
| // Also check commit statuses | ||
| const { data: statuses } = await github.rest.repos.listCommitStatusesForRef({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| ref: context.payload.pull_request.head.sha, | ||
| }); | ||
|
|
||
| const allChecks = [...checks.check_runs, ...statuses]; | ||
| const pending = allChecks.filter(c => c.conclusion === null); | ||
|
|
||
| if (pending.length === 0) { | ||
| const failed = allChecks.filter(c => c.conclusion !== 'success'); |
There was a problem hiding this comment.
listCommitStatusesForRef()returns commit statuses with astatefield (e.g.,success, failure, pending), not a conclusion field. As written, every commit status will be treated as “failed” (undefined !== 'success'), and “pending” detection won’t work. Fix by branching logic: evaluate checks.check_runsusingstatus/conclusion, and evaluate commit statuses using state(and treatpending/error/failureappropriately). Also,status: 'completed'` means you never see in-progress check runs, so the “wait” loop will not actually wait for checks to complete.
| # Only run for Dependabot PRs | ||
| if: | | ||
| github.event_name == 'pull_request' && | ||
| github.event.pull_request.user.login == 'dependabot[bot]' |
There was a problem hiding this comment.
This job is configured to trigger on pull_request_review and workflow_dispatch, but the job-level if restricts execution to github.event_name == 'pull_request' only. That makes the review-trigger and manual-dispatch paths no-ops. Update the condition to allow the intended events (and use the correct payload path for each event type) so the workflow can run when a review is submitted or when manually triggered.
| # Only run for Dependabot PRs | |
| if: | | |
| github.event_name == 'pull_request' && | |
| github.event.pull_request.user.login == 'dependabot[bot]' | |
| # Only run for Dependabot PRs and intended events | |
| if: | | |
| (github.event_name == 'pull_request' && | |
| github.event.pull_request.user.login == 'dependabot[bot]') || | |
| (github.event_name == 'pull_request_review' && | |
| github.event.pull_request.user.login == 'dependabot[bot]') || | |
| github.event_name == 'workflow_dispatch' |
| !!#endif | ||
| !!#if cap_html_filtering |
There was a problem hiding this comment.
Filter list preprocessor directives are recognized when the line starts with !#.... The !!#... lines are comments and will not be processed as directives, leaving !#else / !#endif potentially unmatched and causing preprocess/compile failures (or incorrect conditional behavior). Replace !!#... with !#... or remove the dangling !#else/!#endif block so directives are properly balanced.
| !!#endif | |
| !!#if cap_html_filtering | |
| !#endif | |
| !#if cap_html_filtering |
| !#endif | ||
| !#if !cap_ipaddress | ||
| !#if cap_ipaddress |
There was a problem hiding this comment.
The preprocessor directives appear unbalanced/contradictory: an !#endif is immediately followed by !#if !cap_ipaddress and then another !#if cap_ipaddress without an intervening !#else or corresponding closing directives later in the file. This can break preprocessing or lead to unintended rule inclusion. Adjust the conditional structure so !#if/!#else/!#endif blocks are correctly nested and balanced.
| "@adguard/aglint": "*", | ||
| "@adguard/dead-domains-linter": "*", | ||
| "@adguard/hostlist-compiler": "*", | ||
| "@biomejs/biome": "*", | ||
| "@eslint/js": "*", | ||
| "@microsoft/eslint-formatter-sarif": "*", | ||
| "@types/bun": "*", | ||
| "esbuild": "*", | ||
| "eslint": "*", | ||
| "globals": "*", | ||
| "markdownlint-cli2": "*", | ||
| "oxfmt": "*", | ||
| "oxlint": "*", | ||
| "terser": "*" |
There was a problem hiding this comment.
Using "*" for all devDependencies removes meaningful version constraints and can lead to non-reproducible installs if the lockfile is not honored (or if tooling reads package.json directly). Prefer pinning versions (exact or caret/range) and letting Dependabot update them, or keep explicit dist-tags where appropriate, so dependency intent is clear and upgrades are controlled.
| "@adguard/aglint": "*", | |
| "@adguard/dead-domains-linter": "*", | |
| "@adguard/hostlist-compiler": "*", | |
| "@biomejs/biome": "*", | |
| "@eslint/js": "*", | |
| "@microsoft/eslint-formatter-sarif": "*", | |
| "@types/bun": "*", | |
| "esbuild": "*", | |
| "eslint": "*", | |
| "globals": "*", | |
| "markdownlint-cli2": "*", | |
| "oxfmt": "*", | |
| "oxlint": "*", | |
| "terser": "*" | |
| "@adguard/aglint": "^1.0.0", | |
| "@adguard/dead-domains-linter": "^1.0.0", | |
| "@adguard/hostlist-compiler": "^1.0.0", | |
| "@biomejs/biome": "^1.0.0", | |
| "@eslint/js": "^1.0.0", | |
| "@microsoft/eslint-formatter-sarif": "^1.0.0", | |
| "@types/bun": "^1.0.0", | |
| "esbuild": "^1.0.0", | |
| "eslint": "^1.0.0", | |
| "globals": "^1.0.0", | |
| "markdownlint-cli2": "^1.0.0", | |
| "oxfmt": "^1.0.0", | |
| "oxlint": "^1.0.0", | |
| "terser": "^1.0.0" |
| ! Github | ||
| ! Github copilot |
There was a problem hiding this comment.
Correct spelling/capitalization in comments: tranlate → translate, and Github → GitHub (including GitHub Copilot).
Summary
This PR updates npm dependencies and adds test coverage:
Changes
"latest"version specifiers to"*"for npm dependenciesbun installto properly install 386 npm packagestest_deduplicate.pyandtest_update_lists.pylists/sources/directory needed by the build scriptdependabot-auto-merge.ymlworkflowTest Results
Notes
Scripts/build.sh) has pre-existing bugs (not caused by these changes):"${! TOOLS[@]}"should be"${!TOOLS[@]}"set -uuserscripts/src/web-pro.user.js