Skip to content
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

Merged
merged 4 commits into from
Dec 7, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions .github/workflows/format-and-push.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@ jobs:
runs-on: ubuntu-latest

steps:
- name: Checkout
- name: Checkout repository
uses: actions/checkout@v4
with:
ref: ${{ github.head_ref }} # Checkout the PR branch
fetch-depth: 0

- uses: pnpm/action-setup@v4
name: Install pnpm
- name: Install pnpm
uses: pnpm/action-setup@v4
with:
version: 9.12.1
run_install: false
Expand All @@ -44,7 +44,9 @@ jobs:
${{ runner.os }}-pnpm-

- name: Install dependencies & turbo
run: pnpm install && pnpm i -g turbo
run: |
pnpm install
pnpm i -g turbo

- name: Format code with Prettier
run: turbo format:write
Expand All @@ -56,7 +58,6 @@ jobs:
git config --global user.email "github-actions[bot]@users.noreply.github.com"
git add .
git commit -m "format: make the code prettier"
git push origin ${{ github.head_ref }}
git push origin HEAD:${{ github.head_ref }}
else
echo "No formatting changes to push."
fi
14 changes: 7 additions & 7 deletions apps/api/__tests__/life.test.ts
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";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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:

- const req = new Request("http://localhost:3000/api/health", {
+ const req = new Request("http://localhost:3000/v1/health", {

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


vi.mock("@repo/db", () => ({
Expand All @@ -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);
Expand All @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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

‼️ 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.

Suggested change
it("GET /v1/mail should return health status", async () => {
const req = new Request("http://localhost:3000/v1/mail/send", {
it("GET /v1/mail/send should return health status", async () => {
const req = new Request("http://localhost:3000/v1/mail/send", {

});
const res = await GET(req);
Expand All @@ -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);
Expand Down
24 changes: 0 additions & 24 deletions apps/api/app/api/[[...route]]/auth.ts

This file was deleted.

18 changes: 0 additions & 18 deletions apps/api/app/layout.tsx

This file was deleted.

19 changes: 0 additions & 19 deletions apps/api/app/page.tsx

This file was deleted.

14 changes: 14 additions & 0 deletions apps/api/app/v1/[[...route]]/auth.ts
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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security: Restrict wildcard routes and add rate limiting

The current implementation using /* wildcards could expose unintended endpoints. Consider:

  1. Explicitly defining allowed routes
  2. Adding rate limiting
  3. Adding request validation
- 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));
+ });

Committable suggestion skipped: line range outside the PR's diff.

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";
Expand All @@ -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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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

‼️ 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.

Suggested change
"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"];

];

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Security: Review CORS configuration

Several security concerns in the CORS configuration:

  1. credentials: true requires strict origin validation
  2. maxAge: 600 is quite long for CORS preflight cache
  3. New HTTP methods (PATCH, DELETE) need proper authentication
  4. Authorization header is exposed without documentation

Recommendations:

  1. Implement strict origin validation when credentials are enabled
  2. Reduce maxAge to 300 seconds (5 minutes) or less
  3. Document authentication requirements for new HTTP methods
  4. Add security headers:
 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,
 }),

Committable suggestion skipped: line range outside the PR's diff.

}),
);

Expand Down
2 changes: 1 addition & 1 deletion apps/api/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ export function middleware(request: NextRequest) {
}

export const config = {
matcher: ["/api/:path*", "/dashboard"],
matcher: ["/v1/:path*", "/dashboard"],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

API endpoints need to be updated from /api to /v1 in test files

The verification reveals inconsistency in API endpoint usage where test files still use the old /api path prefix while the rest of the codebase has migrated to /v1.

Locations requiring updates:

  • apps/api/__tests__/life.test.ts:
    • Line containing /api/health
    • Line containing /api/mail/send
    • Line containing /api/mail/send-batch
🔗 Analysis chain

Verify API version migration completeness

The path matcher has been updated from /api to /v1.

🏁 Scripts executed

The 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

};
2 changes: 1 addition & 1 deletion apps/app/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ const redirectUrl =
: "http://localhost:3003/auth";
export default async function authMiddleware(request: NextRequest) {
const { data: session } = await betterFetch<Session>(
`${baseDomain}/api/auth/get-session`,
`${baseDomain}/v1/auth/get-session`,
{
baseURL: request.nextUrl.origin,
headers: {
Expand Down
118 changes: 111 additions & 7 deletions apps/status/app/page.tsx
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;
5 changes: 3 additions & 2 deletions apps/status/components/custom/site/footer.tsx
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import Link from 'next/link'
import React from 'react'

export default function Footer() {
return (
<footer className="py-6 md:px-8 md:py-0">
<div className="flex flex-col items-center gap-4 md:h-24 md:flex-row">
<p className="text-balance text-center text-sm leading-loose text-muted-foreground md:text-left">
&copy; 2024 Plura Ai. All rights reserved.
<p className="inline-flex gap-1 text-balance text-center text-sm leading-loose text-muted-foreground md:text-left">
Powered by <Link href={"https://l.devwtf.in/plura-x"} className='hover:underline font-semibold text-primary cursor-pointer'>Plura Ai</Link>
</p>
</div>
</footer>
Expand Down
Loading
Loading