-
-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix auth and cors #104
fix auth and cors #104
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -1,4 +1,4 @@ | ||||||||||
import { GET } from "../app/api/[[...route]]/route"; | ||||||||||
import { GET } from "../app/v1/[[...route]]/route"; | ||||||||||
import { describe, it, expect, vi } from "vitest"; | ||||||||||
|
||||||||||
vi.mock("@repo/db", () => ({ | ||||||||||
|
@@ -13,8 +13,8 @@ vi.mock("@repo/db", () => ({ | |||||||||
})); | ||||||||||
|
||||||||||
describe("Health API", () => { | ||||||||||
it("GET /api/health should return health status", async () => { | ||||||||||
const req = new Request("http://localhost:3000/api/health", { | ||||||||||
it("GET /v1/health should return health status", async () => { | ||||||||||
const req = new Request("http://localhost:3000/v1/health", { | ||||||||||
method: "GET", | ||||||||||
}); | ||||||||||
const res = await GET(req); | ||||||||||
|
@@ -29,8 +29,8 @@ describe("Health API", () => { | |||||||||
}); | ||||||||||
|
||||||||||
describe("Mail API", () => { | ||||||||||
it("GET /api/mail should return health status", async () => { | ||||||||||
const req = new Request("http://localhost:3000/api/mail/send", { | ||||||||||
it("GET /v1/mail should return health status", async () => { | ||||||||||
const req = new Request("http://localhost:3000/v1/mail/send", { | ||||||||||
method: "GET", | ||||||||||
Comment on lines
+32
to
+33
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix inconsistent test description The test description doesn't match the actual endpoint being tested. - it("GET /v1/mail should return health status", async () => {
+ it("GET /v1/mail/send should return health status", async () => { 📝 Committable suggestion
Suggested change
|
||||||||||
}); | ||||||||||
const res = await GET(req); | ||||||||||
|
@@ -45,8 +45,8 @@ describe("Mail API", () => { | |||||||||
}); | ||||||||||
|
||||||||||
describe("Mail Batch API", () => { | ||||||||||
it("GET /api/mail should return health status", async () => { | ||||||||||
const req = new Request("http://localhost:3000/api/mail/send-batch", { | ||||||||||
it("GET /v1/mail should return health status", async () => { | ||||||||||
const req = new Request("http://localhost:3000/v1/mail/send-batch", { | ||||||||||
method: "GET", | ||||||||||
}); | ||||||||||
const res = await GET(req); | ||||||||||
|
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import { auth } from "@repo/auth"; | ||
import { Hono } from "hono"; | ||
import { auth as Auth } from "@repo/auth"; | ||
|
||
const app = new Hono<{ | ||
Variables: { | ||
user: typeof Auth.$Infer.Session.user | null; | ||
session: typeof Auth.$Infer.Session.session | null; | ||
}; | ||
}>(); | ||
|
||
app.get("/*", (c) => auth.handler(c.req.raw)); | ||
app.post("/*", (c) => auth.handler(c.req.raw)); | ||
Comment on lines
+12
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security: Restrict wildcard routes and add rate limiting The current implementation using
- app.get("/*", (c) => auth.handler(c.req.raw));
- app.post("/*", (c) => auth.handler(c.req.raw));
+ import { RateLimiter } from '@hono/rate-limiter'
+
+ const limiter = new RateLimiter({
+ max: 100, // max requests
+ window: '1m' // per minute
+ });
+
+ // Define specific auth routes
+ const authRoutes = ['/login', '/logout', '/register', '/session'] as const;
+
+ authRoutes.forEach(route => {
+ app.use(route, limiter);
+ app.get(route, (c) => auth.handler(c.req.raw));
+ app.post(route, (c) => auth.handler(c.req.raw));
+ });
|
||
export default app; |
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,6 +1,5 @@ | ||||||||||||||||||
import { handle } from "hono/vercel"; | ||||||||||||||||||
import { Hono } from "hono"; | ||||||||||||||||||
import { auth as Auth } from "@repo/auth"; | ||||||||||||||||||
import mail from "./mail"; | ||||||||||||||||||
import test from "./test"; | ||||||||||||||||||
import session from "./session"; | ||||||||||||||||||
|
@@ -10,27 +9,29 @@ import health from "./health"; | |||||||||||||||||
import user from "./user"; | ||||||||||||||||||
import contributors from "./contributors"; | ||||||||||||||||||
import { cors } from "hono/cors"; | ||||||||||||||||||
import { HonoBase } from "hono/hono-base"; | ||||||||||||||||||
|
||||||||||||||||||
export const runtime = "edge"; | ||||||||||||||||||
|
||||||||||||||||||
const app = new Hono<{ | ||||||||||||||||||
Variables: { | ||||||||||||||||||
user: typeof Auth.$Infer.Session.user | null; | ||||||||||||||||||
session: typeof Auth.$Infer.Session.session | null; | ||||||||||||||||||
}; | ||||||||||||||||||
}>().basePath("/api"); | ||||||||||||||||||
const app = new Hono().basePath("/v1"); | ||||||||||||||||||
|
||||||||||||||||||
const allowedOrigins = [ | ||||||||||||||||||
"http://localhost:3002", | ||||||||||||||||||
"http://localhost:3003", | ||||||||||||||||||
"http://localhost:3004", | ||||||||||||||||||
"https://www.plura.pro", | ||||||||||||||||||
"https://app.plura.pro", | ||||||||||||||||||
Comment on lines
+19
to
23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Review development origins in production Multiple localhost origins are included in the allowedOrigins array. These should be environment-specific. Consider using environment variables: -const allowedOrigins = [
- "http://localhost:3002",
- "http://localhost:3003",
- "http://localhost:3004",
- "https://www.plura.pro",
- "https://app.plura.pro",
-];
+const allowedOrigins = process.env.NODE_ENV === 'development'
+ ? ["http://localhost:3002", "http://localhost:3003", "http://localhost:3004"]
+ : ["https://www.plura.pro", "https://app.plura.pro"]; 📝 Committable suggestion
Suggested change
|
||||||||||||||||||
]; | ||||||||||||||||||
|
||||||||||||||||||
app.use( | ||||||||||||||||||
"*", | ||||||||||||||||||
cors({ | ||||||||||||||||||
origin: allowedOrigins, // Allow requests from your frontend origin | ||||||||||||||||||
allowMethods: ["GET", "POST", "OPTIONS"], | ||||||||||||||||||
origin: allowedOrigins, | ||||||||||||||||||
allowMethods: ["GET", "POST", "PATCH", "DELETE", "OPTIONS"], | ||||||||||||||||||
allowHeaders: ["Content-Type", "Authorization"], | ||||||||||||||||||
exposeHeaders: ["Content-Length"], | ||||||||||||||||||
maxAge: 600, | ||||||||||||||||||
credentials: true, | ||||||||||||||||||
Comment on lines
+29
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security: Review CORS configuration Several security concerns in the CORS configuration:
Recommendations:
cors({
origin: allowedOrigins,
allowMethods: ["GET", "POST", "PATCH", "DELETE", "OPTIONS"],
allowHeaders: ["Content-Type", "Authorization"],
exposeHeaders: ["Content-Length"],
- maxAge: 600,
+ maxAge: 300,
credentials: true,
+ // Add security headers
+ preflightContinue: false,
+ optionsSuccessStatus: 204,
}),
|
||||||||||||||||||
}), | ||||||||||||||||||
); | ||||||||||||||||||
|
||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,5 +15,5 @@ export function middleware(request: NextRequest) { | |
} | ||
|
||
export const config = { | ||
matcher: ["/api/:path*", "/dashboard"], | ||
matcher: ["/v1/:path*", "/dashboard"], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification API endpoints need to be updated from The verification reveals inconsistency in API endpoint usage where test files still use the old Locations requiring updates:
🔗 Analysis chainVerify API version migration completeness The path matcher has been updated from 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check for any remaining /api references that need updating
rg -g '!*.md' -g '!package*.json' '/api/'
# Check for consistency in new /v1 endpoint usage
rg -g '!*.md' -g '!package*.json' '/v1/'
Length of output: 1609 |
||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,124 @@ | ||
"use client"; | ||
import { useEffect, useState } from "react"; | ||
import StatusCard from "@/components/custom/status.card"; | ||
import { StatusData } from "@/components/custom/status.tracker"; | ||
import { Activity } from "lucide-react"; | ||
|
||
export default function Home() { | ||
interface StatusT { | ||
timestamp: string; | ||
latencies: { | ||
WEB: number; | ||
API: number; | ||
APP: number; | ||
}; | ||
statuses: { | ||
WEB: string; | ||
API: string; | ||
APP: string; | ||
}; | ||
totalLatency: number; | ||
operationCount: number; | ||
averageLatency: number; | ||
} | ||
|
||
const Page = () => { | ||
const [webStatus, setWebStatus] = useState<StatusData[]>([]); | ||
const [apiStatus, setApiStatus] = useState<StatusData[]>([]); | ||
const [appStatus, setAppStatus] = useState<StatusData[]>([]); | ||
const [isFetching, setIsFetching] = useState(true); | ||
|
||
const fetchStatusData = async () => { | ||
try { | ||
setIsFetching(true); | ||
const siteResponse = await fetch("http://localhost:3001/v1/status/site"); | ||
const data = await siteResponse.json(); | ||
|
||
if (data.siteStatus && data.siteStatus.length > 0) { | ||
const today = new Date().toISOString().split("T")[0]; | ||
|
||
const todayStatus = data.siteStatus.filter((status: StatusT) => | ||
status.timestamp.startsWith(today) | ||
); | ||
console.log("todayStatus", todayStatus); | ||
|
||
if (todayStatus.length > 0) { | ||
const getServiceStatus = ( | ||
latency: number, | ||
originalStatus: string | ||
): "operational" | "warning" | "down" => { | ||
if (originalStatus === "DOWN" || latency >= 1000) return "down"; | ||
if (latency >= 700) return "warning"; | ||
return "operational"; | ||
}; | ||
|
||
const webData = todayStatus.map((status: StatusT) => ({ | ||
status: getServiceStatus(status.latencies.WEB, status.statuses.WEB), | ||
timestamp: status.timestamp, | ||
})); | ||
|
||
const apiData = todayStatus.map((status: StatusT) => ({ | ||
status: getServiceStatus(status.latencies.API, status.statuses.API), | ||
timestamp: status.timestamp, | ||
})); | ||
|
||
const appData = todayStatus.map((status: StatusT) => ({ | ||
status: getServiceStatus(status.latencies.APP, status.statuses.APP), | ||
timestamp: status.timestamp, | ||
})); | ||
|
||
setWebStatus(webData); | ||
setApiStatus(apiData); | ||
setAppStatus(appData); | ||
} else { | ||
setWebStatus([]); | ||
setApiStatus([]); | ||
setAppStatus([]); | ||
} | ||
} | ||
} catch (error) { | ||
console.error("Error fetching status data:", error); | ||
} finally { | ||
setIsFetching(false); | ||
} | ||
}; | ||
|
||
useEffect(() => { | ||
fetchStatusData(); | ||
const interval = setInterval(fetchStatusData, 4 * 60 * 1000); | ||
return () => clearInterval(interval); | ||
}, []); | ||
|
||
return ( | ||
<div className="flex flex-col gap-10 h-full w-full items-center justify-center my-10 overflow-auto"> | ||
<div className="fixed bottom-0 left-[-20%] right-0 top-[-10%] h-[500px] w-[500px] rounded-full bg-[radial-gradient(circle_farthest-side,rgba(211,211,211,0.15),rgba(255,255,255,0))]" /> | ||
<div className="flex flex-col items-center justify-center gap-2"> | ||
<div className="p-2 bg-secondary rounded-full"> | ||
<Activity className="size-8 text-green-500"/> | ||
<Activity className="size-8 text-green-500" /> | ||
</div> | ||
<h1 className="text-4xl font-bold">All services are online</h1> | ||
<span className="text-sm text-muted-foreground">Last updated on Nov 16 at 10:36am IST</span> | ||
<h1 className="text-4xl font-bold"> | ||
{isFetching | ||
? "All services are ..." | ||
: webStatus.length > 0 && | ||
apiStatus.length > 0 && | ||
appStatus.length > 0 && | ||
webStatus[webStatus.length - 1].status === "operational" && | ||
apiStatus[apiStatus.length - 1].status === "operational" && | ||
appStatus[appStatus.length - 1].status === "operational" | ||
? "All services are online" | ||
: "Some services are experiencing issues"} | ||
</h1> | ||
<span className="text-sm text-muted-foreground"> | ||
Last updated:{" "} | ||
{webStatus.length > 0 | ||
? `${new Date(webStatus[webStatus.length - 1].timestamp).toLocaleTimeString('en-IN', { hour: '2-digit', minute: '2-digit', second: '2-digit' })}` | ||
: "..."} | ||
</span> | ||
</div> | ||
<div className="flex w-full h-full px-10 md:px-40"> | ||
<StatusCard/> | ||
<div className="flex w-full h-full px-10 md:px-40 gap-4"> | ||
<StatusCard statusData={webStatus} /> | ||
</div> | ||
</div> | ||
); | ||
} | ||
}; | ||
|
||
export default Page; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update test endpoints to match new API version
The test endpoints still use the
/api/
prefix while the import has been updated to use the new/v1/
versioned routes.Update the test endpoints to match the new API version:
This change should be applied to all test cases in this file to maintain consistency with the new API versioning.
Also applies to: 15-19