add services - #295
Conversation
WalkthroughA service layer architecture is introduced with new business logic modules for base64 encoding/decoding, basic authentication, redirect validation, shutdown control, and HTTP status validation. Route handlers are refactored to delegate to these services instead of performing inline validation and error handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a dedicated service layer to the application, extracting business logic from route handlers into reusable service functions. This refactoring improves code organization, maintainability, and testability by ensuring that route handlers remain thin and primarily focus on request/response orchestration, while the core application logic resides in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Logic now resides, Routes are thin, a cleaner path, Services now shine. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully introduces a service layer to separate business logic from route handlers, which is a great improvement for maintainability. The new service files are well-implemented and follow the intended design of returning result objects instead of throwing errors.
I've pointed out a minor inconsistency in src/routes/base64.ts where unnecessary try...catch blocks were used around service calls. Removing them will make the code cleaner and more consistent with the new architecture. Overall, this is a solid refactoring effort.
| let result; | ||
| try { | ||
| const encoded = Buffer.from(valueToEncode, 'utf8').toString('base64'); | ||
| res.status(HttpStatusCodes.OK).json({ encoded }); | ||
| } catch (error) { | ||
| log.error(error, 'Failed to encode value to Base64'); | ||
| res.status(HttpStatusCodes.INTERNAL_SERVER_ERROR).json({ | ||
| error: { | ||
| message: 'Failed to encode value to Base64', | ||
| }, | ||
| }); | ||
| result = base64Encode(req.body); | ||
| } catch (err) { | ||
| log.error({ err }, 'Failed to encode value to Base64'); | ||
| res | ||
| .status(HttpStatusCodes.INTERNAL_SERVER_ERROR) | ||
| .json({ error: { message: 'Failed to encode value to Base64' } }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The try...catch block is unnecessary here. According to the new architecture described in AGENTS.md, service functions are designed to return a result object and not throw exceptions for expected errors. Removing the try...catch will make the code cleaner and more consistent with the other route handlers in this PR.
| let result; | |
| try { | |
| const encoded = Buffer.from(valueToEncode, 'utf8').toString('base64'); | |
| res.status(HttpStatusCodes.OK).json({ encoded }); | |
| } catch (error) { | |
| log.error(error, 'Failed to encode value to Base64'); | |
| res.status(HttpStatusCodes.INTERNAL_SERVER_ERROR).json({ | |
| error: { | |
| message: 'Failed to encode value to Base64', | |
| }, | |
| }); | |
| result = base64Encode(req.body); | |
| } catch (err) { | |
| log.error({ err }, 'Failed to encode value to Base64'); | |
| res | |
| .status(HttpStatusCodes.INTERNAL_SERVER_ERROR) | |
| .json({ error: { message: 'Failed to encode value to Base64' } }); | |
| return; | |
| } | |
| const result = base64Encode(req.body); |
| let result; | ||
| try { | ||
| const decodedBuffer = Buffer.from(valueToDecode, 'base64'); | ||
|
|
||
| // Validate Base64 format | ||
| if (decodedBuffer.toString('base64') !== valueToDecode) { | ||
| res.status(HttpStatusCodes.BAD_REQUEST).json({ | ||
| error: { message: 'Invalid Base64 format' }, | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| const decoded = decodedBuffer.toString('utf8'); | ||
| res.status(HttpStatusCodes.OK).json({ decoded }); | ||
| } catch (error) { | ||
| log.error(error, 'An unexpected error occurred during decoding.'); | ||
| res.status(HttpStatusCodes.INTERNAL_SERVER_ERROR).json({ | ||
| error: { message: 'An unexpected error occurred during decoding.' }, | ||
| }); | ||
| result = base64Decode(req.body); | ||
| } catch (err) { | ||
| log.error({ err }, 'An unexpected error occurred during decoding.'); | ||
| res | ||
| .status(HttpStatusCodes.INTERNAL_SERVER_ERROR) | ||
| .json({ error: { message: 'An unexpected error occurred during decoding.' } }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Similar to the /encode endpoint, this try...catch block is not needed. The base64Decode service function handles errors by returning a result object, so it won't throw. Removing the try...catch will improve consistency and simplify the code.
| let result; | |
| try { | |
| const decodedBuffer = Buffer.from(valueToDecode, 'base64'); | |
| // Validate Base64 format | |
| if (decodedBuffer.toString('base64') !== valueToDecode) { | |
| res.status(HttpStatusCodes.BAD_REQUEST).json({ | |
| error: { message: 'Invalid Base64 format' }, | |
| }); | |
| return; | |
| } | |
| const decoded = decodedBuffer.toString('utf8'); | |
| res.status(HttpStatusCodes.OK).json({ decoded }); | |
| } catch (error) { | |
| log.error(error, 'An unexpected error occurred during decoding.'); | |
| res.status(HttpStatusCodes.INTERNAL_SERVER_ERROR).json({ | |
| error: { message: 'An unexpected error occurred during decoding.' }, | |
| }); | |
| result = base64Decode(req.body); | |
| } catch (err) { | |
| log.error({ err }, 'An unexpected error occurred during decoding.'); | |
| res | |
| .status(HttpStatusCodes.INTERNAL_SERVER_ERROR) | |
| .json({ error: { message: 'An unexpected error occurred during decoding.' } }); | |
| return; | |
| } | |
| const result = base64Decode(req.body); |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| res.redirect(redirectStatus, url); | ||
| res.redirect(result.status, result.url); |
Check warning
Code scanning / CodeQL
Server-side URL redirect Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 5 months ago
In general terms, the problem should be fixed by validating or constraining the redirect target derived from user input before passing it to res.redirect. Typical strategies are: (a) allow only relative URLs (paths) that stay on the same host, (b) enforce a domain whitelist, or (c) map user-provided keys to a server-side whitelist of concrete URLs. Any user-provided absolute URL to an arbitrary origin should be rejected or normalized to a safe default.
The single best way to fix this without changing existing functionality too much is to add URL validation in src/services/redirect.ts, because that is where the redirect parameters are already being validated. We can add a small helper isLocalUrl that checks that the provided URL is either a relative path or resolves to the same origin as the application. Since we cannot assume the real domain name, we can use a placeholder base (such as https://example.com) in the URL constructor and ensure that the computed origin matches this base; additionally, we accept URLs that start with / (relative paths) and reject any that are protocol-relative (//evil.com) or absolute to a different host. If the URL is invalid or external, we return an error (e.g., HTTP 400) instead of a success result. This keeps src/routes/redirect.ts unchanged in behavior for valid internal URLs, but prevents open redirects to arbitrary domains.
Concretely:
- In
src/services/redirect.ts, add a helperisSafeRedirectUrl(url: string): booleanabove theredirectfunction. It should:- Reject empty strings.
- Accept relative paths that start with
/and do not start with//. - For other inputs, construct
new URL(url, BASE)with a fixed origin like"https://example.com"and ensure the resultingoriginequals the base origin.
- In the
redirectfunction, after checking thaturlParamis present and before returning success, callisSafeRedirectUrl(urlParam). If it returns false, return{ ok: false, status: HttpStatusCodes.BAD_REQUEST, body: { error: { message: 'Invalid redirect url' } } }. - No changes are required in
src/routes/redirect.ts, since it already handlesok: falseresponses by returning the error JSON and avoids callingres.redirectin that case. - No new external dependencies are needed; we just use the built-in
URLclass available in Node.js.
| @@ -6,6 +6,37 @@ | ||
| | { ok: false; status: number; body: { error: { message: string } } }; | ||
|
|
||
| /** | ||
| * Checks whether the given URL is safe to use as a redirect target. | ||
| * | ||
| * A safe URL is either: | ||
| * - a relative path starting with a single "/", or | ||
| * - an absolute URL that, when resolved against a fixed base, keeps the same origin. | ||
| */ | ||
| function isSafeRedirectUrl(url: string): boolean { | ||
| if (!url) { | ||
| return false; | ||
| } | ||
|
|
||
| // Disallow protocol-relative URLs like "//evil.com" | ||
| if (url.startsWith('//')) { | ||
| return false; | ||
| } | ||
|
|
||
| // Allow simple relative paths | ||
| if (url.startsWith('/')) { | ||
| return true; | ||
| } | ||
|
|
||
| try { | ||
| const base = 'https://example.com'; | ||
| const parsed = new URL(url, base); | ||
| return parsed.origin === base; | ||
| } catch { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Validates redirect parameters and returns a result object. | ||
| * | ||
| * @param urlParam - The target URL from the query parameter. | ||
| @@ -24,6 +55,14 @@ | ||
| }; | ||
| } | ||
|
|
||
| if (!isSafeRedirectUrl(urlParam)) { | ||
| return { | ||
| ok: false, | ||
| status: HttpStatusCodes.BAD_REQUEST, | ||
| body: { error: { message: 'Invalid redirect url' } }, | ||
| }; | ||
| } | ||
|
|
||
| const redirectStatus = statusParam ? toSafeInteger(statusParam) : HttpStatusCodes.FOUND; | ||
|
|
||
| if (redirectStatus === undefined || !RedirectStatuses.has(redirectStatus)) { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/services/shutdown.ts (1)
3-5: Returnstatuson the success branch too.Line 22 is the only new service success path here without a
status, which forces callers to special-case this result instead of handling every service response the same way.♻️ Proposed change
type ShutdownResult = - | { ok: true; body: { message: string } } + | { ok: true; status: number; body: { message: string } } | { ok: false; status: number; body: { error: { message: string } } }; @@ - return { ok: true, body: { message: 'Server shutting down' } }; + return { + ok: true, + status: HttpStatusCodes.OK, + body: { message: 'Server shutting down' }, + };Also applies to: 13-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/shutdown.ts` around lines 3 - 5, The success variant of the ShutdownResult type is missing a status field which forces callers to special-case responses; update the type so the ok: true branch includes status: number (e.g., change { ok: true; body: { message: string } } to { ok: true; status: number; body: { message: string } }) and then ensure any functions that return a successful shutdown (the code paths that construct ShutdownResult with ok: true) include an appropriate HTTP status value so all callers can uniformly read .status on both branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 37: Update the service contract documentation line so it describes the
actual discriminated-union return shape used by the new services: mention that
each service function returns a result object with an "ok" boolean discriminator
(e.g., { ok: true | false, status, body?, headers? }) and that successful (ok:
true) branches may omit body and headers; replace the inaccurate sentence "Each
service function returns a result object (`{ status, body, headers? }`) without
throwing" with this corrected description to match the implementations of the
service functions and their result type.
In `@src/services/basic-auth.ts`:
- Around line 38-47: The auth check currently requires the exact "Basic "
casing; change it to accept the scheme case-insensitively by splitting
authHeader into scheme and value (e.g., const [scheme, base64Credentials] =
authHeader.split(' ', 2)), verify scheme.toLowerCase() === 'basic', and then
continue using the extracted base64Credentials with
Buffer.from(base64Credentials, 'base64').toString('utf-8') so headers like
"basic ..." are accepted while preserving correct credential decoding.
In `@src/services/redirect.ts`:
- Around line 27-29: The current ternary treats an empty statusParam
("?status=") as missing and defaults to 302; change the logic so you only
default to HttpStatusCodes.FOUND when statusParam is strictly undefined,
otherwise attempt to parse and validate the provided value. Concretely, set
redirectStatus = statusParam === undefined ? HttpStatusCodes.FOUND :
toSafeInteger(statusParam) and then keep the existing validation using
RedirectStatuses.has(redirectStatus); ensure toSafeInteger returns undefined or
NaN for empty/invalid input so the subsequent check (redirectStatus ===
undefined || !RedirectStatuses.has(redirectStatus)) will catch an explicit empty
string instead of silently using 302.
- Around line 19-42: The current handler returns user-supplied urlParam directly
(see urlParam and the final return) creating an open-redirect; update the
validation so only safe relative-path redirects are allowed: reject any urlParam
that contains a scheme (e.g. "http:"), a double-slash host ("//"), or that does
not begin with a single "/" path segment, and keep the existing redirectStatus
validation (toSafeInteger, RedirectStatuses) untouched; on invalid targets
return the BAD_REQUEST error like other checks and only return ok:true with url
when the urlParam passes the relative-path-only check.
---
Nitpick comments:
In `@src/services/shutdown.ts`:
- Around line 3-5: The success variant of the ShutdownResult type is missing a
status field which forces callers to special-case responses; update the type so
the ok: true branch includes status: number (e.g., change { ok: true; body: {
message: string } } to { ok: true; status: number; body: { message: string } })
and then ensure any functions that return a successful shutdown (the code paths
that construct ShutdownResult with ok: true) include an appropriate HTTP status
value so all callers can uniformly read .status on both branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c826e9f9-5c14-4c8f-8e66-1c9dba14ea3c
📒 Files selected for processing (12)
AGENTS.mddocs/DEVELOPMENT_GUIDE.mdsrc/routes/base64.tssrc/routes/basic-auth.tssrc/routes/redirect.tssrc/routes/shutdown.tssrc/routes/status.tssrc/services/base64.tssrc/services/basic-auth.tssrc/services/redirect.tssrc/services/shutdown.tssrc/services/status.ts
|
|
||
| **Service Layer**: | ||
| - Business logic extracted into `src/services/` to keep routes thin | ||
| - Each service function returns a result object (`{ status, body, headers? }`) without throwing |
There was a problem hiding this comment.
Service contract documentation is currently inaccurate.
The new services return discriminated union results with an ok field, and success branches do not always include body/headers. The documented contract should match that to avoid drift.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` at line 37, Update the service contract documentation line so it
describes the actual discriminated-union return shape used by the new services:
mention that each service function returns a result object with an "ok" boolean
discriminator (e.g., { ok: true | false, status, body?, headers? }) and that
successful (ok: true) branches may omit body and headers; replace the inaccurate
sentence "Each service function returns a result object (`{ status, body,
headers? }`) without throwing" with this corrected description to match the
implementations of the service functions and their result type.
| if (!authHeader || !authHeader.startsWith('Basic ')) { | ||
| return { | ||
| status: HttpStatusCodes.UNAUTHORIZED, | ||
| body: { authenticated: false, message: 'Authentication required' }, | ||
| headers: { 'WWW-Authenticate': 'Basic realm="Access to /basic-auth"' }, | ||
| }; | ||
| } | ||
|
|
||
| const base64Credentials = authHeader.substring('Basic '.length); | ||
| const credentials = Buffer.from(base64Credentials, 'base64').toString('utf-8'); |
There was a problem hiding this comment.
Accept the Basic auth scheme case-insensitively.
Line 38 only matches the exact Basic prefix, and Line 46 slices based on that exact casing. Valid headers like Authorization: basic ... will be rejected even though auth schemes are case-insensitive.
🐛 Proposed fix
- if (!authHeader || !authHeader.startsWith('Basic ')) {
+ const basicMatch = authHeader?.match(/^basic\s+(\S+)$/i);
+ if (!basicMatch) {
return {
status: HttpStatusCodes.UNAUTHORIZED,
body: { authenticated: false, message: 'Authentication required' },
headers: { 'WWW-Authenticate': 'Basic realm="Access to /basic-auth"' },
};
}
- const base64Credentials = authHeader.substring('Basic '.length);
+ const [, base64Credentials] = basicMatch;📝 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.
| if (!authHeader || !authHeader.startsWith('Basic ')) { | |
| return { | |
| status: HttpStatusCodes.UNAUTHORIZED, | |
| body: { authenticated: false, message: 'Authentication required' }, | |
| headers: { 'WWW-Authenticate': 'Basic realm="Access to /basic-auth"' }, | |
| }; | |
| } | |
| const base64Credentials = authHeader.substring('Basic '.length); | |
| const credentials = Buffer.from(base64Credentials, 'base64').toString('utf-8'); | |
| const basicMatch = authHeader?.match(/^basic\s+(\S+)$/i); | |
| if (!basicMatch) { | |
| return { | |
| status: HttpStatusCodes.UNAUTHORIZED, | |
| body: { authenticated: false, message: 'Authentication required' }, | |
| headers: { 'WWW-Authenticate': 'Basic realm="Access to /basic-auth"' }, | |
| }; | |
| } | |
| const [, base64Credentials] = basicMatch; | |
| const credentials = Buffer.from(base64Credentials, 'base64').toString('utf-8'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/basic-auth.ts` around lines 38 - 47, The auth check currently
requires the exact "Basic " casing; change it to accept the scheme
case-insensitively by splitting authHeader into scheme and value (e.g., const
[scheme, base64Credentials] = authHeader.split(' ', 2)), verify
scheme.toLowerCase() === 'basic', and then continue using the extracted
base64Credentials with Buffer.from(base64Credentials,
'base64').toString('utf-8') so headers like "basic ..." are accepted while
preserving correct credential decoding.
| if (!urlParam) { | ||
| return { | ||
| ok: false, | ||
| status: HttpStatusCodes.BAD_REQUEST, | ||
| body: { error: { message: 'Missing `url` query parameter' } }, | ||
| }; | ||
| } | ||
|
|
||
| const redirectStatus = statusParam ? toSafeInteger(statusParam) : HttpStatusCodes.FOUND; | ||
|
|
||
| if (redirectStatus === undefined || !RedirectStatuses.has(redirectStatus)) { | ||
| return { | ||
| ok: false, | ||
| status: HttpStatusCodes.BAD_REQUEST, | ||
| body: { | ||
| error: { | ||
| message: | ||
| 'Invalid redirect status code. Supported statuses are 301, 302, 303, 307 and 308', | ||
| }, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| return { ok: true, status: redirectStatus, url: urlParam }; |
There was a problem hiding this comment.
Untrusted redirect target creates an open-redirect vector.
The service forwards user-controlled urlParam to redirect response construction without target validation. This can be abused for phishing via trusted domain links.
Suggested mitigation (relative-path only)
export function redirect(
urlParam: string | undefined,
statusParam: string | undefined,
): RedirectResult {
if (!urlParam) {
return {
ok: false,
status: HttpStatusCodes.BAD_REQUEST,
body: { error: { message: 'Missing `url` query parameter' } },
};
}
+
+ if (!urlParam.startsWith('/')) {
+ return {
+ ok: false,
+ status: HttpStatusCodes.BAD_REQUEST,
+ body: {
+ error: { message: 'Invalid redirect target. Only relative paths are allowed' },
+ },
+ };
+ }
const redirectStatus =
statusParam === undefined ? HttpStatusCodes.FOUND : toSafeInteger(statusParam);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/redirect.ts` around lines 19 - 42, The current handler returns
user-supplied urlParam directly (see urlParam and the final return) creating an
open-redirect; update the validation so only safe relative-path redirects are
allowed: reject any urlParam that contains a scheme (e.g. "http:"), a
double-slash host ("//"), or that does not begin with a single "/" path segment,
and keep the existing redirectStatus validation (toSafeInteger,
RedirectStatuses) untouched; on invalid targets return the BAD_REQUEST error
like other checks and only return ok:true with url when the urlParam passes the
relative-path-only check.
| const redirectStatus = statusParam ? toSafeInteger(statusParam) : HttpStatusCodes.FOUND; | ||
|
|
||
| if (redirectStatus === undefined || !RedirectStatuses.has(redirectStatus)) { |
There was a problem hiding this comment.
Handle empty status explicitly instead of defaulting to 302.
statusParam ? ... : ... treats ?status= as missing and falls back to 302. That silently accepts invalid input.
Suggested fix
- const redirectStatus = statusParam ? toSafeInteger(statusParam) : HttpStatusCodes.FOUND;
+ const redirectStatus =
+ statusParam === undefined ? HttpStatusCodes.FOUND : toSafeInteger(statusParam);📝 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 redirectStatus = statusParam ? toSafeInteger(statusParam) : HttpStatusCodes.FOUND; | |
| if (redirectStatus === undefined || !RedirectStatuses.has(redirectStatus)) { | |
| const redirectStatus = | |
| statusParam === undefined ? HttpStatusCodes.FOUND : toSafeInteger(statusParam); | |
| if (redirectStatus === undefined || !RedirectStatuses.has(redirectStatus)) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/services/redirect.ts` around lines 27 - 29, The current ternary treats an
empty statusParam ("?status=") as missing and defaults to 302; change the logic
so you only default to HttpStatusCodes.FOUND when statusParam is strictly
undefined, otherwise attempt to parse and validate the provided value.
Concretely, set redirectStatus = statusParam === undefined ?
HttpStatusCodes.FOUND : toSafeInteger(statusParam) and then keep the existing
validation using RedirectStatuses.has(redirectStatus); ensure toSafeInteger
returns undefined or NaN for empty/invalid input so the subsequent check
(redirectStatus === undefined || !RedirectStatuses.has(redirectStatus)) will
catch an explicit empty string instead of silently using 302.
2 participants
Summary by CodeRabbit
Refactor
Documentation