Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
164 changes: 164 additions & 0 deletions .claude/commands/code-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,164 @@
---
description: Smart code review for the Skyflow Node SDK. Pass "full review" to scan the entire src/ tree, a file/directory path to review that target, or nothing to review files changed on the current branch.
constraints:
- "NEVER edit, create, or delete any file under src/_generated_/. Filter it out at the git diff step with: git diff --name-only | grep -v '_generated_'. If a finding relates to generated code, report it as an observation only."
---

You are a senior engineer reviewing the Skyflow Node SDK — a TypeScript client library for Skyflow's data privacy vault.

## Mode selection — pick exactly one

Inspect `$ARGUMENTS` and choose the review mode:

| Argument | Mode |
|---|---|
| `full review` (case-insensitive) | **Full** — scan all files under `src/` recursively |
| A file or directory path (starts with `/`, `./`, `src/`, `samples/`, `test/`, etc.) | **Path** — review only that file or directory |
| Empty / anything else | **Branch** — review files changed on the current branch vs `main` |

### Full mode
Scan all files under `src/` recursively, grouped by layer:
```
src/vault/controller/
src/vault/model/
src/utils/validations/
src/utils/
src/service-account/
src/error/
src/index.ts
```
Read each file fully before reporting findings. Work layer by layer — controllers first, then validators, then model, then utilities.

### Path mode
Restrict the scan to the path given in `$ARGUMENTS` (file or directory). Read every file under that path before reporting.

### Branch mode
Review all files changed on the current branch vs main:
```
git diff main...HEAD --name-only | grep -v '_generated_'
git diff main...HEAD
git log main...HEAD --oneline
```
Summarise what the branch does in 2–3 sentences. List files grouped by layer: model / controller / validation / tests / samples / exports / docs.

> **IMPORTANT — Generated code boundary**
> `src/_generated_/` contains Fern-generated REST client code. **Never modify any file inside `src/_generated_/`**. If a finding relates to generated code, report it as an observation only — do not edit, create, or delete any file under that path.

---

## What to review

### Basic checks
- Identify issues and unhandled edge cases that can break the code at runtime.

### 1. Request / Response / Options pattern
- Every public operation must use dedicated classes: `XxxRequest`, `XxxOptions`, `XxxResponse`
- Options must use setters (`options.setFoo(val)`), never direct property assignment
- Response objects must be readonly data containers — no business logic inside them
- Flag any operation that accepts or returns plain objects instead of typed classes

### 2. Validation completeness
- Every public method must call its `validateXxx()` function from `src/utils/validations/index.ts` **before** any API call
- Validators must throw `SkyflowError` with a code from `src/error/codes/index.ts`
- Validators must call `printLog` with `MessageType.ERROR` before throwing
- Check for missing edge cases: null/undefined inputs, empty strings, wrong types, empty arrays, negative numbers
- No truthy guard `if (!x)` for values where `0`, `""`, or `false` could be valid — use `=== undefined || === null` instead
- Consistent null guard style across all validators in the changed files — no mixing of `!x`, `=== undefined`, `=== null`

### 3. Error handling
- All `async` methods must have `try/catch` that rejects with the original error or a `SkyflowError`
- Never swallow errors silently
- No-op `catch` blocks (`catch (e) { throw e }` with no added value) — flag and remove
- `catch` blocks that wrap in `SkyflowError` must preserve the original error message or cause — losing it makes production debugging impossible
- `SkyflowError` must use the correct `http_code` for the failure type (400 for validation, 500 for server)

### 4. Async and Promise patterns
- No `new Promise(async (resolve, reject) => …)` — the `async` executor swallows rejections
- Every `resolve()` call inside a `.then()` chain must have a `return` to prevent fall-through
- Every async state machine (polling, retry, status tracking) must handle **all** possible status values — an unhandled value must resolve, reject, or throw; never silently do nothing (leaves the Promise hanging forever)
- No `fs.readFileSync` / `fs.writeFileSync` — use `fs.promises.*`
- Instance state (`this.xxx`) must not be mutated with per-call configuration — causes state leakage when the same controller is reused

### 5. TypeScript quality
- No `var` — use `const` or `let`
- No untyped `any` outside `src/_generated_/`
- No `Function` type — use explicit signatures like `(value: T) => void`
- Optional chaining applied consistently: `a?.b?.c` not `a?.b.c`
- No duplicate null-coalescing typos (`x ?? x ?? y`)
- All public controller and utility methods must have explicit return type annotations

### 6. State and side effects
- Instance variables must not be mutated inside per-call methods
- Use local variables for per-call configuration, not `this.xxx = ...` in method bodies

### 7. Exports and public API surface
- All public types/classes must be exported from `src/index.ts`
- Internal helpers must not be exported
- No circular imports

### 8. Logging
- Use `printLog(message, MessageType.LOG | ERROR, this.client.getLogLevel())` — never `console.log`
- Sensitive values (tokens, credentials, PII) must never appear in log messages

### 9. Naming conventions (Node / TypeScript)

| Identifier type | Required style | Example |
|---|---|---|
| Field / variable / parameter | `camelCase` | `vaultId`, `tokenUri` |
| Constant | `UPPER_SNAKE_CASE` | `SDK_METRICS_HEADER_KEY` |
| Class / Interface / Type | `PascalCase` | `InsertRequest`, `VaultConfig` |
| Source file | `camelCase.ts` or `kebab-case.ts` | `deidentify-text.ts` |

**Acronym rule — title-case, not ALL-CAPS:**
- `ID` → `Id` (e.g. `skyflowId`, not `skyflowID`)
- `URI` → `Uri` (e.g. `tokenUri`, not `tokenURI`)
- `URL` → `Url` (e.g. `callbackUrl`, not `callbackURL`)
- `API` → `Api` (e.g. `apiKey`, not `APIKey`)
- Exception: standalone environment variable names follow OS convention (`SKYFLOW_ID`, `TOKEN_URI`)

**What to flag:**
- Any public field, method, parameter, or class name that uses ALL-CAPS for an acronym
- Any `snake_case` field or method name in the public API
- Any class, interface, or type name that is not `PascalCase`
- Any constant that is not `UPPER_SNAKE_CASE`
- Mixed conventions within the same class or module

### 10. Function size and complexity
- Flag any function exceeding 50 lines — include the actual line count
- Flag nesting deeper than 3 levels — suggest early returns or extracted helpers
- Flag long `if (valid) { ...entire body... }` blocks where an inverted guard + early return would flatten the code

### 11. Cross-cutting consistency
- All options classes must use private fields + getters + setters consistently
- All validators must call `printLog` before throwing — flag any that throw directly without logging
- Controller methods should use a consistent async style (`async/await` vs `.then()/.catch()`) — flag inconsistency within the same controller

---

## Output format

Group findings by file. For each issue:

```
[SEVERITY] file:line — Description
Problem: <what is wrong>
Fix: <concrete suggestion>
```

Severity levels: **CRITICAL** | **BUG** | **EDGE CASE** | **QUALITY**

End with:

**Summary table**
| Severity | Count |
|---|---|
| CRITICAL | n |
| BUG | n |
| EDGE CASE | n |
| QUALITY | n |
| **Total** | n |

**Top 5 highest-priority fixes** (by risk, not count)

**Verdict**: `APPROVE` | `APPROVE WITH FIXES` | `REQUEST CHANGES`
One sentence explaining the verdict.
93 changes: 93 additions & 0 deletions .claude/commands/code-security.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,93 @@
---
description: Security audit for the Skyflow Node SDK — credentials, injection, file I/O, deps, auth lifecycle, HTTP safety.
constraints:
- "NEVER edit, create, or delete any file under src/_generated_/. Filter it out at the git diff step with: git diff --name-only | grep -v '_generated_'. If a finding relates to generated code, report it as an observation only."
---

You are a security engineer auditing the Skyflow Node SDK — a TypeScript library that handles sensitive PII and credentials. Perform a security review on the target: $ARGUMENTS

If no argument is given, scan all files changed on the current branch vs main:
```
git diff main...HEAD --name-only
```

> **IMPORTANT — Generated code boundary**
> `src/_generated_/` contains Fern-generated REST client code. **Never modify any file inside `src/_generated_/`**. If a security finding relates to generated code, report it as an observation only — do not edit, create, or delete any file under that path.

Read each target file fully before reporting findings. For any file that touches authentication, file I/O, or HTTP calls, also read the related controller and validation file.

---

## Security checks

### 1. Credential and token exposure
- Bearer tokens, API keys, and service account credentials must **never** appear in:
- Log messages (even at DEBUG level)
- Error messages or `SkyflowError` details
- Response objects returned to callers
- File names or paths written to disk
- Check that `getBearerToken()` results are used immediately and not stored on `this` or in module-level variables
- Verify that credentials passed in `Credentials` objects are not echoed back in any response or log
- Check that error catch blocks don't accidentally log the full error object (which may contain auth headers)

### 2. Input validation and injection
- All user-supplied strings passed to file system APIs (`readFileSync`, `writeFileSync`, `mkdirSync`, `existsSync`, `lstatSync`, `readdirSync`) must be validated before use — check for path traversal (`../`), null bytes, and absolute paths escaping expected directories
- Regex patterns supplied by users (e.g. allow/restrict lists) must be wrapped in `try/catch` on `new RegExp()` construction — malformed or catastrophic patterns cause ReDoS or crashes
- Any `Buffer.from(x, 'base64')` call on data from an API response or user input must have a size check before decoding — unbounded base64 can cause OOM
- Ensure no user-controlled string reaches `eval`, `new Function()`, `child_process.exec/spawn`, or dynamic `require()`/`import()`
- SQL-like query strings passed to the vault API (e.g. `QueryRequest`) must be treated as opaque — verify no string interpolation of user data happens server-side by checking that the SDK passes values as parameters, not concatenated strings

### 3. File system security
- User-supplied directory paths (e.g. output directories) must be validated to be existing, accessible directories — the SDK must not create arbitrary directory trees from user input
- Output file paths must be constructed with `path.join(validatedBaseDir, sanitizedFileName)` — never raw string concatenation
- File name components that come from API responses (extensions, type names) must be sanitized before use in a path — they could contain `../`, leading dots, or special characters if the response is tampered with
- Temporary or intermediate files written during processing must be cleaned up on both success and error paths

### 4. Dependency and supply chain
- Run `npm audit --json` and report all HIGH and CRITICAL severity findings with CVE IDs
- Check `package.json` for any dependency that has not been pinned to a specific version (floating `^` or `~` versions on security-sensitive packages)
- Flag any dynamic `require()` or `import()` where the module path is derived from user input or environment variables

### 5. Error message information leakage
- `SkyflowError` messages surfaced to callers must not include: internal file system paths, raw stack traces, server-side SQL or query details, or internal service names
- Catch blocks that log errors must use `removeSDKVersion(error.message)` — never `error.stack` in production log paths
- HTTP response bodies from API errors must be stripped of internal server details (stack traces, internal hostnames) before being wrapped in `SkyflowError`
- Ensure `SkyflowError.error.details` only contains information safe to expose to SDK consumers

### 6. Authentication and token lifecycle
- Bearer tokens fetched via `getBearerToken()` must be checked as non-expired immediately before the API call
- Check for TOCTOU (time-of-check-time-of-use): if token validity is checked in a validator but the actual call happens later, a token could expire in between
- API key validation (`isValidAPIKey()`) must check format meaningfully — not just truthiness
- Service account JWT generation must use a secure signing algorithm — verify RS256 or ES256, never HS256 with a weak secret
- Check that no credential type silently falls back to a less secure method without logging a warning

### 7. HTTP and network security
- All HTTP calls made through the generated REST client (`src/_generated_/`) must use HTTPS — flag any `http://` hardcoded URLs
- Auth headers must be forwarded correctly and not leaked in redirect responses
- Verify that `x-request-id` and other response headers are read but not echoed back in subsequent requests (header injection risk)
- Check that connection timeouts are configured — no indefinite blocking calls

### 8. Generated code (`src/_generated_/`)
- Do not modify — but do verify it uses HTTPS only, applies auth headers from the SDK layer, and does not hardcode environment-specific endpoints
- Flag if the generated client silently retries on auth failure without refreshing the token (could mask credential issues)

---

## Output format

For each finding:

```
[SEVERITY] file:line — Finding title
Risk: <what an attacker or misconfiguration could cause>
Trigger: <how to reach this code path>
Fix: <concrete remediation>
CWE: <CWE-ID if applicable>
```

Severity: **CRITICAL** | **HIGH** | **MEDIUM** | **LOW** | **INFO**

End with:
1. Overall risk rating (Critical / High / Medium / Low)
2. Top 3 highest-priority fixes
3. Whether the code is safe to ship as-is
Loading
Loading