Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds interactive 3LO authentication (login/logout with configurable callback port), a complete ACC Issues toolset (raw and helper methods), a new issues helpers module (models, validators, summarizers), wiring into the main tool bundle, and packaging updates to include helper entry files. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ToolHandler as Tool Handler
participant Auth as Auth (3LO)
participant API as APS Issues API
participant Summarizer as Summarizer
Client->>ToolHandler: invoke aps_issues_list(projectId, filters)
ToolHandler->>Auth: getValid3loToken() or use 2LO token
Auth-->>ToolHandler: access_token
ToolHandler->>API: GET /construction/issues/v1/projects/{id}/issues (with headers, token)
API-->>ToolHandler: raw issues list
ToolHandler->>Summarizer: summarizeIssuesList(raw)
Summarizer-->>ToolHandler: IssueSummary[]
ToolHandler-->>Client: return paginated issues
sequenceDiagram
participant User
participant LoginTool as aps_login
participant Browser
participant CallbackSrv as Local Callback Server
participant AuthSrv as APS Auth Server
User->>LoginTool: performAps3loLogin(clientId, clientSecret, scope)
LoginTool->>Browser: open authorization URL
Browser->>AuthSrv: user consents -> redirect to callback URL
AuthSrv->>CallbackSrv: GET/POST /callback?code=...
CallbackSrv->>LoginTool: deliver authorization code
LoginTool->>AuthSrv: exchange code for tokens
AuthSrv-->>LoginTool: access_token + refresh_token
LoginTool->>LoginTool: persist tokens to cache
LoginTool-->>User: return access_token + message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (1 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 1018-1044: Add a validation that if args.assigned_to is provided
then args.assigned_to_type must also be provided: check args.assigned_to and if
truthy and args.assigned_to_type is missing, call fail("assigned_to_type is
required when assigned_to is provided."); then only set body.assignedTo and
body.assignedToType when both exist (or set assignedToType separately if
present). Apply the same check and behavior in both APS handlers referenced
(aps_issues_create and aps_issues_update) around the code that builds the body
and sets assignedTo/assignedToType.
🧹 Nitpick comments (1)
src/index.ts (1)
894-904: Consider hoisting Issues header helpers out ofhandleTool.This avoids re‑creating the functions on every call and keeps helpers with the other shared utilities.
♻️ Possible refactor
@@ function json(obj: unknown) { return ok(JSON.stringify(obj, null, 2)); } +/** Build optional headers for Issues API calls. */ +function issuesHeaders(region?: string): Record<string, string> { + const h: Record<string, string> = {}; + if (region) h["x-ads-region"] = region; + return h; +} + +/** Build headers for Issues API write operations (POST/PATCH). */ +function issuesWriteHeaders(region?: string): Record<string, string> { + return { "Content-Type": "application/json", ...issuesHeaders(region) }; +} + /** Format an ApsApiError with troubleshooting context. */ function richError(err: ApsApiError) { @@ - /** Build optional headers for Issues API calls. */ - function issuesHeaders(region?: string): Record<string, string> { - const h: Record<string, string> = {}; - if (region) h["x-ads-region"] = region; - return h; - } - - /** Build headers for Issues API write operations (POST/PATCH). */ - function issuesWriteHeaders(region?: string): Record<string, string> { - return { "Content-Type": "application/json", ...issuesHeaders(region) }; - }
Adds the new helper files to the server directory. This enables new features related to issues and direct messaging.
Ensures `assigned_to_type` is required when `assigned_to` is provided. Allows `assigned_to_type` to be set independently.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/index.ts`:
- Around line 1121-1124: The limit parameter in aps_issues_get_comments is not
validated like in aps_issues_list; update the handling of args.limit (used to
build query) to enforce the same 1–100 bounds (e.g., clamp or reject values
outside that range) before assigning query.limit = String(args.limit), so
behavior is consistent with aps_issues_list; make the change in the
aps_issues_get_comments code path that builds the query object (referencing
args.limit and query) and preserve the existing String conversion.
🧹 Nitpick comments (2)
src/index.ts (2)
894-904: Consider extracting helper functions to module scope.
issuesHeadersandissuesWriteHeadersare defined insidehandleTool, causing them to be recreated on every invocation. Moving them to module level (alongsideok,fail,json) would be slightly more efficient and consistent with the existing helper pattern.♻️ Suggested refactor
+// ── Issues API Headers ────────────────────────────────────────── + +/** Build optional headers for Issues API calls. */ +function issuesHeaders(region?: string): Record<string, string> { + const h: Record<string, string> = {}; + if (region) h["x-ads-region"] = region; + return h; +} + +/** Build headers for Issues API write operations (POST/PATCH). */ +function issuesWriteHeaders(region?: string): Record<string, string> { + return { "Content-Type": "application/json", ...issuesHeaders(region) }; +} + // ── Tool definitions ─────────────────────────────────────────────Then remove the duplicate definitions from inside
handleTool(lines 894-904).
1144-1157: Consider renaming the variable to avoid confusion with request body.The variable
body(line 1144) represents the comment text, but the same identifier is commonly used for request bodies, making line 1155 ({ body: { body }, ... }) harder to parse at a glance.♻️ Suggested refactor for clarity
- const body = args.body as string; - if (!body) return fail("body is required."); + const commentBody = args.body as string; + if (!commentBody) return fail("body is required."); const pid = toIssuesProjectId(projectId); const region = args.region as string | undefined; const t = await token(); const raw = await apsDmRequest( "POST", `construction/issues/v1/projects/${pid}/issues/${issueId}/comments`, t, - { body: { body }, headers: issuesWriteHeaders(region) }, + { body: { body: commentBody }, headers: issuesWriteHeaders(region) }, );
Implements 3-legged OAuth flow for user-context APS access. Introduces `aps_login` and `aps_logout` tools for managing user sessions. Falls back to 2-legged authentication when no 3LO session is active.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/aps-auth.ts`:
- Around line 262-272: The error_description from
reqUrl.searchParams.get("error_description") is inserted into HTML unescaped;
sanitize it before embedding to prevent XSS. In the error handling block (where
you read error via reqUrl.searchParams.get("error") and desc is set),
HTML-escape characters like <, >, &, ", and ' in desc (or use a small escape
utility) and use the escaped value in the res.end HTML string and in the Error
message if appropriate; keep the server.close() and reject(new Error(...)) logic
the same but reference the escaped/cleaned description when rendering the
response.
🧹 Nitpick comments (4)
src/aps-auth.ts (2)
205-208: Consider restricting file permissions for the token cache.The token file contains sensitive credentials (access_token, refresh_token) but is written with default permissions, which may be world-readable depending on the system's umask. Consider explicitly setting restrictive permissions (e.g.,
0o600) to prevent other users from reading the tokens.🔒 Proposed fix
function write3loCache(data: Aps3loTokenData): void { - if (!existsSync(TOKEN_DIR)) mkdirSync(TOKEN_DIR, { recursive: true }); - writeFileSync(TOKEN_FILE, JSON.stringify(data, null, 2)); + if (!existsSync(TOKEN_DIR)) mkdirSync(TOKEN_DIR, { recursive: true, mode: 0o700 }); + writeFileSync(TOKEN_FILE, JSON.stringify(data, null, 2), { mode: 0o600 }); }
362-367: Clear the timeout on successful completion to avoid resource leak.The 2-minute timeout is not cleared when login completes successfully. While calling
reject()afterresolve()is harmless, the dangling timer is a minor resource leak.♻️ Proposed fix
+ let timeoutHandle: ReturnType<typeof setTimeout>; + server.listen(callbackPort, () => { const authUrl = new URL(APS_AUTHORIZE_URL); // ... authUrl setup ... openBrowser(authUrl.toString()); }); // Give the user 2 minutes to complete login - setTimeout(() => { + timeoutHandle = setTimeout(() => { server.close(); reject(new Error("3LO login timed out after 2 minutes. Try again.")); }, 120_000);Then clear it on success (around line 334):
server.close(); + clearTimeout(timeoutHandle); resolve({ access_token: data.access_token,And on errors (lines 269, 280, 307, 347):
server.close(); + clearTimeout(timeoutHandle); reject(...);src/index.ts (2)
77-77: Validate parsed port to handle non-numeric input gracefully.If
APS_CALLBACK_PORTis set to a non-numeric string (e.g.,"abc"),parseIntreturnsNaN, which would be passed to the HTTP server'slisten()call and could cause unexpected behavior.🛡️ Proposed fix
-const APS_CALLBACK_PORT = parseInt(process.env.APS_CALLBACK_PORT ?? "8910", 10); +const APS_CALLBACK_PORT = (() => { + const port = parseInt(process.env.APS_CALLBACK_PORT ?? "8910", 10); + return Number.isNaN(port) || port < 1 || port > 65535 ? 8910 : port; +})();
967-977: Consider moving helper functions outsidehandleTool.
issuesHeaders()andissuesWriteHeaders()are defined insidehandleTool, causing them to be recreated on every invocation. Moving them outside would be slightly more efficient and improve code organization.♻️ Proposed refactor
+// ── Issues API helpers ─────────────────────────────────────────── + +/** Build optional headers for Issues API calls. */ +function issuesHeaders(region?: string): Record<string, string> { + const h: Record<string, string> = {}; + if (region) h["x-ads-region"] = region; + return h; +} + +/** Build headers for Issues API write operations (POST/PATCH). */ +function issuesWriteHeaders(region?: string): Record<string, string> { + return { "Content-Type": "application/json", ...issuesHeaders(region) }; +} + async function handleTool( name: string, args: Record<string, unknown>, ) { // ... handlers ... - - /** Build optional headers for Issues API calls. */ - function issuesHeaders(region?: string): Record<string, string> { - // ... - } - - /** Build headers for Issues API write operations (POST/PATCH). */ - function issuesWriteHeaders(region?: string): Record<string, string> { - // ... - }
Ensures error messages displayed to the user are properly escaped to prevent potential HTML injection vulnerabilities. Limits the number of items listed in the tool's output to a maximum of 100 to avoid performance issues when retrieving large datasets. It will always be greater or equal to 1
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/aps-auth.ts`:
- Around line 314-322: The HTML response in the token exchange failure path
embeds raw `text` directly into `res.end(...)`, which can lead to XSS; update
the handler that uses `res.writeHead`, `res.end`, `server.close`, and `reject`
so that `text` (from `tokenRes`) is HTML-escaped before interpolation into the
`<pre>` block (or use an existing escape utility if one exists), then send the
escaped string in the response and include the escaped text in the Error message
passed to `reject` to keep logs consistent and safe.
- Around line 228-237: The openBrowser function currently constructs a shell
command and calls exec(cmd), which risks command injection; change it to call a
child-process API that takes an argv array (child_process.spawn or execFile)
instead of interpolating the URL into a shell string. Update openBrowser to pick
the platform-specific program name ("open", "xdg-open", or Windows via "cmd"
"/c" "start") and pass the URL as a separate argument array (no shell=true), and
replace exec(cmd) with spawn/program-exec that forwards stdout/stderr and
handles errors; keep the same function name openBrowser and use process.platform
to choose the program and args.
🧹 Nitpick comments (5)
src/aps-auth.ts (2)
215-218: Consider restricting file permissions for token cache.The token file contains sensitive OAuth credentials (access and refresh tokens). On Unix systems, the file is created with default permissions (typically 0644), which may allow other users on the system to read it.
🔒 Proposed fix to restrict permissions
function write3loCache(data: Aps3loTokenData): void { - if (!existsSync(TOKEN_DIR)) mkdirSync(TOKEN_DIR, { recursive: true }); - writeFileSync(TOKEN_FILE, JSON.stringify(data, null, 2)); + if (!existsSync(TOKEN_DIR)) mkdirSync(TOKEN_DIR, { recursive: true, mode: 0o700 }); + writeFileSync(TOKEN_FILE, JSON.stringify(data, null, 2), { mode: 0o600 }); }
373-378: Potential race condition between timeout and successful callback.If the OAuth callback arrives just as the 2-minute timeout fires, both paths may attempt to close the server and settle the promise. While this is unlikely in practice, it could lead to an unhandled rejection or unexpected behavior.
🔧 Proposed fix using a settled flag
return new Promise((resolve, reject) => { + let settled = false; const server = createServer( async (req: IncomingMessage, res: ServerResponse) => { + if (settled) { + res.writeHead(503); + res.end("Request already handled"); + return; + } // ... existing callback logic ... // Before each resolve/reject, add: settled = true; }, ); // ... server.listen ... // Give the user 2 minutes to complete login - setTimeout(() => { - server.close(); - reject(new Error("3LO login timed out after 2 minutes. Try again.")); - }, 120_000); + const timeoutId = setTimeout(() => { + if (!settled) { + settled = true; + server.close(); + reject(new Error("3LO login timed out after 2 minutes. Try again.")); + } + }, 120_000); + // Also clear timeout on successful completion (add clearTimeout(timeoutId) before resolve/reject in callback) });src/index.ts (3)
967-977: Consider moving helper functions outsidehandleTool.
issuesHeadersandissuesWriteHeadersare defined inside thehandleToolfunction, meaning they're recreated on every call. For minor optimization and better code organization, consider moving them to module scope.
1108-1115: Consider whetherassignedToTypealone is valid.The validation correctly rejects
assigned_towithoutassigned_to_type, but the code allowsassigned_to_typewithoutassigned_to(lines 1113-1115). According to the APS Issues API,assignedToTypetypically only makes sense when paired withassignedTo. SendingassignedToTypealone may be silently ignored or cause unexpected behavior.🔧 Proposed fix to reject orphaned assignedToType
if (args.assigned_to && !args.assigned_to_type) return fail("assigned_to_type is required when assigned_to is provided."); if (args.assigned_to != null && args.assigned_to_type != null) { body.assignedTo = args.assigned_to; body.assignedToType = args.assigned_to_type; - } else if (args.assigned_to_type != null) { - body.assignedToType = args.assigned_to_type; } + // Note: assignedToType without assignedTo is not sent as it's not meaningful
1194-1197: Minor inconsistency in offset sanitization.In
aps_issues_list(line 1051), offset is sanitized withString(Number(args.offset) || 0), but here it's justString(args.offset). If a non-numeric value is passed (e.g.,"abc"), it would be sent directly to the API rather than defaulting to 0.🔧 Proposed fix for consistent offset handling
const query: Record<string, string> = {}; if (args.limit != null) query.limit = String(Math.min(Math.max(Number(args.limit) || 100, 1), 100)); - if (args.offset != null) query.offset = String(args.offset); + if (args.offset != null) query.offset = String(Number(args.offset) || 0); if (args.sort_by) query.sortBy = args.sort_by as string;
| /** Open a URL in the user's default browser (cross‑platform). */ | ||
| function openBrowser(url: string): void { | ||
| const cmd = | ||
| process.platform === "win32" | ||
| ? `start "" "${url}"` | ||
| : process.platform === "darwin" | ||
| ? `open "${url}"` | ||
| : `xdg-open "${url}"`; | ||
| exec(cmd); | ||
| } |
There was a problem hiding this comment.
Shell command injection risk in openBrowser.
The URL is directly interpolated into a shell command. While the URL is constructed internally from trusted constants and sanitized query parameters, this pattern is fragile. If the URL ever contains shell metacharacters (e.g., from a malformed scope parameter with backticks or $(...)), it could lead to command injection.
🛡️ Proposed fix using spawn for safer execution
-import { exec } from "node:child_process";
+import { spawn } from "node:child_process";
/** Open a URL in the user's default browser (cross‑platform). */
function openBrowser(url: string): void {
- const cmd =
- process.platform === "win32"
- ? `start "" "${url}"`
- : process.platform === "darwin"
- ? `open "${url}"`
- : `xdg-open "${url}"`;
- exec(cmd);
+ const args = process.platform === "win32" ? ["start", "", url] : [url];
+ const cmd = process.platform === "win32"
+ ? "cmd"
+ : process.platform === "darwin"
+ ? "open"
+ : "xdg-open";
+ const opts = process.platform === "win32" ? { shell: true } : {};
+ spawn(cmd, args, { ...opts, detached: true, stdio: "ignore" }).unref();
}🤖 Prompt for AI Agents
In `@src/aps-auth.ts` around lines 228 - 237, The openBrowser function currently
constructs a shell command and calls exec(cmd), which risks command injection;
change it to call a child-process API that takes an argv array
(child_process.spawn or execFile) instead of interpolating the URL into a shell
string. Update openBrowser to pick the platform-specific program name ("open",
"xdg-open", or Windows via "cmd" "/c" "start") and pass the URL as a separate
argument array (no shell=true), and replace exec(cmd) with spawn/program-exec
that forwards stdout/stderr and handles errors; keep the same function name
openBrowser and use process.platform to choose the program and args.
| res.writeHead(500, { "Content-Type": "text/html" }); | ||
| res.end( | ||
| `<html><body><h2>Token exchange failed</h2><pre>${text}</pre></body></html>`, | ||
| ); | ||
| server.close(); | ||
| reject( | ||
| new Error(`Token exchange failed (${tokenRes.status}): ${text}`), | ||
| ); | ||
| return; |
There was a problem hiding this comment.
Escape error text in token exchange failure response.
The text from the token exchange failure response is rendered directly into HTML without escaping, similar to the issue that was fixed for error_description.
🛡️ Proposed fix
if (!tokenRes.ok) {
const text = await tokenRes.text();
res.writeHead(500, { "Content-Type": "text/html" });
res.end(
- `<html><body><h2>Token exchange failed</h2><pre>${text}</pre></body></html>`,
+ `<html><body><h2>Token exchange failed</h2><pre>${escapeHtml(text)}</pre></body></html>`,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| res.writeHead(500, { "Content-Type": "text/html" }); | |
| res.end( | |
| `<html><body><h2>Token exchange failed</h2><pre>${text}</pre></body></html>`, | |
| ); | |
| server.close(); | |
| reject( | |
| new Error(`Token exchange failed (${tokenRes.status}): ${text}`), | |
| ); | |
| return; | |
| res.writeHead(500, { "Content-Type": "text/html" }); | |
| res.end( | |
| `<html><body><h2>Token exchange failed</h2><pre>${escapeHtml(text)}</pre></body></html>`, | |
| ); | |
| server.close(); | |
| reject( | |
| new Error(`Token exchange failed (${tokenRes.status}): ${text}`), | |
| ); | |
| return; |
🤖 Prompt for AI Agents
In `@src/aps-auth.ts` around lines 314 - 322, The HTML response in the token
exchange failure path embeds raw `text` directly into `res.end(...)`, which can
lead to XSS; update the handler that uses `res.writeHead`, `res.end`,
`server.close`, and `reject` so that `text` (from `tokenRes`) is HTML-escaped
before interpolation into the `<pre>` block (or use an existing escape utility
if one exists), then send the escaped string in the response and include the
escaped text in the Error message passed to `reject` to keep logs consistent and
safe.
Sanitizes the error message displayed to the user and included in the error object when the token exchange fails. This prevents potential cross-site scripting (XSS) vulnerabilities by escaping HTML entities in the error text.
Replaces `exec` with `spawn` for opening URLs in the default browser to avoid potential command injection vulnerabilities and improve reliability. The previous implementation used `exec`, which could be vulnerable if the URL contained malicious characters. `spawn` offers better control over the process and avoids shell interpretation.
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
Summary by CodeRabbit
New Features
Chores