Repository navigation
feat(self-hosted): do not require openssl or node for new auth - #45941
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughBoth key-generation utility scripts now dynamically select Node.js execution: preferring a locally-installed ChangesKey generation utilities Node.js execution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker/utils/add-new-auth-keys.sh`:
- Around line 60-63: The script currently reads the JWT secret from
process.argv[1] (see the node runner invocation and the jwtSecret variable),
which exposes the secret on the process command line; change the Node script to
read the secret from stdin instead (e.g., read process.stdin until EOF and
assign to jwtSecret) and update the docker/node runner invocation to pass the
secret via stdin (enable -i on the runner) so the secret is never passed as a
command-line argument; apply the same change to the other occurrence around the
second instance (line ~134) that also uses process.argv.
- Around line 22-37: Update the runtime selection to verify the local node
version (require >=14.18.0 or your preferred minimum, e.g., >=16) before
choosing "node" so older local Node will not be used; if the version check
fails, fall back to the containerized runner by setting node_runner to the
existing "docker run --rm node:22-alpine node" path (this affects the
node_runner assignment logic in the shown block and should be mirrored in
rotate-new-api-keys.sh). Also stop passing JWT_SECRET as a positional argument
to the Node script (the invocation that currently supplies JWT_SECRET on
process.argv); instead provide the secret via a safe channel such as an
environment variable (export into the shell and use process.env.JWT_SECRET in
the Node script, or pass it into the docker run with --env and read
process.env.JWT_SECRET) or via stdin, and remove the JWT_SECRET positional
argument so it no longer appears in the process list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 94bb4bdf-c986-4434-a261-2a8f08431e30
📒 Files selected for processing (2)
docker/utils/add-new-auth-keys.shdocker/utils/rotate-new-api-keys.sh
| if ! command -v openssl >/dev/null 2>&1; then | ||
| echo "Error: openssl is required but not found." | ||
| exit 1 | ||
| # Resolve how to run node: local install (>= 16) preferred, docker fallback. |
There was a problem hiding this comment.
FYI Node.js oldest supported version is now v22.
Even v20 went EOL 2 weeks ago.
There was a problem hiding this comment.
Well, can't enforce people with already existing node to update, though :) The docker path uses node:22-alpine, yes.
Braintrust eval report
|
I have read the CONTRIBUTING.md file.
YES
What kind of change does this PR introduce?
Drop node and openssl pre-requisites for self-hosted new auth / new api key utility scripts:
opensslrequirement. Moved EC P-256 keypair generation into the Node script viacrypto.generateKeyPairSync("ec", { namedCurve: "P-256" })nodeoptional. Both scripts now auto-detect: localnodeif available, else fall back todocker run --rm node:22-alpine nodePulling node:22-alpine (first-run only)...and pre-pull the imageadd-new-auth-keys.shnow strips\rwhen readingJWT_SECRETfrom.envfor better compat with Windows-environmentsCloses #45802
Summary by CodeRabbit
Summary by CodeRabbit
Bug Fixes
Chores