Skip to content

fix(security): reject URL injection in Qualtrics datacenter and Piped… - #3912

Open
AnkitSegment wants to merge 3 commits into
mainfrom
fix/url-injection-qualtrics-pipedrive-v2
Open

fix(security): reject URL injection in Qualtrics datacenter and Piped…#3912
AnkitSegment wants to merge 3 commits into
mainfrom
fix/url-injection-qualtrics-pipedrive-v2

Conversation

@AnkitSegment

Copy link
Copy Markdown
Contributor

A summary of your pull request, including the what change you're making and why.

Validates that user-supplied datacenter (Qualtrics) and domain (Pipedrive) fields contain only alphanumeric characters and hyphens before interpolating them into API base URLs. Without this check, an attacker-controlled value could redirect requests — including bearer tokens and API keys — to an arbitrary host. Fixes SECOPS-25215 (Qualtrics) and SECOPS-25240 (Pipedrive).

Testing

Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
  • [If destination is already live] Tested for backward compatibility of destination. Note: New required fields are a breaking change.
  • [Segmenters] Tested in the staging environment
  • [Segmenters] [If applicable for this change] Tested for regression with Hadron.

Security Review

Please ensure sensitive data is properly protected in your integration.

  • Reviewed all field definitions for sensitive data (API keys, tokens, passwords, client secrets) and confirmed they use type: 'password'

New Destination Checklist

  • Extracted all action API versions to verioning-info.ts file. example

…rive domain settings

Validates that user-supplied datacenter (Qualtrics) and domain (Pipedrive)
fields contain only alphanumeric characters and hyphens before interpolating
them into API base URLs. Without this check, an attacker-controlled value
could redirect requests — including bearer tokens and API keys — to an
arbitrary host. Fixes SECOPS-25215 (Qualtrics) and SECOPS-25240 (Pipedrive).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings July 30, 2026 05:41
@AnkitSegment
AnkitSegment requested a review from a team as a code owner July 30, 2026 05:41
@github-actions
github-actions Bot requested review from arnav777dev and nk1107 July 30, 2026 05:42

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds input validation to prevent URL injection via user-controlled subdomain components used to construct Qualtrics and Pipedrive API base URLs (SECOPS-25215 / SECOPS-25240).

Changes:

  • Validate datacenter (Qualtrics) to allow only alphanumeric characters and hyphens before building the base URL.
  • Validate domain (Pipedrive) to allow only alphanumeric characters and hyphens before making API requests.
  • Add unit tests covering common URL-injection payload patterns (/, ?, @).

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
packages/destination-actions/src/destinations/qualtrics/qualtricsApiClient.ts Rejects unsafe datacenter strings before interpolating into Qualtrics base URL.
packages/destination-actions/src/destinations/qualtrics/tests/index.test.ts Adds tests ensuring injected datacenter values are rejected.
packages/destination-actions/src/destinations/pipedrive/utils.ts Introduces validateDomain to reject unsafe domain strings before URL construction.
packages/destination-actions/src/destinations/pipedrive/pipedriveApi/pipedrive-client.ts Calls validateDomain when constructing the Pipedrive client.
packages/destination-actions/src/destinations/pipedrive/index.ts Calls validateDomain during testAuthentication before issuing the request.
packages/destination-actions/src/destinations/pipedrive/createUpdatePerson/tests/index.test.ts Adds tests ensuring injected domain values are rejected.

await expect(testDestination.testAuthentication(authData)).rejects.toThrowError(/401/)
})

it('throw error when datacenter contains URL injection characters', async () => {
await expect(testDestination.testAuthentication(authData)).rejects.toThrowError(/Invalid datacenter ID/)
})

it('throw error when datacenter contains @ injection', async () => {
const PERSON_ID = 33333

describe('Pipedrive domain validation', () => {
it('should throw when domain contains URL injection characters', async () => {
).rejects.toThrowError(/Invalid domain/)
})

it('should throw when domain contains @ injection', async () => {
Comment thread packages/destination-actions/src/destinations/pipedrive/utils.ts
Copilot AI review requested due to automatic review settings August 10, 2026 06:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

Suppressed comments (7)

packages/destination-actions/src/destinations/pipedrive/utils.ts:11

  • The current regex allows values like '-' or strings starting/ending with a hyphen (e.g. '-abc', 'abc-'), which are not valid DNS labels for the subdomain portion of a hostname. Consider tightening validation to enforce DNS label rules (start/end with alphanumeric, hyphens only in the middle, and optionally length <= 63) to avoid accepting values that will later fail as invalid hostnames.
export function validateDomain(domain: string): void {
  if (!/^[a-zA-Z0-9-]+$/.test(domain)) {
    throw new InvalidAuthenticationError(
      'Invalid domain. Domain must contain only alphanumeric characters and hyphens.'
    )
  }
}

packages/destination-actions/src/destinations/qualtrics/qualtricsApiClient.ts:134

  • Same validation concern as Pipedrive: this regex permits invalid DNS labels (e.g. leading/trailing '-'), which can produce invalid baseUrl hostnames. Tightening the validation (DNS label rules and optional length limit) would make the error deterministic and user-friendly instead of failing later in request routing.
    const datacenter = dc || 'iad1'
    if (!/^[a-zA-Z0-9-]+$/.test(datacenter)) {
      throw new InvalidAuthenticationError(
        'Invalid datacenter ID. Datacenter must contain only alphanumeric characters and hyphens.'
      )
    }
    this.baseUrl = `https://${datacenter}.qualtrics.com`

packages/destination-actions/src/destinations/pipedrive/index.ts:65

  • This adds a new validation branch in testAuthentication, but the PR’s new Pipedrive tests validate the action path (via testAction) rather than the auth test path. Add a focused test that calls testDestination.testAuthentication(...) with an injected domain (similar to the Qualtrics test) to ensure this specific entrypoint remains protected.
    testAuthentication: (request, { settings }) => {
      validateDomain(settings.domain)
      return request(`https://${settings.domain}.pipedrive.com/api/v1/users/me`)
    }

packages/destination-actions/src/destinations/qualtrics/tests/index.test.ts:46

  • Test descriptions are grammatically incorrect; consider updating to use third-person singular (e.g., 'throws an error when ...') for consistency/readability.
    it('throw error when datacenter contains URL injection characters', async () => {

packages/destination-actions/src/destinations/qualtrics/tests/index.test.ts:55

  • Test descriptions are grammatically incorrect; consider updating to use third-person singular (e.g., 'throws an error when ...') for consistency/readability.
    it('throw error when datacenter contains @ injection', async () => {

packages/destination-actions/src/destinations/pipedrive/createUpdatePerson/tests/index.test.ts:12

  • For consistency with other tests and clearer grammar, consider changing 'should throw' to 'throws an error' (or 'rejects with an error') in these test descriptions.
  it('should throw when domain contains URL injection characters', async () => {

packages/destination-actions/src/destinations/pipedrive/createUpdatePerson/tests/index.test.ts:21

  • For consistency with other tests and clearer grammar, consider changing 'should throw' to 'throws an error' (or 'rejects with an error') in these test descriptions.
  it('should throw when domain contains @ injection', async () => {

…uthentication

Moves datacenter/domain validation out of the per-request client
constructors and into testAuthentication, the single place settings are
verified. This avoids redundant validation on every perform call and
fixes Qualtrics snapshot tests that failed when fuzzed settings didn't
match the datacenter regex.
Copilot AI review requested due to automatic review settings August 10, 2026 07:48

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (3)

packages/destination-actions/src/destinations/qualtrics/index.ts:37

  • validateDatacenter requires a string, but input.settings.datacenter may be optional depending on generated settings (and the client constructor already defaults to 'iad1'). If datacenter is undefined, /.../.test(datacenter) will throw at runtime. Fix by applying the same default before validating (e.g., validate the resolved datacenter value) or by making validateDatacenter accept an optional value and handle the default internally.
      validateDatacenter(input.settings.datacenter)
      const apiClient = new QualtricsApiClient(input.settings.datacenter, input.settings.apiToken, request)
      const response = await apiClient.whoaAmI()

packages/destination-actions/src/destinations/qualtrics/utils.ts:27

  • This block appears to stringify the entire data object rather than the current data[key] value. That will produce the same JSON for every offending key and can leak unrelated fields into each entry. Consider stringifying data[key] instead.
      try {
        parsedData[key] = JSON.stringify(data)

packages/destination-actions/src/destinations/pipedrive/utils.ts:11

  • validateDomain is effectively identical to validateDatacenter in the Qualtrics destination (same regex/shape, different message). To avoid future drift/inconsistent hardening, consider extracting a shared helper (e.g., validateSubdomainLike(value, label)) used by both destinations.
export function validateDomain(domain: string): void {
  if (!/^[a-zA-Z0-9-]+$/.test(domain)) {
    throw new InvalidAuthenticationError(
      'Invalid domain. Domain must contain only alphanumeric characters and hyphens.'
    )
  }
}

Comment on lines 127 to 132
constructor(dc: string, apiToken: string, request: RequestClient) {
this.baseUrl = `https://${dc || 'iad1'}.qualtrics.com`
const datacenter = dc || 'iad1'
this.baseUrl = `https://${datacenter}.qualtrics.com`
this.apiToken = apiToken
this.request = request
}
Comment on lines 19 to 25
if (typeof data[key] === 'string') {
parsedData[key] = data[key] as string
parsedData[key] = data[key]
} else if (typeof data[key] === 'number') {
parsedData[key] = data[key] as number
parsedData[key] = data[key]
} else if (typeof data[key] === 'boolean') {
parsedData[key] = data[key] as boolean
parsedData[key] = data[key]
} else {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we making this change?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an intentional change — this repo's lint-staged pre-commit hook runs eslint --fix over the whole file whenever any part of it is staged, and the project's own @typescript-eslint/no-unnecessary-type-assertion rule flags these three casts as redundant (the preceding typeof checks already narrow the type). Since utils.ts got touched for validateDatacenter, the hook auto-stripped them. No behavior change — tried reverting it manually but the hook just re-strips it on every commit that touches this file, so leaving it as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants