Merged
Conversation
Adds a set of tools for interacting with the ACC Submittals API, including raw request, item listing, package listing, specs listing, and attachment retrieval. Provides helper functions for constructing API paths, summarizing responses, and validating input. Also includes quick-reference documentation for the Submittals API.
WalkthroughAdds seven ACC Submittals tools and server handlers, plus a new submittals helper module with types, validators, path builders, response summarizers, and quick-reference docs; manifest and build scripts updated to include the new helper entry. Changes
Sequence DiagramsequenceDiagram
participant Client
participant MCP_Server as MCP Server
participant Validator as Validator
participant ACC_API as ACC Submittals API
participant Summarizer as Response Summarizer
Client->>MCP_Server: Invoke tool (e.g., aps_list_submittal_items)
MCP_Server->>Validator: validate project_id / path / params
Validator-->>MCP_Server: validation result
alt Validation passes
MCP_Server->>ACC_API: HTTP request (with auth token)
ACC_API-->>MCP_Server: raw JSON response
MCP_Server->>Summarizer: summarize response
Summarizer-->>MCP_Server: typed summary + pagination
MCP_Server-->>Client: structured response
else Validation fails
MCP_Server-->>Client: error (invalid input)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 712-783: The handlers aps_get_submittal_item and
aps_get_submittal_item_attachments currently interpolate raw args.item_id into
submittalPath, which can produce malformed paths for values with "/" or other
reserved chars; URL-encode the itemId (e.g., const itemId =
encodeURIComponent(args.item_id as string)) before calling submittalPath (and
keep existing validateSubmittalItemId checks), so both submittalPath(projectId,
`items/${itemId}`) and submittalPath(projectId, `items/${itemId}/attachments`)
use the encoded id.
🧹 Nitpick comments (1)
src/aps-helpers.ts (1)
551-568: Encode the project ID insubmittalPathfor safer URL construction.Even if project IDs are expected to be UUIDs, encoding/trim guards against stray whitespace or unexpected characters that would break the path.
🔧 Suggested change
export function submittalPath(projectId: string, subPath: string): string { - const pid = toAccProjectId(projectId); + const pid = encodeURIComponent(toAccProjectId(projectId).trim()); const sub = subPath.replace(/^\//, ""); return `${SUBMITTALS_BASE}/projects/${pid}/${sub}`; }
Resolves merge conflicts by combining main's changes (3-legged OAuth, Issues API tools, aps-helpers rename to aps-dm-helpers) with the Submittals API tools from this branch. - Extracted submittal helpers into aps-submittals-helpers.ts - Added submittal tools and handlers alongside issues tools in index.ts - Combined both tool sets in manifest.json
Includes the submittals helper functions in the server build process. This ensures that the server component has access to necessary utilities for handling submittals logic.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/aps-dm-helpers.ts (1)
491-705: Consolidate submittals helpers to avoid divergent duplicates.These helpers appear duplicated in
src/aps-submittals-helpers.ts. Keeping two copies risks drift and inconsistent behavior. Consider re-exporting from a single module (or moving this block into the dedicated submittals helper) and importing from there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aps-dm-helpers.ts` around lines 491 - 705, The submittals helper implementations (e.g., toAccProjectId, submittalPath, summarizeSubmittalItems, summarizeSubmittalPackages, summarizeSubmittalSpecs, summarizeSubmittalAttachments) are duplicated in src/aps-submittals-helpers.ts; consolidate by keeping a single canonical module and re-exporting or importing it: move these functions/interfaces into the dedicated submittals helper module (or delete the duplicate block here) and update callers to import from that single module so behavior is not duplicated across files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aps-dm-helpers.ts`:
- Around line 710-718: The current validateSubmittalProjectId and
validateSubmittalItemId only check for non-empty values; harden them to reject
path traversal and malformed IDs by returning an error if the string contains
'/', '\\', '%2F', or the segment '..' and (optionally) validate against a UUID
regex (e.g. 8-4-4-4-12 hex pattern) for accepted formats; update
validateSubmittalProjectId to accept either the DM form like "b.uuid" (allow a
single dot but still disallow traversal tokens) or a plain UUID, and update
validateSubmittalItemId similarly, ensuring both functions return descriptive
error strings when invalid.
In `@src/aps-submittals-helpers.ts`:
- Around line 221-230: Harden validateSubmittalProjectId and
validateSubmittalItemId to reject path-traversal or malformed IDs by explicitly
checking for forbidden substrings and enforcing UUID form: in
validateSubmittalProjectId, return an error if id contains "/" or "..", then
accept either a plain UUID (use a UUID v4 regex) or the "b." prefixed UUID
pattern and return an error if neither matches; in validateSubmittalItemId,
return an error if id contains "/" or ".." and require a plain UUID format (or
adjust to accept "b."+UUID if items use that), updating the error messages to be
specific (e.g., "project_id must be a UUID or b.<uuid> and must not contain '/'
or '..'") and referencing the functions validateSubmittalProjectId and
validateSubmittalItemId.
---
Duplicate comments:
In `@src/index.ts`:
- Around line 1506-1517: The code in the aps_get_submittal_item handler uses raw
itemId when calling submittalPath(`items/${itemId}`), which can break URLs if
item_id contains reserved characters; update the handler to URL-encode the item
id (e.g., use encodeURIComponent on itemId) before interpolating into
submittalPath so the GET path is safe; apply the same change to the other
handler(s) that build submittal paths with itemId (the other occurrence that
constructs `items/${itemId}`) to ensure consistency.
---
Nitpick comments:
In `@src/aps-dm-helpers.ts`:
- Around line 491-705: The submittals helper implementations (e.g.,
toAccProjectId, submittalPath, summarizeSubmittalItems,
summarizeSubmittalPackages, summarizeSubmittalSpecs,
summarizeSubmittalAttachments) are duplicated in src/aps-submittals-helpers.ts;
consolidate by keeping a single canonical module and re-exporting or importing
it: move these functions/interfaces into the dedicated submittals helper module
(or delete the duplicate block here) and update callers to import from that
single module so behavior is not duplicated across files.
Comment on lines
+710
to
+718
| export function validateSubmittalProjectId(id: string): string | null { | ||
| if (!id) return "project_id is required."; | ||
| // Accept both 'b.uuid' (DM format) and plain UUID (ACC format) | ||
| return null; | ||
| } | ||
|
|
||
| export function validateSubmittalItemId(id: string): string | null { | ||
| if (!id) return "item_id is required."; | ||
| return null; |
There was a problem hiding this comment.
Harden submittal ID validation to prevent path traversal / malformed URLs.
project_id and item_id are interpolated into request paths, but validation only checks non‑empty. Reject path separators or .., and (optionally) enforce UUID formats.
🔒 Proposed validation hardening
export function validateSubmittalProjectId(id: string): string | null {
if (!id) return "project_id is required.";
- // Accept both 'b.uuid' (DM format) and plain UUID (ACC format)
+ if (id.includes("..") || id.includes("/")) return "project_id must not contain '/' or '..'.";
+ // Accept both 'b.uuid' (DM format) and plain UUID (ACC format)
+ if (!/^b\.[0-9a-f-]+$/i.test(id) && !/^[0-9a-f-]+$/i.test(id)) {
+ return "project_id must be a UUID (with optional 'b.' prefix).";
+ }
return null;
}
export function validateSubmittalItemId(id: string): string | null {
if (!id) return "item_id is required.";
+ if (id.includes("..") || id.includes("/")) return "item_id must not contain '/' or '..'.";
+ if (!/^[0-9a-f-]+$/i.test(id)) return "item_id must be a UUID.";
return null;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/aps-dm-helpers.ts` around lines 710 - 718, The current
validateSubmittalProjectId and validateSubmittalItemId only check for non-empty
values; harden them to reject path traversal and malformed IDs by returning an
error if the string contains '/', '\\', '%2F', or the segment '..' and
(optionally) validate against a UUID regex (e.g. 8-4-4-4-12 hex pattern) for
accepted formats; update validateSubmittalProjectId to accept either the DM form
like "b.uuid" (allow a single dot but still disallow traversal tokens) or a
plain UUID, and update validateSubmittalItemId similarly, ensuring both
functions return descriptive error strings when invalid.
Adds validation to `project_id` and `item_id` to prevent directory traversal attacks and ensures that `item_id` is properly encoded for the API request. This change enhances security and ensures that the `item_id` is correctly formatted when making requests to the APS DM API.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/index.ts (3)
1475-1481: Consider setting Content-Type only for POST requests with a body.Currently, the
Content-Type: application/jsonheader is set unconditionally. For GET requests without a body, this header is unnecessary. The Issues handler (aps_issues_request, lines 1210-1215) conditionally setsContent-Typeonly for POST/PATCH with a body.♻️ Align with Issues handler pattern
+ const headers: Record<string, string> = {}; + if (method === "POST" && body !== undefined) { + headers["Content-Type"] = "application/json"; + } + const data = await apsDmRequest( method as "GET" | "POST" | "PATCH" | "DELETE", fullPath, t, - { query, body, headers: { "Content-Type": "application/json" } }, + { query, body, headers }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1475 - 1481, The request always sets headers: { "Content-Type": "application/json" } unconditionally; change the call to apsDmRequest so that the Content-Type header is only included when the outgoing request has a body (e.g., method is "POST" or "PATCH" and body is truthy). Update the headers argument passed to apsDmRequest in the block that builds the call with method, fullPath, t, { query, body, headers: ... } to conditionally include "Content-Type": "application/json" (or omit headers entirely) when there is no body, mirroring the pattern used in aps_issues_request.
1499-1502: Content-Type header is unnecessary for GET requests.The
Content-Typeheader is typically used for requests with a body (POST, PATCH, PUT). For GET requests, this header is ignored by most servers. While not harmful, removing it from GET-only calls would align with HTTP conventions and reduce noise.♻️ Suggested cleanup for GET requests
const raw = await apsDmRequest("GET", submittalPath(projectId, "items"), t, { query, - headers: { "Content-Type": "application/json" }, });Apply similar changes to other GET requests in this section.
Also applies to: 1517-1519, 1535-1538, 1554-1557, 1572-1577
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 1499 - 1502, Remove the unnecessary "Content-Type": "application/json" header from GET requests that call apsDmRequest with submittalPath (e.g., the call assigning const raw via apsDmRequest("GET", submittalPath(projectId, "items"), ...)) and the other listed GET calls (lines referenced near 1517-1519, 1535-1538, 1554-1557, 1572-1577); update each apsDmRequest("GET", ...) invocation to omit the headers property or remove the Content-Type key so only requests with a body (POST/PATCH/PUT) continue to send that header.
826-830: Add PATCH method to aps_submittals_request for consistency.The
aps_submittals_requesttool currently supports onlyGETandPOSTmethods, while the equivalent raw toolsaps_dm_requestandaps_issues_requestsupportGET,POST,PATCH, andDELETE. The Submittals API v2 supports PATCH operations for updating submittal items and attachments, so adding it to the enum would improve consistency and enable full API coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 826 - 830, The aps_submittals_request tool's method enum only lists "GET" and "POST" but needs to include "PATCH" to match aps_dm_request and aps_issues_request and support Submittals API v2 updates; update the method schema for aps_submittals_request (the object with property name method) to add "PATCH" to the enum array (so it becomes ["GET","POST","PATCH"]) and ensure any validation/usage that checks allowed methods (e.g., code that reads method from aps_submittals_request) accepts "PATCH" as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/index.ts`:
- Around line 1475-1481: The request always sets headers: { "Content-Type":
"application/json" } unconditionally; change the call to apsDmRequest so that
the Content-Type header is only included when the outgoing request has a body
(e.g., method is "POST" or "PATCH" and body is truthy). Update the headers
argument passed to apsDmRequest in the block that builds the call with method,
fullPath, t, { query, body, headers: ... } to conditionally include
"Content-Type": "application/json" (or omit headers entirely) when there is no
body, mirroring the pattern used in aps_issues_request.
- Around line 1499-1502: Remove the unnecessary "Content-Type":
"application/json" header from GET requests that call apsDmRequest with
submittalPath (e.g., the call assigning const raw via apsDmRequest("GET",
submittalPath(projectId, "items"), ...)) and the other listed GET calls (lines
referenced near 1517-1519, 1535-1538, 1554-1557, 1572-1577); update each
apsDmRequest("GET", ...) invocation to omit the headers property or remove the
Content-Type key so only requests with a body (POST/PATCH/PUT) continue to send
that header.
- Around line 826-830: The aps_submittals_request tool's method enum only lists
"GET" and "POST" but needs to include "PATCH" to match aps_dm_request and
aps_issues_request and support Submittals API v2 updates; update the method
schema for aps_submittals_request (the object with property name method) to add
"PATCH" to the enum array (so it becomes ["GET","POST","PATCH"]) and ensure any
validation/usage that checks allowed methods (e.g., code that reads method from
aps_submittals_request) accepts "PATCH" as well.
Adds validation to ensure project and item IDs are properly formatted UUIDs and do not contain path traversal tokens. This prevents potential security vulnerabilities and ensures data integrity.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/aps-submittals-helpers.ts (1)
221-255: Consider blocking percent‑encoded traversal invalidateSubmittalPath.
It currently only checks literal... If raw paths are user-supplied, inputs like%2e%2ecould bypass the check. Adding a case‑insensitive%2e/%2fguard would harden it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aps-submittals-helpers.ts` around lines 221 - 255, validateSubmittalPath currently only checks for literal ".." so percent‑encoded traversal like "%2e%2e" can bypass it; update validateSubmittalPath to also detect percent‑encoded traversal by normalizing the path to lowercase and either checking for "%2e", "%2f" and "%5c" sequences (and "%2e%2e" specifically) or safer: attempt a decodeURIComponent(path) in a try/catch and then run the existing checks (e.g., test decoded for ".." or "/"/backslash), ensuring you still validate the original type and return the same error string when traversal is found; reference validateSubmittalPath and containsTraversalTokens when adding these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/aps-submittals-helpers.ts`:
- Around line 221-255: validateSubmittalPath currently only checks for literal
".." so percent‑encoded traversal like "%2e%2e" can bypass it; update
validateSubmittalPath to also detect percent‑encoded traversal by normalizing
the path to lowercase and either checking for "%2e", "%2f" and "%5c" sequences
(and "%2e%2e" specifically) or safer: attempt a decodeURIComponent(path) in a
try/catch and then run the existing checks (e.g., test decoded for ".." or
"/"/backslash), ensuring you still validate the original type and return the
same error string when traversal is found; reference validateSubmittalPath and
containsTraversalTokens when adding these checks.
|
🎉 This PR is included in version 1.2.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds a set of tools for interacting with the ACC Submittals API, including raw request, item listing, package listing, specs listing, and attachment retrieval.
Provides helper functions for constructing API paths, summarizing responses, and validating input.
Also includes quick-reference documentation for the Submittals API.
Summary by CodeRabbit
New Features
Documentation