Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .github/astro-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,15 @@ trigger:
label: astro-review
review:
skill: .agents/skills/astro-review
severity: [critical, high, medium, low]
areas:
- design
- correctness
- security
- runtime
- completeness
- error-handling
- tests
- maintainability
- documentation
- changeset
31 changes: 31 additions & 0 deletions index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
const http = require('node:http');

const users = [
{ id: 1, name: 'Ada', role: 'admin' },
{ id: 2, name: 'Grace', role: 'member' },
];

function findUser(id) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[critical][correctness]: findUser mutates records due to assignment in predicate

The users.find predicate uses user.id = Number(id) (assignment) instead of user.id === Number(id). On every lookup it overwrites the first user's id with the requested value and returns that mutated object if the assigned value is truthy. This corrupts the in-memory user store and makes /users/:id return the wrong record (or undefined for id 0). Change the predicate to a strict comparison and consider using === consistently throughout the file.

return users.find((user) => (user.id = Number(id)));
Comment thread
ematipico marked this conversation as resolved.
Comment thread
ematipico marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[critical][correctness]: Assignment operator used instead of equality comparison in findUser

user.id = Number(id) uses assignment instead of comparison. This mutates every user object in the users array to the queried ID and causes findUser to always return the first user regardless of the requested ID. It should be user.id === Number(id).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[critical][correctness]: findUser mutates users and always returns the first entry

The predicate inside users.find uses the assignment operator = (user.id = Number(id)) instead of a comparison. This mutates every visited user's id property and causes find to always return the first element because the resulting number is truthy. Use strict equality (===) to compare without mutating the source array.

}

const server = http.createServer((request, response) => {
const url = new URL(request.url, 'http://localhost');

if (url.pathname === '/welcome') {
const name = url.searchParams.get('name');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[high][security]: Reflected XSS in /welcome endpoint

The name query parameter is interpolated directly into the HTML response without escaping or sanitization. A request such as /welcome?name=<script>alert(1)</script> will execute JavaScript in the visitor's browser. Escape the value before embedding it in HTML (for example with a function that replaces &, <, >, and quotes), or serve the parameter as plain text with content-type: text/plain.

response.setHeader('content-type', 'text/html');
response.end(`<h1>Welcome, ${name}</h1>`);
Comment thread
ematipico marked this conversation as resolved.
Comment thread
ematipico marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[critical][security]: Unescaped query parameter rendered as HTML enables XSS

The name query parameter is interpolated directly into the HTML response without sanitization. A malicious URL such as ?name=&lt;script>alert(1)&lt;/script> will execute arbitrary JavaScript in the victim's browser. Escape or sanitize name before including it in the response, or use a safe templating mechanism.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[critical][security]: Reflected XSS via unescaped query parameter in /welcome

The name query parameter is interpolated directly into an HTML response without escaping or sanitization. An attacker can inject arbitrary scripts by providing a malicious name value (e.g., ?name=&lt;script>alert(1)&lt;/script>). Escape HTML entities before rendering, or switch the response type to text/plain.

}

if (url.pathname.startsWith('/users/')) {
const user = findUser(url.pathname.split('/')[2]);
response.setHeader('content-type', 'application/json');
response.end(JSON.stringify(user));
Comment thread
ematipico marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium][correctness]: Missing user returns invalid JSON body

If findUser returns undefined (once the assignment bug is fixed), JSON.stringify(undefined) produces the literal string undefined, which is not valid JSON. The endpoint should check if the user exists and return a proper 404 response or a valid JSON body such as null.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium][correctness]: Missing user returns HTTP 200 with empty body

If findUser fails to match a user, it returns undefined. JSON.stringify(undefined) evaluates to undefined, so response.end() sends no body while the status code remains the default 200. Check whether user exists and set response.statusCode = 404 before sending the response when no user is found.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[critical][correctness]: Successful responses fall through to the 404 path

After /welcome and /users/:id call response.end(), execution continues to lines 23-24, which set statusCode = 404 and call response.end('Not found') again. This triggers ERR_HTTP_HEADERS_SENT and crashes the request handling for every matched route. Add return after each response.end() in the matched branches, or restructure the conditionals with else if so only one response is sent per request.

}

response.statusCode = 404;
Comment thread
ematipico marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[high][correctness]: Missing return statements cause 404 response to override successful routes

After handling /welcome or /users/:id, execution falls through to the 404 handler at the bottom. This causes response.statusCode = 404 and a second response.end() to be called. Either add return after each response.end() in the route handlers, or restructure the conditions into an if / else if / else chain.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium][correctness]: Request handlers fall through to 404 logic

Neither the /welcome nor the /users/ branch returns or exits after calling response.end(), so execution always falls through to response.statusCode = 404. While Node.js currently suppresses the extra end() call, this is incorrect and fragile. Add return after each response.end() or restructure the branches with else if.

response.end('Not found');
Comment thread
ematipico marked this conversation as resolved.
});

server.listen(3000);
13 changes: 13 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"name": "astro-test",
"version": "1.0.0",
"description": "",
"main": "index.js",
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
},
"keywords": [],
"author": "",
"license": "ISC",
"packageManager": "pnpm@10.28.0"
}