fix: prevent wildcard CSS leakage in sb-errordisplay_main#34064
fix: prevent wildcard CSS leakage in sb-errordisplay_main#34064karimKandil0 wants to merge 2 commits intostorybookjs:nextfrom
Conversation
📝 WalkthroughWalkthroughUpdates error display styling in Storybook's base preview head by reorganizing CSS selectors for improved specificity and isolation under Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. 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: 4
🧹 Nitpick comments (1)
code/core/assets/server/base-preview-head.html (1)
152-223: Please add the regression test called out in the issue.This is the kind of template-level CSS bug that can regress silently. A focused check that the emitted preview stylesheet contains no raw
&selectors and no top-level* { background/color }rule would lock in the fix from#34036.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/assets/server/base-preview-head.html` around lines 152 - 223, Add a regression test that loads/produces the preview stylesheet and asserts it contains neither raw ampersand selectors nor a top-level universal rule setting background or color; specifically check that patterns like "&" (raw nested selector) do not appear and that there is no rule matching "* { background" or "* { color" (or variations) — target the preview CSS emitted for the Storybook error display (selectors shown in the diff such as ".sb-errordisplay_main *") and name the test something like "preview-stylesheet-no-raw-ampersand-or-universal-color-rule" so it fails if generation reintroduces a top-level "* { background/color }" or stray "&" in the emitted stylesheet.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/assets/server/base-preview-head.html`:
- Around line 157-161: The global reset selector .sb-errordisplay_main * is
overwriting ANSI-rendered <span> children inside `#error-stack`; update the CSS to
avoid clobbering those spans by either narrowing the reset selector or adding an
overriding rule that restores ANSI children inside the error block—e.g., change
the broad reset so it doesn't target `#error-stack` (or use :not(`#error-stack`))
and add/extend the existing restoration around the code block (references:
selector .sb-errordisplay_main *, the `#error-stack` element and the code
restoration rules near the later block) to include `#error-stack` span {
background: transparent; color: inherit; } so ANSI spans keep the dark code
theme.
- Around line 222-223: The CSS selector is inverted and never matches because
.sb-errordisplay_main is inside .sb-errordisplay; update the rule selector from
".sb-errordisplay_main .sb-errordisplay pre" to the correct descendant order
".sb-errordisplay .sb-errordisplay_main pre" (or simply target ".sb-errordisplay
pre" if you want all stack blocks) so that the white-space: revert rule actually
applies to the rendered error stack block.
In `@flake.nix`:
- Around line 12-15: The default dev shell currently exposes pnpm (packages list
including pnpm) which conflicts with the repo's Yarn Berry workflow; remove pnpm
from the packages array and add the Yarn package instead (e.g., replace pnpm
with yarn or the matching yarn package name provided by your nixpkgs) so the
packages list contains nodejs and yarn, ensuring contributors use the repo's
pinned Yarn version configured via .yarnrc.yml and package.json.
- Around line 6-11: The flake currently hardcodes system = "x86_64-linux" which
breaks multi-arch dev shells; replace this with flake-utils' multi-system
pattern by using flake-utils.lib.eachDefaultSystem (or forAllSystems) to iterate
default systems and generate outputs.devShells per system, import nixpkgs with
the system variable inside that loop (e.g., pkgs = import nixpkgs { inherit
system; }), and keep the existing devShells.${system}.default = pkgs.mkShell
block unchanged except for being created per-system; reference symbols: outputs,
system, pkgs, nixpkgs, devShells, mkShell, and flake-utils.
---
Nitpick comments:
In `@code/core/assets/server/base-preview-head.html`:
- Around line 152-223: Add a regression test that loads/produces the preview
stylesheet and asserts it contains neither raw ampersand selectors nor a
top-level universal rule setting background or color; specifically check that
patterns like "&" (raw nested selector) do not appear and that there is no rule
matching "* { background" or "* { color" (or variations) — target the preview
CSS emitted for the Storybook error display (selectors shown in the diff such as
".sb-errordisplay_main *") and name the test something like
"preview-stylesheet-no-raw-ampersand-or-universal-color-rule" so it fails if
generation reintroduces a top-level "* { background/color }" or stray "&" in the
emitted stylesheet.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: daf1ab83-c117-4945-bc06-6afdd62fe3ae
📒 Files selected for processing (2)
code/core/assets/server/base-preview-head.htmlflake.nix
| /* Prevent wildcard leakage */ | ||
| .sb-errordisplay_main * { | ||
| background: white; | ||
| color: black; | ||
| } |
There was a problem hiding this comment.
Keep ANSI-rendered stack spans inside the dark code theme.
#error-stack is filled with ansiConverter.toHtml(detail), so the universal reset on Lines 157-160 also hits nested <span> nodes. Right now only the code element is restored on Lines 217-220, which leaves ANSI children with white backgrounds inside the dark <pre>.
Suggested fix
- .sb-errordisplay_main .sb-errordisplay_code code {
+ .sb-errordisplay_main .sb-errordisplay_code code,
+ .sb-errordisplay_main .sb-errordisplay_code * {
background-color: inherit;
color: inherit;
}Also applies to: 217-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/assets/server/base-preview-head.html` around lines 157 - 161, The
global reset selector .sb-errordisplay_main * is overwriting ANSI-rendered
<span> children inside `#error-stack`; update the CSS to avoid clobbering those
spans by either narrowing the reset selector or adding an overriding rule that
restores ANSI children inside the error block—e.g., change the broad reset so it
doesn't target `#error-stack` (or use :not(`#error-stack`)) and add/extend the
existing restoration around the code block (references: selector
.sb-errordisplay_main *, the `#error-stack` element and the code restoration rules
near the later block) to include `#error-stack` span { background: transparent;
color: inherit; } so ANSI spans keep the dark code theme.
| .sb-errordisplay_main .sb-errordisplay pre { | ||
| white-space: revert; |
There was a problem hiding this comment.
This pre selector never matches the rendered error DOM.
In code/core/assets/server/base-preview-body.html, .sb-errordisplay_main sits inside .sb-errordisplay, not the other way around, so this rule is dead. That means white-space: revert never applies to the stack block.
Suggested fix
- .sb-errordisplay_main .sb-errordisplay pre {
+ .sb-errordisplay_main pre {
white-space: revert;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .sb-errordisplay_main .sb-errordisplay pre { | |
| white-space: revert; | |
| .sb-errordisplay_main pre { | |
| white-space: revert; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/assets/server/base-preview-head.html` around lines 222 - 223, The
CSS selector is inverted and never matches because .sb-errordisplay_main is
inside .sb-errordisplay; update the rule selector from ".sb-errordisplay_main
.sb-errordisplay pre" to the correct descendant order ".sb-errordisplay
.sb-errordisplay_main pre" (or simply target ".sb-errordisplay pre" if you want
all stack blocks) so that the white-space: revert rule actually applies to the
rendered error stack block.
| outputs = { self, nixpkgs }: | ||
| let | ||
| system = "x86_64-linux"; | ||
| pkgs = import nixpkgs { inherit system; }; | ||
| in { | ||
| devShells.${system}.default = pkgs.mkShell { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'flake.nix system key:\n'
nl -ba flake.nix | sed -n '6,15p'
printf '\nRepo-supported architectures from .yarnrc.yml:\n'
rg -n -A6 -B2 'supportedArchitectures|cpu:|os:|- x64|- arm64|- linux|- darwin' .yarnrc.ymlRepository: storybookjs/storybook
Length of output: 146
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'flake.nix (lines 6-15):\n'
sed -n '6,15p' flake.nix | cat -n
printf '\nRepo-supported architectures from .yarnrc.yml:\n'
rg -n 'supportedArchitectures|cpu:|os:|x64|arm64|linux|darwin' .yarnrc.yml | head -20Repository: storybookjs/storybook
Length of output: 547
Make the dev shell portable across all supported architectures.
The hardcoded system = "x86_64-linux" means nix develop will only work for contributors on Linux x86-64. This conflicts with .yarnrc.yml, which declares support for both darwin and arm64 architectures. Contributors on macOS or ARM64 systems cannot use the dev shell.
Use flake-utils to generate shells for all default systems:
Portable flake pattern
{
description = "storybook dev environment";
inputs.nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable";
+ inputs.flake-utils.url = "github:numtide/flake-utils";
- outputs = { self, nixpkgs }:
- let
- system = "x86_64-linux";
- pkgs = import nixpkgs { inherit system; };
- in {
- devShells.${system}.default = pkgs.mkShell {
- packages = with pkgs; [
- nodejs
- pnpm
- ];
- };
- };
+ outputs = { nixpkgs, flake-utils, ... }:
+ flake-utils.lib.eachDefaultSystem (system:
+ let
+ pkgs = import nixpkgs { inherit system; };
+ in {
+ devShells.default = pkgs.mkShell {
+ packages = with pkgs; [
+ nodejs
+ pnpm
+ ];
+ };
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| outputs = { self, nixpkgs }: | |
| let | |
| system = "x86_64-linux"; | |
| pkgs = import nixpkgs { inherit system; }; | |
| in { | |
| devShells.${system}.default = pkgs.mkShell { | |
| { | |
| description = "storybook dev environment"; | |
| inputs.nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable"; | |
| inputs.flake-utils.url = "github:numtide/flake-utils"; | |
| outputs = { nixpkgs, flake-utils, ... }: | |
| flake-utils.lib.eachDefaultSystem (system: | |
| let | |
| pkgs = import nixpkgs { inherit system; }; | |
| in { | |
| devShells.default = pkgs.mkShell { | |
| packages = with pkgs; [ | |
| nodejs | |
| pnpm | |
| ]; | |
| }; | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 6 - 11, The flake currently hardcodes system =
"x86_64-linux" which breaks multi-arch dev shells; replace this with
flake-utils' multi-system pattern by using flake-utils.lib.eachDefaultSystem (or
forAllSystems) to iterate default systems and generate outputs.devShells per
system, import nixpkgs with the system variable inside that loop (e.g., pkgs =
import nixpkgs { inherit system; }), and keep the existing
devShells.${system}.default = pkgs.mkShell block unchanged except for being
created per-system; reference symbols: outputs, system, pkgs, nixpkgs,
devShells, mkShell, and flake-utils.
| packages = with pkgs; [ | ||
| nodejs | ||
| pnpm | ||
| ]; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Declared package manager:\n'
rg -n '"packageManager"\s*:\s*"[^"]+"' package.json
printf '\nYarn-specific repo config:\n'
rg -n 'yarnPath|nodeLinker|npmRegistryServer' .yarnrc.yml
printf '\nflake.nix shell packages:\n'
nl -ba flake.nix | sed -n '11,15p'Repository: storybookjs/storybook
Length of output: 368
🏁 Script executed:
sed -n '12,15p' flake.nixRepository: storybookjs/storybook
Length of output: 131
Use the repo's Yarn workflow here, not pnpm.
The repository is explicitly pinned to yarn@4.10.3 in package.json:72, and .yarnrc.yml is already configured for Yarn Berry. Including pnpm in the default shell nudges contributors toward an unsupported install path.
🧹 Minimal fix
packages = with pkgs; [
nodejs
- pnpm
];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| packages = with pkgs; [ | |
| nodejs | |
| pnpm | |
| ]; | |
| packages = with pkgs; [ | |
| nodejs | |
| ]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@flake.nix` around lines 12 - 15, The default dev shell currently exposes pnpm
(packages list including pnpm) which conflicts with the repo's Yarn Berry
workflow; remove pnpm from the packages array and add the Yarn package instead
(e.g., replace pnpm with yarn or the matching yarn package name provided by your
nixpkgs) so the packages list contains nodejs and yarn, ensuring contributors
use the repo's pinned Yarn version configured via .yarnrc.yml and package.json.
1 participant
Closes #34036
What I did
Fix nested CSS in
.sb-errordisplay_maininsidebase-preview-head.html.The styles previously used nested selectors such as:
Since this file is plain HTML and not processed by a CSS preprocessor, those selectors can produce invalid or improperly scoped CSS. In particular, the wildcard rule:
could leak styles outside the intended container.
This change rewrites the nested selectors as explicit scoped selectors, ensuring all styles remain contained within
.sb-errordisplay_main.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
.sb-errordisplay_maindo not leak to the rest of the page.Documentation
This change only fixes CSS scoping in the internal error display UI.
Summary by CodeRabbit
Style
Chores