feat: Introduce a calculator UI application, integrating it as a serv…#791
feat: Introduce a calculator UI application, integrating it as a serv…#791sugoi-yuzuru wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new calculator UI application and integrates it as a server resource. However, it contains critical security vulnerabilities, particularly in calculator.html, related to cross-window communication. Specifically, the use of wildcard origins in postMessage and CORS policies, along with missing origin validation in message listeners, could lead to data leakage or spoofing attacks. These issues must be addressed by restricting origins to trusted values. Additionally, a bug in calculation logging, a missing edge case for division by zero, and opportunities for improving JavaScript code robustness and maintainability have been identified.
| } | ||
|
|
||
| window.addEventListener('message', (event) => { | ||
| const data = event.data; |
There was a problem hiding this comment.
For security, you must verify the message's origin to ensure it's coming from a trusted source. Without this check, your application is vulnerable to attacks from malicious sites. Please add a check for event.origin at the beginning of this event listener. The placeholder origin in the suggestion should be replaced with the actual origin of the parent application.
| const data = event.data; | |
| if (event.origin !== 'https://your-parent-origin.com') return; | |
| const data = event.data; |
|
|
||
| function sendRequest(method, params) { | ||
| const id = nextId++; | ||
| window.parent.postMessage({ jsonrpc: "2.0", id, method, params }, '*'); |
There was a problem hiding this comment.
The use of window.parent.postMessage with a wildcard target origin ('*') is a significant security risk. This allows any page embedding the application in an iframe to receive potentially sensitive messages, leading to data leakage or exploitation. Always specify the exact origin of the parent window instead of '*'. This issue also applies to lines 203 and 214.
| window.parent.postMessage({ jsonrpc: "2.0", id, method, params }, '*'); | |
| window.parent.postMessage({ jsonrpc: "2.0", id, method, params }, 'https://your-parent-origin.com'); |
| const id = nextId++; | ||
| window.parent.postMessage({ jsonrpc: "2.0", id, method, params }, '*'); | ||
| return new Promise((resolve, reject) => { | ||
| const listener = (event) => { |
There was a problem hiding this comment.
The application listens for message events without validating the origin property of the event. This is a security vulnerability as it allows any window or iframe to send messages, potentially leading to spoofing or data manipulation. Always check event.origin and ensure it matches the expected host. Additionally, the sendRequest function adds a 'message' event listener for each request, which could lead to memory leaks if responses don't arrive. Consider implementing a timeout to remove listeners and reject promises.
| <button class="clear" onclick="clearDisplay()">AC</button> | ||
| <button class="operator" onclick="appendOperator('/')">÷</button> | ||
| <button class="operator" onclick="appendOperator('*')">×</button> | ||
| <button onclick="appendNumber('7')">7</button> | ||
| <button onclick="appendNumber('8')">8</button> | ||
| <button onclick="appendNumber('9')">9</button> | ||
| <button class="operator" onclick="appendOperator('-')">-</button> | ||
| <button onclick="appendNumber('4')">4</button> | ||
| <button onclick="appendNumber('5')">5</button> | ||
| <button onclick="appendNumber('6')">6</button> | ||
| <button class="operator" onclick="appendOperator('+')">+</button> | ||
| <button onclick="appendNumber('1')">1</button> | ||
| <button onclick="appendNumber('2')">2</button> | ||
| <button onclick="appendNumber('3')">3</button> | ||
| <button class="equals" onclick="calculate()">=</button> | ||
| <button onclick="appendNumber('0')">0</button> | ||
| <button onclick="appendPoint()">.</button> |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
1682a38 to
08700af
Compare
sugoi-yuzuru
left a comment
There was a problem hiding this comment.
fixed a few feedback
jgindin
left a comment
There was a problem hiding this comment.
Should we push this sample down to, e.g., samples/mcp/calculator to make it easier if we have multiple mcp examples?
| window.addEventListener('message', (event) => { | ||
| const data = event.data; | ||
| if (data?.method === 'ping') { | ||
| // Respond to ping if needed, or just ignore as it might be handled by transport |
There was a problem hiding this comment.
Do we need all of this comment? Or maybe the code is self-explanatory?
There was a problem hiding this comment.
You are correct. I have removed this comment, but added a new one to clarify the use of this listner in the context of MCP Apps.
67ba136 to
317a676
Compare
I have pushed it under |
…er resource and tool, and updating related dependencies. This calculator MCP App is a precursor to a future sample application that intends to integrate MCP Apps into A2UI. A2UI Custom component that will allow MCP Apps rendering will be a following up. https://screenshot.googleplex.com/3tQRreZ5eQE9qsQ is how the calculator looks.
317a676 to
338f424
Compare
feat: Introduce a calculator UI application, integrating it as a server resource and tool, and updating related dependencies.
This calculator MCP App is a precursor to a future sample application that intends to integrate MCP Apps into A2UI. A2UI Custom component that will allow MCP Apps rendering will be a following up.
https://screenshot.googleplex.com/3tQRreZ5eQE9qsQ is how the calculator looks.
List which issues are fixed by this PR. For larger changes, raising an issue first helps reduce redundant work.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.