fix: disable recaptcha and analytics when in dev mode#48
fix: disable recaptcha and analytics when in dev mode#48mfrancisc merged 9 commits intocodeready-toolchain:masterfrom
Conversation
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant SandboxHeader
participant ConfigAPI
participant DOM
participant SandboxContext
participant RecaptchaLoader
participant SegmentService
participant RegistrationClient
Browser->>SandboxHeader: mount
SandboxHeader->>ConfigAPI: read sandbox.environment
ConfigAPI-->>SandboxHeader: environment (DEV | PROD)
alt environment == PROD
SandboxHeader->>DOM: inject trustarc & dpal scripts
SandboxHeader->>SandboxContext: set isProd = true
SandboxContext->>RecaptchaLoader: useRecaptcha(enabled=true) -> load script
SandboxContext->>SegmentService: init with write key
else environment == DEV
SandboxHeader->>DOM: skip injecting scripts
SandboxHeader->>SandboxContext: set isProd = false
SandboxContext->>RecaptchaLoader: useRecaptcha(enabled=false) -> skip load
SandboxContext->>SegmentService: skip init
end
Browser->>RegistrationClient: submit signup form
RegistrationClient->>ConfigAPI: read sandbox.environment
ConfigAPI-->>RegistrationClient: environment
alt environment == PROD
RegistrationClient->>RecaptchaLoader: get token
RecaptchaLoader-->>RegistrationClient: token
RegistrationClient->>Browser: POST /signup with Recaptcha-Token header
else environment == DEV
RegistrationClient->>Browser: POST /signup without Recaptcha-Token header
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (1)
plugins/sandbox/src/components/__tests__/SandboxHeader.test.tsx (1)
153-177: Add one regression test for non-canonical environment values.To lock down the dev-safety behavior, add a case for values like
'dev'/' DEV 'so tracker scripts stay disabled unless value is explicitly canonical production.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/sandbox/src/components/__tests__/SandboxHeader.test.tsx` around lines 153 - 177, Add a regression test to ensure non-canonical environment values (e.g., 'dev', ' DEV ') are treated as non-PROD so analytics scripts remain disabled: update SandboxHeader.test.tsx by adding a test that calls renderComponent('My Page Title', 'dev') and another or a parameterized case for ' DEV ' (or trim/normalize variants) and assert document.getElementById('trustarc') and document.getElementById('dpal') are null; reference renderComponent and the analytics script ids ('trustarc', 'dpal') to locate the test area and follow the existing test structure used for 'DEV'/'PROD' cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/sandbox/src/hooks/useSandboxContext.tsx`:
- Around line 66-69: Normalize the environment string before comparing so
non-exact variants like "dev", " Dev ", or "dev\n" don't count as production:
change the isProd computation in useSandboxContext (the
configApi/getOptionalString usage that sets isProd and calls useRecaptcha) to
obtain the optional string, apply .trim() and .toUpperCase() and then compare to
'DEV' (i.e., isProd = (env?.trim().toUpperCase() ?? 'PROD') !== 'DEV'); apply
the exact same normalization pattern in the SandboxHeader component where it
parses sandbox.environment (replace the current loose comparison with the same
trim+toUpperCase flow) so both places behave consistently.
---
Nitpick comments:
In `@plugins/sandbox/src/components/__tests__/SandboxHeader.test.tsx`:
- Around line 153-177: Add a regression test to ensure non-canonical environment
values (e.g., 'dev', ' DEV ') are treated as non-PROD so analytics scripts
remain disabled: update SandboxHeader.test.tsx by adding a test that calls
renderComponent('My Page Title', 'dev') and another or a parameterized case for
' DEV ' (or trim/normalize variants) and assert
document.getElementById('trustarc') and document.getElementById('dpal') are
null; reference renderComponent and the analytics script ids ('trustarc',
'dpal') to locate the test area and follow the existing test structure used for
'DEV'/'PROD' cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 642e9fc7-d369-4797-92d0-60ddef19cd9a
📒 Files selected for processing (7)
app-config.production.yamldeploy/base/app-config.yamlplugins/sandbox/config.d.tsplugins/sandbox/src/components/SandboxHeader.tsxplugins/sandbox/src/components/__tests__/SandboxHeader.test.tsxplugins/sandbox/src/hooks/useRecaptcha.tsplugins/sandbox/src/hooks/useSandboxContext.tsx
| const configApi = useApi(configApiRef); | ||
| const isProd = | ||
| (configApi.getOptionalString('sandbox.environment') ?? 'PROD') !== 'DEV'; | ||
| useRecaptcha(isProd); |
There was a problem hiding this comment.
Fail-open environment parsing can silently re-enable tracking.
Line 67 currently treats anything except exact 'DEV' as production. Values like dev, Dev, or ' DEV ' will enable reCAPTCHA/Segment unexpectedly.
🔧 Proposed fix
- const isProd =
- (configApi.getOptionalString('sandbox.environment') ?? 'PROD') !== 'DEV';
+ const environment =
+ (configApi.getOptionalString('sandbox.environment') ?? 'PROD')
+ .trim()
+ .toUpperCase();
+ const isProd = environment === 'PROD';Please apply the same normalization pattern in plugins/sandbox/src/components/SandboxHeader.tsx (Lines 34-39) to keep behavior consistent.
As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 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.
| const configApi = useApi(configApiRef); | |
| const isProd = | |
| (configApi.getOptionalString('sandbox.environment') ?? 'PROD') !== 'DEV'; | |
| useRecaptcha(isProd); | |
| const configApi = useApi(configApiRef); | |
| const environment = | |
| (configApi.getOptionalString('sandbox.environment') ?? 'PROD') | |
| .trim() | |
| .toUpperCase(); | |
| const isProd = environment === 'PROD'; | |
| useRecaptcha(isProd); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/sandbox/src/hooks/useSandboxContext.tsx` around lines 66 - 69,
Normalize the environment string before comparing so non-exact variants like
"dev", " Dev ", or "dev\n" don't count as production: change the isProd
computation in useSandboxContext (the configApi/getOptionalString usage that
sets isProd and calls useRecaptcha) to obtain the optional string, apply .trim()
and .toUpperCase() and then compare to 'DEV' (i.e., isProd =
(env?.trim().toUpperCase() ?? 'PROD') !== 'DEV'); apply the exact same
normalization pattern in the SandboxHeader component where it parses
sandbox.environment (replace the current loose comparison with the same
trim+toUpperCase flow) so both places behave consistently.
MikelAlejoBR
left a comment
MikelAlejoBR
left a comment
There was a problem hiding this comment.
Looking really good to me!
plugins/sandbox/src/components/__tests__/SandboxHeader.test.tsx
Outdated
Show resolved
Hide resolved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mfrancisc, MikelAlejoBR, rajivnathan, rsoaresd The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest failed after clicking on "Try it", I suspect that could be due to flakiness, but let's see |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/sandbox/src/api/RegistrationBackendClient.tsx`:
- Around line 114-116: Normalize and validate the sandbox.environment string
before computing isDev: read the raw value via
this.configApi.getOptionalString('sandbox.environment'), compute const env =
raw?.trim().toUpperCase(), set isDev = env === 'DEV', and if raw is non-empty
and env is not one of the expected values ('DEV' or 'PROD') emit a warning (use
the class logger or console.warn) so typos like "dev" or " DEV " don't silently
act as PROD; update the isDev computation in RegistrationBackendClient (the
const isDev) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d41b7bad-1b4f-40cc-a8a5-1a44bc80c4d7
📒 Files selected for processing (1)
plugins/sandbox/src/api/RegistrationBackendClient.tsx
| const isDev = | ||
| (this.configApi.getOptionalString('sandbox.environment') ?? 'PROD') === | ||
| 'DEV'; |
There was a problem hiding this comment.
Harden sandbox.environment parsing to avoid silent PROD fallback on typos.
Line 115 currently treats any non-DEV value as PROD. Since the config type is free-form string, values like dev or DEV will unexpectedly enable reCAPTCHA. Normalize and validate before branching.
Suggested fix
- const isDev =
- (this.configApi.getOptionalString('sandbox.environment') ?? 'PROD') ===
- 'DEV';
+ const environment = (
+ this.configApi.getOptionalString('sandbox.environment') ?? 'PROD'
+ )
+ .trim()
+ .toUpperCase();
+ if (environment !== 'DEV' && environment !== 'PROD') {
+ throw new Error(
+ `Invalid sandbox.environment: "${environment}". Expected "DEV" or "PROD".`,
+ );
+ }
+ const isDev = environment === 'DEV';As per coding guidelines, "**: Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 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.
| const isDev = | |
| (this.configApi.getOptionalString('sandbox.environment') ?? 'PROD') === | |
| 'DEV'; | |
| const environment = ( | |
| this.configApi.getOptionalString('sandbox.environment') ?? 'PROD' | |
| ) | |
| .trim() | |
| .toUpperCase(); | |
| if (environment !== 'DEV' && environment !== 'PROD') { | |
| throw new Error( | |
| `Invalid sandbox.environment: "${environment}". Expected "DEV" or "PROD".`, | |
| ); | |
| } | |
| const isDev = environment === 'DEV'; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/sandbox/src/api/RegistrationBackendClient.tsx` around lines 114 -
116, Normalize and validate the sandbox.environment string before computing
isDev: read the raw value via
this.configApi.getOptionalString('sandbox.environment'), compute const env =
raw?.trim().toUpperCase(), set isDev = env === 'DEV', and if raw is non-empty
and env is not one of the expected values ('DEV' or 'PROD') emit a warning (use
the class logger or console.warn) so typos like "dev" or " DEV " don't silently
act as PROD; update the isDev computation in RegistrationBackendClient (the
const isDev) accordingly.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugins/sandbox/src/api/RegistrationBackendClient.tsx (1)
115-117:⚠️ Potential issue | 🟠 MajorNormalize
sandbox.environmentbefore comparison.Line 115 only treats exact
'DEV'as DEV. Values like'dev'or' DEV 'silently behave as PROD and can unexpectedly re-enable reCAPTCHA on dev deployments. Normalize (trim().toUpperCase()) before branching, and warn on unexpected values.Suggested fix
- const isDev = - (this.configApi.getOptionalString('sandbox.environment') ?? - SandboxEnvironment.PROD) === SandboxEnvironment.DEV; + const rawEnvironment = this.configApi.getOptionalString('sandbox.environment'); + const environment = (rawEnvironment ?? SandboxEnvironment.PROD).trim().toUpperCase(); + if ( + rawEnvironment && + environment !== SandboxEnvironment.DEV && + environment !== SandboxEnvironment.PROD + ) { + // Prefer class logger if available + // eslint-disable-next-line no-console + console.warn( + `Invalid sandbox.environment value "${rawEnvironment}". Expected DEV or PROD.`, + ); + } + const isDev = environment === SandboxEnvironment.DEV;As per coding guidelines, "**: Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/sandbox/src/api/RegistrationBackendClient.tsx` around lines 115 - 117, The current isDev check in RegistrationBackendClient.tsx uses the raw value from configApi.getOptionalString('sandbox.environment') which treats only exact 'DEV' as DEV; normalize the value by calling .trim().toUpperCase() before comparing to SandboxEnvironment.DEV and defaulting to SandboxEnvironment.PROD, and emit a warning via the logger if the original value is non-empty and not one of the expected normalized values (e.g., not 'DEV' or 'PROD') so misconfigured casing/whitespace is visible; update the isDev logic and add the warning near where isDev is computed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugins/sandbox/src/api/RegistrationBackendClient.tsx`:
- Around line 115-117: The current isDev check in RegistrationBackendClient.tsx
uses the raw value from configApi.getOptionalString('sandbox.environment') which
treats only exact 'DEV' as DEV; normalize the value by calling
.trim().toUpperCase() before comparing to SandboxEnvironment.DEV and defaulting
to SandboxEnvironment.PROD, and emit a warning via the logger if the original
value is non-empty and not one of the expected normalized values (e.g., not
'DEV' or 'PROD') so misconfigured casing/whitespace is visible; update the isDev
logic and add the warning near where isDev is computed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d05d2dc2-cfa0-4d26-be23-90c02c03dcdf
📒 Files selected for processing (5)
plugins/sandbox/src/api/RegistrationBackendClient.tsxplugins/sandbox/src/components/SandboxHeader.tsxplugins/sandbox/src/components/__tests__/SandboxHeader.test.tsxplugins/sandbox/src/const.tsplugins/sandbox/src/hooks/useSandboxContext.tsx
✅ Files skipped from review due to trivial changes (1)
- plugins/sandbox/src/const.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/sandbox/src/hooks/useSandboxContext.tsx
- plugins/sandbox/src/components/SandboxHeader.tsx
4 participants
wrt https://redhat-internal.slack.com/archives/C085PFGDLGY/p1774362139343779
Jira: https://redhat.atlassian.net/browse/SANDBOX-1738
e2e tests : codeready-toolchain/toolchain-e2e#1270
this PR adds a new configuration variable
environmentthat can be defined at deploy time.When set to
DEVit will disable:by default it's set to
PROD.Summary by CodeRabbit
New Features
Chores
Tests