Repository navigation
Conversation
…controls
Second narrow widening of the plugin-chrome exemption, following the same rule
as the first: a path belongs here only if rewriting it cannot mislead the person
deciding whether to trust a plugin.
Twenty-three paths, in three groups. The install dialog's own form — tab labels,
field labels, placeholders and the "you left this blank" validation — asserts
nothing about a plugin; it collects a URL or a folder path. The log affordances
on an installed row ("View logs", "Restarting", "No log lines recorded.") report
what the plugin is doing after the trust decision, not before it. And the
Experimental badge names how finished the feature is.
What stays protected is the reason the prefix is broad. Trust state on the same
row — `blocked`, `bundled`, `dev`, `needsReview`, `invalid`, `viewAdvisory` —
stays, as do enable, remove and rollback. `gitRefRequired` stays because it
argues why pinning a ref matters. The components the security tests pin whole
are untouched.
Tests pin both directions: the newly exempt paths parse, and the trust copy
beside them still fails with 'protected security copy'.
|
@nwparker — no reviewer again, same reason as #12455: This is the second wave you left room for, and it takes one of the two follow-ups you named on that PR:
Reading it key by key, the mix separates: the form (tabs, labels, placeholders, blank-field validation) collects a URL or a folder path before any plugin is named, while The other follow-up — splitting Rebased onto current |
📝 WalkthroughWalkthroughThe plugin translation allowlist now includes development search, experimental status, installation-dialog, log, and runtime-state paths. Tests cover every exempt path and confirm that adjacent security-sensitive settings and installation fields remain protected. Mergeability Score: 🔵 Low · up to The PR allows language packs to translate additional non-trust plugin labels, but an accidental removal of an allowlisted path could go undetected and leave part of the UI untranslated. This bounded test-integrity risk is mergeable with explicit owner follow-up. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/plugins/plugin-language-pack-artifact.test.ts (1)
64-101: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCover every new allowlist path and documented protected sibling.
The positive cases omit
plugins.search.development, tenPluginInstallDialogpaths, and sevenPluginSettingsRowpaths. The rejection cases omitblocked,bundled, anddev, although the policy comment declares them protected.Add parameterized cases for these paths. This will lock the translation boundary against namespace and allowlist regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 860a4676-21dd-4b75-a42c-82e4ff046434
📒 Files selected for processing (2)
src/shared/plugins/plugin-language-pack-artifact.test.tssrc/shared/plugins/plugin-translatable-chrome.ts
CodeRabbit noticed the positive cases sampled the allowlist — nine of the fifty-seven paths — and that `blocked`, `bundled` and `dev` are declared protected in the policy comment without a test holding them there. The positive side is now parameterised on `translatablePluginChromePaths()`, so every exempt path is walked end to end through the parser and a path added later is covered on the day it lands. This is not a restatement of the list: membership does not make a path reachable, because the walk rejects its protected container first unless the container stays walkable. Breaking `translatablePluginChromeContainer` fails 57 of these; the sampled version missed it. The three trust badges join the refusal cases, so leaking any of them into the allowlist fails instead of merely contradicting a comment.
|
@coderabbitai — both halves taken, one of them differently than suggested. The refusal cases were a real gap. The positive cases are now driven off the list instead of enumerated. Writing the missing paths out by hand would mirror the allowlist into the test file, and that mirror rots on its own — it only ever covers what someone remembered to copy across. The positive case is parameterised on That is not a restatement of the list. Set membership does not make a path reachable: the walk rejects a protected container before it reaches the leaf, so a path only parses while Existence of each path in
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline 1d44fa6
printf '\n--- changed paths ---\n'
git show --format='' --name-only 1d44fa6
printf '\n--- implementation outline ---\n'
ast-grep outline src/shared/plugins/plugin-translatable-chrome.ts --items all
printf '\n--- relevant test outline ---\n'
ast-grep outline src/shared/plugins/plugin-language-pack-artifact.test.ts --items all
ast-grep outline src/renderer/src/i18n/plugin-chrome-allowlist.test.ts --items all
printf '\n--- policy, allowlist, and test assertions ---\n'
rg -n -C 5 'blocked|bundled|dev|translatablePluginChromePaths|translatablePluginChromeContainer|policy|protected' \
src/shared/plugins/plugin-translatable-chrome.ts \
src/shared/plugins/plugin-language-pack-artifact.test.ts \
src/renderer/src/i18n/plugin-chrome-allowlist.test.tsLength of output: 24848
The refusal tests now protect The positive test now iterates over
The prior findings are addressed. 🐇✅ ✏️ Learnings added
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 755de4e3-6c99-48be-8840-8e9b20442d30
📒 Files selected for processing (1)
src/shared/plugins/plugin-language-pack-artifact.test.ts
| it.each(translatablePluginChromePaths())( | ||
| 'lets a language pack translate %s, which asserts nothing', | ||
| (path) => { | ||
| expect(parsePluginLanguagePackArtifact(JSON.stringify(catalogFor(path, 'Перевод'))).ok).toBe( | ||
| true | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add an independent assertion for the expected allowlist.
it.each(translatablePluginChromePaths()) derives every case from the allowlist itself. If a required path is removed from that list, its test case also disappears and the test still passes. Assert the expected 23 new paths independently, while retaining this loop to test parser reachability.
|
Thanks for the PR! Taking a look |
|
@AmethystLiang — friendly nudge, no rush intended. You picked this up on 13 Aug ("Taking a look"), so I wanted to check whether anything here is blocking rather than let it sit silently. The PR has stayed green since: If you would rather hand it to someone else, or if it needs to wait on something upstream of it, just say so and I will adjust — a redirect is more useful to me than an approval. For context, the sibling PR #14127 is also green and mergeable again: upstream split |
This branch has not been deployed
4 participants
Summary
Second narrow widening of the plugin-chrome exemption landed in #12514, following the same rule as the first: a path belongs here only if rewriting it cannot mislead the person deciding whether to trust a plugin.
This takes one of the two follow-ups you left explicitly open on #12455:
Having read that module key by key, the mix is real but it separates cleanly. The dialog's form — tab labels, field labels, placeholders, the "you left this blank" validation — asserts nothing about any plugin. It collects a URL or a folder path before a plugin is even named. The one key in there that does argue a security position is
gitRefRequired, and it stays protected.The other follow-up — splitting
PluginMarketplaceListingRowsoinstalledandcheckUpdatecan move — is untouched. That module holdsofficialandblockedin the same file, and that split is still your call, not mine.What changed
23 exact paths, in three groups, appended to the allowlist in
plugin-translatable-chrome.ts. Exact paths rather than a pattern, same as before: anything new stays protected until someone deliberately adds it.PluginInstallDialog— the formgitTab,localTab,gitLabel,localLabel, both placeholders,gitUrlRequired,localRequired,source,title,install,installing,cancelPluginSettingsRow— log affordancesviewLogs,hideLogs,loadingLogs,noLogs,logCount,restarting,restartCount,running,moreActionsPluginsSettingsSectionexperimentalWhat deliberately stays protected
On the very same rows as the newly exempt copy:
blocked,bundled,dev,needsReview,invalid,viewAdvisory. These are what a reader weighs when deciding whether this plugin should run at all.PluginInstallDialog.gitRefRequired— it argues why pinning a ref matters. That is a security claim, and it is the reason this module was flagged as mixed in the first place.The boundary is deliberately awkward here:
viewLogsbecomes translatable whileblockedtwo elements away does not. That asymmetry is the point — the rule follows what the copy asserts, not where it sits.Screenshots
No visual change in English — no English string is added, removed, or reworded. Only the set of paths a language pack is permitted to override changes.
For context, Settings → Plugins in a build with a Russian language pack before this series began — the inner list translated while the frame stayed English:
Experimentalbeside the heading and theInstall pluginbutton that opens the dialog are both in this PR; the dialog's own form is what the 13 paths above cover.Testing
pnpm lintpnpm typecheckpnpm testpnpm buildplugin-language-pack-artifact.test.tspins both directions: each newly exempt path parses, and the trust copy sitting beside it still fails withprotected security copy. A future edit that widens the allowlist past the rule fails the second half.src/shared/plugins/is green end to end — 16 files, 153 tests. The full suite reports 75 failures in 9 files, all outside this diff: six IME/xterm specs underterminal-pane, plusssh-posix-command-wrapper,agent-exec-handlerandterminal-snapshot-osc8-roundtrip. Those fail on a clean checkout here too, and the count drifted between two consecutive runs (76 → 75), so they are environmental rather than deterministic.AI Review Report
Reviewed the diff for correctness, security-boundary reasoning, and cross-platform impact on macOS, Linux, and Windows.
main(83 commits of drift since it was written); all 57 allowlisted paths — 34 from fix(i18n): consolidate the open community translation PRs #12514 plus these 23 — resolve inen.jsontoday. A stale entry would be dead weight in a security allowlist, so this was checked mechanically rather than by eye.gitRefRequiredfailed that test and stayed out, which is the same judgment that keepssystemDescriptionand the*Failedfamily protected in fix(i18n): consolidate the open community translation PRs #12514.Security Audit
*Failedfamily remain refused.plugin-language-pack-artifact.test.tsmean the boundary is enforced by tests rather than by review memory.Notes
If you would rather take the log affordances and leave
PluginInstallDialogfor a separate pass, the three groups are independent and can be split — say the word and I will resubmit them apart.