Conversation
Summary of ChangesHello @jeremi, 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 significantly enhances the platform's data management and visualization capabilities by introducing comprehensive support for Humanitarian Exchange Language (HXL). It enables users to seamlessly import, aggregate, and visualize area-level humanitarian data, fostering better data interoperability and informed decision-making. The new modules streamline the process of transforming raw field data into actionable insights on geographical maps. Highlights
Changelog
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
|
![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 introduces new modules (spp_hxl, spp_hxl_area, spp_gis_indicators) to enhance HXL data integration and GIS-based indicator visualization. However, it contains critical security vulnerabilities, specifically a confirmed Cross-Site Scripting (XSS) in the HXL import wizard's data preview and a high risk of insecure code evaluation in the aggregation engine. Additionally, the review suggests modernizing SQL constraint definitions and clarifying a misleading code comment to improve maintainability.
| # WARNING: eval() is dangerous - in production use a safe expression evaluator | ||
| if self._eval_filter(row, rule.filter_expression): |
There was a problem hiding this comment.
There is a high risk of insecure code evaluation in the aggregation engine. The developer's own comments and documentation explicitly warn that eval() is used and that it poses a security risk if expressions are user-provided (e.g., in spp.hxl.aggregation.rule). Although the code currently calls spp.cel.service.evaluate_expression, the presence of these warnings suggests that the underlying implementation may be insecure or that the transition to a safe evaluator is incomplete. The comment on line 139 is misleading as _eval_filter uses spp.cel.service for evaluation, not eval(). To remediate the overall risk, ensure the expression evaluation service uses a strictly sandboxed environment and does not rely on Python's eval(). If the implementation is already safe, remove all misleading warning comments and update the documentation to reflect the security of the system.
| html.append(f"<th>{col}</th>") | ||
| html.append("</tr></thead>") | ||
|
|
||
| # Rows | ||
| html.append("<tbody>") | ||
| for row in rows: | ||
| html.append("<tr>") | ||
| for col in columns: | ||
| value = row.get(col, "") | ||
| html.append(f"<td>{value}</td>") |
There was a problem hiding this comment.
The _generate_preview_html method is vulnerable to Cross-Site Scripting (XSS). It constructs an HTML table by directly concatenating strings with untrusted data from the uploaded HXL file (both headers and cell values). This HTML is then rendered in the wizard's preview tab. An attacker can craft a malicious HXL file containing JavaScript which will execute in the context of the user's session when they preview the data.
To remediate this, ensure all untrusted data is properly escaped before being inserted into the HTML string. You can use Odoo's html_escape utility.
from odoo.tools import html_escape
# ...
html.append(f"<th>{html_escape(col)}</th>")
# ...
html.append(f"<td>{html_escape(str(value))}</td>")| is_standard = fields.Boolean(default=True, help="True if part of HXL 1.1 standard specification") | ||
| active = fields.Boolean(default=True) | ||
|
|
||
| _code_unique = models.Constraint("unique(code)", "HXL attribute code must be unique!") |
There was a problem hiding this comment.
The use of models.Constraint is a deprecated way of defining SQL constraints in Odoo. It's recommended to use the _sql_constraints attribute instead for better maintainability and adherence to modern Odoo development practices.
| _code_unique = models.Constraint("unique(code)", "HXL attribute code must be unique!") | |
| _sql_constraints = [ | |
| ("code_unique", "unique(code)", "HXL attribute code must be unique!"), | |
| ] |
|
|
||
| active = fields.Boolean(default=True) | ||
|
|
||
| _code_unique = models.Constraint("unique(code)", "Export profile code must be unique!") |
There was a problem hiding this comment.
The use of models.Constraint is a deprecated way of defining SQL constraints in Odoo. It's recommended to use the _sql_constraints attribute instead for better maintainability and adherence to modern Odoo development practices.
| _code_unique = models.Constraint("unique(code)", "Export profile code must be unique!") | |
| _sql_constraints = [ | |
| ("code_unique", "unique(code)", "Export profile code must be unique!"), | |
| ] |
| ) | ||
| active = fields.Boolean(default=True) | ||
|
|
||
| _hashtag_unique = models.Constraint("unique(hashtag)", "HXL hashtag must be unique!") |
There was a problem hiding this comment.
The use of models.Constraint is a deprecated way of defining SQL constraints in Odoo. It's recommended to use the _sql_constraints attribute instead for better maintainability and adherence to modern Odoo development practices.
| _hashtag_unique = models.Constraint("unique(hashtag)", "HXL hashtag must be unique!") | |
| _sql_constraints = [ | |
| ("hashtag_unique", "unique(hashtag)", "HXL hashtag must be unique!"), | |
| ] |
| _unique_indicator = models.Constraint( | ||
| "UNIQUE(area_id, variable_id, period_key, incident_id, batch_id)", | ||
| "Indicator must be unique per area/variable/period/incident/batch", | ||
| ) |
There was a problem hiding this comment.
The use of models.Constraint is a deprecated way of defining SQL constraints in Odoo. It's recommended to use the _sql_constraints attribute instead for better maintainability and adherence to modern Odoo development practices.
_sql_constraints = [
(
"unique_indicator",
"UNIQUE(area_id, variable_id, period_key, incident_id, batch_id)",
"Indicator must be unique per area/variable/period/incident/batch",
),
]| help="Number of imports using this profile", | ||
| ) | ||
|
|
||
| _code_unique = models.Constraint("UNIQUE(code)", "Profile code must be unique") |
There was a problem hiding this comment.
The use of models.Constraint is a deprecated way of defining SQL constraints in Odoo. It's recommended to use the _sql_constraints attribute instead for better maintainability and adherence to modern Odoo development practices.
| _code_unique = models.Constraint("UNIQUE(code)", "Profile code must be unique") | |
| _sql_constraints = [("code_unique", "UNIQUE(code)", "Profile code must be unique")] |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #47 +/- ##
===========================================
- Coverage 71.31% 57.99% -13.33%
===========================================
Files 299 155 -144
Lines 23618 11494 -12124
===========================================
- Hits 16844 6666 -10178
+ Misses 6774 4828 -1946
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| "Invalid hex color format: %s. " "Expected #RGB or #RRGGBB", | ||
| color, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Hex color validation doesn't check digit characters
Low Severity
The _check_colors_json constraint validates that color strings start with # and have length 4 or 7, but never verifies that the remaining characters are valid hexadecimal digits (0-9, a-f, A-F). Values like #zzzzzz or #hello! pass validation but won't render as visible colors in the choropleth legend or map, silently producing broken visualizations.
New modules: - spp_hxl: HXL hashtag/attribute management and export profiles, integrates with CEL variables and Studio - spp_hxl_area: area-level HXL indicator import, aggregation rules, batch import with area matching - spp_gis_indicators: choropleth visualization for area-level indicators, configurable color scales and classification methods (quantile, equal interval, manual breaks)
…UI fixes - Promote both modules to Beta status - Fix mode="tree" to mode="list" (Odoo 19 compat) - Add missing list view for aggregation rules - Make import batch fields readonly when completed/failed - Remove redundant header buttons - Fix PII in log messages - Improve form layouts
…val comments - Use html_escape() on all untrusted HXL data (headers, cell values, error messages) inserted into the preview HTML table to prevent XSS attacks - Replace the misleading eval() warning comment in aggregation_engine with accurate documentation reflecting the CEL sandboxed evaluator in use
…int trigger Move manual classification check before empty-data early return so manual_breaks can be computed without any imported indicator data. Add classification_method to @api.constrains so the validator fires when the method is changed to "manual" without touching manual_breaks.
jeremi
force-pushed
the
feat/gis-indicators-hxl
branch
from
84157c9 to
5b18879
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| colors = self.color_scale_id.get_colors() | ||
|
|
||
| if not colors: | ||
| return {} |
There was a problem hiding this comment.
Missing empty-breaks guard in get_feature_colors
Medium Severity
When classification produces empty breaks (e.g., all identical indicator values), break_values is set to "[]". _compute_legend_html correctly guards against this with if not breaks on the parsed list, returning empty HTML. But get_feature_colors only checks if not self.break_values on the raw string — "[]" is truthy, so it proceeds to classify all areas into a single class and assign the first color. This causes colored areas with no corresponding legend, creating an inconsistent and confusing choropleth display.
Additional Locations (1)
emjay0921
left a comment
emjay0921
left a comment
There was a problem hiding this comment.
spp_hxl: 54 tests pass. spp_hxl_area and spp_gis_indicators could not be tested (libhxl not in Docker). Code review clean. Minor: name_get() in hxl_area_indicator.py should be _compute_display_name for Odoo 19. GPS area matching is a stub.
2 participants
Summary
Dependencies
spp_hxlandspp_hxl_areausespp_cel_domainspp_gis_indicatorsusesspp_gisspp_hxl_areaalso depends onspp_hazard(already in OpenSPP2)Note
spp_hxlandspp_hxl_areaexisted in the private repo but were not included in the original OpenSPP2 export.spp_gis_indicatorsis new, developed on theclaude/global-alliance-policy-basketbranch.Test plan
libhxlpython dependency)Note
Medium Risk
Adds new addons with new models, security rules, and UI entry points; main risk is data/permission misconfiguration or performance issues from indicator searches and classification on large datasets.
Overview
Introduces two new Odoo addons.
spp_hxladds HXL interoperability primitives: registries for HXL hashtags/attributes, export profiles with HXL-tagged columns, and extendsspp.cel.variablewith HXL mapping plus import/export behavior flags, with seeded standard tags/attributes and access controls.spp_gis_indicatorsadds choropleth visualization for area indicators: aspp.gis.color.scalemodel with presets, anspp.gis.indicator.layermodel that classifies indicator values (quantile/equal-interval/manual) and generates legends/color mappings, and extendsspp.gis.data.layerwith a newgeo_repr='choropleth'option wired into the UI and security rules; both modules include comprehensive transaction tests and autogenerated README/description assets. Also updatesrequirements.txtto includelibhxl.Written by Cursor Bugbot for commit 5b18879. This will update automatically on new commits. Configure here.