fix: normalize booru auth domains#131
Merged
AlejandroAkbal merged 12 commits intomainfrom Mar 16, 2026
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoves percent-decoding of tag strings; centralizes domain normalization, credential deduplication, and sensitive-parameter redaction in BooruAuthManager; expands credential scoping to include optional passwords; makes interceptor URL parsing defensive and includes password in auth-failure events; adds SENSITIVE_AUTH_PARAMS and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Config as ConfigService
participant AuthMgr as BooruAuthManagerService
participant Store as CredentialStore
participant Interceptor as BooruErrorsInterceptor
participant Client as Request Handler
Config->>AuthMgr: onModuleInit(BOORU_AUTH_CONFIG)
AuthMgr->>AuthMgr: normalizeAuthConfig()\n(dedupe, aliases, build keys)
AuthMgr->>Store: persist normalized pools
Client->>AuthMgr: getAvailableCredential(domain/url)
AuthMgr->>AuthMgr: normalizeDomain(domain/url)
AuthMgr->>Store: lookup by normalized domain (user[,password])
alt credential available
AuthMgr-->>Client: return credential (user[,password])
else none
AuthMgr-->>Client: null
end
Client->>Interceptor: request triggers error (auth failure / empty)
Interceptor->>AuthMgr: reportAuthFailure(domain/url,user,password?)
AuthMgr->>AuthMgr: normalizeDomain()\nmark disabled (user or user+password)
AuthMgr-->>Interceptor: ack (redacted failure info)
Interceptor-->>Client: response (sanitized/redacted)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/booru/dto/booru-queries.dto.spec.ts`:
- Around line 6-27: Tests for booruQueryValuesPostsDTO use percent-encoded tag
fixtures but Nest's HTTP adapters decode query params before DTO transforms run;
update the unit tests (the plainToInstance usage) to use decoded strings (e.g.,
'panty_&_stocking_with_garterbelt' and 'rating:safe') or alternatively move
these specs into request-level integration tests that exercise the full HTTP
pipeline so the DTO receives decoded input; locate the failing specs referencing
plainToInstance and booruQueryValuesPostsDTO and replace percent-encoded
fixtures with their decoded equivalents (or convert the test file into an
integration test suite).
In `@src/booru/services/booru-auth-manager.service.ts`:
- Around line 17-20: extractDomainFromUrl currently strips a leading "www."
before calling normalizeDomain, which causes any www.* host to be mapped to apex
aliases and defeats the explicit mapping in domainAliases; stop removing "www."
unconditionally in extractDomainFromUrl (or only strip it when the post-strip
host matches an entry in domainAliases) so normalizeDomain can consult
domainAliases first and respect explicit aliases like 'www.rule34.xxx' declared
in the domainAliases record; adjust domainAliases entries if needed and keep
references to extractDomainFromUrl, normalizeDomain, and domainAliases when
making the change.
- Around line 156-164: normalizeAuthConfig currently concatenates credential
arrays for aliases into the canonical domain without removing duplicates,
causing getCredentialStats and getAvailableCredential to overcount mirrored
accounts; update normalizeAuthConfig so that after merging credentials for each
normalizedDomain (the block that sets normalizedAuthConfig[normalizedDomain])
you deduplicate the pool — e.g., collapse identical credential objects (or a
unique key like username+password) while preserving one instance per account
before returning normalizedAuthConfig; refer to normalizeAuthConfig and the
normalizedAuthConfig[normalizedDomain] merge logic to implement the
de-duplication.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 537a8945-aecd-4029-af3c-1f440add54e1
📒 Files selected for processing (4)
src/booru/dto/booru-queries.dto.spec.tssrc/booru/dto/booru-queries.dto.tssrc/booru/services/booru-auth-manager.service.spec.tssrc/booru/services/booru-auth-manager.service.ts
💤 Files with no reviewable changes (1)
- src/booru/dto/booru-queries.dto.ts
Comment on lines
+17
to
+20
| private readonly domainAliases: Record<string, string> = { | ||
| 'www.rule34.xxx': 'rule34.xxx', | ||
| 'api.rule34.xxx': 'rule34.xxx' | ||
| } |
Contributor
There was a problem hiding this comment.
Restrict www. normalization to explicit aliases.
extractDomainFromUrl() strips www. before normalizeDomain() consults domainAliases, so every www.* host now collapses into the apex credential pool. That is broader than the Rule34-only aliasing in this PR and makes the explicit www.rule34.xxx alias unnecessary.
Suggested change
private extractDomainFromUrl(url: string): string {
try {
- const normalizedUrl = url.startsWith('http') ? url : `https://${url}`
+ const normalizedUrl = /^https?:\/\//i.test(url) ? url : `https://${url}`
const urlObj = new URL(normalizedUrl)
- return urlObj.hostname.replace(/^www\./, '').toLowerCase()
+ return urlObj.hostname.toLowerCase()
} catch (error) {
return url
- .replace(/^(https?:\/\/)?(www\.)?/, '')
+ .replace(/^(https?:\/\/)?/i, '')
.split('/')[0]
.toLowerCase()
}
}Also applies to: 172-181
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/booru/services/booru-auth-manager.service.ts` around lines 17 - 20,
extractDomainFromUrl currently strips a leading "www." before calling
normalizeDomain, which causes any www.* host to be mapped to apex aliases and
defeats the explicit mapping in domainAliases; stop removing "www."
unconditionally in extractDomainFromUrl (or only strip it when the post-strip
host matches an entry in domainAliases) so normalizeDomain can consult
domainAliases first and respect explicit aliases like 'www.rule34.xxx' declared
in the domainAliases record; adjust domainAliases entries if needed and keep
references to extractDomainFromUrl, normalizeDomain, and domainAliases when
making the change.
Member
Author
|
Applied the valid review fixes. Summary:
Validation:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/booru/services/booru-auth-manager.service.ts (1)
90-117:⚠️ Potential issue | 🟠 Major
useris no longer a unique disabled-credential key.
dedupeCredentials()now keeps same-user/different-password entries distinct, butreportAuthFailure(),disableCredentialLocally(), andisCredentialDisabled()still collapse disabled state to${domain}:${user}. After alias merging or key rotation, one bad API key will disable every credential for that username in the canonical pool.Also applies to: 131-134, 169-180
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/booru/services/booru-auth-manager.service.ts` around lines 90 - 117, The disabled-credential key currently collapses to `${domain}:${user}` which wrongly disables all same-user credentials; change the keying to include a unique credential identifier (e.g., credentialId or fingerprint) used by dedupeCredentials so keys become `${normalizedDomain}:${credential.user}:${credentialId}` (or similar unique token). Update reportAuthFailure to read the unique id from AuthFailureEvent and build the new credentialKey, include that id in DisabledCredential, then update disableCredentialLocally and isCredentialDisabled to normalize domain and build/check the same three-part key; also update any broadcasts (broadcastDisabledCredential) and any code paths noted (around the other ranges) that create or compare disabled keys so they use the new unique-key format consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/booru/services/booru-auth-manager.service.ts`:
- Around line 108-111: The log writes raw authFailure.error (from
AuthFailureEvent.error) which may contain secrets; before logging in the block
that calls getDomainStats(normalizedDomain) and logs
authFailure.user/authFailure.error, sanitize or redact any credentials by
replacing values for keys like api_key, auth_pass, token (e.g. with a regex that
masks values or call a new helper sanitizeAuthError(error: string): string) and
then log the sanitized string instead of authFailure.error; update the
console.error call to use the sanitized variable and add unit tests for
sanitizeAuthError if you add it.
---
Outside diff comments:
In `@src/booru/services/booru-auth-manager.service.ts`:
- Around line 90-117: The disabled-credential key currently collapses to
`${domain}:${user}` which wrongly disables all same-user credentials; change the
keying to include a unique credential identifier (e.g., credentialId or
fingerprint) used by dedupeCredentials so keys become
`${normalizedDomain}:${credential.user}:${credentialId}` (or similar unique
token). Update reportAuthFailure to read the unique id from AuthFailureEvent and
build the new credentialKey, include that id in DisabledCredential, then update
disableCredentialLocally and isCredentialDisabled to normalize domain and
build/check the same three-part key; also update any broadcasts
(broadcastDisabledCredential) and any code paths noted (around the other ranges)
that create or compare disabled keys so they use the new unique-key format
consistently.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a360f824-a8b3-4051-a964-49a7074ab707
📒 Files selected for processing (5)
src/booru/dto/booru-queries.dto.spec.tssrc/booru/interceptors/booru-exception.interceptor.spec.tssrc/booru/interceptors/booru-exception.interceptor.tssrc/booru/services/booru-auth-manager.service.spec.tssrc/booru/services/booru-auth-manager.service.ts
Member
Author
|
Per your no-mocks requirement, I removed the new mocked auth/interceptor coverage and replaced it with real Nest-based tests. Follow-up commit: What changed:
Validation:
|
Member
Author
|
Applied the remaining CodeRabbit auth-manager follow-ups. Changes:
Validation:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/booru/interceptors/booru-exception.interceptor.ts (2)
176-186:⚠️ Potential issue | 🟡 MinorApply case-insensitive protocol check.
Same issue as in the auth manager service -
url.startsWith('http')is case-sensitive.🐛 Proposed fix
private extractDomainFromUrl(url: string): string { try { - const normalizedUrl = url.startsWith('http') ? url : `https://${url}` + const normalizedUrl = /^https?:\/\//i.test(url) ? url : `https://${url}` const urlObj = new URL(normalizedUrl) return urlObj.hostname.toLowerCase() } catch (error) { return url - .replace(/^(https?:\/\/)?/, '') + .replace(/^(https?:\/\/)?/i, '') .split('/')[0] .toLowerCase() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/booru/interceptors/booru-exception.interceptor.ts` around lines 176 - 186, The protocol check in extractDomainFromUrl is case-sensitive; update the normalization to detect "http"/"https" case-insensitively (e.g., use a case-insensitive regex like /^https?:\/\//i.test(url) or compare url.toLowerCase().startsWith('http')) so URLs with uppercase protocol schemes are correctly prefixed with "https://" before creating the URL object in extractDomainFromUrl.
101-112:⚠️ Potential issue | 🟠 MajorMissing try-catch for URL parsing.
Unlike the auth manager's
sanitizeUrl, this method lacks error handling. A malformed URL that matches the regex pattern will cause an unhandled exception, potentially breaking the interceptor.🐛 Proposed fix
private sanitizeUrl(url: string): string { + try { const urlObj = new URL(url) // Check each query parameter and redact sensitive ones for (const [key] of urlObj.searchParams.entries()) { if (this.sensitiveParams.some((param) => param.toLowerCase() === key.toLowerCase())) { urlObj.searchParams.set(key, 'REDACTED') } } return urlObj.toString() + } catch (error) { + return url + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/booru/interceptors/booru-exception.interceptor.ts` around lines 101 - 112, sanitizeUrl currently constructs a new URL without error handling so a malformed URL can throw; wrap the URL parsing in a try-catch inside the sanitizeUrl method (the new URL(url) call) and on parse failure return the original input (or a safe fallback) so the interceptor won't crash, while keeping the existing logic that iterates this.sensitiveParams and redacts matching query keys when parsing succeeds.src/booru/services/booru-auth-manager.service.ts (1)
136-150:⚠️ Potential issue | 🔴 CriticalCredential key inconsistency between worker and primary process allows alternate passwords.
When a credential is disabled in a worker,
disableCredentialLocally()stores the password-scoped key (domain:user:password). However, the primary process receives the broadcast credential and stores a user-scoped key (domain:userat line 37 ofcluster.service.ts), while other workers store password-scoped keys from the rebroadcast.This creates a critical inconsistency: if a worker disables
domain:user:passwordA, the primary will reject any use of that user regardless of password (because it has the user-scoped key), but other workers will only reject that specific password. A different passwordpasswordBfor the same user will pass credential checks in the workers but fail in the primary.This could allow requests to succeed in worker processes while failing in the primary, causing unpredictable behavior across the cluster.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/booru/services/booru-auth-manager.service.ts` around lines 136 - 150, The worker and primary use different disabled-credential key formats causing inconsistency; ensure a single canonical key is used cluster-wide by switching to a user-scoped key (domain:user) everywhere. In disableCredentialLocally(), compute the user-scoped key (use normalizeDomain and getCredentialKey or add/overload a getCredentialKey variant that omits password) and add that key to this.disabledCredentials instead of the password-scoped key; when broadcasting in broadcastDisabledCredential(), include the same user-scoped identifier in the payload so the primary and other workers store the identical key (also update any related handler in cluster.service to expect the same user-scoped key).
♻️ Duplicate comments (1)
src/booru/services/booru-auth-manager.service.ts (1)
230-241:⚠️ Potential issue | 🟡 MinorCase-insensitive protocol check recommended.
Line 232 uses
url.startsWith('http')which is case-sensitive. URLs with uppercase protocols likeHTTP://orHTTPS://would incorrectly havehttps://prepended, resulting in an invalid URL.🐛 Proposed fix
private extractDomainFromUrl(url: string): string { try { - const normalizedUrl = url.startsWith('http') ? url : `https://${url}` + const normalizedUrl = /^https?:\/\//i.test(url) ? url : `https://${url}` const urlObj = new URL(normalizedUrl) return urlObj.hostname.toLowerCase() } catch (error) { return url - .replace(/^(https?:\/\/)?/, '') + .replace(/^(https?:\/\/)?/i, '') .split('/')[0] .toLowerCase() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/booru/services/booru-auth-manager.service.ts` around lines 230 - 241, The protocol check in extractDomainFromUrl is case-sensitive and will miss URLs like "HTTP://..."; change the check to a case-insensitive test (e.g., use a regex like /^(https?:\/\/)/i.test(url)) so you only prepend "https://" when the input truly lacks a protocol, then proceed to construct the URL object and extract hostname as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/booru/interfaces/auth-manager.interface.ts`:
- Around line 10-16: Disable credential key construction in cluster service is
inconsistent with booru-auth-manager: when DisabledCredential.password is
present cluster code builds `${credential.domain}:${credential.user}` while
booru-auth-manager uses `${domain}:${encodedUser}:${encodedPassword}`. Update
the IPC handling in cluster.service.ts to build the same credential key as
booru-auth-manager.service.ts (include encoded user and append encoded password
when present) or move the key-building logic into a shared utility used by both
modules; reference the DisabledCredential type and ensure the cluster path uses
the identical `${domain}:${encodedUser}` and optional `:${encodedPassword}`
pattern so password-scoped disables remain consistent across worker and primary.
In `@src/booru/services/booru-auth-manager.service.spec.ts`:
- Around line 34-41: The test cleanup currently uses afterAll to restore
process.env.BOORU_AUTH_CONFIG (referencing originalAuthConfig and
process.env.BOORU_AUTH_CONFIG), which can leak state between tests; change the
teardown to use afterEach instead of afterAll so the block that deletes or
restores process.env.BOORU_AUTH_CONFIG runs after every test, keeping
originalAuthConfig logic the same (check for undefined to delete vs set) but
moving it into an afterEach hook to ensure per-test isolation.
In `@src/booru/services/booru-auth-manager.service.ts`:
- Around line 17-40: Extract the duplicated sensitive parameter list into a
single exported constant (e.g., export const SENSITIVE_PARAMS = new Set([...]))
and replace the local private readonly sensitiveParams occurrences with imports
of that constant; specifically, remove the local sensitiveParams declaration in
the BooruAuthManagerService and in the BooruExceptionInterceptor and import
SENSITIVE_PARAMS where needed, ensuring all references (sensitiveParams) are
updated to use SENSITIVE_PARAMS and updating imports accordingly so both classes
use the shared source of truth.
---
Outside diff comments:
In `@src/booru/interceptors/booru-exception.interceptor.ts`:
- Around line 176-186: The protocol check in extractDomainFromUrl is
case-sensitive; update the normalization to detect "http"/"https"
case-insensitively (e.g., use a case-insensitive regex like
/^https?:\/\//i.test(url) or compare url.toLowerCase().startsWith('http')) so
URLs with uppercase protocol schemes are correctly prefixed with "https://"
before creating the URL object in extractDomainFromUrl.
- Around line 101-112: sanitizeUrl currently constructs a new URL without error
handling so a malformed URL can throw; wrap the URL parsing in a try-catch
inside the sanitizeUrl method (the new URL(url) call) and on parse failure
return the original input (or a safe fallback) so the interceptor won't crash,
while keeping the existing logic that iterates this.sensitiveParams and redacts
matching query keys when parsing succeeds.
In `@src/booru/services/booru-auth-manager.service.ts`:
- Around line 136-150: The worker and primary use different disabled-credential
key formats causing inconsistency; ensure a single canonical key is used
cluster-wide by switching to a user-scoped key (domain:user) everywhere. In
disableCredentialLocally(), compute the user-scoped key (use normalizeDomain and
getCredentialKey or add/overload a getCredentialKey variant that omits password)
and add that key to this.disabledCredentials instead of the password-scoped key;
when broadcasting in broadcastDisabledCredential(), include the same user-scoped
identifier in the payload so the primary and other workers store the identical
key (also update any related handler in cluster.service to expect the same
user-scoped key).
---
Duplicate comments:
In `@src/booru/services/booru-auth-manager.service.ts`:
- Around line 230-241: The protocol check in extractDomainFromUrl is
case-sensitive and will miss URLs like "HTTP://..."; change the check to a
case-insensitive test (e.g., use a regex like /^(https?:\/\/)/i.test(url)) so
you only prepend "https://" when the input truly lacks a protocol, then proceed
to construct the URL object and extract hostname as before.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: c95ceddc-28e6-4498-822d-599fb4886275
📒 Files selected for processing (4)
src/booru/interceptors/booru-exception.interceptor.tssrc/booru/interfaces/auth-manager.interface.tssrc/booru/services/booru-auth-manager.service.spec.tssrc/booru/services/booru-auth-manager.service.ts
Comment on lines
+17
to
+40
| private readonly domainAliases: Record<string, string> = { | ||
| 'www.rule34.xxx': 'rule34.xxx', | ||
| 'api.rule34.xxx': 'rule34.xxx' | ||
| } | ||
| private readonly sensitiveParams = new Set([ | ||
| 'user_id', | ||
| 'api_key', | ||
| 'password', | ||
| 'password_hash', | ||
| 'pass_hash', | ||
| 'auth_user', | ||
| 'auth_pass', | ||
| 'token', | ||
| 'secret', | ||
| 'key', | ||
| 'access_token', | ||
| 'auth_token', | ||
| 'session_id', | ||
| 'session', | ||
| 'login', | ||
| 'username', | ||
| 'user', | ||
| 'hash' | ||
| ]) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting shared sensitiveParams to avoid duplication.
The same list of sensitive parameters is defined here and in src/booru/interceptors/booru-exception.interceptor.ts (lines 22-41). Extract this to a shared constant or the interface file to maintain a single source of truth.
♻️ Proposed refactor
Create a shared constant in a common location:
// src/booru/constants/sensitive-params.constant.ts
export const SENSITIVE_PARAMS = new Set([
'user_id',
'api_key',
'password',
// ... rest of params
])Then import in both files:
+import { SENSITIVE_PARAMS } from '../constants/sensitive-params.constant'
`@Injectable`()
export class BooruAuthManagerService implements OnModuleInit {
private disabledCredentials = new Set<string>()
private authConfig: BooruAuthConfig = {}
private readonly domainAliases: Record<string, string> = {
'www.rule34.xxx': 'rule34.xxx',
'api.rule34.xxx': 'rule34.xxx'
}
- private readonly sensitiveParams = new Set([
- 'user_id',
- // ... rest
- ])
+ private readonly sensitiveParams = SENSITIVE_PARAMS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/booru/services/booru-auth-manager.service.ts` around lines 17 - 40,
Extract the duplicated sensitive parameter list into a single exported constant
(e.g., export const SENSITIVE_PARAMS = new Set([...])) and replace the local
private readonly sensitiveParams occurrences with imports of that constant;
specifically, remove the local sensitiveParams declaration in the
BooruAuthManagerService and in the BooruExceptionInterceptor and import
SENSITIVE_PARAMS where needed, ensuring all references (sensitiveParams) are
updated to use SENSITIVE_PARAMS and updating imports accordingly so both classes
use the shared source of truth.
Member
Author
|
Applied the remaining valid review fixes. Latest commit: Changes:
Validation:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cluster.service.ts (1)
37-50:⚠️ Potential issue | 🟠 MajorDo not log the password-scoped credential key.
credentialKeynow includes the URL-encoded password when present, so this log line writes reversible auth material to primary-process logs.🛠️ Proposed fix
const credential = message.payload as DisabledCredential const credentialKey = this.getCredentialKey(credential.domain, credential.user, credential.password) @@ console.log( - `🔄 Broadcasting disabled credential ${credentialKey} to ${Object.keys(cluster.workers || {}).length - 1} other workers` + `🔄 Broadcasting disabled credential for ${credential.domain} to ${Object.keys(cluster.workers || {}).length - 1} other workers` )Also applies to: 56-65
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cluster.service.ts` around lines 37 - 50, The console.log is emitting credentialKey which contains URL-encoded passwords; change the logging to avoid any reversible auth material by not logging credentialKey directly. Update the broadcast block around getCredentialKey/disabledCredentials and the similar block at the later occurrence (lines referenced in the review) to log either a redacted key (e.g., domain and user only), or a non-sensitive summary such as the number of workers broadcast to; keep getCredentialKey and this.disabledCredentials usage unchanged but ensure logs never include the full credentialKey or password-derived data.src/booru/interceptors/booru-exception.interceptor.ts (1)
88-96:⚠️ Potential issue | 🔴 CriticalKeep auth redaction working for uppercase and unparsable URLs.
The matcher only sees lowercase
http(s)://, and the catch path returns the raw URL. An auth error built fromHTTPS://...or an invalidly encoded upstream URL will bypass redaction and can echo configured booru credentials back to callers. Please mirror the same fix insrc/booru/services/booru-auth-manager.service.tsso logs stay redacted too.🛠️ Proposed fix
- const urlPattern = /https?:\/\/[^\s]+/g + const urlPattern = /https?:\/\/[^\s]+/gi @@ - } catch (error) { - return url + } catch { + return url.replace(/([?&])([^=&]+)=([^&#]*)/g, (match, prefix, key) => { + return this.sensitiveParams.some((param) => param.toLowerCase() === key.toLowerCase()) + ? `${prefix}${key}=REDACTED` + : match + }) }Also applies to: 101-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/booru/interceptors/booru-exception.interceptor.ts` around lines 88 - 96, The sanitizer currently only matches lowercase http(s) and can return raw URLs if sanitizeUrl fails, so update sanitizeErrorMessage (and mirror the same change in booru-auth-manager.service.ts) to use a case-insensitive URL matcher (e.g., make the /https?:\/\/.../ pattern case-insensitive) and protect against unparsable URLs by wrapping the sanitizeUrl(url) call in a try/catch (or checking its result) and, on error or no change, apply a fallback redaction that strips credential fragments (user:pass@) from the URL using a credential-focused regex before returning the message; reference sanitizeErrorMessage and sanitizeUrl so you change both places consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/booru/services/booru-auth-manager.service.ts`:
- Around line 200-211: The dedupeCredentials function currently builds keys with
`${credential.user}:${credential.password}` which can collide if user or
password contains colons; update dedupeCredentials to build an unambiguous key
(e.g., use JSON.stringify([credential.user, credential.password]) or another
escaping/encoding scheme) when creating credentialKey, keep the Map-based dedup
logic and return Array.from(uniqueCredentials.values()) unchanged, and ensure
you handle possible undefined/null user/password values consistently when
forming the key.
---
Outside diff comments:
In `@src/booru/interceptors/booru-exception.interceptor.ts`:
- Around line 88-96: The sanitizer currently only matches lowercase http(s) and
can return raw URLs if sanitizeUrl fails, so update sanitizeErrorMessage (and
mirror the same change in booru-auth-manager.service.ts) to use a
case-insensitive URL matcher (e.g., make the /https?:\/\/.../ pattern
case-insensitive) and protect against unparsable URLs by wrapping the
sanitizeUrl(url) call in a try/catch (or checking its result) and, on error or
no change, apply a fallback redaction that strips credential fragments
(user:pass@) from the URL using a credential-focused regex before returning the
message; reference sanitizeErrorMessage and sanitizeUrl so you change both
places consistently.
In `@src/cluster.service.ts`:
- Around line 37-50: The console.log is emitting credentialKey which contains
URL-encoded passwords; change the logging to avoid any reversible auth material
by not logging credentialKey directly. Update the broadcast block around
getCredentialKey/disabledCredentials and the similar block at the later
occurrence (lines referenced in the review) to log either a redacted key (e.g.,
domain and user only), or a non-sensitive summary such as the number of workers
broadcast to; keep getCredentialKey and this.disabledCredentials usage unchanged
but ensure logs never include the full credentialKey or password-derived data.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 1bdcd531-3786-428d-9f1b-88c9e62307a4
📒 Files selected for processing (5)
src/booru/interceptors/booru-exception.interceptor.spec.tssrc/booru/interceptors/booru-exception.interceptor.tssrc/booru/services/booru-auth-manager.service.spec.tssrc/booru/services/booru-auth-manager.service.tssrc/cluster.service.ts
Member
Author
|
Applied the remaining valid review item. Latest commit: What changed:
Validation:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/booru/interceptors/booru-exception.interceptor.ts`:
- Around line 102-115: sanitizeUrl currently returns the original URL on parse
failure, which can leak sensitive query parameters; update the catch block in
sanitizeUrl to perform a manual, case-insensitive redaction of query parameters
using this.sensitiveParams (e.g., replace occurrences like "(?i)(param)=([^&]*)"
with "$1=REDACTED" or remove the value) so that even when new URL(url) throws,
the returned string has sensitive param values redacted; ensure you iterate
this.sensitiveParams and apply the regex replacements to the original url before
returning it.
In `@src/booru/services/booru-auth-manager.service.ts`:
- Around line 244-266: Update sanitizeErrorMessage to match protocols
case-insensitively (use a global, case-insensitive regex such as changing
/https?:\/\/[^\s]+/g to a /.../gi form) so uppercase HTTP/HTTPS are caught; in
sanitizeUrl, do not return the raw input on parse failure—catch block should
return a safe redacted representation (e.g., strip query/fragment and
credentials or return a constant like "REDACTED_URL") instead of the original
URL so malformed inputs cannot leak secrets; functions to change:
sanitizeErrorMessage and sanitizeUrl.
In `@src/cluster.service.ts`:
- Line 37: The current code uses getCredentialKey(credential.domain,
credential.user, credential.password) to build credentialKey and then logs it,
which risks leaking the password; change the logging to use a password-safe key
by either adding a new helper (e.g., getCredentialKeyWithoutPassword or
getMaskedCredentialKey) that omits or masks the password component, or construct
the log key from getCredentialKey(domain, user, '') and append a fixed mask for
the password; update the logging site that prints credentialKey to use the safe
helper (referencing getCredentialKey and the variable credentialKey) while
keeping the original getCredentialKey for internal use.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 318bdb37-b54d-4abc-b0e7-adb7866ebff9
📒 Files selected for processing (5)
src/booru/interceptors/booru-exception.interceptor.spec.tssrc/booru/interceptors/booru-exception.interceptor.tssrc/booru/services/booru-auth-manager.service.spec.tssrc/booru/services/booru-auth-manager.service.tssrc/cluster.service.ts
Member
Author
|
Applied the remaining valid security-related review items. Latest commit: What changed:
Validation:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/booru/services/booru-auth-manager.service.ts`:
- Around line 236-240: The fallback branch in reportAuthFailure() returns a host
derived from a malformed URL but only strips the protocol and splits on '/',
which leaves query/fragment data (e.g., '?page=...') in the returned string;
change the fallback to first remove protocol then strip any query or fragment
before splitting the path segment: e.g. use url.replace(/^(https?:\/\/)?/i,
'').replace(/[?#].*$/, '').split('/')[0].toLowerCase(); also update the
duplicate extractor in src/booru/interceptors/booru-exception.interceptor.ts to
use the same logic so both places stay aligned.
In `@src/cluster.service.ts`:
- Around line 49-53: The current console.log in cluster.service.ts prints raw
credential identifiers (`credential.domain` and `credential.user`) which may
leak sensitive info and allow control-character injection; replace the raw
interpolation with a sanitized/masked identifier (e.g., create and call a small
helper like maskCredential or maskIdentifier used by the broadcast path) that
strips control characters and masks most characters (keep maybe 1-2 chars or a
deterministic short hash) before logging, and use that masked value in the
message along with existing `scope` and `Object.keys(cluster.workers ||
{}).length - 1` values.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 8a3ab8fe-fa6b-4cba-912c-3f91c45bfb00
📒 Files selected for processing (5)
src/booru/interceptors/booru-exception.interceptor.spec.tssrc/booru/interceptors/booru-exception.interceptor.tssrc/booru/services/booru-auth-manager.service.spec.tssrc/booru/services/booru-auth-manager.service.tssrc/cluster.service.ts
Member
Author
|
Applied the remaining valid review items in
Validation:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/booru/interceptors/booru-exception.interceptor.ts`:
- Around line 100-110: The sanitizeRawUrl and extractDomainFromUrl logic is
duplicated; refactor by extracting the shared URL/domain normalization helpers
into a single utility (e.g., booruUrlUtils) and replace the local
implementations in booru-exception.interceptor.ts (sanitizeRawUrl) and
BooruAuthManagerService (extractDomainFromUrl) to import and use the shared
functions; ensure the helpers preserve current behavior (escaping keys, regex
pattern used in sanitizeRawUrl and the domain extraction algorithm) and update
any tests or call sites to reference the new function names.
In `@src/booru/services/booru-auth-manager.service.ts`:
- Around line 198-207: The credential key builder getCredentialKey currently
leaves domain unencoded which breaks parsing when domain contains ":"; update
getCredentialKey to encode the domain (e.g., use encodeURIComponent like
encodedUser/encodedPassword) so keys become `${encodedDomain}:${encodedUser}` or
`${encodedDomain}:${encodedUser}:${encodedPassword}`, and then update the
counterpart parser getDisabledCredentials (and any other code that splits these
keys around ":" such as the code referenced at lines ~266-270) to decode the
domain via decodeURIComponent when reconstructing domain/user/password values so
encoding/decoding is symmetric.
In `@src/cluster.service.ts`:
- Around line 58-67: Extract the credential-key generation logic from
ClusterService.getCredentialKey into a single shared utility function (e.g.,
createCredentialKey or credentialKey) and replace the duplicate implementations
in ClusterService.getCredentialKey and the auth-manager equivalent with calls to
that shared utility; ensure the utility encodes user and optional password via
encodeURIComponent and returns the same `${domain}:${encodedUser}` or
`${domain}:${encodedUser}:${encodedPassword}` format so both ClusterService and
AuthManager use the identical implementation.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: ad2d7d8b-6a76-4c97-9fc5-a2c0278a516c
📒 Files selected for processing (5)
src/booru/constants/sensitive-auth-params.tssrc/booru/interceptors/booru-exception.interceptor.tssrc/booru/services/booru-auth-manager.service.spec.tssrc/booru/services/booru-auth-manager.service.tssrc/cluster.service.ts
Comment on lines
+100
to
110
| private sanitizeRawUrl(url: string): string { | ||
| let sanitizedUrl = url | ||
|
|
||
| for (const key of this.sensitiveParams) { | ||
| const escapedKey = key.replace(/[.*+?^${}()|[\]\\]/g, '\\$&') | ||
| const pattern = new RegExp(`([?&]${escapedKey}=)[^&#\\s]*`, 'gi') | ||
| sanitizedUrl = sanitizedUrl.replace(pattern, '$1REDACTED') | ||
| } | ||
|
|
||
| return urlObj.toString() | ||
| return sanitizedUrl | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider sharing URL/domain normalization helpers with BooruAuthManagerService.
sanitizeRawUrl and extractDomainFromUrl are effectively mirrored across components; consolidating them will prevent future drift in auth-failure routing behavior.
Also applies to: 174-186
🧰 Tools
🪛 ast-grep (0.41.1)
[warning] 104-104: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(([?&]${escapedKey}=)[^&#\\s]*, 'gi')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/booru/interceptors/booru-exception.interceptor.ts` around lines 100 -
110, The sanitizeRawUrl and extractDomainFromUrl logic is duplicated; refactor
by extracting the shared URL/domain normalization helpers into a single utility
(e.g., booruUrlUtils) and replace the local implementations in
booru-exception.interceptor.ts (sanitizeRawUrl) and BooruAuthManagerService
(extractDomainFromUrl) to import and use the shared functions; ensure the
helpers preserve current behavior (escaping keys, regex pattern used in
sanitizeRawUrl and the domain extraction algorithm) and update any tests or call
sites to reference the new function names.
src/cluster.service.ts
Outdated
Comment on lines
+58
to
+67
| private static getCredentialKey(domain: string, user: string, password?: string): string { | ||
| const encodedUser = encodeURIComponent(user) | ||
|
|
||
| if (password === undefined) { | ||
| return `${domain}:${encodedUser}` | ||
| } | ||
|
|
||
| const encodedPassword = encodeURIComponent(password) | ||
| return `${domain}:${encodedUser}:${encodedPassword}` | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract credential-key generation to a shared utility used by both cluster and auth-manager.
The same key-format logic now exists in multiple files. Any future divergence will silently desync disable behavior across worker/primary paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cluster.service.ts` around lines 58 - 67, Extract the credential-key
generation logic from ClusterService.getCredentialKey into a single shared
utility function (e.g., createCredentialKey or credentialKey) and replace the
duplicate implementations in ClusterService.getCredentialKey and the
auth-manager equivalent with calls to that shared utility; ensure the utility
encodes user and optional password via encodeURIComponent and returns the same
`${domain}:${encodedUser}` or `${domain}:${encodedUser}:${encodedPassword}`
format so both ClusterService and AuthManager use the identical implementation.
Member
Author
|
Applied one more high-value follow-up from the latest review in commit What I changed:
Why this one was taken:
What I intentionally did not change in this pass:
Validation:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/booru/services/booru-auth-manager.service.ts`:
- Line 113: The log at console.error currently prints authFailure.user directly
(alongside normalizedDomain and sanitizedError), which leaks usernames and may
allow control-character injection; update the logging to redact or pseudonymize
the username before logging (e.g., replace with a fixed token like "<redacted>"
or a deterministic hash/hex of authFailure.user) and ensure any control
characters are removed or escaped; locate the console.error in the booru auth
flow where normalizedDomain and sanitizedError are used and change
authFailure.user to the sanitized/redacted value (or call a small helper like
sanitizeUsername or hashUsername) so logs never contain raw credential
identifiers.
- Around line 219-226: The sanitizeErrorMessage function currently only strips
http(s):// URLs via the urlPattern and sanitizeUrl, but leaves non-URL secret
tokens (e.g., auth_pass=..., token=..., password=...) intact; update
sanitizeErrorMessage to additionally scan the message for common key=value
secret patterns (like token, auth_pass, password, secret, api_key) using a
case-insensitive regex and replace their values with a redaction placeholder (or
call sanitizeUrl-like logic) while preserving existing URL sanitization via
sanitizeUrl; ensure you reference the sanitizeErrorMessage and sanitizeUrl
methods so the new token-regex replacement runs before/after the URL replace to
avoid double-processing.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: de0b8326-7927-43bb-894e-3e4c03cf7ebf
📒 Files selected for processing (4)
src/booru/services/booru-auth-manager.service.spec.tssrc/booru/services/booru-auth-manager.service.tssrc/booru/services/credential-key.util.tssrc/cluster.service.ts
Member
Author
|
Addressed the latest two open review items in a focused follow-up commit: 1d55cb1 What changed:
Validation:
|
Member
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/booru/services/booru-auth-manager.service.ts`:
- Around line 238-248: The static-analysis ReDoS warning on the dynamic regex in
sanitizeKeyValueTokens is a false positive because the patterns are built from
the hardcoded SENSITIVE_AUTH_PARAMS (exposed via this.sensitiveParams) which
contains only simple alphanumeric keys and the regex uses a non-backtracking
character class, so update the code by adding a concise comment above
sanitizeKeyValueTokens (or above the sensitiveParams declaration) stating that
sensitiveParams is derived from the trusted, hardcoded SENSITIVE_AUTH_PARAMS
constant, not user input, and that the regex uses safe character classes to
prevent backtracking; reference sanitizeKeyValueTokens and
this.sensitiveParams/SENSITIVE_AUTH_PARAMS in the comment so future reviewers
and static tools understand the safety assumptions.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 22285bf8-edbc-4846-b2bd-226bb184285c
📒 Files selected for processing (2)
src/booru/services/booru-auth-manager.service.spec.tssrc/booru/services/booru-auth-manager.service.ts
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.
Summary
api.rule34.xxxandwww.rule34.xxxresolve to the canonicalrule34.xxxcredential poolValidation
pnpm test -- booru/dto/booru-queries.dto.spec.ts booru/services/booru-auth-manager.service.spec.ts booru/booru.service.spec.tspnpm exec eslint src/booru/dto/booru-queries.dto.ts src/booru/dto/booru-queries.dto.spec.ts src/booru/services/booru-auth-manager.service.ts src/booru/services/booru-auth-manager.service.spec.tsSummary by CodeRabbit
Bug Fixes
Refactor
Documentation/Interfaces
Tests