[WEB-4806] chore: improved the html validation #7676
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a formal HTML sanitization policy (CUSTOM_TAGS, ALLOWED_TAGS, ATTRIBUTES, SAFE_PROTOCOLS), a diff helper to detect removed tags/attributes, and updates validate_html_content to sanitize via nh3.clean, log removals, and return (is_valid, error_msg, clean_html). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Validator as validate_html_content
participant NH3 as nh3.clean
participant Diff as _compute_html_sanitization_diff
participant Logger as log_exception
Caller->>Validator: validate_html_content(html_content)
Validator->>NH3: clean(html_content, tags=ALLOWED_TAGS, attributes=ATTRIBUTES, url_schemes=SAFE_PROTOCOLS)
NH3-->>Validator: clean_html
alt removals detected
Validator->>Diff: _compute_html_sanitization_diff(html_content, clean_html)
Diff-->>Validator: {removed_tags, removed_attributes}
Validator->>Logger: log_exception(JSON_summary, warning=True)
end
Validator-->>Caller: (True/False, error_msg|None, clean_html|None)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Pull Request Linked with Plane Work Items
Comment Automatically Generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/plane/utils/content_validator.py (1)
236-272: Add tests for sanitizer policy (tags, attributes, schemes)High-risk area; add unit tests covering: img/src retained, a[target+rel] retained, link inert, custom tags preserved, disallowed tags stripped, data: in anchors rejected, size limit path, and diff logging path.
I can scaffold pytest tests for validate_html_content and the diff helper if helpful.
🧹 Nitpick comments (3)
apps/api/plane/utils/content_validator.py (3)
241-243: Return empty string for clean_html on empty inputStabilizes return types for callers (str vs None).
Apply:
- if not html_content: - return True, None, None + if not html_content: + return True, None, ""
256-268: Sanitization logging may be noisyEvery removal logs a warning. Consider sampling or demoting to info to avoid log flood on rich documents.
Example:
- if diff.get("removed_tags") or diff.get("removed_attributes"): + # Log only when notable removals occur + total_removed = sum(diff.get("removed_tags", {}).values()) + sum( + len(v) for v in diff.get("removed_attributes", {}).values() + ) + if total_removed >= 5: try: import json
96-98: Comment vs behavior mismatch (“dynamically include all data-*”)The code doesn’t dynamically include data-* seen in input. Either implement the behavior or adjust the comment to avoid misleading future changes.
I can draft a lightweight pre-scan (bs4) to collect data-* names and union them into MERGED_ATTRIBUTES["*"] per-call if you want.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/api/plane/db/models/page.py(1 hunks)apps/api/plane/utils/content_validator.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/utils/content_validator.py (1)
apps/api/plane/utils/exception_logger.py (1)
log_exception(9-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
apps/api/plane/db/models/page.py (1)
61-61: Whitespace-only change — safe to mergeNo functional impact, no migration needed. Verify your formatter doesn’t fight this in future diffs.
apps/api/plane/utils/content_validator.py (2)
117-121: Attribute case consistency: aspectRatio vs aspectratioHTML attribute names are case-insensitive and usually lowercased by parsers. Using aspectRatio for some tags and aspectratio for others will drop attributes unexpectedly. Normalize to lowercase (or the exact case your editor serializes).
Would you confirm the actual attribute casing emitted by the editor for image/imageComponent? If it’s lowercase, update those entries accordingly.
Also applies to: 150-150
20-62: Inline data URIs will be flagged byvalidate_binary_data
The new check rejects anydata:<type>/<subtype>;base64patterns, but we have inline data URIs used for default favicons inapps/api/plane/utils/content_validator.py(lines 146 and 167). Please confirm whether data-URI-based icons should be permitted—if so, we’ll need to whitelistdata:URIs or adjust the validator to align with product expectations.
There was a problem hiding this comment.
Pull Request Overview
This PR improves HTML validation by expanding the sanitization capabilities to support custom editor tags and attributes. The changes enhance content validation while providing better observability of sanitization operations.
- Added custom tag support for editor-specific components (images, mentions, tables, code blocks, etc.)
- Expanded allowed attributes to include data-* attributes and component-specific properties
- Introduced diff tracking to log when content is sanitized or removed
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
apps/api/plane/utils/content_validator.py (2)
214-233: Use merged attributes in nh3.clean and gate expensive diff/loggingThis repeats earlier feedback; current call still passes ATTRIBUTES and will strip required attrs. Also gate the diff/logging to when content actually changes.
Apply:
- clean_html = nh3.clean( + clean_html = nh3.clean( html_content, tags=ALLOWED_TAGS, - attributes=ATTRIBUTES, + attributes=MERGED_ATTRIBUTES, url_schemes=ALLOWED_URL_SCHEMES, ) - # Report removals to logger (Sentry) if anything was stripped - diff = _compute_html_sanitization_diff(html_content, clean_html) - if diff.get("removed_tags") or diff.get("removed_attributes"): + # Report removals to logger (Sentry) only if anything was stripped + if clean_html != html_content: + diff = _compute_html_sanitization_diff(html_content, clean_html) + if diff.get("removed_tags") or diff.get("removed_attributes"): try: import json
147-147: Remove global allowance for data: URLsAllowing data: globally enables unsafe anchors (e.g., data:text/html). Remove it now; if inline images are a requirement, reintroduce via a per-attribute allowlist (only data:image/*;base64 for img/src).
Apply:
-ALLOWED_URL_SCHEMES = {"http", "https", "mailto", "tel", "data"} +ALLOWED_URL_SCHEMES = {"http", "https", "mailto", "tel"}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/api/plane/utils/content_validator.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/utils/content_validator.py (1)
apps/api/plane/utils/exception_logger.py (1)
log_exception(9-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Lint API
🔇 Additional comments (1)
apps/api/plane/utils/content_validator.py (1)
22-23: Docstring tweak looks goodNo issues with clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
apps/api/plane/utils/content_validator.py (4)
118-118: Anchor hardening: preserve rel and enforce safe targetsYou added target but not rel; add rel (done above) and consider restricting target to [_self, _blank, _parent, _top] via a small post-pass if needed.
in body can trigger network fetches and abuse. Remove it from CUSTOM_TAGS.
67-87: Do not allow native elements in user contentCUSTOM_TAGS = { @@ - "link", @@ }
88-148: Passing ATTRIBUTES as-is overwrites nh3 defaults (breaks core attrs like img[src])Deep-merge your custom map with nh3.ALLOWED_ATTRIBUTES; also preserve a.rel and drop the separate link entry.
ATTRIBUTES = { @@ - "a": {"href", "target"}, + "a": {"href", "target", "rel"}, @@ - "link": {"href", "target"}, @@ # image-component (from editor extension and seeds) "image-component": {"src", "id", "width", "height", "aspectratio", "alignment"}, } + +# Merge nh3 defaults with our custom attributes +MERGED_ATTRIBUTES = {k: set(v) for k, v in nh3.ALLOWED_ATTRIBUTES.items()} +for _tag, _attrs in ATTRIBUTES.items(): + MERGED_ATTRIBUTES.setdefault(_tag, set()).update(_attrs)Notes:
- Merging keeps native
attrs (src, alt, srcset, etc.) without having to redefine them.
- If you must support both aspectRatio and aspectratio, keep both intentionally. Otherwise, normalize one casing.
215-233: Use merged attributes, add CSS sanitizer, and avoid unnecessary diffingPassing ATTRIBUTES drops defaults; also only compute the diff when content changed.
- clean_html = nh3.clean( + clean_html = nh3.clean( html_content, tags=ALLOWED_TAGS, - attributes=ATTRIBUTES, - url_schemes=SAFE_PROTOCOLS, + attributes=MERGED_ATTRIBUTES, + url_schemes=SAFE_PROTOCOLS, + css_sanitizer=CSS_SANITIZER if 'CSS_SANITIZER' in globals() else None, ) - # Report removals to logger (Sentry) if anything was stripped - diff = _compute_html_sanitization_diff(html_content, clean_html) - if diff.get("removed_tags") or diff.get("removed_attributes"): + # Report removals to logger (Sentry) only if content changed + if clean_html != html_content: + diff = _compute_html_sanitization_diff(html_content, clean_html) + if diff.get("removed_tags") or diff.get("removed_attributes"): try: import json @@ log_exception( f"HTML sanitization removals: {summary}", warning=True, )
🧹 Nitpick comments (2)
apps/api/plane/utils/content_validator.py (2)
88-101: Comment promises “dynamic data- allow” but it isn’t implemented*Either implement dynamic inclusion of data-* keys or drop the claim. Right now only the explicit list is allowed.
- # (wildcards like data-* are NOT supported by nh3; we add known keys - # here and dynamically include all data-* seen in the input below) + # (wildcards like data-* are NOT supported by nh3; we add known keys here)
153-200: Make diff helper cheap and resilient
- Skip BeautifulSoup work when there are no sanitizer changes.
- Short-circuit if bs4 isn’t available.
- Narrow exceptions only around parsing.
def _compute_html_sanitization_diff(before_html: str, after_html: str): @@ - try: + if before_html == after_html: + return {"removed_tags": {}, "removed_attributes": {}} + if BeautifulSoup is None: + return {"removed_tags": {}, "removed_attributes": {}} + try: @@ - soup_before = BeautifulSoup(before_html or "", "html.parser") - soup_after = BeautifulSoup(after_html or "", "html.parser") + soup_before = BeautifulSoup(before_html or "", "html.parser") + soup_after = BeautifulSoup(after_html or "", "html.parser") @@ - except Exception: + except Exception: # Best-effort only; if diffing fails we don't block the request return {"removed_tags": {}, "removed_attributes": {}}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/api/plane/utils/content_validator.py(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Build and lint web apps
🔇 Additional comments (5)
apps/api/plane/utils/content_validator.py (5)
150-150: LGTM: Safe URL protocolsGood hardening by excluding data:. Keep it this way unless you add a per-attribute data: allowlist (e.g., only data:image/... for img).
120-122: Attribute casing: confirm aspectRatio vs aspectratioYou allow camelCase on image/imageComponent and lowercase on image-component. Confirm both are required; otherwise normalize to one to reduce surprises.
Also applies to: 146-148
5-7: bs4 dependency verified
beautifulsoup4==4.12.3 is already pinned in apps/api/requirements/base.txt, so allfrom bs4 import BeautifulSoupusages are supported.
207-209: Return empty string for empty input to keep return-type stable
Callers expectsanitized_htmlto be astrwhenis_validisTrue; returningNoneis inconsistent.- if not html_content: - return True, None, None + if not html_content: + return True, None, ""
90-98: Sanitize or disablestyleattributes
nh3.clean doesn’t accept a Bleach-styleCSSSanitizer. Instead, configure yournh3.Cleanerwithfilter_style_properties(or anattribute_filter) to whitelist/transform allowed CSS, or temporarily disable"style"in yourALLOWED_ATTRIBUTES.Example refactors:
# apps/api/plane/utils/content_validator.py -from nh3 import clean +from nh3 import Cleaner # replace your existing clean call... -cleaned = clean(html, **options) +cleaner = Cleaner(**options, filter_style_properties={"color", "background", "margin", /* etc */}) +cleaned = cleaner.clean(html)# apps/api/plane/utils/content_validator.py:90-98 - "style", + # "style", # re-enable after adding filter_style_properties/attribute_filterLikely an incorrect or invalid review comment.
* chore: improved the html validation * chore: removed the models changes * chore: removed extra filters * chore: changed the protocols
Description
improved the HTML validation by adding custom tags and attributes with proper validation.
Type of Change
Summary by CodeRabbit
New Features
Bug Fixes