Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: run dist with environment variables kyb-app #2645

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

chore: run dist with environment variables kyb-app #2645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
10936ef
chore: run dist with environment variables kyb-app
pratapalakshmi Aug 18, 2024
e5c465e
fix: formatting errors
pratapalakshmi Nov 4, 2024
67ba0f0
Merge branch 'dev' into chore/run/time/dist/kyb-app
alonp99 Nov 6, 2024
79908d6
chore: code refactor for runtime env variables
pratapalakshmi Nov 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions apps/kyb-app/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,29 @@ RUN npm install --legacy-peer-deps
COPY . .

RUN mv /app/.env.example /app/.env

RUN npm run build

ENV PATH="$PATH:./node_modules/.bin"
ENV PATH="$PATH:/app/node_modules/.bin"

EXPOSE 5201

CMD ["npm","run","dev", "--host"]
CMD ["npm", "run", "dev", "--host"]

FROM nginx:stable-alpine as prod

WORKDIR /app

COPY --from=dev /app/dist /usr/share/nginx/html

COPY --from=dev /app/entrypoint.sh /app/entrypoint.sh

COPY example.nginx.conf /etc/nginx/conf.d/default.conf

RUN chmod a+x /app/entrypoint.sh;

EXPOSE 80

ENTRYPOINT [ "/app/entrypoint.sh" ]

CMD ["nginx", "-g", "daemon off;"]
46 changes: 46 additions & 0 deletions apps/kyb-app/entrypoint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
#!/usr/bin/env sh

Comment on lines +1 to +2
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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:

  1. Change the shebang to use bash:
-#!/usr/bin/env sh
+#!/usr/bin/env bash
  1. Or use POSIX-compatible syntax:
-if [[ -z "$VITE_DOMAIN" ]]
+if [ -z "$VITE_DOMAIN" ]

Committable 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
Comment on lines +8 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Security: Avoid hardcoding default API key

Using a hardcoded default API key ("secret") could be dangerous if accidentally used in non-local environments.

Consider:

  1. Making the API key required (fail if not provided)
  2. 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.

Suggested change
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)


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
Comment on lines +3 to +32
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ 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:

  1. Validation for provided values
  2. 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
 fi

Committable 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)


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
Comment on lines +34 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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:

  1. Moving config to a non-public directory and importing it server-side
  2. Using environment variables directly in your application
  3. 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;
+EOF

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


# Handle CMD command
exec "$@"
3 changes: 3 additions & 0 deletions apps/kyb-app/global.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
declare global {
export var env: { [key: string]: any };
}
Comment on lines +1 to +3
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider improving type safety and documentation

While the implementation works, there are several improvements that could make it more robust and maintainable:

  1. Replace any with a specific interface for environment variables
  2. Add readonly protection to prevent accidental mutations
  3. 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.env is used in vite.config.ts (4 occurrences)
  • import.meta.env is used extensively across the application (17 occurrences)
  • globalThis.env is used in a few places (4 occurrences)

Key issues:

  • The global declaration in global.d.ts suggests using globalThis.env, but most of the codebase uses import.meta.env
  • process.env usage 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

1 change: 1 addition & 0 deletions apps/kyb-app/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Collaborator

Choose a reason for hiding this comment

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

wrong path

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No in the dist index.html and config.js stay in the same directory

Copy link
Contributor

Choose a reason for hiding this comment

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

💡 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 access
  • VITE_SENTRY_AUTH_TOKEN: Though empty by default, exposing auth tokens is risky
  • VITE_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

</head>
<body>
<div id="root"></div>
Expand Down
11 changes: 11 additions & 0 deletions apps/kyb-app/public/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
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',
Comment on lines +7 to +8
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

VITE_SENTRY_AUTH_TOKEN: import.meta.env.VITE_SENTRY_AUTH_TOKEN || '',
VITE_SENTRY_DSN: import.meta.env.VITE_SENTRY_DSN || '',
};
3 changes: 2 additions & 1 deletion apps/kyb-app/src/common/utils/request.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 || `${window.location.origin}/api/v1/`,
retry: {
limit: 1,
statusCodes: [500, 408, 404, 404, 403, 401],
Expand Down
Loading