chore: run dist with environment variables kyb-app #2645
Conversation
|
WalkthroughThe changes in this pull request involve modifications to the Dockerfile, the addition of an Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
pratapalakshmi
force-pushed
the
chore/run/time/dist/kyb-app
branch
from
33eb823 to
9a7d109
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- apps/kyb-app/Dockerfile (1 hunks)
- apps/kyb-app/entrypoint.sh (1 hunks)
- apps/kyb-app/global.d.ts (1 hunks)
- apps/kyb-app/index.html (1 hunks)
- apps/kyb-app/public/config.js (1 hunks)
- apps/kyb-app/src/common/utils/request.ts (1 hunks)
- apps/kyb-app/src/main.tsx (2 hunks)
Files skipped from review due to trivial changes (1)
- apps/kyb-app/global.d.ts
Additional context used
Shellcheck
apps/kyb-app/entrypoint.sh
[warning] 3-3: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 8-8: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 13-13: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 19-19: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 24-24: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 29-29: In POSIX sh, [[ ]] is undefined.
(SC3010)
Additional comments not posted (7)
apps/kyb-app/index.html (1)
8-8: Script Tag Added for ConfigurationThe addition of the
<script type="text/javascript" src="/config.js"></script>tag is approved as it ensures the global configuration is loaded early in the application lifecycle. However, ensure that no sensitive information is exposed through this script to enhance security.Consider verifying the contents of
config.jsto ensure no sensitive data is exposed to the client-side.apps/kyb-app/Dockerfile (1)
23-36: Dockerfile Enhancements for Better Structure and FunctionalityThe modifications to the Dockerfile are approved. These include:
- Setting the working directory to
/appwhich helps in standardizing the execution context.- Adding an entrypoint script which allows for custom initialization logic.
- Making the script executable to avoid permission issues.
These changes enhance the Docker container's structure and functionality, facilitating smoother deployment. Ensure that the entrypoint script (
entrypoint.sh) is thoroughly tested to confirm it performs as expected.Consider adding automated tests or manual steps to verify the functionality of the
entrypoint.shscript.apps/kyb-app/entrypoint.sh (2)
34-43: Review the configuration file generation logic.The script writes environment variables to a JavaScript configuration file. Ensure that the file path and variable names are correctly specified and that they match the application's requirements.
Verify the correctness and security implications of writing environment variables directly to a JavaScript file accessible via the web server.
3-6: Replace[[ ]]with POSIX-compliant syntax and verify default values.The use of
[[ ]]for condition checks is not POSIX compliant. Replace it with[ ]to ensure compatibility across different shell environments. Additionally, verify that the default values set for the environment variables are appropriate for the application's needs.Apply this diff to correct the syntax:
- if [[ -z "$VITE_DOMAIN" ]] + if [ -z "$VITE_DOMAIN" ] - if [[ -z "$VITE_API_KEY" ]] + if [ -z "$VITE_API_KEY" ] - if [[ -z "$VITE_ENVIRONMENT_NAME" ]] + if [ -z "$VITE_ENVIRONMENT_NAME" ] - if [[ -z "$VITE_DEFAULT_EXAMPLE_TOKEN" ]] + if [ -z "$VITE_DEFAULT_EXAMPLE_TOKEN" ] - if [[ -z "$VITE_SENTRY_AUTH_TOKEN" ]] + if [ -z "$VITE_SENTRY_AUTH_TOKEN" ] - if [[ -z "$VITE_SENTRY_DSN" ]] + if [ -z "$VITE_SENTRY_DSN" ]Also applies to: 8-11, 13-16, 19-22, 24-27, 29-32
Tools
Shellcheck
[warning] 3-3: In POSIX sh, [[ ]] is undefined.
(SC3010)
apps/kyb-app/src/main.tsx (2)
17-17: Review the import of the configuration file.Ensure that the import statement for the configuration file is correct and that the file is properly loaded into the application. This is crucial for the application's configuration management.
36-39: Address TypeScript type-checking issues.The use of
//@ts-ignoreto bypass TypeScript type-checking could lead to runtime errors. Ensure that the types are correctly defined and that any potential issues are resolved before bypassing type-checking.apps/kyb-app/src/common/utils/request.ts (1)
6-7: Review the modification of theprefixUrlproperty.The change to use
globalThis.env.VITE_API_URLfor theprefixUrlis a significant shift that should be carefully reviewed to ensure it aligns with the application's architecture and security requirements.
apps/kyb-app/public/config.js
Outdated
| globalThis.env = { | ||
| VITE_API_URL: 'http://google.com', | ||
| VITE_KYB_DEFINITION_ID: "kyb_parent_kyc_session_example", | ||
| VITE_API_KEY: "secret", | ||
| VITE_ENVIRONMENT_NAME: "local", | ||
| VITE_DEFAULT_EXAMPLE_TOKEN: 12345678-1234-1234-1234-123456789012, | ||
| VITE_SENTRY_AUTH_TOKEN: "", | ||
| VITE_SENTRY_DSN: "", | ||
| }; |
There was a problem hiding this comment.
Security and Syntax Concerns in Global Configuration
The configuration file introduces several potential issues:
- Security Risk: Hardcoding sensitive information like API keys (
VITE_API_KEY) and tokens (VITE_DEFAULT_EXAMPLE_TOKEN) in a public JavaScript file poses a significant security risk. Consider fetching these values securely from environment variables or a secure vault at runtime. - Syntax Error: The numeric token (
VITE_DEFAULT_EXAMPLE_TOKEN) should be enclosed in quotes to prevent syntax errors and ensure it is treated as a string.
Consider the following changes to address these issues:
- VITE_API_KEY: "secret",
- VITE_DEFAULT_EXAMPLE_TOKEN: 12345678-1234-1234-1234-123456789012,
+ VITE_API_KEY: process.env.VITE_API_KEY,
+ VITE_DEFAULT_EXAMPLE_TOKEN: process.env.VITE_DEFAULT_EXAMPLE_TOKEN,Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
should use default value if preset, if case someone set his value while running the docker container so you should consider the provided value:
globalThis.env = {
VITE_API_URL: import.meta.env.VITE_API_URL || 'http://google.com',
VITE_KYB_DEFINITION_ID: import.meta.env.VITE_KYB_DEFINITION_ID || 'kyb_parent_kyc_session_example',
VITE_API_KEY: import.meta.env.VITE_API_KEY || 'secret',
VITE_ENVIRONMENT_NAME: import.meta.env.VITE_ENVIRONMENT_NAME || 'local',
VITE_DEFAULT_EXAMPLE_TOKEN: import.meta.env.VITE_DEFAULT_EXAMPLE_TOKEN || '12345678-1234-1234-1234-123456789012',
VITE_SENTRY_AUTH_TOKEN: import.meta.env.VITE_SENTRY_AUTH_TOKEN || '',
VITE_SENTRY_DSN: import.meta.env.VITE_SENTRY_DSN || '',
};
There was a problem hiding this comment.
@liorzam, thank you for the clarification.
Using import.meta.env.VAR_NAME || 'default value' is a good approach for setting default values while allowing overrides. However, please note that import.meta.env is only available at build time in Vite and may not be accessible in the config.js file at runtime when it's loaded in the browser.
Since config.js is served as a static file and loaded at runtime, accessing import.meta.env in this context may not work as expected. An alternative is to use the entrypoint.sh script to inject the environment variables into config.js at runtime, which seems to be the approach in this PR.
Additionally, we should be cautious about exposing sensitive information like API keys and tokens in frontend code, as they can be accessed by anyone using the application.
Let me know your thoughts on this.
(*￣-￣)
Learnings added
Learnt from: liorzam
PR: ballerine-io/ballerine#2645
File: apps/kyb-app/public/config.js:1-9
Timestamp: 2024-09-23T07:47:38.881Z
Learning: In `config.js`, avoid using `import.meta.env` as it is not available at runtime; instead, use environment variables injected via the `entrypoint.sh` script.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @@ -3,7 +3,8 @@ import * as Sentry from '@sentry/react'; | |||
| import ky, { HTTPError } from 'ky'; | |||
|
|
|||
| export const request = ky.create({ | |||
| prefixUrl: import.meta.env.VITE_API_URL || `${window.location.origin}/api/v1/`, | |||
| //@ts-ignore | |||
| prefixUrl: globalThis.env.VITE_API_URL, | |||
There was a problem hiding this comment.
as you see we used this import meta env which takes the value from env var
| @@ -5,6 +5,7 @@ | |||
| <link rel="icon" type="image/svg+xml" href="/vite.svg" /> | |||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | |||
| <title>KYB - Collection Flow</title> | |||
| <script type="text/javascript" src="/config.js"></script> | |||
There was a problem hiding this comment.
No in the dist index.html and config.js stay in the same directory
pratapalakshmi
force-pushed
the
chore/run/time/dist/kyb-app
branch
from
a9dde22 to
10936ef
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (6)
apps/kyb-app/public/config.js (2)
1-1: Consider a more secure configuration approachExposing configuration through
globalThisin a public JavaScript file makes it accessible to anyone, potentially exposing sensitive information. Consider implementing a secure configuration service that provides only necessary, non-sensitive values to the frontend.
9-11: Improve error tracking configurationWhile empty defaults for Sentry configuration are acceptable, consider:
- Adding validation to ensure Sentry is properly initialized when credentials are provided
- Implementing a fallback error handling mechanism when Sentry is not configured
// Example validation in your app initialization if (globalThis.env.VITE_SENTRY_DSN) { // Initialize Sentry } else { // Initialize fallback error handling console.warn('Sentry not configured, using fallback error handling'); }apps/kyb-app/Dockerfile (1)
27-32: Consider relocating entrypoint.shConsider moving the entrypoint script to a dedicated
dockerorscriptsdirectory for better organization and maintainability.-COPY --from=dev /app/entrypoint.sh /app/entrypoint.sh +COPY --from=dev /app/docker/entrypoint.sh /app/entrypoint.shapps/kyb-app/entrypoint.sh (1)
45-46: Add command validation and error handlingThe script executes commands without validation or error handling.
Consider adding basic safeguards:
+# Ensure we have a command to execute +if [ $# -eq 0 ]; then + echo "Error: No command provided" + exit 1 +fi + # Handle CMD command exec "$@"apps/kyb-app/src/main.tsx (1)
17-17: LGTM! Consider adding a commentThe config import is correctly set up for runtime configuration loading.
Consider adding a comment explaining the purpose of the ?url suffix:
+// Import runtime configuration using Vite's ?url suffix import '../public/config.js?url';apps/kyb-app/src/common/utils/request.ts (1)
6-7: Consider implementing a configuration service pattern.While using
globalThis.envworks, consider implementing a more robust configuration service pattern that:
- Provides type safety
- Includes validation
- Centralizes configuration management
- Supports different environments
Example configuration service:
// config.service.ts class ConfigService { private static instance: ConfigService; private config: Record<string, string>; private constructor() { this.config = { apiUrl: globalThis.env.VITE_API_URL, // other configs }; this.validate(); } static getInstance(): ConfigService { if (!ConfigService.instance) { ConfigService.instance = new ConfigService(); } return ConfigService.instance; } private validate() { if (!this.config.apiUrl) { throw new Error('API URL is required'); } // Add other validation } get apiUrl(): string { return this.config.apiUrl; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
apps/kyb-app/Dockerfile(1 hunks)apps/kyb-app/entrypoint.sh(1 hunks)apps/kyb-app/global.d.ts(1 hunks)apps/kyb-app/index.html(1 hunks)apps/kyb-app/public/config.js(1 hunks)apps/kyb-app/src/common/utils/request.ts(1 hunks)apps/kyb-app/src/main.tsx(2 hunks)
🧰 Additional context used
🪛 Shellcheck
apps/kyb-app/entrypoint.sh
[warning] 3-3: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 8-8: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 13-13: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 19-19: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 24-24: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 29-29: In POSIX sh, [[ ]] is undefined.
(SC3010)
🪛 Biome
apps/kyb-app/src/main.tsx
[error] 16-16: Shouldn't redeclare 'initializeMonitoring'. Consider to delete it or rename it.
'initializeMonitoring' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (8)
apps/kyb-app/public/config.js (1)
2-6:
Runtime and security issues in configuration
import.meta.envis not available at runtime in the browser. This configuration should be injected by theentrypoint.shscript instead.- Default values for API URL and API key in public code pose security risks.
Apply this approach in the entrypoint script instead:
globalThis.env = {
VITE_API_URL: '{{VITE_API_URL}}',
VITE_KYB_DEFINITION_ID: '{{VITE_KYB_DEFINITION_ID}}',
VITE_API_KEY: '{{VITE_API_KEY}}',
VITE_ENVIRONMENT_NAME: '{{VITE_ENVIRONMENT_NAME}}',
// ...
};apps/kyb-app/index.html (1)
10-10: LGTM! Verify path resolution in production.
The absolute path /config.js is correct assuming both files are served from the same directory in the production build.
Let's verify the file locations in the production build:
✅ Verification successful
Path resolution is correct for production deployment
The verification confirms that the absolute path /config.js is correct because:
- The
entrypoint.shscript generatesconfig.jsat/usr/share/nginx/html/config.js - The production Dockerfile (
FROM nginx:stable-alpine as prod) copies the built app to the same directory/usr/share/nginx/html - Both files are served from the same nginx root directory, making the absolute path
/config.jsresolve correctly
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if config.js exists in the expected location relative to index.html
# Expected: Both files should be in the same directory in the production build
# Find the production build directory
fd -t f "index.html" dist
Length of output: 117
Script:
#!/bin/bash
# Let's check the build configuration and deployment setup to understand the file locations
# Check package.json for build configuration
fd -t f "package.json" apps/kyb-app --exec cat {}
# Check for any build or deployment scripts
fd -t f "entrypoint.sh|Dockerfile" apps/kyb-app --exec cat {}
# Look for vite.config or similar build configs
fd -t f "vite.config" apps/kyb-app --exec cat {}
Length of output: 6608
apps/kyb-app/Dockerfile (5)
12-12: LGTM: Build setup and PATH configuration
The build step is correctly placed after environment setup, and the PATH variable now uses an absolute path which is more reliable in a container context.
Also applies to: 15-15
23-24: LGTM: Consistent working directory
Setting WORKDIR to /app in production stage maintains consistency with development stage.
35-36: LGTM: Container initialization setup
The ENTRYPOINT and CMD configuration properly separates initialization (environment setup) from the main process (nginx).
27-28: Verify entrypoint.sh existence and permissions
The entrypoint script is crucial for environment configuration. Let's verify its presence and content.
#!/bin/bash
# Check if entrypoint.sh exists and contains necessary environment setup
if [ -f "apps/kyb-app/entrypoint.sh" ]; then
echo "Entrypoint script found"
cat "apps/kyb-app/entrypoint.sh"
else
echo "Warning: entrypoint.sh not found in expected location"
fiAlso applies to: 31-32
19-19: Verify the security implications of --host flag
The --host flag typically allows external connections to the development server. While this might be necessary for container networking, ensure this is intentional and that appropriate network security measures are in place.
✅ Verification successful
Let me gather more information about the development server configuration and network settings.
Let me check the content of the Vite configuration file to understand the server settings.
Let me check if there are any Docker Compose or network configuration files that might provide additional context about the container's network security.
Based on the gathered information, I can now provide the final response:
The --host flag usage is appropriate in this Docker context
The --host flag is necessary here as it allows the Vite development server to accept connections from outside the container, which is required for Docker container networking. The security implications are mitigated by:
- The server is configured to run on a specific port (5201) as seen in vite.config.ts
- The container's network exposure is controlled by Docker's networking layer
- Environment variables and configuration are properly managed through the entrypoint script
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any security measures in place (like reverse proxy configs or firewall rules)
rg -l "proxy_pass|location|allow|deny" example.nginx.conf
Length of output: 1004
Script:
#!/bin/bash
# Check Vite configuration and network settings
rg -l "server\s*:|host\s*:|port\s*:" apps/kyb-app/
Length of output: 197
Script:
#!/bin/bash
# Check the Vite configuration file content
cat apps/kyb-app/vite.config.ts
Length of output: 1929
Script:
#!/bin/bash
# Check for Docker Compose files and network configurations
fd -e yml -e yaml . apps/kyb-app/ | xargs cat
Length of output: 451
apps/kyb-app/src/common/utils/request.ts (1)
6-7: 🛠️ Refactor suggestion
Consider addressing type safety instead of using @ts-ignore.
The @ts-ignore comment suppresses TypeScript errors but doesn't address the underlying type safety concern. Consider properly typing the globalThis.env object to maintain type safety across the application.
Create a type declaration file (e.g., types/global.d.ts):
declare global {
var env: {
VITE_API_URL: string;
// Add other environment variables as needed
};
}This will provide proper type checking and autocompletion for the global environment variables.
Let's verify the configuration setup:
✅ Verification successful
Using globalThis.env is the correct approach here
The verification shows that the codebase has a proper runtime configuration setup:
- The
entrypoint.shscript creates a/usr/share/nginx/html/config.jsfile that correctly initializesglobalThis.envwith environment variables, includingVITE_API_URL. - This approach is part of a broader configuration strategy where runtime values are injected via the entrypoint script, allowing for dynamic configuration in containerized environments.
- The
@ts-ignoreis warranted here since the type information for the runtime-injectedglobalThis.envisn't available during build time.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the configuration setup for VITE_API_URL
# Test 1: Check if config.js exists and contains VITE_API_URL
echo "Checking for config.js file..."
fd -t f "config.js" -x cat {} \; | grep -A 1 "VITE_API_URL"
# Test 2: Check for other potential usages of VITE_API_URL
echo "Checking for other VITE_API_URL usages..."
rg "VITE_API_URL"
# Test 3: Check for entrypoint script that sets up the config
echo "Checking for entrypoint script..."
fd "entrypoint.sh" -x cat {}
Length of output: 6991
| declare global { | ||
| export var env: { [key: string]: any }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving type safety and documentation
While the implementation works, there are several improvements that could make it more robust and maintainable:
- Replace
anywith a specific interface for environment variables - Add readonly protection to prevent accidental mutations
- Include JSDoc documentation
Consider this improved implementation:
+/**
+ * Global environment configuration interface.
+ * Initialized at runtime via config.js and provides access to environment variables.
+ * @example
+ * // Access API URL
+ * globalThis.env.VITE_API_URL
+ */
declare global {
- export var env: { [key: string]: any };
+ export interface EnvConfig {
+ VITE_API_URL: string;
+ // Add other expected environment variables with their specific types
+ }
+
+ export var env: Readonly<EnvConfig>;
}Committable suggestion skipped: line range outside the PR's diff.
💡 Codebase verification
Environment variable access patterns need standardization
The codebase shows inconsistent patterns for accessing environment variables:
process.envis used in vite.config.ts (4 occurrences)import.meta.envis used extensively across the application (17 occurrences)globalThis.envis used in a few places (4 occurrences)
Key issues:
- The global declaration in
global.d.tssuggests usingglobalThis.env, but most of the codebase usesimport.meta.env process.envusage in vite.config.ts is correct as it's a Node.js context- Environment variables are initialized in multiple places (entrypoint.sh, config.js, main.tsx)
🔗 Analysis chain
Verify environment variable usage across the application
Let's ensure all environment variable accesses are consistent with this global declaration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for environment variable access patterns
echo "Checking direct process.env usage (should be replaced with globalThis.env):"
rg "process\.env\." apps/kyb-app/
echo -e "\nChecking import.meta.env usage (should be used only during initialization):"
rg "import\.meta\.env" apps/kyb-app/
echo -e "\nChecking globalThis.env usage (preferred pattern):"
rg "globalThis\.env" apps/kyb-app/
Length of output: 3090
| VITE_DEFAULT_EXAMPLE_TOKEN: | ||
| import.meta.env.VITE_DEFAULT_EXAMPLE_TOKEN || '12345678-1234-1234-1234-123456789012', |
There was a problem hiding this comment.
Remove default authentication token
The default example token should not be included in the source code, even as a fallback. This could lead to unintended authentication if the environment variable is not properly set.
- VITE_DEFAULT_EXAMPLE_TOKEN:
- import.meta.env.VITE_DEFAULT_EXAMPLE_TOKEN || '12345678-1234-1234-1234-123456789012',
+ VITE_DEFAULT_EXAMPLE_TOKEN: '{{VITE_DEFAULT_EXAMPLE_TOKEN}}',Committable suggestion skipped: line range outside the PR's diff.
| @@ -7,6 +7,7 @@ | |||
| <link rel="icon" type="image/png" sizes="16x16" href="/favicon-16x16.png" /> | |||
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | |||
| <title>KYB - Collection Flow</title> | |||
| <script type="text/javascript" src="/config.js"></script> | |||
There was a problem hiding this comment.
💡 Codebase verification
Security: Environment variables exposed in public config.js require attention
The following sensitive variables are being exposed in the publicly accessible config.js:
VITE_API_KEY: Default value "secret" is exposed and can be used for API accessVITE_SENTRY_AUTH_TOKEN: Though empty by default, exposing auth tokens is riskyVITE_DEFAULT_EXAMPLE_TOKEN: A static UUID token is exposed
Recommendations:
- Move sensitive API keys and tokens to server-side only
- Use session-based authentication instead of exposing API keys
- If client-side config is needed, expose only public, non-sensitive variables
🔗 Analysis chain
Security: Review environment variable exposure.
Since config.js is publicly accessible and contains environment variables, ensure that no sensitive information (API keys, secrets, etc.) is exposed through globalThis.env.
Let's check what environment variables are being exposed:
Let me gather more specific information about the environment variables being exposed in the config.js file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential sensitive environment variables in the configuration
# Expected: Only safe, public variables should be exposed
# Search for environment variable assignments in entrypoint.sh
rg -A 5 "globalThis.env = {"
# Look for potentially sensitive patterns in environment variable names
rg -i "key|token|secret|password|credential" apps/kyb-app/
Length of output: 15749
Script:
#!/bin/bash
# Check the actual content of config.js to see what variables are exposed
cat apps/kyb-app/public/config.js
# Also check entrypoint.sh to see how the config is generated
cat apps/kyb-app/entrypoint.sh
Length of output: 1523
| if [[ -z "$VITE_API_KEY" ]] | ||
| then | ||
| VITE_API_KEY="secret" | ||
| fi |
There was a problem hiding this comment.
Security: Avoid hardcoding default API key
Using a hardcoded default API key ("secret") could be dangerous if accidentally used in non-local environments.
Consider:
- Making the API key required (fail if not provided)
- Or generating a random default value:
- VITE_API_KEY="secret"
+ VITE_API_KEY=$(openssl rand -hex 16)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ -z "$VITE_API_KEY" ]] | |
| then | |
| VITE_API_KEY="secret" | |
| fi | |
| if [[ -z "$VITE_API_KEY" ]] | |
| then | |
| VITE_API_KEY=$(openssl rand -hex 16) | |
| fi |
🧰 Tools
🪛 Shellcheck
[warning] 8-8: In POSIX sh, [[ ]] is undefined.
(SC3010)
| #!/usr/bin/env sh | ||
|
|
There was a problem hiding this comment.
Fix shell compatibility issues
The script uses #!/usr/bin/env sh but contains bash-specific [[ syntax. This will fail in containers with only POSIX sh.
Either:
- Change the shebang to use bash:
-#!/usr/bin/env sh
+#!/usr/bin/env bash- Or use POSIX-compatible syntax:
-if [[ -z "$VITE_DOMAIN" ]]
+if [ -z "$VITE_DOMAIN" ]Committable suggestion skipped: line range outside the PR's diff.
| cat << EOF > /usr/share/nginx/html/config.js | ||
| globalThis.env = { | ||
| VITE_API_URL: "$VITE_DOMAIN/api/v1/", | ||
| VITE_API_KEY: "$VITE_API_KEY", | ||
| VITE_ENVIRONMENT_NAME: "$VITE_ENVIRONMENT_NAME", | ||
| VITE_DEFAULT_EXAMPLE_TOKEN: "$VITE_DEFAULT_EXAMPLE_TOKEN", | ||
| VITE_SENTRY_AUTH_TOKEN: "$VITE_SENTRY_AUTH_TOKEN", | ||
| VITE_SENTRY_DSN: "$VITE_SENTRY_DSN", | ||
| } | ||
| EOF |
There was a problem hiding this comment.
Security: Reconsider config file location and format
Writing configuration with sensitive values to /usr/share/nginx/html/ makes it publicly accessible through the web server.
Consider:
- Moving config to a non-public directory and importing it server-side
- Using environment variables directly in your application
- If client-side config is necessary, exclude sensitive values and add security headers:
cat << EOF > /usr/share/nginx/html/config.js
globalThis.env = {
VITE_API_URL: "$VITE_DOMAIN/api/v1/",
- VITE_API_KEY: "$VITE_API_KEY",
VITE_ENVIRONMENT_NAME: "$VITE_ENVIRONMENT_NAME",
- VITE_DEFAULT_EXAMPLE_TOKEN: "$VITE_DEFAULT_EXAMPLE_TOKEN",
- VITE_SENTRY_AUTH_TOKEN: "$VITE_SENTRY_AUTH_TOKEN",
VITE_SENTRY_DSN: "$VITE_SENTRY_DSN",
}
EOF
+
+# Add security headers to nginx config
+cat << EOF > /etc/nginx/conf.d/security-headers.conf
+add_header Content-Security-Policy "default-src 'self';" always;
+add_header X-Content-Type-Options "nosniff" always;
+EOFCommittable suggestion skipped: line range outside the PR's diff.
| if [[ -z "$VITE_DOMAIN" ]] | ||
| then | ||
| VITE_DOMAIN="http://localhost:3000" | ||
| fi | ||
|
|
||
| if [[ -z "$VITE_API_KEY" ]] | ||
| then | ||
| VITE_API_KEY="secret" | ||
| fi | ||
|
|
||
| if [[ -z "$VITE_ENVIRONMENT_NAME" ]] | ||
| then | ||
| VITE_ENVIRONMENT_NAME=local | ||
| fi | ||
|
|
||
|
|
||
| if [[ -z "$VITE_DEFAULT_EXAMPLE_TOKEN" ]] | ||
| then | ||
| VITE_DEFAULT_EXAMPLE_TOKEN=12345678-1234-1234-1234-123456789012 | ||
| fi | ||
|
|
||
| if [[ -z "$VITE_SENTRY_AUTH_TOKEN" ]] | ||
| then | ||
| VITE_SENTRY_AUTH_TOKEN="" | ||
| fi | ||
|
|
||
| if [[ -z "$VITE_SENTRY_DSN" ]] | ||
| then | ||
| VITE_SENTRY_DSN="" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation and logging for environment variables
The script silently falls back to defaults without any validation or logging, which could mask configuration issues.
Consider adding:
- Validation for provided values
- Logging of which defaults are being used
+log_default() {
+ echo "Warning: $1 not set, using default value: $2"
+}
if [ -z "$VITE_DOMAIN" ]
then
+ log_default "VITE_DOMAIN" "http://localhost:3000"
VITE_DOMAIN="http://localhost:3000"
+elif ! echo "$VITE_DOMAIN" | grep -qE '^https?://'
+then
+ echo "Error: VITE_DOMAIN must start with http:// or https://"
+ exit 1
fiCommittable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Shellcheck
[warning] 3-3: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 8-8: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 13-13: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 19-19: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 24-24: In POSIX sh, [[ ]] is undefined.
(SC3010)
[warning] 29-29: In POSIX sh, [[ ]] is undefined.
(SC3010)
apps/kyb-app/src/main.tsx
Outdated
| @@ -13,6 +13,8 @@ import { App } from './App'; | |||
| import { Head } from './Head'; | |||
| import './i18next'; | |||
| import './index.css'; | |||
| import { initializeMonitoring } from '@/initialize-monitoring/initialize-monitoring'; | |||
There was a problem hiding this comment.
Remove duplicate import of initializeMonitoring
The initializeMonitoring module is already imported on line 5, making this import redundant.
-import { initializeMonitoring } from '@/initialize-monitoring/initialize-monitoring';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { initializeMonitoring } from '@/initialize-monitoring/initialize-monitoring'; |
🧰 Tools
🪛 Biome
[error] 16-16: Shouldn't redeclare 'initializeMonitoring'. Consider to delete it or rename it.
'initializeMonitoring' is defined here:
(lint/suspicious/noRedeclare)
apps/kyb-app/src/main.tsx
Outdated
| //@ts-ignore | ||
| globalThis.env = globalThis.env || { | ||
| API_URL: import.meta.env.VITE_API_URL, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety and environment variable handling
The current implementation has several issues:
- Using @ts-ignore masks potential type errors
- No validation of required environment variables
- Lacks proper TypeScript declarations
Consider this type-safe alternative:
-//@ts-ignore
-globalThis.env = globalThis.env || {
- API_URL: import.meta.env.VITE_API_URL,
-};
+interface AppConfig {
+ API_URL: string;
+}
+
+declare global {
+ var env: AppConfig;
+}
+
+if (!import.meta.env.VITE_API_URL) {
+ throw new Error('VITE_API_URL environment variable is required');
+}
+
+globalThis.env = {
+ API_URL: import.meta.env.VITE_API_URL,
+};This approach:
- Adds proper TypeScript declarations
- Validates required environment variables
- Maintains type safety without ignoring TypeScript checks
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| //@ts-ignore | |
| globalThis.env = globalThis.env || { | |
| API_URL: import.meta.env.VITE_API_URL, | |
| }; | |
| interface AppConfig { | |
| API_URL: string; | |
| } | |
| declare global { | |
| var env: AppConfig; | |
| } | |
| if (!import.meta.env.VITE_API_URL) { | |
| throw new Error('VITE_API_URL environment variable is required'); | |
| } | |
| globalThis.env = { | |
| API_URL: import.meta.env.VITE_API_URL, | |
| }; |
Summary by CodeRabbit
Release Notes
New Features
entrypoint.shscript to initialize environment variables for the application.envto store environment-related settings.config.js.Improvements
Bug Fixes