Security Review Remediations by smar-sean-sekora · Pull Request #91 · smartsheet-platform/smar-mcp

smar-sean-sekora · 2026-03-27T18:22:29Z

Pull Request

Description

Related Issue

Type of Change

How Has This Been Tested?

Checklist

My code follows the code style of this project
I have added tests that prove my fix is effective or that my feature works
I have updated the documentation accordingly
My changes generate no new warnings or errors

…p-node

…ons in CI and release workflows

Copilot

Pull request overview

Implements a set of security-focused remediations across the Smartsheet MCP server, including stricter input validation, safer request construction/logging, updated dependencies/tooling, and enabling CI test execution.

Changes:

Add Zod validation hardening for tool inputs (numeric IDs, email format) and constrain sheet cell value types.
Improve Smartsheet API client safety: enforce HTTPS endpoint, avoid logging query strings/base URL, sanitize error logging, cap rate-limit backoff, and pass query params via request(..., queryParams).
Add Jest tests + CI workflow, update GitHub Actions usage, and refresh dependency/toolchain versions.

Reviewed changes

Copilot reviewed 15 out of 19 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
tsconfig.json	Excludes `__tests__` from TypeScript compilation.
src/tools/smartsheet-workspace-tools.ts	Minor handler signature cleanup.
src/tools/smartsheet-user-tools.ts	Add numeric ID validation for `userId`.
src/tools/smartsheet-update-request-tools.ts	Add numeric ID + email validation.
src/tools/smartsheet-sheet-tools.ts	Add numeric ID validation; restrict cell `value` types; tweak URL parsing regex.
src/tools/smartsheet-search-tools.ts	Add numeric ID validation; tweak URL parsing regex.
src/tools/smartsheet-folder-tools.ts	Add numeric ID validation for folder/workspace IDs.
src/tools/smartsheet-discussion-tools.ts	Add numeric ID validation for sheet/row IDs.
src/tools/tests/smartsheet-sheet-tools.test.ts	New tests verifying cell value validation behavior.
src/apis/smartsheet-search-api.ts	Use `request(..., queryParams)` instead of interpolating query strings.
src/apis/smartsheet-api.ts	Enforce HTTPS base URL, sanitize logs, cap backoff delay, reduce request log sensitivity.
src/apis/tests/smartsheet-api.test.ts	New tests for HTTPS enforcement and token/query/baseURL leakage prevention.
package.json	Update engines/deps/devDeps; add Jest typings.
jest.config.js	Disable ts-jest diagnostics.
.gitignore	Ignore Claude plans directory.
.github/workflows/release.yml	Pin actions to SHAs, bump Node version, run tests during release.
.github/workflows/ci.yml	New CI workflow running lint/typecheck/build/test.
.eslintrc.json	Add ESLint configuration for TypeScript.

Comments suppressed due to low confidence (1)

package.json:72

@types/jest is ^30.0.0 while jest is ^29.5.0. This major-version mismatch can introduce incompatible typings and typecheck failures. Align @types/jest to the same major as jest (or upgrade Jest to match).

		"@types/jest": "^30.0.0",
		"@types/node": "^22.13.10",
		"@typescript-eslint/eslint-plugin": "^8.0.0",
		"@typescript-eslint/parser": "^8.0.0",
		"eslint": "^8.57.0",
		"husky": "^8.0.3",
		"jest": "^29.5.0",
		"lint-staged": "^15.2.0",

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-30T18:27:57Z

      throw new Error('SMARTSHEET_ENDPOINT environment variable is not set');
    }
+
+    if (!this.baseUrl.startsWith('https://')) {


The HTTPS enforcement uses a case-sensitive startsWith('https://') check. Valid HTTPS URLs with an uppercase or mixed-case scheme (which are still valid per URL parsing) will be rejected. Consider validating with new URL(baseUrl) and checking url.protocol === 'https:' instead; it’s more robust and avoids rejecting otherwise-valid configuration.

Suggested change

if (!this.baseUrl.startsWith('https://')) {

let parsedUrl: URL;

try {

parsedUrl = new URL(this.baseUrl);

} catch {

throw new Error('SMARTSHEET_ENDPOINT is not a valid URL');

}

if (parsedUrl.protocol !== 'https:') {

Copilot · 2026-03-30T18:27:58Z

 	"engines": {
-		"node": ">=16.0.0",
-		"npm": ">=7.0.0"
+		"node": ">=25.8.2",


The engines field now requires Node >=25.8.2, which is a breaking change for consumers and may also be stricter than your CI configuration (workflows currently use node-version: 25). Consider either relaxing the engine constraint (e.g., >=25) or pinning CI to 25.8.2 (or higher) so installs don’t fail due to an engines mismatch.

Suggested change

"node": ">=25.8.2",

"node": ">=25",

Copilot · 2026-03-30T18:27:58Z

+        uses: actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6
        with:
-          node-version: 22
+          node-version: 25


This workflow uses node-version: 25, but package.json requires Node >=25.8.2. If setup-node resolves to an earlier 25.x release, installs can fail due to the engines check. Consider pinning the workflow to 25.8.2 (or higher) or relaxing the engine requirement.

Suggested change

node-version: 25

node-version: 25.8.2

Copilot · 2026-03-30T18:27:59Z

+      - name: Setup Node.js
+        uses: actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6
+        with:
+          node-version: 25


This workflow uses node-version: 25, but package.json requires Node >=25.8.2. If setup-node installs an earlier 25.x version, npm ci may fail because of the engines constraint. Pin to 25.8.2 (or higher) or relax the engine requirement to match CI.

Suggested change

node-version: 25

node-version: 25.8.2

fix: use queryParams to prevent query parameter injection in search API

07aac0b

smar-sean-sekora changed the title ~~fix: use queryParams to prevent query parameter injection in search API~~ fix: Security Review Remediations Mar 27, 2026

smar-sean-sekora changed the title ~~fix: Security Review Remediations~~ Security Review Remediations Mar 27, 2026

smar-sean-sekora added 17 commits March 27, 2026 14:35

fix: add numeric regex validation to all ID parameters

de8501d

fix: add email format validation to update request recipients

8974aa9

fix: sanitize error and request logs to prevent token leakage

8f33b92

ci: add PR workflow for build and test

86e173c

fix: add @types/jest and exclude tests from build

58b7bd9

chore: add eslint config and fix lint errors

9ec3cc5

ci: opt into Node.js 24 for GitHub Actions

00ef4b0

ci: update GitHub Actions to use latest versions of checkout and setu…

f03b479

…p-node

fix: replace z.any() with typed union for cell values

8ca1d5d

chore: bump Node.js to 25 and update dependencies

94e1b60

chore: update @typescript-eslint packages to version 8.0.0

9da4fbc

fix: enforce HTTPS for Smartsheet API endpoint

985b601

test: add HTTPS enforcement edge-case tests

5742bec

ci: enable tests in release workflow

72e0630

test: add strict equality checks for SmartsheetAPI constructor

e3bd6ca

fix: use strict equality checks for access token and base URL validation

fcde296

fix: update actions/checkout and actions/setup-node to specific versi…

9d2d93c

…ons in CI and release workflows

smar-sean-sekora marked this pull request as ready for review March 28, 2026 18:44

smar-sean-sekora requested a review from Copilot March 30, 2026 18:23

Copilot started reviewing on behalf of smar-sean-sekora March 30, 2026 18:23 View session

fix: bump version to 1.6.1 in package.json and package-lock.json

e55ec6f

Copilot AI reviewed Mar 30, 2026

View reviewed changes

smar-sean-sekora merged commit 2796b7b into main Mar 31, 2026
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Security Review Remediations#91

Security Review Remediations#91
smar-sean-sekora merged 19 commits into
mainfrom
security-review-remediations

smar-sean-sekora commented Mar 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 30, 2026

Uh oh!

Copilot AI Mar 30, 2026

Uh oh!

Copilot AI Mar 30, 2026

Uh oh!

Copilot AI Mar 30, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-    if (!this.baseUrl.startsWith('https://')) {
+    let parsedUrl: URL;
+    try {
+      parsedUrl = new URL(this.baseUrl);
+    } catch {
+      throw new Error('SMARTSHEET_ENDPOINT is not a valid URL');
+    }
+    if (parsedUrl.protocol !== 'https:') {

Uh oh!

Conversation

smar-sean-sekora commented Mar 27, 2026

Pull Request

Description

Related Issue

Type of Change

How Has This Been Tested?

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants