feat:[NEXT-1932]: Authorizer Lambda for Download.#2391
Closed
shaikcyber wants to merge 5 commits into
Closed
Conversation
Contributor
WalkthroughAdds a Lambda@Edge-style Node.js module under Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant CF as CloudFront
participant Handler as AuthHandler
participant JWKS as CognitoJWKS
participant DB as DynamoDB
Client->>CF: GET /download-report/report.csv\nAuthorization: Bearer {token}
CF->>Handler: Lambda@Edge event (request)
Handler->>Handler: Parse domain, folder, Authorization header
Handler->>JWKS: Fetch (or use cached) JWKS
Handler->>Handler: Convert JWK -> PEM, verify JWT (RS256), check token_use, extract tenantId
Handler->>DB: Query `tenant-config` for tenant_id
Handler->>Handler: Compare s3_folder_name & distribution_domain_name with request
alt valid
Handler->>CF: Return original request (allow)
CF->>Client: Serve resource
else invalid
Handler->>CF: Return 401 Unauthorized response
CF->>Client: 401 Unauthorized
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
subsystem/auth-download/index.mjs (3)
104-107: Null-safe access can mask missing configuration.If
s3_folder_nameordistribution_domain_nameis missing from the DynamoDB item, the comparison will beundefined === folderName, which fails silently. Consider explicit validation and logging for debugging.Proposed improvement
const s3FolderName = result.Item.s3_folder_name?.S const s3DistributionDomainName = result.Item.distribution_domain_name?.S + if (!s3FolderName || !s3DistributionDomainName) { + console.error("Tenant config incomplete:", { tenantId, s3FolderName, s3DistributionDomainName }) + return false + } + return (s3FolderName === folderName && s3DistributionDomainName === domainName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystem/auth-download/index.mjs` around lines 104 - 107, The current equality check uses optional chaining (s3FolderName and s3DistributionDomainName) which masks missing keys; update the logic that computes s3FolderName and s3DistributionDomainName (reading from result.Item) to explicitly validate presence (e.g., check result.Item.s3_folder_name and result.Item.distribution_domain_name exist and their .S values are non-empty), log a clear error or warning including the missing field names when absent, and then return a deterministic false (or throw) instead of relying on undefined === folderName; ensure the final comparison still uses the validated s3FolderName/s3DistributionDomainName variables against folderName and domainName.
8-13: Hardcoded configuration should be externalized.The Cognito pool ID, region, and table name are hardcoded. For Lambda@Edge, environment variables are not supported in viewer request/response triggers, but you can:
- Use origin request/response triggers which support environment variables
- Inject configuration at build/deployment time
- Use AWS Systems Manager Parameter Store with caching
The current approach makes it difficult to deploy across environments (dev/staging/prod).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystem/auth-download/index.mjs` around lines 8 - 13, Replace the hardcoded CONFIG object (region, jwksURL, tableName, accessClaim) with injected configuration: read from environment variables (e.g., process.env.AWS_REGION, process.env.COGNITO_JWKS_URL, process.env.TENANT_TABLE_NAME, process.env.ACCESS_CLAIM) with sensible fallbacks, and add an alternative loader that fetches values from AWS Systems Manager Parameter Store with an in-memory TTL cache to support Lambda@Edge constraints; update any code using CONFIG (look for references to CONFIG.region, CONFIG.jwksURL, CONFIG.tableName, CONFIG.accessClaim) to call the new config loader so you can support build-time injection or SSM-backed retrieval depending on deployment environment.
89-89: DynamoDB client instantiated on every request - consider reusing.Creating a new
DynamoDBClientinstance per invocation adds latency. In standard Lambda, the client should be instantiated outside the handler for connection reuse. For Lambda@Edge, this optimization still applies.Proposed fix
+const dynamoClient = new DynamoDBClient({ region: CONFIG.region }) + // Validate access from DynamoDB const validateAccess = async (tenantId, region, domainName, folderName) => { - - const client = new DynamoDBClient({ region }) const command = new GetItemCommand({ TableName: CONFIG.tableName, Key: { tenant_id: { S: tenantId } } }) - const result = await client.send(command) + const result = await dynamoClient.send(command)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystem/auth-download/index.mjs` at line 89, The DynamoDBClient is being created per request (const client = new DynamoDBClient({ region })) which prevents connection reuse; move the instantiation to module scope so the single client is reused across invocations—create the client once at top-level (using the same region determination logic), remove the in-handler/new-per-request creation, and ensure all handler code (that currently references client) uses this top-level DynamoDBClient instance.subsystem/auth-download/test.mjs (1)
21-26: Ensure real tokens are never committed.The empty
value: ""placeholder is correct. Consider adding a.gitignoreentry or using environment variables to prevent accidental token commits during local testing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystem/auth-download/test.mjs` around lines 21 - 26, The authorization entry currently uses a literal placeholder value ("value: \"\"") which risks accidental token commits; replace this with a runtime-sourced secret (e.g., read from an environment variable such as process.env.AUTH_TOKEN or a local .env loaded at test runtime) and ensure any local secrets file is added to .gitignore; retain the authorization array and the "Authorization" key but remove any real token literals so tests pull the token from the environment instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystem/auth-download/index.mjs`:
- Around line 16-34: The cachedKeys object used by getPublicKeys currently never
expires, causing stale PEMs after key rotation; change cachedKeys to store an
entry object like {pems, fetchedAt} and add a configurable TTL (e.g.,
KEY_TTL_MS) at module scope, then in getPublicKeys check cachedKeys[jwksURL] and
if (Date.now() - entry.fetchedAt) < KEY_TTL_MS return entry.pems, otherwise
fetch new keys, build pems, and replace cachedKeys[jwksURL] with {pems,
fetchedAt: Date.now()}; ensure the code path for a cache miss and an expired
entry both perform the same fetch-and-store logic so keys are refreshed when TTL
has passed.
- Around line 22-23: The fetch and JSON parsing for JWKS (const res = await
fetch(jwksURL); const { keys } = await res.json()) lack error handling; wrap the
fetch/res.json logic in a try/catch, check res.ok before parsing (and handle
non-2xx responses), validate that the parsed object contains a keys array before
using it, and on any failure log a clear, non-sensitive error and return/throw a
controlled error so the Lambda doesn't crash or leak stack traces; reference
jwksURL, res, res.json, and keys when implementing these checks and logs.
- Around line 50-52: The code verifies the token and checks decoded.token_use
but omits issuer validation; add a check after jwt.verify (the decoded variable)
to ensure decoded.iss exactly matches the expected issuer URL constructed from
your CONFIG (use CONFIG.region and a new CONFIG.userPoolId) e.g.
"https://cognito-idp.{region}.amazonaws.com/{userPoolId}", returning null (or
rejecting) if it does not match; update CONFIG to include userPoolId and use
that in the validation near the existing jwt.verify/decoded.token_use logic.
In `@subsystem/auth-download/package.json`:
- Line 5: The package.json "main" entry currently points to "index.js" but the
package's actual module file is "index.mjs"; update the "main" field in
package.json to "index.mjs" and ensure the package's module type is declared
(add or verify "type": "module") so Node resolves ES modules correctly when
importing the package.
---
Nitpick comments:
In `@subsystem/auth-download/index.mjs`:
- Around line 104-107: The current equality check uses optional chaining
(s3FolderName and s3DistributionDomainName) which masks missing keys; update the
logic that computes s3FolderName and s3DistributionDomainName (reading from
result.Item) to explicitly validate presence (e.g., check
result.Item.s3_folder_name and result.Item.distribution_domain_name exist and
their .S values are non-empty), log a clear error or warning including the
missing field names when absent, and then return a deterministic false (or
throw) instead of relying on undefined === folderName; ensure the final
comparison still uses the validated s3FolderName/s3DistributionDomainName
variables against folderName and domainName.
- Around line 8-13: Replace the hardcoded CONFIG object (region, jwksURL,
tableName, accessClaim) with injected configuration: read from environment
variables (e.g., process.env.AWS_REGION, process.env.COGNITO_JWKS_URL,
process.env.TENANT_TABLE_NAME, process.env.ACCESS_CLAIM) with sensible
fallbacks, and add an alternative loader that fetches values from AWS Systems
Manager Parameter Store with an in-memory TTL cache to support Lambda@Edge
constraints; update any code using CONFIG (look for references to CONFIG.region,
CONFIG.jwksURL, CONFIG.tableName, CONFIG.accessClaim) to call the new config
loader so you can support build-time injection or SSM-backed retrieval depending
on deployment environment.
- Line 89: The DynamoDBClient is being created per request (const client = new
DynamoDBClient({ region })) which prevents connection reuse; move the
instantiation to module scope so the single client is reused across
invocations—create the client once at top-level (using the same region
determination logic), remove the in-handler/new-per-request creation, and ensure
all handler code (that currently references client) uses this top-level
DynamoDBClient instance.
In `@subsystem/auth-download/test.mjs`:
- Around line 21-26: The authorization entry currently uses a literal
placeholder value ("value: \"\"") which risks accidental token commits; replace
this with a runtime-sourced secret (e.g., read from an environment variable such
as process.env.AUTH_TOKEN or a local .env loaded at test runtime) and ensure any
local secrets file is added to .gitignore; retain the authorization array and
the "Authorization" key but remove any real token literals so tests pull the
token from the environment instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3280b900-82b3-4777-8863-6697302dae3d
📒 Files selected for processing (3)
subsystem/auth-download/index.mjssubsystem/auth-download/package.jsonsubsystem/auth-download/test.mjs
Comment on lines
+16
to
+34
| async function getPublicKeys(jwksURL) { | ||
|
|
||
| if (cachedKeys[jwksURL]) { | ||
| return cachedKeys[jwksURL] | ||
| } | ||
|
|
||
| const res = await fetch(jwksURL) | ||
| const { keys } = await res.json() | ||
|
|
||
| const pems = {} | ||
|
|
||
| keys.forEach(k => { | ||
| pems[k.kid] = jwkToPem(k) | ||
| }) | ||
|
|
||
| cachedKeys[jwksURL] = pems | ||
|
|
||
| return pems | ||
| } |
Contributor
There was a problem hiding this comment.
JWKS cache never expires - stale keys can cause auth failures.
The cachedKeys object caches JWKS indefinitely. When Cognito rotates signing keys, cached PEMs become stale, causing valid tokens to be rejected. Implement TTL-based cache invalidation.
Proposed fix with TTL
-let cachedKeys = {}
+let cachedKeys = {}
+let cacheTimestamp = 0
+const CACHE_TTL_MS = 60 * 60 * 1000 // 1 hour
// Fetch Cognito JWKS
async function getPublicKeys(jwksURL) {
+ const now = Date.now()
+ if (cachedKeys[jwksURL] && (now - cacheTimestamp) < CACHE_TTL_MS) {
+ return cachedKeys[jwksURL]
+ }
- if (cachedKeys[jwksURL]) {
- return cachedKeys[jwksURL]
- }
-
const res = await fetch(jwksURL)
const { keys } = await res.json()
const pems = {}
keys.forEach(k => {
pems[k.kid] = jwkToPem(k)
})
cachedKeys[jwksURL] = pems
+ cacheTimestamp = now
return pems
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystem/auth-download/index.mjs` around lines 16 - 34, The cachedKeys
object used by getPublicKeys currently never expires, causing stale PEMs after
key rotation; change cachedKeys to store an entry object like {pems, fetchedAt}
and add a configurable TTL (e.g., KEY_TTL_MS) at module scope, then in
getPublicKeys check cachedKeys[jwksURL] and if (Date.now() - entry.fetchedAt) <
KEY_TTL_MS return entry.pems, otherwise fetch new keys, build pems, and replace
cachedKeys[jwksURL] with {pems, fetchedAt: Date.now()}; ensure the code path for
a cache miss and an expired entry both perform the same fetch-and-store logic so
keys are refreshed when TTL has passed.
| "name": "lambdanode", | ||
| "version": "1.0.0", | ||
| "description": "", | ||
| "main": "index.js", |
Contributor
There was a problem hiding this comment.
Mismatch between main entry and actual module file.
The main field points to index.js, but the actual handler is in index.mjs. For ES modules, this should be updated.
Proposed fix
- "main": "index.js",
+ "main": "index.mjs",📝 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.
Suggested change
| "main": "index.js", | |
| "main": "index.mjs", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystem/auth-download/package.json` at line 5, The package.json "main"
entry currently points to "index.js" but the package's actual module file is
"index.mjs"; update the "main" field in package.json to "index.mjs" and ensure
the package's module type is declared (add or verify "type": "module") so Node
resolves ES modules correctly when importing the package.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
subsystem/auth-download/index.mjs (1)
87-98: ReuseDynamoDBClientacross invocations.Creating a new SDK client at Line 89 for every request adds avoidable latency to the authorizer hot path and drops connection reuse. Hoist it to module scope once the config is finalized.
♻️ Suggested refactor
let cachedKeys = {} +let dynamoClient @@ const validateAccess = async (tenantId, region, domainName, folderName) => { - - const client = new DynamoDBClient({ region }) + dynamoClient ??= new DynamoDBClient({ region }) @@ - const result = await client.send(command) + const result = await dynamoClient.send(command)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystem/auth-download/index.mjs` around lines 87 - 98, The validateAccess function creates a new DynamoDBClient on every call which prevents connection reuse; move the DynamoDBClient instantiation out of validateAccess to module scope (after CONFIG is finalized) so a single client instance is reused, update references to use the hoisted client (replace local client variable in validateAccess), and ensure any config-dependent initialization happens once before the function is invoked; keep the existing GetItemCommand usage and key construction (tenant_id) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystem/auth-download/index.mjs`:
- Around line 8-13: The CONFIG constant is hardcoded with empty strings (region,
jwksURL, tableName) causing authorization to always fail; update the module to
populate CONFIG from environment or a config source and validate it at startup:
read process.env (e.g., REGION, JWKS_URL, TABLE_NAME) into CONFIG.region,
CONFIG.jwksURL, CONFIG.tableName (or load from SSM/Parameter Store if
preferred), add a startup check that logs and exits or throws when any required
CONFIG field is missing, and ensure validateAccess() and the request handler use
these populated values instead of empty strings so DynamoDB queries and JWKS
validation run correctly.
- Around line 8-13: The code currently builds CONFIG without an audience and
never validates the Cognito token's aud claim; add a new CONFIG field (e.g.,
clientId or audience) and pass it as the audience option to jsonwebtoken.verify
(or verify from jsonwebtoken) so verify enforces the token's aud matches your
expected app client ID. Update any code that calls jsonwebtoken.verify(token,
key, ...) to include { audience: CONFIG.clientId } (or the chosen field name)
and ensure tests/consumers provide the correct client ID in CONFIG.
---
Nitpick comments:
In `@subsystem/auth-download/index.mjs`:
- Around line 87-98: The validateAccess function creates a new DynamoDBClient on
every call which prevents connection reuse; move the DynamoDBClient
instantiation out of validateAccess to module scope (after CONFIG is finalized)
so a single client instance is reused, update references to use the hoisted
client (replace local client variable in validateAccess), and ensure any
config-dependent initialization happens once before the function is invoked;
keep the existing GetItemCommand usage and key construction (tenant_id) intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4d3c30e1-fe0a-4aa0-b871-969f6765c3be
📒 Files selected for processing (2)
subsystem/auth-download/index.mjssubsystem/auth-download/test.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- subsystem/auth-download/test.mjs
Comment on lines
+8
to
+13
| const CONFIG = { | ||
| region: "", | ||
| jwksURL: "", | ||
| tableName: "", | ||
| accessClaim: "custom:accessId" | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### auth-download files ###"
fd -t f . subsystem/auth-download | while read -r f; do
printf '\n--- %s ---\n' "$f"
sed -n '1,240p' "$f"
done
echo
echo "### Look for config injection / templating references ###"
rg -n -C2 'auth-download|jwksURL|tableName|custom:accessId|region:\s*""|jwksURL:\s*""|tableName:\s*""|envsubst|templatefile|replace|terraform|cloudformation|serverless' .Repository: PaladinCloud/CE
Length of output: 50372
CONFIG object has hardcoded empty strings that prevent any authorization.
The CONFIG object (lines 8-13) declares region, jwksURL, and tableName as empty strings with no mechanism to populate them. The handler returns 401 for every request at line 117-119 because CONFIG.region and CONFIG.jwksURL are falsy. Even if those were somehow set, CONFIG.tableName remains empty and would cause validateAccess() to fail at line 92 when querying DynamoDB.
There is no code reading from environment variables, configuration files, or any other external source to hydrate these values. Either:
- Add code to read configuration from environment variables or AWS Systems Manager Parameter Store, or
- Accept that this module requires a deployment-time code injection mechanism and document it explicitly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystem/auth-download/index.mjs` around lines 8 - 13, The CONFIG constant
is hardcoded with empty strings (region, jwksURL, tableName) causing
authorization to always fail; update the module to populate CONFIG from
environment or a config source and validate it at startup: read process.env
(e.g., REGION, JWKS_URL, TABLE_NAME) into CONFIG.region, CONFIG.jwksURL,
CONFIG.tableName (or load from SSM/Parameter Store if preferred), add a startup
check that logs and exits or throws when any required CONFIG field is missing,
and ensure validateAccess() and the request handler use these populated values
instead of empty strings so DynamoDB queries and JWKS validation run correctly.
🧩 Analysis chain
🌐 Web query:
Does Amazon Cognito place the app client ID in the audclaim for ID tokens, and doesjsonwebtoken.verify()support enforcing that with theaudience option?
💡 Result:
Yes.
- Amazon Cognito (User Pools) ID tokens include an
audclaim whose value should match the app client ID (the client that requested the token). [1] jsonwebtoken.verify()supports enforcing this via theaudienceoption, which validates the token’saudclaim (and fails verification on mismatch). [2]
Sources:
[1] AWS Cognito Developer Guide – Verifying a JWT (ID token aud matches app client ID) (docs.aws.amazon.com)
[2] auth0/node-jsonwebtoken README – jwt.verify(..., { audience: ... }) (github.com)
Citations:
- 1: https://docs.aws.amazon.com/cognito/latest/developerguide/amazon-cognito-user-pools-using-tokens-verifying-a-jwt.html?utm_source=openai
- 2: https://github.com/auth0/node-jsonwebtoken?utm_source=openai
🏁 Script executed:
cat -n subsystem/auth-download/index.mjs | head -60Repository: PaladinCloud/CE
Length of output: 1762
Bind JWT verification to the expected app client.
The code accepts any valid Cognito ID token from the same pool because it never validates the aud claim. Every ID token from Cognito includes aud set to the app client ID, and jsonwebtoken.verify() can enforce this. Add audience validation to prevent tokens minted for different app clients from being accepted.
Suggested fix
const CONFIG = {
region: "",
jwksURL: "",
tableName: "",
+ audience: "",
accessClaim: "custom:accessId"
}
- const decoded = jwt.verify(token, pem, { algorithms: ['RS256'] })
+ const decoded = jwt.verify(token, pem, {
+ algorithms: ['RS256'],
+ audience: CONFIG.audience
+ })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystem/auth-download/index.mjs` around lines 8 - 13, The code currently
builds CONFIG without an audience and never validates the Cognito token's aud
claim; add a new CONFIG field (e.g., clientId or audience) and pass it as the
audience option to jsonwebtoken.verify (or verify from jsonwebtoken) so verify
enforces the token's aud matches your expected app client ID. Update any code
that calls jsonwebtoken.verify(token, key, ...) to include { audience:
CONFIG.clientId } (or the chosen field name) and ensure tests/consumers provide
the correct client ID in CONFIG.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
subsystem/auth-download/index.mjs (2)
51-53:⚠️ Potential issue | 🟠 MajorBind JWT verification to the expected app client.
This still accepts any valid ID token from the same user pool because
audis never checked. The Go authorizer already validates audience insubsystem/auth/services/svc-api-authorizer/utils/jwt/utils_jwt.go:139-146; this handler should do the same.Suggested fix
const CONFIG = { region: process.env.REGION ?? "", jwksURL: process.env.JWKS_URL ?? "", issuer: process.env.ISSUER ?? "", + audience: process.env.AUDIENCE ?? "", tableName: process.env.TABLE_NAME ?? "", accessClaim: "custom:accessId" } - const decoded = jwt.verify(token, pem, { algorithms: ['RS256'], issuer: CONFIG.issuer }) + const decoded = jwt.verify(token, pem, { + algorithms: ['RS256'], + issuer: CONFIG.issuer, + audience: CONFIG.audience + })Does jsonwebtoken.verify() support enforcing the JWT `aud` claim via the `audience` option, and do Amazon Cognito ID tokens set `aud` to the app client ID?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystem/auth-download/index.mjs` around lines 51 - 53, The JWT verification currently only checks token_use; update the jwt.verify call to enforce the expected audience by passing the audience option (e.g., jwt.verify(token, pem, { algorithms: ['RS256'], issuer: CONFIG.issuer, audience: CONFIG.clientId })) or, if you prefer explicit post-checking, after decoding verify decoded.aud === CONFIG.clientId in addition to decoded.token_use === 'id'; use the existing symbols jwt.verify, token, pem, CONFIG.clientId and decoded.token_use/decoded.aud to locate and implement the change.
8-14:⚠️ Potential issue | 🔴 CriticalHydrate and validate all required config before serving requests.
CONFIGis still initialized with empty strings, and the handler only guardsregion/jwksURL. Withjwt.verify(..., { issuer: CONFIG.issuer }), an emptyissuerrejects every valid token, and an emptytableNamepushes the DynamoDB path into a runtime failure instead of a clear startup error.Suggested fix
const CONFIG = { - region: "", - jwksURL: "", - issuer: "", - tableName: "", + region: process.env.REGION ?? "", + jwksURL: process.env.JWKS_URL ?? "", + issuer: process.env.ISSUER ?? "", + tableName: process.env.TABLE_NAME ?? "", accessClaim: "custom:accessId" } + +const REQUIRED_CONFIG_KEYS = ["region", "jwksURL", "issuer", "tableName"] // Lambda@Edge handler export const handler = async (event) => { try { - if (!CONFIG.region || !CONFIG.jwksURL) { + const missing = REQUIRED_CONFIG_KEYS.filter((key) => !CONFIG[key]) + if (missing.length) { + console.error("Missing auth-download config:", missing.join(", ")) return unauthorized() }Also applies to: 118-120
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystem/auth-download/index.mjs` around lines 8 - 14, CONFIG is being initialized with empty strings and not fully validated, causing jwt.verify(..., { issuer: CONFIG.issuer }) to always fail and DynamoDB usage to error at runtime; update startup to hydrate CONFIG from env/config source and validate required fields (at minimum region, jwksURL, issuer, tableName) before serving requests, throwing a clear startup error if any are missing or empty, and ensure code paths that call jwt.verify (using CONFIG.issuer) and DynamoDB access (using CONFIG.tableName) rely on the validated CONFIG; also consider validating accessClaim if your code expects a specific format.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystem/auth-download/index.mjs`:
- Around line 88-99: CONFIG is declared but never populated from environment
variables, causing CONFIG.tableName, CONFIG.region, and CONFIG.jwksURL to be
empty when validateAccess and other functions run; populate CONFIG at module
initialization (before validateAccess is used) by reading from process.env
(e.g., TABLE_NAME, REGION, JWKS_URL) and assign to CONFIG.tableName,
CONFIG.region, CONFIG.jwksURL (with optional defaults or throw if missing) so
that validateAccess and DynamoDBClient creation use the real values.
---
Duplicate comments:
In `@subsystem/auth-download/index.mjs`:
- Around line 51-53: The JWT verification currently only checks token_use;
update the jwt.verify call to enforce the expected audience by passing the
audience option (e.g., jwt.verify(token, pem, { algorithms: ['RS256'], issuer:
CONFIG.issuer, audience: CONFIG.clientId })) or, if you prefer explicit
post-checking, after decoding verify decoded.aud === CONFIG.clientId in addition
to decoded.token_use === 'id'; use the existing symbols jwt.verify, token, pem,
CONFIG.clientId and decoded.token_use/decoded.aud to locate and implement the
change.
- Around line 8-14: CONFIG is being initialized with empty strings and not fully
validated, causing jwt.verify(..., { issuer: CONFIG.issuer }) to always fail and
DynamoDB usage to error at runtime; update startup to hydrate CONFIG from
env/config source and validate required fields (at minimum region, jwksURL,
issuer, tableName) before serving requests, throwing a clear startup error if
any are missing or empty, and ensure code paths that call jwt.verify (using
CONFIG.issuer) and DynamoDB access (using CONFIG.tableName) rely on the
validated CONFIG; also consider validating accessClaim if your code expects a
specific format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 22251242-f21f-4488-891e-0d30d573c853
📒 Files selected for processing (2)
subsystem/auth-download/index.mjssubsystem/auth-download/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- subsystem/auth-download/package.json
Comment on lines
+88
to
+99
| const validateAccess = async (tenantId, region, domainName, folderName) => { | ||
|
|
||
| const client = new DynamoDBClient({ region }) | ||
|
|
||
| const command = new GetItemCommand({ | ||
| TableName: CONFIG.tableName, | ||
| Key: { | ||
| tenant_id: { S: tenantId } | ||
| } | ||
| }) | ||
|
|
||
| const result = await client.send(command) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### auth-download deployment references ###"
rg -n -C2 'auth-download|tenant-config|TABLE_NAME|DYNAMODB_TABLE_NAME|requireDynamoDB|GetItemCommand|DynamoDBClient|JWKS_URL|ISSUER|AUDIENCE' .
echo
echo "### manifests mentioning auth-download or DynamoDB ###"
fd 'package\.json|serverless|template|yaml|yml|tf|json' . | while read -r f; do
if rg -q 'auth-download|tenant-config|DynamoDB|TABLE_NAME|requireDynamoDB' "$f"; then
printf '\n--- %s ---\n' "$f"
sed -n '1,220p' "$f"
fi
doneRepository: PaladinCloud/CE
Length of output: 41496
🏁 Script executed:
#!/bin/bash
# Search for auth-download deployment config
echo "### Files in auth-download directory ###"
fd . subsystem/auth-download/ --type f
echo
echo "### package.json or serverless config in auth-download ###"
cat subsystem/auth-download/package.json 2>/dev/null || echo "No package.json found"
echo
echo "### Checking for TABLE_NAME or DYNAMODB config in auth-download directory ###"
rg -n 'TABLE_NAME|DYNAMODB_TABLE|CONFIG\.' subsystem/auth-download/ || echo "No matches"
echo
echo "### Full content of auth-download index.mjs ###"
head -120 subsystem/auth-download/index.mjs | tail -40Repository: PaladinCloud/CE
Length of output: 2376
🏁 Script executed:
#!/bin/bash
# Check where CONFIG is initialized in index.mjs
echo "### CONFIG initialization in auth-download/index.mjs ###"
head -60 subsystem/auth-download/index.mjs
echo
echo "### Search for environment variable loading ###"
rg -n 'process\.env|CONFIG\s*=' subsystem/auth-download/index.mjsRepository: PaladinCloud/CE
Length of output: 1506
🏁 Script executed:
#!/bin/bash
# Check entire file for CONFIG population or environment variable references
echo "### Entire auth-download/index.mjs file ###"
cat subsystem/auth-download/index.mjsRepository: PaladinCloud/CE
Length of output: 4086
CONFIG object is never initialized from environment variables; this prevents deployment.
The code initializes CONFIG as an object with empty string values (lines 8-13) but contains no code to populate these properties from environment variables. When the handler executes, CONFIG.tableName, CONFIG.region, and CONFIG.jwksURL remain empty strings, causing the DynamoDB query at line 93 and the validation check at line 118 to fail. Add code to read environment variables into CONFIG (e.g., via process.env.TABLE_NAME) before the handler is invoked, or configure these values through Lambda initialization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystem/auth-download/index.mjs` around lines 88 - 99, CONFIG is declared
but never populated from environment variables, causing CONFIG.tableName,
CONFIG.region, and CONFIG.jwksURL to be empty when validateAccess and other
functions run; populate CONFIG at module initialization (before validateAccess
is used) by reading from process.env (e.g., TABLE_NAME, REGION, JWKS_URL) and
assign to CONFIG.tableName, CONFIG.region, CONFIG.jwksURL (with optional
defaults or throw if missing) so that validateAccess and DynamoDBClient creation
use the real values.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
subsystem/auth-download/index.mjs (1)
20-48:⚠️ Potential issue | 🔴 CriticalCache TTL is broken:
cacheTimestampis never updated after fetching keys.The TTL check at line 23 uses
cacheTimestamp, but after successfully fetching and caching keys (lines 45-47),cacheTimestampis never updated from its initial value of0. This means:
cachedKeys[jwksURL]gets populatedcacheTimestampstays0- On next call:
(now - 0)is always >>CACHE_TTL_MS, so the condition fails- Keys are re-fetched on every invocation, negating the cache entirely
Proposed fix
cachedKeys[jwksURL] = pems + cacheTimestamp = now return pems }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystem/auth-download/index.mjs` around lines 20 - 48, The cache TTL never works because cacheTimestamp is never updated after getPublicKeys fetches keys; fix by recording the fetch time when you populate cachedKeys so the TTL check uses a fresh timestamp—either set cacheTimestamp = Date.now() immediately after assigning cachedKeys[jwksURL] in getPublicKeys or (better) maintain a per-URL timestamp map (e.g., cacheTimestamps[jwksURL] = Date.now()) and update the code that checks CACHE_TTL_MS to use that per-URL timestamp; ensure you reference and update the same timestamp variable you use in the initial TTL comparison.
🧹 Nitpick comments (1)
subsystem/auth-download/index.mjs (1)
100-106: DynamoDBClient instantiated on every invocation degrades performance.Creating a new
DynamoDBClientper request adds latency and connection overhead. Lambda@Edge reuses execution contexts across warm invocations, so the client should be instantiated once at module scope to benefit from connection reuse.Proposed fix (after CONFIG is populated from env vars)
import { DynamoDBClient, GetItemCommand } from "@aws-sdk/client-dynamodb" let cachedKeys = {} let cacheTimestamp = 0 const CACHE_TTL_MS = 60 * 60 * 1000 // 1 hour + +// Initialize client at module scope for connection reuse +let dynamoClient = null +const getDynamoClient = (region) => { + if (!dynamoClient) { + dynamoClient = new DynamoDBClient({ region }) + } + return dynamoClient +} // ... const validateAccess = async (tenantId, region, domainName, folderName) => { - const client = new DynamoDBClient({ region }) + const client = getDynamoClient(region) const command = new GetItemCommand({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@subsystem/auth-download/index.mjs` around lines 100 - 106, The DynamoDBClient is being created inside validateAccess on every invocation; move the client instantiation to module scope so Lambda warm starts can reuse the connection. Create a single shared DynamoDBClient (e.g., const dynamoClient = new DynamoDBClient({ region: CONFIG.region || process.env.AWS_REGION })) after CONFIG is populated from env vars, then update validateAccess to use that shared dynamoClient instead of new DynamoDBClient({ region }) while keeping GetItemCommand and existing parameter usage intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@subsystem/auth-download/index.mjs`:
- Around line 131-133: The initial config check currently returns unauthorized()
when CONFIG.region or CONFIG.jwksURL are missing but omits CONFIG.tableName;
update the validation to also verify CONFIG.tableName is present (non-empty)
before proceeding so validateAccess() won't attempt a DynamoDB query with a
missing table name. Locate the validation block that checks CONFIG.region and
CONFIG.jwksURL and add a check for CONFIG.tableName (used by validateAccess())
and return unauthorized() if it's missing.
---
Duplicate comments:
In `@subsystem/auth-download/index.mjs`:
- Around line 20-48: The cache TTL never works because cacheTimestamp is never
updated after getPublicKeys fetches keys; fix by recording the fetch time when
you populate cachedKeys so the TTL check uses a fresh timestamp—either set
cacheTimestamp = Date.now() immediately after assigning cachedKeys[jwksURL] in
getPublicKeys or (better) maintain a per-URL timestamp map (e.g.,
cacheTimestamps[jwksURL] = Date.now()) and update the code that checks
CACHE_TTL_MS to use that per-URL timestamp; ensure you reference and update the
same timestamp variable you use in the initial TTL comparison.
---
Nitpick comments:
In `@subsystem/auth-download/index.mjs`:
- Around line 100-106: The DynamoDBClient is being created inside validateAccess
on every invocation; move the client instantiation to module scope so Lambda
warm starts can reuse the connection. Create a single shared DynamoDBClient
(e.g., const dynamoClient = new DynamoDBClient({ region: CONFIG.region ||
process.env.AWS_REGION })) after CONFIG is populated from env vars, then update
validateAccess to use that shared dynamoClient instead of new DynamoDBClient({
region }) while keeping GetItemCommand and existing parameter usage intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: de10f845-d9bf-4627-ad01-c0d782b38741
📒 Files selected for processing (1)
subsystem/auth-download/index.mjs
Comment on lines
+131
to
+133
| if (!CONFIG.region || !CONFIG.jwksURL) { | ||
| return unauthorized() | ||
| } |
Contributor
There was a problem hiding this comment.
Add CONFIG.tableName to the initial configuration validation.
The check validates CONFIG.region and CONFIG.jwksURL but omits CONFIG.tableName, which is required by validateAccess() at line 106. If tableName is empty, the DynamoDB query will fail with an error instead of returning a clean 401.
Proposed fix
- if (!CONFIG.region || !CONFIG.jwksURL) {
+ if (!CONFIG.region || !CONFIG.jwksURL || !CONFIG.tableName) {
return unauthorized()
}📝 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.
Suggested change
| if (!CONFIG.region || !CONFIG.jwksURL) { | |
| return unauthorized() | |
| } | |
| if (!CONFIG.region || !CONFIG.jwksURL || !CONFIG.tableName) { | |
| return unauthorized() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@subsystem/auth-download/index.mjs` around lines 131 - 133, The initial config
check currently returns unauthorized() when CONFIG.region or CONFIG.jwksURL are
missing but omits CONFIG.tableName; update the validation to also verify
CONFIG.tableName is present (non-empty) before proceeding so validateAccess()
won't attempt a DynamoDB query with a missing table name. Locate the validation
block that checks CONFIG.region and CONFIG.jwksURL and add a check for
CONFIG.tableName (used by validateAccess()) and return unauthorized() if it's
missing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Please include a summary of the changes and the related issues. Please also include relevant motivation and context. List
any dependencies that are required for this change.
Problem
Solution
Fixes # (issue if any)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also
list any relevant details for your test configuration
Checklist:
Other Information:
List any documentation updates that are needed for the Wiki
Summary by CodeRabbit
New Features
Tests