fix: close script injection paths and protect masked settings - #313
Merged
Merged
Conversation
Confirm modal content is rendered with innerHTML, so resource names from the API could inject markup. Add an escapeHtml helper and use it where callers build HTML strings. Escape uploaded file names before SunEditor insertHTML, and destroy the editor on re-init and destroy. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
setupPlace passes ignore_api_key, so ts-client never uses a stored API key. The handler only wrote the key and trusted=true to localStorage, and the trusted flag changes the OAuth flow. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Driver default_uri, repository uri and extension url links used the safe 'url' pipe, which let javascript: URLs through. Let Angular sanitise them instead. Reject script schemes in validateURI, and strip repository credentials with the URL API for both link text and href. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Plugin URIs are usually same origin, so allow-same-origin with allow-scripts gave plugins full access to the backoffice session. Drop allow-same-origin, match plugin messages on event.source as the probe already does, and allow only http(s) plugin URIs. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Single key hotkeys like KeyE fired, and called preventDefault, on Cmd/Ctrl+E. Skip a key event when Control, Alt or Meta is held and no combination for that key includes the modifier. Also use isContentEditable to detect editable focus. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The settings form had bare A and C hotkeys for save all and clear, and save all wrote every level, including masked encrypted (level 3) YAML, which could overwrite secrets. Move the hotkeys to Alt+Shift, save only dirty levels the user can edit, and refuse to save encrypted settings that still hold masked values. Also mask admin and support levels for lower privilege users (is_admin and is_support were not called), fix the save guard, push the setting found by level, and return new arrays from signal updates. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The embed query param went straight into the iframe src through a resource URL bypass, so embed=javascript:... ran script in our origin. Load it only when it is http(s) and matches an extension URL configured for the item. Accept messages only from the frame window and origin, reply to that origin instead of '*', and ignore invalid JSON. Handle each message directly, as the per-action timeout dropped all but the last message of each type. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
validateURI read the host of `example.com:8080` as a URI scheme, so bare host:port values passed while `192.168.1.1:80` failed. Require an explicit `scheme://` form and keep blocking script schemes. Show the URI error on the driver form, and only validate the module URI for service and websocket modules, where the field is visible. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Page hotkeys fired under an open dialog, so "e" in the New User dialog opened an Edit dialog on top. A listener now belongs to the dialog that is on top when it registers (or the page) and fires only while that layer is on top. Modifier state now comes from the key event flags, so Shift then Alt then A triggers Alt+Shift+A. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Option labels are rendered with innerHTML, so a name like `<img src=x>` rendered an image. Escape names and details before building the HTML, close the detail span, and keep the `<Unnamed>` fallback visible. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A review found several ways to run script or leak secrets:
?embed=query param into an iframesrcthroughbypassSecurityTrustResourceUrl. A link withembed=javascript:...ran script in the backoffice origin. Itsmessagelistener also accepted messages from any window.safe: 'url', so a storedjavascript:URL ran when an admin clicked it.href.allow-scripts allow-same-origin. Plugins load from the same origin, so the sandbox did nothing.Akey press. It saved every level, including masked<MASKED>secrets, which overwrote the stored values.This is part of a stack of 6 PRs from a review of the whole app. Merge them in order: confirm-cancel, security, data loss, broken features, async errors, cleanup. Each PR targets the branch before it, so the diff shows only its own changes.
Fix
safe: 'url'bypass.validateURIrequiresscheme://and rejectsjavascript:,vbscript:anddata:. The driver form shows the error.maskUriCredentialsremoves the user and password from repository URIs.allow-same-originfrom the plugin sandbox.escapeHtmland use it for names in confirm dialogs, the rich text editor and item search results.?x-api-key=handler. Withignore_api_key: truethe key did nothing, but it settrusted=true.Behaviour changes: URIs without
//(for examplelocalhost:80ormailto:) fail validation. Signage plugins cannot use localStorage or cookies.Checks
tsc,bun run lint,bunx vitest run(994 tests) pass. New tests cover the URL checks, credential masking, hotkey rules and the settings form.embed=javascript:gave a blank frame with no alert. Plugin frames hadorigin=null. Masked secrets stayed encrypted after save attempts. Injected<img>names showed as text with no network request.Made by Claude Opus 5.5 (1M context) in Claude Code (T3 Code).
🤖 Generated with Claude Code