Skip to content

fix: normalize legacy configuration safely - #49

Open
usercao wants to merge 1 commit into
mainfrom
fix/legacy-configuration-normalization
Open

usercao wants to merge 1 commit into
mainfrom
fix/legacy-configuration-normalization

Conversation

@usercao

@usercao usercao commented Oct 30, 2026

Copy link
Copy Markdown
Collaborator

Summary

Direct callers of the public PluginConfigurationManager can pass JavaScript objects rather than JSON. A throwing getter or revoked proxy currently escapes normalization, caller mutations can change accepted tag arrays or Emmet options without a configuration update, and an unknown __proto__ lint key is lost.

Copy accepted tag arrays and top-level configuration records, retain nested object references, and preserve unknown lint keys as own data properties. Validate array lengths before normalizing entries; logging a rejected value can still read its properties. If reading or normalizing configuration throws, reset to defaults, report the problem through the existing safe logger, and notify configuration listeners once. Listener callbacks remain outside the normalization catch.

This change contains the legacy normalization work separated from PR #43: the normalization guard, getArrayLength, own-property lint writes, and cloneConfigurationRecord. It is based directly on main and introduces no editor configuration protocol types, listener lifecycle changes, or diagnostic refresh changes.

Testing

  • corepack yarn test:unit -t 'PluginConfigurationManager'
  • corepack yarn verify

The seven new regression cases failed on main before the implementation change and pass afterward. The full verify run passes.

Copilot AI balanced review requested due to automatic review settings October 30, 2026 17:13
@changeset-bot

changeset-bot Bot commented Oct 30, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 088af11

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@styled/typescript-styled-plugin Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

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.

🟢 Approval recommended

No active review issues were identified for the legacy configuration-normalization changes.

No open issues 🎉

What changed in this PR

Hardens legacy configuration normalization for direct JavaScript callers of the public PluginConfigurationManager, without changing the editor configuration protocol or listener lifecycle.

Changes:

  • Copies accepted tag arrays and top-level configuration records while preserving nested references and unknown lint keys.
  • Validates array lengths and recovers from configuration read failures by restoring defaults, logging safely, and notifying listeners.
  • Adds regression coverage, documents the behavior, and includes a patch changeset.
File Description
test/​unit/​plugin-configuration.test.ts Adds regression cases for copying, special lint keys, invalid lengths, and throwing inputs.
src/​configuration/​plugin-configuration.ts Adds guarded normalization, shallow copies, safe lint property writes, and array-length validation.
docs/​architecture.md Documents recovery, copying, and listener behavior.
.changeset/​steady-legacy-configuration.md Records the configuration fixes for a patch release.

Review effort: Balanced

@pullfrog pullfrog 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.

✅ No new issues found.

Reviewed changes

  • Reset on unreadable input: updateFromPluginConfig now wraps normalization in try/catch; a throwing getter, revoked proxy, or throwing length resets the whole configuration to defaults, logs one message, and still notifies listeners exactly once with listener callbacks kept outside the guard.
  • Mutation independence: accepted tag arrays are copied, and emmet/merged lint records are shallow-copied via Object.fromEntries, while nested values stay shared and lint.validProperties is rebuilt as a new array.
  • Safe array length: new getArrayLength rejects arrays whose reported length is not a nonnegative safe integer before iteration, applied to both tags and lint.validProperties.
  • Own-key preservation: unknown lint keys are written with Object.defineProperty, so __proto__ survives as an own data property without changing the record's prototype.
  • Tests/docs/changeset: seven targeted regression cases, an updated Configuration section in docs/architecture.md, and a patch changeset.

I re-ran the full unit suite (942 pass) and yarn typecheck, and confirmed the seven new tests fail when only src/configuration/plugin-configuration.ts is reverted to the base commit. The __proto__ behavior is preserved end to end (spread and Object.fromEntries both create an own data property) and is inert in the downstream vscode-css-languageservice lint configuration, which only reads own keys.

Pullfrog  | View workflow run | Using big-pickle (free) | 𝕏

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants