refactor: replaces server side turnstile config provision with injected config#1353
refactor: replaces server side turnstile config provision with injected config#1353
Conversation
WalkthroughThis update refactors the mechanism for injecting and verifying UI configuration, particularly for Turnstile site key handling and UI tests. It replaces header-based token injection with cryptographically signed configuration using RSA keys. The codebase removes related API endpoints, services, and hooks, updates Playwright test fixtures, and introduces new environment variables and config decoding logic. Associated workflow and environment files are adjusted accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Playwright Test Runner
participant Page as Browser Page
participant App as Next.js App
participant Window as window object
TestRunner->>Page: injectUIConfig()
Note right of Page: Generate signed config with private key
Page->>Window: Set window.__SIGNED_UI_CONFIG
Page->>App: Load application
App->>Window: Read window.__SIGNED_UI_CONFIG
App->>App: decodeInjectedConfig() with public key
App-->>App: If verified, use decoded config
App->>App: Render Turnstile with config
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (21)
💤 Files with no reviewable changes (4)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (15)
⏰ Context from checks skipped due to timeout of 90000ms (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1353 +/- ##
==========================================
+ Coverage 33.90% 33.96% +0.06%
==========================================
Files 798 796 -2
Lines 19415 19418 +3
Branches 3601 3604 +3
==========================================
+ Hits 6582 6595 +13
+ Misses 12245 12234 -11
- Partials 588 589 +1
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
apps/deploy-web/env/.env.sample (1)
53-54: 🛠️ Refactor suggestionUpdate template with new UI config public key variable.
The
.env.sampleonly adds a newline afterUNLEASH_SERVER_API_TOKEN=but doesn’t reflect the newly introducedNEXT_PUBLIC_UI_CONFIG_PUBLIC_KEY. It’s helpful to include a commented placeholder in the sample to guide setup:UNLEASH_SERVER_API_TOKEN= + +# NEXT_PUBLIC_UI_CONFIG_PUBLIC_KEY="-----BEGIN PUBLIC KEY-----\n...your PEM key here...\n-----END PUBLIC KEY-----"apps/deploy-web/src/components/turnstile/Turnstile.tsx (1)
52-99: 🛠️ Refactor suggestionSecurity consideration with fetch interception
The code intercepts fetch calls to handle Turnstile challenges, but the implementation doesn't verify the origin of requests, which could potentially lead to security issues with CSRF attacks.
Consider adding origin verification when intercepting fetch calls:
window.fetch = async (...args) => { let response = await originalFetch(...args); - if (typeof args[0] === "string" && args[0].startsWith("/") && response.status > 400 && turnstileRef.current) { + if (typeof args[0] === "string" && + args[0].startsWith("/") && + response.status > 400 && + turnstileRef.current && + (!document.referrer || new URL(document.referrer).origin === window.location.origin)) { turnstileRef.current?.remove(); turnstileRef.current?.render(); turnstileRef.current?.execute();
🧹 Nitpick comments (10)
apps/deploy-web/env/.env.staging (1)
36-46: Ensure PEM key is loaded correctly in staging.As in production, the multi-line
NEXT_PUBLIC_UI_CONFIG_PUBLIC_KEYmay not parse with default dotenv behavior. Please verify that staging builds and the browser can read this key. If parsing fails, consider using a single-line, escaped\nformat or base64 encoding..github/actions/console-web-ui-testing/action.yml (2)
50-50: Ensure the private key is masked in logs
UI_CONFIG_SIGNATURE_PRIVATE_KEYis exported to the shell environment.
Although GitHub masks secrets, any echo or logging inside the test commands will dump the key verbatim.
For extra safety, add:env: UI_CONFIG_SIGNATURE_PRIVATE_KEY: ${{ secrets.ui-config-signature-private-key }} # Prevent accidental echo ACTIONS_STEP_DEBUG: falseor move the key to a file in
$RUNNER_TEMPand reference it from the test code to avoid accidental exposure.
81-81: Debug statement risks leaking sensitive data
console.dir(pr, { depth: null })prints the full pull-request payload, including labels and potentially secret-scoped data (in private repos).
Consider removing or wrapping it withcore.debugso it only shows up whenACTIONS_STEP_DEBUG=true.apps/deploy-web/src/pages/_app.tsx (3)
58-65: Loss of first paint while waiting for configRendering a full-page Loading… spinner blocks hydration until
decodeInjectedConfigresolves, even though the fallbackbrowserEnvConfigis already available.A less jarring UX is to render immediately with the fallback and update once the promise settles:
- const [isResolvedConfig, setIsResolvedConfig] = useState(false); + const [isResolvedConfig, setIsResolvedConfig] = useState(true); // render immediately- if (!isResolvedConfig) { - return <Loading text="Loading config..." />; - }and keep
enabled/siteKeywrapped in the same fallback logic.
This eliminates an extra paint and noticeably reduces the perceived TTI.
61-65: Swallowing verification errors hides mis-configuration
decodeInjectedConfig()errors are ignored. If signature verification fails you silently fall back to defaults, which masks both security and CI mistakes.useEffect(() => { decodeInjectedConfig() - .then(setConfig) - .finally(() => setIsResolvedConfig(true)); + .then(setConfig) + .catch(err => { + console.error("Injected UI config verification failed:", err); + }) + .finally(() => setIsResolvedConfig(true)); }, []);Surfacing the error (even only in dev / test) will shorten debugging cycles.
73-76: Type-safety: guard againstundefinedbooleans
config?.NEXT_PUBLIC_TURNSTILE_ENABLEDmay beundefined, which results in React passingenabled={undefined}.
IfClientOnlyTurnstileexpects a strict boolean, wrap it:enabled={Boolean(config?.NEXT_PUBLIC_TURNSTILE_ENABLED ?? browserEnvConfig.NEXT_PUBLIC_TURNSTILE_ENABLED)}Avoids React warnings and prevents accidental prop type coercion.
apps/deploy-web/tests/fixture/base-test.ts (2)
34-49: Delimiter collision: JSON may legally contain.The signed payload is concatenated as
<json>.<signature>.
If future config fields include a dot (e.g., version strings"1.2.3"),decodeInjectedConfigmay split at the wrong place.To future-proof:
-const result = `${serializedConfig}.${sigBase64}`; +const result = `${Buffer.from(serializedConfig).toString("base64url")}.${sigBase64}`;Base-64-encoding the JSON removes delimiter ambiguity and slightly shrinks the payload size.
51-63: Node compatibility: rely on a stable Crypto import
crypto.subtleis only globally available from Node 20.
Pinning to it silently breaks CI images pinned to LTS-18.-importPrivateKey +import { webcrypto as crypto } from "node:crypto";or gate with:
const subtle = (globalThis.crypto ?? crypto).subtle;Ensures the tests run on both 18 & 20.
apps/deploy-web/src/config/browser-env.config.ts (2)
44-44: Type definition might cause confusion with existing schema-based typeThere appears to be a potential naming conflict or duplication with another
BrowserEnvConfigtype defined inenv-config.schema.ts(which is derived from a Zod schema). Consider renaming this type or consolidating the types to avoid confusion.-export type BrowserEnvConfig = typeof browserEnvConfig; +export type RuntimeBrowserEnvConfig = typeof browserEnvConfig;
46-70: Secure implementation of config verificationThe implementation correctly uses Web Crypto API with RSASSA-PKCS1-v1_5 and SHA-256 for signature verification. The function properly handles error cases by returning null when verification fails or required components are missing.
However, there's a potential issue with error handling. The function silently returns null without logging any failures, which might make debugging difficult in production.
Consider adding logging for verification failures (especially in non-production environments):
if (!isValidSignature) { + if (process.env.NEXT_PUBLIC_NODE_ENV !== 'production') { + console.warn('Config signature verification failed'); + } return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
.github/actions/console-web-ui-testing/action.yml(3 hunks).github/workflows/console-web-release.yml(1 hunks)apps/deploy-web/env/.env.production(1 hunks)apps/deploy-web/env/.env.sample(1 hunks)apps/deploy-web/env/.env.staging(1 hunks)apps/deploy-web/package.json(1 hunks)apps/deploy-web/playwright.config.ts(1 hunks)apps/deploy-web/src/components/turnstile/Turnstile.tsx(3 hunks)apps/deploy-web/src/config/browser-env.config.ts(1 hunks)apps/deploy-web/src/config/env-config.schema.ts(1 hunks)apps/deploy-web/src/pages/_app.tsx(3 hunks)apps/deploy-web/src/pages/api/config.ts(0 hunks)apps/deploy-web/src/queries/useAppConfig.ts(0 hunks)apps/deploy-web/src/services/config/config.service.ts(0 hunks)apps/deploy-web/src/services/http-factory/http-factory.service.ts(0 hunks)apps/deploy-web/tests/fixture/base-test.ts(1 hunks)apps/deploy-web/tests/fixture/context-with-extension.ts(3 hunks)apps/deploy-web/tests/fixture/test-env.config.ts(1 hunks)apps/deploy-web/tests/pages/DeployBasePage.tsx(1 hunks)
💤 Files with no reviewable changes (4)
- apps/deploy-web/src/services/http-factory/http-factory.service.ts
- apps/deploy-web/src/pages/api/config.ts
- apps/deploy-web/src/queries/useAppConfig.ts
- apps/deploy-web/src/services/config/config.service.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/deploy-web/tests/fixture/context-with-extension.ts (1)
apps/deploy-web/tests/fixture/base-test.ts (1)
injectUIConfig(16-30)
apps/deploy-web/src/config/browser-env.config.ts (1)
apps/deploy-web/src/config/env-config.schema.ts (1)
BrowserEnvConfig(69-69)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test-deploy-web-build
🔇 Additional comments (13)
apps/deploy-web/env/.env.production (1)
37-48:Details
❓ Verification inconclusive
Verify multi-line env var parsing for Next.js.
You’ve inlined a PEM-encoded public key across multiple lines inside quotes. Standard dotenv parsers may not support literal newlines, which could cause build/runtime errors. Please confirm that Next.js (or
@akashnetwork/env-loader) correctly loads this format. If not, consider one of the following:
- Base64-encode the key and decode at runtime.
- Serialize newlines (
\n) inside a single-line string.
Verify multi-line environment variable support
It looks like you’ve inlined a PEM-encoded public key across multiple lines inside the
.env.productionfile. The default Next.js loader (viadotenv/dotenv-expand) does not reliably parse literal newlines inside quoted values, which can lead to missing or malformed keys at build/runtime. Please confirm that your setup (Next.js or any custom@akashnetwork/env-loader) accepts this format. If it doesn’t, consider one of the following:
- Base64-encode the entire key and decode it at runtime.
- Serialize the newlines as
\nin a single-line string, for example:NEXT_PUBLIC_UI_CONFIG_PUBLIC_KEY="-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0B…IDAQAB\n-----END PUBLIC KEY-----"apps/deploy-web/package.json (1)
143-143: Playwright version bump approved.Bumping
@playwright/testfrom^1.45.0to^1.52.0aligns with the updated test fixtures and config changes.apps/deploy-web/tests/pages/DeployBasePage.tsx (1)
4-4: Import path correction looks good.Switching from an absolute to a relative import for
testEnvConfig("../fixture/test-env.config") aligns with the project structure and updated test-fixture layout.apps/deploy-web/tests/fixture/context-with-extension.ts (2)
8-8: Update to the test fixture authentication mechanismThe import change from
addUITestsTokentoinjectUIConfigaligns with the PR's goal of improving performance by replacing server-side token authentication with client-side injected configuration.
65-65: Function call updated to use signed config injectionThis change replaces the token-based authentication with cryptographically signed configuration injection, which is more secure and efficient. The new approach avoids the performance issues associated with page route interception.
.github/workflows/console-web-release.yml (1)
54-54: GitHub workflow updated to use private key for config signingThe workflow now uses a private key for cryptographically signing the UI configuration instead of a token. This is a more secure approach and supports the broader change in authentication strategy.
apps/deploy-web/src/config/env-config.schema.ts (1)
66-66: Removed environment variables as part of server-side config refactoringThe removal of
TURNSTILE_TEST_SITE_KEYandUI_TESTS_TOKENfrom the server environment schema is appropriate since the server-side config endpoints and related hooks have been removed in favor of client-side injected configuration.apps/deploy-web/playwright.config.ts (2)
22-22: Reduced test timeout for faster feedbackReducing the test timeout from 60 to 30 seconds will provide faster feedback when tests fail, which is appropriate if tests are expected to complete within this timeframe.
30-32: Added video recording for failed testsAdding video capture for test failures will improve debugging capabilities without adding overhead to successful test runs. This enhancement to test diagnostics complements the existing trace collection functionality.
apps/deploy-web/src/components/turnstile/Turnstile.tsx (4)
41-44: Explicit props improve component reliabilityMaking the
enabledandsiteKeyprops explicit requirements ensures the component has the necessary configuration at render time, avoiding potential runtime errors.
46-46: Component no longer relies on fetched configurationThe refactored component now takes configuration directly through props, which aligns with the PR objective of improving performance by eliminating server-side config fetching.
111-113: Early return pattern improves readabilityThe early return pattern when the component is disabled is a good practice that simplifies the component logic.
179-179: Dynamic import improves initial load performanceUsing Next.js dynamic import with
ssr: falseensures the Turnstile component only loads client-side, which helps with initial page load performance.
stalniy
force-pushed
the
refactor/ui-config-for-testing
branch
2 times, most recently
from
b4e6331 to
6cd3201
Compare
stalniy
force-pushed
the
refactor/ui-config-for-testing
branch
from
6cd3201 to
520ae7d
Compare
3 participants
Why
Because the approach with page route interception is too slow. It currently adds +30secs to page load time (the first paint happens in 30secs after navigation). Probably due to big amount of external requests.
What
Provides config via injected global variable with signed signature to ensure that nobody else except of us can inject config and overwrite turnstile configuration.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores