[codex] Fix Snowflake gzip payload parsing and auth handling

[shadimoudallal]

Summary

• Decode base64 and decompress gzip Snowflake request bodies before JSON parsing
• Add safer Lambda context/body logging around API Gateway and Snowflake requests
• Support bearer token credentials and tolerate sky-* API keys passed via SKYFLOW_BEARER_TOKEN
• Allow deploys to override FUNCTION_NAME and use env-provided Skyflow credentials

Validation

• Ran Node smoke test for gzip/base64 Snowflake payload parsing
• Deployed and exercised the updated Lambda flow during debugging

[claude]

Code Review — gzip parsing & bearer-token auth

Thanks for the fixes — the gzip handling and awsRequestId correction are real improvements. A few items I'd want addressed (especially the first one) before this lands.

🔴 Critical — sensitive data is being written to CloudWatch

lambda/snowflake-handler.js:84-87 logs the raw request body in full:

console.log('Raw event.body length:', String(bodyStr).length);
console.log('Raw event.body first 200 chars:', String(bodyStr).substring(0, 200));
console.log('Raw event.body (full):', bodyStr);

For Snowflake tokenize, event.body contains the plaintext PII being sent to the vault (emails, SSNs, names…). For detokenize it contains tokens, which are far less sensitive but still customer data. The whole point of this Lambda is to keep that data out of downstream systems — dumping it to CloudWatch (where logs are long-retention, broadly readable by anyone with logs:GetLogEvents, and often shipped to SIEMs) is a meaningful regression.

Same concern, slightly smaller blast radius, in handler.js:33 and snowflake-handler.js:65 with eventBodyFirst50 (50 chars of a tokenize body can easily contain a full email/SSN), and snowflake-handler.js:68 Full headers: ... will print whatever auth/signing headers a caller sent.

Suggested action: these look like debugging breadcrumbs left over from the issue you were chasing. Before merge, please either remove them or gate them behind something like if (process.env.DEBUG_LOG_BODIES === 'true') so they're explicitly off in prod. If you want a structured request log, log lengths and shapes (rows.length, Object.keys(body)) — never values.

🟠 Bug — deploy script allows env-only credentials but only wires up SKYFLOW_BEARER_TOKEN

deploy.sh:343 was updated to accept any of SKYFLOW_API_KEY / SKYFLOW_CLIENT_ID / SKYFLOW_BEARER_TOKEN as env-based credentials when no config file exists. But the subsequent JWT and API-Key branches (deploy.sh:361-388) still only read from skyflow-config.json:

if [ -f "skyflow-config.json" ]; then
    CLIENT_ID=$(jq -r '.credentials.clientID // empty' skyflow-config.json)
    API_KEY=$(jq -r '.credentials.apiKey // empty' skyflow-config.json)
fi

So a user who exports SKYFLOW_API_KEY=sky-… and runs ./deploy.sh with no config file will pass the early gate, then hit the final else and error out with No valid credentials found…. Only the bearer-token env path actually works. Either:

1. extend the JWT/API-Key branches to fall back to env vars (API_KEY="${API_KEY:-$SKYFLOW_API_KEY}"), or
2. tighten the gate at line 343 to require the file unless SKYFLOW_BEARER_TOKEN is set, matching what the script actually supports.

🟠 Inconsistency — main handler still does naive JSON.parse

handler.js:49 is unchanged:

const body = JSON.parse(event.body || '{}');

If gzip/base64 was a real problem from Snowflake, the same shape can hit /process and /processDatabricks (API Gateway base64-encodes any body whose content-type isn't in the binary-media-types allowlist, and Databricks UDFs can compress payloads). Worth extracting parseRequestBody into utils/ and using it from both handlers so behavior is symmetric and we don't end up debugging the same class of bug a second time.

🟡 Minor

• zlib.gunzipSync blocks the event loop. Lambda usually handles one request per invocation so it's not a correctness issue, but for large batches the async zlib.gunzip (or a promisify'd version) is a free win and matches Node idioms.
• Bearer token caching. skyflow-client.js:32-71 caches an SDK client per cluster:vault:env indefinitely. If SKYFLOW_BEARER_TOKEN is a short-lived token (common for OAuth bearers), every cached client keeps using the stale token until the container recycles. If bearer-token support is meant for long-lived tokens only, please call that out in config.example*.json and/or CLAUDE.md; if it's meant for short-lived tokens, the client needs a refresh hook.
• API-key-shaped bearer token heuristic (config.js:34): the startsWith('sky-') reclassification is reasonable but silent — consider only console.warn'ing once at startup (which you do) and adding a brief note to the deploy script's bearer-token branch so users learn to set SKYFLOW_API_KEY instead.
• Credential precedence is undocumented. With env vars, API_KEY > CLIENT_ID > BEARER_TOKEN. If a user sets two by accident, the silent winner can confuse. A line in CLAUDE.md or config.js's header comment would help.
• No tests committed. PR description says "Ran Node smoke test for gzip/base64 Snowflake payload parsing" and parseRequestBody is now exported at the bottom of snowflake-handler.js — looks like it was set up for a test. Worth checking that test in under lambda/__tests__/ (or wherever fits) so the gzip/base64 contract is locked in.
• awsContext?.requestId fallback in handler.js:28 / snowflake-handler.js:60 is unreachable for real Lambda invocations — the AWS context exposes awsRequestId, not requestId. The ?? keeps a test harness path open, which is fine; just noting in case it was meant as defensive coding.

✅ Things I liked

• Catching awsRequestId vs the previous (always-undefined) context.requestId is a real bugfix.
• The gzip magic-byte sniff (0x1f 0x8b) plus content-encoding check is a sensible belt-and-suspenders.
• Optional chaining on awsContext?.getRemainingTimeInMillis?.() makes the handlers safe to unit-test with a stub context.

Happy to look again once the body-logging is gated and the deploy script's env-credential gap is closed.

[claude]

PR Review — Snowflake gzip parsing, bearer token auth, deploy improvements

Thanks for this — the gzip parsing fix in particular addresses a real Snowflake/API Gateway interop gap. Overall the changes are well-scoped. A few notes below organized by category.

Strengths

• utils/body.js is well-designed: defensive gzip magic-byte detection (0x1f 0x8b) means it works even when callers forget the Content-Encoding: gzip header — a common Snowflake quirk. Good touch.
• Bearer-token auto-correction (config.js:37-41): detecting a sky- prefix in SKYFLOW_BEARER_TOKEN and routing it through API-key auth instead is a nice ergonomic improvement that will save real debugging time.
• Backwards-compatible deploy.sh: still works from skyflow-config.json, layering env-var overrides on top.
• Defensive logging (isBase64Encoded, body length/type) is exactly the kind of instrumentation needed to debug payload-encoding issues going forward.
• New body.test.js covers the three primary encoding paths (gzip+base64, plain JSON, base64-only).

Issues & Suggestions

1. Deploy auth precedence silently changed (potential regression)

Before: deploy.sh checked CLIENT_ID first, then API_KEY. Now it checks API_KEY first. Any user with both in skyflow-config.json will now deploy with API-key auth where they previously got JWT. This may be intentional (it's now consistent with config.js), but it's worth calling out in the PR description / release notes, and ideally a warning when both are present:

if [ -n "$API_KEY" ] && [ -n "$CLIENT_ID" ]; then
    echo -e "${YELLOW}Warning: both API key and JWT credentials found; using API key${NC}"
fi

2. Dead code in snowflake-handler.js:58-66

const bodyStr = event.body || '{}';
console.log('Request body type:', typeof bodyStr);
body = parseRequestBody(event);

bodyStr is only used to log its type, and that's already logged at the top of the handler (eventBodyType). The try/catch wrapper around parseRequestBody is also asymmetric: handler.js:51 calls it without a try/catch and relies on the outer handler. Either drop the inner try/catch in the Snowflake handler (for consistency), or add the same wrapping in handler.js. I'd lean toward dropping it — the outer handler already produces a useful error response.

3. Verbose debug logging looks like leftover instrumentation

console.log('Full event keys:', Object.keys(event)); in both handlers is useful while debugging but noisy in steady-state production. Consider gating behind process.env.DEBUG or removing once the Snowflake integration is stable. CloudWatch costs scale with log volume, and Object.keys(event) on every invocation adds up.

4. Bearer token: verify SDK shape and document the env-only limitation

• The { token: '...' } credential shape passed to the Skyflow Node SDK at skyflow-client.js:49-52 — please confirm against the installed SDK version (skyflow-node@^2.0.2) that this is the supported field name. The SDK docs I'm familiar with use credentialsString/apiKey/path; if token isn't a first-class credential type, this will fail at runtime even though config validation passes.
• SKYFLOW_BEARER_TOKEN is only loaded from env (never from skyflow-config.json). That's a reasonable design decision (bearer tokens are short-lived and don't belong in a file), but it deserves a one-liner in CLAUDE.md so the next reader doesn't go looking for credentials.token in the JSON config.
• The doc comment at config.js:8-9 says "Bearer tokens are passed through as-is and are not refreshed by this Lambda" — good. Worth also noting in CLAUDE.md under "Configuration Setup" so operators know they need a refresh strategy for long-lived deployments.

5. Edge case: corrupted gzip body produces a generic error

zlib.gunzipSync will throw an opaque error for truncated/corrupt gzip input. The Snowflake handler wraps it with Failed to parse request body: ... which is fine, but the standard /process handler doesn't, so the user just gets the raw zlib error in the 500 response. Consider catching in parseRequestBody itself and throwing a typed error like new Error('Invalid request body: failed to decompress gzip').

6. Test coverage gaps

• No negative tests in body.test.js (invalid base64, truncated gzip, non-JSON content after decompression).
• The sky--prefixed SKYFLOW_BEARER_TOKEN redirection in config.js:37-41 has no test — easy to regress and easy to test in isolation.
• No npm test script. Adding "test": "node lambda/utils/headers.test.js && node lambda/utils/body.test.js" to package.json would make CI / contributor experience nicer.

7. Doc updates missing per CLAUDE.md conventions

The PR adds a new auth mechanism and a new env var (FUNCTION_NAME override, SKYFLOW_BEARER_TOKEN) but CLAUDE.md's "Configuration Setup" section still only documents API Key and JWT. Suggest a short addition listing all three auth modes and noting bearer is env-var-only.

Minor nits

• parseRequestBody returns {} for empty body but the original code returned JSON.parse('{}') which is functionally equivalent. Fine.
• config.js:84 uses .trim().length === 0 for whitespace validation but config.js:37 uses .startsWith('sky-') without trimming. A SKYFLOW_BEARER_TOKEN=" sky-foo" would slip past the API-key redirect. Trivial, but if you're worried about copy-paste whitespace, normalize once at the top.
• Step counter mismatch in deploy.sh ([1/6] vs subsequent [2/5]) is preexisting — flagging in case you want to fix while you're in there.

Security

• No credential logging — good. authType is logged but not values.
• The deploy script's env-var path is an improvement: secrets via env (e.g., from CI secret store) keep them out of skyflow-config.json on disk. Good direction.
• One thing to watch: console.log('Full event keys:', Object.keys(event)) is keys-only so no PII, but the new eventBodyLength paired with verbose debug logs could be informative to anyone with CloudWatch read access. Probably fine for now, but worth a permissions review if this Lambda handles real PII tokens.

Summary

Solid, focused changes that address a real bug (Snowflake gzip) and add useful flexibility (bearer auth, env-var deploy). Main asks: clean up the dead code in snowflake-handler.js, verify the SDK accepts { token: ... } credentials, update CLAUDE.md docs for the new auth mode and env vars, and tone down the steady-state debug logging once verified in prod.

[claude]

Review: Snowflake gzip payload parsing and auth handling

Overall this is a focused, well-scoped fix for a real interop problem (Snowflake delivers external function payloads as gzip+base64) and the auth/deploy improvements are reasonable. The new utils/body.js is a clean addition with tests, and centralizing the parser between handler.js and snowflake-handler.js avoids drift. A few concrete observations below.

Bugs / correctness

• utils/body.js — non-string non-object bodies bypass parsing. The early return if (typeof event.body !== 'string') return event.body; (line 14) treats anything non-string as already-parsed. But Lambda payload sources (function URLs with binary, some test harnesses) can deliver event.body as a Buffer. A Buffer will fall through and reach downstream code (body.data || []), which silently produces an empty result instead of an error. Consider explicitly handling Buffer (decode + parse) or rejecting unknown types.
• deploy.sh — sky-* rerouting lives in two places. config.js:37-41 re-routes a sky-* bearer token to apiKey at runtime, but deploy.sh:412-421 still ships it as SKYFLOW_BEARER_TOKEN (with a warning). That works, but the env var visible in the Lambda console is misleading and the runtime warning fires on every cold start. Cleaner to do the rerouting once, in deploy.sh, so the Lambda env var matches the auth method actually used.
• config.js — JWT precedence interaction. If SKYFLOW_CLIENT_ID is set with partial JWT fields and SKYFLOW_BEARER_TOKEN is also set, the JWT branch wins (CLIENT_ID is truthy) and the requiredJwtFields check throws — bearer is never tried as a fallback. That matches the documented precedence, but worth a one-line test to lock it in.
• Body-length log on Buffer (handler.js:34, snowflake-handler.js:41). String(event.body).length stringifies the entire buffer just to log a length; event.body.length works for both strings and Buffers without the copy. Minor, but it fires on every invocation.

Security

• Gzip decompression has no size cap. zlib.gunzipSync will expand whatever it's given. Normally fine behind API Gateway's payload limits, but a small gzip bomb (10 MB of zeros → many GB) could OOM the function. Consider zlib.gunzipSync(buffer, { maxOutputLength: <reasonable cap> }) — supported on the Node 18.x runtime.
• Good removal of raw body logging in commit 13f25e8. The body-length-only replacement is a sensible compromise.
• SKYFLOW_BEARER_TOKEN rerouting log — the console.warn in config.js:38 says "appears to contain an API key", which leaks a small fact about the secret's shape into CloudWatch. Probably acceptable, but worth a deliberate decision.

Test coverage

The three body.test.js cases (gzip+base64+header, raw JSON, base64'd raw JSON) cover the happy paths. Missing cases worth adding cheaply:

• Empty body (event.body === '', null, undefined) → expects {}.
• Gzip body without content-encoding: gzip header (relies on the magic-byte fallback at line 23) — this is the actual case Snowflake hits per the PR description, so it deserves explicit coverage.
• Invalid JSON inside a valid gzip → expects a throw the caller can catch.
• No tests cover the new config.js bearer-token branches or the sky-* rerouting. A small Node assertion script that stubs process.env would lock in the precedence rules.

Style / conventions

• utils/body.js has a top-of-file comment that just restates the filename. Per CLAUDE.md's "default to writing no comments" guidance, it can be dropped.
• The rename context → awsContext is a defensible clarity improvement, but CLAUDE.md references line numbers (e.g. "see handler.js:12") that have now shifted — worth a sweep.
• config.js:97 ternary apiKey ? 'API_KEY' : token ? 'TOKEN' : 'JWT' is duplicated at skyflow-client.js:24 — consider extracting a tiny helper if a third call site appears.

Performance

Nothing concerning. Gzip decompression is sync but Lambda is single-request-per-instance, and the parser sits on the same hot path it already did. The added log fields per request are negligible.

Summary

The core fix is correct and the test for it is appropriate. Main asks before merging: (1) decide on a single place to handle sky-* token rerouting (deploy or runtime, not both), (2) add a no-content-encoding-header gzip test case, (3) consider maxOutputLength on gunzipSync. The Buffer-body edge case and the docstring trim are nice-to-haves.

[claude]

Code Review

Nice, focused PR. The gzip/base64 fix is the right call for Snowflake — Snowflake compresses external function payloads above a threshold and AWS API Gateway in REST mode delivers them base64-encoded, so the previous JSON.parse(event.body) would silently fail. Below are some observations.

Things that look good

• lambda/utils/body.js covers all the realistic body shapes (Buffer, base64 string, utf8 string, parsed object) and falls back to gzip magic-byte sniffing when Content-Encoding is absent — Snowflake does not always set the header reliably, so that fallback matters.
• Bounding the decompressed size via zlib.gunzipSync(buf, { maxOutputLength: MAX_DECOMPRESSED_BODY_BYTES }) is the right defense against gzip-bomb payloads. The 6 MiB default also aligns with the Lambda sync invocation payload limit.
• Error messages preserve the underlying cause without leaking the body content.
• awsContext?.awsRequestId ?? awsContext?.requestId is the correct field for Lambda runtime — the previous context.requestId was undefined (the runtime exposes awsRequestId), so the request-ID log line is actually useful now.
• body.test.js has good branch coverage (gzip+b64, plain, b64 only, magic-byte-only, raw Buffer, empty/null/undefined, malformed JSON, truncated gzip, wrong type) and is wired up via npm test.

Potential issues / suggestions

1. Bearer-token operational risk (worth documenting more loudly). config.js:9 and CLAUDE.md note bearer tokens are passed through and not refreshed, but a Skyflow bearer token typically lasts ~60 minutes. A Lambda deployed with SKYFLOW_BEARER_TOKEN will stop working at expiry and require a redeploy. Consider:

   • Adding a startup log line (or response error) that explicitly says "TOKEN expires; refresh by redeploying" when this auth mode is selected.
   • Or, recommending bearer-token mode only for short-lived debugging (already implied in CLAUDE.md but could be stronger in config.example*.json / deploy output).

2. deploy.sh sky-* heuristic is implicit (deploy.sh:373-377). Promoting SKYFLOW_BEARER_TOKEN=sky-... to SKYFLOW_API_KEY is friendly, but it silently changes auth semantics. The warning text is good; consider also logging the resulting auth mode at the end of the script so the operator sees what was actually deployed.

3. Auth precedence change in deploy.sh. Previously, when both clientID and apiKey were present in skyflow-config.json, JWT won (if [ -n \"$CLIENT_ID\" ] came first). Now API key wins. The warning at deploy.sh:379-381 flags it, but this is a behavior change for anyone whose config has both fields set — worth a note in the PR description / release notes.

4. MAX_DECOMPRESSED_BODY_BYTES parsing (body.js:4). parseInt of a malformed env value yields NaN, which zlib.gunzipSync will reject (likely with a confusing message). A Number.isFinite(...) || throw guard, or a fallback to the default, would make misconfiguration easier to diagnose.

5. eventBodyLength logged as 0 for object bodies (handler.js:34, snowflake-handler.js:41). When tests / direct invocation pass an already-parsed event.body, the log claims length 0, which can mislead troubleshooting. Either skip the field or report Object.keys(...).length.

6. utils/body.js lacks JSDoc — the rest of lambda/utils/ documents public functions. A short JSDoc on parseRequestBody describing inputs/throws would match the existing style.

7. config.js:37-41 — when SKYFLOW_CLIENT_ID is unset but other JWT fields are set, we now silently fall through to the bearer-token branch. That ordering relies on the deploy script never producing such a partial JWT env, which is true today, but a defensive log/error here would catch a misconfigured deploy faster.

8. SDK { token } shape (skyflow-client.js:49-52). The PR assumes skyflow-node 2.x accepts credentials: { token } directly in a vault config. Worth a quick confirmation against the installed SDK version (^2.0.2) — if it expects credentialsString: JSON.stringify({ accessToken }) or similar, this branch will silently fail at first API call. A smoke test against a real vault (mentioned in Validation) likely covered it, but a one-line comment citing the SDK doc/version would protect against future SDK upgrades.

Security

• No new injection surfaces. Body parsing is bounded. Bearer token is read from env and never logged.
• Confirm Lambda env vars are KMS-encrypted at rest (default is unencrypted in transit between AWS console and service); this is an AWS deployment posture issue, not a code issue, but worth noting since the PR expands credential surface.

Test coverage

• body.test.js is solid for the parser itself. The end-to-end path (Snowflake-style request → parseRequestBody → snowflake-handler.handler) isn't exercised; a small unit test that feeds a gzip+b64 Snowflake event into snowflake-handler.handler (with a mocked skyflowClient) would lock in the fix against future regressions.
• Consider adding a test that asserts parseRequestBody rejects payloads exceeding MAX_DECOMPRESSED_BODY_BYTES (set the env var low for that one case).

Overall: the fix is well-targeted and the body parser is a tidy abstraction. The bearer-token changes are the larger conceptual addition — clearer documentation around expiry/redeploy is the main thing I'd want before this lands in front of users who treat it as a first-class auth mode.

🤖 Generated with Claude Code