sketchup-review-extension

name sketchup-review-extension description Find common extension issues. Based on the Extension Warehouse review. Review a SketchUp extension for Extension Warehouse compliance. Inputs Input Path Description Source code SketchUp/SourcePath in .rubocop.yml (default src/ ) The extension source files in this workspace Output Report the review directly in the chat, in the output format specified below. Workflow Determine the source path from SketchUp/SourcePath in .rubocop.yml . Fall back to src/ if the config or key is absent. Run RuboCop-SketchUp static analysis: bundle exec rubocop --format json . If RuboCop is not available (e.g. bundle install has not been run), skip this step, note it in the output, and continue with the manual review. List all files under the source path. Read every Ruby file, JS file and HTML file. Evaluate the rules below. Report the review in the chat, in the exact output format specified. RuboCop Results If the RuboCop run succeeded, incorporate its findings. RuboCop offenses with severity: "error" and cop names starting with SketchupRequirements/ map to rejections. Cross-reference with the rules below to use the correct wording. Don't duplicate — if RuboCop flags something already caught by your manual review, mention it once. Developer Feedback Evaluate every rule below against the code. Each rule is either a Rejection (blocks publishing) or a Note (recommendation, does not block). Rejections These block publishing. Use the exact wording provided, substituting placeholders. Root file does too much The root .rb file - the one .rb file directly in the source path (see Workflow step 1) - should ONLY register the SketchupExtension. No business logic, no loading other files beyond the single support file referenced by the SketchupExtension object, no require of gems or stdlib, no file I/O. If the user chooses to disable the extension in Extension Manager, no other code should run. Exception: The root .rb may load the extension data from a .json file. Exception: The root may define constants. Exception: The root may require "sketchup.rb" and "extension.rb" Rejection: The root .rb file is only allowed to contain the SketchupExtension registration. All other logic should load only through the file referenced by the SketchupExtension object. If the user chooses to disable the extension in the Extension Manager, no other code should run. https://ruby.sketchup.com/file.extension_requirements.html#label-File+Structure (Excess code should be moved to the main file. Version definitions needed for the SketchupExtension registration should be defined directly in the root file.) No top-level namespace All class, module, constant and method definitions must be wrapped inside a single uniquely named top-level module. Ignore if: Code outside the module only uses local variables (which go out of scope when the file ends) and does not define methods, constants, or classes. Rejection: To avoid clashes between extensions, the code needs to be wrapped in a single top level namespace module with a unique name. No global methods are allowed. https://ruby.sketchup.com/file.extension_requirements.html#label-Wrapping+Module Multiple top-level namespaces Only one top-level module is allowed across the entire extension. Rejection: To avoid clashes between extensions, only one top level namespace module is allowed. https://ruby.sketchup.com/file.extension_requirements.html#label-Wrapping+Module Global variables Global variables ( $foo ) are not permitted. Check setting global variables. Reading existing global variables is okay. Rejection: To avoid clashes between extensions we don't allow global variables. You can use instance variables with getter/setter methods instead. https://ruby.sketchup.com/file.extension_requirements.html#label-Global+Variables Console printing (unconditional) puts , print , p called unconditionally in production code path. Exception: Printing inside rescue blocks is acceptable and should NOT be flagged. Only flag unconditional printing outside error handling. Rejection: This extension prints debug information to the Ruby Console. If every extension does this, it clutters the console and makes it harder to use for other developers. Also this information never reaches typical end users, so it can't be relied upon for important information. Please only print to the console when the extension is set to debug mode, not in production. https://ruby.sketchup.com/file.extension_requirements.html#label-Printing+to+the+Console Load error from require (encrypted extension) Using Ruby's require or require_relative with .rb extension. EW encrypts .rb → .rbe by default, breaking these calls. Must use Sketchup.require without file extension for files within the extension. Can use Ruby's require for std lib and other files outside of the extension. Files outside of extensions are not encrypted. Rejection: Load error. By default Extension Warehouse encrypts extensions and turns .rb files to .rbe files. To load these files, use Sketchup.require instead of Ruby's own require and omit the file extension. If require_relative specifically, append: Instead of require_relative, use Sketchup.require with a path relative to the extension install location (Plugins folder). Load error from extension registration (encrypted extension) Hardcoded .rb extension in the path passed to SketchupExtension.new . Rejection: Load error. By default Extension Warehouse encrypts extensions and turns .rb files to .rbe files. Omit the .rb file extension when registering your extension. https://ruby.sketchup.com/file.extension_requirements.html#label-Requiring+Files Installing gems at runtime Rejection: Installing Gems does not work well in SketchUp. It freezes up the program during installation and some Gems need special build tools to be made functional. Also different extensions may want to use different versions of the same gem. Instead copy the code of the Gem into your own extension support folder and wrap it under your own unique namespace. https://ruby.sketchup.com/file.extension_requirements.html#label-Gems Code injection in execute_script Interpolating variables directly into execute_script strings without escaping. Not a rejection if: The interpolated value is produced by JSON.generate , to_json , or another mechanism that guarantees proper escaping. Rejection: Interpolating variables directly into `execute_script` is error prone in case the value contains quotation marks and can be used for code injection attacks. Use `to_json` and omit the quotation marks to make sure the value is fully escaped. # Bad dialog.execute_script("showMessage('#{message}')") # Good require "json" dialog.execute_script("showMessage(#{message.to_json})") if code uses gsub , also explain that to_json is more robust. If every single interpolated variable is clearly coming from safe sources (hardcoded in the source, timestamps etc), downgrade to a Note, but point out it's best practice to escape consistently. Code injection in HTML Passing unsanitized variables into HTML — either through string interpolation or by assigning directly to innerHTML — via WebDialog.set_html , HtmlDialog#set_html , or JavaScript executed by the extension. Rejection: Passing unsanitized variables into HTML is error prone in case the value contains HTML escape characters and can be used for code injection attacks. Escape strings before interpolating them into HTML, or use `textContent` instead of `innerHTML` when inserting plain text. # Bad - user data interpreted as HTML via interpolation name = "<script>alert('Code injection!')</script>" html = "<div>#{name}</div>" # Bad - user data assigned directly to innerHTML element.innerHTML = userProvidedValue # Good - special characters are escaped (Ruby) require "cgi" name = "<script>alert('Code injection!')</script>" html = "<div>#{CGI.escape_html(name)}</div>" # Result: <div>&lt;script&gt;alert('Code injection!')&lt;/script&gt;</div> # Good - use textContent for plain text (JavaScript) element.textContent = userProvidedValue eval usage eval , instance_eval , class_eval , module_eval , Binding#eval on untrusted input. Rejection: `eval` is vulnerable to code injection attacks and should not be used. https://ruby.sketchup.com/file.extension_requirements.html#label-Eval Command injection via system commands Any mechanism that executes an OS command — system , exec , backticks ( ` ), %x{} , IO.popen , Open3 ( popen2 , popen3 , capture2 , capture3 ), Process.spawn , Kernel.spawn , PTY.spawn — where untrusted input is interpolated into the command string without proper sanitization. Untrusted input includes: data from the SketchUp model (attribute dictionaries, entity names, component paths, file paths from model.path ), server responses, user-provided strings (inputboxes, file dialogs), and environment variables set by other extensions. Not a rejection if: The command is entirely hardcoded with no dynamic input, or uses the array form of system / spawn / Open3 which bypasses shell interpolation (e.g. system("cmd", arg1, arg2) ). Rejection: Passing unsanitized input to a system command is a command injection vulnerability. An attacker who controls the input (e.g. via a malicious model file or compromised server) can execute arbitrary commands on the user's machine. # Bad — shell interprets special characters system("open #{user_path}") # Good — array form bypasses the shell entirely system("open", user_path) Third-party update mechanism Code that downloads and installs updates, bypassing EW review. Rejection: SketchUp and Extension Warehouse has infrastructure for extension updates. Adding functionality that downloads and installs updates is not allowed as it bypasses the review. https://ruby.sketchup.com/file.extension_requirements.html#label-Third+Party+Updates Monkey-patching the SketchUp API Reopening or modifying SketchUp API classes/modules ( Sketchup::Model , Geom::* , UI::* , etc.). Rejection: Since SketchUp extensions run in a shared environment, changing the modules and classes of the Ruby API from one extension can clash with another extension. Don't change these modules and classes. https://ruby.sketchup.com/file.extension_requirements.html#label-Monkey+Patching+the+SketchUp+Ruby+API Modifying $LOAD_PATH Rejection: Don't modify the `$LOAD_PATH`. Doing so may cause other extensions to load the wrong files. Instead include your extension support folder name in the path whenever you load a file. https://ruby.sketchup.com/file.extension_requirements.html#label-24LOAD_PATH Modifying environment variables Rejection: Don't modify environment variables. Doing so can cause other extensions to malfunction. https://ruby.sketchup.com/file.extension_requirements.html#label-Environment+Variables Using exit/exit! Rejection: `exit` and `exit!` should not be used to stop the Ruby execution, as all Ruby extensions run in a shared interpreter. Instead use `return`, `next`, `break` or `raise` to stop the execution of your own code. https://ruby.sketchup.com/file.extension_requirements.html#label-Exit Attribute changes without undo wrapping set_attribute and delete_attribute must always be wrapped in model.start_operation / model.commit_operation , even for a single call. Without wrapping, the undo entry falls back to a generic "Properties" label. This also applies inside observers. Model changes within observers should use start_operation / commit_operation with the 4th argument set to true to make the operation transparent to the previous operation (e.g. model.start_operation('Set Attributes', true, false, true) ). Rejection: Attribute changes (set_attribute, delete_attribute) must be wrapped in model.start_operation and model.commit_operation. Without this, the undo entry shows a generic "Properties" label instead of a descriptive name. https://ruby.sketchup.com/file.extension_requirements.html#label-Undo+Stack Multiple model changes without undo wrapping When code makes multiple model modifications in sequence (e.g. add_face + pushpull, creating multiple entities, changing material properties), these must be wrapped in model.start_operation / model.commit_operation so the user can undo them as a single step. This also applies inside observers. Model changes within observers should use start_operation / commit_operation with the 4th argument set to true to make the operation transparent to the previous operation. Not a model modification: Rendering options, camera, selection, and other transient view properties do not record to the undo stack and do not need wrapping. Rejection: When your extension makes several model changes, join them together as one entry to the undo stack using model.start_operation and model.commit_operation. If the user activates it as a single high level action, let them also undo it in a single step. https://ruby.sketchup.com/file.extension_requirements.html#label-Undo+Stack Model changes not triggered by the user If the extension causes changes to the SketchUp model that were not triggered by a user action. Example: Writing attributes at model load Allowed: Making model changes triggered by buttons and similar direct user actions Allowed: Making model changes that responds to other model changes using observers Not allowed: Making model changes when the extension loads, when a model is created or when a model is opened. Rejection: Don't make model changes that are not initiated by the user. It adds to the undo stack and causes SketchUp to ask if the user wants to save when closing the model, even if they didn't do anything to it. WebDialog When the extension uses the WebDialog class. Exception: It's only used as fallback when HtmlDialog is missing (SketchUp versions older to 2017). Exception: It's only used as fallback for opening Extension Warehouse if UI.show_extension_warehouse is missing (SketchUp versions older than 206.1). Rejection: WebDialog has been deprecated since SketchUp 2017. Use HtmlDialog instead, which works consistently across platforms and is actively maintained. Extension name suggests SketchUp is the author When the extension name starts with "SketchUp", e.g. "SketchUp MCP Server" or "SketchUp AI Render" Rejection: The name "SketchUp {product name}" suggests SketchUp is the author of this extension. Prefer "{product name} for SketchUp" or similar to avoid such confusion. File names suggest SketchUp is the author When the extension root file/support folder starts with "su_" or "sketchup_" Rejection: The prefix "su_" of the file names suggests SketchUp is the author. Please change to your name/your company name or remove the prefix. Silent Purge Unused If purge_unused is called without it being clearly initialized by the user Rejection: purge_unused removes assets the user may want to use later. Only call it with the user's knowledge, e.g. from a button that says "Purge Unused". Never do it as a hidden side effect. https://ruby.sketchup.com/file.extension_requirements.html#label-Data+Loss Silent Save If the active model is saved without it clearly being initialized by the user Rejection: Never save the model without the user's knowledge. Saving the model behind the user's back can lead to data loss if they've made some destructive changes they did not intend to save. Only save the model with the user's knowledge, e.g. from a button that says "Save Model". https://ruby.sketchup.com/file.extension_requirements.html#label-Data+Loss Notes These are recommendations. They do not block publishing. Using remote resource in file:// context Flag when a WebDialog or HtmlDialog running in a local file:// context loads any remote resource. This includes: JavaScript network calls: fetch , XMLHttpRequest , WebSocket , EventSource Remote resource tags in HTML: <script src="https://..."> , <link href="https://..."> , <img src="https://..."> The dialog runs in a file:// context if: the dialog uses set_html the dialog uses set_file Not a flag if: The dialog uses set_url pointing to a remote URL (the entire dialog is remote, not file://). Note: Making network requests from local HTML files may be blocked in the future due to security concerns. Prefer choosing if each HtmlDialog is either using local or remote content, not both. To access remote APIs in a local HTML page, prefer making the network calls from Ruby, not JS. Outdated copyright year Check copyright year, if any, on the SketchupExtension#copyright attribute. Ignore any code comments or file headers not visible to the end user. Note: Your copyright year is outdated. Dynamic copyright year If copyright year is Time.now.year Note: Prefer hardcoding the copyright year. Showing the current year can lead to confusion of what version the user is actually on. SketchUp version compatibility check in root rb If the sketchUpExtension setup is wrapped in a Sketchup.version check for compatibility. Note: Prefer checking the SketchUp version inside the main file, not the root file. If the extension is disabled, the message should not be shown. Reinventing Length handling Hardcoded unit conversions instead of using SketchUp's Length class. Also flag when the UI (dialogs, labels, input fields) is hardcoded to a single unit (e.g. all labels say "m" or "mm") rather than using Length#to_s and String#to_l to respect the model's unit setting. Exception: Ignore speeds (mph, km/h, m/s etc). SketchUp has no built in unit handling for this. Note: When working with lengths in SketchUp, prefer using the power of the SketchUp Length class. Rather than using integers or floats and hard coding for a specific unit, prefer using Length internally, and Length#to_s and String#to_l to convert to and from strings in the UI. This way the extension automatically supports all the SketchUp length units, and by default uses the unit of the open model. https://developer.sketchup.com/article-lengths-and-units Very short namespace (initials) Skip this note for known early developers who conventionally use initials (TT for ThomThom, AE for Aerelius, etc.).