fix: stricter validation on path parameters

[merll]

No description provided.

[Copilot]

🟡 Changes recommended

Invalid identifier validation currently throws a generic Error that will surface as a 500 instead of a client error, which is not acceptable for request/path validation behavior.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR tightens identifier validation across the API surface by reusing shared OpenAPI schema definitions, removing ad-hoc URL decoding in handlers, and adding runtime validation to filesystem path construction to prevent unsafe identifiers.

Changes:

• Updated multiple OpenAPI parameters to reference shared schemas (e.g., definitions.yaml#/idName, #/repoUrl, #/annotation) for stricter validation.
• Added idName regex validation to file path builders in file-map.ts, with a new Jest test suite covering path traversal-style inputs.
• Removed decodeURIComponent(...) usage from many handlers to rely on upstream decoding + stricter validation, and adjusted error middleware messaging for pattern validation errors.

File summaries

File	Description
src/openapi/api.yaml	Replaces many type: string parameters with shared schema $refs for stricter validation.
src/middleware/error.ts	Special-cases pattern validation errors to surface the validator’s message.
src/fileStore/file-map.ts	Adds an idName regex and validates identifiers before building resource file paths.
src/fileStore/file-map.test.ts	Adds unit tests validating correct paths and rejecting traversal-like/invalid identifiers.
src/api/v2/teams/{teamId}/workloads/{workloadName}.ts	Removes manual decoding of teamId/workloadName before calling req.otomi.
src/api/v2/teams/{teamId}/workloads.ts	Removes manual decoding of teamId.
src/api/v2/teams/{teamId}/services/{serviceName}.ts	Removes manual decoding of teamId/serviceName.
src/api/v2/teams/{teamId}/services.ts	Removes manual decoding of teamId.
src/api/v2/teams/{teamId}/sealedsecrets/{sealedSecretName}.ts	Removes manual decoding of teamId/sealedSecretName.
src/api/v2/teams/{teamId}/sealedsecrets.ts	Removes manual decoding of teamId.
src/api/v2/teams/{teamId}/policies/{policyName}.ts	Removes manual decoding of teamId/policyName.
src/api/v2/teams/{teamId}/policies.ts	Removes manual decoding of teamId.
src/api/v2/teams/{teamId}/netpols/{netpolName}.ts	Removes manual decoding of teamId/netpolName.
src/api/v2/teams/{teamId}/netpols.ts	Removes manual decoding of teamId.
src/api/v2/teams/{teamId}/coderepos/{codeRepositoryName}.ts	Removes manual decoding of teamId/codeRepositoryName.
src/api/v2/teams/{teamId}/coderepos.ts	Removes manual decoding of teamId.
src/api/v2/teams/{teamId}/builds/{buildName}.ts	Removes manual decoding of teamId/buildName.
src/api/v2/teams/{teamId}/builds.ts	Removes manual decoding of teamId.
src/api/v2/namespaces/{namespace}/sealedsecrets/{sealedSecretName}.ts	Removes manual decoding of namespace/sealedSecretName.
src/api/v2/namespaces/{namespace}/sealedsecrets.ts	Removes manual decoding of namespace.
src/api/v2/catalogs/{catalogId}/charts/{chartName}.ts	Removes manual decoding of catalogId/chartName.
src/api/v2/catalogs/{catalogId}/charts.ts	Removes manual decoding of catalogId.
src/api/v2/catalogs/{catalogId}.ts	Removes manual decoding of catalogId.
src/api/v1/users/{userId}.ts	Removes manual decoding of userId.
src/api/v1/teams/{teamId}/policies/{policyName}.ts	Removes manual decoding of teamId/policyName.
src/api/alpha/teams/{teamId}/kb/{knowledgeBaseName}.ts	Removes manual decoding of teamId/knowledgeBaseName.
src/api/alpha/teams/{teamId}/kb.ts	Removes manual decoding of teamId.
src/api/alpha/teams/{teamId}/agents/{agentName}.ts	Removes manual decoding of teamId/agentName.
src/api/alpha/teams/{teamId}/agents.ts	Removes manual decoding of teamId.

Review details

• Files reviewed: 30/30 changed files
• Comments generated: 1
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Changes recommended

A few new validation/error-handling changes can produce incorrect 500/undefined-error responses and one OpenAPI constraint is too strict for the secret query parameter.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (3)

src/fileStore/file-map.ts:244

• validateIdParameters throws a plain Error, which will be surfaced by errorMiddleware as a 500 (since no code/status is attached). For invalid path/name identifiers this should be a client error (400), otherwise callers get an internal error response for bad input.

export function validateIdParameters(idNames: Array<string | undefined>) {
  for (const idName of idNames) {
    if (idName !== undefined && !ID_NAME_PATTERN.test(idName)) {
      throw new Error(`${idName} is not a valid identifier`)
    }
  }

src/middleware/error.ts:35

• The pattern.openapi.validation branch forces msg = e.message, which can be undefined for some validator error shapes (where the details only exist under e.errors[*].message). In that case the response becomes { error: undefined } and you also skip the existing fallback that joins e.errors messages.

    if (errorCode === 'pattern.openapi.validation') {
      msg = e.message
    } else if (errorCode.includes('openapi.requestValidation') || errorCode.includes('request')) {

src/openapi/api.yaml:1810

• secret in /v2/testRepoConnectPlatform is passed through as a secret/token value (see handler usage), so constraining it to idName will reject common secrets containing characters outside [a-z0-9-]. This should remain an unconstrained string (or use a dedicated secret schema), while secretName can stay as idName.

        - name: secret
          in: query
          schema:
            $ref: 'definitions.yaml#/idName'

• Files reviewed: 30/30 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟡 Changes recommended

OpenAPI now incorrectly constrains the secret value for /v2/testRepoConnectPlatform, and validateIdParameters currently throws a 500/error message that can reflect untrusted input.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (2)

src/fileStore/file-map.ts:244

• validateIdParameters throws a generic Error that (a) will default to HTTP 500 in errorMiddleware and (b) reflects the invalid identifier value back to the caller via e.message. Since these values can come from request path params, this should be treated as a client error (400) and avoid echoing untrusted input.

export function validateIdParameters(idNames: Array<string | undefined>) {
  for (const idName of idNames) {
    if (idName !== undefined && !ID_NAME_PATTERN.test(idName)) {
      throw new Error(`${idName} is not a valid identifier`)
    }
  }

src/openapi/api.yaml:1811

• The /v2/testRepoConnectPlatform handler treats the secret query parameter as the secret value (it is passed directly to getTestRepoConnect), but this OpenAPI change constrains it to idName (DNS-label style). That will reject most valid secrets/tokens and break existing clients.

        - name: secret
          in: query
          schema:
            $ref: 'definitions.yaml#/idName'
      responses:

• Files reviewed: 31/31 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

🟢 Ready to approve

The changes consistently enforce stricter identifier validation and add targeted regression coverage, with only a minor OpenAPI description typo noted.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

src/openapi/api.yaml:2175

• Typo in parameter description: "Namspace" should be "Namespace".

      description: Namspace to write file under in manifest

• Files reviewed: 31/31 changed files
• Comments generated: 0 new
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/openapi/api.yaml:1810

• The secret query parameter for GET /v2/testRepoConnectPlatform is validated as idName, but the handler passes it through as an arbitrary secret/token string (req.query.secret || ''). Restricting it to ^[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?$ will incorrectly reject valid secrets containing other characters. Use an unconstrained string schema here (or a dedicated secret/token schema) instead of idName.

        - name: secret
          in: query
          schema:
            $ref: 'definitions.yaml#/idName'

[Copilot]

Pull request overview

Copilot reviewed 31 out of 31 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/openapi/api.yaml:1810

• The secret query parameter for /v2/testRepoConnectPlatform is validated as idName, but the handler treats it as an arbitrary secret/token string (req.query.secret) and passes it through to getTestRepoConnect. Using idName will reject most real secrets (e.g., tokens containing uppercase, _, .) with a 400 before the handler runs.

        - name: secret
          in: query
          schema:
            $ref: 'definitions.yaml#/idName'

src/api-v2.authz.test.ts:815

• This comment block says traversal payloads in sealedSecretName must be rejected before any business logic, but the subsequent "plain traversal" cases intentionally rely on Express path normalization and then exercise authz/business logic (403/200). Updating the comment to describe both behaviors will prevent future confusion when interpreting these tests.

      // Regression tests: path traversal payloads in sealedSecretName must be rejected
      // before reaching any business logic, regardless of the caller's team membership.

src/api-v2.authz.test.ts:862

• The test name reads like a security exception ("traversal allowed"), but what’s actually being asserted is Express’s path normalization resolving the request to the target team’s route. Renaming to reflect normalization avoids suggesting that traversal to another team is permitted by the API logic.

      test('cross-team dot-dot traversal allowed due to Express path normalization', async () => {

src/fileStore/file-map.ts:4

• ID_NAME_PATTERN duplicates the OpenAPI definitions.yaml#/idName regex. Without an explicit link, it’s easy for one to change and the other to drift, reintroducing inconsistent validation between request validation and file path generation.

const ID_NAME_PATTERN = /^[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?$/

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.