[dev] [tofikwest] feat/browser-automation-ui#3417
Open
github-actions[bot] wants to merge 161 commits into
Open
[dev] [tofikwest] feat/browser-automation-ui#3417github-actions[bot] wants to merge 161 commits into
github-actions[bot] wants to merge 161 commits into
Conversation
Browser automations can now sign back in on their own when a vendor session expires, instead of always requiring a person to re-authenticate. - Store a profile's login (username, password, optional authenticator setup key) in a per-organization 1Password vault via a new credential-storage service and a POST profiles/:id/credentials endpoint. - Resolve those credentials just-in-time during a run through the previously-stubbed credential vault adapter, now backed by 1Password, and drive sign-in with Stagehand variable substitution so secret values are never sent to the model. - Retry once with a freshly generated one-time code to cover the rotation boundary; fall back to human re-authentication when a login needs a step the agent cannot complete (SMS/email/push/SSO). - Let the scheduler attempt a run for a needs-reauth profile that has stored credentials, and mark a profile verified again after a successful run. - Load the 1Password SDK lazily and mark it external for Trigger.dev builds; the feature stays inert unless OP_SERVICE_ACCOUNT_TOKEN is set. Adds unit tests for the credential resolver and the re-login flow.
Adds the first step of the browser-automation connect wizard: a credentials form (username, password, optional authenticator setup key) shown before the live sign-in. On submit it resolves the auth profile and stores the login via the credentials endpoint so scheduled and manual runs can sign in on their own. Credential storage failures are non-fatal and fall back to manual sign-in. Part of the Browser Automations redesign (connect-a-login flow). Adds unit tests for the form (fields, validation, submit, password toggle).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
22 issues found across 27 files
Confidence score: 1/5
- In
apps/api/src/browserbase/browser-auth-profiles.controller.ts,totpSeedis not redacted in audit logging, so raw MFA seeds can be persisted and exposed as sensitive secrets if logs are accessed—addtotpSeedto redaction and ship a regression test before merging. - In
apps/api/src/browserbase/onepassword-credential-vault.adapter.ts,integration:createcallers can reference arbitraryop://items readable by the shared service account, which risks cross-organization credential use—enforce org-scoped authorization on vault item references before merge. - In
apps/api/src/browserbase/browser-credential-storage.service.tsandapps/api/src/browserbase/browser-auth-profiles.controller.ts, credential saves can mis-store OTP fields and create orphaned replacement 1Password items, leaving stale passwords/TOTP secrets active in the vault—fix OTP field ID handling and update/save logic to reuse or clean up provider-owned items before merging. - In
apps/api/src/browserbase/browser-automation-execution.service.tsandapps/api/src/browserbase/onepassword-credential-vault.adapter.ts, result ordering and vault error handling can misclassify failures (needs_reauth) and overwrite newerblocked/reauth states, causing incorrect operator actions—guard status updates by version/order and surface vault outages distinctly before merge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/hooks/useBrowserContext.ts">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/hooks/useBrowserContext.ts:65">
P3: Incorrect credential payloads or regressions in the fallback behavior can ship unnoticed because the new credential-storage branch is untested. A hook test covering the credentials POST and an error response that still proceeds to the manual session flow would protect this sensitive path.</violation>
<violation number="2" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/hooks/useBrowserContext.ts:65">
P2: The auth flow now sends raw login secrets to a new save endpoint before the session is started. If session creation or navigation fails afterward, the profile can end up with stored credentials but no completed connection, so the UI should make that multi-step failure mode explicit or roll the credential save into the same transactional path.</violation>
</file>
<file name="apps/api/src/browserbase/browserbase.service.ts">
<violation number="1" location="apps/api/src/browserbase/browserbase.service.ts:72">
P3: The new credential-storage façade has no focused test verifying that the organization/profile identifiers and secret fields reach `BrowserCredentialStorageService`. A delegation test would catch regressions in this API path without exercising the external vault.</violation>
</file>
<file name="apps/api/src/browserbase/browser-auth-profile.service.ts">
<violation number="1" location="apps/api/src/browserbase/browser-auth-profile.service.ts:209">
P3: The new verification transition is untested, so regressions in clearing `blockedReason`, recording `lastVerifiedAt`, or handling an already-verified profile can pass unnoticed. Adding focused service tests for the missing-profile, already-verified, and state-transition cases would cover this path.</violation>
<violation number="2" location="apps/api/src/browserbase/browser-auth-profile.service.ts:214">
P2: Successful automation runs on an already-verified profile do not refresh `lastVerifiedAt`, so the API can report the original verification time instead of the latest successful authentication. Persist the successful-run timestamp even when the status is already `verified`; only avoid a status-only write if no metadata needs updating.</violation>
</file>
<file name="apps/api/src/browserbase/browser-evidence-execution.ts">
<violation number="1" location="apps/api/src/browserbase/browser-evidence-execution.ts:88">
P3: The new authentication branch can regress without detecting whether `executeBrowserEvidence` resumes actions after successful recovery or returns the expected `needs_reauth` result after each failure mode. A focused execution-level test covering those branches would protect the integration between `checkAuth`, the vault-backed relogin helper, and result classification.</violation>
<violation number="2" location="apps/api/src/browserbase/browser-evidence-execution.ts:97">
P3: `relogin.page` is never used after this assignment: failure returns immediately, and success overwrites `page` during evidence-page resolution. Removing this dead assignment keeps the re-auth flow clearer and avoids implying that the returned page controls the subsequent action.</violation>
</file>
<file name="apps/api/src/browserbase/browser-automation-execution.service.ts">
<violation number="1" location="apps/api/src/browserbase/browser-automation-execution.service.ts:144">
P2: A `needs_reauth` profile with only `vaultProvider` set is treated as auto-relogin-capable even though the credential adapter cannot resolve credentials without `vaultExternalItemRef`; this causes credential-less profiles to consume a full automation attempt before returning `needs_reauth`. Gate this path on the actual stored credential reference (and supported provider) rather than the provider label alone.</violation>
<violation number="2" location="apps/api/src/browserbase/browser-automation-execution.service.ts:144">
P3: The new auto-relogin and success-to-verified transitions are untested, so regressions in credential gating or profile-state recovery can silently reach scheduled runs. Adding execution-service tests for usable versus incomplete vault metadata, successful recovery, and a failed result preserving `needs_reauth` would make this behavior verifiable.</violation>
<violation number="3" location="apps/api/src/browserbase/browser-automation-execution.service.ts:199">
P2: A stale successful run can reset a newer `needs_reauth` or `blocked` result because `markVerified` is applied outside the runner's profile lock and has no result-order/version guard. Serializing profile-result updates or making the transition conditional on the run's completion order would prevent an older run from re-enabling a profile after a later auth failure.</violation>
</file>
<file name="apps/api/src/browserbase/onepassword-credential-vault.adapter.ts">
<violation number="1" location="apps/api/src/browserbase/onepassword-credential-vault.adapter.ts:34">
P1: A caller with `integration:create` can point a profile at any `op://` item the shared service account can read, allowing an automation to use credentials from another organization's vault when the reference is known. The reference should be derived from server-owned storage or validated against the profile's organization/connection before resolving it.</violation>
<violation number="2" location="apps/api/src/browserbase/onepassword-credential-vault.adapter.ts:71">
P2: A 1Password outage or access-denied response is silently converted into missing credentials, so runs report `needs_reauth` and can trigger unnecessary manual reauthentication instead of exposing a vault failure. Only a classified missing-field error should be suppressed; authentication, permission, and transport errors should propagate.</violation>
<violation number="3" location="apps/api/src/browserbase/onepassword-credential-vault.adapter.ts:74">
P2: A malformed or caller-supplied reference can inject lines into application logs, and normal failures disclose vault/item metadata. Logging a fixed error message or a sanitized/structured identifier would avoid treating the raw reference as log text.</violation>
</file>
<file name="apps/api/src/browserbase/browser-credential-storage.service.ts">
<violation number="1" location="apps/api/src/browserbase/browser-credential-storage.service.ts:38">
P3: The new credential-storage workflow has no unit coverage for tenant scoping, vault reuse/creation, TOTP field construction, external failures, or profile-reference persistence. Adding a colocated service spec would catch regressions in this secrets-handling path.</violation>
<violation number="2" location="apps/api/src/browserbase/browser-credential-storage.service.ts:98">
P2: Concurrent credential saves can create multiple vaults for one organization because the list-then-create check is not atomic. A per-organization lock or post-create recheck/deduplication would keep all profiles in the intended single vault.</violation>
<violation number="3" location="apps/api/src/browserbase/browser-credential-storage.service.ts:139">
P1: Providing a TOTP seed does not create the documented 1Password Login OTP field because its ID is set to the display title. Keeping the title for secret references but using the built-in ID `onetimepassword` allows OTP generation and resolution to work reliably.</violation>
</file>
<file name="apps/api/src/browserbase/browser-auth-profiles.controller.ts">
<violation number="1" location="apps/api/src/browserbase/browser-auth-profiles.controller.ts:117">
P2: Audit records for this credential update will be mislabeled as `create` because the route uses POST while requiring `integration:update`. Have the audit action honor the declared permission action (or use an update verb) so compliance history reflects the RBAC decision.</violation>
<violation number="2" location="apps/api/src/browserbase/browser-auth-profiles.controller.ts:125">
P2: Successful credential saves will return HTTP 201, but the OpenAPI contract advertises 200, which can make generated clients treat a valid response as unexpected. Either add `@HttpCode(200)` or document 201 here.</violation>
<violation number="3" location="apps/api/src/browserbase/browser-auth-profiles.controller.ts:129">
P0: The new credentials request can persist the raw TOTP seed in the audit database. `password` is redacted by `SENSITIVE_KEYS`, but `totpSeed` is not, so this endpoint should add TOTP seed redaction (with a regression test) before accepting secrets through an audited mutation.</violation>
<violation number="4" location="apps/api/src/browserbase/browser-auth-profiles.controller.ts:131">
P1: Repeated saves through this endpoint create a new 1Password Login item while leaving the previous item—and its old password/TOTP secret—in the organization vault. Reuse the existing provider-owned item or remove the old item after a successful replacement so credential updates do not accumulate stale secrets.</violation>
</file>
<file name="apps/api/src/browserbase/browser-credential-vault.factory.ts">
<violation number="1" location="apps/api/src/browserbase/browser-credential-vault.factory.ts:14">
P3: The environment-dependent adapter selection is untested, so a change to `OP_SERVICE_ACCOUNT_TOKEN` handling or either return branch could silently break credential resolution. A focused factory test covering configured, whitespace-only, and absent tokens would satisfy the API rule that every feature include tests.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/BrowserAutomations.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/BrowserAutomations.tsx:127">
P2: The credential form is only reachable when there are no automations yet, so existing tasks without a browser context lose the ability to start the new connect flow. It would be safer to expose the same start-connect action anywhere `context.status === 'no-context'` can occur, not just in the empty state.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| async storeProfileCredentials( | ||
| @OrganizationId() organizationId: string, | ||
| @Param('profileId') profileId: string, | ||
| @Body() dto: StoreAuthProfileCredentialsDto, |
Contributor
There was a problem hiding this comment.
P0: The new credentials request can persist the raw TOTP seed in the audit database. password is redacted by SENSITIVE_KEYS, but totpSeed is not, so this endpoint should add TOTP seed redaction (with a regression test) before accepting secrets through an audited mutation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-auth-profiles.controller.ts, line 129:
<comment>The new credentials request can persist the raw TOTP seed in the audit database. `password` is redacted by `SENSITIVE_KEYS`, but `totpSeed` is not, so this endpoint should add TOTP seed redaction (with a regression test) before accepting secrets through an audited mutation.</comment>
<file context>
@@ -112,6 +113,30 @@ export class BrowserAuthProfilesController {
+ async storeProfileCredentials(
+ @OrganizationId() organizationId: string,
+ @Param('profileId') profileId: string,
+ @Body() dto: StoreAuthProfileCredentialsDto,
+ ): Promise<BrowserAuthProfileResponseDto> {
+ return (await this.browserbaseService.storeAuthProfileCredentials({
</file context>
| }): Promise<RuntimeCredentialMaterial | null> { | ||
| if (params.provider !== ONEPASSWORD_PROVIDER) return null; | ||
|
|
||
| const itemRef = params.externalItemRef?.trim(); |
Contributor
There was a problem hiding this comment.
P1: A caller with integration:create can point a profile at any op:// item the shared service account can read, allowing an automation to use credentials from another organization's vault when the reference is known. The reference should be derived from server-owned storage or validated against the profile's organization/connection before resolving it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/onepassword-credential-vault.adapter.ts, line 34:
<comment>A caller with `integration:create` can point a profile at any `op://` item the shared service account can read, allowing an automation to use credentials from another organization's vault when the reference is known. The reference should be derived from server-owned storage or validated against the profile's organization/connection before resolving it.</comment>
<file context>
@@ -0,0 +1,78 @@
+ }): Promise<RuntimeCredentialMaterial | null> {
+ if (params.provider !== ONEPASSWORD_PROVIDER) return null;
+
+ const itemRef = params.externalItemRef?.trim();
+ if (!itemRef) return null;
+
</file context>
|
|
||
| if (totpSeed?.trim()) { | ||
| fields.push({ | ||
| id: TOTP_FIELD_TITLE, |
Contributor
There was a problem hiding this comment.
P1: Providing a TOTP seed does not create the documented 1Password Login OTP field because its ID is set to the display title. Keeping the title for secret references but using the built-in ID onetimepassword allows OTP generation and resolution to work reliably.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-credential-storage.service.ts, line 139:
<comment>Providing a TOTP seed does not create the documented 1Password Login OTP field because its ID is set to the display title. Keeping the title for secret references but using the built-in ID `onetimepassword` allows OTP generation and resolution to work reliably.</comment>
<file context>
@@ -0,0 +1,148 @@
+
+ if (totpSeed?.trim()) {
+ fields.push({
+ id: TOTP_FIELD_TITLE,
+ title: TOTP_FIELD_TITLE,
+ fieldType: ItemFieldType.Totp,
</file context>
| @Param('profileId') profileId: string, | ||
| @Body() dto: StoreAuthProfileCredentialsDto, | ||
| ): Promise<BrowserAuthProfileResponseDto> { | ||
| return (await this.browserbaseService.storeAuthProfileCredentials({ |
Contributor
There was a problem hiding this comment.
P1: Repeated saves through this endpoint create a new 1Password Login item while leaving the previous item—and its old password/TOTP secret—in the organization vault. Reuse the existing provider-owned item or remove the old item after a successful replacement so credential updates do not accumulate stale secrets.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-auth-profiles.controller.ts, line 131:
<comment>Repeated saves through this endpoint create a new 1Password Login item while leaving the previous item—and its old password/TOTP secret—in the organization vault. Reuse the existing provider-owned item or remove the old item after a successful replacement so credential updates do not accumulate stale secrets.</comment>
<file context>
@@ -112,6 +113,30 @@ export class BrowserAuthProfilesController {
+ @Param('profileId') profileId: string,
+ @Body() dto: StoreAuthProfileCredentialsDto,
+ ): Promise<BrowserAuthProfileResponseDto> {
+ return (await this.browserbaseService.storeAuthProfileCredentials({
+ organizationId,
+ profileId,
</file context>
| if (!profile) { | ||
| throw new NotFoundException('Browser auth profile not found'); | ||
| } | ||
| // Avoid a needless write when the profile is already verified. |
Contributor
There was a problem hiding this comment.
P2: Successful automation runs on an already-verified profile do not refresh lastVerifiedAt, so the API can report the original verification time instead of the latest successful authentication. Persist the successful-run timestamp even when the status is already verified; only avoid a status-only write if no metadata needs updating.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-auth-profile.service.ts, line 214:
<comment>Successful automation runs on an already-verified profile do not refresh `lastVerifiedAt`, so the API can report the original verification time instead of the latest successful authentication. Persist the successful-run timestamp even when the status is already `verified`; only avoid a status-only write if no metadata needs updating.</comment>
<file context>
@@ -206,6 +206,24 @@ export class BrowserAuthProfileService {
+ if (!profile) {
+ throw new NotFoundException('Browser auth profile not found');
+ }
+ // Avoid a needless write when the profile is already verified.
+ if (profile.status === 'verified') return profile;
+
</file context>
| return { profile: updated, auth }; | ||
| } | ||
|
|
||
| async markVerified(input: { organizationId: string; profileId: string }) { |
Contributor
There was a problem hiding this comment.
P3: The new verification transition is untested, so regressions in clearing blockedReason, recording lastVerifiedAt, or handling an already-verified profile can pass unnoticed. Adding focused service tests for the missing-profile, already-verified, and state-transition cases would cover this path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-auth-profile.service.ts, line 209:
<comment>The new verification transition is untested, so regressions in clearing `blockedReason`, recording `lastVerifiedAt`, or handling an already-verified profile can pass unnoticed. Adding focused service tests for the missing-profile, already-verified, and state-transition cases would cover this path.</comment>
<file context>
@@ -206,6 +206,24 @@ export class BrowserAuthProfileService {
return { profile: updated, auth };
}
+ async markVerified(input: { organizationId: string; profileId: string }) {
+ const profile = await this.getProfile(input);
+ if (!profile) {
</file context>
| (await checkAuth(activeStagehand)).isLoggedIn, | ||
| log: (message) => log('auth', message), | ||
| }); | ||
| page = relogin.page ?? page; |
Contributor
There was a problem hiding this comment.
P3: relogin.page is never used after this assignment: failure returns immediately, and success overwrites page during evidence-page resolution. Removing this dead assignment keeps the re-auth flow clearer and avoids implying that the returned page controls the subsequent action.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-evidence-execution.ts, line 97:
<comment>`relogin.page` is never used after this assignment: failure returns immediately, and success overwrites `page` during evidence-page resolution. Removing this dead assignment keeps the re-auth flow clearer and avoids implying that the returned page controls the subsequent action.</comment>
<file context>
@@ -74,12 +81,38 @@ export async function executeBrowserEvidence({
+ (await checkAuth(activeStagehand)).isLoggedIn,
+ log: (message) => log('auth', message),
+ });
+ page = relogin.page ?? page;
+
+ if (!relogin.isLoggedIn) {
</file context>
| ); | ||
| log('auth', classified.userFacing); | ||
| return toExecutionFailure({ classified, logs }); | ||
| const relogin = await reloginWithStoredCredentials({ |
Contributor
There was a problem hiding this comment.
P3: The new authentication branch can regress without detecting whether executeBrowserEvidence resumes actions after successful recovery or returns the expected needs_reauth result after each failure mode. A focused execution-level test covering those branches would protect the integration between checkAuth, the vault-backed relogin helper, and result classification.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-evidence-execution.ts, line 88:
<comment>The new authentication branch can regress without detecting whether `executeBrowserEvidence` resumes actions after successful recovery or returns the expected `needs_reauth` result after each failure mode. A focused execution-level test covering those branches would protect the integration between `checkAuth`, the vault-backed relogin helper, and result classification.</comment>
<file context>
@@ -74,12 +81,38 @@ export async function executeBrowserEvidence({
);
- log('auth', classified.userFacing);
- return toExecutionFailure({ classified, logs });
+ const relogin = await reloginWithStoredCredentials({
+ stagehand: activeStagehand,
+ sessions,
</file context>
| // own, so let a needs_reauth profile attempt the run instead of blocking it. | ||
| // Profiles that are blocked or never verified still require a human. | ||
| const canAutoRelogin = | ||
| profile.status === 'needs_reauth' && Boolean(profile.vaultProvider); |
Contributor
There was a problem hiding this comment.
P3: The new auto-relogin and success-to-verified transitions are untested, so regressions in credential gating or profile-state recovery can silently reach scheduled runs. Adding execution-service tests for usable versus incomplete vault metadata, successful recovery, and a failed result preserving needs_reauth would make this behavior verifiable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-automation-execution.service.ts, line 144:
<comment>The new auto-relogin and success-to-verified transitions are untested, so regressions in credential gating or profile-state recovery can silently reach scheduled runs. Adding execution-service tests for usable versus incomplete vault metadata, successful recovery, and a failed result preserving `needs_reauth` would make this behavior verifiable.</comment>
<file context>
@@ -137,7 +137,12 @@ export class BrowserAutomationExecutionService {
+ // own, so let a needs_reauth profile attempt the run instead of blocking it.
+ // Profiles that are blocked or never verified still require a human.
+ const canAutoRelogin =
+ profile.status === 'needs_reauth' && Boolean(profile.vaultProvider);
+ if (profile.status !== 'verified' && !canAutoRelogin) {
const result = this.profileBlockedResult(profile.status);
</file context>
| @@ -0,0 +1,148 @@ | |||
| import { | |||
Contributor
There was a problem hiding this comment.
P3: The new credential-storage workflow has no unit coverage for tenant scoping, vault reuse/creation, TOTP field construction, external failures, or profile-reference persistence. Adding a colocated service spec would catch regressions in this secrets-handling path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-credential-storage.service.ts, line 38:
<comment>The new credential-storage workflow has no unit coverage for tenant scoping, vault reuse/creation, TOTP field construction, external failures, or profile-reference persistence. Adding a colocated service spec would catch regressions in this secrets-handling path.</comment>
<file context>
@@ -0,0 +1,148 @@
+ * item. Comp never persists the raw secrets itself — only the `op://` reference.
+ */
+@Injectable()
+export class BrowserCredentialStorageService {
+ private readonly logger = new Logger(BrowserCredentialStorageService.name);
+
</file context>
Replaces the bare "enter a URL" prompt with the three-step explainer (01 Connect a login / 02 Describe what to capture / 03 Evidence on schedule), a single "Connect a vendor login" action, and the 1Password reassurance line. The URL is now collected in the connect credentials form rather than here. Part of the Browser Automations redesign (first-time setup).
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/BrowserEmptyStates.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/BrowserEmptyStates.tsx:49">
P2: Users without `integration:create` can still see and open this connection flow, then receive a failed request when they submit it because the API rejects `/v1/browserbase/profiles/resolve`. The empty state could use the same permission check as `BrowserAutomationsList` and hide or disable this CTA for users who cannot create integrations.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| </div> | ||
|
|
||
| <div className="mt-6 flex flex-col gap-3 sm:flex-row sm:items-center sm:justify-between"> | ||
| <Button onClick={onConnect} loading={isStartingAuth} disabled={isStartingAuth}> |
Contributor
There was a problem hiding this comment.
P2: Users without integration:create can still see and open this connection flow, then receive a failed request when they submit it because the API rejects /v1/browserbase/profiles/resolve. The empty state could use the same permission check as BrowserAutomationsList and hide or disable this CTA for users who cannot create integrations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/BrowserEmptyStates.tsx, line 49:
<comment>Users without `integration:create` can still see and open this connection flow, then receive a failed request when they submit it because the API rejects `/v1/browserbase/profiles/resolve`. The empty state could use the same permission check as `BrowserAutomationsList` and hide or disable this CTA for users who cannot create integrations.</comment>
<file context>
@@ -1,63 +1,58 @@
- </div>
+
+ <div className="mt-6 flex flex-col gap-3 sm:flex-row sm:items-center sm:justify-between">
+ <Button onClick={onConnect} loading={isStartingAuth} disabled={isStartingAuth}>
+ Connect a vendor login
+ <ArrowRight size={14} />
</file context>
Switches the first-time setup screen to the designer's 2c variant — a split card with the pitch and "Connect a Vendor Login" action on the left and a quiet "How it works" rail (01/02/03) on the right. Stacks to one column on mobile.
Adds a login-analysis backend: POST /v1/browserbase/analyze-login opens the vendor sign-in page in a throwaway cloud browser, detects the supported login methods (password / SSO / passkey, identifier type, extra fields), and returns a recommendation (ready / works-with-check-ins / manual). Reads a public page only — no credentials — and always degrades to a manual fallback if it can't be read. Foundation for the smart-connect flow. Unit tests for the recommendation logic and the analyzer service.
Contributor
There was a problem hiding this comment.
6 issues found across 9 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/browserbase/browserbase.service.ts">
<violation number="1" location="apps/api/src/browserbase/browserbase.service.ts:40">
P3: The new login-analysis API path is not covered at the `BrowserbaseService` boundary, so a broken or missing delegation could pass the current tests even though the analyzer itself is tested. A focused service test (and controller request test) would cover the new endpoint wiring.</violation>
</file>
<file name="apps/api/src/browserbase/dto/browserbase.dto.ts">
<violation number="1" location="apps/api/src/browserbase/dto/browserbase.dto.ts:91">
P3: The generated API contract permits arbitrary method names and generated clients lose the supported-value set. Document this as an enum array to match `LoginMethod` and the values returned by the analyzer.</violation>
<violation number="2" location="apps/api/src/browserbase/dto/browserbase.dto.ts:97">
P3: The `extraFields` property in `LoginAnalysisResponseDto` uses `@ApiProperty({ type: [Object] })`, which renders the nested structure as a generic array of plain objects in the Swagger/OpenAPI spec. Consumers won't see the `{ label: string }` shape. Define a small DTO class (e.g. `ExtraFieldDto`) with a single `@ApiProperty()` on `label` and reference it via `@ApiProperty({ type: [ExtraFieldDto] })` so the generated spec is self-documenting.</violation>
</file>
<file name="apps/api/src/browserbase/browser-login-analysis.ts">
<violation number="1" location="apps/api/src/browserbase/browser-login-analysis.ts:84">
P2: Sites with password forms but SMS, email-code, push approval, or another unsupported MFA method are reported as `ready` even though scheduled re-login will fall back to `needs_reauth`. Since this first-page analysis cannot identify the MFA method, the recommendation should remain check-in/manual until live sign-in proves unattended re-login, or the response should avoid claiming `READY`.</violation>
<violation number="2" location="apps/api/src/browserbase/browser-login-analysis.ts:84">
P2: A reachable password page with a required workspace/company/subdomain field is also labeled `ready`, but that field is not captured or stored for re-login. Including `extraFields.length === 0` in the unattended path, with an appropriate check-in/manual recommendation otherwise, would prevent this false-ready result.</violation>
</file>
<file name="apps/api/src/browserbase/browser-login-analyzer.service.ts">
<violation number="1" location="apps/api/src/browserbase/browser-login-analyzer.service.ts:34">
P2: Every login analysis leaks a persistent Browserbase context: `createSessionWithContext` saves browser data with `persist: true`, while `finally` closes only the session and drops the fresh `contextId`. Repeated analyses can retain vendor cookies/local storage indefinitely and accumulate orphaned contexts; an ephemeral session or explicit context deletion would avoid this.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| if (detection.hasPasskey) detectedMethods.push('passkey'); | ||
|
|
||
| let recommendation: LoginAnalysis['recommendation'] = MANUAL; | ||
| if (detection.reachable && detection.hasPasswordField) { |
Contributor
There was a problem hiding this comment.
P2: A reachable password page with a required workspace/company/subdomain field is also labeled ready, but that field is not captured or stored for re-login. Including extraFields.length === 0 in the unattended path, with an appropriate check-in/manual recommendation otherwise, would prevent this false-ready result.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-login-analysis.ts, line 84:
<comment>A reachable password page with a required workspace/company/subdomain field is also labeled `ready`, but that field is not captured or stored for re-login. Including `extraFields.length === 0` in the unattended path, with an appropriate check-in/manual recommendation otherwise, would prevent this false-ready result.</comment>
<file context>
@@ -0,0 +1,114 @@
+ if (detection.hasPasskey) detectedMethods.push('passkey');
+
+ let recommendation: LoginAnalysis['recommendation'] = MANUAL;
+ if (detection.reachable && detection.hasPasswordField) {
+ // A password form means we can capture credentials + an authenticator key
+ // and run fully unattended (the 2FA specifics surface during sign-in).
</file context>
| if (detection.hasPasskey) detectedMethods.push('passkey'); | ||
|
|
||
| let recommendation: LoginAnalysis['recommendation'] = MANUAL; | ||
| if (detection.reachable && detection.hasPasswordField) { |
Contributor
There was a problem hiding this comment.
P2: Sites with password forms but SMS, email-code, push approval, or another unsupported MFA method are reported as ready even though scheduled re-login will fall back to needs_reauth. Since this first-page analysis cannot identify the MFA method, the recommendation should remain check-in/manual until live sign-in proves unattended re-login, or the response should avoid claiming READY.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-login-analysis.ts, line 84:
<comment>Sites with password forms but SMS, email-code, push approval, or another unsupported MFA method are reported as `ready` even though scheduled re-login will fall back to `needs_reauth`. Since this first-page analysis cannot identify the MFA method, the recommendation should remain check-in/manual until live sign-in proves unattended re-login, or the response should avoid claiming `READY`.</comment>
<file context>
@@ -0,0 +1,114 @@
+ if (detection.hasPasskey) detectedMethods.push('passkey');
+
+ let recommendation: LoginAnalysis['recommendation'] = MANUAL;
+ if (detection.reachable && detection.hasPasswordField) {
+ // A password form means we can capture credentials + an authenticator key
+ // and run fully unattended (the 2FA specifics surface during sign-in).
</file context>
| ) {} | ||
|
|
||
| async analyzeLogin(url: string): Promise<LoginAnalysis> { | ||
| const contextId = await this.sessions.createBrowserbaseContext(); |
Contributor
There was a problem hiding this comment.
P2: Every login analysis leaks a persistent Browserbase context: createSessionWithContext saves browser data with persist: true, while finally closes only the session and drops the fresh contextId. Repeated analyses can retain vendor cookies/local storage indefinitely and accumulate orphaned contexts; an ephemeral session or explicit context deletion would avoid this.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-login-analyzer.service.ts, line 34:
<comment>Every login analysis leaks a persistent Browserbase context: `createSessionWithContext` saves browser data with `persist: true`, while `finally` closes only the session and drops the fresh `contextId`. Repeated analyses can retain vendor cookies/local storage indefinitely and accumulate orphaned contexts; an ephemeral session or explicit context deletion would avoid this.</comment>
<file context>
@@ -0,0 +1,66 @@
+ ) {}
+
+ async analyzeLogin(url: string): Promise<LoginAnalysis> {
+ const contextId = await this.sessions.createBrowserbaseContext();
+ const { sessionId } =
+ await this.sessions.createSessionWithContext(contextId);
</file context>
| @ApiProperty() | ||
| reachable: boolean; | ||
|
|
||
| @ApiProperty({ type: [String] }) |
Contributor
There was a problem hiding this comment.
P3: The generated API contract permits arbitrary method names and generated clients lose the supported-value set. Document this as an enum array to match LoginMethod and the values returned by the analyzer.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/dto/browserbase.dto.ts, line 91:
<comment>The generated API contract permits arbitrary method names and generated clients lose the supported-value set. Document this as an enum array to match `LoginMethod` and the values returned by the analyzer.</comment>
<file context>
@@ -62,10 +62,51 @@ export class AuthStatusResponseDto {
+ @ApiProperty()
+ reachable: boolean;
+
+ @ApiProperty({ type: [String] })
+ detectedMethods: string[];
+
</file context>
Suggested change
| @ApiProperty({ type: [String] }) | |
| @ApiProperty({ enum: ['password', 'sso', 'passkey'], isArray: true }) |
| ) {} | ||
|
|
||
| async analyzeLogin(url: string) { | ||
| return this.loginAnalyzer.analyzeLogin(url); |
Contributor
There was a problem hiding this comment.
P3: The new login-analysis API path is not covered at the BrowserbaseService boundary, so a broken or missing delegation could pass the current tests even though the analyzer itself is tested. A focused service test (and controller request test) would cover the new endpoint wiring.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browserbase.service.ts, line 40:
<comment>The new login-analysis API path is not covered at the `BrowserbaseService` boundary, so a broken or missing delegation could pass the current tests even though the analyzer itself is tested. A focused service test (and controller request test) would cover the new endpoint wiring.</comment>
<file context>
@@ -24,11 +25,21 @@ export class BrowserbaseService {
) {}
+ async analyzeLogin(url: string) {
+ return this.loginAnalyzer.analyzeLogin(url);
+ }
+
</file context>
| @ApiProperty({ enum: ['email', 'username', 'either', 'unknown'] }) | ||
| identifierType: string; | ||
|
|
||
| @ApiProperty({ type: [Object] }) |
Contributor
There was a problem hiding this comment.
P3: The extraFields property in LoginAnalysisResponseDto uses @ApiProperty({ type: [Object] }), which renders the nested structure as a generic array of plain objects in the Swagger/OpenAPI spec. Consumers won't see the { label: string } shape. Define a small DTO class (e.g. ExtraFieldDto) with a single @ApiProperty() on label and reference it via @ApiProperty({ type: [ExtraFieldDto] }) so the generated spec is self-documenting.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/dto/browserbase.dto.ts, line 97:
<comment>The `extraFields` property in `LoginAnalysisResponseDto` uses `@ApiProperty({ type: [Object] })`, which renders the nested structure as a generic array of plain objects in the Swagger/OpenAPI spec. Consumers won't see the `{ label: string }` shape. Define a small DTO class (e.g. `ExtraFieldDto`) with a single `@ApiProperty()` on `label` and reference it via `@ApiProperty({ type: [ExtraFieldDto] })` so the generated spec is self-documenting.</comment>
<file context>
@@ -62,10 +62,51 @@ export class AuthStatusResponseDto {
+ @ApiProperty({ enum: ['email', 'username', 'either', 'unknown'] })
+ identifierType: string;
+
+ @ApiProperty({ type: [Object] })
+ extraFields: { label: string }[];
+
</file context>
Replaces the connect flow with the "Browser-First Split" (1C) design: a stepper rail plus a browser stage that opens the vendor URL, runs AI login-method detection (/analyze-login), shows a recommendation (ready / works-with-check-ins / manual), opens the live sign-in, then captures the reusable credentials (username, password, authenticator setup key) after verification. Removes the superseded credentials-first form. Adds tests for the capture form. The SMS-only -> "enable an authenticator app" recommendation is deferred to the live sign-in step (2FA method isn't visible before sign-in).
Contributor
There was a problem hiding this comment.
3 issues found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/hooks/types.ts">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/hooks/types.ts:67">
P3: Invalid login methods and identifier types can flow through the UI without a compile-time error because this response type widens the API's finite value sets to `string`. Mirroring the API unions here would keep consumers type-checked when the login contract changes.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/ConnectCaptureForm.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/ConnectCaptureForm.tsx:43">
P3: Validation failures are only visually connected to the fields, so screen-reader users may not learn which required credential is invalid. Associate each input with its error via `aria-invalid` and `aria-describedby`, and expose the error as an alert.</violation>
<violation number="2" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/ConnectCaptureForm.tsx:62">
P2: The authenticator setup key is rendered as plaintext, allowing anyone viewing this capture screen to copy the MFA seed and generate future codes. Treat this value like the password with a masked input, optionally adding an explicit reveal control.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| <Label htmlFor="capture-totp">Authenticator setup key</Label> | ||
| <span className="text-xs text-muted-foreground">Optional — recommended</span> | ||
| </div> | ||
| <Input id="capture-totp" placeholder="e.g. JBSW Y3DP EHPK 3PXP" {...register('totpSeed')} /> |
Contributor
There was a problem hiding this comment.
P2: The authenticator setup key is rendered as plaintext, allowing anyone viewing this capture screen to copy the MFA seed and generate future codes. Treat this value like the password with a masked input, optionally adding an explicit reveal control.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/ConnectCaptureForm.tsx, line 62:
<comment>The authenticator setup key is rendered as plaintext, allowing anyone viewing this capture screen to copy the MFA seed and generate future codes. Treat this value like the password with a masked input, optionally adding an explicit reveal control.</comment>
<file context>
@@ -0,0 +1,79 @@
+ <Label htmlFor="capture-totp">Authenticator setup key</Label>
+ <span className="text-xs text-muted-foreground">Optional — recommended</span>
+ </div>
+ <Input id="capture-totp" placeholder="e.g. JBSW Y3DP EHPK 3PXP" {...register('totpSeed')} />
+ <p className="text-xs text-muted-foreground leading-relaxed">
+ Shown when you set up the authenticator app (“can't scan? enter this
</file context>
Comment on lines
+67
to
+68
| detectedMethods: string[]; | ||
| identifierType: string; |
Contributor
There was a problem hiding this comment.
P3: Invalid login methods and identifier types can flow through the UI without a compile-time error because this response type widens the API's finite value sets to string. Mirroring the API unions here would keep consumers type-checked when the login contract changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/hooks/types.ts, line 67:
<comment>Invalid login methods and identifier types can flow through the UI without a compile-time error because this response type widens the API's finite value sets to `string`. Mirroring the API unions here would keep consumers type-checked when the login contract changes.</comment>
<file context>
@@ -60,6 +60,20 @@ export interface BrowserLoginCredentials {
+
+export interface LoginAnalysis {
+ reachable: boolean;
+ detectedMethods: string[];
+ identifierType: string;
+ extraFields: { label: string }[];
</file context>
Suggested change
| detectedMethods: string[]; | |
| identifierType: string; | |
| detectedMethods: ('password' | 'sso' | 'passkey')[]; | |
| identifierType: 'email' | 'username' | 'either' | 'unknown'; |
|
|
||
| <div className="flex flex-col gap-2"> | ||
| <Label htmlFor="capture-username">Username or email</Label> | ||
| <Input id="capture-username" autoComplete="off" {...register('username')} /> |
Contributor
There was a problem hiding this comment.
P3: Validation failures are only visually connected to the fields, so screen-reader users may not learn which required credential is invalid. Associate each input with its error via aria-invalid and aria-describedby, and expose the error as an alert.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/ConnectCaptureForm.tsx, line 43:
<comment>Validation failures are only visually connected to the fields, so screen-reader users may not learn which required credential is invalid. Associate each input with its error via `aria-invalid` and `aria-describedby`, and expose the error as an alert.</comment>
<file context>
@@ -0,0 +1,79 @@
+
+ <div className="flex flex-col gap-2">
+ <Label htmlFor="capture-username">Username or email</Label>
+ <Input id="capture-username" autoComplete="off" {...register('username')} />
+ {errors.username?.message && (
+ <p className="text-xs text-destructive">{errors.username.message}</p>
</file context>
Rewrites the automations list to group automations under their vendor connection. Each connection shows its status (Connected / Needs reconnect / Needs your action / Not connected) with a one-click Reconnect for the ones that need a human. Adds a useBrowserProfiles hook to load connection statuses and wires reconnect through the existing live sign-in.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Confidence score: 3/5
- In
apps/api/src/browserbase/browser-evidence-page.ts, the tab-selection logic can pick the newest unrelated non-initial tab when no page matchestargetHostname, so evidence may be captured from the wrong page and misattributed to the run — tighten selection to the actual navigation/page context (or fail explicitly when no verified match exists).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/browserbase/browser-evidence-page.ts">
<violation number="1" location="apps/api/src/browserbase/browser-evidence-page.ts:65">
P2: Evidence capture can select an unrelated open tab whenever it is the newest non-initial page and no non-initial page matches `targetHostname`; the selector does not establish that this tab is the page used by the navigation instruction. Tracking the agent's final/active page (or retaining a validated candidate set) would avoid returning a stale popup as evidence.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| // (e.g. AWS console vs the sign-in host), commonly in a new tab. Prefer the | ||
| // newest non-initial page over the (now stale) initial one, so the screenshot | ||
| // matches where the agent actually ended up — not the starting/homepage tab. | ||
| const newestNonInitial = reversed.find((page) => page !== initialPage); |
Contributor
There was a problem hiding this comment.
P2: Evidence capture can select an unrelated open tab whenever it is the newest non-initial page and no non-initial page matches targetHostname; the selector does not establish that this tab is the page used by the navigation instruction. Tracking the agent's final/active page (or retaining a validated candidate set) would avoid returning a stale popup as evidence.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-evidence-page.ts, line 65:
<comment>Evidence capture can select an unrelated open tab whenever it is the newest non-initial page and no non-initial page matches `targetHostname`; the selector does not establish that this tab is the page used by the navigation instruction. Tracking the agent's final/active page (or retaining a validated candidate set) would avoid returning a stale popup as evidence.</comment>
<file context>
@@ -50,15 +50,22 @@ export function selectEvidencePage<Page extends EvidencePageCandidate>({
+ // (e.g. AWS console vs the sign-in host), commonly in a new tab. Prefer the
+ // newest non-initial page over the (now stale) initial one, so the screenshot
+ // matches where the agent actually ended up — not the starting/homepage tab.
+ const newestNonInitial = reversed.find((page) => page !== initialPage);
+ if (newestNonInitial) return newestNonInitial;
</file context>
Users must give Comp AI the authenticator setup key (TOTP seed), not the 6-digit code, for unattended scheduled sign-ins — but they rarely know where to find it. Add AI-generated, per-vendor instructions (no hardcode): - BrowserMfaInstructionsService: generateObject (Sonnet) + strict schema with a confidence flag; confidence-gated to a single universal fallback so we never show shaky invented steps. In-memory per-hostname TTL cache (1 day) — public, cheap-to-regenerate guidance, so no DB migration; a shared DB cache is a drop-in swap behind getInstructions. Web-search grounding is the next step. - GET /v1/browserbase/mfa-instructions?host=... (integration:read). - MfaSetupHelp: lazy 'How do I find this key?' collapsible under the setup-key field, shown in both the task and settings connect flows. - Manage panel 'add 2FA later': 2FA checkbox + setup-key field + helper in Change login, reusing the credentials endpoint that already stores the TOTP seed. Tests: api 4, app 6 (+ existing browser suites green: api 140, app 97).
…e docs Before generating the per-vendor authenticator instructions, pull the vendor's current help docs via Firecrawl web search (v2/search) and feed them to the model, so steps track the live UI instead of the model's training snapshot. - Best-effort: no FIRECRAWL_API_KEY, a failed/timed-out request, or empty results all fall back to ungrounded generation — grounding never blocks, and the confidence gate still protects the output. - Grounding runs inside generate(), so the per-hostname cache means Firecrawl is hit at most once per vendor per day. - New 'grounded' flag flows to the UI: MfaSetupHelp shows a subtle 'Checked against this vendor's current help docs' trust line when true. Tests: api +2 (grounded path, Firecrawl-failure path), app +1 (trust line). Suites green: api 142, app 98.
Contributor
There was a problem hiding this comment.
9 issues found across 11 files (changes from recent commits).
Confidence score: 2/5
- In
apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ManageConnectionSheet.tsx, the TOTP setup secret can persist when reopening the same connection and is displayed in plain text, which risks exposing or reusing a sensitive authenticator key across sessions — reset credential state on sheet close/reopen and mask the field by default. - In
apps/api/src/browserbase/browser-mfa-instructions.service.ts, untrusted search content is treated as instruction authority and the Firecrawl request shape can disable grounding, so users may receive manipulated or hallucinated MFA steps that lead to lockouts or unsafe actions — restrict sources to verified vendor docs and fix the v2 format contract so grounded content is required. - In
apps/api/src/browserbase/browser-auth-profiles.controller.ts, allowing arbitrary unboundedhostvalues withintegration:readenables repeated model generation and cache growth, creating a practical abuse path for cost and resource exhaustion — enforce strict hostname/URL validation with bounds and add throttling/quota controls. - In
apps/api/src/browserbase/browser-mfa-instructions.service.ts, cache entries are never evicted after TTL and step-count limits are only prompt-level, so memory and response size can grow beyond intended limits under varied traffic — add bounded eviction (LRU or periodic sweep) and enforce the 3–7 range in the Zod schema.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/browserbase/browser-auth-profiles.controller.ts">
<violation number="1" location="apps/api/src/browserbase/browser-auth-profiles.controller.ts:55">
P3: The new route's 400 handling, service delegation, and RBAC metadata can regress without a failing test. A controller test covering a missing/blank `host`, a valid request, and the `integration:read` requirement would make this endpoint's contract executable.</violation>
<violation number="2" location="apps/api/src/browserbase/browser-auth-profiles.controller.ts:66">
P3: OpenAPI consumers will not receive a usable schema for the new 200 response, so generated clients and API docs cannot discover `hostname`, `steps`, `tips`, or the status fields. Declaring a response DTO or an explicit Swagger object schema would keep the endpoint contract visible.</violation>
<violation number="3" location="apps/api/src/browserbase/browser-auth-profiles.controller.ts:73">
P2: This endpoint lets any caller with `integration:read` submit unlimited distinct host values, causing a model generation and a permanent cache entry for each one. Validating the input as a bounded hostname/URL and adding request/resource limits or a bounded shared cache would prevent an authenticated user from turning this into memory and AI-cost exhaustion.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ManageConnectionSheet.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ManageConnectionSheet.tsx:80">
P2: Closing and reopening the same connection can repopulate the previous TOTP setup key because the reset runs only when `connection` changes, while the parent retains that connection when the sheet closes. Reset credential state on the sheet's close/open transition so a previous secret is not reused or shown to the next edit.</violation>
<violation number="2" location="apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ManageConnectionSheet.tsx:244">
P2: The authenticator setup key is rendered as a plain text input, making the reusable TOTP secret visible on screen. Conceal this field with `type="password"` (with an explicit reveal affordance only if users need to verify it).</violation>
</file>
<file name="apps/api/src/browserbase/browser-mfa-instructions.service.ts">
<violation number="1" location="apps/api/src/browserbase/browser-mfa-instructions.service.ts:42">
P3: The 3–7 step limit is only prompt metadata, not validation, so responses outside the intended range are returned unchanged. Enforcing the range in the Zod schema would keep the generated instruction payload bounded and consistent with the contract.</violation>
<violation number="2" location="apps/api/src/browserbase/browser-mfa-instructions.service.ts:110">
P2: Retrieved web content is treated as trusted instructions rather than untrusted reference material, allowing a malicious search result to influence the MFA steps shown to users. Restrict results to verified vendor documentation where possible and explicitly isolate/ignore instructions in retrieved content before generating user-facing steps.</violation>
<violation number="3" location="apps/api/src/browserbase/browser-mfa-instructions.service.ts:148">
P2: The TTL cache leaks one entry for every distinct hostname because expired entries are never evicted, so repeated requests for many hosts can grow API-process memory without bound. A bounded LRU or periodic expired-entry cleanup would keep this singleton cache bounded.</violation>
<violation number="4" location="apps/api/src/browserbase/browser-mfa-instructions.service.ts:220">
P2: Grounding is effectively disabled with the documented Firecrawl v2 request shape: the search call uses string formats instead of format objects, so `result.markdown` may be absent and the service falls back to ungrounded model output. Using the documented format object would make the current-help-docs path work.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| if (!host?.trim()) { | ||
| throw new BadRequestException('host query parameter is required'); | ||
| } | ||
| return this.mfaInstructionsService.getInstructions(host.trim()); |
Contributor
There was a problem hiding this comment.
P2: This endpoint lets any caller with integration:read submit unlimited distinct host values, causing a model generation and a permanent cache entry for each one. Validating the input as a bounded hostname/URL and adding request/resource limits or a bounded shared cache would prevent an authenticated user from turning this into memory and AI-cost exhaustion.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-auth-profiles.controller.ts, line 73:
<comment>This endpoint lets any caller with `integration:read` submit unlimited distinct host values, causing a model generation and a permanent cache entry for each one. Validating the input as a bounded hostname/URL and adding request/resource limits or a bounded shared cache would prevent an authenticated user from turning this into memory and AI-cost exhaustion.</comment>
<file context>
@@ -40,7 +47,31 @@ import {
+ if (!host?.trim()) {
+ throw new BadRequestException('host query parameter is required');
+ }
+ return this.mfaInstructionsService.getInstructions(host.trim());
+ }
</file context>
| setUsername(''); | ||
| setPassword(''); | ||
| setUse2fa(false); | ||
| setTotpSeed(''); |
Contributor
There was a problem hiding this comment.
P2: Closing and reopening the same connection can repopulate the previous TOTP setup key because the reset runs only when connection changes, while the parent retains that connection when the sheet closes. Reset credential state on the sheet's close/open transition so a previous secret is not reused or shown to the next edit.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ManageConnectionSheet.tsx, line 80:
<comment>Closing and reopening the same connection can repopulate the previous TOTP setup key because the reset runs only when `connection` changes, while the parent retains that connection when the sheet closes. Reset credential state on the sheet's close/open transition so a previous secret is not reused or shown to the next edit.</comment>
<file context>
@@ -73,6 +76,8 @@ export function ManageConnectionSheet({
setUsername('');
setPassword('');
+ setUse2fa(false);
+ setTotpSeed('');
setConfirmingRemove(false);
}, [connection]);
</file context>
| {use2fa && ( | ||
| <div className="flex flex-col gap-2"> | ||
| <Input | ||
| value={totpSeed} |
Contributor
There was a problem hiding this comment.
P2: The authenticator setup key is rendered as a plain text input, making the reusable TOTP secret visible on screen. Conceal this field with type="password" (with an explicit reveal affordance only if users need to verify it).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ManageConnectionSheet.tsx, line 244:
<comment>The authenticator setup key is rendered as a plain text input, making the reusable TOTP secret visible on screen. Conceal this field with `type="password"` (with an explicit reveal affordance only if users need to verify it).</comment>
<file context>
@@ -220,13 +226,52 @@ export function ManageConnectionSheet({
+ {use2fa && (
+ <div className="flex flex-col gap-2">
+ <Input
+ value={totpSeed}
+ onChange={(event) => setTotpSeed(event.target.value)}
+ placeholder="Authenticator setup key (e.g. JBSW Y3DP EHPK 3PXP)"
</file context>
| query: `${hostname} set up an authenticator app (TOTP) for two-factor authentication and reveal the manual setup key / secret key`, | ||
| limit: GROUNDING_RESULT_LIMIT, | ||
| sources: [{ type: 'web' }], | ||
| scrapeOptions: { formats: ['markdown'], onlyMainContent: true }, |
Contributor
There was a problem hiding this comment.
P2: Grounding is effectively disabled with the documented Firecrawl v2 request shape: the search call uses string formats instead of format objects, so result.markdown may be absent and the service falls back to ungrounded model output. Using the documented format object would make the current-help-docs path work.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-mfa-instructions.service.ts, line 220:
<comment>Grounding is effectively disabled with the documented Firecrawl v2 request shape: the search call uses string formats instead of format objects, so `result.markdown` may be absent and the service falls back to ungrounded model output. Using the documented format object would make the current-help-docs path work.</comment>
<file context>
@@ -0,0 +1,261 @@
+ query: `${hostname} set up an authenticator app (TOTP) for two-factor authentication and reveal the manual setup key / secret key`,
+ limit: GROUNDING_RESULT_LIMIT,
+ sources: [{ type: 'web' }],
+ scrapeOptions: { formats: ['markdown'], onlyMainContent: true },
+ }),
+ signal: controller.signal,
</file context>
|
|
||
| CURRENT VENDOR DOCS (from a live web search — treat as the source of truth for the vendor's current UI): | ||
| """ | ||
| ${grounding} |
Contributor
There was a problem hiding this comment.
P2: Retrieved web content is treated as trusted instructions rather than untrusted reference material, allowing a malicious search result to influence the MFA steps shown to users. Restrict results to verified vendor documentation where possible and explicitly isolate/ignore instructions in retrieved content before generating user-facing steps.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-mfa-instructions.service.ts, line 110:
<comment>Retrieved web content is treated as trusted instructions rather than untrusted reference material, allowing a malicious search result to influence the MFA steps shown to users. Restrict results to verified vendor documentation where possible and explicitly isolate/ignore instructions in retrieved content before generating user-facing steps.</comment>
<file context>
@@ -0,0 +1,261 @@
+
+CURRENT VENDOR DOCS (from a live web search — treat as the source of truth for the vendor's current UI):
+"""
+${grounding}
+"""
+
</file context>
| value = this.fallback(hostname); | ||
| } | ||
|
|
||
| this.cache.set(hostname, { value, expiresAt: Date.now() + CACHE_TTL_MS }); |
Contributor
There was a problem hiding this comment.
P2: The TTL cache leaks one entry for every distinct hostname because expired entries are never evicted, so repeated requests for many hosts can grow API-process memory without bound. A bounded LRU or periodic expired-entry cleanup would keep this singleton cache bounded.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-mfa-instructions.service.ts, line 148:
<comment>The TTL cache leaks one entry for every distinct hostname because expired entries are never evicted, so repeated requests for many hosts can grow API-process memory without bound. A bounded LRU or periodic expired-entry cleanup would keep this singleton cache bounded.</comment>
<file context>
@@ -0,0 +1,261 @@
+ value = this.fallback(hostname);
+ }
+
+ this.cache.set(hostname, { value, expiresAt: Date.now() + CACHE_TTL_MS });
+ return value;
+ }
</file context>
| name: 'host', | ||
| description: 'The vendor sign-in URL or hostname (e.g. github.com).', | ||
| }) | ||
| @ApiResponse({ status: 200 }) |
Contributor
There was a problem hiding this comment.
P3: OpenAPI consumers will not receive a usable schema for the new 200 response, so generated clients and API docs cannot discover hostname, steps, tips, or the status fields. Declaring a response DTO or an explicit Swagger object schema would keep the endpoint contract visible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-auth-profiles.controller.ts, line 66:
<comment>OpenAPI consumers will not receive a usable schema for the new 200 response, so generated clients and API docs cannot discover `hostname`, `steps`, `tips`, or the status fields. Declaring a response DTO or an explicit Swagger object schema would keep the endpoint contract visible.</comment>
<file context>
@@ -40,7 +47,31 @@ import {
+ name: 'host',
+ description: 'The vendor sign-in URL or hostname (e.g. github.com).',
+ })
+ @ApiResponse({ status: 200 })
+ async getMfaInstructions(
+ @Query('host') host?: string,
</file context>
Suggested change
| @ApiResponse({ status: 200 }) | |
| @ApiResponse({ | |
| status: 200, | |
| schema: { | |
| type: 'object', | |
| required: ['hostname', 'steps', 'tips', 'confident', 'grounded', 'source'], | |
| properties: { | |
| hostname: { type: 'string' }, | |
| steps: { type: 'array', items: { type: 'string' } }, | |
| tips: { type: 'array', items: { type: 'string' } }, | |
| confident: { type: 'boolean' }, | |
| grounded: { type: 'boolean' }, | |
| source: { type: 'string', enum: ['generated', 'fallback'] }, | |
| }, | |
| }, | |
| }) |
| private readonly mfaInstructionsService: BrowserMfaInstructionsService, | ||
| ) {} | ||
|
|
||
| @Get('mfa-instructions') |
Contributor
There was a problem hiding this comment.
P3: The new route's 400 handling, service delegation, and RBAC metadata can regress without a failing test. A controller test covering a missing/blank host, a valid request, and the integration:read requirement would make this endpoint's contract executable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-auth-profiles.controller.ts, line 55:
<comment>The new route's 400 handling, service delegation, and RBAC metadata can regress without a failing test. A controller test covering a missing/blank `host`, a valid request, and the `integration:read` requirement would make this endpoint's contract executable.</comment>
<file context>
@@ -40,7 +47,31 @@ import {
+ private readonly mfaInstructionsService: BrowserMfaInstructionsService,
+ ) {}
+
+ @Get('mfa-instructions')
+ @RequirePermission('integration', 'read')
+ @ApiOperation({
</file context>
|
|
||
| const instructionSchema = z.object({ | ||
| steps: z | ||
| .array(z.string().min(1)) |
Contributor
There was a problem hiding this comment.
P3: The 3–7 step limit is only prompt metadata, not validation, so responses outside the intended range are returned unchanged. Enforcing the range in the Zod schema would keep the generated instruction payload bounded and consistent with the contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-mfa-instructions.service.ts, line 42:
<comment>The 3–7 step limit is only prompt metadata, not validation, so responses outside the intended range are returned unchanged. Enforcing the range in the Zod schema would keep the generated instruction payload bounded and consistent with the contract.</comment>
<file context>
@@ -0,0 +1,261 @@
+
+const instructionSchema = z.object({
+ steps: z
+ .array(z.string().min(1))
+ .describe(
+ 'Ordered, concrete steps for a user of THIS vendor to add a NEW authenticator (TOTP) app and reveal its manual "setup key" / "secret key". Each step is one short action. 3-7 steps.',
</file context>
Surface 1 (2a — quiet disclosure): the 'How do I find this key?' helper is now a muted link-row that opens a soft panel with numbered per-vendor steps and a single footer line — a green 'Checked against <vendor>'s current help docs · <date>' trust line when grounded, or an honest tinted generic-fallback note. Loading and error (with retry) states included. Adds a checkedAt timestamp to the API payload. Surface 2 (2d — Automatic 2FA row): 'add 2FA later' moves out of Change login into its own row in the Manage panel's DETAILS group with an ON / NOT SET UP pill. 'Add authenticator key' stores ONLY the TOTP seed — no username/password re-entry: - New credential-storage methods read/attach/remove just the vault item's TOTP field (1Password items.get + put); status is read live from the vault, so no DB flag can drift and no migration is needed. - Endpoints: GET/POST/DELETE /v1/browserbase/profiles/:id/totp (integration read/update). useTotpStatus hook drives the pill; Replace / Turn off when on. Tests: api +7 (storage TOTP status/set/clear), app 2FA-row + 2a states green. Suites: api 149, app 100.
Contributor
There was a problem hiding this comment.
9 issues found across 12 files (changes from recent commits).
Confidence score: 2/5
- In
apps/api/src/browserbase/browser-auth-profiles.controller.ts, the endpoint can be driven with an arbitrary vault item reference, letting a user cause the service account to read/overwrite a different 1Password item than the connection’s managed item; this is the highest-risk integrity/security gap—enforce server-side binding to the connection-owned vault item and reject mismatched references. - Across
apps/app/.../ManageConnectionSheet.tsxandapps/api/src/browserbase/browser-credential-storage.service.ts, credential updates and 2FA disable flows can report success while leaving TOTP state wrong (setup key dropped on login change, or seed not removed when 1Password is unavailable), which can silently disable or unexpectedly retain Automatic 2FA—preserve existing TOTP fields on replacement and fail the operation when credential-store writes/deletes cannot complete. - In
apps/app/.../BrowserConnectionClient.tsx, enable/disable 2FA toasts can show success even when POST/DELETE calls return error responses, so users are told settings changed when they did not—checkresponse.errorbefore success messaging and surface failure states. - In
apps/app/.../hooks/useTotpStatus.ts, failed status reads are treated like “2FA off,” which can drive incorrect setup guidance in the Manage panel—propagate and handle fetch errors distinctly from a true disabled state.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/hooks/useTotpStatus.ts">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/hooks/useTotpStatus.ts:29">
P2: A failed status read is currently indistinguishable from a connection without 2FA, so the Manage panel can tell users that Automatic 2FA is off and offer setup after the GET failed. Returning the SWR error (and handling it as an unknown/error state in the panel) would avoid presenting a false vault status.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/BrowserConnectionClient.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/BrowserConnectionClient.tsx:127">
P2: Automatic 2FA is reported as enabled even when the setup-key request fails: `apiClient.post` returns an error response instead of throwing, and the result is ignored here. Checking `response.error` before showing the success toast would prevent the UI from claiming the key was saved when the vault write was rejected.</violation>
<violation number="2" location="apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/BrowserConnectionClient.tsx:139">
P2: Turning off Automatic 2FA is reported as successful even when the delete request returns 4xx/5xx because the `apiClient.delete` response is ignored. Checking `response.error` before the success toast would keep the status and user feedback accurate when the vault update fails.</violation>
</file>
<file name="apps/api/src/browserbase/browser-auth-profiles.controller.ts">
<violation number="1" location="apps/api/src/browserbase/browser-auth-profiles.controller.ts:206">
P1: A user can create a connection carrying an arbitrary vault item reference and then use this endpoint to make the service account read and overwrite that referenced 1Password item, rather than only the connection's managed login. The storage service should validate the provider and organization-owned vault/item reference before exposing TOTP read/write operations.</violation>
</file>
<file name="apps/api/src/browserbase/browser-credential-storage.service.ts">
<violation number="1" location="apps/api/src/browserbase/browser-credential-storage.service.ts:105">
P2: Turning off automatic 2FA appears successful when 1Password is unavailable, but the stored TOTP seed is not removed and later runs can still use it. Treat an unavailable credential store as `ServiceUnavailableException` here, matching `setProfileTotp`, rather than returning a false status.</violation>
<violation number="2" location="apps/api/src/browserbase/browser-credential-storage.service.ts:203">
P3: This service now exceeds the API's 300-line file limit (309 lines), increasing the maintenance cost of an already multi-responsibility class. Moving the TOTP operations into a focused service would keep the credential-storage service within the repository convention.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/MfaSetupHelp.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/MfaSetupHelp.tsx:43">
P2: Screen readers receive the setup instructions as unrelated generic elements even though the UI presents an ordered sequence, so users of assistive technology lose the list/step structure. Rendering this block with `<ol>` and `<li>` while preserving the custom counter would retain the current visual design and expose the correct semantics.</violation>
<violation number="2" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/MfaSetupHelp.tsx:50">
P3: The new help, chevron, and check glyphs bypass the repository's required `@trycompai/design-system/icons` exports, creating duplicate icon path definitions and inconsistent UI maintenance. Reusing the shared `Information`, `ChevronDown`, and `Checkmark` icons would align this component with the app's design-system convention.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ManageConnectionSheet.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ManageConnectionSheet.tsx:32">
P1: Changing the login now silently disables Automatic 2FA: the credentials endpoint creates a replacement vault item without carrying forward the existing setup key. Preserving the existing TOTP field server-side (or requiring/reapplying it during this workflow) would keep scheduled runs from unexpectedly falling back to manual codes.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| }); | ||
| } | ||
|
|
||
| @Post('profiles/:profileId/totp') |
Contributor
There was a problem hiding this comment.
P1: A user can create a connection carrying an arbitrary vault item reference and then use this endpoint to make the service account read and overwrite that referenced 1Password item, rather than only the connection's managed login. The storage service should validate the provider and organization-owned vault/item reference before exposing TOTP read/write operations.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-auth-profiles.controller.ts, line 206:
<comment>A user can create a connection carrying an arbitrary vault item reference and then use this endpoint to make the service account read and overwrite that referenced 1Password item, rather than only the connection's managed login. The storage service should validate the provider and organization-owned vault/item reference before exposing TOTP read/write operations.</comment>
<file context>
@@ -181,6 +184,66 @@ export class BrowserAuthProfilesController {
+ });
+ }
+
+ @Post('profiles/:profileId/totp')
+ @RequirePermission('integration', 'update')
+ @ApiOperation({
</file context>
| onRename: (connection: Connection, name: string) => Promise<void> | void; | ||
| onChangeLogin: ( | ||
| connection: Connection, | ||
| creds: { username: string; password: string }, |
Contributor
There was a problem hiding this comment.
P1: Changing the login now silently disables Automatic 2FA: the credentials endpoint creates a replacement vault item without carrying forward the existing setup key. Preserving the existing TOTP field server-side (or requiring/reapplying it during this workflow) would keep scheduled runs from unexpectedly falling back to manual codes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ManageConnectionSheet.tsx, line 32:
<comment>Changing the login now silently disables Automatic 2FA: the credentials endpoint creates a replacement vault item without carrying forward the existing setup key. Preserving the existing TOTP field server-side (or requiring/reapplying it during this workflow) would keep scheduled runs from unexpectedly falling back to manual codes.</comment>
<file context>
@@ -28,8 +29,10 @@ interface ManageConnectionSheetProps {
onChangeLogin: (
connection: Connection,
- creds: { username: string; password: string; totpSeed?: string },
+ creds: { username: string; password: string },
) => Promise<void> | void;
+ onSetTotp: (connection: Connection, totpSeed: string) => Promise<void> | void;
</file context>
| ); | ||
|
|
||
| return { | ||
| configured: data?.configured ?? false, |
Contributor
There was a problem hiding this comment.
P2: A failed status read is currently indistinguishable from a connection without 2FA, so the Manage panel can tell users that Automatic 2FA is off and offer setup after the GET failed. Returning the SWR error (and handling it as an unknown/error state in the panel) would avoid presenting a false vault status.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/hooks/useTotpStatus.ts, line 29:
<comment>A failed status read is currently indistinguishable from a connection without 2FA, so the Manage panel can tell users that Automatic 2FA is off and offer setup after the GET failed. Returning the SWR error (and handling it as an unknown/error state in the panel) would avoid presenting a false vault status.</comment>
<file context>
@@ -0,0 +1,33 @@
+ );
+
+ return {
+ configured: data?.configured ?? false,
+ isLoading: enabled && isLoading,
+ mutate,
</file context>
| const handleClearTotp = useCallback(async (connection: Connection) => { | ||
| setBusy(true); | ||
| try { | ||
| await apiClient.delete(`/v1/browserbase/profiles/${connection.id}/totp`); |
Contributor
There was a problem hiding this comment.
P2: Turning off Automatic 2FA is reported as successful even when the delete request returns 4xx/5xx because the apiClient.delete response is ignored. Checking response.error before the success toast would keep the status and user feedback accurate when the vault update fails.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/BrowserConnectionClient.tsx, line 139:
<comment>Turning off Automatic 2FA is reported as successful even when the delete request returns 4xx/5xx because the `apiClient.delete` response is ignored. Checking `response.error` before the success toast would keep the status and user feedback accurate when the vault update fails.</comment>
<file context>
@@ -124,6 +121,30 @@ export function BrowserConnectionClient({
+ const handleClearTotp = useCallback(async (connection: Connection) => {
+ setBusy(true);
+ try {
+ await apiClient.delete(`/v1/browserbase/profiles/${connection.id}/totp`);
+ toast.success('Automatic 2FA turned off.');
+ } catch (err) {
</file context>
| const handleSetTotp = useCallback(async (connection: Connection, totpSeed: string) => { | ||
| setBusy(true); | ||
| try { | ||
| await apiClient.post(`/v1/browserbase/profiles/${connection.id}/totp`, { totpSeed }); |
Contributor
There was a problem hiding this comment.
P2: Automatic 2FA is reported as enabled even when the setup-key request fails: apiClient.post returns an error response instead of throwing, and the result is ignored here. Checking response.error before showing the success toast would prevent the UI from claiming the key was saved when the vault write was rejected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/BrowserConnectionClient.tsx, line 127:
<comment>Automatic 2FA is reported as enabled even when the setup-key request fails: `apiClient.post` returns an error response instead of throwing, and the result is ignored here. Checking `response.error` before showing the success toast would prevent the UI from claiming the key was saved when the vault write was rejected.</comment>
<file context>
@@ -124,6 +121,30 @@ export function BrowserConnectionClient({
+ const handleSetTotp = useCallback(async (connection: Connection, totpSeed: string) => {
+ setBusy(true);
+ try {
+ await apiClient.post(`/v1/browserbase/profiles/${connection.id}/totp`, { totpSeed });
+ toast.success('Automatic 2FA is on. Scheduled runs generate the code for you.');
+ } catch (err) {
</file context>
| organizationId: string; | ||
| profileId: string; | ||
| }): Promise<{ configured: boolean }> { | ||
| if (!isOnePasswordConfigured()) return { configured: false }; |
Contributor
There was a problem hiding this comment.
P2: Turning off automatic 2FA appears successful when 1Password is unavailable, but the stored TOTP seed is not removed and later runs can still use it. Treat an unavailable credential store as ServiceUnavailableException here, matching setProfileTotp, rather than returning a false status.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-credential-storage.service.ts, line 105:
<comment>Turning off automatic 2FA appears successful when 1Password is unavailable, but the stored TOTP seed is not removed and later runs can still use it. Treat an unavailable credential store as `ServiceUnavailableException` here, matching `setProfileTotp`, rather than returning a false status.</comment>
<file context>
@@ -91,6 +92,127 @@ export class BrowserCredentialStorageService {
+ organizationId: string;
+ profileId: string;
+ }): Promise<{ configured: boolean }> {
+ if (!isOnePasswordConfigured()) return { configured: false };
+
+ const profile = await this.findProfile(input);
</file context>
| const checkedOn = instructions ? formatCheckedAt(instructions.checkedAt) : null; | ||
|
|
||
| return ( | ||
| <div className="flex flex-col gap-2"> |
Contributor
There was a problem hiding this comment.
P2: Screen readers receive the setup instructions as unrelated generic elements even though the UI presents an ordered sequence, so users of assistive technology lose the list/step structure. Rendering this block with <ol> and <li> while preserving the custom counter would retain the current visual design and expose the correct semantics.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/MfaSetupHelp.tsx, line 43:
<comment>Screen readers receive the setup instructions as unrelated generic elements even though the UI presents an ordered sequence, so users of assistive technology lose the list/step structure. Rendering this block with `<ol>` and `<li>` while preserving the custom counter would retain the current visual design and expose the correct semantics.</comment>
<file context>
@@ -8,72 +8,160 @@ interface MfaSetupHelpProps {
- ))}
- </ol>
+ return (
+ <div className="flex flex-col gap-2">
+ <button
+ type="button"
</file context>
| return { configured: false }; | ||
| } | ||
|
|
||
| private async findProfile(input: { |
Contributor
There was a problem hiding this comment.
P3: This service now exceeds the API's 300-line file limit (309 lines), increasing the maintenance cost of an already multi-responsibility class. Moving the TOTP operations into a focused service would keep the credential-storage service within the repository convention.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-credential-storage.service.ts, line 203:
<comment>This service now exceeds the API's 300-line file limit (309 lines), increasing the maintenance cost of an already multi-responsibility class. Moving the TOTP operations into a focused service would keep the credential-storage service within the repository convention.</comment>
<file context>
@@ -91,6 +92,127 @@ export class BrowserCredentialStorageService {
+ return { configured: false };
+ }
+
+ private async findProfile(input: {
+ organizationId: string;
+ profileId: string;
</file context>
| aria-expanded={open} | ||
| className="inline-flex items-center gap-1.5 self-start text-[11px] text-muted-foreground transition-colors hover:text-foreground" | ||
| > | ||
| <svg |
Contributor
There was a problem hiding this comment.
P3: The new help, chevron, and check glyphs bypass the repository's required @trycompai/design-system/icons exports, creating duplicate icon path definitions and inconsistent UI maintenance. Reusing the shared Information, ChevronDown, and Checkmark icons would align this component with the app's design-system convention.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/MfaSetupHelp.tsx, line 50:
<comment>The new help, chevron, and check glyphs bypass the repository's required `@trycompai/design-system/icons` exports, creating duplicate icon path definitions and inconsistent UI maintenance. Reusing the shared `Information`, `ChevronDown`, and `Checkmark` icons would align this component with the app's design-system convention.</comment>
<file context>
@@ -8,72 +8,160 @@ interface MfaSetupHelpProps {
+ aria-expanded={open}
+ className="inline-flex items-center gap-1.5 self-start text-[11px] text-muted-foreground transition-colors hover:text-foreground"
+ >
+ <svg
+ width="12"
+ height="12"
</file context>
…idance - The model sometimes returns markdown bold (**Settings**); we render raw text, so the asterisks showed literally. Strip **/__/backticks in the service and tell the model to write plain text — the 2a design shows unstyled steps. - Prefetch the per-vendor guidance while the user fills in the connect form, so the 'How do I find this key?' helper opens instantly. Bounded by the per-host cache, so this just front-loads the once-per-vendor-per-day generation and warms it for the next person connecting the same vendor. Tests: api +1 (markdown strip). Suites: api 150, app 100.
Contributor
There was a problem hiding this comment.
3 issues found across 3 files (changes from recent commits).
Confidence score: 3/5
- In
apps/api/src/browserbase/browser-mfa-instructions.service.ts, responses that are technically non-empty before cleanup can become empty after cleanup and still returnsteps: [], which can leave users with no MFA guidance instead of the universal fallback — validate the cleaned steps and fall back when the result is empty. - In
apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/ConnectCaptureForm.tsx, capture form open currently triggers Firecrawl/AI generation for uncached vendors even when 2FA help is never needed, creating avoidable cost and latency — keep this hook disabled until 2FA/help is actually relevant. - In
apps/api/src/browserbase/browser-mfa-instructions.service.ts, the sanitizer only strips**so single-asterisk emphasis like*Open Settings*can leak into UI text, reducing instruction readability and polish — extend sanitation to single-emphasis and list-marker markdown patterns.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/ConnectCaptureForm.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/ConnectCaptureForm.tsx:68">
P2: Opening a capture form for any uncached vendor now spends a Firecrawl/AI generation even when the login does not use 2FA and the user never opens the help panel. Keeping this hook disabled until 2FA/help is relevant, or moving the prefetch behind the 2FA selection, would preserve the documented lazy behavior and avoid unnecessary generation cost.</violation>
</file>
<file name="apps/api/src/browserbase/browser-mfa-instructions.service.ts">
<violation number="1" location="apps/api/src/browserbase/browser-mfa-instructions.service.ts:105">
P2: Single-asterisk markdown can still reach the UI: `*Open Settings*` is unchanged because this sanitizer only removes `**`. Extending the sanitizer to handle single emphasis and list markers would make the plain-text guarantee enforceable rather than prompt-dependent.</violation>
<violation number="2" location="apps/api/src/browserbase/browser-mfa-instructions.service.ts:186">
P2: A model response containing only markdown delimiters or whitespace passes the pre-cleanup nonempty check, but this line returns an empty `steps` array instead of the universal fallback. Validating the cleaned array before constructing the generated response would preserve the service's non-empty-instructions guarantee.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| // Warm the per-vendor 2FA guidance while the user fills in the form, so the | ||
| // "How do I find this key?" helper opens instantly — and the server-side cache | ||
| // means the next person connecting the same vendor gets it immediately too. | ||
| useMfaInstructions(hostname, Boolean(hostname?.trim())); |
Contributor
There was a problem hiding this comment.
P2: Opening a capture form for any uncached vendor now spends a Firecrawl/AI generation even when the login does not use 2FA and the user never opens the help panel. Keeping this hook disabled until 2FA/help is relevant, or moving the prefetch behind the 2FA selection, would preserve the documented lazy behavior and avoid unnecessary generation cost.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/ConnectCaptureForm.tsx, line 68:
<comment>Opening a capture form for any uncached vendor now spends a Firecrawl/AI generation even when the login does not use 2FA and the user never opens the help panel. Keeping this hook disabled until 2FA/help is relevant, or moving the prefetch behind the 2FA selection, would preserve the documented lazy behavior and avoid unnecessary generation cost.</comment>
<file context>
@@ -61,6 +62,11 @@ export function ConnectCaptureForm({
+ // Warm the per-vendor 2FA guidance while the user fills in the form, so the
+ // "How do I find this key?" helper opens instantly — and the server-side cache
+ // means the next person connecting the same vendor gets it immediately too.
+ useMfaInstructions(hostname, Boolean(hostname?.trim()));
+
const {
</file context>
|
|
||
| /** Strip markdown emphasis the model sometimes emits (`**bold**`, `__`, backticks). */ | ||
| function stripEmphasis(text: string): string { | ||
| return text.replace(/\*\*/g, '').replace(/__/g, '').replace(/`/g, '').trim(); |
Contributor
There was a problem hiding this comment.
P2: Single-asterisk markdown can still reach the UI: *Open Settings* is unchanged because this sanitizer only removes **. Extending the sanitizer to handle single emphasis and list markers would make the plain-text guarantee enforceable rather than prompt-dependent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-mfa-instructions.service.ts, line 105:
<comment>Single-asterisk markdown can still reach the UI: `*Open Settings*` is unchanged because this sanitizer only removes `**`. Extending the sanitizer to handle single emphasis and list markers would make the plain-text guarantee enforceable rather than prompt-dependent.</comment>
<file context>
@@ -97,8 +97,14 @@ RULES:
+/** Strip markdown emphasis the model sometimes emits (`**bold**`, `__`, backticks). */
+function stripEmphasis(text: string): string {
+ return text.replace(/\*\*/g, '').replace(/__/g, '').replace(/`/g, '').trim();
+}
+
</file context>
Suggested change
| return text.replace(/\*\*/g, '').replace(/__/g, '').replace(/`/g, '').trim(); | |
| return text | |
| .replace(/\*\*/g, '') | |
| .replace(/__/g, '') | |
| .replace(/`/g, '') | |
| .replace(/^\s*[*+-]\s+/, '') | |
| .replace(/\*([^*]+)\*/g, '$1') | |
| .replace(/_([^_]+)_/g, '$1') | |
| .trim(); |
| ); | ||
| return { | ||
| hostname, | ||
| steps: object.steps.map(stripEmphasis).filter((step) => step.length > 0), |
Contributor
There was a problem hiding this comment.
P2: A model response containing only markdown delimiters or whitespace passes the pre-cleanup nonempty check, but this line returns an empty steps array instead of the universal fallback. Validating the cleaned array before constructing the generated response would preserve the service's non-empty-instructions guarantee.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-mfa-instructions.service.ts, line 186:
<comment>A model response containing only markdown delimiters or whitespace passes the pre-cleanup nonempty check, but this line returns an empty `steps` array instead of the universal fallback. Validating the cleaned array before constructing the generated response would preserve the service's non-empty-instructions guarantee.</comment>
<file context>
@@ -177,7 +183,7 @@ export class BrowserMfaInstructionsService {
return {
hostname,
- steps: object.steps,
+ steps: object.steps.map(stripEmphasis).filter((step) => step.length > 0),
tips: UNIVERSAL_TIPS,
confident: true,
</file context>
…hint - Native <button>s (header schedule / Connect another vendor / New evidence, the MfaSetupHelp trigger + retry, Turn off, Add-a-field) had no cursor:pointer. - The 2FA setup-key hint dropped the spaces around <em>once</em> / <strong>not</strong> because the tags sat on line breaks (JSX strips whitespace adjacent to tags), rendering 'oncewhen' / 'notthe'. Rewrote it as plain text (matches the 2a design).
During 'Test this step', the agent opens/switches tabs (a vendor may open sign-in or a console in a new tab), but the composer's live view stayed on the starting tab. Poll the newest tab during the agent run and stream its live-view URL (metadata 'testLiveViewUrl'); the composer points the iframe at it. Same tab-following we already do for the sign-in flow. Best-effort — a follower error never affects the run. Mirrors the evidence-screenshot fix so watch matches result.
… reconnect On reconnect (and scheduled relogin) the sign-in step showed a generic 'Entering username' because the detected field label (e.g. 'IAM username') was only passed transiently from the connect capture form. Persist it on the connection at connect time and fall back to it at sign-in when the caller doesn't supply one, so the real field name shows uniformly. - New BrowserAuthProfile.identifierLabel column (+ migration). - Capture flow stores it via POST /credentials (usernameLabel); sign-in uses input.usernameLabel ?? profile.identifierLabel. Tests: api +1 (label persisted, trimmed). Suites: api 150, app 100. NOTE: run 'bunx prisma migrate deploy' (packages/db) on the stand before deploy.
Contributor
There was a problem hiding this comment.
6 issues found across 15 files (changes from recent commits).
Confidence score: 3/5
- In
apps/api/src/browserbase/browser-evidence-execution.ts, the follower currently keys off URL/first-match page selection, so switching to another already-open tab (especially same-URL tabs) can stay invisible and keep users on the wrong live view — pass a stable tab/page identity through the follow logic instead of relying on URL matching alone. - In
apps/api/src/browserbase/browser-evidence-execution.ts, settinglastUrlbefore live-view lookup succeeds means one transient lookup error can suppress all future updates for that tab, causing the watched run to appear stuck — only recordlastUrlafter a successful lookup (or clear it on failure). - In
apps/api/src/browserbase/browser-evidence-execution.ts, follow mode starts too late (after auth flow) and the first poll waits 2.5s, so auth-opened tabs and short runs can miss critical tab switches — start following earlier and run an immediatetick()before the interval. - In
apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/InstructionComposer.tsx, stale realtime state during a second test can bind the iframe to the prior run’s tab, leading to cross-run confusion in the UI — mirror the existingrunState.idguard and includetestRunin the relevant dependency checks; add focused follower tests for cleanup/failure/tab-change paths to reduce regression risk.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/api/src/browserbase/browser-evidence-execution.ts">
<violation number="1" location="apps/api/src/browserbase/browser-evidence-execution.ts:88">
P3: The new live-view follower has no direct tests for timer cleanup, async lookup failures, or tab changes, making regressions in the watched test flow easy to introduce. A focused execution spec covering those cases would provide useful protection for this stateful polling code.</violation>
<violation number="2" location="apps/api/src/browserbase/browser-evidence-execution.ts:110">
P2: A transient live-view lookup failure permanently suppresses updates for the current tab because `lastUrl` is recorded before the lookup succeeds. Recording the URL only after a live-view URL is returned (or resetting it in the catch) lets later polls recover.</violation>
<violation number="3" location="apps/api/src/browserbase/browser-evidence-execution.ts:111">
P2: Switching to an already-open tab, especially one sharing the same URL, is invisible to this follower: only the URL is tracked and the live-view selector chooses the first matching page. Passing a tab/page identity or exposing the browser's active-page identity would make the live view follow the actual tab.</violation>
<violation number="4" location="apps/api/src/browserbase/browser-evidence-execution.ts:121">
P2: Short test runs can miss a tab switch entirely because the first follower poll is delayed by 2.5 seconds. Invoking `tick()` once before creating the interval would cover immediate navigation while retaining periodic tracking.</violation>
<violation number="5" location="apps/api/src/browserbase/browser-evidence-execution.ts:253">
P2: When expired authentication opens a new sign-in or 2FA tab, the watched run keeps the initial live view because tab following is installed only after the entire auth flow and is skipped on auth failure. Starting the follower before relogin and stopping it in outer cleanup, or forwarding the callback through relogin, would keep takeover on the page that needs the user.</violation>
</file>
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/InstructionComposer.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/InstructionComposer.tsx:187">
P2: Starting a second test can point the iframe at the previous test's browser tab when stale realtime data is present during the run switch. Matching the existing `runState.id` guard and including `testRun` in the dependencies would keep live-view updates scoped to the active test.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| }; | ||
|
|
||
| const interval = setInterval(() => void tick(), 2500); |
Contributor
There was a problem hiding this comment.
P2: Short test runs can miss a tab switch entirely because the first follower poll is delayed by 2.5 seconds. Invoking tick() once before creating the interval would cover immediate navigation while retaining periodic tracking.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-evidence-execution.ts, line 121:
<comment>Short test runs can miss a tab switch entirely because the first follower poll is delayed by 2.5 seconds. Invoking `tick()` once before creating the interval would cover immediate navigation while retaining periodic tracking.</comment>
<file context>
@@ -78,6 +78,53 @@ async function runCuaNavigation({
+ }
+ };
+
+ const interval = setInterval(() => void tick(), 2500);
+ return () => {
+ stopped = true;
</file context>
Suggested change
| const interval = setInterval(() => void tick(), 2500); | |
| void tick(); | |
| const interval = setInterval(() => void tick(), 2500); |
| const instruction = `${input.instruction}. Work out the path yourself — you don't need exact directions. If the instruction names a specific item or page, open it so the final screenshot clearly shows just that item, not a long list of many. Before finishing, verify the page matches what was asked and correct it if you opened the wrong thing. Don't take unnecessary detours. When the right thing is clearly shown, stop and wait.`; | ||
| const primaryModel = resolveCuaModel(logger); | ||
| // Follow the agent across tabs while it works (test/watched runs only). | ||
| const stopFollowing = onLiveView |
Contributor
There was a problem hiding this comment.
P2: When expired authentication opens a new sign-in or 2FA tab, the watched run keeps the initial live view because tab following is installed only after the entire auth flow and is skipped on auth failure. Starting the follower before relogin and stopping it in outer cleanup, or forwarding the callback through relogin, would keep takeover on the page that needs the user.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-evidence-execution.ts, line 253:
<comment>When expired authentication opens a new sign-in or 2FA tab, the watched run keeps the initial live view because tab following is installed only after the entire auth flow and is skipped on auth failure. Starting the follower before relogin and stopping it in outer cleanup, or forwarding the callback through relogin, would keep takeover on the page that needs the user.</comment>
<file context>
@@ -190,28 +249,41 @@ export async function executeBrowserEvidence({
const instruction = `${input.instruction}. Work out the path yourself — you don't need exact directions. If the instruction names a specific item or page, open it so the final screenshot clearly shows just that item, not a long list of many. Before finishing, verify the page matches what was asked and correct it if you opened the wrong thing. Don't take unnecessary detours. When the right thing is clearly shown, stop and wait.`;
const primaryModel = resolveCuaModel(logger);
+ // Follow the agent across tabs while it works (test/watched runs only).
+ const stopFollowing = onLiveView
+ ? startLiveViewFollower({
+ stagehand: activeStagehand,
</file context>
| const url = pages.at(-1)?.url?.() ?? ''; | ||
| if (url && url !== lastUrl) { | ||
| lastUrl = url; | ||
| const liveUrl = await sessions.getActivePageLiveViewUrl(sessionId, url); |
Contributor
There was a problem hiding this comment.
P2: Switching to an already-open tab, especially one sharing the same URL, is invisible to this follower: only the URL is tracked and the live-view selector chooses the first matching page. Passing a tab/page identity or exposing the browser's active-page identity would make the live view follow the actual tab.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-evidence-execution.ts, line 111:
<comment>Switching to an already-open tab, especially one sharing the same URL, is invisible to this follower: only the URL is tracked and the live-view selector chooses the first matching page. Passing a tab/page identity or exposing the browser's active-page identity would make the live view follow the actual tab.</comment>
<file context>
@@ -78,6 +78,53 @@ async function runCuaNavigation({
+ const url = pages.at(-1)?.url?.() ?? '';
+ if (url && url !== lastUrl) {
+ lastUrl = url;
+ const liveUrl = await sessions.getActivePageLiveViewUrl(sessionId, url);
+ if (liveUrl && !stopped) onLiveView(liveUrl);
+ }
</file context>
Comment on lines
+110
to
+112
| lastUrl = url; | ||
| const liveUrl = await sessions.getActivePageLiveViewUrl(sessionId, url); | ||
| if (liveUrl && !stopped) onLiveView(liveUrl); |
Contributor
There was a problem hiding this comment.
P2: A transient live-view lookup failure permanently suppresses updates for the current tab because lastUrl is recorded before the lookup succeeds. Recording the URL only after a live-view URL is returned (or resetting it in the catch) lets later polls recover.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-evidence-execution.ts, line 110:
<comment>A transient live-view lookup failure permanently suppresses updates for the current tab because `lastUrl` is recorded before the lookup succeeds. Recording the URL only after a live-view URL is returned (or resetting it in the catch) lets later polls recover.</comment>
<file context>
@@ -78,6 +78,53 @@ async function runCuaNavigation({
+ const pages = stagehand.context?.pages?.() ?? [];
+ const url = pages.at(-1)?.url?.() ?? '';
+ if (url && url !== lastUrl) {
+ lastUrl = url;
+ const liveUrl = await sessions.getActivePageLiveViewUrl(sessionId, url);
+ if (liveUrl && !stopped) onLiveView(liveUrl);
</file context>
Suggested change
| lastUrl = url; | |
| const liveUrl = await sessions.getActivePageLiveViewUrl(sessionId, url); | |
| if (liveUrl && !stopped) onLiveView(liveUrl); | |
| const liveUrl = await sessions.getActivePageLiveViewUrl(sessionId, url); | |
| if (liveUrl && !stopped) { | |
| onLiveView(liveUrl); | |
| lastUrl = url; | |
| } |
Comment on lines
+187
to
+189
| const streamed = runState?.metadata?.testLiveViewUrl as string | undefined; | ||
| if (streamed) setLiveViewUrl(streamed); | ||
| }, [runState]); |
Contributor
There was a problem hiding this comment.
P2: Starting a second test can point the iframe at the previous test's browser tab when stale realtime data is present during the run switch. Matching the existing runState.id guard and including testRun in the dependencies would keep live-view updates scoped to the active test.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/InstructionComposer.tsx, line 187:
<comment>Starting a second test can point the iframe at the previous test's browser tab when stale realtime data is present during the run switch. Matching the existing `runState.id` guard and including `testRun` in the dependencies would keep live-view updates scoped to the active test.</comment>
<file context>
@@ -181,6 +181,13 @@ export function InstructionComposer({
+ // Follow the agent across tabs: when the run reports a new active-tab live
+ // view, point the iframe at it (AWS etc. open sign-in/console in new tabs).
+ useEffect(() => {
+ const streamed = runState?.metadata?.testLiveViewUrl as string | undefined;
+ if (streamed) setLiveViewUrl(streamed);
+ }, [runState]);
</file context>
Suggested change
| const streamed = runState?.metadata?.testLiveViewUrl as string | undefined; | |
| if (streamed) setLiveViewUrl(streamed); | |
| }, [runState]); | |
| if (!testRun) return; | |
| if (runState && runState.id !== testRun.runId) return; | |
| const streamed = runState?.metadata?.testLiveViewUrl as string | undefined; | |
| if (streamed) setLiveViewUrl(streamed); | |
| }, [testRun, runState]); |
| * of showing the stale initial one. Best-effort: never lets live-view following | ||
| * disturb the run. Returns a stop function. | ||
| */ | ||
| function startLiveViewFollower({ |
Contributor
There was a problem hiding this comment.
P3: The new live-view follower has no direct tests for timer cleanup, async lookup failures, or tab changes, making regressions in the watched test flow easy to introduce. A focused execution spec covering those cases would provide useful protection for this stateful polling code.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/browserbase/browser-evidence-execution.ts, line 88:
<comment>The new live-view follower has no direct tests for timer cleanup, async lookup failures, or tab changes, making regressions in the watched test flow easy to introduce. A focused execution spec covering those cases would provide useful protection for this stateful polling code.</comment>
<file context>
@@ -78,6 +78,53 @@ async function runCuaNavigation({
+ * of showing the stale initial one. Best-effort: never lets live-view following
+ * disturb the run. Returns a stop function.
+ */
+function startLiveViewFollower({
+ stagehand,
+ sessions,
</file context>
…idence section The 'Drafts — not saved yet' strip rendered as a standalone card above the section, so it read as unattached — unclear it belongs to Browser evidence. Move it inside the section card, as an inset band directly under the header (new DraftsStrip 'nested' variant). The standalone card is kept only for the no-automations-yet state, where there's no section to nest into.
Contributor
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Confidence score: 4/5
- In
apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/BrowserAutomationsList.tsx, the Continue/Delete render guard relies on callback presence instead of RBAC, so read-only or custom-role users can be shown draft mutation actions they should not access, which creates permission leakage and potential unauthorized mutation attempts—gate each action on the correspondingtask...permission checks.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/BrowserAutomationsList.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/BrowserAutomationsList.tsx:131">
P2: Read-only and custom-role users can see draft mutation actions because this render guard does not consult RBAC; callback presence is not a permission check. Please gate Continue and Delete with the corresponding `task:create`/`task:update` and `task:delete` permissions (ideally independently) so the UI matches the API authorization model.
(Based on your team's feedback about RBAC permission checks.) [FEEDBACK_USED]</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| </div> | ||
| </div> | ||
| </div> | ||
| {drafts.length > 0 && onContinueDraft && onDeleteDraft && ( |
Contributor
There was a problem hiding this comment.
P2: Read-only and custom-role users can see draft mutation actions because this render guard does not consult RBAC; callback presence is not a permission check. Please gate Continue and Delete with the corresponding task:create/task:update and task:delete permissions (ideally independently) so the UI matches the API authorization model.
(Based on your team's feedback about RBAC permission checks.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/tasks/[taskId]/components/browser-automations/BrowserAutomationsList.tsx, line 131:
<comment>Read-only and custom-role users can see draft mutation actions because this render guard does not consult RBAC; callback presence is not a permission check. Please gate Continue and Delete with the corresponding `task:create`/`task:update` and `task:delete` permissions (ideally independently) so the UI matches the API authorization model.
(Based on your team's feedback about RBAC permission checks.) </comment>
<file context>
@@ -116,6 +128,16 @@ export function BrowserAutomationsList({
onCreate={onCreate}
/>
+ {drafts.length > 0 && onContinueDraft && onDeleteDraft && (
+ <DraftsStrip
+ nested
</file context>
…ons' 'Connections' was ambiguous (easily confused with integration connections). Name it 'Browser connections' so it's clear it manages the vendor logins browser automations sign in with. Label only — the /settings/browser-connection route is unchanged. Updated the sidebar item, the browser-tab title, and the active-tab label.
Someone landing on the page from Settings had no idea what it's for. Wrap it in a
titled Section ('Browser connections' + one-line description) and add a compact
'How it works' explainer: (1) connect a vendor login here — Comp signs in incl.
2FA, (2) it signs in on a schedule, screenshots audit evidence, and re-signs in on
its own when a session expires, (3) the automations that use these logins are
built inside a task under 'Browser evidence' — so a lost user knows where to go.
- The Section repeated 'Browser connections' under the layout's big page title — removed the Section title, kept the description + Connect action. - 'How it works' steps now use a flex row (number + text with a gap) so the spacing after the number is consistent — item 3 was rendering as '3.You'.
Tie the copy to the product's real terms so people know where these logins apply: description now reads 'The vendor logins your evidence tasks use to sign in and capture audit evidence automatically', and step 3 points to adding automations inside an evidence task's Browser evidence section.
…below the table - Add a search box above the table (filters by vendor name, hostname, or login). - Paginate at 10 rows/page with Previous / Page X of Y / Next (shown only when a filtered list exceeds one page) — previously every connection rendered at once. - Move the 'N connections · M active' summary from above to below the table, next to the pager. Empty-search shows a 'No connections match …' row.
…ount reflect it - Search now lives in a toolbar at the top of the table card (right-aligned on desktop, full-width on mobile) with a magnifier icon, instead of floating above. - The summary under the table now reflects the search: 'X of Y connections · N active' while filtering (active/attention counts are of the shown set), and the plain total when not searching.
…ide) Move the search out of a separate toolbar and into the column-header row itself — right-aligned on the same line as the VENDOR/… headers (desktop). On mobile the table scrolls horizontally, so the search stays as a full-width bar above the table instead of hiding behind the scroll. Both share one query state.
Put the search right of the 'Vendor' label (left of 'Method'), inside the Vendor header cell, instead of as its own right-side column. Mobile keeps the full-width search bar above the table.
Mintlify page (packages/docs/browser-automations.mdx) covering what the feature does, connecting a vendor, unattended 2FA via the authenticator setup key, creating a browser-evidence automation, what happens on a schedule (self-healing re-login), managing connections, best practices, and troubleshooting. Added to the Guides → Get Started nav next to Automated Evidence.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| } | ||
|
|
||
| const vault = resolveBrowserCredentialVaultAdapter(); | ||
| const { outcome } = await signInAndClassify({ |
Contributor
Author
There was a problem hiding this comment.
🤖 Security Issue: The automated sign-in flow navigates to the caller-supplied input.url and then auto-fills the profile's vault-stored username/password/TOTP (via signInAndClassify -> performCredentialLogin) without ever checking that the URL's hostname matches the profile's own hostname. The SignInAuthProfileDto.url only enforces @IsSafeUrl (SSRF/private-IP protection), which still permits any arbitrary public URL. Stored credentials are intentionally write-only (users cannot read the plaintext back from the vault), so this navigation-to-arbitrary-host behavior breaks that boundary.
Severity: MEDIUM
Category: authorization_bypass
Tool: ClaudeCode AI Security Analysis
Exploit Scenario: An insider with only integration:update permission (who cannot read the org's stored vendor passwords in plaintext) hosts an attacker-controlled page that mimics a login form and captures submitted values. They call POST /v1/browserbase/profiles/{profileId}/sign-in with an existing password-backed profileId (e.g. the org's GitHub service account) and url set to their evil page. The background automation opens that page in the live browser and types the profile's stored username and password (and a freshly generated TOTP code) into the attacker's form, exfiltrating credentials the user was never authorized to see.
Recommendation: Before performing the credential auto-fill, verify that normalizeHostnameFromUrl(input.url) === profile.hostname (or is a known sign-in host of that profile) and reject/990 otherwise. Do not allow arbitrary caller-supplied URLs to drive an automated fill of vault-stored secrets.
A few plain-language bullets on where and why it's most useful — reaching evidence with no API, handling 2FA unattended, staying fresh on its own, and being auditor-ready — plus the sweet-spot use case (recurring evidence from admin consoles / security-settings pages that have no export).
Contributor
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
Four screenshots, one per key moment: the audit-ready evidence output (hero), the 2FA setup-key step with per-vendor guidance (Unattended 2FA), the live test run with the AI driving the browser (Create an automation), and the Browser connections page (Manage your connections).
… hero Swap the hero for a version with the org's repo navigation blurred out — keeps the positive, secret-free part (2FA enforced, source URL) for the public docs.
Make explicit that automations run automatically on a schedule you control (Daily / Weekly / Monthly / Quarterly / Yearly, one cadence per task, changeable any time) and that you can also Run any automation on demand — not only manually.
Contributor
There was a problem hiding this comment.
1 issue found across 9 files (changes from recent commits).
Confidence score: 5/5
- In
apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ConnectionsTable.tsx, changing the search while on a later page can momentarily show an empty-state row before matching results load, which may confuse users into thinking there are no results — resetpageat the same time asqueryin the search change handler to remove the flicker.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ConnectionsTable.tsx">
<violation number="1" location="apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ConnectionsTable.tsx:65">
P3: Searching from a later page can briefly render the empty-state row before results appear, because the page reset happens after the query render. Reset `page` in the search change handler alongside `query`, so filtering and pagination update atomically.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
| }, [connections, query]); | ||
|
|
||
| // Back to the first page whenever the search narrows the list. | ||
| useEffect(() => setPage(0), [query]); |
Contributor
There was a problem hiding this comment.
P3: Searching from a later page can briefly render the empty-state row before results appear, because the page reset happens after the query render. Reset page in the search change handler alongside query, so filtering and pagination update atomically.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/app/(app)/[orgId]/settings/browser-connection/components/ConnectionsTable.tsx, line 65:
<comment>Searching from a later page can briefly render the empty-state row before results appear, because the page reset happens after the query render. Reset `page` in the search change handler alongside `query`, so filtering and pagination update atomically.</comment>
<file context>
@@ -44,22 +47,97 @@ export function ConnectionsTable({
+ }, [connections, query]);
+
+ // Back to the first page whenever the search narrows the list.
+ useEffect(() => setPage(0), [query]);
+
+ const pageCount = Math.max(1, Math.ceil(filtered.length / PAGE_SIZE));
</file context>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an automated pull request to merge feat/browser-automation-ui into dev.
It was created by the [Auto Pull Request] action.
Summary by cubic
Builds a method‑aware connect‑and‑capture flow with
1Password‑backed credentials, unattended re‑login, live step timelines with in‑place take‑over, multi‑step automations with per‑step evidence (full‑page + focused close‑up), and an org‑level “Browser connections” page to reconnect, edit, and remove vendor logins.New Features
1PasswordviaPOST /v1/browserbase/profiles/:id/credentials; resolve just‑in‑time (secrets never reach the model), fill by label with Stagehand substitution, retry TOTP once, classify outcomes (invalid_credentials/needs_2fa/challenge/sso_handoff), then verify and set the authenticated home URL for reuse. The scheduler can runneeds_reauthprofiles with stored creds.@1password/sdkis lazy‑loaded and externalized for@trigger.dev; best‑effort cleanup deletes the 1Password item.POST /v1/browserbase/analyze-loginopens a throwaway browser, detects methods, and returns a run handle; pick password or SSO, auto‑navigate to the sign‑in form/IdP, then run the first sign‑in on a live session with a streamed step timeline, live‑view phase signals, and take‑over in place. Capture credentials once (including site‑specific fields) and verify; on success, future runs open the signed‑in app directly. Falls back to manual when Browserbase is unavailable. Adds per‑vendor MFA setup guidance during capture.POST /v1/browserbase/instructions/testwith streamed activity. Evaluation is anchored to the instruction’s target. Human‑facing sessions render smaller for legibility (1280×800); unattended capture uses 1920×1080 and full‑page for sharp evidence. Reduced‑motion styles steady views. Navigation is swappable via env (defaults toopenai/gpt-5.6-terrawith a Claude fallback and runtime failover).Migration
BROWSERBASE_API_KEYandBROWSERBASE_PROJECT_ID; optionalBROWSERBASE_CUA_MODEL(defaults toopenai/gpt-5.6-terrawith a Claude fallback) andBROWSERBASE_STAGEHAND_MODEL. SetOP_SERVICE_ACCOUNT_TOKEN; build/deploy with@1password/sdk(lazy‑loaded, externalized in@trigger.devbuilds).BrowserAutomationStep,BrowserAutomationStepRun, andBrowserAutomationDraft; migration backfills one step per existing automation (matched by hostname).Written for commit c0174bf. Summary will update on new commits.