Skip to content

fix(registry): validate content URL schemes#2105

Open
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-unvalidated-content-urls-vulnerability
Open

fix(registry): validate content URL schemes#2105
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-unvalidated-content-urls-vulnerability

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • Prevent untrusted community JSON metadata from reaching browser navigation sinks (e.g. href / window.open) with unsafe schemes like javascript: or data:.
  • Allow legitimate first-party download paths under /downloads/ while rejecting unsafe download schemes.
  • Add regression coverage to catch unsafe URL metadata in content submissions.

Description

  • Add a set of URL-bearing metadata fields and enforce HTTPS for them in validateEntry (packages/registry/src/content-schema.js) by introducing HTTPS_URL_FIELDS and checking isHttpsUrl for each field.
  • Add list validation for URL arrays (HTTPS_URL_LIST_FIELDS) using invalidHttpsUrlListItems to ensure each item is an HTTPS URL.
  • Introduce isSafeDownloadUrl to accept safe /downloads/ paths (with simple sanity checks) while requiring HTTPS for remote download URLs, and validate repoUrl as HTTPS.
  • Add regression tests that exercise unsafe documentationUrl, sourceUrl, sourceUrls, and downloadUrl cases and the allowed /downloads/ path in tests/content-validation.test.ts.

Testing

  • Ran pnpm exec vitest run tests/content-validation.test.ts and the content validation test suite passed.
  • Ran the repository content validator with pnpm validate:content:strict and it completed successfully.
  • Performed a full site build with pnpm build which completed without errors.
  • Ran git diff --check to ensure no whitespace/check failures and it reported clean results.

Codex Task

@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@JSONbored, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 14 minutes and 14 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 18a21964-e3b7-46d8-8427-801c7e170549

📥 Commits

Reviewing files that changed from the base of the PR and between 8529dff and a87bed8.

📒 Files selected for processing (2)
  • packages/registry/src/content-schema.js
  • tests/content-validation.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-unvalidated-content-urls-vulnerability

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

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

@gittensory

gittensory Bot commented Jun 11, 2026

Copy link
Copy Markdown

Important

Gittensory found maintainer review notes

Public GitHub metadata was checked for review readiness. Gittensor-specific context appears only when confirmed.

Readiness score: 93/100

Signal Result Evidence Action
Linked issue ⚠️ Missing No linked issue or no-issue rationale found. Explain no-issue PR.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found. No action.
Review load ✅ 20/20 Readiness component derived from cached public PR metadata and labels; size label size:M. No action.
Validation evidence ✅ 25/25 PR body includes validation/test evidence. No action.
Open PR queue ✅ 10/10 0 open PR(s), 0 likely reviewable. No action.
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 62 PR(s), 347 issue(s). No action.
Gate result ✅ Passing No configured blocker found. No action.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Review load = cached public PR metadata such as size labels, changed paths, and preflight status.
  • Open PR queue = repo-wide review pressure; it is not a PR quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 62 PR(s), 347 issue(s).
  • PR-specific overlap: none found.
Maintainer notes
  • No linked issue detected: The planned PR does not reference a closing issue or explicit linked issue number.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
  • Re-run Gittensory review

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers. Learn more about Gittensor contribution workflows.

@gittensory gittensory Bot added the gittensory:reviewed Gittensor contributor context label Jun 11, 2026
@superagent-security

Copy link
Copy Markdown

Superagent didn't find any vulnerabilities or security issues in this PR.

@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jun 11, 2026
@JSONbored JSONbored self-assigned this Jun 11, 2026
@JSONbored JSONbored closed this Jun 11, 2026
@JSONbored JSONbored reopened this Jun 11, 2026
@reviewwed

reviewwed Bot commented Jun 11, 2026

Copy link
Copy Markdown

🔍 Maintainer advisory review

Reviewed 2 changed file(s) — two independent AI reviewers. This is an advisory review — it does not merge or close the PR.

Suggested action: 🛠️ Request changes. (reviewers split: request changes / merge) Address the items below before merging.

Reviewer A · gpt-oss-120b — recommends 🛠️ request changes
The change adds sensible HTTPS‑only validation for many metadata fields, introduces safe‑download path checks, and expands test coverage. Functionally it matches the PR description and the new tests exercise the added logic. However, the implementation relies on simple string prefix checks (isHttpsUrl) which can be bypassed with crafted URLs (e.g., "https://example.com@evil.com"). Adding repoUrl validation may also break existing entries that use non‑HTTPS schemes (git://, http://). These points merit a review before merging.

Suggestions

  • Replace the naïve isHttpsUrl string check with proper URL parsing (new URL(value).protocol === "https:") to avoid scheme‑bypass tricks.
  • Document the new repoUrl requirement and consider whether existing entries need migration or a deprecation path.
  • Add unit tests for edge‑case URLs (e.g., "https://example.com@evil.com", "HTTPS://example.com", "https://example.com\n") to ensure robust validation.
  • Ensure that the validation error aggregation still reports all offending fields, not just the first.

Worth double-checking

  • Existing content with non‑HTTPS repoUrl or other URL fields will now fail validation, causing a regression in the submission pipeline.
  • Potential URL‑scheme bypass if isHttpsUrl remains a simple prefix check.

Reviewer B · nemotron-3-120b-a12b — recommends ✅ merge
The change improves URL validation by extending HTTPS requirements to more fields, adding list validation, and securing downloadUrl with path-based allowlisting. It correctly prevents javascript/data: URL injections and enforces HTTPS for critical metadata fields. The tests cover new validation cases and unsafe scenarios. Security risks are mitigated through input sanitization (no shell/command execution) and strict URL schemes. Maintenance is improved via centralized field lists. Regression risk is limited to blocking previously accepted non-HTTPS/content, which is intentional for security.

Suggestions

  • Consider tightening isSafeDownloadUrl to check path components for '..' rather than substring to avoid false positives (e.g., 'file..txt'), though risk is low.
  • Ensure repoUrl validation aligns with actual usage (may need to allow http for legacy internal mirrors? but HTTPS-only is correct for public-facing content).

Worth double-checking

  • Overly restrictive downloadUrl validation might block edge-case legitimate filenames containing '..' (e.g., 'plugin..v1.mcpb'), though such names are highly atypical.
  • Adding repoUrl validation may break existing entries using HTTP repos, but this is desirable for security and aligns with HTTPS-first policy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

aardvark codex gittensory:reviewed Gittensor contributor context size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant