feat(presto/trino-driver): Support custom headers

[ovr]

No description provided.

[claude]

Claude finished @ovr's task in 1m 53s —— View job

---

Code Review: feat(presto/trino-driver): Support custom headers

• Gather context and review PR diff
• Review PrestoDriver changes
• Review TrinoDriver changes
• Review test coverage
• Review package.json changes
• Provide final review summary

---

Summary

Clean, well-scoped feature that adds headers?: Record<string, string> to the Presto/Trino driver config. The implementation correctly passes custom headers through both the presto-client execute() calls and the Trino testConnection() fetch call. The test file structure reorganization (splitting unit vs integration) is a nice improvement.

What looks good

• Auth precedence is correct in TrinoDriver.testConnection() — custom headers are spread first, then Authorization is set from customAuth/basicAuth, so auth config always wins over user-supplied headers.
• Test reorganization — splitting test/unit/ and test/integration/ with corresponding package.json script updates is well done.
• Unit test properly mocks node-fetch and verifies headers are forwarded.

Issues & Suggestions

1. Security — no header key validation (medium)
There's no blocklist preventing users from overriding sensitive headers like Authorization or Content-Type. While the Trino testConnection path handles this correctly (auth is set after spread), the presto-client library's execute() path may not. Consider documenting that overriding auth headers via headers config is unsupported, or adding a guard.

2. Test coverage gaps (medium)
The single test only covers the happy path. Missing scenarios:

• Custom headers with auth config simultaneously (verifying auth takes precedence)
• Empty/undefined headers (no spread issues)
• The useSelectTestConnection codepath doesn't get tested for header forwarding

3. Unrelated formatting changes (low)
The diff includes several cosmetic changes unrelated to the feature: whitespace in type annotations (lines 342, 366), error message reformatting (line 319-320), and indentation (line 430). These add noise to the diff — prefer keeping PRs focused on the feature.

4. headers config discoverability (low)
There's no environment variable mapping for headers (unlike host, port, catalog, etc. which all have getEnv() wrappers). Users can only pass custom headers via programmatic config, not via CUBEJS_DB_* env vars. This may be intentional, but worth noting in documentation or the PR description.

See inline comments for specific details on each file.

[claude]

Security concern: The headers config is spread into new presto.Client({ ...this.config }) at line 124-127, meaning headers is passed both at the client level and per-execute() call. This is likely harmless if the presto-client library ignores unknown options at the client level, but it's worth verifying there's no unintended double-application or conflict.

More importantly, there's no validation on the header keys/values. A user could potentially override critical protocol headers (e.g., Authorization, Content-Type, Host) which could cause unexpected behavior or security issues. Consider either:

1. Documenting that overriding auth headers via headers is unsupported, or
2. Adding a blocklist check to reject known sensitive headers like Authorization.

[claude]

Good pattern — spreading extraHeaders first and then setting Authorization after means auth config always takes precedence over custom headers, which is the correct behavior. This prevents a custom header from accidentally (or maliciously) overriding the auth mechanism.

However, the inverse isn't true for the PrestoDriver's testConnection (inherited from base) — that path uses the presto-client library's nodes() call, which doesn't get the custom headers. So custom headers only work on the Trino side's testConnection. Worth noting in documentation.

[codecov]

Codecov Report

❌ Patch coverage is 50.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 58.38%. Comparing base (5ded927) to head (3f25af6).
⚠️ Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
...ackages/cubejs-prestodb-driver/src/PrestoDriver.ts	50.00%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (5ded927) and HEAD (3f25af6). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (5ded927)	HEAD (3f25af6)
cubesql	1	0

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #10902       +/-   ##
===========================================
- Coverage   78.87%   58.38%   -20.49%     
===========================================
  Files         470      216      -254     
  Lines       92439    16949    -75490     
  Branches     3435     3436        +1     
===========================================
- Hits        72907     9895    -63012     
+ Misses      19030     6552    -12478     
  Partials      502      502               

Flag	Coverage Δ
cube-backend	58.38% <50.00%> (ø)
cubesql	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.