ci(frontend): use playwright container, add cache + path filter#1810
Conversation
- Run tests in mcr.microsoft.com/playwright:v1.57.0-noble so the workflow no longer downloads browsers at runtime. - Fix yarn test (watch-mode) -> yarn test:ci. - Add yarn cache via setup-node@v4 and yarn install --immutable. - Path-filter triggers to frontend/** plus this workflow file. - Cancel superseded PR runs via concurrency group. - Bump license job Node 16 -> 24, drop unused setup-just step. - Modernize action versions (checkout@v5, setup-node@v4). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughFrontend CI workflow is narrowed to frontend path changes, adds concurrency and a default working directory, containerizes tests using Playwright, updates actions to modern versions with Node 24 and Yarn caching, and modernizes the license job. Backend adds a legacy otplib guardrails export and applies it to OTP verification in three use-cases. ChangesFrontend CI Workflow Modernization
OTP Guardrails and Verification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/workflows/frontend.yml (1)
36-41: ⚡ Quick winReassess Yarn cache hardening in frontend.yml PRs
This workflow runs on
pull_request(notpull_request_target) and the cache key is deterministic (frontend/yarn.lock), with no evidence of other privileged workflows consuming the same cached artifacts across trust boundaries—so splitting cached vs non-cachedactions/setup-nodeby event type is unnecessary here. Just ensure you never cache secrets/tokens or non-deterministic build outputs.Also applies to: 50-54
🤖 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 36 - 41, The workflow currently splits cached vs non-cached setup-nightly logic; instead consolidate to a single actions/setup-node@v4 step using the deterministic cache settings (keep node-version: '24', cache: 'yarn', cache-dependency-path: 'frontend/yarn.lock') and keep the yarn install --immutable run; remove any event-type conditional around setup-node and ensure no secrets or non-deterministic outputs are written into the cache (verify keys and paths only reference frontend/yarn.lock and other stable files).
🤖 Prompt for all review comments with 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.
Inline comments:
In @.github/workflows/frontend.yml:
- 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.
- Around line 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.
- Around line 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.
---
Nitpick comments:
In @.github/workflows/frontend.yml:
- Around line 36-41: The workflow currently splits cached vs non-cached
setup-nightly logic; instead consolidate to a single actions/setup-node@v4 step
using the deterministic cache settings (keep node-version: '24', cache: 'yarn',
cache-dependency-path: 'frontend/yarn.lock') and keep the yarn install
--immutable run; remove any event-type conditional around setup-node and ensure
no secrets or non-deterministic outputs are written into the cache (verify keys
and paths only reference frontend/yarn.lock and other stable files).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de2213a5-931e-4886-95d7-dfce27d193d2
📒 Files selected for processing (1)
.github/workflows/frontend.yml
| jobs: | ||
| test: |
There was a problem hiding this comment.
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.
| steps: | ||
| - uses: actions/checkout@v2 | ||
| - uses: actions/setup-node@v3 | ||
| - uses: actions/checkout@v5 |
There was a problem hiding this comment.
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: falseAlso 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/checkout@v5 | ||
| - uses: actions/setup-node@v4 |
There was a problem hiding this comment.
🧩 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" || trueRepository: 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" || trueRepository: 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.
The yarn.lock resolves playwright@^1.57.0 to 1.58.1; the container tag must match exactly or Vitest's browser provider refuses to run. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Users enrolled before the otplib v12 -> v13 upgrade have 10-byte secrets stored, which v13's RFC 4226 guardrail (16-byte minimum) rejects with "Secret must be at least 16 bytes". Pass a relaxed `MIN_SECRET_BYTES: 10` guardrail to verifySync so existing users can still authenticate; new secrets continue to use v13's 20-byte default. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with 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.
Inline comments:
In `@backend/src/entities/user/use-cases/otp-login-use.case.ts`:
- Around line 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).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 57f19288-f1f0-4c40-8c1d-ca63eb9a37c2
📒 Files selected for processing (5)
.github/workflows/frontend.ymlbackend/src/entities/user/use-cases/disable-otp.use.case.tsbackend/src/entities/user/use-cases/otp-login-use.case.tsbackend/src/entities/user/use-cases/verify-otp-use.case.tsbackend/src/entities/user/utils/otp-guardrails.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/frontend.yml
| const isValid = verifySync({ | ||
| token: otpToken, | ||
| secret: foundUser.otpSecretKey, | ||
| guardrails: legacyOtpGuardrails, | ||
| }).valid; |
There was a problem hiding this comment.
🧩 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.tsRepository: 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 -nRepository: 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 -nRepository: 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 -nRepository: 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:
- 1: https://otplib.yeojz.dev/api/@otplib/totp/functions/verifySync.html
- 2: https://github.com/yeojz/otplib/blob/e0da1d1275a42d5dd1f139598755c8e261d4c180/packages/otplib/src/functional.ts
- 3: https://otplib.yeojz.dev/api/otplib/
- 4: https://www.jsdocs.io/package/otplib
- 5: https://www.npmjs.com/package/otplib
- 6: Why sometimes is invalid my token? yeojz/otplib#67
- 7: authenticator always return invalid token when Window OPTION is set to 0 yeojz/otplib#376
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).
1 participant
Summary by CodeRabbit
Bug Fixes
Chores