Restoring the ordering of webhook before parsing json (#2389)
# Pull Request Checklist ## Description - [ ] **Description of PR**: Clear and concise description of what this PR accomplishes - [ ] **Breaking Changes**: Document any breaking changes (if applicable) - [ ] **Related Issues**: Link to any related issues or tickets ## Testing - [ ] **Test cases Attached**: All relevant test cases have been added/updated - [ ] **Manual Testing**: Manual testing completed for the changes ## Monitoring & Debugging - [ ] **Logging in place**: Appropriate logging has been added for debugging user issues - [ ] **Sentry will be able to catch errors**: Error handling ensures Sentry can capture and report errors - [ ] **Avoid Dev based/Prisma logging**: No development-only or Prisma-specific logging in production code ## Configuration - [ ] **Env variables newly added**: Any new environment variables are documented in .env.example file or mentioned in description --- ## Additional Notes <!-- Add any additional context, screenshots, or notes for reviewers here -->
This commit is contained in:
parent
899db4ed56
commit
55f0a8b60a
6 changed files with 48 additions and 39 deletions
1
.gitignore
vendored
1
.gitignore
vendored
|
|
@ -256,3 +256,4 @@ fabric.properties
|
|||
*/node_modules/*
|
||||
|
||||
/cli/experiments/js-serialization-experiment/node_modules/*
|
||||
/cli/packages/codeflash/.npmrc
|
||||
|
|
|
|||
|
|
@ -242,10 +242,14 @@ export const githubApp = await (async () => {
|
|||
// Close any open optimization PRs targeting the branch of the closed PR
|
||||
// Ensure we only close PRs that are targeting the branch of the PR that was just closed
|
||||
const closedPrBranch = payload.pull_request.head.ref
|
||||
// Logic to close any open optimization PRs targeting this branch
|
||||
logger.info(`Closing optimization PRs targeting branch ${closedPrBranch}`, webhookContext(payload, "close_dependent_prs"))
|
||||
const closeCtx = webhookContext(payload, "close_dependent_prs")
|
||||
logger.info(`Closing optimization PRs targeting branch ${closedPrBranch}`, closeCtx, {
|
||||
is_user_code_flash,
|
||||
closedPrBranch,
|
||||
APP_USER_ID,
|
||||
})
|
||||
if (payload.installation === undefined) {
|
||||
logger.error("Installation ID is missing from payload. Cannot close PRs for this installation!", webhookContext(payload, "close_dependent_prs"))
|
||||
logger.error("Installation ID is missing from payload. Cannot close PRs for this installation!", closeCtx)
|
||||
return
|
||||
}
|
||||
try {
|
||||
|
|
@ -257,6 +261,12 @@ export const githubApp = await (async () => {
|
|||
base: closedPrBranch,
|
||||
})
|
||||
|
||||
logger.info(`Found ${openPrs.data.length} open PRs targeting branch ${closedPrBranch}`, closeCtx, {
|
||||
openPrCount: openPrs.data.length,
|
||||
openPrNumbers: openPrs.data.map(pr => pr.number).join(","),
|
||||
openPrUsers: openPrs.data.map(pr => `#${pr.number}:${pr.user?.login}(id=${pr.user?.id},type=${pr.user?.type})`).join(","),
|
||||
})
|
||||
|
||||
for (const pr of openPrs.data) {
|
||||
// Check if the PR is opened by the Codeflash GitHub App and targets the same base branch as the closed PR
|
||||
if (
|
||||
|
|
|
|||
|
|
@ -11,10 +11,10 @@ import { logger } from "./utils/logger.js"
|
|||
import { posthog } from "./analytics.js"
|
||||
import { GlobalExceptionHandler } from "./exceptions/index.js"
|
||||
import { githubApp } from "./github/github-app.js"
|
||||
import { enhancedRequestLogger, logRequestBody } from "./middlewares/enhanced-logging.js"
|
||||
import { enhancedRequestLogger } from "./middlewares/enhanced-logging.js"
|
||||
import { registerRoutes } from "./routes/index.js"
|
||||
import { initializeCronJobs } from "./cron/index.js"
|
||||
import { DEFAULT_PORT, JSON_BODY_LIMIT, GITHUB_WEBHOOK_PATH } from "./constants/index.js"
|
||||
import { DEFAULT_PORT, GITHUB_WEBHOOK_PATH } from "./constants/index.js"
|
||||
|
||||
// ========================================
|
||||
// APPLICATION SETUP
|
||||
|
|
@ -54,16 +54,7 @@ logger.info("Mounting GitHub webhook middleware", {
|
|||
// ROUTE REGISTRATION
|
||||
// ========================================
|
||||
|
||||
// Register webhook routes first (before body parsers)
|
||||
// Then body parser middleware
|
||||
appExpress.use(express.json({ limit: JSON_BODY_LIMIT }))
|
||||
|
||||
// Log request body in development
|
||||
if (process.env.NODE_ENV !== "PRODUCTION") {
|
||||
appExpress.use(logRequestBody)
|
||||
}
|
||||
|
||||
// Register all routes
|
||||
// Register all routes (body parser is inside registerRoutes, after webhook routes)
|
||||
registerRoutes(appExpress)
|
||||
|
||||
// ========================================
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
import express from "express"
|
||||
import { AsyncExpressApp } from "../types.js"
|
||||
import { API_BASE_ROUTE } from "../constants/index.js"
|
||||
import { API_BASE_ROUTE, JSON_BODY_LIMIT } from "../constants/index.js"
|
||||
|
||||
// Route modules
|
||||
import { rootRoutes, publicApiRoutes } from "./public.routes.js"
|
||||
|
|
@ -13,21 +14,34 @@ import userRoutes from "./user.routes.js"
|
|||
import { checkForValidAPIKey } from "../middlewares/check-valid-api-key.js"
|
||||
import { trackEndpointCalls } from "../middlewares/track-endpoint-calls.js"
|
||||
import { idLimiter } from "../middlewares/rate-limit.js"
|
||||
import { logAuthEvent } from "../middlewares/enhanced-logging.js"
|
||||
import { logAuthEvent, logRequestBody } from "../middlewares/enhanced-logging.js"
|
||||
/**
|
||||
* Register all routes on the Express application
|
||||
*
|
||||
* Route registration order:
|
||||
* 1. Webhook routes (must be before body parsers for raw body access)
|
||||
* 2. Public routes (no authentication required)
|
||||
* 3. Protected routes (require API key authentication)
|
||||
* 2. Body parser middleware (express.json)
|
||||
* 3. Public routes (no authentication required)
|
||||
* 4. Protected routes (require API key authentication)
|
||||
*/
|
||||
export function registerRoutes(app: AsyncExpressApp): void {
|
||||
// ========================================
|
||||
// WEBHOOK ROUTES (before body parsers)
|
||||
// Webhook handlers (GitHub, Stripe) need raw body access for
|
||||
// signature verification. express.json() MUST NOT run before these.
|
||||
// ========================================
|
||||
app.use(webhookRoutes)
|
||||
|
||||
// ========================================
|
||||
// BODY PARSER (after webhook routes, before everything else)
|
||||
// ========================================
|
||||
app.use(express.json({ limit: JSON_BODY_LIMIT }))
|
||||
|
||||
// Log request body in development
|
||||
if (process.env.NODE_ENV !== "PRODUCTION") {
|
||||
app.use(logRequestBody)
|
||||
}
|
||||
|
||||
// ========================================
|
||||
// PUBLIC ROUTES (no authentication)
|
||||
// ========================================
|
||||
|
|
|
|||
|
|
@ -17,34 +17,18 @@ router.postAsync(ROUTES.GITHUB_WEBHOOKS, async (req: Request, res: Response, nex
|
|||
const deliveryId = (req.headers["x-github-delivery"] as string) || "unknown_delivery"
|
||||
const contentLength = req.headers["content-length"]
|
||||
|
||||
// Best-effort extract repo/PR context from raw body for structured logging
|
||||
let repoOwner: string | undefined
|
||||
let repoName: string | undefined
|
||||
let prNumber: number | undefined
|
||||
try {
|
||||
const rawBody = typeof req.body === "string" ? req.body : typeof req.body === "object" && req.body ? JSON.stringify(req.body) : undefined
|
||||
if (rawBody) {
|
||||
const parsed = typeof req.body === "object" ? req.body : JSON.parse(rawBody)
|
||||
repoOwner = parsed?.repository?.owner?.login
|
||||
repoName = parsed?.repository?.name
|
||||
prNumber = parsed?.pull_request?.number
|
||||
}
|
||||
} catch {
|
||||
// Graceful degradation: body may not be JSON or not yet parsed
|
||||
}
|
||||
|
||||
// Note: req.body is NOT available here — express.json() runs AFTER webhook routes
|
||||
// to preserve raw body stream for Octokit's signature verification.
|
||||
// Repo/PR context is logged by the webhook handlers in github-app.ts instead.
|
||||
const webhookLogContext = {
|
||||
requestId: req.requestId,
|
||||
traceId: req.traceId,
|
||||
operation: "github_webhook",
|
||||
eventType,
|
||||
deliveryId,
|
||||
...(repoOwner && { repoOwner }),
|
||||
...(repoName && { repoName }),
|
||||
...(prNumber && { prNumber }),
|
||||
}
|
||||
|
||||
logger.info("Processing GitHub webhook", webhookLogContext, { contentLength })
|
||||
logger.info("Processing GitHub webhook", webhookLogContext, { contentLength, eventType })
|
||||
|
||||
try {
|
||||
await ghAppMiddleware(req, res, next)
|
||||
|
|
|
|||
|
|
@ -421,6 +421,15 @@ export class Logger {
|
|||
if (error.stack) essentialInfo.push(`stack:${error.stack.split("\n")[0]}`)
|
||||
}
|
||||
|
||||
// Add custom context fields (repoOwner, repoName, prNumber, etc.)
|
||||
if (context) {
|
||||
Object.entries(context).forEach(([key, value]) => {
|
||||
if (!["requestId", "endpoint", "userId", "traceId", "correlationId", "operation"].includes(key) && value !== undefined) {
|
||||
essentialInfo.push(`${key}:${value}`)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
// Add custom metadata
|
||||
if (metadata) {
|
||||
Object.entries(metadata).forEach(([key, value]) => {
|
||||
|
|
|
|||
Loading…
Reference in a new issue