Feat/worker - #8
Conversation
Moves job delay logic to a dedicated module for better organization and reusability. This change enhances the queue processing by centralizing the delay calculation and making it easier to adjust and maintain. It also ensures consistency in how job delays are handled across different parts of the application.
Implements middleware to parse the request body, allowing raw access to the body as a string and parsing it to JSON.
Removes the `/` endpoint, as it is no longer needed with the introduction of the worker architecture.
Removes the original server entrypoint (src/index.ts) and related scripts. This is done to shift the application architecture to a worker-based model. The original index.ts file contained the Express server setup which is now handled by a dedicated server entrypoint (src/server.ts).
Updates the webhook route from "/transaction" to "/webhook" to align with the worker service.
Ensures that duplicate messages are not sent to the queue for transactions. If a message already exists for a given transaction, the function now returns early and logs an info message instead of sending a duplicate. This change prevents redundant notifications and ensures that the queue processes only unique transaction-related messages.
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe PR refactors the application from a monolithic architecture into a separated server-worker model using Redis for job queueing. It introduces new Docker services, establishes token-based API security, adds structured logging via pino, and splits entry points into server and worker processes with corresponding npm scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Server<br/>(HTTP)
participant Redis as Redis<br/>(Queue)
participant Worker
participant FireflyAPI as Firefly API
Client->>Server: POST /webhook<br/>(with signature)
Server->>Server: Verify webhook signature
Server->>Redis: Enqueue job<br/>(e.g., unbudgeted_transactions)
Server-->>Client: 200 OK
Worker->>Redis: Poll for jobs
Worker->>FireflyAPI: Fetch transaction details
Worker->>FireflyAPI: Get budget limits
Worker->>FireflyAPI: Create/Update budget assignments
Worker->>Redis: Mark job complete
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 markdownlint-cli2 (0.18.1)README.mdmarkdownlint-cli2 v0.18.1 (markdownlint v0.38.0) 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| app.get("/transaction/:transactionId/budget/:budget_id", TokenMiddleware, settingBudgetForTransaction) | ||
| app.get("/transaction/:transactionId/category/:category_id", TokenMiddleware, settingCategoryForTransaction) | ||
| app.post("/webhook", verifyWebhookMiddleware, webhook) |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
In general, to fix missing rate limiting on a sensitive or potentially expensive route, you introduce a rate‑limiting middleware (e.g. using express-rate-limit) and apply it to that specific route or to the entire app, depending on your requirements. The limiter should enforce a reasonable maximum number of requests per IP (or other key) over a time window, returning 429 responses when exceeded. This mitigates simple denial‑of‑service attempts that rely on flooding the endpoint.
For this codebase, the minimal, targeted fix is to add express-rate-limit and apply a limiter only to the /webhook route, leaving the other routes unchanged. Concretely in src/server.ts, we will:
- Import
express-rate-limitnear the top of the file (without changing existing imports). - Define a
webhookLimiterconstant after creating theappor before defining the routes. This limiter will set a window and a maximum number of requests per IP. - Attach
webhookLimiterto the/webhookroute by adding it to the middleware chain:app.post("/webhook", webhookLimiter, verifyWebhookMiddleware, webhook).
No existing functionality is removed; we are only adding a new middleware layer in front of the current ones.
| @@ -1,5 +1,6 @@ | ||
| import express from "express" | ||
| import pino from "pino" | ||
| import rateLimit from "express-rate-limit" | ||
|
|
||
| import { env } from "./config" | ||
| import { settingBudgetForTransaction } from "./endpoints/settingBudgetForTransaction" | ||
| @@ -12,11 +13,16 @@ | ||
| const logger = pino() | ||
| const app = express() | ||
|
|
||
| const webhookLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 webhook requests per window | ||
| }) | ||
|
|
||
| app.use(ParseBodyMiddleware) | ||
|
|
||
| app.get("/transaction/:transactionId/budget/:budget_id", TokenMiddleware, settingBudgetForTransaction) | ||
| app.get("/transaction/:transactionId/category/:category_id", TokenMiddleware, settingCategoryForTransaction) | ||
| app.post("/webhook", verifyWebhookMiddleware, webhook) | ||
| app.post("/webhook", webhookLimiter, verifyWebhookMiddleware, webhook) | ||
|
|
||
| async function startServer() { | ||
| try { |
| @@ -30,7 +30,8 @@ | ||
| "express": "^5.1.0", | ||
| "luxon": "^3.5.0", | ||
| "pino": "^10.2.0", | ||
| "pino-pretty": "^13.1.3" | ||
| "pino-pretty": "^13.1.3", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@codedependant/semantic-release-docker": "^5.0.3", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
|
|
||
| export async function TokenMiddleware(req: Request, res: Response, next: NextFunction): Promise<void> { | ||
| // Token is passed in the query parameters as ?api_token=...TOKEN | ||
| const token = req.query.api_token as string | undefined |
Check warning
Code scanning / CodeQL
Sensitive data read from GET request Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 7 months ago
In general, sensitive credentials like API tokens should not be transmitted in URLs (query strings). Instead, they should be sent in HTTP headers (for example, Authorization or a custom header) or, less ideally, in the body of POST/PUT requests. The middleware should validate tokens from these safer locations and stop encouraging the use of query parameters.
The best way to fix this code without changing existing functionality too drastically is to (1) stop reading the token from req.query.api_token, and (2) read it from a commonly used secure location, such as a bearer token in the Authorization header, while optionally also checking req.body.api_token for non‑GET requests. To avoid breaking callers too hard, we can support multiple inputs but prioritize headers and bodies, and we should remove the explicit guidance in the comment that tells users to put tokens in the query string. Within src/utils/tokenMiddleware.ts, change line 10 so that it derives token from req.headers.authorization (parsing a Bearer <token> scheme) and, as a fallback, from req.body.api_token if present. This does not require any new imports; Express already exposes req.headers and req.body (assuming body‑parsing middleware is configured elsewhere).
Concretely:
- Update the comment on line 9 to no longer recommend query parameters; instead, document the new, safer mechanism.
- Replace
const token = req.query.api_token as string | undefinedwith logic that:- Reads
req.headers.authorizationand, if it starts withBearer(case-insensitive), extracts the token value. - If no header token is present, attempts to read
req.body.api_token(typed asany/unknownthen cast to string | undefined).
- Reads
- Keep the rest of the middleware unchanged so that the validation and responses behave the same, only the token source changes.
| @@ -6,8 +6,16 @@ | ||
| const logger = pino() | ||
|
|
||
| export async function TokenMiddleware(req: Request, res: Response, next: NextFunction): Promise<void> { | ||
| // Token is passed in the query parameters as ?api_token=...TOKEN | ||
| const token = req.query.api_token as string | undefined | ||
| // Token should be provided via the Authorization header as "Bearer <token>" | ||
| let token: string | undefined | ||
|
|
||
| const authHeader = req.headers["authorization"] | ||
| if (typeof authHeader === "string" && authHeader.toLowerCase().startsWith("bearer ")) { | ||
| token = authHeader.slice(7).trim() | ||
| } else if (req.body && typeof (req.body as any).api_token === "string") { | ||
| token = (req.body as any).api_token | ||
| } | ||
|
|
||
| logger.info("Verifying API token") | ||
|
|
||
| if (!token || token !== env.apiToken) { |
2 participants
Summary by CodeRabbit
Release Notes
New Features
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.