This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

Introduction

These security review guidelines try to describe the latest best practices, and should supersede any existing practice that may be found in our codebase. The whole codebase can't always be adapted whenever our CI checks are improved and entries are added here. Do not hesitate to contact the security team if you think there is an omission or error.

Why was my code code flagged?

The CI/security check identifies the use of dangerous methods. This does not mean your code is wrong but that it can be dangerous if misused. We ask you to verify explain why your usage is valid and why it won’t lead to security weaknesses.

Example of bad justifications:

• “I haven’t written the flagged line, only moved it.” Maybe the line was not flagged at that time? Maybe the reviewer missed it? Maybe the justification no longer hold (different parameters,…). Go check the justification at that time.
• “I followed the specification/X told me to do it this way.” But why was the specification/advise this way?
• “I need to use it to get X.” That’s explaining what is the purpose of the code but not why it is safe.

Examples of good justifications:

• “The flagged line uses Markup to construct a message body from parts that are static literals, and the dynamic parts are added via Markup formatting. This looks like the safe way to do it, according to the doc.“
• “The glob and os modules are only being used in this test to obtain the list of static asset files and their timestamps, to verify the asset compilation system. This should be safe as far as I can tell, and is not reachable from business code.“

In the below explanations, we give alternatives or ways to use these methods safely. Check them and don’t hesitate to ask around if these are not clear.

Python

PY001

• eval is completely unsafe in more or less all situations, never use
• safe_eval is safer, but care must be taken not to expose unsafe APIs and objects via the context, prefer passing exclusively Python builtins if possible

And in general prefer literal_eval if at all possible, and expr_eval if not (less flexible than a full-blown safe_eval).

PY002

open functions generally allow unrestricted filesystem access, and thus sandbox escape when not running in jails / containers.

Prefer file_open and static-ish paths to module contents. In case of more dynamic paths, use the ext filter to limit flexibility (e.g. limit odds of making partner module code accessible).

PY003

Static file helpers behave essentially like an unrestricted open and their entire purpose is to make content user-accessible. Any user-provided data when constructing paths needs to be carefully checked, ideally should be limited to intrinstically safe constructs (literal strings, get_resource_path, ...)

PY004

Modules which allow access to filesystem or machine resources, and could permit sandbox escape or data leaks.

Compression modules are generally safe when operating on in-memory buffers, although care should be taken when loading data (zip bombs, etc...)

PY005

Cryptography modules, easily misused, rolling your own crypto is usually inadvisable.

Prefer odoo.misc.hmac or modules like uuid or secrets which provide less error-prone APIs.

PY006

• requested URLs should be hard-coded or carefully constructed / checked to avoid being an open relay or giving the ability to access DMZ content
• requests must have a timeout to mitigate slow network or slowloris-type attacks
• consider using sessions if doing http requests in loops (sessions enable keep-alive), do not store sessions on models & al as they are not thread-safe and would allow direct unchecked use

PY007

Used to store / prepare evaluation contexts for qweb templates or eval/safe_eval, adding contents to these evaluation contexts is dangerous as that expands the amount of access to sometimes unpredictable scopes

• prefer providing only built-in python objects (~json compatible)
• never add python modules

PY008

Critical data, extreme care should be taken not to leak this information.

PY009

Bypasses some or all HTML sanitisation, allows storing arbitrary (and potentially risky) attributes, hence risks of XSS.

PY010

If used to initialise evaluation contexts, can allow injection or replacement of template or evaluation data in ways which are uncontrolled and unsafe.

PY011

If you need this, something has gone very wrong (it's only necessary for multi-megabyte documents or depths in the hundreds of levels).

PY012

Instantiating custom XML/HTML parsers should be avoided. If customising parsers is necessary, resolve_entities=False should always be passed. As noted in the introduction, this applies even if our codebase doesn't do it consistently. Cfr https://lxml.de/FAQ.html#how-do-i-use-lxml-safely-as-a-web-service-endpoint

PY013

Escaped content is safe. Unescaping content is generally a bad idea as it re-opens any downstream use to XSS.

PY014

Markup whitelists content as not needing escaping, it should only be used on a literal string (or no parameter, which is identical to an empty string), and dynamic content should be formatted in via % or .format. Documentation on Markup.

PY015

The SQL linter should be reliable, dynamic queries should be constructed by passing values to the execute functions, or using the psycopg2.sql helpers to construct the query itself.

If the linter fails on static queries, ping @flvr as that might be a bug in the linter.

PY016

Dynamic attribute resolution can allow information leak and dangerous object access to users or external parties.

How to fix it:

• for dynamic field names, recorsets can be used as mappings e.g. record.field -> record['field'] which is much safer
• prefer static function overrides with key parameters
• if dynamic resolution is necessary, either object or attribute should be dynamic, not both
• attributes should be patterned (partially static e.g. prefixed with a hard-coded item) to limit opportunities
• odoo.models.check_method_name can be used to limit access to public ORM methods

/!\ If a method declaration is flagged, this is also prone to the creation of "DotDict" objects. Those objects allow dict-like items to be made accessible as attributes, e.g. you can do foo['bar'] = x with any arbitrary x and then retrieve it with foo.bar. This is a vector for exploits and must be avoided. Cfr PY029.

PY017

Can leak data or be mis-treated as safe in various contexts (e.g. passed to safe_eval)

• components should be carefully checked
• dynamically injected data should only be the repr of built-in python types (aka json-compatible)

Note:
According to __repr__ documentation, the representation of an object must be a valid Python expression. If this expression is evaluated, we must be able to retrieve an object with the same values as the initial object. A good practice is to use quotation marks to enclose string variables in the representation. If this is not possible (or that we want to prevent an unsafe eval), it is necessary to surround the representation with <...>.

PY018

Equality of cryptographic contents (keys, tokens, ...) should be checked via constant-time equality functions e.g. secrets.compare_digest, to avoid risks of timing attacks

PY019

has_group(group_no_one) is true for all internal users, user_has_groups is the function with a special case for debug mode.

PY020

Care is necessary e.g. needs to be checked in all locations with data not just create and/or write e.g. default_ keys in contexts, depth in m2m/x2m values, etc...

PY021

Should only be used with UUID / randomised channels or partner records. Partner channels are safe (only the corresponding user can connect), but deterministically crafted channel names allow subscribing without authentication, as they are public.

PY022

• JSON routes don't need CSRF in the first place so disabling it is never necessary
• Non-JSON routes do need CSRF protection as all Odoo Cloud databases are considered the same site for CORS and allow cross-site contents
• Valid opt-outs are limited to
  • non-sensitive operations
  • operations with their own protection which should not be accessible to an attacker (e.g. shared secrets)

PY023

Should never be called from business code, it's an internal ORM feature meant to be used in a clean environment. When called from a user-controlled scope it is subject to injection issues, leading to privilege escalation.

PY024

Image plugins are restricted to just the core set because image parsers are a common source of vulnerabilities e.g. most recently as of writing this.

Plugins can be enabled on a case-by-case basis by importing the corresponding module, and write-only modules are generally safe, but because pillow doesn't really support scoping or unloading modules loading most image plugins is an unncessary risk, which rarely comes with a huge reward.

PY025

_get_manifest_cached() should never be called from business code, it's an internal cached version. Use get_manifest() instead.

PY026

Must be used with extreme caution, as it is subject to leaking types. Prefer the safer isinstance() along with hardcoded types, or contact the security team for help. If trying to access a field definition, use self._fields[<field_name>] instead of type(self).<field_name>.

If you want to log the (wrong) type of a value, use repr(value) or f"Wrong value: {value!r}" instead.

PY027

Modules which allow access to filesystem or machine resources, and could permit sandbox escape or data leaks. Care should be taken when using the API to avoid allows arbitrary/dangerous parameters and avoid returning/leaking API objects.

PY028

Odoo workers may share memory, mutable global data is an easy way to get race conditions and leak data between instances.

PY029

DotDict and other dynamic objects proxying attributes as "items" may allow bypassing attribute access restrictions. They should never be returned/leaked by model methods, only used internally.

PY030

Despite their names, urllib.parse.urljoin and werkzeug.urls.url_join are usually not what you want to use, because they implement a full browser-type URL resolution where the first parameter is the "base", that is:

>>> urljoin("https://my.partner.com/foo", "https://evil.com")
'https://evil.com'
>>> urljoin("https://my.partner.com/payment/42", "/user/")
'https://my.partner.com/user/'

So an untrusted second parameter is a big problem. And that's usually the parameter that is untrusted.

While there are rare use cases for this behaviour, somewhat ironically it's generally much safer to use regular string concatenation, possibly with some validation of the second parameter. Starting odoo 19 there is also a odoo.tools.urls.urljoin which implements less browser-like and more intuitive URL joining.

PY031

In several contexts, instance attributes are used specifically to shadow (make inaccessible) class attributes which are security-sensitive. In those cases, removing the instance attributes re-reveals the class attributes and (re)opens the security issue.

A non-exhaustive excerpt of this pattern is various methods of Registry or Request.

PY032

Because relateds are computed under sudo by default they're dangerous, especially binary relateds into attachments as they allow modifying e.g. bundles without checks.

PY033

SQL objects are designed to be easy to compose in order to safely create dynamic SQL queries. Like MarkupSafe, they should essentially always be called with a literal string as first parameter and there are vanishingly few cases where that doesn't work.

If you have string values that you have whitelisted, create a map or match/case of string literals to SQL objects.

PY034

In the context of Odoo programmatic attribute resolution is fraught as it can easily expose a "dotdict", which can be used to bypass checkers and attribute restrictions (cf PY029). As such, overriding __getattr__ or __getattribute__ should be avoided, and if used it should offer no access to any user code (including server actions).

Notably __getattr__ should not fallback to a mapping, or set attributes during lookup.

PY123

Should always use odoo.http.request.redirect:

• defaults to local redirects only (avoids open redirect issues)
• supports database-specific interception and processing

JavaScript

JS001

eval is an out-and-out XSS, obviously rather dangerous, avoid entirely (maybe use pseudo-python evaluator instead?)

JS002

jQuery APIs can take arbitary HTML with little validation, see JS003 for more.

JS003

APIs which opt into risk of XSS. Should be avoided.

Usually fine if the value is a string literal (especially empty), if non-literal, value should be constructed carefully and everything under user (thus potentially attacker) control should be html-escaped.

JS004

Markup and markup serve to whitelist strings as "raw markup" for t-out. Ultimate risks are similar to JS002, but generally easier to audit. Markup / markup whitelisting should be as close to the source as possible to make this auditing possible.

JS005

In Select2 3.5, the return value of formatSelection/formatResult is always interpreted as markup. It is necessary to use the 4th parameter ("escapeMarkup") when user-controlled content is needed.

• https://github.com/select2/select2/issues/4587#issuecomment-731138740
• https://select2.github.io/select2/#documention

JS006

In Select2 4, templateSelection / templateResult are safe when returning text, however it's possible to return DOM or jQuery objects for rich content, which is obviously unsafe, cf https://select2.org/dropdown#templating

JS007

The javascript: url scheme can be used to execute JS code. Location headers from the server are sanitised, but setting / updating the location via JS API is not, so this is a risk of XSS as well.

encodeURIComponent allows sanitising user-controlled data, although it is limiting.

Not solutions are new URL(...).toString() (which doesn't actually to anything useful there, although it can be useful to tear the URL apart in order to create a known-safe versions e.g. without a scheme / domain / ...), or encodeURI (which is like the goggles).

JS008

LeafletJS have numerous APIs which can take raw HTML, as such it is very much a risk center.

XML

XML001

Similar to JS002 for older Odoo versions. For newer Odoo versions, should use t-out with markup whitelisting instead.

XML002

Useless on a fields.Html, generally dangerous on a fields.Char or fields.Text as it acts like a t-raw, anyone able to set the field can inject arbitrary markup.

CSV

CSV001

Global ACLs apply to portal and even public users, which is usually undesirable (unless 0,0,0,0 aka deny everyone). Prefer targeting more precisely, and avoiding giving public users blanket access to models.

Specific files

FILE001

One of the files being touched contains a very sensitive piece of code, and each PR requires a security review. Some of the risks related to these files are the following:

• odoo/tools/safe_eval.py: contains the Odoo sandbox. Each change, including Python OPCODE or sandbox logic, may have security consequences. A full security review is necessary in each case.
• odoo/addons/web/static/lib/pdfjs/*: PDF.js contains multiple security risks, include an unsafe Wasm binary sandbox, embedded script execution logic, and more. Before modifying or updating our version, a full review of these risks is necessary.

Misc file types

NGX001

Using the $uri nginx context variable in order to generate a redirect or other client-targeted URI is a security problem, because this variable has been unescaped and normalized, i.e. it has possibly transformed an encoded payload into real bytes, weaponizing it.

Instead, always use the raw $request_uri, which can only contain ASCII. Control characters or CR/NL chars would break the protocol so they can't be in there. They can however appear inside $uri as a result of the decoding, and that can be abused.

You still need to pay attention to the consequence of an injection here, as the $request_uri could still be arbitrary, even if conforming to the HTTP protocol.

NGX002

In the context of injecting the Host header from a given request into a response, or when writing logic that depends on it, always use the normalized, and cleaned up $host, and never the raw $http_host, which can contain any arbitrary values that don't correspond to any known host on the server side.

This is the opposite of rule NGX001, but in this case it's safe because nginx will ensure that $host matches a real Host value that we can route, and this is the value we are looking for. It also needs to be a valid HTTP 1.1 Host value, and a valid DNS host, so it can only be a domain name ([A-Za-z0-9.-]), or an IPv4 or IPv6 literal. This safe against most kinds of injections.