fix(Markdown): link destinations follow navigation contract, image sources reject every data: URL by ManoharPaturi · Pull Request #6355 · facebook/astryx · GitHub
Skip to content

fix(Markdown): link destinations follow navigation contract, image sources reject every data: URL - #6355

Open
ManoharPaturi wants to merge 1 commit into
facebook:mainfrom
ManoharPaturi:fix/parser-is-safe-url-data
Open

fix(Markdown): link destinations follow navigation contract, image sources reject every data: URL#6355
ManoharPaturi wants to merge 1 commit into
facebook:mainfrom
ManoharPaturi:fix/parser-is-safe-url-data

Conversation

@ManoharPaturi

@ManoharPaturi ManoharPaturi commented Sep 17, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #6323.

Rebuilt against current main (the branch had drifted behind the url.ts policy refactor).

Policy split, per the review:

  • Links follow the shared navigation contract: javascript:, vbscript:, and data:text/html are blocked; other data: URLs (e.g. data:image/png) stay reachable as links.
  • Images keep the broader embedded-resource rejection: every data: payload is blocked, via the new isSafeMarkdownImageUrl in url.ts.

Tests pin each side: data:image/png links render, data:text/html links are dropped, data:image/* image sources are rejected, javascript: is rejected for both roles, and the standalone-image path is covered. Full parser suite 149/149 green, ruff clean.

Changeset updated to the split-policy wording.

The old head fought the url.ts refactor; this branch is cut fresh from main so the diff is just the policy split + tests + changeset.

@ManoharPaturi
fix(Markdown): reject all data: URL schemes in parser isSafeUrl
6729465 
Signed-off-by: Manohar Paturi <186662190+ManoharPaturi@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 17, 2026 06:53
@vercel

vercel Bot commented Sep 17, 2026 •
edited
Loading

Copy link
Copy Markdown

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Meta Open Source bot. label Sep 17, 2026
Copilot AI reviewed Sep 17, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge labels Sep 17, 2026
@astracat-bot

astracat-bot Bot commented Sep 17, 2026 •
edited
Loading

Copy link
Copy Markdown

AI review status for this pull request.

Review status Updated
🟡 Reviewed (for maintainers only) Sep 23, 2026, 6:49 PM UTC

github-actions Bot added a commit that referenced this pull request Sep 17, 2026
@github-actions
Deploy PR #6355 preview
0901600
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

PR Analysis Report

📚 Storybook Preview

View Storybook for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

🧪 Sandbox Preview

View Sandbox for this PR
GitHub Pages may take up to a minute to hydrate after deploy.

No new or modified components detected.

Bundle Size Summary

Package Size (ESM) Size (CJS) Gzipped
@astryxdesign/core N/A 4.8KB 1.2KB

Accessibility Audit

Status: 36 accessibility violation(s) found — 2 critical, 34 serious.

Button pattern - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/18 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
ChatComposer - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/18 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
ChatComposerInput - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/14 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
ChatReasoning - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 4/5 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
ChatToolCalls - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 10/11 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
Checkbox pattern - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 2/33 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
CheckboxList - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 4/17 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
ClickableCard - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/5 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
CodeEditor - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 3/7 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
CodeEditorPerf - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 2/2 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
CodeEditorTheme - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 13/14 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
CodeTheme - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 14/16 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
DateRangeInput - 1 issue(s)
  • 🔴 critical: Ensure an element's role supports its ARIA attributes
    • Rule: aria-allowed-attr · Affects 1/21 stories · Learn more
    • WCAG: 4.1.2 (Level A)
FileInput - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 3/15 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
GridMasonry - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/3 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
Heading - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/16 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
Icon - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/18 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
LogStream - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 2/3 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
MediaTheme Auto - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/5 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
PowerSearch - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 2/26 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
ProgressBar - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 2/18 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
Radio group pattern - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 3/26 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
RadioList - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 3/13 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
RichTextEditor - 2 issue(s)
  • 🟠 serious: Ensure every ARIA input field has an accessible name
    • Rule: aria-input-field-name · Affects 2/18 stories · Learn more
    • WCAG: 4.1.2 (Level A)
  • 🔴 critical: Ensure every form element has a label
    • Rule: label · Affects 1/18 stories · Learn more
    • WCAG: 4.1.2 (Level A)
SelectableCard - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/4 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
Stepper - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/30 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
TableGroupedRows - 1 issue(s)
  • 🟠 serious: Ensure ARIA attributes are used as described in the specification of the element's role
    • Rule: aria-conditional-attr · Affects 3/3 stories · Learn more
    • WCAG: 4.1.2 (Level A)
TableTree - 1 issue(s)
  • 🟠 serious: Ensure ARIA attributes are used as described in the specification of the element's role
    • Rule: aria-conditional-attr · Affects 8/9 stories · Learn more
    • WCAG: 4.1.2 (Level A)
Text - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/23 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
Theme - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/5 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
PopArt - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 3/6 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
Thumbnail - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/12 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
Timestamp - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 1/20 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
Token - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 2/10 stories · Learn more
    • WCAG: 1.4.3 (Level AA)
Tokenizer - 1 issue(s)
  • 🟠 serious: Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
    • Rule: color-contrast · Affects 2/24 stories · Learn more
    • WCAG: 1.4.3 (Level AA)

Visual Regression

Status: No visual change across 4 compared shot(s).

Generated by PR Enrichment workflow | Storybook | Sandbox | View full report

astracat-bot[bot]
astracat-bot Bot suggested changes Sep 23, 2026
View reviewed changes

@astracat-bot astracat-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Closes a genuine gap: non-HTML data: URLs could reach parser AST nodes even though the documented contract names data: as rejected, and the new tests plus typecheck and formatting all pass.

One change is needed before approval. The patch uses a single URL check for both links and images, so it also rejects every data: link destination. The current navigation contract blocks javascript:, vbscript:, and data:text/html for navigation and keeps broader rejection reserved for embedded resources such as images. Please keep the stricter rejection for image sources and restore the existing navigation behavior for links, with tests pinning each side.

[Automated review]

@ManoharPaturi ManoharPaturi changed the title fix(Markdown): reject all data: schemes in parser isSafeUrl (#6323) fix(Markdown): link destinations follow navigation contract, image sources reject every data: URL Sep 26, 2026
@ManoharPaturi

ManoharPaturi commented Sep 26, 2026

Copy link
Copy Markdown
Contributor Author

This branch was successfully deployed

1 active deployment
Preview — 67294656 Deployed Sep 17, 2026 by vercel[bot]
View deployment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@cixzhang cixzhang Awaiting requested review from cixzhang cixzhang is a code owner
@imdreamrunner imdreamrunner Awaiting requested review from imdreamrunner imdreamrunner is a code owner
@josephfarina josephfarina Awaiting requested review from josephfarina josephfarina is a code owner
+1 more reviewer
@astracat-bot astracat-bot[bot] astracat-bot[bot] requested changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Meta Open Source bot. community Authored by a community contributor (not on the eng/design team) needs:code-review High-risk change (new package/component/API) — needs human code review before merge

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Markdown: parser's isSafeUrl documents rejecting data: but only rejects data:text/html

2 participants

@ManoharPaturi