Conversation
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)
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")] |
…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
1 participant
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)