-
Notifications
You must be signed in to change notification settings - Fork 0
fix(#1): reject whitespace-only titles in POST and PATCH /tasks #3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
7e696ce
ec9f2e7
3080a7b
a002d76
75b4a79
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,7 +5,15 @@ const store = new TaskStore(); | |
| const PORT = process.env.PORT || 3000; | ||
|
|
||
| const server = http.createServer((req, res) => { | ||
| const url = new URL(req.url, `http://${req.headers.host}`); | ||
| let url; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] scope-creep The PR adds try/catch around URL parsing but Issue #1 only authorizes fixing whitespace-only title validation. This is a small, correct hardening change but undocumented scope expansion. Suggested fix: Consider mentioning the URL hardening in the PR description, or splitting it into a separate PR. |
||
| try { | ||
| url = new URL(req.url, `http://${req.headers.host}`); | ||
| } catch (e) { | ||
| res.setHeader('Content-Type', 'application/json'); | ||
| res.writeHead(400); | ||
| res.end(JSON.stringify({ error: 'bad request' })); | ||
| return; | ||
| } | ||
| const method = req.method; | ||
|
|
||
| res.setHeader('Content-Type', 'application/json'); | ||
|
|
@@ -23,12 +31,12 @@ const server = http.createServer((req, res) => { | |
| req.on('end', () => { | ||
| try { | ||
| const { title, description } = JSON.parse(body); | ||
| if (!title) { | ||
| if (typeof title !== 'string' || !title.trim()) { | ||
| res.writeHead(400); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] edge-case Description field not trimmed in POST handler, asymmetric with title trimming. Pre-existing behavior, outside PR scope. |
||
| res.end(JSON.stringify({ error: 'title is required' })); | ||
| return; | ||
| } | ||
| const task = store.create(title, description); | ||
| const task = store.create(title.trim(), description); | ||
| res.writeHead(201); | ||
| res.end(JSON.stringify(task)); | ||
| } catch (e) { | ||
|
|
@@ -61,6 +69,14 @@ const server = http.createServer((req, res) => { | |
| req.on('end', () => { | ||
| try { | ||
| const updates = JSON.parse(body); | ||
| if (updates.title !== undefined && (typeof updates.title !== 'string' || !updates.title.trim())) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] validation-pattern-consistency PATCH uses two separate if-blocks for validation and trimming while POST combines them. Both correct; structural difference reflects conditional PATCH updates. |
||
| res.writeHead(400); | ||
| res.end(JSON.stringify({ error: 'title is required' })); | ||
| return; | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] logic-error PATCH trim guard uses truthiness check instead of strict undefined check, inconsistent with validation gate on line 72. Functionally correct today but fragile if validation is relaxed. Suggested fix: Change if (updates.title) to if (updates.title !== undefined) for consistency. |
||
| if (updates.title !== undefined) { | ||
| updates.title = updates.title.trim(); | ||
| } | ||
| const task = store.update(id, updates); | ||
| if (!task) { | ||
| res.writeHead(404); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,215 @@ | ||
| const { describe, it, before, after } = require('node:test'); | ||
| const assert = require('node:assert'); | ||
| const http = require('node:http'); | ||
|
|
||
| let server; | ||
| let baseUrl; | ||
|
|
||
| function request(method, path, body) { | ||
| return new Promise((resolve, reject) => { | ||
| const url = new URL(path, baseUrl); | ||
| const options = { | ||
| hostname: url.hostname, | ||
| port: url.port, | ||
| path: url.pathname, | ||
| method, | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }; | ||
| const req = http.request(options, (res) => { | ||
| let data = ''; | ||
| res.on('data', chunk => { data += chunk; }); | ||
| res.on('end', () => { | ||
| resolve({ | ||
| statusCode: res.statusCode, | ||
| body: data ? JSON.parse(data) : null, | ||
| }); | ||
| }); | ||
| }); | ||
| req.on('error', reject); | ||
| if (body !== undefined) { | ||
| req.write(JSON.stringify(body)); | ||
| } | ||
| req.end(); | ||
| }); | ||
| } | ||
|
|
||
| // Use port 0 to get a random available port | ||
| before(() => { | ||
| process.env.PORT = '0'; | ||
| // Clear require cache so index.js binds to port 0 | ||
| delete require.cache[require.resolve('./index')]; | ||
| const mod = require('./index'); | ||
| server = mod.server; | ||
| return new Promise((resolve) => { | ||
| if (server.listening) { | ||
| baseUrl = `http://localhost:${server.address().port}`; | ||
| resolve(); | ||
| } else { | ||
| server.on('listening', () => { | ||
| baseUrl = `http://localhost:${server.address().port}`; | ||
| resolve(); | ||
| }); | ||
| } | ||
| }); | ||
| }); | ||
|
|
||
| after(() => { | ||
| return new Promise((resolve) => { | ||
| server.close(resolve); | ||
| }); | ||
| }); | ||
|
|
||
| function rawRequest(method, rawPath) { | ||
| return new Promise((resolve, reject) => { | ||
| const parsedBase = new URL(baseUrl); | ||
| const options = { | ||
| hostname: parsedBase.hostname, | ||
| port: parsedBase.port, | ||
| path: rawPath, | ||
| method, | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| }; | ||
| const req = http.request(options, (res) => { | ||
| let data = ''; | ||
| res.on('data', chunk => { data += chunk; }); | ||
| res.on('end', () => { | ||
| resolve({ | ||
| statusCode: res.statusCode, | ||
| body: data ? JSON.parse(data) : null, | ||
| }); | ||
| }); | ||
| }); | ||
| req.on('error', reject); | ||
| req.end(); | ||
| }); | ||
| } | ||
|
|
||
| describe('Malformed URL handling', () => { | ||
| it('returns 400 for URL that triggers parsing failure instead of crashing', async () => { | ||
| const res = await rawRequest('GET', '///'); | ||
| assert.strictEqual(res.statusCode, 400); | ||
| assert.strictEqual(res.body.error, 'bad request'); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] edge-case The malformed-URL test relies on new URL("///", base) throwing an error, which is Node-version-dependent. On older Node versions /// may parse successfully, causing the test to fail. The project has no engines field in package.json. Suggested fix: Use a URL string that reliably triggers a parse error across all supported Node versions (e.g., /%ZZ), or add an engines field to package.json. |
||
| }); | ||
| }); | ||
|
|
||
| describe('POST /tasks title validation', () => { | ||
| it('rejects empty string title with 400', async () => { | ||
| const res = await request('POST', '/tasks', { title: '' }); | ||
| assert.strictEqual(res.statusCode, 400); | ||
| assert.strictEqual(res.body.error, 'title is required'); | ||
| }); | ||
|
|
||
| it('rejects whitespace-only title with 400', async () => { | ||
| const res = await request('POST', '/tasks', { title: ' ' }); | ||
| assert.strictEqual(res.statusCode, 400); | ||
| assert.strictEqual(res.body.error, 'title is required'); | ||
| }); | ||
|
|
||
| it('rejects tab-only title with 400', async () => { | ||
| const res = await request('POST', '/tasks', { title: '\t' }); | ||
| assert.strictEqual(res.statusCode, 400); | ||
| assert.strictEqual(res.body.error, 'title is required'); | ||
| }); | ||
|
|
||
| it('rejects null title with 400', async () => { | ||
| const res = await request('POST', '/tasks', { title: null }); | ||
| assert.strictEqual(res.statusCode, 400); | ||
| assert.strictEqual(res.body.error, 'title is required'); | ||
| }); | ||
|
|
||
| it('rejects numeric title with 400', async () => { | ||
| const res = await request('POST', '/tasks', { title: 123 }); | ||
| assert.strictEqual(res.statusCode, 400); | ||
| assert.strictEqual(res.body.error, 'title is required'); | ||
| }); | ||
|
|
||
| it('rejects boolean title with 400', async () => { | ||
| const res = await request('POST', '/tasks', { title: true }); | ||
| assert.strictEqual(res.statusCode, 400); | ||
| assert.strictEqual(res.body.error, 'title is required'); | ||
| }); | ||
|
|
||
| it('accepts valid title and trims whitespace', async () => { | ||
| const res = await request('POST', '/tasks', { title: ' Buy groceries ' }); | ||
| assert.strictEqual(res.statusCode, 201); | ||
| assert.strictEqual(res.body.title, 'Buy groceries'); | ||
| }); | ||
|
|
||
| it('accepts valid title without extra whitespace', async () => { | ||
| const res = await request('POST', '/tasks', { title: 'Clean house' }); | ||
| assert.strictEqual(res.statusCode, 201); | ||
| assert.strictEqual(res.body.title, 'Clean house'); | ||
| }); | ||
|
|
||
| it('preserves description with leading/trailing whitespace', async () => { | ||
| const res = await request('POST', '/tasks', { title: 'Task', description: ' spaced out ' }); | ||
| assert.strictEqual(res.statusCode, 201); | ||
| assert.strictEqual(res.body.description, ' spaced out '); | ||
| }); | ||
| }); | ||
|
|
||
| describe('PATCH /tasks/:id title validation', () => { | ||
| it('rejects whitespace-only title update with 400', async () => { | ||
| const createRes = await request('POST', '/tasks', { title: 'Original' }); | ||
| const id = createRes.body.id; | ||
|
|
||
| const res = await request('PATCH', `/tasks/${id}`, { title: ' ' }); | ||
| assert.strictEqual(res.statusCode, 400); | ||
| assert.strictEqual(res.body.error, 'title is required'); | ||
| }); | ||
|
|
||
| it('rejects empty string title update with 400', async () => { | ||
| const createRes = await request('POST', '/tasks', { title: 'Original' }); | ||
| const id = createRes.body.id; | ||
|
|
||
| const res = await request('PATCH', `/tasks/${id}`, { title: '' }); | ||
| assert.strictEqual(res.statusCode, 400); | ||
| assert.strictEqual(res.body.error, 'title is required'); | ||
| }); | ||
|
|
||
| it('rejects numeric title update with 400', async () => { | ||
| const createRes = await request('POST', '/tasks', { title: 'Original' }); | ||
| const id = createRes.body.id; | ||
|
|
||
| const res = await request('PATCH', `/tasks/${id}`, { title: 42 }); | ||
| assert.strictEqual(res.statusCode, 400); | ||
| assert.strictEqual(res.body.error, 'title is required'); | ||
| }); | ||
|
|
||
| it('rejects null title update with 400', async () => { | ||
| const createRes = await request('POST', '/tasks', { title: 'Original' }); | ||
| const id = createRes.body.id; | ||
|
|
||
| const res = await request('PATCH', `/tasks/${id}`, { title: null }); | ||
| assert.strictEqual(res.statusCode, 400); | ||
| assert.strictEqual(res.body.error, 'title is required'); | ||
| }); | ||
|
|
||
| it('accepts valid title update and trims whitespace', async () => { | ||
| const createRes = await request('POST', '/tasks', { title: 'Original' }); | ||
| const id = createRes.body.id; | ||
|
|
||
| const res = await request('PATCH', `/tasks/${id}`, { title: ' Updated ' }); | ||
| assert.strictEqual(res.statusCode, 200); | ||
| assert.strictEqual(res.body.title, 'Updated'); | ||
| }); | ||
|
|
||
| it('allows updates without title field', async () => { | ||
| const createRes = await request('POST', '/tasks', { title: 'Keep this' }); | ||
| const id = createRes.body.id; | ||
|
|
||
| const res = await request('PATCH', `/tasks/${id}`, { completed: true }); | ||
| assert.strictEqual(res.statusCode, 200); | ||
| assert.strictEqual(res.body.title, 'Keep this'); | ||
| assert.strictEqual(res.body.completed, true); | ||
| }); | ||
|
|
||
| it('preserves description with leading/trailing whitespace on update', async () => { | ||
| const createRes = await request('POST', '/tasks', { title: 'Original' }); | ||
| const id = createRes.body.id; | ||
|
|
||
| const res = await request('PATCH', `/tasks/${id}`, { description: ' spaced out ' }); | ||
| assert.strictEqual(res.statusCode, 200); | ||
| assert.strictEqual(res.body.description, ' spaced out '); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] scope-creep
URL parsing error handling added but not authorized by issue #1. Small defensive change with its own test, architecturally reasonable.