fix: propagate safeMode thru components - #1593
Conversation
WalkthroughThe change adds the public Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
__tests__/lib/hast.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Error while loading rule ' __tests__/lib/mdxishTags.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Error while loading rule ' __tests__/lib/tags.test.tsOops! Something went wrong! :( ESLint: 8.57.1 Error: Error while loading rule '
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@processor/transform/flatten-attribute-expressions.ts`:
- Around line 16-23: Update flattenAttributeExpressions so flattened expression
values retain their original source provenance or are marked to bypass
subsequent decodeHTMLStrict processing in getAttrs(), preserving character
references such as String("&") exactly. Add a regression test asserting the
resulting attribute source remains String("&").
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c396c049-b918-4986-b5b4-bc325e5f060f
📒 Files selected for processing (11)
__tests__/lib/hast.test.ts__tests__/lib/mdxishTags.test.ts__tests__/lib/tags.test.ts__tests__/transformers/readme-components.test.tslib/ast-processor.tslib/hast.tslib/mdxishTags.tslib/tags.tsprocessor/transform/flatten-attribute-expressions.tsprocessor/transform/index.tsprocessor/utils.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
readmeio/ai(manual)readmeio/gitto(manual)readmeio/markdown(manual)readmeio/readme(manual)
| const flattenAttributeExpressions = (): Transform => tree => { | ||
| visit(tree, isMDXElement, (node: MdxJsxFlowElement | MdxJsxTextElement) => { | ||
| node.attributes.forEach(attr => { | ||
| if (!('name' in attr)) return; | ||
| if (attr.value === null || typeof attr.value === 'string') return; | ||
|
|
||
| attr.value = attr.value.value; | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve character references in flattened expression source.
Line 22 converts expression source into a normal string. getAttrs() then applies decodeHTMLStrict() to that value. For icon={String("&")}, safe mode returns String("&") instead of the original expression source.
Keep provenance for flattened expression values, or bypass HTML decoding for them. Add a regression test that asserts the exact source text remains String("&").
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@processor/transform/flatten-attribute-expressions.ts` around lines 16 - 23,
Update flattenAttributeExpressions so flattened expression values retain their
original source provenance or are marked to bypass subsequent decodeHTMLStrict
processing in getAttrs(), preserving character references such as
String("&") exactly. Add a regression test asserting the resulting attribute
source remains String("&").
| 🎫 Resolve ISSUE_ID |
| :-----------------: |
## 🎯 What does this PR do?
- **Context.** `getAttrs()` resolves attribute expressions with `new
Function`, and it does so as a side effect of *building* the tree. Every
`mdast()`, `hast()`, `tags()` and `exports()` call therefore executes
whatever sits in `attr={…}`, no matter what the caller does with the
result — `tags()` only wants component names and still runs the code.
- **Solution.** New `evaluateLiteralExpression()` resolves those
expressions by folding the acorn AST instead of executing it. It's an
allowlist of literal node types (literals, arrays, objects,
substitution-free templates, `+ - * /`, unary `-`, `undefined`);
everything else throws and the caller keeps the raw source, which is the
fallback a thrown `evaluate()` already had. `evaluate()` itself is
untouched and now has no ungated callers — the four that remain are
already behind mdxish's `safeMode`.
- **Why `evaluate()` was there.** `getAttrs()` used to `JSON.parse` the
expression source. That can't take the forms JSX attributes actually use
— `{ textAlign: "left" }`, `{ color: 'red' }` — and it threw uncaught,
so those crashed the parse. It worked in practice only because mdxish's
preprocessor pre-evaluated attributes before `getAttrs` ever saw them.
When that preprocessing was removed (it was destroying the authored
source, `{1 + 1}` → `"2"`), `evaluate()` filled the gap. Folding covers
the same forms without running anything.
- **Limitation** Expressions that need a scope or call a method:
`{item.url}`, `{"a".toUpperCase()}`, `{['a','b'].join('/')}`, no longer
resolve in `getAttrs`. **mdxish rendering is unaffected** since
`resolveDeferredAttributeExpressionProps` still resolves those
downstream with scope. What changes is anything reading attributes
straight off the tree: (`mdast`, `hast`, `tags`, `exports`), and the
mdxish editor AST.
- For the editor side, this affects components that's transformers use
`getAttrs` such as images, callouts, etc. This means those components
containing function calls in the attributes won't get WYSIWYG, and if
pasted, not evaluate. E.g. Pasting "<Image
src="https://files.readme.io/b8674d6-pizzabro.jpg" align="center"
caption={"aaa".toUpperCase()} />" wouldn't execute the `toUpperCase()`
anymore. However, this is same behaviour as the old editor so I think
it's fine to have, and if we manually type the expressions in the editor
it would still get retained.
- **Overlap with #1593.** Since folding never runs code, `safeMode` no
longer changes the *outcome* for attribute expressions — both settings
yield the source string, so its two "evaluates by default" tests are
flipped here. `safeMode` still gates the ESM and expression
transformers, and `flattenAttributeExpressions` still flattens earlier
and more explicitly; the two just overlap on attributes now.
## 🧪 QA tips
- [ ] Paste the block below into a page and diff against `next`. **In
the MDXish renderer everything should be identical, above and below the
divider** — rendering is unchanged. In the MDX renderer, and in the
MDXish editor's attribute fields, the cases below the divider should now
show the authored source instead of a computed value.
```markdown
<!-- Still resolves — unchanged from `next`, in both engines -->
<Image src="/x.png" border={true} width={100} />
<Callout icon={undefined} theme={"info"}>Quoted string, either quote style</Callout>
<Callout icon={`plain`}>Template literal with no substitution</Callout>
<Callout icon={1 + 1}>Arithmetic between literals</Callout>
<Anchor href={'https://' + 'example.com' + '/x'}>Concatenated literals</Anchor>
<Foo align={["left", null, "center"]} style={{ textAlign: "left", color: 'red' }} />
<!-- ————— No longer resolves: renders the source text as written ————— -->
<Callout icon={"hi".toUpperCase()}>Method call on a literal</Callout>
<Anchor href={['a', 'b'].join('/')}>Method call on an array</Anchor>
<Callout icon={item.url}>Identifier / member access (already unresolved on `next`)</Callout>
```
- [ ] Confirm nothing executes: `<Callout icon={process.env.HOME} />`
must render the source, not your home directory.
`__tests__/lib/utils/literal-expression.test.ts` asserts this along with
shell-command and sandbox-escape payloads; each of those tests fails if
`evaluate()` is put back in `getAttrs`.
## 📸 Screenshot or Loom
Changes in the editor (left is prod, right is the new behaviour):
https://github.com/user-attachments/assets/b70cb312-16b8-4708-ad7c-acb7601e53c8
https://github.com/user-attachments/assets/f7525662-3a13-405e-b775-e265fb83c724
While these are behaviour changes, I think it is still acceptable & made
sense. The first video behaviour change I would argue is the more
correct one, whilst the second one we can revisit the serializer if we
want keep the WYSIWSYG-ness
## Version 15.0.2 ### 🛠 Fixes & Updates * **editor:** add spacing between items inside of tabs ([#1590](#1590)) ([a1dbd12](a1dbd12)) * propagate safeMode thru components ([#1593](#1593)) ([87761af](87761af)) * resolve attribute expressions without executing them ([#1585](#1585)) ([50441cf](50441cf)) * support structured user variables ([#1488](#1488)) ([279c200](279c200)), closes [mdx-renderer#317](https://github.com/readmeio/mdx-renderer/issues/317) <!--SKIP CI-->
|
This PR was released!🚀 Changes included in v15.0.2 |
4 participants
🎯 What does this PR do?
safeModeis supported throughout the stack🧪 QA tips
📸 Screenshot or Loom