Skip to content

fix(#1): reject whitespace-only titles in POST and PATCH /tasks - #3

Open
fullsend-ai-coder[bot] wants to merge 5 commits into
mainfrom
agent/1-whitespace-title-validation
Open

fix(#1): reject whitespace-only titles in POST and PATCH /tasks#3
fullsend-ai-coder[bot] wants to merge 5 commits into
mainfrom
agent/1-whitespace-title-validation

Conversation

@fullsend-ai-coder

Copy link
Copy Markdown

The POST /tasks endpoint used if (!title) to validate titles, which only catches falsy values. Whitespace-only strings like " " are truthy in JavaScript and bypassed this check. The PATCH /tasks/:id endpoint had no title validation at all.

Changes:

  • POST handler: add .trim() check so whitespace-only titles
    are rejected with 400. Trim accepted titles before storing.
  • PATCH handler: add title validation when updates.title is
    provided, rejecting empty/whitespace-only values with 400.
    Trim accepted title updates before storing.
  • Add integration tests covering both endpoints for empty,
    whitespace-only, null, and valid title inputs.

Closes #1

Post-script verification

  • Branch is not main/master (agent/1-whitespace-title-validation)
  • Secret scan passed (gitleaks — 8165fcfe86e993b6f0e3c191e8c23ed4d2097643..HEAD)
  • Pre-commit hooks passed (authoritative run on runner)
  • Tests ran inside sandbox

The POST /tasks endpoint used `if (!title)` to validate titles,
which only catches falsy values. Whitespace-only strings like
"   " are truthy in JavaScript and bypassed this check. The
PATCH /tasks/:id endpoint had no title validation at all.

Changes:
- POST handler: add `.trim()` check so whitespace-only titles
  are rejected with 400. Trim accepted titles before storing.
- PATCH handler: add title validation when `updates.title` is
  provided, rejecting empty/whitespace-only values with 400.
  Trim accepted title updates before storing.
- Add integration tests covering both endpoints for empty,
  whitespace-only, null, and valid title inputs.

Closes #1
@fullsend-ai-review

fullsend-ai-review Bot commented Jun 1, 2026

Copy link
Copy Markdown

Review

Findings

Medium

  • [scope-creep] package.json:12 — The PR adds an engines field requiring Node.js >=20, but issue Bug: POST /tasks accepts whitespace-only titles #1 makes no mention of Node.js version requirements. This is unrelated to the whitespace validation bug and represents scope creep beyond the authorized work.
    Remediation: Remove the engines field addition from this PR. If Node.js version enforcement is desired, create a separate issue.

  • [scope-creep] src/index.js:8 — The PR adds URL parsing error handling with a try-catch block and 400 response for malformed URLs. Issue Bug: POST /tasks accepts whitespace-only titles #1 only authorizes fixing whitespace-only title validation. URL validation is a separate concern.
    Remediation: Remove the URL parsing try-catch block from this PR. If URL validation is needed, create a separate issue.

  • [missing-doc] README.md:10 — The PR introduces strict title validation for POST /tasks and PATCH /tasks/:id (must be a non-empty string, whitespace-only titles are rejected, titles are trimmed). The README's endpoint table says "Create a task" and "Update a task" without documenting required fields, validation rules, or trimming behavior.
    Remediation: Expand the API documentation to include request/response formats and validation rules for POST and PATCH endpoints.

Low

  • [missing-doc] README.md — The PR adds a Node.js version requirement (engines.node: ">=20") in package.json, but the README does not document this requirement. (Contingent on the engines field being kept.)

  • [architectural-layering] src/index.js:39 — Data normalization (trimming) is done in the API layer before passing to the store layer. The store layer accepts values as-is without normalization, creating inconsistency in where validation occurs.

  • [test-adequacy] src/index.test.js:14 — The test helper request() uses url.pathname but drops url.search, so any test that passes query parameters would silently strip them. No current tests are affected.

  • [test-organization] src/index.test.js:86 — The test file mixes describe block naming styles: 'Malformed URL handling' (concern-based) vs 'POST /tasks title validation' (endpoint-based).

Info

  • [test-adequacy] src/index.test.js — Tests share a single TaskStore instance across all test cases. Tasks created by earlier tests persist, making tests order-dependent.

  • [validation-consistency] src/index.js:72 — The PR only validates and trims the title field but not the description field. No comment explains why title gets special treatment.

  • [test-naming-consistency] src/index.test.js:87 — Test description 'returns 400 for URL that triggers parsing failure instead of crashing' is more verbose than existing patterns. Consider 'returns 400 for malformed URL'.

  • [test-coverage-alignment] src/index.test.js — The PR adds 215 lines of integration tests. Adding tests for a bug fix is reasonable engineering practice.

Previous run

Review

Findings

Low

  • [edge-case] src/index.test.js:91 — The malformed-URL test relies on new URL("///", base) throwing an error, which is Node-version-dependent behavior. On Node v22 this throws "Invalid URL", but on older Node versions (e.g., v18) /// may parse successfully, causing the test to fail. The project has no engines field in package.json.
    Remediation: 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.

  • [scope-creep] src/index.js:8 — The PR adds try/catch around URL parsing to handle malformed URLs, but Issue Bug: POST /tasks accepts whitespace-only titles #1 only authorizes fixing whitespace-only title validation. This is a small, obviously correct hardening change that prevents server crashes, but it is undocumented scope expansion.
    Remediation: Consider mentioning the URL hardening in the PR description, or splitting it into a separate PR.

  • [test-adequacy] src/index.test.js — The test suite validates title rejection and trimming well, but does not test that description is passed through unchanged (not trimmed) on POST or PATCH. A test confirming description passthrough would guard against future regressions.
    Remediation: Add a test case that sends a description with leading/trailing whitespace and asserts it is preserved as-is.

  • [test-adequacy] src/index.test.js — The test suite shares a single module-scoped TaskStore instance across all test cases. Tests accumulate state and task IDs increment across tests, making them order-dependent. Not a bug in the current suite.

Info

  • [input-validation] src/index.js:63 — The PATCH endpoint passes the full parsed JSON object to store.update(). The store only applies known fields (title, description, completed), but description and completed accept any type without validation. No exploitable vector in the current in-memory architecture.

  • [trim-behavior-consistency] src/index.js — Trimming titles before storing is a behavioral change beyond just rejecting whitespace-only titles. Issue Bug: POST /tasks accepts whitespace-only titles #1 requested rejecting whitespace-only titles, not normalizing all titles by trimming. This is a reasonable design choice but worth noting.

Previous run (2)

Review

Findings

Low

  • [logic-error] src/index.js:76 — In the PATCH handler, the trim guard uses if (updates.title) (truthiness) instead of if (updates.title !== undefined) to match the validation gate on line 72. The code is functionally correct today because validation on line 72 guarantees any defined title reaching line 76 is a non-empty string, but the inconsistency could cause subtle bugs if validation is ever relaxed.
    Remediation: Change if (updates.title) to if (updates.title !== undefined) for consistency.

  • [test-inadequate] src/index.test.js — The test suite does not include a test for PATCH with a null title ({ title: null }). The POST tests cover null, but the PATCH validation path differs (updates.title !== undefined gate). Since null !== undefined is true, a null title enters the validation branch and is rejected by the typeof check — worth an explicit test to prevent regression.

  • [scope-creep] src/index.js:7 — PR adds malformed URL error handling (try-catch around new URL()) not mentioned in issue Bug: POST /tasks accepts whitespace-only titles #1. This is a small (8-line) defensive change with its own test, architecturally reasonable, but technically outside the authorized scope.

  • [edge-case] src/index.js:35 — In the POST handler, description is not trimmed before being passed to store.create(). Consistent with pre-existing behavior and outside the PR scope, but asymmetric with the trimming applied to title.

  • [validation-pattern-consistency] src/index.js:72 — The PATCH endpoint uses two separate if-blocks for title validation and trimming, while POST combines them more concisely. Both work correctly; the structural difference reflects the conditional nature of PATCH updates.

Info

  • [test-file-organization] src/index.test.js — New test file follows the same import pattern (node:test, node:assert) and describe/it structure as src/store.test.js. No action needed.
Previous run (3)

Review

Findings

No findings.

Previous run (4)

Review

Findings

Low

  • [correctness] src/index.js:26 — Non-string title values (e.g., {title: 123}) will throw TypeError on .trim(), which is caught by the catch block and returned as "invalid JSON". The error message is misleading since the JSON itself is valid — the value type is wrong. This is a pre-existing pattern (the original !title check also didn't validate type), and the PR arguably improves behavior by rejecting non-string titles rather than silently accepting them. Consider adding an explicit type check (typeof title !== 'string') with a clearer error message as follow-up work.
    Remediation: Add typeof title !== "string" guard before .trim() with a descriptive error message like "title must be a string".

Info

  • [correctness] src/index.test.js — Tests are well-structured with good coverage of edge cases (empty, whitespace-only, tab, null, valid input, trimming) for both POST and PATCH endpoints. The test setup correctly uses port 0 for isolation and cleans up the require cache. No gaps identified in test coverage for the validation changes.

Comment thread src/index.js Outdated
try {
const { title, description } = JSON.parse(body);
if (!title) {
if (!title || !title.trim()) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[low] correctness

Non-string title values (e.g., {title: 123}) will throw TypeError on .trim(), caught by catch block and returned as "invalid JSON". The error message is misleading. Pre-existing pattern, but consider adding explicit type check as follow-up.

Suggested fix: Add typeof title !== "string" guard before .trim() with error message "title must be a string".

@fullsend-ai-review fullsend-ai-review Bot added the ready-for-merge All reviewers approved — ready to merge label Jun 1, 2026
@ggallen

ggallen commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

/fs-fix

Replace `!title` checks with `typeof title !== 'string'` guards in both
POST and PATCH /tasks handlers so non-string values (numbers, booleans)
return 400 with "title is required" instead of falling through to the
catch block with a misleading "invalid JSON" error.

Add tests for numeric and boolean title inputs in both endpoints.

Addresses review feedback on #3

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Signed-off-by: fullsend-fix <fullsend-code@users.noreply.github.com>
@fullsend-ai-coder

Copy link
Copy Markdown
Author

🔧 Fix agent — iteration 1 (human-triggered)

Addressed review feedback: replaced falsy checks with typeof string guards in both POST and PATCH title validation to handle non-string inputs correctly. Added 3 new tests.

Fixed (3):

  1. non-string title values cause misleading 'invalid JSON' error (src/index.js): Replaced !title with typeof title !== 'string' guard in POST /tasks handler so non-string values (numbers, booleans) return 400 with 'title is required' instead of throwing TypeError caught as 'invalid JSON'
  2. non-string title values cause misleading 'invalid JSON' error in PATCH handler (src/index.js): Added typeof updates.title !== 'string' guard in PATCH /tasks/:id handler for the same non-string title issue
  3. missing test coverage for non-string title inputs (src/index.test.js): Added tests for numeric and boolean title values in both POST and PATCH endpoints to cover the new typeof guards

Tests: passed

Updated by fullsend fix agent

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jun 1, 2026
@ascerra

ascerra commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

/fs-fix Apply similar input validation hardening for the pattern described in #2

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jun 16, 2026

Copy link
Copy Markdown
Author

🤖 Finished Fix · ✅ Success · Started 1:41 PM UTC · Completed 1:45 PM UTC
Commit: 8165fcf · View workflow run →

Wrap `new URL()` constructor in the request handler with try/catch so
malformed URL paths return 400 Bad Request instead of crashing the
server process. Add integration test for the malformed URL scenario.

Addresses review feedback on #3
@fullsend-ai-coder

Copy link
Copy Markdown
Author

🔧 Fix agent — iteration 2 (human-triggered)

Applied input validation hardening per issue #2: wrapped URL parsing in try/catch to prevent server crash on malformed URL paths. Added integration test. All 20 tests pass.

Fixed (2):

  1. Guard URL parsing to prevent server crash on malformed requests (issue Bug: Server crashes on malformed URL path #2) (src/index.js): Wrapped new URL(req.url, ...) in a try/catch block at the top of the request handler. On parse failure, the server now returns 400 Bad Request with a JSON error body instead of crashing with an unhandled TypeError.
  2. Add test coverage for malformed URL handling (src/index.test.js): Added a rawRequest() helper that sends HTTP requests with arbitrary paths (bypassing client-side URL validation) and a test that sends a path causing URL parse failure, asserting a 400 response.

Tests: passed

Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 16, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 1:46 PM UTC · Completed 1:55 PM UTC
Commit: 8165fcf · View workflow run →

Comment thread src/index.js
res.writeHead(400);
res.end(JSON.stringify({ error: 'title is required' }));
return;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Comment thread src/index.js
@@ -5,7 +5,15 @@ const store = new TaskStore();
const PORT = process.env.PORT || 3000;

const server = http.createServer((req, res) => {

Copy link
Copy Markdown

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.

Comment thread src/index.js
const { title, description } = JSON.parse(body);
if (!title) {
if (typeof title !== 'string' || !title.trim()) {
res.writeHead(400);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Comment thread src/index.js
req.on('end', () => {
try {
const updates = JSON.parse(body);
if (updates.title !== undefined && (typeof updates.title !== 'string' || !updates.title.trim())) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jun 16, 2026
@ascerra

ascerra commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

/fs-fix Follow the instructions at https://gist.github.com/ascerra/d77bba6af784edd4b4ca137086b6a37e

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jun 16, 2026

Copy link
Copy Markdown
Author

🤖 Finished Fix · ✅ Success · Started 1:59 PM UTC · Completed 2:02 PM UTC
Commit: 8165fcf · View workflow run →

- Change PATCH trim guard from truthiness check to strict undefined
  check for consistency with validation gate (logic-error finding)
- Add missing test for PATCH with null title (test-inadequate finding)

Addresses review feedback on #3
@fullsend-ai-coder

Copy link
Copy Markdown
Author

🔧 Fix agent — iteration 3 (human-triggered)

Addressed 2 actionable review findings (logic-error: PATCH trim guard consistency, test-inadequate: missing null title test). Could not access the gist URL from the human instruction due to network restrictions. Recorded 4 disagreements for informational/out-of-scope findings.

Fixed (2):

  1. PATCH trim guard uses truthiness check instead of strict undefined check (src/index.js): Changed if (updates.title) to if (updates.title !== undefined) on line 77 for consistency with the validation gate on line 72, as recommended by the reviewer.
  2. Missing PATCH null title test (src/index.test.js): Added test case for PATCH with { title: null } to match POST test coverage and prevent regression on the typeof validation path.

Disagreed (4):

  1. Could not access gist instructions: The human instruction referenced https://gist.github.com/ascerra/d77bba6af784edd4b4ca137086b6a37e which is inaccessible from the sandbox environment (403 Forbidden via proxy). Addressed the two actionable review findings (logic-error and test-inadequate) from the latest review instead.
  2. scope-creep: URL parsing error handling outside issue Bug: POST /tasks accepts whitespace-only titles #1: The URL parsing try-catch was explicitly authorized by human in iteration 2 (/fs-fix referencing issue Bug: Server crashes on malformed URL path #2). No change needed.
  3. edge-case: description field not trimmed in POST handler: Pre-existing behavior outside PR scope, as the reviewer acknowledged.
  4. validation-pattern-consistency: PATCH uses two if-blocks vs POST single block: Reviewer noted this is informational — both work correctly and the structural difference reflects the conditional nature of PATCH updates.

Tests: passed

Updated by fullsend fix agent

@fullsend-ai-review

Copy link
Copy Markdown

🤖 Review · Started 2:04 PM UTC
Commit: 8165fcf · View workflow run →

@ascerra

ascerra commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

/fs-fix Follow the instructions at https://jsonblob.com/api/jsonBlob/019ed0c5-2f9c-7490-abc4-5fc2027ba46d

Comment thread src/index.test.js
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');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

Comment thread src/index.js

const server = http.createServer((req, res) => {
const url = new URL(req.url, `http://${req.headers.host}`);
let url;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The 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.

@fullsend-ai-review fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed ready-for-merge All reviewers approved — ready to merge labels Jun 16, 2026
@fullsend-ai-review

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:04 PM UTC · Completed 2:13 PM UTC
Commit: 8165fcf · View workflow run →

@fullsend-ai-coder

fullsend-ai-coder Bot commented Jun 16, 2026

Copy link
Copy Markdown
Author

🤖 Finished Fix · ✅ Success · Started 2:15 PM UTC · Completed 2:19 PM UTC
Commit: 8165fcf · View workflow run →

- Add engines field (node >=20) to package.json to document the
  minimum Node version, addressing the edge-case finding about the
  malformed URL test relying on Node-version-dependent behavior.
- Add description passthrough tests for both POST and PATCH endpoints
  to guard against future regressions where description might be
  inadvertently trimmed.

Addresses review feedback on #3
@fullsend-ai-coder

Copy link
Copy Markdown
Author

🔧 Fix agent — iteration 4 (human-triggered)

Addressed 2 remaining actionable review findings: added engines field to package.json for Node >=20 (fixing the malformed URL test portability concern) and added description passthrough tests for both POST and PATCH endpoints. Could not access the jsonblob URL from the human instruction due to network restrictions. Disagreed with 4 informational/out-of-scope findings.

Fixed (2):

  1. malformed URL test uses Node-version-dependent /// path (package.json): Added engines field (node >=20) to package.json to document the minimum supported Node version. Node 18 reached EOL in April 2025, and the /// URL parsing behavior is consistent on Node >=20. This addresses the reviewer's concern about the test being unreliable across Node versions.
  2. no test for description passthrough (not trimmed) (src/index.test.js): Added two tests: one for POST and one for PATCH confirming that description values with leading/trailing whitespace are preserved as-is (not trimmed), guarding against future regressions.

Disagreed (5):

  1. scope-creep: URL parsing error handling outside issue Bug: POST /tasks accepts whitespace-only titles #1: The URL parsing try-catch was explicitly authorized by the human in iteration 2 via /fs-fix referencing issue Bug: Server crashes on malformed URL path #2. No change needed.
  2. shared TaskStore state across tests makes them order-dependent: The reviewer noted this is not a bug in the current suite. Refactoring test isolation is out of scope for this PR.
  3. PATCH passes full parsed JSON to store.update without type validation on description/completed: Reviewer marked this as Info with no exploitable vector. Out of scope for this PR.
  4. trim-behavior-consistency: trimming titles is a behavioral change beyond rejecting whitespace-only: Reviewer marked this as Info and called it a reasonable design choice. No change needed.
  5. could not access jsonblob URL from human instruction: The human instruction referenced https://jsonblob.com/api/jsonBlob/019ed0c5-2f9c-7490-abc4-5fc2027ba46d which is inaccessible from the sandbox environment (403 Forbidden via proxy). Addressed the two remaining actionable review findings instead.

Tests: passed

Updated by fullsend fix agent

@fullsend-ai-review

fullsend-ai-review Bot commented Jun 16, 2026

Copy link
Copy Markdown

🤖 Finished Review · ✅ Success · Started 2:21 PM UTC · Completed 2:30 PM UTC
Commit: 8165fcf · View workflow run →

@fullsend-ai-review fullsend-ai-review Bot added requires-manual-review Review requires human judgment and removed ready-for-merge All reviewers approved — ready to merge labels Jun 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

requires-manual-review Review requires human judgment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: POST /tasks accepts whitespace-only titles

2 participants