Linkry redirect architecture changes #385
Conversation
This will allow us to use the acm-uiuc/js-shared package
WalkthroughThe pull request migrates the linkryEdge Lambda@Edge function from Python to TypeScript, introduces new organization type definitions, modifies the linkry API to support organization IDs and stricter slug validation, removes delegated link record support, and updates infrastructure configuration to deploy the new implementation. Changes
Sequence Diagram(s)sequenceDiagram
participant CF as CloudFront
participant Handler as Lambda@Edge Handler
participant DDB as DynamoDB
participant Origin as Target URL
CF->>Handler: CloudFrontRequestEvent (path)
alt Root path (empty)
Handler->>Origin: 301 redirect to DEFAULT_URL
else Valid slug path
Handler->>Handler: selectReplica(lambdaRegion)
Handler->>DDB: Query slug & access begins_with('OWNER#')
alt Item found with redirect
DDB-->>Handler: redirect URL
Handler->>Origin: 302 redirect with cache headers
else Item not found or no redirect
Handler->>Origin: 307 redirect to FALLBACK_URL
end
end
alt Query error
Handler->>Origin: 307 redirect to FALLBACK_URL (error fallback)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
💰 Infracost reportMonthly estimate generatedThis comment will be updated when code changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (10)
package.json(2 hunks)src/api/functions/linkry.ts(2 hunks)src/common/types/generic.ts(2 hunks)src/common/types/linkry.ts(2 hunks)src/linkryEdgeFunction/build.js(1 hunks)src/linkryEdgeFunction/index.ts(1 hunks)src/linkryEdgeFunction/main.py(0 hunks)src/linkryEdgeFunction/package.json(1 hunks)src/linkryEdgeFunction/tsconfig.json(1 hunks)terraform/modules/lambdas/main.tf(2 hunks)
💤 Files with no reviewable changes (1)
- src/linkryEdgeFunction/main.py
🧰 Additional context used
🪛 ESLint
src/api/functions/linkry.ts
[error] 8-8: Unexpected use of file extension "js" for "common/types/linkry.js"
(import/extensions)
src/common/types/linkry.ts
[error] 2-2: Unexpected use of file extension "js" for "./generic.js"
(import/extensions)
[error] 23-23: Replace '#' with "#"
(prettier/prettier)
[error] 24-24: Insert ,
(prettier/prettier)
src/common/types/generic.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/FAndLzydGk'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/extensions)
[error] 29-29: Insert ⏎···
(prettier/prettier)
[error] 31-31: Insert ,
(prettier/prettier)
[error] 32-32: Insert ;
(prettier/prettier)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Unit Tests
- GitHub Check: Build Application
🔇 Additional comments (11)
src/api/functions/linkry.ts (2)
8-8: LGTM: Import statement correctly updated.The removal of
DelegatedLinkRecordfrom the import aligns with its removal from the type definitions insrc/common/types/linkry.ts.
258-258: LGTM: Type cast correctly updated.The cast to
LinkRecordis consistent with the function's return type and the removal of theDelegatedLinkRecordtype.src/common/types/linkry.ts (3)
2-2: LGTM: Import added for neworgIdfield.The import of
OrgUniqueIdis necessary to support the new optionalorgIdfield increateRequest.
26-26: LGTM: Optional organization ID field added.The addition of the optional
orgIdfield enables linking short URLs to specific ACM sub-organizations without breaking existing API consumers.
10-10: Verify production database has no slugs exceeding 100 characters.The change from 1000 to 100 chars is a breaking API change. All test data shows slugs well under 100 chars (max observed: 9 chars), and the new limit aligns with typical URL slug patterns. However, confirm that existing production database records don't exceed this limit before merging. Run a database query like:
SELECT slug, LENGTH(slug) as len FROM linkry_links WHERE LENGTH(slug) > 100;If any records exist with slugs > 100 chars, data migration will be required.
src/common/types/generic.ts (1)
28-32: Verify the impact ofObject.keys()type inference with your development environment.The concern about type inference is technically valid:
Object.keys(Organizations)returnsstring[]at runtime, which loses TypeScript's literal type information compared to passingOrganizationsdirectly toz.enum().However, I cannot inspect the external
@acm-uiuc/js-sharedpackage to confirm whether this causes actual TypeScript type errors in your codebase. Zod v4 does support passing TypeScript enums directly toz.enum(), so ifOrganizationsis an enum orconstobject, you may get better type inference by using it directly.package.json (1)
9-10: LGTM! Workspace configuration updated correctly.The new
linkryEdgeFunctionworkspace is properly added and integrated into the build pipeline.Also applies to: 16-16
terraform/modules/lambdas/main.tf (1)
15-15: LGTM! Terraform configuration correctly updated for Node.js runtime.The Lambda configuration properly references the new build output directory and uses the correct handler and runtime for the TypeScript implementation.
Also applies to: 512-513
src/linkryEdgeFunction/package.json (1)
1-24: LGTM! Package configuration is well-structured.The package manifest correctly defines the workspace with appropriate scripts, dependencies, and metadata.
src/linkryEdgeFunction/index.ts (2)
48-67: LGTM! Empty path handling is correct.The handler properly redirects root/empty paths to the default URL with a permanent redirect (301) and appropriate caching headers.
119-129: LGTM! Fallback redirect logic is appropriate.The use of 307 (Temporary Redirect) for the fallback case is correct, maintaining the request method and indicating the redirect is not permanent.
| export const OrgUniqueId = z.enum(Object.keys(Organizations)).meta({ | ||
| description: "The unique org ID for a given ACM sub-organization. See https://github.com/acm-uiuc/js-shared/blob/main/src/orgs.ts#L15", | ||
| examples: ["A01", "C01"], | ||
| id: "OrgUniqueId" | ||
| }) |
There was a problem hiding this comment.
Fix formatting issues.
The code has several formatting violations that should be addressed:
- Missing line breaks and indentation
- Missing trailing comma in the metadata object
- Missing semicolon at the end
Run prettier --write src/common/types/generic.ts to auto-fix these issues.
🧰 Tools
🪛 ESLint
[error] 29-29: Insert ⏎···
(prettier/prettier)
[error] 31-31: Insert ,
(prettier/prettier)
[error] 32-32: Insert ;
(prettier/prettier)
🤖 Prompt for AI Agents
In src/common/types/generic.ts around lines 28 to 32, the OrgUniqueId export is
improperly formatted: fix line breaks and indentation so the z.enum call and its
meta object are each on their own lines, add the missing trailing comma after
the meta object properties, and append a semicolon at the end of the statement;
then run prettier --write src/common/types/generic.ts (or apply your project's
formatter) to ensure consistent styling.
| slug: linkrySlug.refine((url) => !url.includes('#'), { | ||
| message: "Slug must not contain a hashtag" | ||
| }), |
There was a problem hiding this comment.
Good validation addition, but fix formatting.
The hashtag validation is sensible since # is a special character in URLs (fragment identifier) and would interfere with slug matching. However, please fix the formatting issues flagged by ESLint (use double quotes and add trailing comma).
- slug: linkrySlug.refine((url) => !url.includes('#'), {
- message: "Slug must not contain a hashtag"
- }),
+ slug: linkrySlug.refine((url) => !url.includes("#"), {
+ message: "Slug must not contain a hashtag",
+ }),📝 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.
| slug: linkrySlug.refine((url) => !url.includes('#'), { | |
| message: "Slug must not contain a hashtag" | |
| }), | |
| slug: linkrySlug.refine((url) => !url.includes("#"), { | |
| message: "Slug must not contain a hashtag", | |
| }), |
🧰 Tools
🪛 ESLint
[error] 23-23: Replace '#' with "#"
(prettier/prettier)
[error] 24-24: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In src/common/types/linkry.ts around lines 23 to 25, the refine call uses single
quotes and is missing a trailing comma; change the string quotes to double
quotes and ensure the object literal has a trailing comma so the line reads with
double-quoted message and ends with a comma to satisfy ESLint formatting rules.
| esbuild | ||
| .build({ | ||
| ...commonParams, | ||
| entryPoints: ["linkryEdgeFunction/index.js"], |
There was a problem hiding this comment.
Critical: Incorrect entry point path.
The entry point is set to "linkryEdgeFunction/index.js", but when this build script runs from the src/linkryEdgeFunction/ directory (via yarn workspace), it would look for src/linkryEdgeFunction/linkryEdgeFunction/index.js, which doesn't exist.
After TypeScript compilation (assuming the tsconfig is fixed to include the current directory), the compiled output should be at ../../dist/linkryEdgeFunction/index.js relative to this build script.
Apply this diff to fix the entry point:
- entryPoints: ["linkryEdgeFunction/index.js"],
+ entryPoints: ["../../dist/linkryEdgeFunction/index.js"],📝 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.
| entryPoints: ["linkryEdgeFunction/index.js"], | |
| entryPoints: ["../../dist/linkryEdgeFunction/index.js"], |
🤖 Prompt for AI Agents
In src/linkryEdgeFunction/build.js around line 33, the entry point is
incorrectly set to "linkryEdgeFunction/index.js" which resolves to
src/linkryEdgeFunction/linkryEdgeFunction/index.js when run from that directory;
change the entryPoints value to point to the compiled output at
"../../dist/linkryEdgeFunction/index.js" so the bundler picks up the post-TS
compilation file (ensure the tsconfig/outDir produces that path).
| function selectReplica(lambdaRegion: string): string { | ||
| // First check if Lambda is running in a replica region | ||
| if (AVAILABLE_REPLICAS.includes(lambdaRegion)) { | ||
| return lambdaRegion; | ||
| } | ||
|
|
||
| // Otherwise, find nearest replica by region prefix matching | ||
| const regionPrefix = lambdaRegion.split("-").slice(0, 2).join("-"); | ||
| if (regionPrefix === "us") { | ||
| return DEFAULT_AWS_REGION; | ||
| } | ||
|
|
||
| for (const replica of AVAILABLE_REPLICAS) { | ||
| if (replica.startsWith(regionPrefix)) { | ||
| return replica; | ||
| } | ||
| } | ||
|
|
||
| return DEFAULT_AWS_REGION; | ||
| } |
There was a problem hiding this comment.
Logic error in region prefix matching.
On line 28, regionPrefix is constructed by taking the first two parts of the region name joined by a dash. For example, "us-east-1" becomes "us-east". However, line 29 checks if regionPrefix === "us", which would never be true since the prefix will always include the geographic area (e.g., "us-east", "us-west").
Apply this diff to fix the logic:
// Otherwise, find nearest replica by region prefix matching
const regionPrefix = lambdaRegion.split("-").slice(0, 2).join("-");
- if (regionPrefix === "us") {
+ if (regionPrefix.startsWith("us")) {
return DEFAULT_AWS_REGION;
}Alternatively, if the intent is to match US regions more broadly:
- const regionPrefix = lambdaRegion.split("-").slice(0, 2).join("-");
- if (regionPrefix === "us") {
+ const regionPrefix = lambdaRegion.split("-")[0];
+ if (regionPrefix === "us") {
return DEFAULT_AWS_REGION;
}📝 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.
| function selectReplica(lambdaRegion: string): string { | |
| // First check if Lambda is running in a replica region | |
| if (AVAILABLE_REPLICAS.includes(lambdaRegion)) { | |
| return lambdaRegion; | |
| } | |
| // Otherwise, find nearest replica by region prefix matching | |
| const regionPrefix = lambdaRegion.split("-").slice(0, 2).join("-"); | |
| if (regionPrefix === "us") { | |
| return DEFAULT_AWS_REGION; | |
| } | |
| for (const replica of AVAILABLE_REPLICAS) { | |
| if (replica.startsWith(regionPrefix)) { | |
| return replica; | |
| } | |
| } | |
| return DEFAULT_AWS_REGION; | |
| } | |
| function selectReplica(lambdaRegion: string): string { | |
| // First check if Lambda is running in a replica region | |
| if (AVAILABLE_REPLICAS.includes(lambdaRegion)) { | |
| return lambdaRegion; | |
| } | |
| // Otherwise, find nearest replica by region prefix matching | |
| const regionPrefix = lambdaRegion.split("-").slice(0, 2).join("-"); | |
| if (regionPrefix.startsWith("us")) { | |
| return DEFAULT_AWS_REGION; | |
| } | |
| for (const replica of AVAILABLE_REPLICAS) { | |
| if (replica.startsWith(regionPrefix)) { | |
| return replica; | |
| } | |
| } | |
| return DEFAULT_AWS_REGION; | |
| } |
🤖 Prompt for AI Agents
In src/linkryEdgeFunction/index.ts around lines 21 to 40, the code builds
regionPrefix as the first two segments (e.g., "us-east") but then checks
regionPrefix === "us" which never matches; fix by either computing regionPrefix
as only the first segment (const regionPrefix = lambdaRegion.split("-")[0];) and
keep the existing equality check, or keep the two-segment prefix and change the
conditional to test for US regions with startsWith (if
(regionPrefix.startsWith("us-")) { return DEFAULT_AWS_REGION; }), and ensure the
rest of the loop still searches AVAILABLE_REPLICAS using the chosen prefix
logic.
| // Query DynamoDB for records with PK=path and SK starting with "OWNER#" | ||
| try { | ||
| const queryParams: QueryCommandInput = { | ||
| TableName: DYNAMODB_TABLE, | ||
| KeyConditionExpression: | ||
| "slug = :slug AND begins_with(access, :owner_prefix)", | ||
| ExpressionAttributeValues: { | ||
| ":slug": { S: path }, | ||
| ":owner_prefix": { S: "OWNER#" }, | ||
| }, | ||
| ProjectionExpression: "redirect", | ||
| Limit: 1, // We only need one result | ||
| }; | ||
|
|
||
| const response = await dynamodb.send(new QueryCommand(queryParams)); | ||
|
|
||
| if (response.Items && response.Items.length > 0) { | ||
| const item = response.Items[0]; | ||
|
|
||
| // Extract the redirect URL from the item | ||
| const redirectUrl = item.redirect?.S; | ||
|
|
||
| if (redirectUrl) { | ||
| console.log(`Found redirect: ${path} -> ${redirectUrl}`); | ||
| return { | ||
| status: "302", | ||
| statusDescription: "Found", | ||
| headers: { | ||
| location: [{ key: "Location", value: redirectUrl }], | ||
| "cache-control": [ | ||
| { key: "Cache-Control", value: `public, max-age=${CACHE_TTL}` }, | ||
| ], | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Verify DynamoDB attribute names and consider URL validation.
The query uses slug and access as attribute names. Please ensure these match the actual DynamoDB table schema (primary key and sort key names).
Additionally, the redirect URL extracted from DynamoDB (line 89) is returned without validation. If an attacker could inject malicious URLs into the database, this could pose a security risk.
Consider adding URL validation before redirecting:
if (redirectUrl) {
// Validate URL format and protocol
try {
const url = new URL(redirectUrl);
if (!['http:', 'https:'].includes(url.protocol)) {
console.log(`Invalid protocol for redirect URL: ${redirectUrl}`);
// Fall through to fallback redirect
} else {
console.log(`Found redirect: ${path} -> ${redirectUrl}`);
return {
status: "302",
statusDescription: "Found",
headers: {
location: [{ key: "Location", value: redirectUrl }],
"cache-control": [
{ key: "Cache-Control", value: `public, max-age=${CACHE_TTL}` },
],
},
};
}
} catch (e) {
console.log(`Invalid URL format: ${redirectUrl}`);
// Fall through to fallback redirect
}
}Run the following script to verify the DynamoDB schema:
🏁 Script executed:
#!/bin/bash
# Verify the DynamoDB table schema for infra-core-api-linkry
# Search for the table definition in Terraform or DynamoDB client code
rg -n "infra-core-api-linkry" -A 10 -B 2 --type=tf
# Search for any LinkryRecord or similar type definitions that show the schema
ast-grep --pattern 'interface $_ {
$$$
slug: $$$
$$$
}'
# Search for DynamoDB operations on the linkry table
rg -n "KeyConditionExpression|AttributeName" -A 5 -B 5 | rg -A 10 -B 10 "linkry"Length of output: 12034
Now I need to check if "slug" and "access" are specifically in the DynamoDB reserved words list. Let me search for that information.
Use ExpressionAttributeNames for attribute names and validate redirect URLs.
Two issues need attention:
-
Attribute naming inconsistency: The query uses direct attribute names
"slug"and"access"in the KeyConditionExpression, but the rest of the linkry codebase consistently uses ExpressionAttributeNames with these same attributes (e.g.,src/api/functions/linkry.tslines 90-94). While"slug"and"access"may not be DynamoDB reserved keywords, using ExpressionAttributeNames when attribute names conflict with reserved words is a defensive best practice throughout your codebase. Update the query to match the codebase pattern:KeyConditionExpression: "#slug = :slug AND begins_with(#access, :owner_prefix)", ExpressionAttributeNames: { "#slug": "slug", "#access": "access", },
-
URL validation missing (line 89): The redirect URL is returned without validation. Add protocol and format validation before redirecting to prevent potential security issues from malicious URLs in the database.
🤖 Prompt for AI Agents
In src/linkryEdgeFunction/index.ts around lines 69 to 103, update the
QueryCommandInput to use ExpressionAttributeNames for the attributes used in
KeyConditionExpression (e.g., change to "#slug" and "#access" and add
ExpressionAttributeNames mapping "#slug"->"slug" and "#access"->"access") to
match the codebase pattern and avoid reserved-word issues; after retrieving
item.redirect, validate the URL (ensure it parses, uses an allowed scheme like
https or http, and is a well-formed absolute URL) and only return the 302
response when the validation passes, otherwise log and handle safely (e.g.,
return a 400/404 or skip redirect) to prevent redirecting to malicious or
malformed URLs.
| "ts-node": { | ||
| "esm": true | ||
| }, | ||
| "include": ["../api/**/*.ts", "../common/**/*.ts"], |
There was a problem hiding this comment.
Critical: Missing current directory in TypeScript includes.
The include array only contains "../api/**/*.ts" and "../common/**/*.ts", which means the current directory's TypeScript files (including index.ts) won't be compiled by tsc. This will cause the build to fail since build.js expects the compiled JavaScript output.
Apply this diff to include the current directory:
- "include": ["../api/**/*.ts", "../common/**/*.ts"],
+ "include": ["../api/**/*.ts", "../common/**/*.ts", "./**/*.ts"],📝 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.
| "include": ["../api/**/*.ts", "../common/**/*.ts"], | |
| "include": ["../api/**/*.ts", "../common/**/*.ts", "./**/*.ts"], |
🤖 Prompt for AI Agents
In src/linkryEdgeFunction/tsconfig.json at line 12, the "include" array omits
the current directory so TypeScript files like index.ts won’t be compiled;
update the "include" array to also include the current directory (e.g., add
"./**/*.ts" or "./*.ts" to the array) so tsc compiles files in this folder along
with ../api and ../common.
Summary by CodeRabbit
New Features
Bug Fixes
Chores