Conversation
|
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| <tr> | ||
| <td><code>${p.sku}</code></td> | ||
| <td>${p.name || '—'}</td> | ||
| <td><code>${p.path || '—'}</code></td> |
There was a problem hiding this comment.
BLOCKING - XSS: Unescaped API data in renderTable
p.name, p.sku, p.path, and data-path attributes are interpolated directly into innerHTML without HTML escaping. A product with name <img src=x onerror=alert(1)> executes JavaScript when any admin views this page.
The same issue exists in:
customers.jslines 38-45:c.email,c.firstName,c.lastName,c.phone,data-emailorders.jslines 37-45:o.id,o.state,o.customer?.emailindices.jslines 37-41:idx.path,data-path
Note that journals.js and service-tokens.js in this PR already define and use escapeHtml correctly. Move escapeHtml into ui.js and import + apply it everywhere in the four vulnerable files.
See AGENTS.md - Security: No XSS vulnerabilities (sanitize user input)
| <div class="form-field"> | ||
| <label for="description">Description</label> | ||
| <textarea id="description" name="description" rows="3">${p.description || ''}</textarea> | ||
| </div> |
There was a problem hiding this comment.
BLOCKING - XSS: Unescaped p.description in textarea
<textarea> content is RCDATA - the closing tag </textarea> always ends it. A description containing </textarea><img src=x onerror=alert(1)> breaks out and executes when this modal renders via innerHTML.
Fix: apply escapeHtml(p.description || '') before interpolation. The same applies to any other raw string fields in this template.
| <button type="button" class="active" data-view="json">JSON</button> | ||
| </div> | ||
| <textarea class="json-editor" id="json-editor">${JSON.stringify(jsonData, null, 2)}</textarea> | ||
| `; |
There was a problem hiding this comment.
BLOCKING - XSS: JSON.stringify output is not HTML-safe
JSON.stringify escapes " and \\ but not < or >. If any product string field (e.g. description, name) contains </textarea><script>alert(1)</script>, it terminates the textarea and executes when assigned via innerHTML.
Fix: HTML-escape the serialized JSON before insertion. Use escapeHtml(JSON.stringify(jsonData, null, 2)) once escapeHtml is exported from ui.js.
| <div class="form-field"> | ||
| <label for="connect-org">Organization</label> | ||
| <input type="text" id="connect-org" name="org" required placeholder="my-org" autocomplete="off" value="${org || ''}"> | ||
| </div> |
There was a problem hiding this comment.
BLOCKING - XSS: URL params org/site injected raw into innerHTML
org and site are read from URLSearchParams (fully attacker-controllable via a crafted link) and interpolated without escaping into value attributes and nav href strings inside sidebar.innerHTML. A URL such as ?org=%22%3E%3Cimg+src%3Dx+onerror%3Dalert(1)%3E causes reflected XSS on page load.
auth.email at diff position 139 also flows through innerHTML without escaping.
Fix: apply escapeHtml to org, site, and auth.email before interpolating into the sidebar template; use encodeURIComponent for org/site in the href attribute.
| skipAuthRedirect: true, | ||
| }); | ||
| if (!resp.ok) { | ||
| showToast(await readError(resp), 'error'); |
There was a problem hiding this comment.
SHOULD FIX - Accessibility: <label> not associated with its <input>
The Email label at the OTP step has no for attribute and the input has no id, so they are not programmatically linked. Screen readers won't announce the label when the input is focused (WCAG 2.1 AA, SC 1.3.1).
| showToast(await readError(resp), 'error'); | |
| <label for="otp-email">Email</label> | |
| <input type="text" id="otp-email" value="${email}" disabled> |
|
|
||
| /* Tablet and up */ | ||
| @media (width >= 600px) { | ||
| .productbus-admin #app-container { |
There was a problem hiding this comment.
SHOULD FIX - CSS: Missing 900px and 1200px breakpoints
The stylesheet has only a single @media (width >= 600px) block. The two-column sidebar layout adapts at 600px but never adjusts for wider viewports.
Per AGENTS.md - CSS: Declare styles mobile first, use range-notation media queries at 600px/900px/1200px for tablet and desktop.
Add @media (width >= 900px) and @media (width >= 1200px) blocks with appropriate layout adjustments.
| ...fetchOptions, | ||
| headers, | ||
| credentials: 'include', | ||
| }); |
There was a problem hiding this comment.
CONSIDER - Security: Since auth is carried exclusively via Authorization: Bearer, credentials: 'include' is not needed and unnecessarily sends ambient cookies for api.adobecommerce.live on every cross-origin request.
| }); | |
| credentials: 'same-origin', |
| <script nonce="aem" src="/scripts/scripts.js" type="module"></script> | ||
| <link rel="stylesheet" href="/styles/styles.css" /> | ||
| <link rel="stylesheet" href="/utils/card-ui/card-ui.css" /> | ||
| <script nonce="aem" src="/tools/productbus-admin/productbus-admin.js" type="module"></script> |
There was a problem hiding this comment.
SHOULD FIX - Unused import: No element in this tool uses any .card-ui CSS classes. This looks like dead weight copied from another tool's template. Remove it to avoid loading an unnecessary stylesheet.
| <script nonce="aem" src="/tools/productbus-admin/productbus-admin.js" type="module"></script> |
|
Code Review - PR 303 (productbus admin tool) BLOCKING: XSS via server error messages. err.message is sourced from the x-error response header in api.js and interpolated directly into innerHTML without escaping in 7 files: admins.js, catalog.js, config.js, customers.js, indices.js, orders.js, productbus-admin.js. A malicious API server can inject arbitrary HTML/JS into the admin session. Fix: wrap with escapeHtml(err.message) at each site -- escapeHtml is already imported in every affected file. See inline suggestions on representative files. SHOULD FIX:
CONSIDER:
VERDICT: REQUEST CHANGES -- the XSS vulnerability is blocking. One-click GitHub Suggestions are available in Files Changed for the XSS fix, unused import, inline style, and credentials issues. |
claude
Bot
dismissed
their stale review
Superseded by review with inline comments — please disregard.
| openAddModal(ctx, () => render(container, ctx)); | ||
| }); | ||
| } catch (err) { | ||
| container.querySelector('#admins-table').innerHTML = `<p class="error">Failed to load admins: ${err.message}</p>`; |
There was a problem hiding this comment.
BLOCKING — XSS: err.message comes from the server-controlled x-error response header (api.js L67-70) and is interpolated into innerHTML without escaping. escapeHtml() is already imported in this file.
| container.querySelector('#admins-table').innerHTML = `<p class="error">Failed to load admins: ${err.message}</p>`; | |
| container.querySelector('#admins-table').innerHTML = `<p class="error">Failed to load admins: ${escapeHtml(err.message)}</p>`; |
| openProductModal(ctx, null, () => render(container, ctx)); | ||
| }); | ||
| } catch (err) { | ||
| container.querySelector('#catalog-table').innerHTML = `<p class="error">Failed to load catalog: ${err.message}</p>`; |
There was a problem hiding this comment.
BLOCKING — XSS: err.message comes from the server-controlled x-error response header (api.js L67-70) and is interpolated into innerHTML without escaping. escapeHtml() is already imported in this file.
| container.querySelector('#catalog-table').innerHTML = `<p class="error">Failed to load catalog: ${err.message}</p>`; | |
| container.querySelector('#catalog-table').innerHTML = `<p class="error">Failed to load catalog: ${escapeHtml(err.message)}</p>`; |
| openCreateModal(ctx, () => render(container, ctx)); | ||
| }); | ||
| } catch (err) { | ||
| container.querySelector('#customers-table').innerHTML = `<p class="error">Failed to load customers: ${err.message}</p>`; |
There was a problem hiding this comment.
BLOCKING — XSS: err.message comes from the server-controlled x-error response header (api.js L67-70) and is interpolated into innerHTML without escaping. escapeHtml() is already imported in this file.
| container.querySelector('#customers-table').innerHTML = `<p class="error">Failed to load customers: ${err.message}</p>`; | |
| container.querySelector('#customers-table').innerHTML = `<p class="error">Failed to load customers: ${escapeHtml(err.message)}</p>`; |
| openCreateModal(ctx, () => render(container, ctx)); | ||
| }); | ||
| } catch (err) { | ||
| container.querySelector('#indices-table').innerHTML = `<p class="error">Failed to load indices: ${err.message}</p>`; |
There was a problem hiding this comment.
BLOCKING — XSS: err.message comes from the server-controlled x-error response header (api.js L67-70) and is interpolated into innerHTML without escaping. escapeHtml() is already imported in this file.
| container.querySelector('#indices-table').innerHTML = `<p class="error">Failed to load indices: ${err.message}</p>`; | |
| container.querySelector('#indices-table').innerHTML = `<p class="error">Failed to load indices: ${escapeHtml(err.message)}</p>`; |
| openCreateModal(ctx, () => render(container, ctx)); | ||
| }); | ||
| } catch (err) { | ||
| container.querySelector('#orders-table').innerHTML = `<p class="error">Failed to load orders: ${err.message}</p>`; |
There was a problem hiding this comment.
BLOCKING — XSS: err.message comes from the server-controlled x-error response header (api.js L67-70) and is interpolated into innerHTML without escaping. escapeHtml() is already imported in this file.
| container.querySelector('#orders-table').innerHTML = `<p class="error">Failed to load orders: ${err.message}</p>`; | |
| container.querySelector('#orders-table').innerHTML = `<p class="error">Failed to load orders: ${escapeHtml(err.message)}</p>`; |
| currentModule = await loader(); | ||
| await currentModule.render(mainContent, { org, site }); | ||
| } catch (err) { | ||
| mainContent.innerHTML = `<p class="error">Failed to load page: ${err.message}</p>`; |
There was a problem hiding this comment.
BLOCKING — XSS: err.message comes from the server-controlled x-error response header (api.js L67-70) and is interpolated into innerHTML without escaping. escapeHtml() is already imported in this file.
| mainContent.innerHTML = `<p class="error">Failed to load page: ${err.message}</p>`; | |
| mainContent.innerHTML = `<p class="error">Failed to load page: ${escapeHtml(err.message)}</p>`; |
| // Config may not exist yet | ||
| if (!err.message.includes('Forbidden')) { | ||
| config = {}; | ||
| } else { | ||
| container.querySelector('#config-content').innerHTML = `<p class="error">Failed to load config: ${err.message}</p>`; | ||
| return; | ||
| } |
There was a problem hiding this comment.
SHOULD FIX — Fragile 403 detection + XSS: err.message.includes('Forbidden') breaks when the server sends a custom x-error header on 403 (e.g. "Admin role required"), causing the code to fall through to config = {} and render an editable form — the user could then POST an empty config accidentally.
Note: 404 never reaches this catch because apiFetch does not throw on 404; the if (resp.ok) check on line 72 already handles that case. The replacement also fixes the unescaped err.message in innerHTML.
| // Config may not exist yet | |
| if (!err.message.includes('Forbidden')) { | |
| config = {}; | |
| } else { | |
| container.querySelector('#config-content').innerHTML = `<p class="error">Failed to load config: ${err.message}</p>`; | |
| return; | |
| } | |
| container.querySelector('#config-content').innerHTML = `<p class="error">Failed to load config: ${escapeHtml(err.message)}</p>`; | |
| return; |
| }); | ||
| if (!ok) return; | ||
| try { | ||
| await apiFetch(ctx.org, ctx.site, `auth/admins/${encodeURIComponent(email)}`, { method: 'DELETE' }); |
There was a problem hiding this comment.
SHOULD FIX — Silent success on HTTP errors: apiFetch only throws on HTTP 401 and 403. For any other non-2xx response (400, 404, 500...), it returns a resolved promise. Without a resp.ok guard, showToast('Admin removed') runs unconditionally — the UI shows success even when the server rejected the operation.
This same pattern repeats across all write operations in admins.js, catalog.js, customers.js, indices.js, orders.js, config.js, and service-tokens.js. Each call site needs:
const resp = await apiFetch(ctx.org, ctx.site, `auth/admins/${encodeURIComponent(email)}`, { method: 'DELETE' });
if (!resp.ok) throw new Error(resp.headers.get('x-error') || `HTTP ${resp.status}`);
showToast('Admin removed');
| const p = new URLSearchParams(); | ||
| p.set('org', org); | ||
| p.set('site', site); | ||
| p.set('page', 'orders'); |
There was a problem hiding this comment.
SHOULD FIX — Dead redirect parameter: api.js stores the originating URL in ?redirect=... when bouncing to login on 401 (api.js L57-62), but after OTP verification this code always navigates to page=orders — the redirect param is never read. Users deep-linked to ?page=catalog or ?page=journals will always land on Orders after logging in.
Fix by reading the param after successful auth (replace lines 134–138):
const redirectHref = new URLSearchParams(window.location.search).get('redirect');
if (redirectHref) {
try {
navigate(new URL(redirectHref).search.slice(1));
} catch {
navigate(`org=${encodeURIComponent(org)}&site=${encodeURIComponent(site)}&page=orders`);
}
} else {
const p = new URLSearchParams();
p.set('org', org);
p.set('site', site);
p.set('page', 'orders');
navigate(p.toString());
}
| <h1>Admins</h1> | ||
| </div> | ||
| <div class="page-actions"> | ||
| <input type="text" class="search-input" placeholder="Search admins..." id="search-admins" value="${escapeHtml(initialQ)}"> |
There was a problem hiding this comment.
SHOULD FIX — Accessibility (WCAG 2.1 AA / SC 4.1.2): Placeholder text alone does not provide a programmatically determinable accessible name. Per AGENTS.md: "Ensure accessibility standards (ARIA labels)" and "Follow WCAG 2.1 AA guidelines". journals.js uses <label for="..."> correctly and is the reference pattern.
| <input type="text" class="search-input" placeholder="Search admins..." id="search-admins" value="${escapeHtml(initialQ)}"> | |
| <input type="text" class="search-input" placeholder="Search admins..." id="search-admins" value="${escapeHtml(initialQ)}" aria-label="Search admins"> |
| <h1>Catalog</h1> | ||
| </div> | ||
| <div class="page-actions"> | ||
| <input type="text" class="search-input" placeholder="Search products..." id="search-catalog" value="${escapeHtml(initialQ)}"> |
There was a problem hiding this comment.
SHOULD FIX — Accessibility (WCAG 2.1 AA / SC 4.1.2): Placeholder text alone does not provide a programmatically determinable accessible name. Per AGENTS.md: "Ensure accessibility standards (ARIA labels)" and "Follow WCAG 2.1 AA guidelines". journals.js uses <label for="..."> correctly and is the reference pattern.
| <input type="text" class="search-input" placeholder="Search products..." id="search-catalog" value="${escapeHtml(initialQ)}"> | |
| <input type="text" class="search-input" placeholder="Search products..." id="search-catalog" value="${escapeHtml(initialQ)}" aria-label="Search products"> |
| <h1>Customers</h1> | ||
| </div> | ||
| <div class="page-actions"> | ||
| <input type="text" class="search-input" placeholder="Search customers..." id="search-customers" value="${escapeHtml(initialQ)}"> |
There was a problem hiding this comment.
SHOULD FIX — Accessibility (WCAG 2.1 AA / SC 4.1.2): Placeholder text alone does not provide a programmatically determinable accessible name. Per AGENTS.md: "Ensure accessibility standards (ARIA labels)" and "Follow WCAG 2.1 AA guidelines". journals.js uses <label for="..."> correctly and is the reference pattern.
| <input type="text" class="search-input" placeholder="Search customers..." id="search-customers" value="${escapeHtml(initialQ)}"> | |
| <input type="text" class="search-input" placeholder="Search customers..." id="search-customers" value="${escapeHtml(initialQ)}" aria-label="Search customers"> |
| <h1>Indices</h1> | ||
| </div> | ||
| <div class="page-actions"> | ||
| <input type="text" class="search-input" placeholder="Search indices..." id="search-indices" value="${escapeHtml(initialQ)}"> |
There was a problem hiding this comment.
SHOULD FIX — Accessibility (WCAG 2.1 AA / SC 4.1.2): Placeholder text alone does not provide a programmatically determinable accessible name. Per AGENTS.md: "Ensure accessibility standards (ARIA labels)" and "Follow WCAG 2.1 AA guidelines". journals.js uses <label for="..."> correctly and is the reference pattern.
| <input type="text" class="search-input" placeholder="Search indices..." id="search-indices" value="${escapeHtml(initialQ)}"> | |
| <input type="text" class="search-input" placeholder="Search indices..." id="search-indices" value="${escapeHtml(initialQ)}" aria-label="Search indices"> |
| <h1>Orders</h1> | ||
| </div> | ||
| <div class="page-actions"> | ||
| <input type="text" class="search-input" placeholder="Search orders..." id="search-orders" value="${escapeHtml(initialQ)}"> |
There was a problem hiding this comment.
SHOULD FIX — Accessibility (WCAG 2.1 AA / SC 4.1.2): Placeholder text alone does not provide a programmatically determinable accessible name. Per AGENTS.md: "Ensure accessibility standards (ARIA labels)" and "Follow WCAG 2.1 AA guidelines". journals.js uses <label for="..."> correctly and is the reference pattern.
| <input type="text" class="search-input" placeholder="Search orders..." id="search-orders" value="${escapeHtml(initialQ)}"> | |
| <input type="text" class="search-input" placeholder="Search orders..." id="search-orders" value="${escapeHtml(initialQ)}" aria-label="Search orders"> |
|
Code Review BLOCKING:
SHOULD FIX:
GitHub Suggestions have been added for all fixable issues (Files changed tab, Commit suggestion). Verdict: REQUEST CHANGES |
1 participant
Test URLs: