Supriya dasari fetch security debugging assignments#523
Supriya dasari fetch security debugging assignments#523Supriyadasari04 wants to merge 2 commits intoaptyInc:mainfrom
Conversation
Summary of ChangesHello @Supriyadasari04, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new assignment, a simple e-commerce application called 'ShopEase'. The application showcases the practical implementation of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a simple e-commerce page to showcase fetching data, handling security, and debugging. The implementation is a good start, but there are several critical security vulnerabilities and areas for improvement in accessibility and maintainability. My review focuses on fixing a Cross-Site Scripting (XSS) vulnerability caused by using innerHTML with API data, improving accessibility by adding labels to form controls, and enhancing code maintainability by using CSS variables and modern HTML attributes. Please address the critical security issues before merging.
| const card = document.createElement("div"); | ||
| card.className = "product-card"; | ||
| card.innerHTML = ` | ||
| <img src="${p.thumbnail}" alt="${p.title}"> | ||
| <h3>${p.title}</h3> | ||
| <p>$${p.price}</p> | ||
| `; | ||
| card.addEventListener("click", () => showProduct(p.id)); | ||
| productList.appendChild(card); |
There was a problem hiding this comment.
Using innerHTML to inject HTML from API data is a major security risk and can lead to Cross-Site Scripting (XSS) attacks. If the API returns malicious data (e.g., a product title containing a <script> tag), it will be executed in the user's browser. You should create DOM elements programmatically and set their content using textContent to automatically sanitize the data.
const card = document.createElement("div");
card.className = "product-card";
const img = document.createElement("img");
img.src = p.thumbnail;
img.alt = p.title;
const title = document.createElement("h3");
title.textContent = p.title;
const price = document.createElement("p");
price.textContent = `$${p.price}`;
card.append(img, title, price);
card.addEventListener("click", () => showProduct(p.id));
productList.appendChild(card);| productDetails.innerHTML = ` | ||
| <h2>${product.title}</h2> | ||
| <img src="${product.thumbnail}" alt="${product.title}"> | ||
| <p>${product.description}</p> | ||
| <p><strong>Price:</strong> $${product.price}</p> | ||
| <p><strong>Brand:</strong> ${product.brand}</p> | ||
| <p><strong>Category:</strong> ${product.category}</p> | ||
| `; |
There was a problem hiding this comment.
Similar to the renderProducts function, using innerHTML here with data from the API response creates a serious Cross-Site Scripting (XSS) vulnerability. Any malicious HTML in the product data could be executed. Please refactor this to create DOM elements safely and use textContent and append to build the structure.
productDetails.innerHTML = '';
const titleEl = document.createElement('h2');
titleEl.textContent = product.title;
const imgEl = document.createElement('img');
imgEl.src = product.thumbnail;
imgEl.alt = product.title;
const descriptionEl = document.createElement('p');
descriptionEl.textContent = product.description;
const priceEl = document.createElement('p');
priceEl.innerHTML = `<strong>Price:</strong> `;
priceEl.append(`$${product.price}`);
const brandEl = document.createElement('p');
brandEl.innerHTML = `<strong>Brand:</strong> `;
brandEl.append(product.brand);
const categoryEl = document.createElement('p');
categoryEl.innerHTML = `<strong>Category:</strong> `;
categoryEl.append(product.category);
productDetails.append(titleEl, imgEl, descriptionEl, priceEl, brandEl, categoryEl);| const data = await res.json(); | ||
| return data.products; | ||
| } catch (err) { | ||
| productList.innerHTML = `<p class="error">${err.message}</p>`; |
There was a problem hiding this comment.
Using innerHTML to display error messages is a risky practice. While your current error messages are static strings, if an error message from an API response were ever displayed this way, it could lead to a Cross-Site Scripting (XSS) vulnerability. It's safer to use textContent to prevent the browser from interpreting the message as HTML. You can apply an error class to the productList element itself for styling. This feedback also applies to lines 27 and 37.
| productList.innerHTML = `<p class="error">${err.message}</p>`; | |
| productList.textContent = err.message; |
|
|
||
| async function searchProducts(query) { | ||
| try { | ||
| const res = await fetch(`https://dummyjson.com/products/search?q=${query}`); |
There was a problem hiding this comment.
The search query is being inserted directly into the fetch URL. User input should always be properly encoded when used in a URL to prevent issues like malformed URLs or potential injection attacks if the endpoint is vulnerable. Use encodeURIComponent() to safely encode the query parameter.
| const res = await fetch(`https://dummyjson.com/products/search?q=${query}`); | |
| const res = await fetch(`https://dummyjson.com/products/search?q=${encodeURIComponent(query)}`); |
| * { | ||
| box-sizing: border-box; | ||
| margin: 0; | ||
| padding: 0; | ||
| } | ||
|
|
||
| body { | ||
| font-family: "Segoe UI", Tahoma, Geneva, Verdana, sans-serif; | ||
| background: #f7f7f7; | ||
| color: #333; | ||
| line-height: 1.6; | ||
| } | ||
|
|
||
| header { | ||
| background: #4a90e2; | ||
| color: white; | ||
| padding: 15px 20px; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| } |
There was a problem hiding this comment.
To improve maintainability, it's best to declare colors as CSS variables. This makes it easy to manage your color palette from one place. I've started the conversion here, which you should continue for other hardcoded colors in this file (e.g., on lines 43, 51, 92, 117).
:root {
--primary-color: #4a90e2;
--background-color: #f7f7f7;
--text-color: #333;
--text-on-primary: white;
}
* {
box-sizing: border-box;
margin: 0;
padding: 0;
}
body {
font-family: "Segoe UI", Tahoma, Geneva, Verdana, sans-serif;
background: var(--background-color);
color: var(--text-color);
line-height: 1.6;
}
header {
background: var(--primary-color);
color: var(--text-on-primary);
padding: 15px 20px;
display: flex;
justify-content: space-between;
align-items: center;
}| padding: 8px; | ||
| border-radius: 4px; | ||
| border: none; | ||
| width: 200px; |
There was a problem hiding this comment.
The search input has a fixed width of 200px. This is not responsive and can cause layout issues on smaller screens. Since its parent .search-bar is a flex container, you can make the input flexible by setting flex: 1;. This will allow it to grow and shrink as needed.
flex: 1;| <header> | ||
| <h1>ShopEase</h1> | ||
| <div class="search-bar"> | ||
| <input type="text" id="search-input" placeholder="Search products..."> |
There was a problem hiding this comment.
This input field is missing a <label>, which is an accessibility issue. Screen readers use labels to describe form controls to users. While a placeholder is present, it's not a replacement for a proper label. You can add an aria-label attribute to provide an accessible name for the input.
| <input type="text" id="search-input" placeholder="Search products..."> | |
| <input type="text" id="search-input" placeholder="Search products..." aria-label="Search products"> |
| <body> | ||
| <details> | ||
| <summary>Assignment 1</summary> | ||
| <iframe src="./assignment-1/shop.html" frameborder="0" width="100%"></iframe> |
There was a problem hiding this comment.
The frameborder attribute is obsolete in HTML5 and should not be used. Styling of frames should be handled with CSS. Additionally, for accessibility, it's important to provide a title attribute for <iframe> elements to describe their content to screen reader users. I've also added a default height to ensure the content is visible.
| <iframe src="./assignment-1/shop.html" frameborder="0" width="100%"></iframe> | |
| <iframe src="./assignment-1/shop.html" style="border: none; width: 100%; height: 600px;" title="E-commerce shop assignment"></iframe> |
1 participant
Terms and Conditions
HTML Best Practices
File Naming Convention:
Follow consistent and descriptive naming (e.g., dashboard.html, user-profile.html).
Use lowercase letters and hyphens instead of spaces.
Page Title:
Ensure the <title> tag is descriptive and aligns with the page content.
Include meaningful keywords for SEO if applicable.
Semantic Markup:
Use appropriate tags like <header>, <footer>, <section>, <article> for better readability and accessibility.
Accessibility Standards:
Ensure the use of alt attributes for images and proper labels for form elements.
Use ARIA roles where necessary.
Validation:
Ensure the code passes HTML validation tools without errors or warnings.
Structure and Indentation:
Maintain consistent indentation and proper nesting of tags.
Attributes:
Ensure all required attributes (e.g., src, href, type, etc.) are correctly used and not left empty.
CSS Best Practices
File Organization:
Use modular CSS files if applicable (e.g., base.css, layout.css, theme.css).
Avoid inline styles unless absolutely necessary.
Naming Conventions:
Use meaningful class names following BEM or other conventions (e.g., block__element--modifier).
Code Reusability:
Avoid duplicate code; use classes or mixins for shared styles.
Responsive Design:
Ensure proper usage of media queries for mobile, tablet, and desktop views.
Performance Optimization:
Minimize the use of unnecessary CSS selectors.
Avoid overly specific selectors and ensure selectors are not overly deep (e.g., avoid #id .class1 .class2 p).
Consistency:
Follow consistent spacing, indentation, and use of units (rem/em vs. px).
Maintain a single coding style (e.g., always use double or single quotes consistently).
Javascript Best Practices
File Organization:
Ensure scripts are modular and logically separated into files if needed.
Avoid mixing inline JavaScript with HTML.
Logic Optimization:
Check for redundancy and ensure the code is optimized for performance.
Avoid unnecessary API calls or DOM manipulations.
Solution Approach:
Confirm that the code solves the given problem efficiently.
Consider scalability for future enhancements.
Readability:
Use clear variable and function names.
Add comments for complex logic or algorithms.
Error Handling:
Ensure proper error handling for API calls or user input validation.
Code Quality:
Check for potential bugs (e.g., missing await, mishandling of null/undefined values).
Avoid unnecessary console.log statements in production code.
Security:
Avoid hardcoding sensitive data.
Sanitize user input to prevent XSS and other vulnerabilities.
Best Practices:
Use const and let instead of var.
Follow ES6+ standards where applicable.