Skip to content

ci(frontend): use playwright container, add cache + path filter #1810

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
gugu merged 3 commits into from
May 28, 2026
Merged

ci(frontend): use playwright container, add cache + path filter #1810

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
31118ba
ci(frontend): use playwright container, add cache + path filter
gugu May 28, 2026
b7a1e25
ci(frontend): bump playwright image tag to v1.58.1-noble
gugu May 28, 2026
d01db25
fix(2fa): accept legacy 10-byte secrets in otplib v13 verification
gugu May 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 40 additions & 15 deletions .github/workflows/frontend.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,31 +1,56 @@
name: frontend

on:
push:
branches:
- main
# Publish semver tags as releases
tags: [ '*.*.*' ]
- main
tags:
- '*.*.*'
paths:
- 'frontend/**'
- '.github/workflows/frontend.yml'
pull_request:
paths:
- 'frontend/**'
- '.github/workflows/frontend.yml'

concurrency:
group: frontend-${{ github.ref }}
cancel-in-progress: true

defaults:
run:
working-directory: frontend

# TODO: add a lint job once ESLint migration replaces the (already-removed in
# @angular-devkit/build-angular v20) `tslint` builder that `yarn lint` calls.

jobs:
test:
Comment on lines 28 to 29

@coderabbitai coderabbitai Bot May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Add an explicit top-level permissions block (least privilege).

No permissions: block means default token scopes apply. Tightening scopes here reduces blast radius for both jobs.

Suggested change 
 jobs:
+permissions:
+  contents: read
+
+jobs:
   test:
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/frontend.yml around lines 28 - 29, Add a top-level GitHub
Actions permissions block to the workflow (at the same root level as jobs) to
enforce least-privilege for the entire workflow; explicitly set only the minimal
permissions required by your job(s) (for example for the test job: contents:
read, checks: write, id-token: write, actions: read — adjust to the actual APIs
your tests use) so the default token scopes are not granted broadly; place this
permissions block above the jobs: definition so it applies to the test job and
any future jobs.

runs-on: ubuntu-latest
timeout-minutes: 20
container:
image: mcr.microsoft.com/playwright:v1.58.1-noble
steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v3
- uses: actions/checkout@v5

@coderabbitai coderabbitai Bot May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Disable credential persistence in checkout steps.

Line 35 and Line 49 should set persist-credentials: false to avoid token reuse from the checked-out repo config.

Suggested change 
-      - uses: actions/checkout@v5
+      - uses: actions/checkout@v5
+        with:
+          persist-credentials: false
...
-      - uses: actions/checkout@v5
+      - uses: actions/checkout@v5
+        with:
+          persist-credentials: false

Also applies to: 49-49

🧰 Tools
🪛 zizmor (1.25.2)

[warning] 35-35: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false

(artipacked)

[error] 35-35: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/frontend.yml at line 35, The checkout steps currently use
actions/checkout@v5 without disabling credential persistence; update each
actions/checkout@v5 step (both occurrences) to include the input
persist-credentials: false so the runner does not persist the repository token
into the checked-out repo config; locate the checkout uses of
actions/checkout@v5 and add the persist-credentials: false option to those
steps.

- uses: actions/setup-node@v4
Comment on lines +35 to +36

@coderabbitai coderabbitai Bot May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

file=".github/workflows/frontend.yml"

echo "== File exists? =="
ls -la "$file" || true

echo
echo "== Show lines 25-70 with numbers =="
nl -ba "$file" | sed -n '25,70p'

echo
echo "== Extract all `uses:` lines =="
rg -n "^\s*-\s*uses:" "$file" || true

echo
echo "== Check for any `permissions:` block =="
rg -n "^\s*permissions:" -S "$file" || true

echo
echo "== Check for checkout credential persistence-related config =="
rg -n "persist-credentials|clean|fetch-depth" "$file" || true

Repository: rocket-admin/rocketadmin

Length of output: 266

🏁 Script executed:

#!/bin/bash
set -euo pipefail

file=".github/workflows/frontend.yml"

echo "== Show lines 25-70 with numbers (cat -n) =="
cat -n "$file" | sed -n '25,70p'

echo
echo "== Extract all `uses:` lines =="
rg -n "^\s*-\s*uses:" "$file" || true

echo
echo "== Check for any `permissions:` block =="
rg -n "^\s*permissions:" -S "$file" || true

echo
echo "== Check for checkout credential persistence-related config (fetch-depth/persist-credentials) =="
rg -n "persist-credentials|fetch-depth|clean:" "$file" || true

Repository: rocket-admin/rocketadmin

Length of output: 1804

Pin GitHub Actions to full commit SHAs.

.github/workflows/frontend.yml uses mutable tags for actions/checkout and actions/setup-node in both jobs (@v5/@v4 at lines 35-36 and again at 49-50); pin to immutable commit SHAs.

Suggested change 
-      - uses: actions/checkout@v5
-      - uses: actions/setup-node@v4
+      - uses: actions/checkout@<checkout-v5-full-commit-sha>
+      - uses: actions/setup-node@<setup-node-v4-full-commit-sha>
...
-      - uses: actions/checkout@v5
-      - uses: actions/setup-node@v4
+      - uses: actions/checkout@<checkout-v5-full-commit-sha>
+      - uses: actions/setup-node@<setup-node-v4-full-commit-sha>
🧰 Tools
🪛 zizmor (1.25.2)

[warning] 35-35: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false

(artipacked)

[error] 35-35: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

[error] 36-36: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

[error] 36-36: runtime artifacts potentially vulnerable to a cache poisoning attack (cache-poisoning): this step

(cache-poisoning)

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/frontend.yml around lines 35 - 36, The workflow uses
mutable tags for two actions—actions/checkout@v5 and actions/setup-node@v4—so
update both occurrences in .github/workflows/frontend.yml (both jobs) to pinned
immutable commit SHAs: look for the strings "actions/checkout@v5" and
"actions/setup-node@v4" and replace each tag with the corresponding full commit
SHA for the release you want to pin (use the official GitHub repo commit SHAs
for actions/checkout and actions/setup-node), making sure to pin both places
where they appear.

with:
node-version: '24'
- run: cd frontend && yarn install
- name: Install Playwright browsers
run: cd frontend && yarn playwright install
- name: run tests
run: cd frontend && yarn test
cache: 'yarn'
cache-dependency-path: frontend/yarn.lock
- run: yarn install --immutable
- name: Run unit tests
run: yarn test:ci

license:
runs-on: ubuntu-latest
timeout-minutes: 10
steps:
- uses: actions/checkout@v2
- uses: actions/setup-node@v3
- uses: actions/checkout@v5
- uses: actions/setup-node@v4
with:
node-version: '16'
- uses: extractions/setup-just@v1
node-version: '24'
cache: 'yarn'
cache-dependency-path: frontend/yarn.lock
- name: license checker
run: 'cd frontend && npx license-checker --onlyAllow="MIT;ISC;Python-2.0;Apache-2.0;BSD;MPL;CC;Custom: http://github.com/dscape/statsd-parser;" --excludePrivatePackages'
run: 'npx --yes license-checker --onlyAllow="MIT;ISC;Python-2.0;Apache-2.0;BSD;MPL;CC;Custom: http://github.com/dscape/statsd-parser;" --excludePrivatePackages'
7 changes: 6 additions & 1 deletion backend/src/entities/user/use-cases/disable-otp.use.case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import { BaseType } from '../../../common/data-injection.tokens.js';
import { Messages } from '../../../exceptions/text/messages.js';
import { OtpDisablingResultDS } from '../application/data-structures/otp-validation-result.ds.js';
import { VerifyOtpDS } from '../application/data-structures/verify-otp.ds.js';
import { legacyOtpGuardrails } from '../utils/otp-guardrails.js';
import { IDisableOTP } from './user-use-cases.interfaces.js';

@Injectable()
Expand Down Expand Up @@ -41,7 +42,11 @@ export class DisableOtpUseCase extends AbstractUseCase<VerifyOtpDS, OtpDisabling
throw new BadRequestException(Messages.OTP_NOT_ENABLED);
}
try {
const isValid = verifySync({ token: otpToken, secret: otpSecretKey }).valid;
const isValid = verifySync({
token: otpToken,
secret: otpSecretKey,
guardrails: legacyOtpGuardrails,
}).valid;
if (isValid) {
foundUser.isOTPEnabled = false;
foundUser.otpSecretKey = null;
Expand Down
7 changes: 6 additions & 1 deletion backend/src/entities/user/use-cases/otp-login-use.case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { SignInAuditService } from '../../user-sign-in-audit/sign-in-audit.servi
import { VerifyOtpDS } from '../application/data-structures/verify-otp.ds.js';
import { generateGwtToken, IToken } from '../utils/generate-gwt-token.js';
import { get2FaScope } from '../utils/is-jwt-scope-need.util.js';
import { legacyOtpGuardrails } from '../utils/otp-guardrails.js';
import { IOtpLogin } from './user-use-cases.interfaces.js';

@Injectable()
Expand All @@ -28,7 +29,11 @@ export class OtpLoginUseCase extends AbstractUseCase<VerifyOtpDS, IToken> implem
if (!foundUser) {
throw new NotFoundException(Messages.USER_NOT_FOUND);
}
const isValid = verifySync({ token: otpToken, secret: foundUser.otpSecretKey }).valid;
const isValid = verifySync({
token: otpToken,
secret: foundUser.otpSecretKey,
guardrails: legacyOtpGuardrails,
}).valid;
Comment on lines +32 to +36

@coderabbitai coderabbitai Bot May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# 1) Inspect otpSecretKey shape/nullability in user entity
fd -i 'user.entity.ts' backend/src/entities | xargs -I{} rg -n -C2 '\botpSecretKey\b' {}

# 2) Compare verifySync handling across OTP use-cases
rg -n -C4 'verifySync\(' \
  backend/src/entities/user/use-cases/disable-otp.use.case.ts \
  backend/src/entities/user/use-cases/otp-login-use.case.ts \
  backend/src/entities/user/use-cases/verify-otp-use.case.ts

Repository: rocket-admin/rocketadmin

Length of output: 2936

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the surrounding code in otp-login-use.case.ts
sed -n '1,120p' backend/src/entities/user/use-cases/otp-login-use.case.ts | nl -ba | sed -n '1,120p'

echo "----"
# Also inspect a narrower window around the reported lines
sed -n '1,90p' backend/src/entities/user/use-cases/otp-login-use.case.ts | nl -ba | sed -n '15,75p'

Repository: rocket-admin/rocketadmin

Length of output: 111

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect otp-login use case around verifySync (lines ~20-80) with line numbers
sed -n '1,120p' backend/src/entities/user/use-cases/otp-login-use.case.ts | cat -n

echo "---- narrower window ----"
sed -n '15,80p' backend/src/entities/user/use-cases/otp-login-use.case.ts | cat -n

Repository: rocket-admin/rocketadmin

Length of output: 5647

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect user entity definition around otpSecretKey
sed -n '60,110p' backend/src/entities/user/user.entity.ts | cat -n

echo "---- verify-otp-use.case.ts ----"
sed -n '1,120p' backend/src/entities/user/use-cases/verify-otp.use.case.ts | cat -n

echo "---- disable-otp.use.case.ts ----"
sed -n '1,120p' backend/src/entities/user/use-cases/disable-otp.use.case.ts | cat -n

Repository: rocket-admin/rocketadmin

Length of output: 1620

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "---- verify-otp-use.case.ts (around verifySync) ----"
sed -n '1,140p' backend/src/entities/user/use-cases/verify-otp-use.case.ts | cat -n

echo "---- disable-otp.use.case.ts (around verifySync) ----"
sed -n '1,140p' backend/src/entities/user/use-cases/disable-otp.use.case.ts | cat -n

Repository: rocket-admin/rocketadmin

Length of output: 5631

🌐 Web query:

otplib verifySync throws exception conditions secret token invalid

💡 Result:

The verifySync function in otplib does not throw an exception when a token is invalid or when the secret is incorrect [1][2]. Instead, it returns a VerifyResult object, which contains a valid boolean property [1][3]. If the verification fails, the valid property will simply be false [3]. The only condition under which verifySync throws an exception is if the provided crypto plugin does not support synchronous HMAC operations (e.g., if you attempt to use the WebCryptoPlugin for synchronous operations) [1][2][4]. If you are experiencing unexpected behavior, consider the following common causes: - Verify that your crypto plugin is configured correctly and supports synchronous operations (e.g., use NodeCryptoPlugin or NobleCryptoPlugin) [1][5]. - Ensure your time-based configurations (such as step, window, or epochTolerance) are set correctly, as mismatches can cause tokens to be rejected even if they seem correct [6][7]. - Confirm that the secret and token provided to the function are in the expected format [1]. If you need to handle invalid tokens, you should check the valid property of the returned result object rather than wrapping the call in a try-catch block for validation logic [1][3].

Citations:

Guard nullable otpSecretKey (and handle unexpected verifySync errors) in OTP login to ensure failed audit + 400, not 500

backend/src/entities/user.entity.ts declares otpSecretKey as nullable (@Column({ default: null })), but backend/src/entities/user/use-cases/otp-login-use.case.ts calls verifySync(...).valid on Line 32 without checking foundUser.otpSecretKey or catching unexpected verifySync/crypto exceptions. The OTP-related use-cases verify-otp.use.case.ts and disable-otp.use.case.ts both guard otpSecretKey and wrap verifySync in try/catch, so otp-login-use.case.ts should follow the same pattern to keep failures auditable and consistently mapped to BadRequestException.

Proposed fix 
+		if (!foundUser.otpSecretKey) {
+			await this.recordSignInAudit(
+				foundUser.email,
+				userId,
+				SignInStatusEnum.FAILED,
+				ipAddress,
+				userAgent,
+				Messages.LOGIN_DENIED_INVALID_OTP,
+			);
+			throw new BadRequestException(Messages.LOGIN_DENIED_INVALID_OTP);
+		}
-		const isValid = verifySync({
-			token: otpToken,
-			secret: foundUser.otpSecretKey,
-			guardrails: legacyOtpGuardrails,
-		}).valid;
+		let isValid: boolean;
+		try {
+			isValid = verifySync({
+				token: otpToken,
+				secret: foundUser.otpSecretKey,
+				guardrails: legacyOtpGuardrails,
+			}).valid;
+		} catch {
+			await this.recordSignInAudit(
+				foundUser.email,
+				userId,
+				SignInStatusEnum.FAILED,
+				ipAddress,
+				userAgent,
+				Messages.LOGIN_DENIED_INVALID_OTP,
+			);
+			throw new BadRequestException(Messages.LOGIN_DENIED_INVALID_OTP);
+		}
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@backend/src/entities/user/use-cases/otp-login-use.case.ts` around lines 32 -
36, The OTP login flow calls verifySync(...) directly and assumes
foundUser.otpSecretKey is present; update the otp-login-use.case.ts to first
guard nullable foundUser.otpSecretKey and return a failed audit + throw
BadRequestException when missing, then wrap the verifySync(...) call (using
otpToken, foundUser.otpSecretKey, legacyOtpGuardrails) in a try/catch so any
crypto/verifySync errors are caught and mapped to a failed audit +
BadRequestException instead of bubbling as a 500; mirror the pattern used in
verify-otp.use.case.ts / disable-otp.use.case.ts (check otpSecretKey, try/catch
verifySync, log/audit failures, throw BadRequestException).

if (!isValid) {
await this.recordSignInAudit(
foundUser.email,
Expand Down
7 changes: 6 additions & 1 deletion backend/src/entities/user/use-cases/verify-otp-use.case.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { BaseType } from '../../../common/data-injection.tokens.js';
import { Messages } from '../../../exceptions/text/messages.js';
import { OtpValidationResultDS } from '../application/data-structures/otp-validation-result.ds.js';
import { VerifyOtpDS } from '../application/data-structures/verify-otp.ds.js';
import { legacyOtpGuardrails } from '../utils/otp-guardrails.js';
import { IVerifyOTP } from './user-use-cases.interfaces.js';

@Injectable()
Expand Down Expand Up @@ -48,7 +49,11 @@ export class VerifyOtpUseCase extends AbstractUseCase<VerifyOtpDS, OtpValidation
);
}
try {
const isValid = verifySync({ token: otpToken, secret: otpSecretKey }).valid;
const isValid = verifySync({
token: otpToken,
secret: otpSecretKey,
guardrails: legacyOtpGuardrails,
}).valid;
if (isValid) {
foundUser.isOTPEnabled = true;
await this._dbContext.userRepository.saveUserEntity(foundUser);
Expand Down
8 changes: 8 additions & 0 deletions backend/src/entities/user/utils/otp-guardrails.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import { createGuardrails } from 'otplib';

// otplib v13 enforces RFC 4226's 128-bit (16-byte) minimum secret length, but
// users enrolled before the v12 -> v13 upgrade have 10-byte secrets stored
// (the v12 `authenticator.generateSecret()` default). Relax the guardrail at
// verify time so those users can still authenticate. New secrets use v13's
// 20-byte default and are RFC-compliant.
export const legacyOtpGuardrails = createGuardrails({ MIN_SECRET_BYTES: 10 });
Loading