# Requirements: QSP-38874 — Security Hardening: Fix Security Review Findings

## Status
✅ Approved

---

## Summary

This ticket addresses all 6 security findings identified in the security review of the Dynatrace Scorecards app. The fixes are confined to API functions and the URL utility — no UI changes. They cover: DQL injection prevention via `version` parameter validation (HIGH), removal of three unused OAuth scopes and the unused `@dynatrace-sdk/client-state` package (HIGH), completeness of DQL string escaping in `findMatchingScorecardDefinition` (MEDIUM), defence-in-depth input validation inside two query builder functions (MEDIUM), an explicit URL scheme allowlist in `safeOpenLinkInNewTab` supplementing DOMPurify (MEDIUM), and DQL-safe escaping of `signalIds` in the `array()` expression (LOW).

---

## Acceptance Criteria

- [ ] AC-1 (Finding #1 — HIGH): `scorecardResults()` rejects any `version` value that is neither the exact string `"summary"` nor a match for `SEMANTIC_VERSIONING_REGEX` with `{ status: 400, message: 'Invalid version format: <value>' }` before building any DQL query.
- [ ] AC-2 (Finding #2 — HIGH): The three unused scope objects (`storage:events:read`, `state:app-states:read`, `state:app-states:write`) are removed from `app.config.cts`. The `@dynatrace-sdk/client-state` package is removed from `package.json` and `package-lock.json`.
- [ ] AC-3 (Finding #3 — MEDIUM): The `safeValue` escaping inside `findMatchingScorecardDefinition` handles all DQL-significant characters in the correct order: `\` → `\\` (first), `"` → `\"`, `\n` → `\n` (literal two-char sequence), `\r` → `\r`, `|` → `\|`.
- [ ] AC-4 (Finding #4 — MEDIUM): `buildSignalEventsQuery` throws `new Error('Invalid assetId format: <value>')` when `assetId` does not match `ASSET_ID_PATTERN`, and throws `new Error('Invalid assetVersion format: <value>')` when `assetVersion` is neither `"summary"` nor a match for `SEMANTIC_VERSIONING_REGEX`. `buildCatalogComponentQuery` throws `new Error('Invalid assetId format: <value>')` when `assetId` does not match `ASSET_ID_PATTERN`. Both validations are independent of any caller-level check.
- [ ] AC-5 (Finding #5 — MEDIUM): `safeOpenLinkInNewTab` retains DOMPurify sanitisation as the first layer. After sanitisation it parses the result with `new URL()` and returns without calling `window.open` when the parsed scheme is not `https:` or `http:`. URLs that fail `new URL()` parsing are handled by the existing `catch` block.
- [ ] AC-6 (Finding #6 — LOW): Each element of `signalIds` in `buildSignalEventsQuery` has `\` escaped to `\\` and `"` escaped to `\"` before being wrapped in `"..."` in the `array(...)` expression.
- [ ] AC-7: All existing passing tests continue to pass after the changes. Existing tests in `url.utils.spec.ts` that incorrectly assert `window.open` is called for `javascript:`, `data:`, `ftp:`, empty-string, and whitespace URLs are updated to assert it is NOT called.
- [ ] AC-8: Manual verification on `https://gmg80500.dev.apps.dynatracelabs.com` confirms scorecard results load for `version = "summary"` and `version = "1.0.0"`, and that passing `version = "'; DROP TABLE"` returns an error response.

---

## Key Decisions

| # | Decision | Rationale |
|---|---|---|
| D-1 | Address all 6 findings in this ticket — none deferred | User confirmed: no deferral |
| D-2 | `version` is valid if `=== "summary"` OR matches `SEMANTIC_VERSIONING_REGEX` | Covers all stated valid forms: `1.0.0`, `1.0.0-dev`, `1.0.0-dev.abc123`, `1.0.0-dev.1744012345` |
| D-3 | Remove `@dynatrace-sdk/client-state` from `package.json` (YAGNI) | Zero imports found anywhere in source — confirmed via grep |
| D-4 | DQL escaping is inline in each builder — no new shared utility | User explicitly chose inline over extracted abstraction |
| D-5 | Defence-in-depth validation inside `buildSignalEventsQuery` and `buildCatalogComponentQuery` | Both builders must be safe regardless of who calls them |
| D-6 | Keep DOMPurify AND add an explicit `https:`/`http:` scheme check as a second layer | Two-layer defence: DOMPurify blocks HTML injection; scheme check blocks non-HTTP protocols |
| D-7 | Escape `signalIds` inside `array(...)` as defence-in-depth | Currently safe (values come from constants), but robustness protects future callers |
| D-8 | Verification environment: `gmg80500.dev.apps.dynatracelabs.com` | User-specified |

---

## 5 Steps to Improve Security — Applicability Mapping

The context for this ticket includes a "5 Steps to Improve Security" guide for Dynatrace apps making HTTP(S) integrations. The table below records how each step applies to this codebase.

| Step | Title | Status | Covered by |
|---|---|---|---|
| 1 | **Validate Incoming Payload** | ✅ Covered | AC-1 (`version` validated before any DQL is built), AC-4 (query builders validate `assetId`/`assetVersion` at their own boundary). `zod` not introduced — regex validation provides the same boundary enforcement without adding a dependency (YAGNI). |
| 2 | **Store Secrets in a Secure Way** | N/A | The app has no secrets, tokens, or connection objects. Authentication is handled by Dynatrace OAuth scopes declared in `app.config.cts` and enforced by the platform. The overprivileged scopes are removed by AC-2. |
| 3 | **Concatenate URLs in a Secure Way** | ✅ Covered | AC-5 uses `new URL()` to parse and validate URLs before `window.open`. Deep link construction at `scorecard-results.function.ts:78` already uses `encodeURIComponent` for both `assetId` and `assetVersion` — no change needed there. |
| 4 | **Do Not Expose Secrets** | N/A | No tokens, webhook URLs, or API keys are handled by the app. No secrets are passed via URL query parameters. |
| 5 | **Always Encode Input Values** | ✅ Covered | AC-1 validates `version` before DQL interpolation. AC-3 escapes all DQL-significant characters in catalog field values. AC-4 validates `assetId` in query builders (restricted to `[a-zA-Z0-9_\-.]` — no DQL-significant chars). AC-6 escapes `signalIds` inside `array(...)`. SDK calls use `queryExecutionClient.queryExecute({ dqlQuery })` (no user IDs in REST path segments) — `encodeURIComponent` is already applied wherever URLs are constructed. |

---

## Data Model

No new TypeScript interfaces. The following existing constants from `src/api/constants/defaults.constants.ts` are reused:

```typescript
// Existing — no changes needed
export const SEMANTIC_VERSIONING_REGEX =
  /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$/;

export const ASSET_ID_PATTERN = /^[a-zA-Z0-9_\-.]+:[a-zA-Z0-9_\-.]+\/[a-zA-Z0-9_\-.]+$/;
```

### Valid `version` values after AC-1 fix

All three formats below are real production versions already observed in the system. The existing `SEMANTIC_VERSIONING_REGEX` covers all of them without modification.

| Input | Format | Accepted | Reason |
|---|---|---|---|
| `"summary"` | Exact keyword | ✅ | Exact whitelist match |
| `"337.16.0"` | `X.Y.Z` | ✅ | Standard SemVer, large major version |
| `"2.3984.0"` | `X.Y.Z` | ✅ | Standard SemVer, large minor version |
| `"2.336.0-20260331121806749"` | `X.Y.Z-<numeric-timestamp>` | ✅ | Pre-release identifier is a pure numeric timestamp |
| `"3.4.2-dev.20260401T060003+bcf72e40"` | `X.Y.Z-dev.<datetime>+<hash>` | ✅ | Pre-release `dev.<ISO-datetime>` + build metadata `<hex-hash>` |
| `"3.4.2-dev.20260330T125645+a25a5d46"` | `X.Y.Z-dev.<datetime>+<hash>` | ✅ | Same pattern — different timestamp and hash |
| `"1.0.0-dev"` | `X.Y.Z-dev` | ✅ | SemVer pre-release: bare `dev` |
| `"1.0.0-dev.abc123"` | `X.Y.Z-dev.<alphanum>` | ✅ | SemVer pre-release: `dev.<hash>` |
| `"'; DROP TABLE"` | — | ❌ | Fails both checks → 400 |
| `"../../../etc"` | — | ❌ | Fails both checks → 400 |
| `""` | — | ❌ | Fails both checks → 400 |
| `undefined` (omitted) | — | ✅ | Defaults to `"summary"` via `?? 'summary'` before validation |

---

## UX / UI Specification

No UI changes. All fixes are in the API layer (`src/api/`) and the URL utility (`src/app/utils/url.utils.ts`). Error responses surface as `{ status: 400 }` or `Error` throws to the function caller.

---

## API Contracts

### `scorecardResults()` — version validation added

```typescript
// src/api/scorecard-results.function.ts
export default async function scorecardResults(ctx: {
  assetID?: string;
  appID?: string;
  version?: string;
}): Promise<ScorecardResult> {
  const { assetID, appID } = ctx;
  const version = ctx.version ?? 'summary';

  // NEW: validate version before any DQL is built
  if (version !== 'summary' && !SEMANTIC_VERSIONING_REGEX.test(version)) {
    throw { status: 400, message: `Invalid version format: ${version}` };
  }

  // ...existing assetID / appID logic unchanged
}
```

Import line extended: `import { ASSET_ID_PATTERN, SEMANTIC_VERSIONING_REGEX } from './constants/defaults.constants';`

---

### `buildSignalEventsQuery()` — self-validation + signalId escaping added

```typescript
// src/api/signals/signal-event.ts
export function buildSignalEventsQuery(assetId: string, assetVersion: string, signalIds: string[]): string {
  if (!ASSET_ID_PATTERN.test(assetId)) {
    throw new Error(`Invalid assetId format: ${assetId}`);
  }
  if (assetVersion !== 'summary' && !SEMANTIC_VERSIONING_REGEX.test(assetVersion)) {
    throw new Error(`Invalid assetVersion format: ${assetVersion}`);
  }
  const signalIdList = signalIds
    .map((s) => `"${s.replace(/\\/g, '\\\\').replace(/"/g, '\\"')}"`)
    .join(', ');
  // ...query string unchanged
}
```

Import added: `import { ASSET_ID_PATTERN, SEMANTIC_VERSIONING_REGEX } from '../constants/defaults.constants';`

---

### `buildCatalogComponentQuery()` — self-validation added

```typescript
// src/api/catalog/component-smartscape-node.ts
export function buildCatalogComponentQuery(assetId: string): string {
  if (!ASSET_ID_PATTERN.test(assetId)) {
    throw new Error(`Invalid assetId format: ${assetId}`);
  }
  // ...query string unchanged
}
```

Import added: `import { ASSET_ID_PATTERN } from '../constants/defaults.constants';`

---

### `findMatchingScorecardDefinition()` — complete DQL escaping

```typescript
// src/api/generate-scorecards-result-event.function.ts  (internal function)
const safeValue =
  v == null
    ? ''
    : String(v)
        .replace(/\\/g, '\\\\')  // \ → \\ — MUST be first
        .replace(/"/g, '\\"')    // " → \"
        .replace(/\n/g, '\\n')   // newline → \n
        .replace(/\r/g, '\\r')   // CR → \r
        .replace(/\|/g, '\\|');  // | → \|
```

---

### `safeOpenLinkInNewTab()` — scheme allowlist added

```typescript
// src/app/utils/url.utils.ts
export function safeOpenLinkInNewTab(link: string) {
  try {
    const sanitized = DOMPurify.sanitize(link);
    const parsed = new URL(sanitized);
    if (parsed.protocol !== 'https:' && parsed.protocol !== 'http:') {
      return;
    }
    window.open(sanitized, '_blank', 'noopener,noreferrer');
  } catch (error) {
    console.error('Invalid URL:', link, error);
  }
}
```

---

### `app.config.cts` — three scopes removed

Remove the following three scope objects from the `scopes` array:

```typescript
// REMOVE:
{ name: 'storage:events:read',   comment: 'needed for fetching events' },
{ name: 'state:app-states:read',  comment: 'Read cached app state for request caching' },
{ name: 'state:app-states:write', comment: 'Write cached app state for request caching' },
```

Remaining scopes after removal: `storage:buckets:read`, `storage:bizevents:read`, `storage:smartscape:read`, `storage:events:write`.

---

### Cache Strategy

No caching involved — all changes are synchronous validation or transformation logic.

---

## DQL Queries

No new DQL queries. Existing query builders are hardened in-place.

---

## Implementation Plan

### Consumer Impact Analysis

No TypeScript interface signatures change. The behavioural change visible to callers: `buildSignalEventsQuery` and `buildCatalogComponentQuery` now throw on invalid input instead of silently building malformed queries. Callers that already validate at a higher level are unaffected at runtime.

| Symbol | Consumer file | Required change |
|---|---|---|
| `scorecardResults` | `src/api/tests/scorecard-results.function.spec.ts` | Add tests for invalid `version` → `{ status: 400 }` |
| `buildSignalEventsQuery` | `src/api/signals/signal-event.spec.ts` | Add tests for invalid `assetId`, invalid `assetVersion`, escaped `signalId` |
| `buildCatalogComponentQuery` | `src/api/catalog/component-smartscape-node.spec.ts` | Add test for invalid `assetId` → `Error` |
| `generateScorecardResults` (via `findMatchingScorecardDefinition`) | `src/api/tests/generate-scorecards-result-event.function.spec.ts` | Add test verifying DQL-special chars in catalog values are escaped in the built query |
| `safeOpenLinkInNewTab` | `src/app/utils/url.utils.spec.ts` | Update existing tests that expect `window.open` for unsafe URLs; add scheme-blocked assertions |
| `app.config.cts` | none | Manual deploy check |
| `@dynatrace-sdk/client-state` in `package.json` | none — zero imports confirmed | Run `npm uninstall @dynatrace-sdk/client-state` |

---

### Step 1: Remove unused scopes from `app.config.cts`

- **File**: `app.config.cts`
- **Change**: Delete the three scope objects for `storage:events:read`, `state:app-states:read`, `state:app-states:write` from the `scopes` array.
- **Verification**: `npm run typecheck` exits 0; `npm run lint` exits 0.

### Step 2: Remove `@dynatrace-sdk/client-state` package

- **Command**: `npm uninstall @dynatrace-sdk/client-state`
- **Verification**: `npm run typecheck` exits 0; `grep -r "client-state" src/` returns no results.

### Step 3: Add `version` validation in `scorecardResults()` (Finding #1)

- **File**: `src/api/scorecard-results.function.ts`
- **Changes**:
  1. Extend the import from `./constants/defaults.constants` to also import `SEMANTIC_VERSIONING_REGEX`.
  2. After `const version = ctx.version ?? 'summary'`, add: `if (version !== 'summary' && !SEMANTIC_VERSIONING_REGEX.test(version)) throw { status: 400, message: \`Invalid version format: ${version}\` };`
- **Verification**: Unit test `version = "'; DROP TABLE"` → throws `{ status: 400 }`; `version = "1.0.0-dev.abc123"` → proceeds to `queryGrail`.

### Step 4: Improve DQL escaping in `findMatchingScorecardDefinition` (Finding #3)

- **File**: `src/api/generate-scorecards-result-event.function.ts`
- **Change**: Replace `String(v).replace(/"/g, '\\"')` with the 5-step chain (shown in API Contracts). Order is fixed — `\` must be replaced first.
- **Verification**: Unit test where a catalog field value contains `\`, `"`, newline, and `|`; assert `queryGrail` spy receives correctly escaped DQL.

### Step 5: Add self-validation and signalId escaping to `buildSignalEventsQuery` (Findings #4 and #6)

- **File**: `src/api/signals/signal-event.ts`
- **Changes**:
  1. Add import for `ASSET_ID_PATTERN` and `SEMANTIC_VERSIONING_REGEX` from `../constants/defaults.constants`.
  2. Throw on invalid `assetId`.
  3. Throw on invalid `assetVersion`.
  4. Escape each `signalId` element (`\` first, then `"`) before wrapping in `"..."`.
- **Verification**: Unit tests for invalid `assetId`, invalid `assetVersion`, and a `signalId` containing `"` and `\`.

### Step 6: Add self-validation to `buildCatalogComponentQuery` (Finding #4)

- **File**: `src/api/catalog/component-smartscape-node.ts`
- **Changes**:
  1. Add import for `ASSET_ID_PATTERN` from `../constants/defaults.constants`.
  2. Throw `new Error('Invalid assetId format: ...')` when `assetId` does not match the pattern.
- **Verification**: Unit test for `buildCatalogComponentQuery("invalid!")` → throws `Error`.

### Step 7: Add URL scheme allowlist to `safeOpenLinkInNewTab` (Finding #5)

- **File**: `src/app/utils/url.utils.ts`
- **Change**: After `DOMPurify.sanitize(link)`, parse with `new URL()` and return early if scheme is not `https:` or `http:`. Leave the existing `catch` block unchanged.
- **Verification**: Tests with `javascript:alert(1)`, `data:text/html,...`, `ftp://...` → `window.open` NOT called; `https://example.com` → called normally.

### Step 8: Update breaking tests in `url.utils.spec.ts`

- **File**: `src/app/utils/url.utils.spec.ts`
- **Change**: Update the following tests whose assertions no longer match the corrected behaviour (see Unit Test Plan for full list). All changes are from `toHaveBeenCalled()` / called-with assertions to `not.toHaveBeenCalled()`.
- **Verification**: All tests in the file pass.

### Step 9: Run full validation

- **Commands** (in order):
  1. `npm run typecheck`
  2. `npm run lint`
  3. `npm run test:app -- --testPathPattern="scorecard-results.function|signal-event|component-smartscape-node|generate-scorecards-result-event|url.utils"`
- **Gate**: All pass; coverage ≥ 90% on all modified files.

---

## Unit Test Plan

### `src/api/tests/scorecard-results.function.spec.ts` — additions

- `version = "summary"` (default) → `queryGrail` is called (no 400)
- `version = "1.0.0"` → proceeds normally
- `version = "1.0.0-dev"` → proceeds normally
- `version = "1.0.0-dev.abc123"` → proceeds normally
- `version = "1.0.0-dev.1744012345"` → proceeds normally
- `version = "'; DROP TABLE"` → throws `{ status: 400, message: 'Invalid version format: ...' }`
- `version = "../../../etc/passwd"` → throws `{ status: 400 }`
- `version = ""` → throws `{ status: 400 }`
- `ctx.version` is `undefined` → defaults to `"summary"` → proceeds normally

### `src/api/signals/signal-event.spec.ts` — additions

- Invalid `assetId = "invalid!"` → throws `Error` containing `"Invalid assetId format"`
- Invalid `assetVersion = "'; DROP"` → throws `Error` containing `"Invalid assetVersion format"`
- `signalId` containing `"` → output query contains `\"`
- `signalId` containing `\` → output query contains `\\`

### `src/api/catalog/component-smartscape-node.spec.ts` — additions

- `assetId = "invalid!input"` → throws `Error` containing `"Invalid assetId format"`
- `assetId = "component:ns/name"` → returns query string (no throw)

### `src/api/tests/generate-scorecards-result-event.function.spec.ts` — additions

- Catalog field value containing `\` → `queryGrail` spy called with `\\` in the query
- Catalog field value containing `"` → query contains `\"`
- Catalog field value containing a newline → query contains `\n` (literal two-char sequence)
- Catalog field value containing `|` → query contains `\|`
- Catalog field value is `null` → query contains `""` (no crash)

### `src/app/utils/url.utils.spec.ts` — updates and additions

| Test description | Before fix | After fix |
|---|---|---|
| `javascript:` URL (line ~85, ~176) | `toHaveBeenCalled()` | `not.toHaveBeenCalled()` |
| `data:` URL (line ~182) | `toHaveBeenCalled()` | `not.toHaveBeenCalled()` |
| `ftp://` URL (line ~233) | `toHaveBeenCalled()` | `not.toHaveBeenCalled()` |
| Empty string `""` (line ~122) | called with `""` | `not.toHaveBeenCalled()` |
| Whitespace `"   "` (line ~128) | `toHaveBeenCalled()` | `not.toHaveBeenCalled()` |
| `null` / `undefined` (line ~109, ~115) | `toHaveBeenCalled()` | `not.toHaveBeenCalled()` |
| Relative URL `/path/to/resource` (line ~246) | `toHaveBeenCalled()` | `not.toHaveBeenCalled()` |
| `https://example.com` | called — retain | called — retain |
| `http://localhost:3000` | called — retain | called — retain |

---

## Edge Cases & Error Handling

- **`version` is `undefined`**: `ctx.version ?? 'summary'` applies before validation; defaults to `"summary"` which passes the exact-match check — no 400.
- **`version` is SemVer with build metadata** (e.g. `1.0.0+build.1`): `SEMANTIC_VERSIONING_REGEX` matches it. Not in scope to block.
- **`signalIds` is empty array**: `signalIdList` is `""` — `array()` empty — query structurally valid; returns no rows. No throw.
- **Backslash escaping order in DQL**: `\` must be replaced first; doing `"` first and `\` second would double-escape already-escaped quotes.
- **`safeOpenLinkInNewTab` with `javascript:alert(1)`**: DOMPurify may or may not strip it; the `new URL()` scheme check blocks it regardless.
- **`safeOpenLinkInNewTab` with a relative path**: `new URL('/path')` throws `TypeError` (no origin) → caught by `catch` → `window.open` not called.
- **`safeOpenLinkInNewTab` with `null` / `undefined`**: `DOMPurify.sanitize(null)` → `""` → `new URL("")` throws → caught. No crash.
- **`findMatchingScorecardDefinition` with null field**: `v == null ? ''` guard already present — escaping chain is skipped entirely.
- **`@dynatrace-sdk/client-state` removal**: Confirmed zero imports in `src/`. No test mocks found. Removal is safe.

---

## Out of Scope

- **Defence-in-depth inside `buildScorecardResultQuery`**: Not named for Finding #4; caller (`scorecardResults`) validates both `assetId` (ASSET_ID_PATTERN) and `version` (AC-1). No additional guard needed in the builder.
- **Validation of `appID` in `buildAppIdLookupQuery`**: Not flagged in the security review for this ticket.
- **DQL escaping for `assetId` in query builders**: `ASSET_ID_PATTERN` restricts `assetId` to `[a-zA-Z0-9_\-.]` characters only, which contains no DQL-significant characters. Separate string escaping is unnecessary.
- **SemVer build metadata blocking** (e.g. `1.0.0+build.1`): Not listed as invalid by the user.
- **Rate limiting, authentication, CSP, CORS**: Out of scope.
- **E2E tests**: Not required — all changes are at the API/utility layer and fully covered by unit tests.


