fix(crud): reject non-strict-numeric :id up-front (closes #162) by JayDS22 · Pull Request #208 · accordproject/apap

JayDS22 · 2026-07-11T18:12:41Z

Summary

Closes #162. parseInt(req.params.id) returns 1 for the string '1abc', so before this guard:

PUT /agreements/1abc with a valid body silently updated row 1
DELETE /templates/1abc silently deleted row 1
GET /templates/1abc silently returned row 1

What this PR does

Adds a route-level router.param('id', ...) middleware in buildCrudRouter that rejects anything that is not a strict decimal integer with 404 on integer-keyed tables. UUID-keyed tables keep the existing pass-through so the DB layer can surface malformed UUIDs.

The guard runs before any body validation, DB read, DB mutation, or side-effectful trigger, so a bad :id never touches the DB and never blocks on Concerto validation of the body.

Tests

7 new cases appended to crud.test.ts covering GET, PUT, DELETE with:

1abc (parseInt partial parse, the case from the issue)
1.5 (float)
-1 (negative)
0x10 (hex)
1e2 (scientific)

PUT and DELETE cases also assert that dbMock.update / dbMock.delete were never called, pinning the safety property that the guard runs before any DB mutation. The accepted-path regression is already covered by the existing DELETE /:id returns 200 when resource exists case.

Also in this PR

Fixes a pre-existing test bug in crud.test.ts that imported DbTemplate from ../db/schema. DbTemplate is a local alias in agreements.ts (import { Template as DbTemplate }), not a named export from the schema module, so the two tests that used it got table: undefined. They silently passed because the exercised code path never touched table before returning. The new :id middleware accesses table.id.columnType up-front, so the real export name is required now. This is a one-word fix (DbTemplate -> Template) with an inline comment explaining why.

Validation

npm test in server/: 98/98 passing (16 in crud.test.ts)
npx tsc --noEmit in server/: clean

Non-goals

UUID format validation is out of scope; the guard skips UUID tables and leaves DB-layer errors in place
No changes to parseInt call sites inside handlers; the middleware guarantees strict-decimal input reaches them
No overlap with fix(crud): prevent SQL injection via identifier validation and operator whitelist #187 (defaultWhereClause SQL injection guard) or fix(crud): harden pagination query parsing #177 (parseQueryParams hardening) — different functions in the same file

Author Checklist

DCO sign-off provided
Tests added and passing
Typecheck clean

…ct#162) parseInt(req.params.id) returns 1 for '1abc', so before this guard PUT /agreements/1abc with a valid body silently updated row 1, and DELETE /templates/1abc silently deleted row 1. A route-level router.param('id') middleware rejects anything that is not a strict decimal integer with 404 for integer-keyed tables. UUID-keyed tables keep the existing pass-through so the DB layer can surface malformed UUIDs. Rejected forms covered by the appended tests: '1abc' (partial parse), '1.5' (float), '-1' (negative), '0x10' (hex), '1e2' (scientific). The existing DELETE /:id test on id 1 covers the accepted-path regression. Also fixes a pre-existing test bug in crud.test.ts that imported `DbTemplate` (a local alias in agreements.ts, not a named export from db/schema.ts). The custom-validation short-circuit meant the code path never touched `table`, so `table: undefined` silently passed. The new :id middleware accesses `table.id.columnType` up-front, so the real export name is required now. Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>

apoorv7g

We should keep the old variable name because it makes it much clearer what the template is actually for

apoorv7g · 2026-07-12T16:27:56Z

+        // up-front, so the real export name is required now.
        const { buildCrudRouter } = require('./crud');
-        const { DbTemplate } = require('../db/schema');
+        const { Template } = require('../db/schema');


Why has this been changed from DbTemplate to Template? What purpose does it achieve ?

DbTemplate isn't a named export from db/schema. It's a local alias inside agreements.ts (import { Template as DbTemplate }). The two tests importing it were getting undefined silently and only passed because the exercised code path never touched table before returning. The new :id middleware accesses table.id.columnType up-front, so the actual export name is required now.

There's an inline comment on the import explaining this, plus the "Also in this PR" section of the description.

Alright, I overlooked it, also here is a better way we could handle the requirement.

const { Template: DbTemplate } = require('../db/schema');

Let me know if this works out well.

JayDS22 · 2026-07-12T17:15:34Z

Follow-up on the review comment about keeping the old name. Noted the readability concern. Alias resolution keeps DbTemplate at the call site while pointing at the real export: const { Template: DbTemplate } = require('../db/schema'). Can apply that if the local name is what matters. The line-comment thread on crud.test.ts:152 has the technical detail on why the literal import can't stay.

apoorv7g · 2026-07-12T17:26:01Z

Sorry, I didn't check this.

Follow-up on the review comment about keeping the old name. Noted the readability concern. Alias resolution keeps DbTemplate at the call site while pointing at the real export: const { Template: DbTemplate } = require('../db/schema'). Can apply that if the local name is what matters. The line-comment thread on crud.test.ts:152 has the technical detail on why the literal import can't stay.

Yep that's what I mean.

Keeps the reader-facing local name apoorv7g flagged in review while sourcing from the real db/schema export. Two test blocks that previously did `require('../db/schema').DbTemplate` (which resolved to `undefined`) now use `{ Template: DbTemplate }`. Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>

JayDS22 · 2026-07-12T17:35:11Z

Applied. 19ffbff aliases at the import site: const { Template: DbTemplate } = require('../db/schema') in both blocks, table: DbTemplate at the call site. Comment updated to explain the aliasing. Local test run stays 16/16 green.

apoorv7g · 2026-07-12T17:23:10Z

+        // up-front, so the real export name is required now.
        const { buildCrudRouter } = require('./crud');
-        const { DbTemplate } = require('../db/schema');
+        const { Template } = require('../db/schema');


Alright, I overlooked it, also here is a better way we could handle the requirement.

const { Template: DbTemplate } = require('../db/schema');

Let me know if this works out well.

niallroche

Approved. Two notes for the record, since this touches resource identity:

This guard hardens how the reference implementation addresses resources — via the Postgres serial id on the /:id routes. That integer is an RI storage detail (the DB primary key), not part of the APAP protocol. The protocol identifies Template, Agreement, and SharedModel by uri (identified by uri in protocol.cto); the OpenAPI contract paths are /{uri}, typed string). While the RI uses Postgress, an APAP compliant server backed by a document store, content-addressed store, or other is free to use any string identity and remains conformant.

So this PR should be read as "the RI rejects malformed integer ids before they reach the DB," not "APAP resources are integer-identified." The longer-term alignment is likely to need some additional work in routing the RI on uri to match the published contract .

github-actions · 2026-07-13T13:03:43Z

🎉 Thank you for your contribution! 🎉

Dear @JayDS22,

Your pull request has been successfully merged into the project! We greatly appreciate your efforts and the time you've dedicated to improving our repository.

What happens next?

Your changes will be included in the next release
Your name will be added to our contributors list
Feel free to take on another issue or suggest new features

Once again, thank you for being part of our community!

Best regards,
The APAP Team

Brings the roadmap current through the first-half deliverables: - Status header now reflects W7 start, first-half PRs (accordproject#184, accordproject#196, accordproject#199, accordproject#200, accordproject#201, accordproject#202) all merged upstream, midterm dispatch published, Thursday sync slot confirmed with Niall. - Workstream table reflects Proposal Core first-half complete, MCP RC migration on track through SEP-2549, Alternatives Evaluation complete with Medium publication. - 12-week schedule marks W4–W6 Done, W7 Active (subscriptions/listen slice scoped in apap-mcp-poc#6), W8 covers upstream port + JSON-RPC error mapping. - Milestones: adds Jul 12 midterm dispatch published and Jul 13 midterm eval submitted. - Open decisions: 2, 3, and 4 all resolved (alternatives shipped, MCP RC transport parallel via accordproject#201, Thursday sync slot confirmed). - Contributions to date: adds accordproject#200, accordproject#201, accordproject#202, accordproject#208, and the peer review posted on accordproject#194. accordproject#196 marked merged; accordproject#197 marked superseded. - Adds Comms deliverables section for the Medium/LinkedIn/Discord surfaces. Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>

…6 sync decisions Applies the factual corrections Steven flagged in his approval review on this PR and folds in the Jul 16 sync outcomes so the doc reflects current reality. Steven's factual fixes: - Status header: "Six PRs merged" corrected to "Eight PRs" and the full list updated to include accordproject#208 (which merged Jul 13 rather than sitting as "awaiting maintainer merge") - "Five parallel lanes" corrected to "Six parallel lanes" now that Workstream 6 (Headroom compression eval) is on the doc - "slice 2" references to accordproject#200 renamed to "slice 3" throughout to match the PR title; accordproject#197 explicitly referenced as the closed slice-2 draft it replaces - pino refactor line: "~28 console.log sites" corrected to "17 console.log + 11 console.error sites" per Steven's exact count Memorialization note added to the workstream table intro explaining that Workstream 6 plus the Comms deliverables and Future work sections were added mid-project with mentor input (Niall Jul 14 Discord on the ACE-Router thread, Steven Jul 16 review asking that the scope expansion be memorialized). Jul 16 sync outcomes: - Status header now reflects W7 day 5 with the PR accordproject#7 subscriptions/listen slice shipped on the POC and issue accordproject#8 tracking the US-C4 follow-up - Explicit note that this PR (accordproject#198) stays open as the rolling roadmap doc through end of GSoC per Niall's Jul 16 call Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>

…slice 1) Slice 1 of the shared-service-layer port from apap-mcp-poc. Adds the first service module upstream and rewires all four MCP template call sites in mcp.ts to call it directly instead of going through the internal `makeApiRequest(${API_BASE_URL}/templates...)` HTTP loop. Files: - server/db/client.ts (new, 8 lines) - Database type export (a PostgresJsDatabase<typeof schema> alias) so services can type the db handle they take as their first argument. - server/services/templateService.ts (new, 69 lines) - six functions (listTemplates, getTemplateById, getTemplateByUri, createTemplate, updateTemplate, deleteTemplate) taking `db` as the first arg. Throws TemplateNotFoundError / TemplateDuplicateError from the existing ServiceError hierarchy introduced in accordproject#184. No transport-specific imports. - server/services/templateService.test.ts (new, 211 lines, jest) - 13 unit tests over a fluent Drizzle query-builder mock. Covers the success and not-found paths for all six functions plus the 23505 unique-violation and generic re-throw branches on createTemplate. - server/handlers/mcp.ts - getServer signature changes from `()` to `(db: Database)`; both callers pass `res.locals.db`. Four call sites now use templateService instead of makeApiRequest: * getTemplates (module fn) uses listTemplates * apap://templates/{templateId} list callback uses listTemplates * apap://templates/{templateId} read callback uses getTemplateById * getTemplate tool uses getTemplateById Same strict-numeric guard as accordproject#208 (regex /^\d+$/) is applied to the templateId path segment before calling getTemplateById, so a malformed resource URI resolves to TemplateNotFoundError instead of NaN. Existing serviceErrorToResourceError / serviceErrorToCallToolResult mappers convert the typed errors to protocol-appropriate responses. Not in slice 1: - agreementService port (slice 2, larger surface with trigger/convert) - REST /templates route rewire to use the service (slice 3) - subscriptionRegistry port (separate slice) Validation: - npm test in server/: 9/9 suites pass (121 tests, 13 new) - npx tsc --noEmit clean - No changes to the REST /templates route, so accordproject#187 SQL injection guard and accordproject#208 :id middleware are untouched Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>

…eement lookups (slice 2) Slice 2 of the shared-service-layer port. Stacked on top of accordproject#211 (slice 1) — this branch is `jay/feat/service-layer-slice-2-agreements` off `jay/feat/service-layer-slice-1-templates`. The diff will collapse to just these three files once accordproject#211 merges. Scope choice: only the CRUD lookup half is ported (`listAgreements`, `getAgreementById`). The POC's `convertAgreement` and `triggerAgreement` are stubs that do not run the real @accordproject/template-engine, so porting them as-is would regress the RI's existing `TemplateArchiveProcessor`-based convert + trigger implementation in server/handlers/agreements.ts. That deserves its own slice (2b) that wraps the real engine rather than replacing it with a POC stub. Files: - server/services/agreementService.ts (new, 25 lines) - two functions (listAgreements, getAgreementById) taking `db` as the first arg. Throws AgreementNotFoundError from the existing ServiceError hierarchy. - server/services/agreementService.test.ts (new, 85 lines, jest) - 4 unit tests over the same fluent Drizzle mock pattern established in templateService.test.ts. Covers the success and not-found paths for both functions. - server/handlers/mcp.ts - four agreement call sites now use the service instead of makeApiRequest: * getAgreement module fn uses getAgreementById * getAgreements module fn uses listAgreements * apap://agreements/{agreementId} list callback uses listAgreements * getAgreement tool uses getAgreementById Same strict-numeric guard as accordproject#208 (/^\d+$/) applied to the agreementId path segment. Also preserves the accordproject#128 fix (agreement's own `uri` field must not overwrite the MCP resource URI). Not in this slice: - convert-agreement-to-format tool (still uses makeApiRequest until slice 2b wraps the real template engine as a service) - trigger-agreement tool (same reason) - REST /agreements route rewire (slice 3 territory, once slice 1 lands) Validation: - npm test in server/: 10/10 suites pass (125 tests, 4 new) - npx tsc --noEmit clean - No changes to REST route, template engine wrap, or convert/trigger logic; those paths remain unchanged Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>

…T + MCP (slice 2b) Slice 2b of the shared-service-layer port. Stacked on top of accordproject#213 (slice 2). Adds `convertAgreement` to `agreementService.ts` so both the REST `/agreements/:id/convert/:format` route and the MCP `convert-agreement-to-format` tool call the same function. Eliminates one more internal HTTP loop from `handlers/mcp.ts`. Scope reduction from the originally planned slice 2b: `triggerAgreement` extraction is deferred to slice 2c. When the trigger logic runs behind a service-layer import chain, jest cannot propagate `--experimental-vm-modules` through the dynamic imports `@accordproject/template-engine` performs, and the existing trigger tests fail with `ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG`. The old inline trigger path (unchanged in this slice) works because template-engine loads from the same module context as the test file. Solving that needs its own investigation and does not belong in this slice. Files: - server/services/agreementService.ts * Adds `resolveAgreementRuntime` (private helper): loads agreement, resolves associated template by hash or URI, reconstructs the Cicero template archive. Mirrors the inline resolveAgreement in handlers/agreements.ts. * Adds `convertAgreement(db, agreementId, format)`: uses resolveAgreementRuntime + TemplateArchiveProcessor.draft. Throws AgreementNotFoundError or AgreementConversionError on failure. * templatebuilder is imported from ../handlers/templatebuilder. It is technically under handlers/ in the current tree but is a transport-agnostic utility (no Express or MCP SDK imports). Moving it to a proper utility directory is a follow-up refactor. - server/handlers/agreements.ts * REST /:id/convert/:format now calls convertAgreement service. Adds the same strict-numeric :id guard as accordproject#208. * Trigger route unchanged (slice 2c territory). * Inline resolveAgreement helper kept because trigger still uses it. Duplication with resolveAgreementRuntime is intentional for slice 2b and gets removed in slice 2c. - server/handlers/mcp.ts * MCP `convert-agreement-to-format` tool now calls convertAgreement service directly. Adds the same strict-numeric guard. * Module fn `draftAgreement` deleted (was the HTTP-loop wrapper). * Trigger tool and its module fn `triggerAgreement` unchanged. Validation: - npm test in server/: 10/10 suites pass, 125 tests total - npx tsc --noEmit clean - Trigger path completely untouched; existing trigger tests (including the ones that exercise template-engine dynamic imports) all still pass Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>

@Thomas-Sedhom

…e (takeover of #165) Adds coverage for CRUD router surface not exercised by the existing 26 tests in server/handlers/crud.test.ts. The router's list envelope shape (items + total + page + limit + totalPages), single-fetch happy + 404, and POST insert happy path were all untested. Original framing and coverage-gap identification: @Thomas-Sedhom's #165 (opened 2026-04-14). That PR sat without author activity for ~4 months while server/handlers/crud.ts was hardened with SQL-injection guards (bdc536e), strict numeric-ID parsing (#208, eff6134), 404-on-missing DELETE (#180), pagination guards (#177), and pagination-query parsing hardening (5e6782d). A straight rebase of #165 would test a router that no longer exists; replacing rather than rebasing per the maintainer stale-PR takeover policy. New coverage (7 tests, 3 describe blocks): GET / list route (envelope + pagination shape): - returns PaginatedResponse envelope with items + total + page + limit + totalPages - returns an empty items array + total=0 when the table has no rows - paginates past total: page beyond last returns empty items but preserves total - computes totalPages as ceil(total / limit) GET /:id single-fetch route: - returns 200 with the row when the id exists - returns 404 when the id does not exist POST / happy path: - inserts a valid body and returns 200 with the created row Mock helper for the list route uses a queued 'then' interceptor on the db mock so the two sequential awaits (count query, items query) can each return their own value without brittle mockResolvedValueOnce chaining. globalErrorHandler is now wired into the test apps so thrown errors surface as JSON responses that assertions can inspect, matching the runtime shape at server/index.ts. Validation: - npm test: 10 suites / 158 tests, all pass Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>

JayDS22 mentioned this pull request Jul 11, 2026

refactor(error-handling): centralize duplicate try/catch into Express error middleware #192

Merged

5 tasks

apoorv7g reviewed Jul 12, 2026

View reviewed changes

apoorv7g approved these changes Jul 12, 2026

View reviewed changes

github-actions Bot added the maintainer-engaged A maintainer has commented or reviewed this item label Jul 13, 2026

niallroche self-requested a review July 13, 2026 13:00

niallroche approved these changes Jul 13, 2026

View reviewed changes

niallroche merged commit 5f0026d into accordproject:main Jul 13, 2026
15 checks passed

This was referenced Jul 14, 2026

fix(crud): prevent SQL injection via identifier validation and operator whitelist #187

Merged

fix(mcp): add TTL-based session cleanup to prevent memory leak #194

Merged

stevenobiajulu mentioned this pull request Jul 16, 2026

docs(gsoc-2026-roadmap): W3+W4 refresh #198

Open

JayDS22 mentioned this pull request Jul 18, 2026

feat(services): port templateService + rewire mcp.ts template loops (slice 1) #211

Merged

5 tasks

This was referenced Jul 18, 2026

feat(services): port agreementService CRUD half + rewire mcp.ts agreement lookups (slice 2) #213

Merged

feat(services): port convertAgreement to agreementService, rewire REST + MCP (slice 2b) #214

Merged

JayDS22 mentioned this pull request Jul 24, 2026

test: add unit tests for CRUD router #165

Closed

JayDS22 mentioned this pull request Jul 31, 2026

test(crud): add GET list envelope + GET :id + POST happy-path coverage (takeover of #165) #226

Merged

4 tasks

Navigation Menu

Uh oh!

Uh oh!

fix(crud): reject non-strict-numeric :id up-front (closes #162) - #208

fix(crud): reject non-strict-numeric :id up-front (closes #162)#208
niallroche merged 2 commits into
accordproject:mainfrom
JayDS22:jay/fix/crud-numeric-id-strict-parse

JayDS22 commented Jul 11, 2026

Uh oh!

apoorv7g left a comment

Uh oh!

apoorv7g Jul 12, 2026

Uh oh!

JayDS22 Jul 12, 2026

Uh oh!

apoorv7g Jul 12, 2026

Uh oh!

JayDS22 commented Jul 12, 2026

Uh oh!

apoorv7g commented Jul 12, 2026

Uh oh!

JayDS22 commented Jul 12, 2026

Uh oh!

apoorv7g Jul 12, 2026

Uh oh!

niallroche left a comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Conversation

JayDS22 commented Jul 11, 2026

Summary

What this PR does

Tests

Also in this PR

Validation

Non-goals

Author Checklist

Uh oh!

apoorv7g left a comment

Choose a reason for hiding this comment

Uh oh!

apoorv7g Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

JayDS22 Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

apoorv7g Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

JayDS22 commented Jul 12, 2026

Uh oh!

apoorv7g commented Jul 12, 2026

Uh oh!

JayDS22 commented Jul 12, 2026

Uh oh!

apoorv7g Jul 12, 2026

Choose a reason for hiding this comment

Uh oh!

niallroche left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Jul 13, 2026

🎉 Thank you for your contribution! 🎉

What happens next?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants