[SECUR-113] fix: ssrf for work item links

[sangeethailango]

Description

Fix SSR for work item links

Summary by CodeRabbit

• Bug Fixes
  • Strengthened URL handling to resist SSRF: stricter scheme/hostname checks, DNS resolution checks against private/loopback/reserved ranges, and clearer errors for invalid or unresolvable hosts.
  • Enforced strict redirect limits with warnings when exceeded.
  • Validated favicon URLs before any fetch to avoid unsafe requests and improve reliability.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds SSRF protections and strict redirect handling to URL fetching: enforces http/https schemes, resolves hostnames via getaddrinfo to validate all IPs against private/loopback/reserved/link-local ranges, introduces MAX_REDIRECTS with manual redirect loop, and applies validation when discovering and fetching favicons.

Changes

Cohort / File(s)	Summary
SSRF & Redirect Handling apps/api/plane/bgtasks/work_item_link_task.py	Enforce http/https and require hostname; add socket.getaddrinfo-based hostname resolution and IP-range checks (private/loopback/reserved/link-local); add MAX_REDIRECTS and manual redirect-follow loop validating each redirect target; validate favicon URLs at discovery, fallback, and fetch points; extend error handling and add socket import.

Sequence Diagram(s)

sequenceDiagram
    participant Worker as Worker/Task
    participant Validator as URL Validator
    participant DNS as DNS Resolver
    participant HTTP as HTTP Client
    participant Remote as Remote Host
    Worker->>Validator: validate initial URL (scheme+hostname)
    Validator->>DNS: resolve hostname (getaddrinfo)
    DNS-->>Validator: list of IPs
    Validator->>Validator: check IPs vs private/loopback/reserved/link-local
    alt allowed
        Worker->>HTTP: HTTP request
        HTTP->>Remote: connect/fetch
        Remote-->>HTTP: response (200 or redirect)
        HTTP-->>Worker: response
        opt redirect
            loop while redirects < MAX_REDIRECTS
                Worker->>Validator: validate redirect URL
                Validator->>DNS: resolve redirect hostname
                DNS-->>Validator: IPs
                Validator->>Worker: allowed?
                Worker->>HTTP: follow redirect
            end
        end
    else blocked
        Validator-->>Worker: raise error (disallowed/unalottable)
    end
    Worker->>Validator: validate discovered favicon URL(s)
    Worker->>HTTP: fetch favicon (if validated)
    HTTP-->>Worker: favicon data

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I sniff each hostname, hop by hop,
I check the IPs before I stop,
Redirects counted, favicons checked twice,
No sneaky private ranges shall slice.
Hooray — safe links, a carrot-sized slice! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description is minimal and vague, lacking required template sections such as Type of Change, Test Scenarios, and References that should accompany a security fix.	Complete the PR description by adding Type of Change (Bug fix), Test Scenarios documenting validation testing, and References linking to SECUR-113 or related issues.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main security fix for SSRF vulnerabilities in work item links, directly matching the primary change in the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-work-item-ssr

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[makeplane]

Linked to Plane Work Item(s)

• [SECUR-113] SSRF on work item links

This comment was auto-generated by Plane

[coderabbitai]

Actionable comments posted: 1

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/bgtasks/work_item_link_task.py (1)

27-63: ⚠️ Potential issue | 🟠 Major

Address DNS rebinding and multicast SSRF vectors.

The current validation has two security gaps: (1) socket.getaddrinfo() resolves once here, but requests.get()/requests.head() at lines 89 and later will resolve the hostname again independently—an attacker can rebind DNS between these calls to pivot to internal IPs; (2) multicast IPs (e.g., 224.0.0.1, ff02::1) are not blocked despite being unreachable from the public internet.

Consider either pinning resolved addresses to the connection (via a custom adapter/resolver in requests) or validating the peer IP after connection. For the multicast gap, using not ip.is_global alone is insufficient since multicast addresses report is_global=True and would still be allowed—you need not ip.is_global or ip.is_multicast.

🤖 Fix all issues with AI agents

In `@apps/api/plane/bgtasks/work_item_link_task.py`:
- Around line 192-195: The SSRF check only validates the initial favicon_url but
requests.get(favicon_url) follows redirects, allowing a 302 to an internal IP to
bypass validation; update the fetch in work_item_link_task (where
validate_url_ip and requests.get are used) to either set allow_redirects=False
on requests.get or, if following redirects, re-validate the response.url with
validate_url_ip (same approach used in crawl_work_item_link_title_and_favicon)
before reading the body so redirected-to hosts cannot be internal/private.

🧹 Nitpick comments (1)

apps/api/plane/bgtasks/work_item_link_task.py (1)

146-161: Skip invalid favicon tags instead of aborting fallback.

If a page exposes a data:/blob: icon (or any non‑HTTP(S) icon), validate_url_ip raises and the function skips checking other tags and /favicon.ico. Catching ValueError lets you continue scanning and still fall back safely.

♻️ Safer fallback behavior

         for selector in favicon_selectors:
             favicon_tag = soup.select_one(selector)
             if favicon_tag and favicon_tag.get("href"):
                 favicon_href = urljoin(base_url, favicon_tag["href"])
-                validate_url_ip(favicon_href)
-                return favicon_href
+                try:
+                    validate_url_ip(favicon_href)
+                except ValueError:
+                    continue
+                return favicon_href
@@
-    except requests.RequestException as e:
+    except (ValueError, requests.RequestException) as e:
         log_exception(e, warning=True)
         return None