Skip to content

[IMP] add manifest-external-assets check #525

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ manifest-author-string | The author key in the manifest file must be a string (w
manifest-behind-migrations | Manifest version (%s) is lower than migration scripts (%s) | E8145
manifest-data-duplicated | The file "%s" is duplicated in lines %s from manifest key "%s" | W8125
manifest-deprecated-key | Deprecated key "%s" in manifest file | C8103
manifest-external-assets | Asset %s should be distributed with module's source code. More info at https://httptoolkit.com/blog/public-cdn-risks/ | W8162
manifest-maintainers-list | The maintainers key in the manifest file must be a list of strings | E8104
manifest-required-author | One of the following authors must be present in manifest: %s | C8101
manifest-required-key | Missing required key "%s" in manifest file | C8102
Expand Down Expand Up @@ -224,6 +225,12 @@ Checks valid only for odoo <= 13.0

- https://github.com/OCA/pylint-odoo/blob/v9.3.2/testing/resources/test_repo/broken_module/__openerp__.py#L7 Deprecated key "description" in manifest file

* manifest-external-assets

- https://github.com/OCA/pylint-odoo/blob/v9.3.2/testing/resources/test_repo/twelve_module/__manifest__.py#L15 Asset https://shady.cdn.com/somefile.js should be distributed with module's source code. More info at https://httptoolkit.com/blog/public-cdn-risks/
- https://github.com/OCA/pylint-odoo/blob/v9.3.2/testing/resources/test_repo/twelve_module/__manifest__.py#L19 Asset https://bad.idea.com/cool.css should be distributed with module's source code. More info at https://httptoolkit.com/blog/public-cdn-risks/
- https://github.com/OCA/pylint-odoo/blob/v9.3.2/testing/resources/test_repo/twelve_module/__manifest__.py#L20 Asset http://insecure.and.bad.idea.com/kiwi.js should be distributed with module's source code. More info at https://httptoolkit.com/blog/public-cdn-risks/

* manifest-maintainers-list

- https://github.com/OCA/pylint-odoo/blob/v9.3.2/testing/resources/test_repo/broken_module3/__openerp__.py#L6 The maintainers key in the manifest file must be a list of strings
Expand Down
33 changes: 33 additions & 0 deletions src/pylint_odoo/checkers/odoo_addons.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@
import string
import warnings
from collections import Counter, defaultdict
from urllib.parse import urlparse

from astroid import ClassDef, FunctionDef, NodeNG, nodes
from pylint.checkers import BaseChecker, utils
Expand Down Expand Up @@ -269,6 +270,11 @@
"prefer-env-translation",
CHECK_DESCRIPTION,
),
"W8162": (
"Asset %s should be distributed with module's source code. More info at https://httptoolkit.com/blog/public-cdn-risks/",
"manifest-external-assets",
CHECK_DESCRIPTION,
),
}

DFTL_MANIFEST_REQUIRED_KEYS = ["license"]
Expand Down Expand Up @@ -1042,6 +1048,7 @@ def visit_call(self, node):
"resource-not-exist",
"website-manifest-key-not-valid-uri",
"manifest-behind-migrations",
"manifest-external-assets",
)
def visit_dict(self, node):
if not os.path.basename(self.linter.current_file) in misc.MANIFEST_FILES or not isinstance(
Expand Down Expand Up @@ -1197,6 +1204,32 @@ def visit_dict(self, node):
):
self.add_message("manifest-maintainers-list", node=manifest_keys_nodes.get("maintainers") or node)

# Check there are no external assets
if self.linter.is_message_enabled("manifest-external-assets"):
assets_node = None
for item in node.items:
if item[0].value == "assets":
assets_node = item[1]

# it is important to use the actual astroid.node instead of manifest_dict, otherwise the
Copy link
Collaborator

@moylop260 moylop260 Mar 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to use manifest_dict + args in order to use the string value with the problem

I mean,

Currenly it is good showing the number of line:

twelve_module/__manifest__.py:15:12: {msg}
twelve_module/__manifest__.py:19:12: {msg}

But it could be good too using

twelve_module/__manifest__.py:12:14: {msg} {wrong url}
twelve_module/__manifest__.py:12:14: {msg} {wrong url}

So, the result final could be:

twelve_module/__manifest__.py:12:14: W8162: Assets https://shady.cdn.com/somefile.js should be distributed with module's source code. More info at https://httptoolkit.com/blog/public-cdn-risks/ (manifest-external-assets)
twelve_module/__manifest__.py:12:14: W8162: Assets https://bad.idea.com/cool.css should be distributed with module's source code. More info at https://httptoolkit.com/blog/public-cdn-risks/ (manifest-external-assets)

It should be good since that manifest_dict is more pythonistic than using astroid nodes

So, it is easy to maintain

Only consider a recent fix that I have added for values wrong formed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I originally tried using manifest_dict but had issues with test_180 which runs parallel jobs. It seems like message were being mangled up because all errors pointed to the same root node (the assets node), also the linenumber was wrong since we were pointing to assets node, instead of the actual offending URL.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point for parallels!!!

I didn't know it

I have bypassed a few checks for parallels

Could you check if you can fix them (another PR)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forget it

I think it is a different matter

# errors are not attributed to the proper node.
if assets_node:
self._check_manifest_external_assets(assets_node)

def _check_manifest_external_assets(self, node):
def is_external_url(url):
return urlparse(url).scheme

for _, item in node.items:
for element in item.elts:
if isinstance(element, nodes.Const):
if is_external_url(element.value):
self.add_message("manifest-external-assets", node=element, args=(element.value,))
elif isinstance(element, (nodes.Tuple, nodes.List)):
for entry in element.elts:
if isinstance(entry, nodes.Const) and is_external_url(entry.value):
self.add_message("manifest-external-assets", node=element, args=(entry.value,))

def check_deprecated_odoo_method(self, node: NodeNG) -> bool:
"""Verify the given method is not marked as deprecated under the set Odoo versions.
:param node: Function definition to be checked
Expand Down
11 changes: 11 additions & 0 deletions testing/resources/test_repo/twelve_module/__manifest__.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,15 @@
'data': [
'security/ir.model.access.csv',
],
"assets": {
"web.assets_common": [
"twelve_module/static/nonexistent.js",
"https://shady.cdn.com/somefile.js"
],
"web.assets_frontend": [
"/twelve_module/hypothetically/good/file.css",
("before", "/web/static/src/css/random.css", "https://bad.idea.com/cool.css"),
["prepend", "/web/static/src/js/hello.js", "http://insecure.and.bad.idea.com/kiwi.js"]
]
}
}
1 change: 1 addition & 0 deletions tests/test_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
"manifest-required-author": 1,
"manifest-required-key": 1,
"manifest-version-format": 3,
"manifest-external-assets": 3,
"method-compute": 1,
"method-inverse": 1,
"method-required-super": 8,
Expand Down