Add OAuth revocation for mobile logout#9
Conversation
📝 WalkthroughWalkthroughOAuthService and OAuthController are extended with token revocation support, fresh login via prompt handling, and enriched userinfo responses with profile data. OAuth metadata advertises revocation endpoint and picture claim support. Production env validation, HTTP security (helmet/HSTS/HTTPS rejection), sensitive-rate limits, redaction utilities, and safer exception logging were also added. ChangesOAuth Service and Controller Enhancements
Infrastructure & Security
Miscellaneous App Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/modules/auth/oauth.service.ts (1)
268-285:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't advertise
private_key_jwtfor revocation until the endpoint actually supports it.These discovery documents now claim
private_key_jwton the revocation endpoint, butOAuthController.revoke()only acceptsclient_id,token, andtoken_type_hint, andOAuthService.revoke()never parses or verifies a client assertion. Any client bootstrapped from discovery will assume confidential revocation is supported and then fail against the live endpoint.Suggested safe fix
- revocation_endpoint_auth_methods_supported: ['none', 'private_key_jwt'], + revocation_endpoint_auth_methods_supported: ['none'],Apply the same change in both
metadata()andauthorizationServerMetadata()unless you also addclient_assertionhandling to/oauth/revoke.Also applies to: 311-324
🤖 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 `@src/modules/auth/oauth.service.ts` around lines 268 - 285, The discovery metadata wrongly advertises private_key_jwt for revocation even though OAuthController.revoke() and OAuthService.revoke() do not accept or validate client assertions; update the revocation_endpoint_auth_methods_supported entry in both metadata() and authorizationServerMetadata() to remove 'private_key_jwt' (leaving only supported methods like 'none'), or alternatively implement client_assertion parsing/verification in OAuthController.revoke() and OAuthService.revoke() before advertising that method—ensure the change targets the revocation_endpoint_auth_methods_supported arrays so clients bootstrapped from discovery see the correct capabilities.
🤖 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 `@src/modules/auth/oauth.controller.ts`:
- Around line 77-89: OAuthController.revoke currently forwards the raw
`@Headers`('dpop') value to OAuthService.revoke which can be a string[] and later
causes parseJwt/token.split errors; normalize the header beforehand in
OAuthController.revoke by converting array values to a single string (e.g., if
Array.isArray(dpop) use dpop[0]), trim empty strings and pass undefined when
empty, so OAuthService.revoke / verifyDpopProof / parseJwt always receive a
single string or undefined (mirrors AuthGuard behavior).
In `@src/modules/auth/oauth.service.ts`:
- Around line 433-457: The transaction in exchangeRefreshToken uses
revokeRefreshTokenFamily inside runTransaction which queries/updates all
documents for a familyId (oauth_refresh_tokens) and can exceed Firestore
transaction limits; change the revocation design so runTransaction only
sets/reads a single durable family revocation marker document (e.g.,
create/update a family revocations collection keyed by familyId) instead of
iterating generations, update exchangeRefreshToken to write that marker (use
familyId and now) inside the transaction and remove the call to
revokeRefreshTokenFamily there, and modify refresh token validation logic to
check the family revocation marker when rotating/validating tokens (or perform
family-wide deletes in bounded batches outside the transaction) so
per-generation documents are not updated within the transaction.
---
Outside diff comments:
In `@src/modules/auth/oauth.service.ts`:
- Around line 268-285: The discovery metadata wrongly advertises private_key_jwt
for revocation even though OAuthController.revoke() and OAuthService.revoke() do
not accept or validate client assertions; update the
revocation_endpoint_auth_methods_supported entry in both metadata() and
authorizationServerMetadata() to remove 'private_key_jwt' (leaving only
supported methods like 'none'), or alternatively implement client_assertion
parsing/verification in OAuthController.revoke() and OAuthService.revoke()
before advertising that method—ensure the change targets the
revocation_endpoint_auth_methods_supported arrays so clients bootstrapped from
discovery see the correct capabilities.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18f73dd9-2d96-42b0-a8e3-f3e8161cb003
📒 Files selected for processing (2)
src/modules/auth/oauth.controller.tssrc/modules/auth/oauth.service.ts
| @Post('revoke') | ||
| @HttpCode(200) | ||
| async revoke( | ||
| @Body() | ||
| body: { | ||
| client_id?: string; | ||
| token?: string; | ||
| token_type_hint?: string; | ||
| }, | ||
| @Headers('dpop') dpop: string | undefined, | ||
| @Req() request: Request, | ||
| ) { | ||
| return this.oauthService.revoke(body, request, dpop); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant controller/service/guard code
files=(
"src/modules/auth/oauth.controller.ts"
"src/modules/auth/auth.guard.ts"
"src/modules/auth/oauth.service.ts"
)
for f in "${files[@]}"; do
if [ -f "$f" ]; then
echo "===== FILE: $f ====="
wc -l "$f"
sed -n '1,220p' "$f" | cat
else
echo "===== FILE MISSING: $f ====="
fi
done
# Focused extraction around revoke and dpop normalization and parseJwt
echo "===== SEARCH: revoke(...) in controller ====="
rg -n "revoke\\(" src/modules/auth/oauth.controller.ts || true
echo "===== SEARCH: dpop normalization in guard ====="
rg -n "dpop" src/modules/auth/auth.guard.ts || true
echo "===== SEARCH: verifyDpopProof and parseJwt in service ====="
rg -n "verifyDpopProof|parseJwt|split\\('\\.'\\)" src/modules/auth/oauth.service.ts || trueRepository: CheFu-code/chefu-inc-backend
Length of output: 14748
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find revoke implementation and show relevant parts
rg -n "async revoke\\(" src/modules/auth/oauth.service.ts
rgrep=$(rg -n "async revoke\\(" src/modules/auth/oauth.service.ts | head -n1 | cut -d: -f1)
echo "revoke line: $rgrep"
sed -n "$((rgrep-40)),$((rgrep+120))p" src/modules/auth/oauth.service.ts | cat
# Show verifyDpopProof and parseJwt implementations
sed -n '1270,1360p' src/modules/auth/oauth.service.ts | cat
sed -n '1540,1605p' src/modules/auth/oauth.service.ts | catRepository: CheFu-code/chefu-inc-backend
Length of output: 9229
Normalize the dpop header before passing it into the service.
OAuthController.revoke() forwards @Headers('dpop') directly to OAuthService.revoke(). When preflightRecord.dpopJkt is present, revoke() calls verifyDpopProof({ dpop, ... }), which calls parseJwt(dpop) and parseJwt(token) does token.split('.')—a string[] header can therefore throw (TypeError) and surface as a 500 instead of an auth error. AuthGuard already normalizes dpop to a single string before delegating.
Suggested fix
- `@Headers`('dpop') dpop: string | undefined,
+ `@Headers`('dpop') rawDpop: string | string[] | undefined,
`@Req`() request: Request,
) {
- return this.oauthService.revoke(body, request, dpop);
+ const dpop = Array.isArray(rawDpop) ? rawDpop[0] : rawDpop;
+ return this.oauthService.revoke(body, request, dpop);
}🤖 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 `@src/modules/auth/oauth.controller.ts` around lines 77 - 89,
OAuthController.revoke currently forwards the raw `@Headers`('dpop') value to
OAuthService.revoke which can be a string[] and later causes
parseJwt/token.split errors; normalize the header beforehand in
OAuthController.revoke by converting array values to a single string (e.g., if
Array.isArray(dpop) use dpop[0]), trim empty strings and pass undefined when
empty, so OAuthService.revoke / verifyDpopProof / parseJwt always receive a
single string or undefined (mirrors AuthGuard behavior).
| const result = await this.firebaseAdmin | ||
| .db() | ||
| .runTransaction(async transaction => { | ||
| const snapshot = await transaction.get(refreshRef); | ||
|
|
||
| if (!snapshot.exists) { | ||
| return { revoked: false, record: null as RefreshTokenDocument | null }; | ||
| } | ||
|
|
||
| const current = snapshot.data() as RefreshTokenDocument; | ||
| if (current.clientId !== client.id) { | ||
| return { revoked: false, record: current }; | ||
| } | ||
|
|
||
| if (!current.revokedAt) { | ||
| await this.revokeRefreshTokenFamily( | ||
| transaction, | ||
| current.familyId, | ||
| 'mobile_logout', | ||
| now, | ||
| ); | ||
| } | ||
|
|
||
| return { revoked: true, record: current }; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the relevant code in the repo
rg -n "class .*oauth|oauth\.service\.ts|exchangeRefreshToken|revokeRefreshTokenFamily|refreshRef|familyId|runTransaction\(" -S src/modules/auth/oauth.service.ts src/modules/auth || true
# Show the revokeRefreshTokenFamily implementation and callers
rg -n "revokeRefreshTokenFamily" -S src/modules/auth/oauth.service.ts src/modules/auth --hidden || true
# Show exchangeRefreshToken and how it writes refresh token generations/family structure
rg -n "exchangeRefreshToken" -S src/modules/auth/oauth.service.ts src/modules/auth --hidden || true
# Print the specific region around the snippet for context
sed -n '380,520p' src/modules/auth/oauth.service.ts || trueRepository: CheFu-code/chefu-inc-backend
Length of output: 9584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show revokeRefreshTokenFamily implementation (approx around the line numbers found)
sed -n '1380,1480p' src/modules/auth/oauth.service.ts || true
# Show exchangeRefreshToken implementation region around discovered line numbers
sed -n '760,920p' src/modules/auth/oauth.service.ts || true
# Show the refresh token document schema / familyId usage if present near top
rg -n "interface RefreshTokenDocument|type RefreshTokenDocument|revokedAt|familyId|uid|email|createdAt|rotation" src/modules/auth/oauth.service.ts -n || true
sed -n '1,120p' src/modules/auth/oauth.service.ts || trueRepository: CheFu-code/chefu-inc-backend
Length of output: 12829
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where refreshTokenTtlMs / token TTLs are configured in this file or related module
rg -n "refreshTokenTtlMs|tokenTtlSeconds|authorizationCodeTtlMs|clockSkewSeconds|issuer|refresh token ttl" src/modules/auth/oauth.service.ts
rg -n "refreshTokenTtlMs|tokenTtlSeconds" -S src/modules/auth | head -n 50
# Also check if refresh token TTL is explicitly set in config files nearby
rg -n "REFRESH.*TTL|refresh.*ttl|refreshTokenTtl" -S . | head -n 50Repository: CheFu-code/chefu-inc-backend
Length of output: 4177
🌐 Web query:
Firebase/Firestore transaction limits max documents per transaction 500
💡 Result:
Cloud Firestore does not have a 500-document limit per transaction for the total number of documents read or written. Instead, the "500" figure often cited in relation to Firestore transactions and batch writes refers to the limit on the number of field transformations that can be performed on a single document [1][2]. Key clarification on limits: 1. Transaction/Batch Size: While there is no hard "500 document" limit for the total number of documents involved in a single atomic operation (transaction or batched write), Firestore imposes a maximum API request size of 10 MiB [1][2]. Large operations that exceed this size will fail, which is why developers are often advised to keep batches and transactions reasonably sized to avoid hitting this limit [3]. 2. Field Transformations: The explicit limit of 500 applies to the number of field transformations (such as server timestamps, array unions/removals, or numeric increments) that can be performed on an individual document within a single commit or transaction [1][2]. 3. Security Rules Limit: In addition to data limits, there is a limit of 20 document access calls (exists, get, getAfter) allowed per atomic operation (transaction or batched write) within security rules, in addition to the standard 10-call limit that applies to each individual document operation [3][4][1]. For high-volume operations, it is recommended to use bulk writer patterns or parallelize writes rather than attempting to include an excessive number of documents in a single atomic transaction [3].
Citations:
- 1: https://firebase.google.com/docs/firestore/enterprise/quotas-native-mode
- 2: https://firebase.google.com/docs/firestore/quotas
- 3: https://firebase.google.com/docs/firestore/manage-data/transactions
- 4: https://firebase.google.com/docs/firestore/enterprise/transactions
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any cap/cleanup logic around familyId, generation, parentTokenHash, or refresh token revocation.
rg -n "familyId.*(limit|cap|max)|generation.*(cap|limit|max)|parentTokenHash|revokeRefreshTokenFamily\\(|refreshToken.*(delete|cleanup|prune|trim)|expiresAt.*(delete|ttl)" -S src/modules/auth/oauth.service.ts src/modules/auth || true
# Search in repo for scheduled cleanup or TTL index on refresh tokens collection
rg -n "refreshTokens\\(|refresh_token|familyId|OAUTH_REFRESH_TOKEN_TTL_DAYS|revokeRefreshTokenFamily" -S . || trueRepository: CheFu-code/chefu-inc-backend
Length of output: 4766
🌐 Web query:
Firestore transaction limit number of documents read or written 500
💡 Result:
The 500-operation limit for Firestore batched writes was removed in March 2023 [1]. Official Firestore documentation on quotas and limits no longer specifies a numerical limit on the number of documents per transaction or batched write operation [2][3][4]. Instead, transactions and batched writes are constrained by the following technical limits: 1. Maximum API request size: 10 MiB [5][2][4]. Large batches or transactions that exceed this size will fail [5]. 2. Transaction time limits: Transactions have a total time limit of 270 seconds and a 60-second idle expiration time [5][2][4]. 3. Security rules: For transactions and batched writes, there is a limit of 20 document access calls for the entire atomic operation (in addition to the standard 10-call limit per individual document operation) [5][2][3]. 4. Field transformations: There is a maximum limit of 500 field transformations that can be performed on a single document within a transaction or commit operation [2][3][4]. While the 500-operation limit is frequently cited in older tutorials and community discussions [6][7], it is no longer an enforced system limit [1]. Users are advised to manage large batches by reducing the number of documents per operation if they encounter size or performance constraints [5].
Citations:
- 1: https://stackoverflow.com/questions/78346612/does-the-firestore-write-batch-limit-of-500-operations-still-exist
- 2: https://firebase.google.com/docs/firestore/quotas
- 3: https://firebase.google.com/docs/firestore/enterprise/quotas-native-mode
- 4: https://docs.cloud.google.com/firestore/quotas
- 5: https://firebase.google.com/docs/firestore/manage-data/transactions
- 6: https://www.rapidevelopers.com/firebase-tutorial/how-to-perform-transactions-in-firestore
- 7: https://estuary.dev/blog/firestore-limitations/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any cleanup/prune/delete logic for oauth_refresh_tokens
rg -n "oauth_refresh_tokens" -S .
rg -n "delete.*oauth_refresh_tokens|where\\('expiresAt'|expiresAt.*delete|prune|cleanup|TTL|scheduled|cron|job.*refresh" -S src . || trueRepository: CheFu-code/chefu-inc-backend
Length of output: 7425
Fix: Family-wide logout revocation is a structural risk for Firestore transaction failures
In src/modules/auth/oauth.service.ts, exchangeRefreshToken() creates a new refresh-token document on every rotation (same familyId) and there’s no generation cap or repo-visible pruning/cleanup of oauth_refresh_tokens. During logout/revocation, revokeRefreshTokenFamily() runs inside the same Firestore transaction and queries all docs with where('familyId', '==', familyId), then updates every matching generation—without filtering by expiresAt. With an unbounded family, the single transaction can exceed Firestore’s operation constraints (e.g., request size/time), causing the whole transaction to roll back and leaving the family active.
const result = await this.firebaseAdmin
.db()
.runTransaction(async transaction => {
const snapshot = await transaction.get(refreshRef);
if (!snapshot.exists) {
return { revoked: false, record: null as RefreshTokenDocument | null };
}
const current = snapshot.data() as RefreshTokenDocument;
if (current.clientId !== client.id) {
return { revoked: false, record: current };
}
if (!current.revokedAt) {
await this.revokeRefreshTokenFamily(
transaction,
current.familyId,
'mobile_logout',
now,
);
}
return { revoked: true, record: current };
});Use a durable revocation marker stored once per familyId (updated outside the per-generation token documents) and checked during refresh, or revoke in bounded batches outside the transaction.
🤖 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 `@src/modules/auth/oauth.service.ts` around lines 433 - 457, The transaction in
exchangeRefreshToken uses revokeRefreshTokenFamily inside runTransaction which
queries/updates all documents for a familyId (oauth_refresh_tokens) and can
exceed Firestore transaction limits; change the revocation design so
runTransaction only sets/reads a single durable family revocation marker
document (e.g., create/update a family revocations collection keyed by familyId)
instead of iterating generations, update exchangeRefreshToken to write that
marker (use familyId and now) inside the transaction and remove the call to
revokeRefreshTokenFamily there, and modify refresh token validation logic to
check the family revocation marker when rotating/validating tokens (or perform
family-wide deletes in bounded batches outside the transaction) so
per-generation documents are not updated within the transaction.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| helmet({ | ||
| contentSecurityPolicy: false, | ||
| crossOriginEmbedderPolicy: false, | ||
| frameguard: { action: 'sameorigin' }, | ||
| hsts: | ||
| process.env.NODE_ENV === 'production' | ||
| ? { includeSubDomains: true, maxAge: 63072000, preload: true } | ||
| : false, | ||
| referrerPolicy: { policy: 'strict-origin-when-cross-origin' }, | ||
| }), |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/common/rate-limit.middleware.ts (1)
220-225: 💤 Low valueDouble-hashing of bearer tokens is redundant.
bearerIdentifierreturnsbearer:${stableHash(token)}, and then on line 168,stableHash(identifier)is called again on this already-hashed value. This double-hashing is computationally wasteful and doesn't add security value since the first hash already obscures the token.Consider returning the raw token from
bearerIdentifierand letting line 168 handle the single hash, or skip the second hash when the identifier is already a hash.🤖 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 `@src/common/rate-limit.middleware.ts` around lines 220 - 225, The bearerIdentifier function currently returns a hashed token (bearer:${stableHash(token)}) which later gets hashed again by stableHash(identifier); remove this double-hash by returning the raw token identifier instead: change bearerIdentifier to return the un-hashed string (e.g., `bearer:${token}` where token is authorization.slice('Bearer '.length).trim()) and let the caller call stableHash once (where stableHash(identifier) is already used). Update any tests or callers that expect a pre-hashed value to use the single stableHash call at the rate-limit hashing site.
🤖 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 `@src/common/rate-limit.middleware.ts`:
- Around line 27-31: The identifier function in rate-limit.middleware.ts
currently substitutes 'unknown-client'/'unknown-grant' which causes
missing-field requests to share a bucket; change the anonymous identifier
function (identifier: request => ...) to read fieldValue(request, 'client_id')
and fieldValue(request, 'grant_type') into local vars and if either is falsy
return null (so the rate limiter skips identifier-based limiting) otherwise
return the joined "client:grant" string; update any callers expecting a string
to handle null as "no identifier" and rely on IP-based limits.
In `@src/common/security-audit.ts`:
- Around line 50-54: The redaction callback in the
SENSITIVE_TEXT_PATTERNS.reduce currently defaults the prefix to 'token' when no
'=' is present, which turns "Bearer abc123" into "token=[redacted]"; update the
callback in security-audit.ts so that after checking for '=' it next checks for
a space and uses the first word as the prefix (e.g., 'Bearer') and preserves the
original separator (use "prefix [redacted]" when the match used a space, and
"prefix=[redacted]" when it used '='); adjust the reduce callback logic around
SENSITIVE_TEXT_PATTERNS to choose the separator and prefix accordingly so Bearer
tokens become "Bearer [redacted]" while key=value forms remain "key=[redacted]".
In `@src/main.ts`:
- Around line 64-78: The HTTPS check in shouldRejectInsecureRequest currently
trusts a raw x-forwarded-proto header and your code also unconditionally enables
proxy trust elsewhere (the proxy-trust setting), so harden it by (1) removing
blind acceptance of x-forwarded-proto: only consider that header after verifying
the request came from a trusted proxy IP/CIDR, (2) configure the proxy-trust
behavior explicitly instead of enabling it unconditionally (use a configured
list/CIDR or express/app.set('trust proxy', value) driven by a trusted proxies
env var), and (3) update shouldRejectInsecureRequest to prefer request.secure
and only parse x-forwarded-proto when remote address is in the configured
trusted proxies list (or use your framework’s built-in trust-proxy mechanism),
falling back to rejecting when neither proves HTTPS.
---
Nitpick comments:
In `@src/common/rate-limit.middleware.ts`:
- Around line 220-225: The bearerIdentifier function currently returns a hashed
token (bearer:${stableHash(token)}) which later gets hashed again by
stableHash(identifier); remove this double-hash by returning the raw token
identifier instead: change bearerIdentifier to return the un-hashed string
(e.g., `bearer:${token}` where token is authorization.slice('Bearer
'.length).trim()) and let the caller call stableHash once (where
stableHash(identifier) is already used). Update any tests or callers that expect
a pre-hashed value to use the single stableHash call at the rate-limit hashing
site.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5e61bcd-a147-4a9c-a396-dc9c4f4cc8e5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
.env.examplepackage.jsonsrc/common/env.tssrc/common/global-exception.filter.tssrc/common/rate-limit.middleware.tssrc/common/security-audit.tssrc/main.tssrc/modules/ai/ai.controller.tssrc/modules/auth/auth.guard.tssrc/modules/auth/oauth.service.tssrc/modules/muzalo/muzalo.service.tssrc/modules/quantum/quantum.service.ts
✅ Files skipped from review due to trivial changes (1)
- src/modules/ai/ai.controller.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/modules/auth/oauth.service.ts
| identifier: request => | ||
| [ | ||
| fieldValue(request, 'client_id') || 'unknown-client', | ||
| fieldValue(request, 'grant_type') || 'unknown-grant', | ||
| ].join(':'), |
There was a problem hiding this comment.
Fallback values may allow rate-limit bucket sharing.
Using 'unknown-client' and 'unknown-grant' as fallbacks when fields are missing means all requests lacking a client_id or grant_type share the same identifier-based rate-limit bucket. An attacker could omit these fields to dilute legitimate traffic or conversely exhaust the bucket for all malformed requests.
Consider returning null from the identifier function when required fields are missing, which would skip identifier-based limiting and rely solely on IP limits for those requests.
Suggested fix
{
identifier: request =>
- [
- fieldValue(request, 'client_id') || 'unknown-client',
- fieldValue(request, 'grant_type') || 'unknown-grant',
- ].join(':'),
+ {
+ const clientId = fieldValue(request, 'client_id');
+ const grantType = fieldValue(request, 'grant_type');
+ if (!clientId || !grantType) return null;
+ return `${clientId}:${grantType}`;
+ },
identifierLimit: numberEnv('OAUTH_TOKEN_CLIENT_RATE_LIMIT_PER_MINUTE', 40),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| identifier: request => | |
| [ | |
| fieldValue(request, 'client_id') || 'unknown-client', | |
| fieldValue(request, 'grant_type') || 'unknown-grant', | |
| ].join(':'), | |
| identifier: request => { | |
| const clientId = fieldValue(request, 'client_id'); | |
| const grantType = fieldValue(request, 'grant_type'); | |
| if (!clientId || !grantType) return null; | |
| return `${clientId}:${grantType}`; | |
| }, |
🤖 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 `@src/common/rate-limit.middleware.ts` around lines 27 - 31, The identifier
function in rate-limit.middleware.ts currently substitutes
'unknown-client'/'unknown-grant' which causes missing-field requests to share a
bucket; change the anonymous identifier function (identifier: request => ...) to
read fieldValue(request, 'client_id') and fieldValue(request, 'grant_type') into
local vars and if either is falsy return null (so the rate limiter skips
identifier-based limiting) otherwise return the joined "client:grant" string;
update any callers expecting a string to handle null as "no identifier" and rely
on IP-based limits.
| return SENSITIVE_TEXT_PATTERNS.reduce( | ||
| (current, pattern) => current.replace(pattern, match => { | ||
| const prefix = match.includes('=') ? match.split('=')[0] : 'token'; | ||
| return `${prefix}=[redacted]`; | ||
| }), |
There was a problem hiding this comment.
Bearer token redaction loses context in output.
When redacting Bearer tokens, the match is "Bearer abc123" but since it doesn't contain =, the prefix defaults to 'token', producing "token=[redacted]" instead of "Bearer [redacted]". This loses context that could help with debugging.
Suggested fix
return SENSITIVE_TEXT_PATTERNS.reduce(
(current, pattern) => current.replace(pattern, match => {
- const prefix = match.includes('=') ? match.split('=')[0] : 'token';
+ if (match.toLowerCase().startsWith('bearer')) return 'Bearer [redacted]';
+ const prefix = match.includes('=') ? match.split('=')[0] : 'token';
return `${prefix}=[redacted]`;
}),
text,
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return SENSITIVE_TEXT_PATTERNS.reduce( | |
| (current, pattern) => current.replace(pattern, match => { | |
| const prefix = match.includes('=') ? match.split('=')[0] : 'token'; | |
| return `${prefix}=[redacted]`; | |
| }), | |
| return SENSITIVE_TEXT_PATTERNS.reduce( | |
| (current, pattern) => current.replace(pattern, match => { | |
| if (match.toLowerCase().startsWith('bearer')) return 'Bearer [redacted]'; | |
| const prefix = match.includes('=') ? match.split('=')[0] : 'token'; | |
| return `${prefix}=[redacted]`; | |
| }), | |
| text, | |
| ); |
🤖 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 `@src/common/security-audit.ts` around lines 50 - 54, The redaction callback in
the SENSITIVE_TEXT_PATTERNS.reduce currently defaults the prefix to 'token' when
no '=' is present, which turns "Bearer abc123" into "token=[redacted]"; update
the callback in security-audit.ts so that after checking for '=' it next checks
for a space and uses the first word as the prefix (e.g., 'Bearer') and preserves
the original separator (use "prefix [redacted]" when the match used a space, and
"prefix=[redacted]" when it used '='); adjust the reduce callback logic around
SENSITIVE_TEXT_PATTERNS to choose the separator and prefix accordingly so Bearer
tokens become "Bearer [redacted]" while key=value forms remain "key=[redacted]".
| function shouldRejectInsecureRequest(request: Request) { | ||
| if ( | ||
| process.env.NODE_ENV !== 'production' || | ||
| process.env.ENFORCE_HTTPS === 'false' | ||
| ) { | ||
| return false; | ||
| } | ||
|
|
||
| const forwardedProto = String(request.headers['x-forwarded-proto'] || '') | ||
| .split(',')[0] | ||
| .trim() | ||
| .toLowerCase(); | ||
|
|
||
| return !request.secure && forwardedProto !== 'https'; | ||
| } |
There was a problem hiding this comment.
Avoid trusting raw x-forwarded-proto for HTTPS enforcement.
At Line 72-77, HTTPS acceptance relies on a raw header value while Line 131 unconditionally enables proxy trust. If the app is ever reachable outside a trusted proxy path (or a proxy forwards/appends client-supplied headers), x-forwarded-proto: https can bypass the rejection gate.
Suggested hardening
- function shouldRejectInsecureRequest(request: Request) {
+ function shouldRejectInsecureRequest(request: Request) {
if (
process.env.NODE_ENV !== 'production' ||
process.env.ENFORCE_HTTPS === 'false'
) {
return false;
}
-
- const forwardedProto = String(request.headers['x-forwarded-proto'] || '')
- .split(',')[0]
- .trim()
- .toLowerCase();
-
- return !request.secure && forwardedProto !== 'https';
+ return !request.secure;
}- app.set('trust proxy', 1);
+ // Keep this explicit and aligned with your deployment topology.
+ app.set('trust proxy', 1);Also applies to: 131-132
🤖 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 `@src/main.ts` around lines 64 - 78, The HTTPS check in
shouldRejectInsecureRequest currently trusts a raw x-forwarded-proto header and
your code also unconditionally enables proxy trust elsewhere (the proxy-trust
setting), so harden it by (1) removing blind acceptance of x-forwarded-proto:
only consider that header after verifying the request came from a trusted proxy
IP/CIDR, (2) configure the proxy-trust behavior explicitly instead of enabling
it unconditionally (use a configured list/CIDR or express/app.set('trust proxy',
value) driven by a trusted proxies env var), and (3) update
shouldRejectInsecureRequest to prefer request.secure and only parse
x-forwarded-proto when remote address is in the configured trusted proxies list
(or use your framework’s built-in trust-proxy mechanism), falling back to
rejecting when neither proves HTTPS.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/deploy.yml:
- Around line 28-29: The checkout step "Checkout repository" is leaving a
writable token; add the YAML key persist-credentials: false to the
actions/checkout@v4 step (the step named "Checkout repository") so the action
does not persist Git credentials into subsequent steps; update that checkout
step to include persist-credentials: false under its uses entry.
- Around line 29-32: The workflow uses tag refs for actions
(actions/checkout@v4, actions/setup-node@v4, actions/upload-artifact@v4,
actions/attest-build-provenance@v2) and leaves checkout credentials persisted;
update each uses: entry to an immutable commit SHA (replace the `@vX` tags with
the corresponding full commit SHAs) and set persist-credentials: false on the
actions/checkout step (remove or set true only if a later step explicitly needs
repo credentials). Ensure you update every occurrence (checkout at lines
referenced and other uses of setup-node, upload-artifact,
attest-build-provenance) so all actions are pinned to SHAs and checkout no
longer persists credentials by default.
In `@test/quantum-conversations.security.spec.ts`:
- Around line 12-16: The tests instantiate QuantumService with a mocked
db/collection but only assert thrown errors; update the tests (the ones creating
new QuantumService with db: () => ({ collection: jest.fn() })) to also assert
the Firestore mock was never invoked by adding explicit not.toHaveBeenCalled()
checks on the mock collection (and any other mocked Firestore methods) after the
expect(...).toThrow assertions so we guarantee rejection happens before any
Firestore access.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: c17b44aa-8c8d-4e7c-84fa-640a497ee57a
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
.github/workflows/deploy.ymljest.config.cjspackage.jsonsrc/modules/quantum/quantum.service.tstest/auth-security.integration.spec.tstest/quantum-conversations.security.spec.tstsconfig.spec.json
✅ Files skipped from review due to trivial changes (1)
- tsconfig.spec.json
| - name: Checkout repository | ||
| uses: actions/checkout@v4 |
There was a problem hiding this comment.
Disable persisted checkout credentials.
actions/checkout should set persist-credentials: false to avoid leaving a writable token in git config for later steps.
Suggested patch
- name: Checkout repository
uses: actions/checkout@v4
+ with:
+ persist-credentials: false📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: Checkout repository | |
| uses: actions/checkout@v4 | |
| - name: Checkout repository | |
| uses: actions/checkout@v4 | |
| with: | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 28-29: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 29-29: 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/deploy.yml around lines 28 - 29, The checkout step
"Checkout repository" is leaving a writable token; add the YAML key
persist-credentials: false to the actions/checkout@v4 step (the step named
"Checkout repository") so the action does not persist Git credentials into
subsequent steps; update that checkout step to include persist-credentials:
false under its uses entry.
Source: Linters/SAST tools
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the workflow and the specific area mentioned
echo "=== deploy.yml (lines 1-200) ==="
nl -ba .github/workflows/deploy.yml | sed -n '1,200p'
echo
echo "=== Uses lines (full file) ==="
rg -n "^\s*uses:\s*" .github/workflows/deploy.yml
echo
echo "=== Checkout persist-credentials config ==="
rg -n "persist-credentials" .github/workflows/deploy.yml || trueRepository: CheFu-code/chefu-inc-backend
Length of output: 166
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Listing workflow files ==="
ls -la .github/workflows || true
echo
echo "=== deploy.yml exists? ==="
test -f .github/workflows/deploy.yml && echo "Found: .github/workflows/deploy.yml" || echo "Missing: .github/workflows/deploy.yml"
echo
echo "=== deploy.yml first 220 lines (with line numbers via awk) ==="
awk '{printf "%d\t%s\n", NR, $0}' .github/workflows/deploy.yml | sed -n '1,220p'
echo
echo "=== All uses: entries ==="
rg -n "^\s*uses:\s*" .github/workflows/deploy.yml || true
echo
echo "=== Any persist-credentials ==="
rg -n "persist-credentials" .github/workflows/deploy.yml || true
echo
echo "=== Any attest-build-provenance upload-artifact uses ==="
rg -n "attest|provenance|upload-artifact" .github/workflows/deploy.yml || trueRepository: CheFu-code/chefu-inc-backend
Length of output: 2997
🌐 Web query:
actions/checkout@v4 persist-credentials default
💡 Result:
For actions/checkout@v4, persist-credentials defaults to true. The v4 action input persist-credentials is “Whether to configure the token or SSH key with the local git config” and its default is true [1]. The README also states that the auth token is persisted in the local git config by default, enabling scripts to run authenticated git commands, and that the token is removed during post-job cleanup; you can opt out by setting persist-credentials: false [2].
Citations:
- 1: https://github.com/actions/checkout/blob/v4/action.yml
- 2: https://github.com/actions/checkout/blob/v4/README.md
Pin GitHub Actions to commit SHAs (and disable checkout credential persistence).
deploy.yml uses tag refs (actions/checkout@v4, actions/setup-node@v4, actions/upload-artifact@v4, actions/attest-build-provenance@v2), which leaves the pipeline exposed to tag-retarget/supply-chain risk—pin each uses: to an immutable commit SHA. Also, actions/checkout@v4 defaults persist-credentials: true, and this workflow doesn’t override it, so credentials remain available to later steps; set persist-credentials: false unless explicitly required.
Lines 29-32
Also applies to: 63-63, 71-71
🧰 Tools
🪛 zizmor (1.25.2)
[error] 29-29: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 32-32: 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/deploy.yml around lines 29 - 32, The workflow uses tag
refs for actions (actions/checkout@v4, actions/setup-node@v4,
actions/upload-artifact@v4, actions/attest-build-provenance@v2) and leaves
checkout credentials persisted; update each uses: entry to an immutable commit
SHA (replace the `@vX` tags with the corresponding full commit SHAs) and set
persist-credentials: false on the actions/checkout step (remove or set true only
if a later step explicitly needs repo credentials). Ensure you update every
occurrence (checkout at lines referenced and other uses of setup-node,
upload-artifact, attest-build-provenance) so all actions are pinned to SHAs and
checkout no longer persists credentials by default.
Source: Linters/SAST tools
| const service = new QuantumService({ | ||
| db: () => ({ | ||
| collection: jest.fn(), | ||
| }), | ||
| } as never); |
There was a problem hiding this comment.
Assert that Firestore is never called for invalid IDs.
The test names say rejection happens before reaching Firestore, but the assertions currently only check exception type. Add explicit not.toHaveBeenCalled() checks on the Firestore mock to lock that guarantee and prevent silent regressions.
Suggested test tightening
describe('Quantum conversation document id hardening', () => {
+ const collectionMock = jest.fn();
const service = new QuantumService({
db: () => ({
- collection: jest.fn(),
+ collection: collectionMock,
}),
} as never);
it('rejects path-like conversation ids instead of silently rewriting them', async () => {
await expect(
service.deleteConversation(testUser, 'abc123/comments'),
).rejects.toBeInstanceOf(BadRequestException);
+ expect(collectionMock).not.toHaveBeenCalled();
});
it('rejects oversized conversation ids before reaching Firestore', async () => {
await expect(
service.deleteConversation(testUser, 'a'.repeat(141)),
).rejects.toBeInstanceOf(BadRequestException);
+ expect(collectionMock).not.toHaveBeenCalled();
});
});Also applies to: 24-27
🤖 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 `@test/quantum-conversations.security.spec.ts` around lines 12 - 16, The tests
instantiate QuantumService with a mocked db/collection but only assert thrown
errors; update the tests (the ones creating new QuantumService with db: () => ({
collection: jest.fn() })) to also assert the Firestore mock was never invoked by
adding explicit not.toHaveBeenCalled() checks on the mock collection (and any
other mocked Firestore methods) after the expect(...).toThrow assertions so we
guarantee rejection happens before any Firestore access.
2 participants
Adds an OAuth refresh-token revocation endpoint, publishes it in discovery metadata, and revokes mobile refresh-token families on logout.\n\nChecks:\n- npm run lint\n- npm run typecheck\n- npm run build
Summary by CodeRabbit
New Features
Bug Fixes / Security
Chores
Tests