Security, Code Quality & Performance Audit of OpenMCT Using Claude Code (AI-Assisted Review) #8300
swenger287
started this conversation in
Show and tell
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
AI-Assisted Security & Code Quality Audit of OpenMCT
I recently used [[Claude Code](https://claude.ai/code)](https://claude.ai/code) (Anthropic's CLI-based AI coding agent) to perform a comprehensive audit of the OpenMCT codebase covering security vulnerabilities, code quality issues, and performance concerns. I wanted to share both the methodology and findings with the community.How I Did It
Tool: Claude Code (powered by Claude Opus 4.6), running locally on my machine.
Methodology:
https://github.com/nasa/openmctv-html,innerHTML,eval), prototype pollution, injection risks, unsafeJSON.parse, CORS misconfigurations, and unvalidated message passingThe entire process took roughly 10 minutes from prompt to final report. I initially prompted it as a C++ review (my mistake — I was confusing it with another NASA repo), and Claude Code corrected me that OpenMCT is JavaScript/Vue.js before proceeding with the appropriate analysis.
Security Findings
S1. XSS via
v-htmlin TextHighlight Component — HighFile:
src/utils/textHighlight/TextHighlight.vue:48-59Both
this.textandthis.highlightare interpolated directly into raw HTML with no escaping. If search text or content names contain HTML characters like<img onerror=...>, the markup is injected as-is. The regex on line 55 also constructs a pattern from unescaped user input, which can break the regex or enable injection.Current code:
Recommended fix — escape HTML entities before interpolation and use a proper regex-escape utility:
S2. CouchDB CORS Wildcard Origin with Credentials — High
File:
src/plugins/persistence/couch/setup-couchdb.sh:59-60The setup script sets
origins: *ANDcredentials: true. Per the Fetch specification,Access-Control-Allow-Origin: *withAccess-Control-Allow-Credentials: trueis forbidden by browsers, but some CouchDB versions may serve it in a way that older or misconfigured clients accept. More importantly, if an operator changes this to a broad pattern later, thecredentials: trueis already in place.Current code:
Recommended fix — use an environment variable for the allowed origin and default to a safe value:
S3. Unprotected
JSON.parseon External Data — HighMultiple locations parse external data (SSE events, drag/drop payloads, localStorage) without try/catch. A malformed payload will throw an unhandled exception and crash the component or worker.
CouchChangesFeed.jsevent.dataCouchObjectProvider.jsevent.dataDisplayLayout.vuedataTransferNotebookEntry.vuedataTransferBrowse.jswindow.localStoragenotebook-storage.jswindow.localStorageExample — current code in
CouchChangesFeed.js:59-68:Recommended fix:
Same pattern should be applied to all 6 locations listed above. For
DisplayLayout.vue:437andNotebookEntry.vue:569, wrap theJSON.parsein try/catch and show anopenmct.notifications.error()on failure.S4. Incomplete Prototype Pollution Defense — Medium
File:
src/utils/sanitization.jsThe
filter__proto__reviver correctly strips__proto__keys duringJSON.parse, but it is only used in 3 of 60+JSON.parsecall sites (specifically:ImportFromJSONAction.js:77,LocalStorageObjectProvider.js:92).Current usage is sparse:
Recommended fix — create a safe parse utility and use it project-wide:
Then adopt
safeParseinCouchChangesFeed.js:63,CouchObjectProvider.js:663,DisplayLayout.vue:437,NotebookEntry.vue:569,notebook-storage.js:56,Browse.js:158, and other locations that parse external input.S5.
mathjsevaluate()on User Expressions — MediumFile:
src/plugins/comps/CompsMathWorker.js:125User-defined mathematical expressions are passed directly to
mathjs.evaluate(). Whilemathjsprovides a sandboxed parser, it has had historical sandbox-escape CVEs. Theexpressionis user-authored configuration data.Current code:
Recommended fix — use
math.compile()+ restrict to a limited function set:S6. No Origin Validation on SharedWorker Messages — Medium
Files:
InMemorySearchWorker.js:41,CouchChangesFeed.js:14,CompsMathWorker.js:7SharedWorkers accept messages from any page on the same origin. Any tab on the same domain (e.g., a compromised or malicious page on the same host) can send commands to these workers.
Current code (
InMemorySearchWorker.js):Recommended fix — validate message structure with a type guard:
S1a. Notebook Markdown Rendering — Mitigated (Informational)
Upon deeper inspection,
NotebookEntry.vue:446-448does passmarkedoutput throughsanitize-htmlwith a strict allowlist (SANITIZATION_SCHEMAat line 165). This properly mitigates XSS from markdown. The initial scan flagged theinnerHTMLusage but the sanitization pipeline is present. Credit to the team for this defense.However, the
eslint-disable vue/no-v-htmlat line 1 suppresses the linter for the entire file. Consider scoping it to only the specific line that needs it.Code Quality Findings
Q1. Monolithic Components — High
src/plugins/plot/MctPlot.vuesrc/plugins/imagery/components/ImageryView.vueRecommended fix — extract into Vue composables:
Q2. Tight Coupling via
$parent.$refs— HighFile:
src/plugins/plot/MctPlot.vue— lines 639, 645, 650, 661, 683, 979, 19067+ references to
this.$parent.$refs.plotWrapper, creating fragile coupling to parent internals.Current code (line 639 and similar):
Recommended fix — use provide/inject:
Q3. Memory Leak — Anonymous Event Listener in ImageryTimeView — High
File:
src/plugins/imagery/components/ImageryTimeView.vue:415Anonymous arrow function passed to
addEventListener— can never be removed. These wrappers are created dynamically per image.Current code:
Recommended fix — use event delegation on the parent container instead of per-element listeners:
When building image wrappers, add
data-timeattribute:Q4. Memory Leak — FileInput Missing Cleanup — High
File:
src/api/forms/components/controls/FileInput.vue:79-81addEventListenerinmounted()with no correspondingunmounted()hook.Current code:
Recommended fix:
Q5. Memory Leak — NotebookSnapshotIndicator Missing Cleanup — High
File:
src/plugins/notebook/components/NotebookSnapshotIndicator.vue:69-76Event subscription and
setTimeoutcreated inmounted()with no cleanup on unmount.Current code:
Recommended fix:
Q6. Unbounded Recursive
setTimeoutin MctTree — HighFile:
src/ui/layout/MctTree.vue:958-994calculateHeights()returns a Promise that recursively callssetTimeout(checkHeights, 100)until DOM conditions are met. There is no maximum retry count, no reject path, and no component unmount guard. If the component is destroyed while polling, this loops forever and the promise never settles.Current code:
Recommended fix:
Q7. Promise Constructor Anti-Patterns — Medium
File:
src/api/objects/Transaction.js:78-89Wraps
action()(which already returns a Promise) insidenew Promise()— the classic unnecessary Promise constructor.Current code:
Recommended fix:
File:
src/api/Editor.js:69-87— same pattern.Current code:
Recommended fix:
Q8. Swallowed Errors — Medium
File:
src/plugins/persistence/couch/CouchObjectProvider.js:579File:
src/plugins/inspectorViews/styles/SavedStyleSelector.vue:141Recommended fix for
CouchObjectProvider.js:Recommended fix for
SavedStyleSelector.vue— distinguish cancellation from error:Q9. Missing Test Coverage for Critical Plugins — Medium
src/plugins/comps/src/plugins/events/Recommended: Add at minimum unit tests for
CompsMathWorker.js(expression evaluation),CompsTelemetryProvider.js(subscription lifecycle), and the events plugin rendering logic.Q10. Legacy SummaryWidget Architecture — Medium
Directory:
src/plugins/summaryWidget/src/The entire plugin uses pre-ES6 patterns: constructor functions with
.prototypemethods, manual DOM manipulation,const self = this. Architecturally inconsistent with the rest of the Vue 3 / ES6+ codebase.Example from
Condition.js:197:Recommended: Refactor incrementally to ES6 classes and Vue components when touching this module, consistent with the rest of the codebase.
Performance Findings
P1. No Virtual Scrolling for Telemetry Tables — High
File:
src/plugins/telemetryTable/collections/TableRowCollection.js:176-178Grep for "virtual scroll" returns zero results. Telemetry table rows are pushed into arrays and all visible rows are rendered to the DOM. Under high data rates (thousands of parameters/sec), this causes severe DOM thrashing.
Recommended fix — implement a virtual scroller that only renders visible rows:
P2.
setIntervalPolling Instead of ResizeObserver — MediumFile:
src/plugins/telemetryTable/components/SizingRow.vue:61Polls element height every 300ms via
setInterval. Similar pattern inMctPlot.vue:681.Current code:
Recommended fix:
P3.
JSON.parse(JSON.stringify())Deep Cloning — Medium60+ occurrences across the codebase use this pattern for deep cloning. It blocks the main thread, drops
undefinedvalues, and cannot cloneMap,Set,Date, orRegExp. The codebase even acknowledges this atInMemorySearchProvider.js:290.Example (
MutableDomainObject.js:77):Recommended fix — replace with
structuredClone()(supported in all target browsers):Apply project-wide via search-and-replace. For cases where
structuredClonedoesn't work (e.g., objects with functions), keepJSON.parse(JSON.stringify())but add a comment explaining why.P4. Full Lodash Import — Low
Files:
src/plugins/telemetryTable/TelemetryTable.js:24,src/plugins/notebook/components/NotebookEntry.vue:152, and others.Recommended fix:
The project already has
eslint-plugin-you-dont-need-lodash-underscoreconfigured — consider promoting its rules to errors.Positive Finding: BatchingWebSocket
File:
src/api/telemetry/BatchingWebSocket.jsThis is a well-architected component. WebSocket connection runs in a dedicated Worker, uses
requestIdleCallbackfor back-pressure, has configurable buffer limits (setMaxBufferSize), throttle rate control, and user notification when telemetry is dropped. TheVisibilityObserverutility (src/utils/visibility/VisibilityObserver.js) also intelligently pausesrequestAnimationFramewhen elements are not visible. These are good patterns that other parts of the codebase could learn from.Summary
JSON.parseon external data (6+ locations)mathjs.evaluate()on user expressions$parent.$refstight coupling (7+ locations)removeEventListeneron unmountsetTimeoutin MctTreesetIntervalpolling vsResizeObserverJSON.parse(JSON.stringify())instead ofstructuredCloneCaveats
sanitize-htmlwith an allowlist schema (S1a). The AI corrected itself after tracing the full data flow.mathjsfinding (S5) depends on whether the deployed version has known sandbox-escape CVEs — the library's sandboxing may be sufficient for this use case.Why Share This
OpenMCT is critical infrastructure used in real mission operations. AI-assisted code review isn't a replacement for human security review, but it can surface issues quickly and at scale. I hope the maintainers and community find this useful as a starting point for further investigation.
All reactions