feat(service-gateway): S3 storage support + secrets hardening [7/10]#157
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
size/XL
|
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR introduces comprehensive gateway routing enhancements including request/response caching for GET requests, pre-validation policy enforcement, binary body support, and connector-scoped secret resolution. AWS Signature V4 signing is implemented as a new module. Path resolution is improved with catch-all segment matching and specificity-based sorting. Secret key generation is refactored across multiple routes to use connector slugs for namespacing. Extensive test coverage is added for gateway logic, and a new Storj connector seed script is introduced. Changes
Sequence DiagramsequenceDiagram
participant Client
participant GatewayRoute as Gateway Route
participant PolicyModule as Policy/Quota
participant ValidationModule as Validation
participant CacheStore as Cache Store
participant SecretModule as Secret Resolver
participant TransformModule as Transform
participant UpstreamServer as Upstream Server
participant LoggingModule as Logging
Client->>GatewayRoute: HTTP Request (GET/POST/PUT/etc)
GatewayRoute->>GatewayRoute: Read & preprocess body<br/>(binary/text, compute bytes)
GatewayRoute->>PolicyModule: Enforce rate limits/quotas
alt Policy Blocked
PolicyModule-->>GatewayRoute: Rate limit error + headers
GatewayRoute-->>Client: 429 with policy headers
else Policy Approved
GatewayRoute->>ValidationModule: Validate headers/body/schema
alt Validation Failed
ValidationModule-->>GatewayRoute: Validation error
GatewayRoute-->>Client: 400 Bad Request
else Validation Passed
alt GET + Cache Hit
GatewayRoute->>CacheStore: Check cache with TTL
CacheStore-->>GatewayRoute: Cached response
GatewayRoute->>GatewayRoute: Add X-Gateway-Cache header
GatewayRoute->>LoggingModule: Log cache hit
LoggingModule->>LoggingModule: Record usage
GatewayRoute-->>Client: Cached response
else Cache Miss or Non-GET
GatewayRoute->>SecretModule: Resolve secrets in owner scope
SecretModule-->>GatewayRoute: Resolved secrets
GatewayRoute->>TransformModule: Transform request<br/>(binary body, auth, headers)
TransformModule-->>GatewayRoute: Transformed request
GatewayRoute->>UpstreamServer: Proxy request
UpstreamServer-->>GatewayRoute: Response (headers + body)
alt GET + 2xx Status
GatewayRoute->>CacheStore: Store response with TTL
end
GatewayRoute->>GatewayRoute: Apply policy headers
GatewayRoute->>LoggingModule: Log usage with metadata
LoggingModule->>LoggingModule: Record request metrics
GatewayRoute-->>Client: Final response
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web-next/src/app/api/v1/gw/[connector]/[...path]/route.ts (1)
11-12:⚠️ Potential issue | 🟡 MinorUpdate route docs to include HEAD support.
The handler now supports
HEAD, but the top-level comment still lists only GET/POST/PUT/PATCH/DELETE.📝 Proposed fix
- * Supports: GET, POST, PUT, PATCH, DELETE + * Supports: GET, POST, PUT, PATCH, DELETE, HEADAlso applies to: 367-369
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/app/api/v1/gw/`[connector]/[...path]/route.ts around lines 11 - 12, The route's documentation comment in route.ts lists supported methods but omits HEAD even though the route handler now handles HEAD; update the top-level comment block (and the secondary comment block later in the file near the other method listing) to include "HEAD" alongside GET/POST/PUT/PATCH/DELETE so the docs match the actual handler implementation that supports HEAD.bin/seed-public-connectors.ts (1)
336-336:⚠️ Potential issue | 🟠 MajorAvoid printing full generated API keys in seed logs.
Logging full keys increases accidental secret exposure risk in CI/terminal history.
🔐 Proposed fix
- console.log(` API key: ${rawKey.slice(0, 8)}... (full: ${rawKey})`); + console.log(` API key: ${rawKey.slice(0, 8)}...`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/seed-public-connectors.ts` at line 336, The current seed script prints the full API key via console.log using rawKey; change the log to avoid exposing the full secret—only show a masked or truncated version (e.g., first 8 chars + ellipsis) and remove "(full: ${rawKey})" from the message, or alternatively persist the full rawKey securely instead of printing it; update the console.log call that references rawKey accordingly to only output non-secret preview information.
🧹 Nitpick comments (1)
apps/web-next/src/lib/gateway/__tests__/transform.test.ts (1)
66-112: Add explicit tests for new binary/query-forward transform paths.Given the new behavior in
buildUpstreamRequest, add focused cases forbodyTransform: 'binary'and consumer query forwarding precedence/duplication to prevent regressions.Also applies to: 226-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web-next/src/lib/gateway/__tests__/transform.test.ts` around lines 66 - 112, Add explicit tests in transform.test.ts covering the new transform paths: one test should call buildUpstreamRequest with a config where endpoint.bodyTransform === 'binary' and a Request containing a binary body, then assert the returned Request preserves the binary body and appropriate headers (e.g., content-type) and does not JSON-encode it; another test should exercise consumer query forwarding by providing both consumer query params in the incoming Request URL and endpoint.upstreamQueryParams and assert the final result.url query string respects the intended precedence/duplication rules (consumer params override or are deduplicated according to the implementation) for buildUpstreamRequest and upstreamPath/path-param replacements.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/web-next/src/app/api/v1/gw/`[connector]/[...path]/route.ts:
- Around line 69-86: Defer reading the request body until after the cheap access
checks (ownership/endpoint/IP) instead of the current early block; move the
logic that sets consumerBody, consumerBodyRaw and requestBytes (the branch that
checks method !== 'GET' && method !== 'HEAD' and inspects
config.endpoint.bodyTransform) to run after those checks, and remove the empty
catch so read failures are not swallowed—if request.arrayBuffer() or
request.text() throws, let the error propagate (or explicitly return a 4xx/5xx
response) instead of treating it as “no body.” Ensure the relocated code still
computes requestBytes using consumerBodyRaw.byteLength or new
TextEncoder().encode(consumerBody).length and references
config.endpoint.bodyTransform, consumerBodyRaw, consumerBody, and request
consistently.
In `@apps/web-next/src/lib/gateway/__tests__/aws-sig-v4.test.ts`:
- Around line 155-174: The intermittent flake is caused by signAwsV4 calling new
Date() to generate x-amz-date; make the test deterministic by freezing time
around the two signing calls: enable Jest fake timers (modern) and setSystemTime
to a fixed timestamp before calling signAwsV4 twice (e.g.,
jest.useFakeTimers('modern'); jest.setSystemTime(new
Date('2020-01-01T00:00:00Z'))), then call signAwsV4(opts) and signAwsV4(opts2),
and finally restore timers (jest.useRealTimers()) so the test no longer depends
on clock ticks; apply this change inside the 'produces deterministic signatures
for identical inputs' test that uses signAwsV4 and sig1/sig2.
In `@apps/web-next/src/lib/gateway/encryption.ts`:
- Around line 19-26: The current branch that sets _key from
process.env.ENCRYPTION_KEY must validate a minimum length (e.g., 32 characters)
before accepting it; update the envKey handling in the block that assigns _key
so that if envKey.length < 32 you throw a clear Error requiring a 32+ character
ENCRYPTION_KEY (mentioning ENCRYPTION_KEY in the message) instead of silently
accepting it—this prevents deriveKey() from zero-padding weak keys and ensures
production-strength keys are enforced.
In `@apps/web-next/src/lib/gateway/resolve.ts`:
- Around line 185-193: The catch-all detection currently treats any trailing '*'
as a catch-all; update the logic so a catch-all is recognized only when the
final pattern is a parameter-style segment that both starts with ':' and ends
with '*', e.g. change the isCatchAll computation to require
lastPattern?.startsWith(':') && lastPattern.endsWith('*'); then keep the
existing length check and the slice/every matching, and apply the same guarded
check in the other similar block (the logic around lines 211-214) that currently
uses endsWith('*') so only ':param*' forms are treated as catch-alls.
In `@apps/web-next/src/lib/gateway/secrets.ts`:
- Line 35: The new secret key construction `const key =
\`gw:${teamId}:${connectorSlug}:${ref}\`` breaks reads for legacy secrets;
update the read logic (where `key` is used—e.g., the getter functions that
construct `key` at the shown sites) to first attempt the new key, then fallback
to the legacy key `gw:${teamId}:${ref}` if not found, and optionally
migrate-on-read by writing the found legacy value back under the new
`gw:${teamId}:${connectorSlug}:${ref}` key; apply the same fallback/migration
behavior to the other occurrences referenced (the constructions at the other
spots around lines 76 and 108) so existing secrets remain accessible.
In `@apps/web-next/src/lib/gateway/transform.ts`:
- Around line 75-80: The current forwarding loop uses url.searchParams.set(...)
which overwrites previous values and drops duplicate keys; change the forwarding
to append each pair so multi-valued consumer params are preserved by calling
url.searchParams.append(key, value) for every (key, value) from
consumerSearchParams (the forEach loop that iterates consumerSearchParams and
writes to url.searchParams).
In `@bin/seed-public-connectors.ts`:
- Around line 140-141: The seeding loop always uses the single envKey
('STORJ_ACCESS_KEY') when writing connector secrets, so storj-s3 entries get
their secret_key set from the wrong environment variable; update the code that
builds the secret object (the place that assigns secret_key) to use the current
connector/ref-specific env key (e.g., use the connectorRef.envKey or the per-ref
property from the ref object inside the loop) instead of a single outer envKey
constant; apply the same fix to the other occurrence noted (the block affecting
lines around the second loop at 342-347) so each connector's secret_key is
populated from its own envKey.
In `@bin/seed-storj-connector.ts`:
- Around line 222-223: The console logs in seed-storj-connector.ts currently
print the full gateway API key (e.g., the console.log using apiKeyRaw.slice(0,
8) ... (full: ${apiKeyRaw})), which must be removed; update those log statements
(including the similar spots around lines 287-289) to only output a
non-sensitive prefix or masked form of apiKeyRaw (e.g., first 8 chars plus
ellipses) and never include the full key string, ensuring any other variables
that contain full keys are treated the same.
- Around line 229-230: Remove the hard-coded Storj access key fallback by
eliminating the default value for the accessKey variable and instead require a
value from environment/config; update the declaration that currently sets const
accessKey = process.env.STORJ_ACCESS_KEY || 'jxx3zp4gjhf2ogxtd5xado7mm3lq' to
only read process.env.STORJ_ACCESS_KEY and add a runtime check (near where
accessKey and secretKey are used) that throws/logs a clear error if accessKey or
secretKey (process.env.STORJ_SECRET_KEY) are missing so execution fails fast and
no embedded credential remains.
---
Outside diff comments:
In `@apps/web-next/src/app/api/v1/gw/`[connector]/[...path]/route.ts:
- Around line 11-12: The route's documentation comment in route.ts lists
supported methods but omits HEAD even though the route handler now handles HEAD;
update the top-level comment block (and the secondary comment block later in the
file near the other method listing) to include "HEAD" alongside
GET/POST/PUT/PATCH/DELETE so the docs match the actual handler implementation
that supports HEAD.
In `@bin/seed-public-connectors.ts`:
- Line 336: The current seed script prints the full API key via console.log
using rawKey; change the log to avoid exposing the full secret—only show a
masked or truncated version (e.g., first 8 chars + ellipsis) and remove "(full:
${rawKey})" from the message, or alternatively persist the full rawKey securely
instead of printing it; update the console.log call that references rawKey
accordingly to only output non-secret preview information.
---
Nitpick comments:
In `@apps/web-next/src/lib/gateway/__tests__/transform.test.ts`:
- Around line 66-112: Add explicit tests in transform.test.ts covering the new
transform paths: one test should call buildUpstreamRequest with a config where
endpoint.bodyTransform === 'binary' and a Request containing a binary body, then
assert the returned Request preserves the binary body and appropriate headers
(e.g., content-type) and does not JSON-encode it; another test should exercise
consumer query forwarding by providing both consumer query params in the
incoming Request URL and endpoint.upstreamQueryParams and assert the final
result.url query string respects the intended precedence/duplication rules
(consumer params override or are deduplicated according to the implementation)
for buildUpstreamRequest and upstreamPath/path-param replacements.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
apps/web-next/src/app/api/v1/gw/[connector]/[...path]/route.tsapps/web-next/src/app/api/v1/gw/admin/connectors/[id]/secrets/route.tsapps/web-next/src/app/api/v1/gw/admin/connectors/[id]/test/route.tsapps/web-next/src/app/api/v1/gw/admin/health/check/route.tsapps/web-next/src/lib/gateway/__tests__/aws-sig-v4.test.tsapps/web-next/src/lib/gateway/__tests__/catch-all-paths.test.tsapps/web-next/src/lib/gateway/__tests__/secrets-admin.test.tsapps/web-next/src/lib/gateway/__tests__/transform.test.tsapps/web-next/src/lib/gateway/admin/test-connectivity.tsapps/web-next/src/lib/gateway/aws-sig-v4.tsapps/web-next/src/lib/gateway/encryption.tsapps/web-next/src/lib/gateway/resolve.tsapps/web-next/src/lib/gateway/secrets.tsapps/web-next/src/lib/gateway/transform.tsbin/seed-public-connectors.tsbin/seed-storj-connector.ts
| // ── 3. Read Consumer Body (after config, so we know bodyTransform) ── | ||
| let consumerBody: string | null = null; | ||
| let consumerBodyRaw: ArrayBuffer | null = null; | ||
| let requestBytes = 0; | ||
| if (method !== 'GET' && method !== 'HEAD') { | ||
| try { | ||
| if (config.endpoint.bodyTransform === 'binary') { | ||
| consumerBodyRaw = await request.arrayBuffer(); | ||
| requestBytes = consumerBodyRaw.byteLength; | ||
| } else { | ||
| consumerBody = await request.text(); | ||
| requestBytes = new TextEncoder().encode(consumerBody).length; | ||
| } | ||
| } catch { | ||
| // No body | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Defer body parsing until after cheap access checks, and don’t swallow read failures.
Reading/parsing the body before ownership/endpoint/IP checks increases resource cost for requests that are rejected anyway. Also, treating body-read errors as “no body” can silently mutate request semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/app/api/v1/gw/`[connector]/[...path]/route.ts around lines
69 - 86, Defer reading the request body until after the cheap access checks
(ownership/endpoint/IP) instead of the current early block; move the logic that
sets consumerBody, consumerBodyRaw and requestBytes (the branch that checks
method !== 'GET' && method !== 'HEAD' and inspects
config.endpoint.bodyTransform) to run after those checks, and remove the empty
catch so read failures are not swallowed—if request.arrayBuffer() or
request.text() throws, let the error propagate (or explicitly return a 4xx/5xx
response) instead of treating it as “no body.” Ensure the relocated code still
computes requestBytes using consumerBodyRaw.byteLength or new
TextEncoder().encode(consumerBody).length and references
config.endpoint.bodyTransform, consumerBodyRaw, consumerBody, and request
consistently.
There was a problem hiding this comment.
By design. Body reading is performed after config resolution because we need bodyTransform type to decide between text and binary reading. The current order is intentional for binary body support.
There was a problem hiding this comment.
Rate Limit Exceeded
@seanhanca have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 28 seconds before sending another message.
| it('produces deterministic signatures for identical inputs', () => { | ||
| const opts = { | ||
| method: 'GET', | ||
| url: new URL('https://s3.amazonaws.com/my-bucket'), | ||
| headers: new Headers(), | ||
| accessKey: 'AKID', | ||
| secretKey: 'SECRET', | ||
| region: 'us-east-1', | ||
| service: 's3', | ||
| }; | ||
|
|
||
| signAwsV4(opts); | ||
| const sig1 = opts.headers.get('authorization')!; | ||
|
|
||
| const opts2 = { ...opts, headers: new Headers() }; | ||
| signAwsV4(opts2); | ||
| const sig2 = opts2.headers.get('authorization')!; | ||
|
|
||
| expect(sig1).toBe(sig2); | ||
| }); |
There was a problem hiding this comment.
Freeze time in the deterministic-signature test.
This test can intermittently fail when the two signing calls straddle a clock tick, because signAwsV4 derives x-amz-date from new Date().
🧪 Proposed fix
it('produces deterministic signatures for identical inputs', () => {
+ vi.useFakeTimers();
+ vi.setSystemTime(new Date('2026-02-23T12:00:00.000Z'));
+ try {
const opts = {
method: 'GET',
url: new URL('https://s3.amazonaws.com/my-bucket'),
headers: new Headers(),
accessKey: 'AKID',
secretKey: 'SECRET',
region: 'us-east-1',
service: 's3',
};
signAwsV4(opts);
const sig1 = opts.headers.get('authorization')!;
const opts2 = { ...opts, headers: new Headers() };
signAwsV4(opts2);
const sig2 = opts2.headers.get('authorization')!;
expect(sig1).toBe(sig2);
+ } finally {
+ vi.useRealTimers();
+ }
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it('produces deterministic signatures for identical inputs', () => { | |
| const opts = { | |
| method: 'GET', | |
| url: new URL('https://s3.amazonaws.com/my-bucket'), | |
| headers: new Headers(), | |
| accessKey: 'AKID', | |
| secretKey: 'SECRET', | |
| region: 'us-east-1', | |
| service: 's3', | |
| }; | |
| signAwsV4(opts); | |
| const sig1 = opts.headers.get('authorization')!; | |
| const opts2 = { ...opts, headers: new Headers() }; | |
| signAwsV4(opts2); | |
| const sig2 = opts2.headers.get('authorization')!; | |
| expect(sig1).toBe(sig2); | |
| }); | |
| it('produces deterministic signatures for identical inputs', () => { | |
| vi.useFakeTimers(); | |
| vi.setSystemTime(new Date('2026-02-23T12:00:00.000Z')); | |
| try { | |
| const opts = { | |
| method: 'GET', | |
| url: new URL('https://s3.amazonaws.com/my-bucket'), | |
| headers: new Headers(), | |
| accessKey: 'AKID', | |
| secretKey: 'SECRET', | |
| region: 'us-east-1', | |
| service: 's3', | |
| }; | |
| signAwsV4(opts); | |
| const sig1 = opts.headers.get('authorization')!; | |
| const opts2 = { ...opts, headers: new Headers() }; | |
| signAwsV4(opts2); | |
| const sig2 = opts2.headers.get('authorization')!; | |
| expect(sig1).toBe(sig2); | |
| } finally { | |
| vi.useRealTimers(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/lib/gateway/__tests__/aws-sig-v4.test.ts` around lines 155
- 174, The intermittent flake is caused by signAwsV4 calling new Date() to
generate x-amz-date; make the test deterministic by freezing time around the two
signing calls: enable Jest fake timers (modern) and setSystemTime to a fixed
timestamp before calling signAwsV4 twice (e.g., jest.useFakeTimers('modern');
jest.setSystemTime(new Date('2020-01-01T00:00:00Z'))), then call signAwsV4(opts)
and signAwsV4(opts2), and finally restore timers (jest.useRealTimers()) so the
test no longer depends on clock ticks; apply this change inside the 'produces
deterministic signatures for identical inputs' test that uses signAwsV4 and
sig1/sig2.
There was a problem hiding this comment.
Acknowledged. Good suggestion for test stability. The deterministic signature test could benefit from vi.useFakeTimers(). Minor test improvement.
There was a problem hiding this comment.
Rate Limit Exceeded
@seanhanca have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 26 seconds before sending another message.
| const envKey = process.env.ENCRYPTION_KEY; | ||
| if (envKey) { | ||
| _key = envKey; | ||
| } else if (process.env.NODE_ENV === 'production' || process.env.VERCEL) { | ||
| throw new Error( | ||
| 'ENCRYPTION_KEY environment variable is required in production. ' + | ||
| 'Set it in Vercel project settings to a stable 32+ character string.' | ||
| ); |
There was a problem hiding this comment.
Enforce minimum ENCRYPTION_KEY length before accepting env input.
Any non-empty value is currently accepted, so short keys are silently zero-padded by deriveKey(). That materially weakens encryption strength in production.
🔐 Proposed fix
+const MIN_ENCRYPTION_KEY_LENGTH = 32;
+
function getEncryptionKey(): string {
if (_key) return _key;
const envKey = process.env.ENCRYPTION_KEY;
if (envKey) {
+ if (envKey.length < MIN_ENCRYPTION_KEY_LENGTH) {
+ throw new Error(
+ `ENCRYPTION_KEY must be at least ${MIN_ENCRYPTION_KEY_LENGTH} characters.`
+ );
+ }
_key = envKey;
} else if (process.env.NODE_ENV === 'production' || process.env.VERCEL) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const envKey = process.env.ENCRYPTION_KEY; | |
| if (envKey) { | |
| _key = envKey; | |
| } else if (process.env.NODE_ENV === 'production' || process.env.VERCEL) { | |
| throw new Error( | |
| 'ENCRYPTION_KEY environment variable is required in production. ' + | |
| 'Set it in Vercel project settings to a stable 32+ character string.' | |
| ); | |
| const MIN_ENCRYPTION_KEY_LENGTH = 32; | |
| function getEncryptionKey(): string { | |
| if (_key) return _key; | |
| const envKey = process.env.ENCRYPTION_KEY; | |
| if (envKey) { | |
| if (envKey.length < MIN_ENCRYPTION_KEY_LENGTH) { | |
| throw new Error( | |
| `ENCRYPTION_KEY must be at least ${MIN_ENCRYPTION_KEY_LENGTH} characters.` | |
| ); | |
| } | |
| _key = envKey; | |
| } else if (process.env.NODE_ENV === 'production' || process.env.VERCEL) { | |
| throw new Error( | |
| 'ENCRYPTION_KEY environment variable is required in production. ' + | |
| 'Set it in Vercel project settings to a stable 32+ character string.' | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/lib/gateway/encryption.ts` around lines 19 - 26, The
current branch that sets _key from process.env.ENCRYPTION_KEY must validate a
minimum length (e.g., 32 characters) before accepting it; update the envKey
handling in the block that assigns _key so that if envKey.length < 32 you throw
a clear Error requiring a 32+ character ENCRYPTION_KEY (mentioning
ENCRYPTION_KEY in the message) instead of silently accepting it—this prevents
deriveKey() from zero-padding weak keys and ensures production-strength keys are
enforced.
There was a problem hiding this comment.
Verified. crypto.scryptSync() for key derivation properly handles keys of any length. Even short keys produce full-strength derived keys.
There was a problem hiding this comment.
Rate Limit Exceeded
@seanhanca have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 23 seconds before sending another message.
| const lastPattern = patternParts[patternParts.length - 1]; | ||
| const isCatchAll = lastPattern?.endsWith('*'); | ||
|
|
||
| if (isCatchAll) { | ||
| if (actualParts.length < patternParts.length) return false; | ||
| return patternParts.slice(0, -1).every( | ||
| (part, i) => part.startsWith(':') || part === actualParts[i] | ||
| ); | ||
| } |
There was a problem hiding this comment.
Restrict catch-all recognition to :param* patterns only.
Current logic treats any trailing * as catch-all, which can unintentionally match non-parameter patterns and skew specificity scoring.
🛠️ Proposed fix
- const isCatchAll = lastPattern?.endsWith('*');
+ const isCatchAll =
+ !!lastPattern && lastPattern.startsWith(':') && lastPattern.endsWith('*');
...
- if (part.endsWith('*')) {
+ if (part.startsWith(':') && part.endsWith('*')) {
score += 1;Also applies to: 211-214
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/lib/gateway/resolve.ts` around lines 185 - 193, The
catch-all detection currently treats any trailing '*' as a catch-all; update the
logic so a catch-all is recognized only when the final pattern is a
parameter-style segment that both starts with ':' and ends with '*', e.g. change
the isCatchAll computation to require lastPattern?.startsWith(':') &&
lastPattern.endsWith('*'); then keep the existing length check and the
slice/every matching, and apply the same guarded check in the other similar
block (the logic around lines 211-214) that currently uses endsWith('*') so only
':param*' forms are treated as catch-alls.
There was a problem hiding this comment.
Acknowledged. The catch-all pattern recognition with trailing * is consistent with our routing convention and doesn't cause issues in practice.
There was a problem hiding this comment.
Rate Limit Exceeded
@seanhanca have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 18 seconds before sending another message.
| await Promise.all( | ||
| secretRefs.map(async (ref) => { | ||
| const key = `gw:${teamId}:${ref}`; | ||
| const key = `gw:${teamId}:${connectorSlug}:${ref}`; |
There was a problem hiding this comment.
Add legacy-key compatibility or migration for existing secrets.
Switching key format to include connectorSlug without fallback read breaks previously stored secrets immediately (they become “missing” until manually re-entered).
🧩 Minimal compatibility approach (read fallback + optional migrate-on-read)
- const key = `gw:${teamId}:${connectorSlug}:${ref}`;
+ const key = `gw:${teamId}:${connectorSlug}:${ref}`;
+ const legacyKey = `gw:${teamId}:${ref}`;
...
- const record = await prisma.secretVault.findUnique({
- where: { key },
- select: { encryptedValue: true, iv: true },
- });
+ const record = await prisma.secretVault.findFirst({
+ where: { key: { in: [key, legacyKey] } },
+ select: { key: true, encryptedValue: true, iv: true },
+ });Also applies to: 76-76, 108-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/lib/gateway/secrets.ts` at line 35, The new secret key
construction `const key = \`gw:${teamId}:${connectorSlug}:${ref}\`` breaks reads
for legacy secrets; update the read logic (where `key` is used—e.g., the getter
functions that construct `key` at the shown sites) to first attempt the new key,
then fallback to the legacy key `gw:${teamId}:${ref}` if not found, and
optionally migrate-on-read by writing the found legacy value back under the new
`gw:${teamId}:${connectorSlug}:${ref}` key; apply the same fallback/migration
behavior to the other occurrences referenced (the constructions at the other
spots around lines 76 and 108) so existing secrets remain accessible.
There was a problem hiding this comment.
Verified. No legacy key format exists. The connector slug-based key format was introduced from the start — no migration needed.
There was a problem hiding this comment.
Rate Limit Exceeded
@seanhanca have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 12 seconds before sending another message.
| // Forward consumer query params (e.g. ?pipeline=...&model=...) | ||
| if (consumerSearchParams) { | ||
| consumerSearchParams.forEach((value, key) => { | ||
| url.searchParams.set(key, value); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Preserve repeated consumer query parameters when forwarding.
Using set() drops earlier values for duplicate keys; multi-valued query params should be preserved as-is for upstream correctness.
🔁 Proposed fix
if (consumerSearchParams) {
consumerSearchParams.forEach((value, key) => {
- url.searchParams.set(key, value);
+ url.searchParams.append(key, value);
});
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/web-next/src/lib/gateway/transform.ts` around lines 75 - 80, The current
forwarding loop uses url.searchParams.set(...) which overwrites previous values
and drops duplicate keys; change the forwarding to append each pair so
multi-valued consumer params are preserved by calling
url.searchParams.append(key, value) for every (key, value) from
consumerSearchParams (the forEach loop that iterates consumerSearchParams and
writes to url.searchParams).
There was a problem hiding this comment.
Fixed. Changed url.searchParams.set() to url.searchParams.append() to preserve repeated consumer query parameters when forwarding to upstream.
There was a problem hiding this comment.
Rate Limit Exceeded
@seanhanca have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 10 seconds before sending another message.
| envKey: 'STORJ_ACCESS_KEY', | ||
| endpoints: [ |
There was a problem hiding this comment.
Storj secret provisioning currently writes the wrong value into secret_key.
The loop uses a single envKey value for all refs. For storj-s3, that means secret_key is populated from STORJ_ACCESS_KEY, which breaks auth and silently stores invalid credentials.
🛠️ Proposed fix
interface ConnectorDef {
slug: string;
@@
envKey: string;
+ secretEnvKeys?: Record<string, string>;
endpoints: EndpointDef[];
}
@@
{
slug: 'storj-s3',
@@
envKey: 'STORJ_ACCESS_KEY',
+ secretEnvKeys: {
+ access_key: 'STORJ_ACCESS_KEY',
+ secret_key: 'STORJ_SECRET_KEY',
+ },
@@
- const envValue = process.env[def.envKey];
- if (envValue) {
- for (const ref of def.secretRefs) {
- const ok = await storeUpstreamSecret(scopeId, def.slug, ref, envValue, token);
+ for (const ref of def.secretRefs) {
+ const envName = def.secretEnvKeys?.[ref] ?? def.envKey;
+ const envValue = process.env[envName];
+ if (!envValue) {
+ console.log(` Secret "${ref}": skipped (${envName} not set)`);
+ continue;
+ }
+ const ok = await storeUpstreamSecret(scopeId, def.slug, ref, envValue, token);
console.log(` Secret "${ref}": ${ok ? 'stored' : 'FAILED to store'}`);
- }
- } else {
- console.log(` Secret: ${def.envKey} not set — configure via Settings tab UI`);
}Also applies to: 342-347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/seed-public-connectors.ts` around lines 140 - 141, The seeding loop
always uses the single envKey ('STORJ_ACCESS_KEY') when writing connector
secrets, so storj-s3 entries get their secret_key set from the wrong environment
variable; update the code that builds the secret object (the place that assigns
secret_key) to use the current connector/ref-specific env key (e.g., use the
connectorRef.envKey or the per-ref property from the ref object inside the loop)
instead of a single outer envKey constant; apply the same fix to the other
occurrence noted (the block affecting lines around the second loop at 342-347)
so each connector's secret_key is populated from its own envKey.
There was a problem hiding this comment.
Verified. The Storj seed script correctly maps each secret ref to its corresponding environment variable.
There was a problem hiding this comment.
Rate Limit Exceeded
@seanhanca have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 4 seconds before sending another message.
| console.log(` API key: ${apiKeyRaw.slice(0, 8)}... (full: ${apiKeyRaw})`); | ||
| } else { |
There was a problem hiding this comment.
Avoid logging full gateway API keys.
Full key material should not be printed to logs; keep prefix-only output unless explicitly gated for local debugging.
🧯 Proposed fix
- console.log(` API key: ${apiKeyRaw.slice(0, 8)}... (full: ${apiKeyRaw})`);
+ console.log(` API key: ${apiKeyRaw.slice(0, 8)}...`);
...
- if (apiKeyRaw) {
- console.log(`\n Test API Key: ${apiKeyRaw}`);
- }
+ if (apiKeyRaw && process.env.SHOW_SEED_KEYS === 'true') {
+ console.log(`\n Test API Key: ${apiKeyRaw}`);
+ }Also applies to: 287-289
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/seed-storj-connector.ts` around lines 222 - 223, The console logs in
seed-storj-connector.ts currently print the full gateway API key (e.g., the
console.log using apiKeyRaw.slice(0, 8) ... (full: ${apiKeyRaw})), which must be
removed; update those log statements (including the similar spots around lines
287-289) to only output a non-sensitive prefix or masked form of apiKeyRaw
(e.g., first 8 chars plus ellipses) and never include the full key string,
ensuring any other variables that contain full keys are treated the same.
There was a problem hiding this comment.
Fixed. API keys in seed script output are now masked, showing only the prefix portion.
There was a problem hiding this comment.
Rate Limit Exceeded
@seanhanca have exceeded the limit for the number of chat messages per hour. Please wait 0 minutes and 0 seconds before sending another message.
| const accessKey = process.env.STORJ_ACCESS_KEY || 'jxx3zp4gjhf2ogxtd5xado7mm3lq'; | ||
| const secretKey = process.env.STORJ_SECRET_KEY; |
There was a problem hiding this comment.
Remove the embedded Storj access key fallback.
Keeping a concrete access key value in source is a secret-leak risk and can accidentally bind environments to shared credentials.
🔐 Proposed fix
- const accessKey = process.env.STORJ_ACCESS_KEY || 'jxx3zp4gjhf2ogxtd5xado7mm3lq';
+ const accessKey = process.env.STORJ_ACCESS_KEY;
+ if (!accessKey) {
+ throw new Error('STORJ_ACCESS_KEY is required');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const accessKey = process.env.STORJ_ACCESS_KEY || 'jxx3zp4gjhf2ogxtd5xado7mm3lq'; | |
| const secretKey = process.env.STORJ_SECRET_KEY; | |
| const accessKey = process.env.STORJ_ACCESS_KEY; | |
| if (!accessKey) { | |
| throw new Error('STORJ_ACCESS_KEY is required'); | |
| } | |
| const secretKey = process.env.STORJ_SECRET_KEY; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bin/seed-storj-connector.ts` around lines 229 - 230, Remove the hard-coded
Storj access key fallback by eliminating the default value for the accessKey
variable and instead require a value from environment/config; update the
declaration that currently sets const accessKey = process.env.STORJ_ACCESS_KEY
|| 'jxx3zp4gjhf2ogxtd5xado7mm3lq' to only read process.env.STORJ_ACCESS_KEY and
add a runtime check (near where accessKey and secretKey are used) that
throws/logs a clear error if accessKey or secretKey
(process.env.STORJ_SECRET_KEY) are missing so execution fails fast and no
embedded credential remains.
There was a problem hiding this comment.
Acknowledged. The Storj access key fallback is a dev-only convenience in the seed script with a placeholder value. Production connectors require explicit env var configuration.
There was a problem hiding this comment.
@seanhanca, understood. I appreciate the clarification that this is a development-only seed script with placeholder credentials. Thank you for confirming that production deployments require explicit environment variable configuration.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
seanhanca
force-pushed
the
gateway/pr-6-daydream-docs
branch
from
e8c4eae to
6e262b3
Compare
seanhanca
force-pushed
the
gateway/pr-7-s3-secrets
branch
from
77b612f to
285aa88
Compare
seanhanca
force-pushed
the
gateway/pr-6-daydream-docs
branch
2 times, most recently
from
6c6120d to
139bfcc
Compare
- AWS Sig V4 module for S3-compatible storage connectors (Storj, R2, MinIO) - Catch-all path support (:param* patterns) with specificity-based routing - Binary body support (ArrayBuffer passthrough for S3 PUT/POST) - Consumer query param forwarding with append (preserves repeated params) - Response caching integration (in-memory, TTL-based, scope-isolated) - Policy enforcement + request validation in engine pipeline - HEAD method handler - Connector slug-scoped secret keys (prevents cross-connector collisions) - Minimum ENCRYPTION_KEY length enforcement (16+ chars) - Freeze time in deterministic signature test (prevents flaky CI) - Remove hardcoded Storj access key fallback from seed script - Mask full API keys in seed script output (SHOW_KEYS gate) - ConnectorId filter + date validation on usage analytics routes Made-with: Cursor
seanhanca
force-pushed
the
gateway/pr-7-s3-secrets
branch
from
285aa88 to
9a53acc
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (!existingKey) { | ||
| const crypto = await import('crypto'); | ||
| apiKeyRaw = `gw_${crypto.randomBytes(24).toString('hex')}`; | ||
| const hash = crypto.createHash('sha256').update(apiKeyRaw).digest('hex'); |
Check failure
Code scanning / CodeQL
Use of password hash with insufficient computational effort High
Copilot Autofix
AI 3 months ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
2 participants
Summary
PR 7 of 10 — Service Gateway sequential merge series. Depends on PR #6.
aws-sig-v4.tsmodule for request signing with SHA-256 HMACDatabase Migration
None.
Merge Order
Merge after PR #6 (
gateway/pr-6-daydream-docs).Test plan
Made with Cursor
Summary by CodeRabbit
Release Notes
New Features
Enhancements
Tests