AOSSIE-Org / AOSSIE-Org/EduAid

[Bug/Refactor] Fix Unhandled Promises, Weak Cryptography (Math.random), and Code Quality Issues

Aperta
#535 1 commento 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
JavaScript
Stelle
171
Fork
425
Metriche di merge delle PR
Nessuna PR unita negli ultimi 30g

Descrizione

## Description
During a recent static code analysis of the `eduaid_web` codebase (using SonarQube and ESLint), several bugs, security warnings, and code quality issues were identified. Addressing these will prevent silent runtime failures, improve security practices, and clean up the production build.

To keep things actionable, I have grouped the top findings into three main categories:

### 1. Unhandled Promise Rejections (Reliability - High)
Promises without proper error handling can cause silent failures and unhandled rejection errors in the browser.
* **`src/pages/Home.jsx` (Line 34):** The `fetchGitHubStars()` function intentionally throws an error if the network request fails (`if (!response.ok) throw new Error(...)`), but the `.then()` chain calling it lacks a `.catch()` block. If the API rate-limits the user, it will cause an unhandled promise rejection.
* **`src/reportWebVitals.js` (Line 3):** The dynamic import `import('web-vitals').then(...)` lacks a `.catch()` block to handle import failures gracefully.

### 2. Weak Cryptography / Insecure Randomness (Security - Medium)
The application relies on `Math.random()` for shuffling arrays. While this may not be a critical vulnerability for standard UI elements, using `Math.random()` is flagged as a Cryptographic Failure (CWE-338) and can lead to highly predictable token/shuffle generation.
* **`src/pages/Output.jsx` (Line 35):** `const j = Math.floor(Math.random() * (i + 1));`
* **`src/workers/pdfWorker.js` (Line 5):** `const j = Math.floor(Math.random() * (i + 1));`

### 3. Leftover Production Console Logs & Dead Code (Maintainability - Low)
* **Production Console Logs:** Multiple instances of `console.error` are left in production components (`src/pages/Text_Input.jsx`, `src/pages/Output.jsx`, `src/utils/apiClient.js`). These should ideally be stripped out or handled via a proper logging utility in production environments.
* **Unused Variables:** There are dozens of unused variables declared across the app (e.g., `stars` state in `Home.jsx`, unused DOM event parameters). These consume memory and trigger unnecessary linter warnings.

## Expected Behavior
* Failed `fetch` requests and dynamic imports should be properly caught and handled (e.g., logging the error safely or displaying a fallback UI state).
* Array shuffling logic should ideally use the cryptographically secure `window.crypto.getRandomValues()` API instead of `Math.random()`.
* Production builds should be completely free of dev-time `console` statements and unused/dead code.

## Proposed Solution
1. **Promises:** Append `.catch((error) => { ... })` to the affected promises in `Home.jsx` and `reportWebVitals.js`.
2. **Security:** Replace `Math.random()` in the `shuffleArray` utility functions with a secure implementation. For example:
```javascript
const randomBuffer = new Uint32Array(1);
window.crypto.getRandomValues(randomBuffer);
const randomFraction = randomBuffer[0] / (0xffffffff + 1);
const j = Math.floor(randomFraction * (i + 1));

Guida per i contributori

Nessuna guida per i contributori indicizzata per questo repository

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.