-
Notifications
You must be signed in to change notification settings - Fork 283
Encode URL in add_link
#1906
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Encode URL in add_link
#1906
Conversation
WalkthroughAdds a reusable encode_url utility and applies it to link insertion and GitHub markdown transformation so URLs are consistently encoded at source and during rendering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Reporter as Finding.add_link
participant Transformer as Transformer.to_github_markdown
participant Util as encode_url
Reporter->>Util: encode_url(link.url)
Util-->>Reporter: encoded_url
Reporter->>Reporter: append link with encoded_url
Transformer->>Util: encode_url(parsed_url)
Util-->>Transformer: encoded_url
Transformer->>Transformer: replace and render markdown
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/robusta/utils/common.py (2)
51-53: Normalize whitespace to prevent accidental empty/invalid URLs.Defensive trimming avoids surprises from
" https://..."and makes the empty-check more robust.- if not url: - return "" + # Normalize leading/trailing whitespace + url = url.strip() + if not url: + return ""
48-50: Tighten the docstring to clarify behavior and idempotency.Spell out which components are encoded and that existing
%xxsequences are preserved.- """ - Encode a URL so that it can be safely used in contexts where special characters must be escaped. - """ + """ + Encode a URL so it’s safe for sinks/renderers that require escaping. + - Encodes path, query, and fragment components. + - Preserves existing percent-escapes (does not double-encode). + - Returns "" for blank input. + """src/robusta/core/reporting/base.py (1)
362-367: Skip appending links with empty/invalid URLs after encoding.If
link.urlis blank or whitespace,encode_urlreturns"", and we’ll still append a broken link. Guard and log instead.def add_link(self, link: Link, suppress_warning: bool = False) -> None: if self.dirty and not suppress_warning: logging.warning("Updating a finding after it was added to the event is not allowed!") - link.url = encode_url(link.url) - self.links.append(link) + link.url = encode_url(link.url) + if not link.url: + logging.warning("Skipping link with empty URL (name=%s)", getattr(link, "name", "")) + return + self.links.append(link)Note: since we mutate
link.url, reusing the sameLinkinstance elsewhere will see the encoded value. If that’s undesirable, consider copying.I can scan the repo for
add_link(callsites to check if any rely on mutatinglink.urlafterward.src/robusta/core/sinks/transformer.py (1)
126-128: Right fix; consider parity in HTML path.Using
encode_urlhere addresses Slack-style inputs like{in URLs. For consistency, consider applyingencode_urlin the HTML conversion path as well (__markdown_to_html, Lines 133–139) so both Markdown and HTML renderers handle unsafe characters identically.- encoded_url = encode_url(splits[0]) - replacement = f"[{splits[1]}]({OPENING_ANGULAR}{encoded_url}{CLOSING_ANGULAR})" + encoded_url = encode_url(splits[0]) + replacement = f"[{splits[1]}]({OPENING_ANGULAR}{encoded_url}{CLOSING_ANGULAR})"Follow-up: mirror this in
__markdown_to_htmlby wrappinglink_parts[0]withencode_url(...).I can open a small follow-up PR to apply the same helper in
__markdown_to_htmland add tests for both codepaths.
📜 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 (3)
src/robusta/core/reporting/base.py(2 hunks)src/robusta/core/sinks/transformer.py(2 hunks)src/robusta/utils/common.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/robusta/core/reporting/base.py (1)
src/robusta/utils/common.py (1)
encode_url(47-64)
src/robusta/core/sinks/transformer.py (1)
src/robusta/utils/common.py (1)
encode_url(47-64)
🔇 Additional comments (3)
src/robusta/utils/common.py (1)
2-2: Import looks good.
urllib.parseis the right module for URL component encoding.src/robusta/core/reporting/base.py (1)
19-19: Good centralization.Importing the shared
encode_urlhelper keeps encoding logic consistent across sinks.src/robusta/core/sinks/transformer.py (1)
9-10: LGTM.Using the shared
encode_urleliminates ad-hoc encoding.
This PR includes URL encoding/quoting before adding a link to a finding.
The original motivation for this PR is a problem I've encountered with Slack links (which I'll describe promptly), but I guess non-encoded URLs may cause an issue with other sinks as well, so a general solution is preferred.
The mentioned Slack problem is as follows:
Robusta produces a "View Graph" button with a URL including unescaped braces, but these characters don't behave nicely with Slack.
Specifically, on the Slack android app, clicking the "View Graph" button will do nothing. (But it works just fine from a browser.)
Here are Slack blocks to demonstrate the problem
{ "blocks": [ { "type": "actions", "elements": [ { "type": "button", "text": { "type": "plain_text", "text": "Won't work from Android app" }, "url": "https://google.com?q={", "action_id": "actionId-0" } ] }, { "type": "actions", "elements": [ { "type": "button", "text": { "type": "plain_text", "text": "Works from Android app" }, "url": "https://google.com?q=", "action_id": "actionId-1" } ] } ] }As mentioned above, the specific Slack problem is not the main point (you could even say it's an issue in Slack), but it would be good practice to encode the URL before sending it to a sink.