[ADD] skills: odoo-guidelines, odoo-review, odoo-security - #5250
Open
fvdsn wants to merge 7 commits into
Open
Conversation
Import the review and security agent skills maintained in the local odoo-cli workspace, as a first step towards maintaining them in the odoo repository itself: - odoo-review: reviewing addon code against the coding guidelines - odoo-security: security review of addon code Imported as-is; review and polish will follow.
Restructure the imported odoo-review skill following the layout introduced for odoo-js-guidelines (odoo#285015): - skills/odoo-guidelines: the house rules themselves, one numbered file per domain (module structure, manifest, Python/ORM, fields, controllers, XML, QWeb reports, access rights, SCSS, performance, tests), dispatched by a routing table keyed on the files being touched. Serves both writing and reviewing. - skills/odoo-review: reduced to the review process (per-file dispatch to the matching guidelines, stable-vs-master policy, version traps, reporting format). The JavaScript section is dropped in favor of odoo-js-guidelines, the security essentials in favor of odoo-security, and the references to odoo-cli tooling (odoo update/test, odoo-changelog) are removed as that tooling is not published yet.
Odoo guideline files stay one-line bullets by default: the reader already knows the framework, and dense rules are applied more reliably than tutorials. An example earns its place only when it corrects a default the reader gets wrong — a shape prose undersells, or a trap where the intuitive code is the mistake. Apply that policy to the three known traps: - 0005 translations: bad/good interpolation examples - 0014 (new, from 0008): view inheritance — stable anchors vs positional xpath, with a live exemplar path - 0015 (new, from 0012/0004): batch ORM calls vs calls in a loop, with a master-API _read_group example and encode the promotion rule in AUTHORING.md.
Re-derive the guideline content from today's master documentation (the skill was distilled from an older snapshot). Stale rules fixed: - tests: standard,post_install is now the default tagging; at_install is the discouraged opt-in (needs a justification comment, blocks runbot parallelization; HttpCase must stay post_install) - translations: self.env._() is the current API; bare _ relies on frame inspection and fails in plain functions/comprehensions - controllers: auth also accepts 'bearer' (with bearer_scope); type='json' is a deprecated 19.0 alias of 'jsonrpc' - orm: SQL constraints/indexes are model attributes (models.Constraint/Index/UniqueIndex), _sql_constraints is legacy - view inheritance: shorthand matching is asymmetric (field by name; other tags by tag + all attributes) New doc-backed traps and conventions: @api.private, fields.Domain composition, index types and models.Index, record-rule perm flags defaulting on and global/group ruleset intersection, template/asset active-on-create-only, t-lang effective only on t-call, silent test-file skips, auto_install subset lists, report/populate file naming, SCSS mixin argument naming, file permissions.
Verify every guideline claim against the actual master implementation (the docs themselves lag the code in places). Major corrections: - security: ir.model.access + ir.rule no longer exist; rewrite 0010 around the unified ir.access model (security/ir.access.csv, operation letter-sets, OR-ed group permissions vs AND-ed group-less restrictions, default deny, base.group_everyone, company restriction rows) - tests: document the loader's silent skips (both/neither install tag, class defined outside the test file, inherited test methods needing allow_inherited_tests_method), assertQueryCount warmup/asymmetry, BaseCommon bases, mute_logger, expectUnloadPage - controllers: undecorated overrides are auto-decorated (warning), an override can never change type; json2 dispatcher exists - orm/fields: _sql_constraints is ignored (never created), constraint/ index attributes must start with _, related x2many chains silently truncate to the first record, shared inverses see False siblings - qweb: t-lang outside t-call raises SyntaxError (docs say ignored) - view inheritance: locator attributes are a subset match, fields match at any depth; exemplar caveat on pre-convention record ids - conventions vs reality: 4 ruff isort import groups, security/ <module>_security.xml, feature-named controllers, populate file names, .wizard naming softened to a preference
Route the audit by grep pattern: a process section up top sweeps the addon for 16 patterns and dispatches each hit to its pitfall section, replacing the audit-checklist and how-to-audit tails that restated the same content. Explicit completion criterion (every pattern swept, every hit judged) and an explicit non-goal (a hit is not a finding by itself). The ACL and record-rule sections collapse into one access-control section written in ir.access vocabulary (the old models are gone from master), pointing at odoo-guidelines 0010/0006 for mechanics and keeping only the attacker's reading. Escaping-vs-sanitizing folds into the XSS section; route auth exposure joins the CSRF section. Pitfall content otherwise carried over verbatim; correctness is verified in a follow-up pass.
Verify every pitfall against the master implementation. Corrections: - XSS: t-raw no longer exists (server QWeb or OWL) — flag occurrences as dead code; the raw-HTML vector is Markup/markup() reaching t-out. Rewrite the jQuery-era JS bullet around today's sinks (innerHTML, insertAdjacentHTML, plain markup()) and helpers (htmlEscape, tagged markup templates, @web/core/utils/html) - CSRF: token validation only exists on type='http' routes; jsonrpc/ json2 have none by design — scope the rule to avoid false positives - related fields: the read-leak enabler is default sudo-compute (related_sudo=True), not readonly=False (write-through is access-checked) - restrictions: an uncovered operation falls back to the permission rows (default deny), not to unrestricted - sudo: x2many Command payloads execute with sudo on the comodel unless _allow_sudo_commands = False — whitelist command lists too - complex objects: the live vector is safe_eval'd server actions walking single-underscore internals; RPC returns die in marshalling and dunders are blocked in safe_eval - pickle: odoo.tools.misc.pickle no longer exists — drop the fallback - RPC surface: get_public_method also blocks @api.private (MRO-wide), classmethods/staticmethods - file_open: also allows the Odoo root and registered temp dirs, and opens existing files in write mode — path confinement, not write protection - sweep table: patterns grounded in what master actually contains (bare @route(, innerHTML/markup(, related sudo-compute)
|
This PR targets the un-managed branch odoo-dev/odoo:master-js-guidelines-ged, it needs to be retargeted before it can be merged. |
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.
Stacked on odoo#285015 (targets its branch so only these commits show; rebase to master once it merges).
Adds three agent skills to
skills/, following the structure introduced there:AUTHORING.md.odoo-js-guidelines), stable-vs-master policy, version-trap warning, explicit completion criterion.ir.access), sudo, SQL/domain injection, XSS, CSRF, eval, file access, and friends.All content was verified in two passes against today's master: first against the documentation, then against the source itself. The source pass overruled the docs in several places (
ir.accessunification,t-rawand the restricted pickle no longer existing, CSRF being http-type-only, test-tag defaults) — a list of the documentation bugs found along the way is being forwarded to the docs team separately.