Add new certificate revocation system and web-server separation.#3
Merged
Add new certificate revocation system and web-server separation.#3
Conversation
Owner
Author
|
oops wrong branch... |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change introduces a separate Bun-based microservice ("porca") to handle certificate signing and revocation operations, migrates certificate generation from local OpenSSL execution to remote API calls, adds guest host routing through the proxy middleware, enhances the UI with multi-domain certificate support, and reorganizes Docker/deployment infrastructure with corresponding initialization scripts. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Middleware as Proxy<br/>(src/proxy.ts)
participant CertAPI as Next.js API<br/>(src/app/api/certs)
participant PorcaAPI as Porca Service<br/>(porca/index.ts)
participant DB as Database
Client->>Middleware: POST /api/certs/create<br/>(CSR, days)
alt Guest Host
Middleware->>Middleware: Detect hostname match
Middleware->>Middleware: Return 401 Unauthorized
else Authenticated Request
Middleware->>CertAPI: Forward to API route
CertAPI->>CertAPI: Validate IGNORE_PROTECTIONS<br/>& days <= 200
CertAPI->>PorcaAPI: POST /api/sign<br/>{proxyToken, csr, days}
PorcaAPI->>PorcaAPI: Validate proxy token
PorcaAPI->>PorcaAPI: Extract CN/SAN from CSR<br/>via OpenSSL
PorcaAPI->>PorcaAPI: Generate x509 certificate<br/>via OpenSSL
PorcaAPI-->>CertAPI: {pb, itemCN, error}
CertAPI->>DB: INSERT certificate record
CertAPI-->>Client: Certificate created
end
Client->>Middleware: DELETE /api/certs/{id}
Middleware->>CertAPI: Forward request
CertAPI->>DB: Query certificate<br/>by id
CertAPI->>CertAPI: Read public key file
CertAPI->>PorcaAPI: POST /api/revoke<br/>{proxyToken, cert}
PorcaAPI->>PorcaAPI: Run openssl ca -revoke
PorcaAPI->>PorcaAPI: Regenerate master.crl.pem
PorcaAPI-->>CertAPI: Success
CertAPI->>DB: DELETE certificate record
CertAPI-->>Client: Certificate Deleted!
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/navBar.tsx`:
- Around line 24-32: The NavBar gating compares window.location.hostname to the
full NEXT_PUBLIC_GUEST_RESOURCES_HOST string, so extract the hostname from
process.env.NEXT_PUBLIC_GUEST_RESOURCES_HOST before comparing: parse
guestResourcesHost using the URL constructor (with a try/catch fallback to strip
protocol/port) to obtain guestHostname, then replace the existing check that
uses guestResourcesHost with window.location.hostname === guestHostname inside
the pathname gating block (refer to guestResourcesHost, pathname, and the
hostname comparison in the if that controls NavBar visibility).
In `@src/proxy.ts`:
- Around line 14-15: The guest-resources rewrite is applied too early and is
capturing requests like /auth/login and /api/certs/master?get=download before
the auth/certificate path guards run; update the proxy route ordering in
src/proxy.ts so the rewrite to "/guest-resources" (the rule that rewrites guest
host requests) has explicit exclusions for the auth and cert download paths
(e.g., exclude "/auth/login" and "/api/certs/master" with the get=download
query) before that rewrite is applied, ensuring the existing path guard checks
(the auth and cert handlers) execute first.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fc7a0573-6de6-497f-8216-6645482377b7
📒 Files selected for processing (3)
.env.examplesrc/app/navBar.tsxsrc/proxy.ts
hpware
changed the title
hpware
changed the title
hpware
changed the title
hpware
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/core/certTooler.ts (1)
49-66:⚠️ Potential issue | 🔴 CriticalInitial config is overwritten and contains undefined references.
Several issues with the certificate extension configuration:
Dead code: The
[ v3_server ]block at line 49 (includingkeyUsage,extendedKeyUsage) is completely overwritten at lines 55 or 63 when SANs or CN exist. The assignment uses=instead of+=.Undefined section reference: Line 49 references
crlDistributionPoints =@crl_dp`` but the[ crl_dp ]section is never defined—OpenSSL will fail to parse this config.Unused section: The
[ server_cert ]section appended at line 66 won't be applied because the OpenSSL command lacks-extensions server_cert. Without this flag, OpenSSL ignores named sections.Missing env validation: If
NEXT_PUBLIC_GUEST_RESOURCES_URLis undefined, the CRL URI becomesURI:undefined/master.crl.pem.Proposed fix
- let configContent = `[ v3_server ]\nsubjectAltName = `@alt_names`\nkeyUsage = critical, digitalSignature, keyEncipherment\nextendedKeyUsage = serverAuth\ncrlDistributionPoints = `@crl_dp`\n\n`; + const crlUrl = process.env.NEXT_PUBLIC_GUEST_RESOURCES_URL; + if (!crlUrl) { + throw new Error("NEXT_PUBLIC_GUEST_RESOURCES_URL environment variable is not set"); + } + + let subjectAltName = ""; const cnMatch = getSAN.match(/Subject:.*?CN\s?=\s?([^\s,+/]+)/); let extractedCN = cnMatch ? cnMatch[1].trim() : ""; extractedCN = extractedCN.replace(/[\[\]]/g, ""); - //[ alt_names ] if (extractedSans) { - configContent = `subjectAltName = ${extractedSans}`; + subjectAltName = extractedSans; } else { if (extractedCN) { const isIP = /^(?:[0-9]{1,3}\.){3}[0-9]{1,3}$/.test(extractedCN) || extractedCN.includes(":"); - const prefix = isIP ? "IP" : "DNS"; - configContent = `subjectAltName = ${prefix}:${extractedCN}`; + subjectAltName = `${prefix}:${extractedCN}`; } } - configContent += `\n\n[ server_cert ]\ncrlDistributionPoints = URI:${process.env.NEXT_PUBLIC_GUEST_RESOURCES_URL}/master.crl.pem`; + + const configContent = `[ server_cert ] +subjectAltName = ${subjectAltName} +keyUsage = critical, digitalSignature, keyEncipherment +extendedKeyUsage = serverAuth +crlDistributionPoints = URI:${crlUrl}/master.crl.pem +`;Also add
-extensions server_certto the OpenSSL command:"-extfile", tempSavePath, + "-extensions", + "server_cert",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/core/certTooler.ts` around lines 49 - 66, The configContent construction currently overwrites the initial “[ v3_server ]” block and leaves an undefined `@crl_dp` section while appending a “[ server_cert ]” block that OpenSSL will ignore; update the logic in certTooler.ts so configContent starts with the full v3_server block (use += when adding SAN/CN lines instead of reassigning configContent), add a concrete “[ crl_dp ]” section (or inline the CRL URI) so `@crl_dp` is defined, validate process.env.NEXT_PUBLIC_GUEST_RESOURCES_URL before building the CRL URI to avoid "undefined", and ensure the OpenSSL invocation includes the -extensions server_cert flag so the appended [ server_cert ] section is actually applied; refer to variables/functions getSAN, extractedSans, extractedCN, configContent and the OpenSSL command invocation when implementing these fixes.
🧹 Nitpick comments (2)
src/components/core/certTooler.ts (1)
100-100: Mixing sync and async file operations.
fs.unlinkSyncis synchronous but is being awaited. This works but is misleading and inconsistent with thefs.promisesusage elsewhere in the file.Suggested fix
- if (fs.existsSync(tempSavePath)) await fs.unlinkSync(tempSavePath); + if (fs.existsSync(tempSavePath)) await fs.promises.unlink(tempSavePath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/core/certTooler.ts` at line 100, The code mixes sync and async file APIs by awaiting a synchronous call; replace the line that checks fs.existsSync(tempSavePath) and then awaits fs.unlinkSync(tempSavePath) with an async unlink using fs.promises (e.g., await fs.promises.unlink(tempSavePath)) or, if you prefer sync, remove the await and keep fs.unlinkSync(tempSavePath); update the usage around the tempSavePath reference to consistently use fs.promises.unlink for asynchronous behavior.src/proxy.ts (1)
22-22: Remove debugconsole.logstatement.This logs the hostname for every request passing through middleware. This adds noise to production logs and could expose internal routing information.
♻️ Proposed fix
const hostname = req.nextUrl.hostname; - console.log(hostname); if (hostname === String(process.env.NEXT_PUBLIC_GUEST_RESOURCES_HOST)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` at line 22, Remove the debug console.log that prints hostname inside the middleware: the stray console.log(hostname) should be deleted so requests no longer log hostnames; locate the middleware function in src/proxy.ts where the hostname variable is defined/used (reference the hostname identifier) and remove that console.log statement (or replace it with a conditional debug logger behind an environment flag if logging is needed).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 3: The .env example uses an inline comment after the environment variable
NEXT_PUBLIC_GUEST_RESOURCES_HOST which many dotenv parsers will include in the
value; remove the inline comment so the value is exactly
NEXT_PUBLIC_GUEST_RESOURCES_HOST=a.dev.yhw.tw and put the explanatory comment on
its own line (e.g. a separate line starting with #) so hostname checks in code
like src/proxy.ts and src/app/navBar.tsx receive the correct value.
In `@src/app/guest-resources/master.crl.pem/route.ts`:
- Line 3: The GET route handler currently returns nothing; update the exported
async function GET(request: NextRequest) to return a proper Response object:
read the CRL PEM contents (e.g., from the same guest-resources/master.crl.pem
file or a configured location), return new Response(crlPemString, { status: 200,
headers: { "Content-Type": "application/pkix-crl; charset=utf-8" } }), and
handle missing-file errors by returning an appropriate error Response (e.g., 404
or 500). Ensure you reference and use the exported GET function and NextRequest
type and do not leave the handler body empty.
In `@src/app/navBar.tsx`:
- Around line 42-44: The rendering gate that checks (guestHostname && typeof
window !== "undefined" && window.location.hostname === guestHostname) causes
hydration mismatch; change it to a client-only state check: import useState and
useEffect, introduce a state like isGuestHost (default false), set isGuestHost
inside useEffect by comparing window.location.hostname to guestHostname, and
replace the direct window check with that state in the NavBar rendering logic so
server and client render the same initial markup and the hostname check runs
only on the client.
---
Outside diff comments:
In `@src/components/core/certTooler.ts`:
- Around line 49-66: The configContent construction currently overwrites the
initial “[ v3_server ]” block and leaves an undefined `@crl_dp` section while
appending a “[ server_cert ]” block that OpenSSL will ignore; update the logic
in certTooler.ts so configContent starts with the full v3_server block (use +=
when adding SAN/CN lines instead of reassigning configContent), add a concrete
“[ crl_dp ]” section (or inline the CRL URI) so `@crl_dp` is defined, validate
process.env.NEXT_PUBLIC_GUEST_RESOURCES_URL before building the CRL URI to avoid
"undefined", and ensure the OpenSSL invocation includes the -extensions
server_cert flag so the appended [ server_cert ] section is actually applied;
refer to variables/functions getSAN, extractedSans, extractedCN, configContent
and the OpenSSL command invocation when implementing these fixes.
---
Nitpick comments:
In `@src/components/core/certTooler.ts`:
- Line 100: The code mixes sync and async file APIs by awaiting a synchronous
call; replace the line that checks fs.existsSync(tempSavePath) and then awaits
fs.unlinkSync(tempSavePath) with an async unlink using fs.promises (e.g., await
fs.promises.unlink(tempSavePath)) or, if you prefer sync, remove the await and
keep fs.unlinkSync(tempSavePath); update the usage around the tempSavePath
reference to consistently use fs.promises.unlink for asynchronous behavior.
In `@src/proxy.ts`:
- Line 22: Remove the debug console.log that prints hostname inside the
middleware: the stray console.log(hostname) should be deleted so requests no
longer log hostnames; locate the middleware function in src/proxy.ts where the
hostname variable is defined/used (reference the hostname identifier) and remove
that console.log statement (or replace it with a conditional debug logger behind
an environment flag if logging is needed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 76ace0cd-ea0f-4b8c-9f87-0e48c610f879
📒 Files selected for processing (6)
.env.examplesrc/app/guest-resources/.gitignoresrc/app/guest-resources/master.crl.pem/route.tssrc/app/navBar.tsxsrc/components/core/certTooler.tssrc/proxy.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/guest-resources/.gitignore
hpware
changed the title
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
porca/index.ts (5)
1-1: Remove unused import.The
$shell helper from Bun is imported but never used in this file.Suggested fix
-import { $ } from "bun"; -import { mkdir, unlink } from "node:fs/promises"; +import { mkdir, unlink } from "node:fs/promises";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@porca/index.ts` at line 1, Remove the unused import of the `$` helper from "bun" in porca/index.ts: delete the line importing `{ $ }` since `$` is not referenced anywhere in this module (ensure no other code relies on that symbol before removal).
96-96: Environment variable uses Next.js naming convention.
NEXT_PUBLIC_GUEST_RESOURCES_URLfollows Next.js conventions for client-exposed variables. Since porca is a standalone Bun service, consider using a more appropriate variable name (e.g.,CRL_DISTRIBUTION_URLorGUEST_RESOURCES_URL) to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@porca/index.ts` at line 96, The code appends a CRL URI using an environment variable named NEXT_PUBLIC_GUEST_RESOURCES_URL which follows Next.js client-exposed naming; update usages to a clearer server-side variable (e.g., CRL_DISTRIBUTION_URL or GUEST_RESOURCES_URL) by replacing process.env.NEXT_PUBLIC_GUEST_RESOURCES_URL in the configContent construction with the chosen server-only env var name and update any related deployment/env docs; ensure the new name is loaded in the Bun service environment and referenced wherever configContent is built so tests and runtime pick up the correct value.
25-27: Stub endpoint:/api/revokeis not implemented.This endpoint returns an empty response regardless of the request. For a certificate revocation system (as per PR title), this is a critical missing feature. Consider either implementing it or removing the route until ready.
Would you like me to help implement the certificate revocation logic, or should I open an issue to track this task?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@porca/index.ts` around lines 25 - 27, The "/api/revoke" route in porca/index.ts is a stub that returns an empty Response; implement proper revocation by validating and authenticating the incoming request (ensure caller is authorized), parse the revocation payload (certificate serial or id and optional reason) from req, look up the certificate in the certificate store/DB, mark it revoked with timestamp and reason (or return 404 if not found), persist the change, and return appropriate HTTP responses (e.g., 200 with JSON confirmation, 400 for bad requests, 401 for unauthorized, 404 for not found, 500 for server errors); add logging for success/failure and error handling. If you prefer not to implement now, remove the "/api/revoke" route or add a clear TODO and return 501 Not Implemented instead.
97-98: Redundant condition:if (configContent)is always true.At this point,
configContentwill always have a value (from lines 85, 93, or the initial assignment). This condition check is unnecessary.Suggested fix
- if (configContent) { - await Bun.write(tempSavePath, configContent); - } + await Bun.write(tempSavePath, configContent);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@porca/index.ts` around lines 97 - 98, The conditional "if (configContent)" is redundant because configContent is guaranteed to be set earlier; remove the guard and always call Bun.write(tempSavePath, configContent) so the file is written unconditionally, leaving the variables configContent and tempSavePath and the Bun.write(...) invocation intact (no additional null checks needed since configContent is always assigned).
64-131: Code duplication withsrc/components/core/certTooler.ts.This function is nearly identical to the
generateCertificatefunction insrc/components/core/certTooler.ts(lines 34-95 in that file). Having two implementations creates maintenance burden—fixes or changes must be applied in both places.Consider either:
- Having the Next.js app call porca's
/api/signendpoint instead of duplicating the logic- Extracting the shared logic into a common package
Based on the relevant code snippets, the Next.js app currently uses its own local implementation rather than calling porca. Clarify the intended architecture to avoid drift between implementations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@porca/index.ts` around lines 64 - 131, There are two near-identical implementations of generateCertificate (the one in porca/index.ts and the one in certTooler.ts) causing duplication; pick one of two fixes: (A) remove the local implementation in certTooler.ts and have the client call porca's /api/sign endpoint (update certTooler.ts to call the API and accept the returned { pb, itemCN }), or (B) extract the shared logic (CSR parsing, SAN/CN extraction, config generation, OpenSSL invocation, and file handling) into a single shared module/package and update both generateCertificate usages to import and call that module (preserve behavior: spawnWithInput, temp config file creation, writing to ./certs/created, and returned shape). Ensure only one canonical generateCertificate implementation remains and adapt error handling and environment usage consistently.porca/package.json (1)
6-8: Pin@types/bunto a specific version for reproducible builds.Using
"latest"for dependencies can lead to inconsistent builds across environments and time. Consider pinning to a specific version (e.g.,"1.2.0"or whatever version is currently in use).Suggested fix
"devDependencies": { - "@types/bun": "latest" + "@types/bun": "^1.2.0" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@porca/package.json` around lines 6 - 8, The devDependency "@types/bun" is pinned to "latest" which makes builds non-reproducible; update the package.json devDependencies entry for "@types/bun" to a specific version string (for example "1.2.0" or the exact version currently used) so that installs are deterministic, then regenerate your lockfile (npm/yarn/pnpm) to lock that version; look for the "@types/bun" key in package.json to make this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@porca/index.ts`:
- Around line 9-16: Validate the incoming days value after parsing body ({ csr,
days }) in porca/index.ts: ensure days is a number, an integer, and within
reasonable bounds (e.g., >=1 and <=3650 or whatever policy you choose); if
validation fails return a 400 Response.json with a clear error message instead
of only checking existence; keep the existing csr presence check and reuse the
same response shape for errors so the caller gets consistent feedback.
- Around line 79-95: The initial multi-line assignment to configContent is dead
because later branches overwrite it; remove that initial assignment and instead
build a base header string (e.g., const v3Header = "[ v3_server ]\nkeyUsage =
...\n\n") and then set configContent by concatenating v3Header + the
subjectAltName portion in the branches that currently assign configContent (the
branches using extractedSans and extractedCN). Update references to
configContent, extractedSans, extractedCN and getSAN so the final configContent
always includes the v3 header plus the computed subjectAltName rather than
replacing the header.
- Around line 4-24: The /api/sign route currently processes requests without
auth; add API key validation at the start of the POST handler to reject
unauthenticated requests before parsing csr/days. Read a secret (e.g.,
process.env.MASTER_API_KEY) and compare it to an incoming header such as
Authorization or x-api-key, returning a 401/403 JSON response if
missing/invalid; only then proceed to parse body and call
generateCertificate(csr, Number(days)). Ensure the auth check happens before any
certificate operations and include descriptive error responses for
missing/invalid keys.
---
Nitpick comments:
In `@porca/index.ts`:
- Line 1: Remove the unused import of the `$` helper from "bun" in
porca/index.ts: delete the line importing `{ $ }` since `$` is not referenced
anywhere in this module (ensure no other code relies on that symbol before
removal).
- Line 96: The code appends a CRL URI using an environment variable named
NEXT_PUBLIC_GUEST_RESOURCES_URL which follows Next.js client-exposed naming;
update usages to a clearer server-side variable (e.g., CRL_DISTRIBUTION_URL or
GUEST_RESOURCES_URL) by replacing process.env.NEXT_PUBLIC_GUEST_RESOURCES_URL in
the configContent construction with the chosen server-only env var name and
update any related deployment/env docs; ensure the new name is loaded in the Bun
service environment and referenced wherever configContent is built so tests and
runtime pick up the correct value.
- Around line 25-27: The "/api/revoke" route in porca/index.ts is a stub that
returns an empty Response; implement proper revocation by validating and
authenticating the incoming request (ensure caller is authorized), parse the
revocation payload (certificate serial or id and optional reason) from req, look
up the certificate in the certificate store/DB, mark it revoked with timestamp
and reason (or return 404 if not found), persist the change, and return
appropriate HTTP responses (e.g., 200 with JSON confirmation, 400 for bad
requests, 401 for unauthorized, 404 for not found, 500 for server errors); add
logging for success/failure and error handling. If you prefer not to implement
now, remove the "/api/revoke" route or add a clear TODO and return 501 Not
Implemented instead.
- Around line 97-98: The conditional "if (configContent)" is redundant because
configContent is guaranteed to be set earlier; remove the guard and always call
Bun.write(tempSavePath, configContent) so the file is written unconditionally,
leaving the variables configContent and tempSavePath and the Bun.write(...)
invocation intact (no additional null checks needed since configContent is
always assigned).
- Around line 64-131: There are two near-identical implementations of
generateCertificate (the one in porca/index.ts and the one in certTooler.ts)
causing duplication; pick one of two fixes: (A) remove the local implementation
in certTooler.ts and have the client call porca's /api/sign endpoint (update
certTooler.ts to call the API and accept the returned { pb, itemCN }), or (B)
extract the shared logic (CSR parsing, SAN/CN extraction, config generation,
OpenSSL invocation, and file handling) into a single shared module/package and
update both generateCertificate usages to import and call that module (preserve
behavior: spawnWithInput, temp config file creation, writing to ./certs/created,
and returned shape). Ensure only one canonical generateCertificate
implementation remains and adapt error handling and environment usage
consistently.
In `@porca/package.json`:
- Around line 6-8: The devDependency "@types/bun" is pinned to "latest" which
makes builds non-reproducible; update the package.json devDependencies entry for
"@types/bun" to a specific version string (for example "1.2.0" or the exact
version currently used) so that installs are deterministic, then regenerate your
lockfile (npm/yarn/pnpm) to lock that version; look for the "@types/bun" key in
package.json to make this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d372c65-b484-4225-8828-771020e501ca
⛔ Files ignored due to path filters (1)
porca/bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
porca/.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdcporca/.gitignoreporca/CLAUDE.mdporca/README.mdporca/index.tsporca/package.jsonporca/tsconfig.json
✅ Files skipped from review due to trivial changes (3)
- porca/CLAUDE.md
- porca/.gitignore
- porca/.cursor/rules/use-bun-instead-of-node-vite-npm-pnpm.mdc
…er config, and proxy debug log - Move inline comment in .env.example to its own line so dotenv parsers don't include it in the host value - Implement GET handler for master.crl.pem route with proper Response, content type, and error handling - Fix navBar hydration mismatch by using useState/useEffect for the guest hostname check instead of direct window access - Fix certTooler configContent overwrites (use +=), add env fallback for CRL URL, add -extensions server_cert to OpenSSL, fix sync/async unlink - Remove stray console.log(hostname) from proxy middleware Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/app/guest-resources/master.crl.pem/route.ts (1)
5-18: Implementation looks correct; minor: prefix unused parameter.The CRL serving logic properly handles file reading, appropriate Content-Type header, and error cases. The implementation addresses the previously flagged empty handler issue.
The
requestparameter is unused—prefix with underscore to signal intent.✏️ Suggested fix
-export const GET = async (request: NextRequest) => { +export const GET = async (_request: NextRequest) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/guest-resources/master.crl.pem/route.ts` around lines 5 - 18, The GET handler currently declares an unused parameter named request; rename it to _request to signal it's intentionally unused (update the function signature export const GET = async (_request: NextRequest) =>) and ensure any references (if added later) are updated accordingly — this keeps linting happy and documents intent for the GET function serving the CRL.src/proxy.ts (2)
1-3: Unused import:headersfromnext/headers.The
headersimport on line 2 is not used anywhere in this file. Consider removing it.✏️ Suggested fix
import { NextRequest, NextResponse } from "next/server"; -import { headers } from "next/headers"; import { auth } from "./components/auth";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` around lines 1 - 3, The import list in src/proxy.ts includes an unused symbol `headers` from "next/headers"; remove `headers` from the import statement (leaving NextRequest, NextResponse and auth imports intact) so the module no longer imports an unused binding and avoids linter/compile warnings.
18-20: Unused variable:url.The
urlvariable on line 19 is declared but never used. Consider removing it.✏️ Suggested fix
export default async function proxy(req: NextRequest) { - const url = req.nextUrl; const path = req.nextUrl.pathname; const hostname = req.nextUrl.hostname;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` around lines 18 - 20, The variable `url` declared in the async function proxy (export default async function proxy(req: NextRequest)) is unused; remove the unused declaration `const url = req.nextUrl;` and reference the request URL directly via `req.nextUrl` or continue using `path = req.nextUrl.pathname` as needed so only `path` (or other used vars) remain in the function.src/components/core/certTooler.ts (3)
103-103: Consider wrapping cleanup in try-catch to avoid masking original errors.If the
tryblock throws andunlinkalso fails, the unlink error will mask the original exception. Additionally, mixing synchronousexistsSyncwith asyncunlinkis a minor code smell.♻️ Proposed safer cleanup
} finally { - if (fs.existsSync(tempSavePath)) await fs.promises.unlink(tempSavePath); + try { + await fs.promises.unlink(tempSavePath); + } catch { + // Ignore cleanup errors to preserve original exception + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/core/certTooler.ts` at line 103, The cleanup using fs.existsSync(tempSavePath) followed by await fs.promises.unlink(tempSavePath) can mask the original error and mixes sync/async calls; change this to attempt an async unlink inside a safe try/catch (or try/finally) so unlink errors are caught and logged/ignored instead of rethrowing and masking the original exception — replace the existsSync + unlink sequence with an unconditional await fs.promises.unlink(tempSavePath) inside its own try/catch, referencing tempSavePath and fs.promises.unlink in your change and ensure any unlink error is handled (e.g., log.debug or swallow) without throwing.
67-70: Redundant condition:configContentis always truthy.Since
configContentis initialized with a non-empty string on line 51, theif (configContent)check on line 68 will always be true. Consider removing the conditional wrapper.♻️ Proposed simplification
configContent += `crlDistributionPoints = URI:${crlBaseUrl}/master.crl.pem\n`; - if (configContent) { - await fs.promises.writeFile(tempSavePath, configContent); - } + await fs.promises.writeFile(tempSavePath, configContent);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/core/certTooler.ts` around lines 67 - 70, The if-check around writing the temp file is redundant because configContent is initialized non-empty; remove the conditional and always call fs.promises.writeFile(tempSavePath, configContent) (so update the block containing configContent and the write call in certTooler.ts by deleting the if (configContent) wrapper and leaving the single await fs.promises.writeFile(...) statement).
67-67: Ensuremaster.crl.pemexists before certificates reference it.The CRL distribution point embeds
${crlBaseUrl}/master.crl.pem, but based on the route handler atsrc/app/guest-resources/master.crl.pem/route.ts, the file./certs/master.crl.pemmust exist or clients performing CRL validation will receive a 404. Consider documenting the CRL generation step or adding a startup check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/core/certTooler.ts` at line 67, The config string in certTooler.ts appends "crlDistributionPoints = URI:${crlBaseUrl}/master.crl.pem" (see the configContent append) but the server-side route for serving master.crl.pem will 404 unless that file exists; update the project so master.crl.pem is created before certificates reference it by either (A) adding CRL generation into your certificate tool flow where configContent is built (generate and write ./certs/master.crl.pem as part of the cert creation routine), or (B) adding a startup health check that verifies the presence of master.crl.pem and fails/start-logs a clear error, and also add a short documentation note in the cert generation README; reference the configContent append in certTooler.ts and the master.crl.pem route handler when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/core/certTooler.ts`:
- Around line 57-65: The code path that appends subjectAltName can leave the
certificate without any SAN when both extractedSans and extractedCN are empty;
update the block that builds configContent so that if neither extractedSans nor
extractedCN is present you (1) emit a clear warning (use the existing logger or
console.warn) mentioning extractedSans and extractedCN are empty and (2) append
a safe fallback SAN (for example "subjectAltName = DNS:localhost") to
configContent to ensure a SAN exists; reference the variables extractedSans,
extractedCN and configContent when making this change.
---
Nitpick comments:
In `@src/app/guest-resources/master.crl.pem/route.ts`:
- Around line 5-18: The GET handler currently declares an unused parameter named
request; rename it to _request to signal it's intentionally unused (update the
function signature export const GET = async (_request: NextRequest) =>) and
ensure any references (if added later) are updated accordingly — this keeps
linting happy and documents intent for the GET function serving the CRL.
In `@src/components/core/certTooler.ts`:
- Line 103: The cleanup using fs.existsSync(tempSavePath) followed by await
fs.promises.unlink(tempSavePath) can mask the original error and mixes
sync/async calls; change this to attempt an async unlink inside a safe try/catch
(or try/finally) so unlink errors are caught and logged/ignored instead of
rethrowing and masking the original exception — replace the existsSync + unlink
sequence with an unconditional await fs.promises.unlink(tempSavePath) inside its
own try/catch, referencing tempSavePath and fs.promises.unlink in your change
and ensure any unlink error is handled (e.g., log.debug or swallow) without
throwing.
- Around line 67-70: The if-check around writing the temp file is redundant
because configContent is initialized non-empty; remove the conditional and
always call fs.promises.writeFile(tempSavePath, configContent) (so update the
block containing configContent and the write call in certTooler.ts by deleting
the if (configContent) wrapper and leaving the single await
fs.promises.writeFile(...) statement).
- Line 67: The config string in certTooler.ts appends "crlDistributionPoints =
URI:${crlBaseUrl}/master.crl.pem" (see the configContent append) but the
server-side route for serving master.crl.pem will 404 unless that file exists;
update the project so master.crl.pem is created before certificates reference it
by either (A) adding CRL generation into your certificate tool flow where
configContent is built (generate and write ./certs/master.crl.pem as part of the
cert creation routine), or (B) adding a startup health check that verifies the
presence of master.crl.pem and fails/start-logs a clear error, and also add a
short documentation note in the cert generation README; reference the
configContent append in certTooler.ts and the master.crl.pem route handler when
making the change.
In `@src/proxy.ts`:
- Around line 1-3: The import list in src/proxy.ts includes an unused symbol
`headers` from "next/headers"; remove `headers` from the import statement
(leaving NextRequest, NextResponse and auth imports intact) so the module no
longer imports an unused binding and avoids linter/compile warnings.
- Around line 18-20: The variable `url` declared in the async function proxy
(export default async function proxy(req: NextRequest)) is unused; remove the
unused declaration `const url = req.nextUrl;` and reference the request URL
directly via `req.nextUrl` or continue using `path = req.nextUrl.pathname` as
needed so only `path` (or other used vars) remain in the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aa7ab161-74e6-432f-bad0-454bd1cbeaa5
📒 Files selected for processing (5)
.env.examplesrc/app/guest-resources/master.crl.pem/route.tssrc/app/navBar.tsxsrc/components/core/certTooler.tssrc/proxy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/navBar.tsx
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (1)
.env.example (1)
15-15:⚠️ Potential issue | 🟡 MinorKeep the
IGNORE_PROTECTIONSnote on its own line.Line 15 reintroduces the same inline-comment pattern that was already fixed above. If this file is copied into a dotenv loader that keeps inline comments, the flag stops being the literal
false.🐛 Proposed fix
-IGNORE_PROTECTIONS=false # FOR GENERIC HOME LAB USE, IT IS SAFE TO SET TO true +# FOR GENERIC HOME LAB USE, IT IS SAFE TO SET TO true +IGNORE_PROTECTIONS=false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.env.example at line 15, The IGNORE_PROTECTIONS entry currently has an inline comment which can corrupt the literal value when loaded; split the inline comment off onto its own line above the variable so the line with IGNORE_PROTECTIONS=false contains only the literal assignment (refer to the IGNORE_PROTECTIONS key in the .env example) and ensure the explanatory text is preserved on its own line for dotenv-safe parsing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.docker:
- Line 16: The PROTECTION_PROXY_URL env var is currently set to localhost which
fails inside the Docker network; update the value of PROTECTION_PROXY_URL to use
the Docker Compose service DNS name "porca:4000" (no trailing slash) so
components like cert-manager that call fetch('http://.../api/sign') reach the
porca service over the internal network; locate the PROTECTION_PROXY_URL
definition in .env.docker and replace "http://localhost:4000" with
"http://porca:4000" (preserve the note about no trailing slash).
In @.gitignore:
- Around line 33-36: The .gitignore currently re-includes .env.docker via the
!.env.docker rule which allows sensitive vars (e.g. PROTECTION_PROXY_TOKEN) to
be committed; remove the negation for !.env.docker so .env.docker remains
ignored, and instead add a non-sensitive template name like .env.docker.example
(and keep !.env.example) so only the example file is committed; update the
entries by deleting !.env.docker and, if needed, adding .env.docker.example to
the repo as the template.
In `@docker-compose.yml`:
- Around line 50-51: The healthcheck uses the literal "{db_user}" which prevents
shell expansion; update the pg_isready invocation in the healthcheck (the test
entry that calls pg_isready) to use the container environment variable with
Docker Compose escaping so the shell inside the container sees a single $ (e.g.,
replace {db_user} with $${DB_USER} or $${DB_USER:-default} in the "CMD-SHELL"
test string), and ensure the container defines DB_USER in its environment so
pg_isready -U sees the actual username.
In `@init.sh`:
- Around line 17-20: After creating the CA key and cert (master.key.pem and
master.pub.pem) in init.sh, also bootstrap an initial CRL at
./certs/master.crl.pem so the revocation URL doesn't 404; do this by invoking
OpenSSL to generate a CRL from the new CA (for example using openssl ca -config
<openssl.cnf> -gencrl -out ./certs/master.crl.pem) or, if your setup lacks an
openssl.cnf, create a minimal valid CRL file to serve until the first real
revocation is issued; ensure the script writes ./certs/master.crl.pem alongside
master.key.pem and master.pub.pem.
- Around line 17-20: The init script currently always overwrites the CA key and
cert; change it to first check for existing CA files (master.key.pem and
master.pub.pem) and abort if either exists: before generating keys, test for
file existence (e.g., if [ -f "master.key.pem" ] || [ -f "master.pub.pem" ] ),
print a clear error message about not regenerating the master CA and exit
non‑zero; keep the mkdir -p and the OpenSSL commands (openssl genrsa and openssl
req) untouched but only run them when no existing CA files are present, and
document that rotation must be performed via an explicit rotation workflow or
flag.
In `@porca/Dockerfile`:
- Around line 8-21: The final Dockerfile stage leaves the service running as
root; create a dedicated non-root runtime user (e.g., "porca" or "signer"),
chown the app files and any mounted volume paths that the service needs to
access, and add USER <username> before CMD so Bun runs without root privileges.
Locate the final stage instructions (WORKDIR /app, COPY --from=builder ...,
EXPOSE, ENV, CMD ["bun","run","index.ts"]) and insert user creation and
ownership steps (using RUN groupadd/useradd and chown on /app and the expected
mount path for the master CA key), then switch to that user with USER <username>
immediately prior to CMD. Ensure file permissions still allow the service to
read the master CA key from the mounted volume but do not run the process as
root.
In `@porca/index.ts`:
- Around line 48-63: The /api/revoke handler currently authenticates via
protectionProxyToken but never records the revoked ID or updates the CRL; update
the POST handler (the "/api/revoke" route) to validate body.revokeID, persist
the revoked serial (e.g., call or implement addRevokedSerial(revokeID) or append
it to your revocation store), and then regenerate the master CRL file (e.g.,
call or implement regenerateMasterCRL() which rewrites ./certs/master.crl.pem).
Ensure errors when persisting or regenerating return an error response and a
successful revocation returns a 200 after the CRL is updated.
- Around line 4-5: The code currently falls back to a weak default for
protectionProxyToken; change it so startup fails if PROTECTION_PROXY_TOKEN is
not set: check process.env.PROTECTION_PROXY_TOKEN and if absent throw an Error
(or call process.exit(1)) with a clear message so the service refuses to boot
rather than using the "defaultToken" value; update the initialization of the
protectionProxyToken constant (the variable named protectionProxyToken) and any
startup path that relies on it to rely on the validated value.
- Around line 109-115: The code currently dumps the full parsed CSR into logs
via console.log(getSAN), which leaks sensitive hostnames/IPs; remove the
console.log(getSAN) call in the block that calls spawnWithInput("openssl",
["req", "-noout", "-text", "-in", "-"], csrText) and avoid logging the full
getSAN variable; instead, if logging is needed, log only minimal, non-sensitive
outcomes (e.g., whether sanMatch was found or the count of SAN entries) using
the existing sanMatch variable so you keep functionality in functions like
spawnWithInput and the sanMatch parsing but do not emit the CSR contents to
logs.
- Around line 149-204: The OpenSSL invocation built in the spawnWithInput call
is missing the "-extensions v3_server" flag so the named [ v3_server ] extfile
section (containing SAN, keyUsage, extendedKeyUsage, crlDistributionPoints)
isn't applied; update the arguments passed to spawnWithInput (the array in the
openssl x509 call that currently includes "-extfile", tempSavePath) to include
"-extensions" followed by "v3_server" (use the same tempSavePath/configContent
and ensure the named section matches "v3_server").
In `@src/components/core/certTooler.ts`:
- Around line 48-57: The fetch call in certTooler.ts that constructs req when
calling the /api/sign endpoint is missing the HTTP method, causing a GET (which
cannot have a body) and a runtime TypeError; update the fetch options for the
request created in the block that builds req to include method: "POST" alongside
the existing headers and body so the call matches the endpoint's POST-only
contract (keep the Content-Type and JSON.stringify(csrText, proxyToken, days)
as-is).
In `@src/proxy.ts`:
- Around line 21-29: The current hostname-check block in src/proxy.ts (the if
that compares hostname to process.env.NEXT_PUBLIC_GUEST_RESOURCES_HOST and uses
path.startsWith("/auth/") || path.startsWith("/api/") to call
NextResponse.next()) incorrectly bypasses auth for every /api/* route; change
this to allowlist only explicit public endpoints (e.g., the public certs
endpoint used by src/app/api/certs/master/route.ts) instead of permitting all
"/api/" paths, and apply the same fix to the similar conditional later in the
file (the block around lines 37-40). Update the condition to check path against
a small array/set of approved public paths (and keep auth exempt for "/auth/*"
as intended) before calling NextResponse.next(), otherwise continue to rewrite
to /guest-resources via NextResponse.rewrite(req.url).
- Line 28: The rewrite currently creates a new URL with new
URL(`/guest-resources${path}`, req.url) which discards query parameters; update
the NextResponse.rewrite call in src/proxy.ts (the code using
NextResponse.rewrite, path, and req.nextUrl) to include the original request's
search/query string (req.nextUrl.search) when constructing the target URL so
that query params are preserved for guest pages.
---
Duplicate comments:
In @.env.example:
- Line 15: The IGNORE_PROTECTIONS entry currently has an inline comment which
can corrupt the literal value when loaded; split the inline comment off onto its
own line above the variable so the line with IGNORE_PROTECTIONS=false contains
only the literal assignment (refer to the IGNORE_PROTECTIONS key in the .env
example) and ensure the explanatory text is preserved on its own line for
dotenv-safe parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9434231a-a4f6-4cf9-a964-aa0c549acfc2
📒 Files selected for processing (15)
.env.docker.env.example.github/workflows/build_docker_porca.yaml.gitignoreREADME.mddocker-compose.ymlinit.shporca/.env.exampleporca/Dockerfileporca/compose.ymlporca/index.tssetup_server.shsrc/app/guest-resources/master.crl.pem/route.tssrc/components/core/certTooler.tssrc/proxy.ts
💤 Files with no reviewable changes (1)
- setup_server.sh
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/build_docker_porca.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/guest-resources/master.crl.pem/route.ts
- .env.example: move IGNORE_PROTECTIONS inline comment to own line - .env.docker → .env.docker.example: use Docker DNS (porca:4000) for PROTECTION_PROXY_URL, rename to prevent committing secrets - .gitignore: track .env.docker.example instead of .env.docker - docker-compose.yml: fix pg_isready healthcheck to use $$POSTGRES_USER - init.sh: guard against overwriting existing CA, bootstrap empty CRL - porca/Dockerfile: run as non-root user - porca/index.ts: fail on missing PROTECTION_PROXY_TOKEN, remove debug console.log(getSAN), add -extensions v3_server, implement revoke - certTooler.ts: add method: "POST" to fetch (was defaulting to GET) - proxy.ts: preserve query params in guest-resources rewrite Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/components/core/certTooler.ts (1)
53-53: Fallback token may mask configuration errors.Using
"defaultToken"as a fallback whenPROTECTION_PROXY_TOKENis not set will cause silent authentication failures against the porca service rather than a clear configuration error.♻️ Consider failing fast on missing config
body: JSON.stringify({ - proxyToken: process.env.PROTECTION_PROXY_TOKEN || "defaultToken", + proxyToken: process.env.PROTECTION_PROXY_TOKEN ?? (() => { + throw new Error("PROTECTION_PROXY_TOKEN environment variable is required"); + })(), csr: csrText, days: generateDays, }),Or validate at the start of the function:
if (!process.env.PROTECTION_PROXY_TOKEN) { throw new Error("PROTECTION_PROXY_TOKEN environment variable is required"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/core/certTooler.ts` at line 53, The code currently falls back to a hardcoded "defaultToken" for proxyToken which can mask misconfiguration; update certTooler initialization to fail fast by validating process.env.PROTECTION_PROXY_TOKEN (used to set proxyToken) and throw a clear Error if missing (e.g., in the function or module where proxyToken is read, such as certTooler or its initializer), so you remove the "defaultToken" fallback and ensure authentication configuration problems surface immediately.README.md (1)
5-7: Minor grammar improvements.Consider hyphenating compound adjectives for consistency:
✏️ Suggested changes
-My self hosted Guest Resources is here: https://certs.default.tw +My self-hosted Guest Resources is here: https://certs.default.tw-This platform is what I need for my home lab, a place I could get custom long term certs for my internal services, without the hassle (and security issues) for exposing them to the internet in order to get SSL certs, now you get your own. +This platform is what I need for my home lab, a place I could get custom long-term certs for my internal services, without the hassle (and security issues) of exposing them to the internet to get SSL certs. Now you get your own.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 5 - 7, The README sentences use un-hyphenated compound adjectives; update the phrase "My self hosted Guest Resources" to "My self-hosted Guest Resources", "what I need for my home lab" to "what I need for my home‑lab" if you want it treated as a compound adjective, and "custom long term certs" to "custom long-term certs" (and consider "internal services" as-is). Locate these phrases in the README content and apply the hyphens for consistency.porca/index.ts (1)
117-127: Hardcoded regional defaults may not suit all deployments.The default values
"TW","Taipei"are region-specific. Consider making these configurable via environment variables or documenting that CSRs should include complete subject information.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@porca/index.ts` around lines 117 - 127, The code hardcodes regional defaults in cMatch, stMatch, and lMatch ("TW"/"Taipei"), which should be configurable; update the logic in the section that computes cMatch, stMatch, and lMatch to use environment-configurable defaults (e.g., process.env.DEFAULT_COUNTRY, DEFAULT_STATE, DEFAULT_LOCALITY) or a central config object and fall back to an empty string if no env/config is provided, ensure the same approach is applied to lMatch and any other subject defaults (oMatch/ouMatch if desired), and update README/docs to state that CSRs should include subject fields or that these defaults can be changed via the new environment variables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.docker:
- Line 14: The inline comment after the environment value risks being included
in the variable value; remove the trailing comment from the same line for
IGNORE_PROTECTIONS (leave the line as "IGNORE_PROTECTIONS=false") and move any
explanation to its own comment line above or below (e.g., a line starting with
"#") so dotenv parsers won't parse the comment as part of the value.
In @.env.example:
- Line 15: The inline comment on the IGNORE_PROTECTIONS entry may be parsed into
the variable value; update the .env.example so IGNORE_PROTECTIONS is set to a
bare value (e.g., IGNORE_PROTECTIONS=false) and move the explanatory text ("FOR
GENERIC HOME LAB USE, IT IS SAFE TO SET TO true") to its own separate comment
line above or below the variable so dotenv parsers won't include the comment in
the value.
In @.github/workflows/build_docker_porca.yaml:
- Around line 46-55: The Docker build step ("Build and push Docker image" using
docker/build-push-action@v5) is using context: . and file: ./Dockerfile which
points at the repo root; update the action to use the porca subdirectory by
setting context to ./porca and file to ./porca/Dockerfile (or remove any
incorrect working-directory usage), so the action and dockerfile resolve from
the porca folder and the build uses the correct context and Dockerfile.
- Around line 34-35: The image reference is missing the repository owner prefix
so GHCR will reject or misplace the push; update the workflow to include the
owner when composing the image name by either ensuring env.REGISTRY includes the
owner or by changing the images input to include the owner explicitly (e.g., use
ghcr.io/${{ github.repository_owner }}/${{ env.IMAGE_NAME }}), so the produced
image reference is in the form ghcr.io/<owner>/<image>.
In `@docker-compose.yml`:
- Around line 35-40: The porca container's healthcheck uses curl but the
oven/bun:debian base lacks it; in the porca Dockerfile add installation of curl
in the production stage (e.g., in the stage named "production" or the final
stage that builds the image) by running apt-get update && apt-get install -y
curl (and then cleanup apt lists) so the healthcheck command used in
docker-compose.yml (test: ["CMD", "curl", "-f", "http://localhost:4000/ok"]) can
succeed; ensure this change is only applied to the production/final stage that
produces the image used by docker-compose.
In `@porca/index.ts`:
- Around line 16-24: The response for invalid/missing proxy tokens currently
returns a 404; update the authentication failure to return a 401 status instead.
Locate the token check that compares body.proxyToken to protectionProxyToken
(the block using Response.json) and change the response status from 404 to 401
and keep the same error message (use 401 for both missing and mismatched
tokens). Ensure the Response.json call that constructs the error payload still
executes in the same place.
- Line 155: The CRL distribution point is being built directly from
process.env.NEXT_PUBLIC_GUEST_RESOURCES_URL which can be undefined; validate and
normalize that env var at module initialization by reading
process.env.NEXT_PUBLIC_GUEST_RESOURCES_URL into a new constant (e.g.,
guestResourcesUrl), check it's present and a valid URL (or throw a clear
error/fail fast or use a safe default), and then use that guestResourcesUrl when
constructing the crlDistributionPoints value (replace the direct use of
process.env.NEXT_PUBLIC_GUEST_RESOURCES_URL in the crlDistributionPoints
assignment with the validated/normalized guestResourcesUrl).
In `@src/components/core/certTooler.ts`:
- Around line 79-85: The revokeCertificate function currently has an empty
try/catch that silently swallows errors; update revokeCertificate(saveUUID:
string = crypto.randomUUID()) to either (a) immediately throw a clear
"NotImplementedError" (or new Error("revokeCertificate not implemented")) from
the function body so callers get explicit feedback, or (b) remove the try/catch
and implement the actual revoke logic; if you keep a try/catch, rethrow or log
the caught error instead of ignoring it (use the caught variable e). Ensure
references to revokeCertificate and the default saveUUID (crypto.randomUUID())
remain unchanged.
---
Nitpick comments:
In `@porca/index.ts`:
- Around line 117-127: The code hardcodes regional defaults in cMatch, stMatch,
and lMatch ("TW"/"Taipei"), which should be configurable; update the logic in
the section that computes cMatch, stMatch, and lMatch to use
environment-configurable defaults (e.g., process.env.DEFAULT_COUNTRY,
DEFAULT_STATE, DEFAULT_LOCALITY) or a central config object and fall back to an
empty string if no env/config is provided, ensure the same approach is applied
to lMatch and any other subject defaults (oMatch/ouMatch if desired), and update
README/docs to state that CSRs should include subject fields or that these
defaults can be changed via the new environment variables.
In `@README.md`:
- Around line 5-7: The README sentences use un-hyphenated compound adjectives;
update the phrase "My self hosted Guest Resources" to "My self-hosted Guest
Resources", "what I need for my home lab" to "what I need for my home‑lab" if
you want it treated as a compound adjective, and "custom long term certs" to
"custom long-term certs" (and consider "internal services" as-is). Locate these
phrases in the README content and apply the hyphens for consistency.
In `@src/components/core/certTooler.ts`:
- Line 53: The code currently falls back to a hardcoded "defaultToken" for
proxyToken which can mask misconfiguration; update certTooler initialization to
fail fast by validating process.env.PROTECTION_PROXY_TOKEN (used to set
proxyToken) and throw a clear Error if missing (e.g., in the function or module
where proxyToken is read, such as certTooler or its initializer), so you remove
the "defaultToken" fallback and ensure authentication configuration problems
surface immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c4c7ef1-0a48-4441-9e59-e197d3f15340
📒 Files selected for processing (15)
.env.docker.env.example.github/workflows/build_docker_porca.yaml.gitignoreREADME.mddocker-compose.ymlinit.shporca/.env.exampleporca/Dockerfileporca/compose.ymlporca/index.tssetup_server.shsrc/app/guest-resources/master.crl.pem/route.tssrc/components/core/certTooler.tssrc/proxy.ts
💤 Files with no reviewable changes (1)
- setup_server.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- .gitignore
- src/app/guest-resources/master.crl.pem/route.ts
- porca/compose.yml
- src/proxy.ts
- .env.docker.example: move IGNORE_PROTECTIONS inline comment to own line - GitHub workflow: fix context/file to ./porca, add repo owner to image name - docker-compose.yml: pass NEXT_PUBLIC_GUEST_RESOURCES_URL to porca service - porca/Dockerfile: install curl for healthcheck - porca/index.ts: return 401 (not 404) for auth failures, validate NEXT_PUBLIC_GUEST_RESOURCES_URL at startup, use env-configurable defaults for subject fields instead of hardcoded TW/Taipei - certTooler.ts: remove defaultToken fallback, throw NotImplementedError from empty revokeCertificate instead of silently swallowing - README.md: fix hyphenation (self-hosted, long-term) and typo Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
generation system suck ass
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Owner
Author
|
Will create another PR for the new SAN migration. |
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/certs/client.tsx (1)
158-164:⚠️ Potential issue | 🟠 MajorThe new default validity already violates the server-side policy.
src/components/core/certTooler.ts:40-46now rejects anything over 200 days unlessIGNORE_PROTECTIONS=true, and.env.docker.example:15leaves that off by default. Keeping the form default at 365 makes the out-of-the-box create flow fail until the user manually lowers the number.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/certs/client.tsx` around lines 158 - 164, The form default of the Days <Input> in src/app/certs/client.tsx currently uses "365" which conflicts with the server-side policy enforced in certTooler.ts (rejects >200 days); change the defaultValue of the Days input to "200" (or another allowed value ≤200) so the out-of-the-box create flow succeeds without toggling IGNORE_PROTECTIONS, and ensure any accompanying label/placeholder text reflects the new default.
♻️ Duplicate comments (2)
src/proxy.ts (1)
21-23:⚠️ Potential issue | 🔴 CriticalAllowlisting all
/api/paths on the guest host still opens private endpoints.
src/app/api/certs/get_file/[slug]/route.ts:11-28never rejects failed auth/token checks, so Line 22 makes certificate downloads reachable on the guest-resources hostname without a session. Restrict this bypass to the small set of intentionally public routes instead of every/api/*path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy.ts` around lines 21 - 23, The current guest-host bypass in the hostname check (comparing hostname to String(process.env.NEXT_PUBLIC_GUEST_RESOURCES_HOST)) calls NextResponse.next() for any path.startsWith("/auth/") or path.startsWith("/api/"), which unintentionally exposes protected endpoints like the certificate download route (src/app/api/certs/get_file/[slug]/route.ts). Replace the broad path.startsWith("/api/") condition with an explicit allowlist of public API paths (e.g., a const PUBLIC_API_PATHS = [...] and a check that path === or path.startsWith any of those entries) so only intentionally public endpoints are bypassed; keep auth-related behavior narrow and ensure the cert download route (get_file/[slug] handler) remains authenticated by not including it in the allowlist and by preserving its existing auth/token checks.porca/index.ts (1)
37-50:⚠️ Potential issue | 🟠 MajorEnforce the
dayscap in porca too.The only 200-day guard currently lives in
src/components/core/certTooler.ts:40-46./api/signstill accepts any numeric-ishdaysvalue and passes it straight to OpenSSL, so any caller with the proxy token can bypass the policy by talking to porca directly. Parsedaysas an integer here and apply the same max before signing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@porca/index.ts` around lines 37 - 50, Parse the incoming days as an integer, validate it (fallback to a safe default if NaN or <=0), clamp it to the same maximum used by the cert tool (the MAX / maxDays constant used in certTooler), and pass the clamped integer to generateCertificate instead of Number(days); update the code around const { csr, days } and the generateCertificate call to use the validated/clamped days value so porca enforces the same 200-day cap as the certTooler.
🧹 Nitpick comments (3)
porca/Dockerfile (2)
18-19:PORTandHOSTNAMEenvironment variables are set but never read by the application.Per
porca/index.ts(lines 17-19), the server hardcodesport: 4000inBun.serve(). These ENV directives have no effect and may mislead future maintainers.♻️ Either remove unused ENVs or update porca to read them
Option 1: Remove unused ENVs
COPY --from=builder --chown=porca:porca /app/index.js ./index.js EXPOSE 4000 -ENV PORT=4000 -ENV HOSTNAME="0.0.0.0" - USER porcaOption 2: Update porca/index.ts to use them
Bun.serve({ port: Number(process.env.PORT) || 4000, hostname: process.env.HOSTNAME || "0.0.0.0", routes: {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@porca/Dockerfile` around lines 18 - 19, The Dockerfile sets ENV PORT and HOSTNAME but porca/index.ts uses a hardcoded port in Bun.serve, so those ENVs are unused; either remove the ENV lines from the Dockerfile or update porca/index.ts so Bun.serve reads process.env.PORT and process.env.HOSTNAME (convert PORT to Number and fall back to 4000, and fall back to "0.0.0.0" for HOSTNAME) to ensure the environment variables actually control the server startup; search for the Bun.serve call in porca/index.ts to apply the change.
11-13: Consider adding--no-install-recommendsto reduce image size.This is a minor optimization that avoids pulling in recommended but unnecessary packages.
♻️ Proposed change
-RUN apt-get update && apt-get install -y libssl-dev openssl curl && \ +RUN apt-get update && apt-get install -y --no-install-recommends libssl-dev openssl curl && \ groupadd -r porca && useradd -r -g porca -d /app porca && \ - chown -R porca:porca /app + chown -R porca:porca /app && \ + rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@porca/Dockerfile` around lines 11 - 13, Update the Dockerfile RUN step that performs apt-get install (the RUN line installing libssl-dev openssl curl and creating the porca user/group) to add the --no-install-recommends flag to apt-get install so unnecessary recommended packages are not pulled in; adjust the same RUN command to keep it chained as a single layer and optionally follow best practices by cleaning apt caches (e.g., removing /var/lib/apt/lists) in the same RUN to reduce final image size.src/app/certs/groups/[slug]/client.tsx (1)
1-19: Placeholder component with unused imports and props.This component accepts
dbDatabut doesn't render it, and several imports (randomString,Button,Link,useEffect,useState) are unused. The{/*TBD */}comment indicates this is intentionally incomplete.Would you like me to help implement this component, or should this be tracked as a separate issue?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/certs/groups/`[slug]/client.tsx around lines 1 - 19, The Client component currently has unused imports (randomString, Button, Link, useEffect, useState) and accepts dbData (type certificates.$inferSelect) but doesn't render it; either remove the unused imports and keep a minimal placeholder, or implement rendering: inside the Client function (export default function Client) iterate over dbData (or render key fields from certificates.$inferSelect) to produce a list/grid, replace the {/*TBD */} with markup that maps dbData items to JSX, and use the Button/Link imports only if you add actions (e.g., "Edit" or "View" buttons); otherwise delete the unused imports to satisfy linting. Ensure the dbData prop type (certificates.$inferSelect) is referenced where you consume fields so TypeScript types line up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.docker.example:
- Around line 18-19: The example env currently ships a real-looking
PROTECTION_PROXY_TOKEN value which allows trivial deployments to boot with that
credential; remove the hardcoded token in .env.docker.example (leave
PROTECTION_PROXY_TOKEN empty or clearly set to a placeholder like "") and update
init.sh to generate and export a unique secure token at setup time; ensure
porca/index.ts (which only checks that PROTECTION_PROXY_TOKEN is set) will
accept the generated token by having init.sh write it into the runtime env or
.env before services start.
In `@certs/.gitignore`:
- Around line 1-2: The .gitignore currently contains a blanket "*" which
inadvertently ignores the .gitignore itself; restore the unignore pattern by
replacing or updating the entries so that the .gitignore file is not ignored
(reintroduce the "!.gitignore" unignore pattern alongside "*" so the directory
can be tracked); locate this change in the certs/.gitignore file and ensure the
final content includes both "*" and "!.gitignore".
In `@porca/index.ts`:
- Around line 247-250: The call to spawnWithInput("openssl", ["ca", "-config",
configPath, "-revoke", "-batch", "--"], cert) is wrong because OpenSSL -revoke
expects a certificate file path, not PEM on stdin; change the logic in the
revocation flow to write the cert string to a secure temporary file (e.g. using
crypto.randomUUID or similar), call spawnWithInput("openssl", ["ca", "-config",
configPath, "-revoke", "-batch", tempCertPath], "") to pass the temp file path
as the -revoke argument, then regenerate the CRL as before, and finally ensure
the temp file is deleted in a finally/cleanup block to avoid leaking the file
(refer to variables/functions cert, configPath, spawnWithInput).
In `@README.md`:
- Around line 24-25: Update the README section under the heading "Will your
master CA private key get leaked if the next Next.js vuln happens?" in porca to
stop downplaying arbitrary certificate issuance: explicitly state that
unauthorized issuance is a major security incident, list concrete response steps
(audit and enumerate all issued certificates, immediately revoke any
unauthorized certs, rotate the CA/private key if issuance cannot be fully
enumerated, disable issuance endpoints until fixed, and notify stakeholders),
and add guidance to use logging/monitoring to detect future misuse and to run
the auto-update CA script only after verifying a safe rotation process.
In `@src/app/api/certs/route.ts`:
- Around line 17-22: The current flow calls revokeCertificate(publicKey) then
deletes the DB row, which can leave the system inconsistent if the DB delete
fails; modify the logic in route handler to make the operation effectively
atomic by updating the certificate row to a "pending_revocation" state (e.g.,
set status or revocation_pending flag on schema.certificates via db.update)
before calling revokeCertificate(publicKey), then call revokeCertificate and
upon success delete the row (or set status to "revoked") and upon failure clear
the pending flag or record the error for retry; alternatively, perform the DB
delete first inside a DB transaction and only call revokeCertificate after
commit, or implement a compensating retry job that retries revokeCertificate for
rows with revocation_pending—use the functions/references revokeCertificate,
db.delete(schema.certificates), and the certificate row update path in this file
to implement the chosen approach.
- Around line 15-19: The handler reads body.id and constructs certPath directly
which allows path traversal; validate and sanitize body.id (e.g., enforce UUID
v4 regex or strict allowed char set) before using it, reject requests with
invalid ids, then build the path using a safe approach (use only the sanitized
id and a fixed directory/filename pattern such as `${sanitizedId}_pub.pem`
assigned to certPath) and only then call fs.promises.readFile and
revokeCertificate; reference the request body parsing and variables certPath,
body.id, and the revokeCertificate call when making this change.
In `@src/app/certs/client.tsx`:
- Around line 167-174: The hidden CN input (id="CN",
value={easySync.domains[0]}) is marked required and causes the form to be
rejected when easySync.domains is empty; remove the required attribute from the
hidden input and treat CN as a derived value only, then in the form submit
handler (the component's submit function) validate easySync.domains.length > 0
before allowing submission and surface a user-visible error if no domains exist;
ensure the hidden input's value is only set when easySync.domains[0] is defined
so the DOM never contains a non-focusable required control without a value.
In `@src/app/certs/groups/`[slug]/page.tsx:
- Around line 13-18: The page currently accepts props.params.slug and queries
certificates by certificates.id without validating the slug, then renders a 200
"No certificate" view for misses; update the page handler (page.tsx) to first
validate that props.params.slug is a valid UUID and if not call the framework's
not-found response, then continue the
db.select().from(certificates).where(eq(certificates.id, slug)) lookup and if
dbData is empty also return not-found instead of rendering a 200 view—use a UUID
check (regex or a small validator) against props.params.slug and the not-found
helper appropriate to the app.
---
Outside diff comments:
In `@src/app/certs/client.tsx`:
- Around line 158-164: The form default of the Days <Input> in
src/app/certs/client.tsx currently uses "365" which conflicts with the
server-side policy enforced in certTooler.ts (rejects >200 days); change the
defaultValue of the Days input to "200" (or another allowed value ≤200) so the
out-of-the-box create flow succeeds without toggling IGNORE_PROTECTIONS, and
ensure any accompanying label/placeholder text reflects the new default.
---
Duplicate comments:
In `@porca/index.ts`:
- Around line 37-50: Parse the incoming days as an integer, validate it
(fallback to a safe default if NaN or <=0), clamp it to the same maximum used by
the cert tool (the MAX / maxDays constant used in certTooler), and pass the
clamped integer to generateCertificate instead of Number(days); update the code
around const { csr, days } and the generateCertificate call to use the
validated/clamped days value so porca enforces the same 200-day cap as the
certTooler.
In `@src/proxy.ts`:
- Around line 21-23: The current guest-host bypass in the hostname check
(comparing hostname to String(process.env.NEXT_PUBLIC_GUEST_RESOURCES_HOST))
calls NextResponse.next() for any path.startsWith("/auth/") or
path.startsWith("/api/"), which unintentionally exposes protected endpoints like
the certificate download route (src/app/api/certs/get_file/[slug]/route.ts).
Replace the broad path.startsWith("/api/") condition with an explicit allowlist
of public API paths (e.g., a const PUBLIC_API_PATHS = [...] and a check that
path === or path.startsWith any of those entries) so only intentionally public
endpoints are bypassed; keep auth-related behavior narrow and ensure the cert
download route (get_file/[slug] handler) remains authenticated by not including
it in the allowlist and by preserving its existing auth/token checks.
---
Nitpick comments:
In `@porca/Dockerfile`:
- Around line 18-19: The Dockerfile sets ENV PORT and HOSTNAME but
porca/index.ts uses a hardcoded port in Bun.serve, so those ENVs are unused;
either remove the ENV lines from the Dockerfile or update porca/index.ts so
Bun.serve reads process.env.PORT and process.env.HOSTNAME (convert PORT to
Number and fall back to 4000, and fall back to "0.0.0.0" for HOSTNAME) to ensure
the environment variables actually control the server startup; search for the
Bun.serve call in porca/index.ts to apply the change.
- Around line 11-13: Update the Dockerfile RUN step that performs apt-get
install (the RUN line installing libssl-dev openssl curl and creating the porca
user/group) to add the --no-install-recommends flag to apt-get install so
unnecessary recommended packages are not pulled in; adjust the same RUN command
to keep it chained as a single layer and optionally follow best practices by
cleaning apt caches (e.g., removing /var/lib/apt/lists) in the same RUN to
reduce final image size.
In `@src/app/certs/groups/`[slug]/client.tsx:
- Around line 1-19: The Client component currently has unused imports
(randomString, Button, Link, useEffect, useState) and accepts dbData (type
certificates.$inferSelect) but doesn't render it; either remove the unused
imports and keep a minimal placeholder, or implement rendering: inside the
Client function (export default function Client) iterate over dbData (or render
key fields from certificates.$inferSelect) to produce a list/grid, replace the
{/*TBD */} with markup that maps dbData items to JSX, and use the Button/Link
imports only if you add actions (e.g., "Edit" or "View" buttons); otherwise
delete the unused imports to satisfy linting. Ensure the dbData prop type
(certificates.$inferSelect) is referenced where you consume fields so TypeScript
types line up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77ca3e32-3011-461f-8bd2-7d7a45f65eb3
📒 Files selected for processing (25)
.env.docker.example.env.example.github/workflows/build_docker_porca.yaml.gitignoreREADME.mdcerts/.gitignoredocker-compose.ymlinit.shporca/.env.exampleporca/.gitignoreporca/Dockerfileporca/README.mdporca/index.tssrc/app/api/certs/route.tssrc/app/blank_page.tsxsrc/app/certs/client.tsxsrc/app/certs/groups/[slug]/client.tsxsrc/app/certs/groups/[slug]/page.tsxsrc/app/certs/view/[slug]/client.tsxsrc/app/certs/view/[slug]/page.tsxsrc/app/guest-resources/page.tsxsrc/app/navBar.tsxsrc/app/route.tssrc/components/core/certTooler.tssrc/proxy.ts
💤 Files with no reviewable changes (1)
- src/app/guest-resources/page.tsx
✅ Files skipped from review due to trivial changes (5)
- porca/README.md
- .gitignore
- porca/.gitignore
- .github/workflows/build_docker_porca.yaml
- init.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- src/app/navBar.tsx
- docker-compose.yml
Comment on lines
+48
to
+58
| const req = await fetch(`${process.env.PROTECTION_PROXY_URL}/api/sign`, { | ||
| method: "POST", | ||
| headers: { | ||
| "Content-Type": "application/json", | ||
| }, | ||
| body: JSON.stringify({ | ||
| proxyToken: process.env.PROTECTION_PROXY_TOKEN, | ||
| csr: csrText, | ||
| days: generateDays, | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Bound the porca calls with a timeout.
These fetches run inside request handlers (src/app/api/certs/generate/route.ts:1-24 and src/app/api/certs/route.ts:1-29). If porca stalls, the request stays pinned until the platform timeout and can exhaust server concurrency. Add a bounded timeout/abort and surface a clear upstream failure instead of waiting indefinitely.
Also applies to: 81-90
Owner
Author
|
Merge! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores