WIP #1374
WIP #1374
Conversation
WalkthroughDerived a Redis TLS CA value from the environment variable REDIS_TLS_CERT_B64 (decoded and split into lines) and introduced a local Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 2 inconclusive)
✨ Finishing touches
🧪 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/configuration/registrations.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Never use direct string comparisons for ActivityPub IDs; use SHA256 hash-based lookups instead
Use Result helper functions (isError, getError, getValue) and avoid destructuring Result values directly
Awilix uses CLASSIC injection: constructor parameter names must match their registration names
Define routes using decorators rather than direct registration
Prefer immutable entities that emit domain events over mutable entities with dirty flags
Use structured error objects with context in Result types instead of string literals
Prefer class-based handlers with dependency injection over function factories
Files:
src/configuration/registrations.ts
🧠 Learnings (1)
📚 Learning: 2025-09-01T15:36:36.980Z
Learnt from: mike182uk
PR: TryGhost/ActivityPub#1251
File: docs/env-vars.md:67-74
Timestamp: 2025-09-01T15:36:36.980Z
Learning: MQ_PUBSUB_MAX_DELIVERY_ATTEMPTS is a required environment variable for production environments in the TryGhost/ActivityPub project, even though it's hardcoded in dev/pubsub/start.sh for development environments. The retry policy environment variables (MQ_PUBSUB_USE_RETRY_TOPIC, MQ_PUBSUB_RETRY_TOPIC_NAME, MQ_PUBSUB_MAX_DELIVERY_ATTEMPTS) should be documented as they are used in production configuration.
Applied to files:
src/configuration/registrations.ts
🧬 Code graph analysis (1)
src/configuration/registrations.ts (1)
src/pubsub.ts (1)
getFullTopic(18-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
src/configuration/registrations.ts
Outdated
| : Buffer.from( | ||
| process.env | ||
| .REDIS_TLS_CERT_B64 || | ||
| '', | ||
| 'base64', | ||
| ) | ||
| .toString('utf-8') | ||
| .split('\n'), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add error handling for base64 decoding.
The base64 decoding lacks validation. If REDIS_TLS_CERT_B64 contains invalid base64 or is empty, the connection will fail silently or with cryptic TLS errors at runtime.
Consider adding validation:
ca: process.env.REDIS_TLS_CERT ||
(() => {
const b64Cert = process.env.REDIS_TLS_CERT_B64;
if (!b64Cert) {
throw new Error('REDIS_TLS_CERT_B64 is empty');
}
try {
const decoded = Buffer.from(b64Cert, 'base64').toString('utf-8');
if (!decoded.includes('BEGIN CERTIFICATE')) {
throw new Error('Decoded certificate does not appear to be valid PEM format');
}
return decoded;
} catch (err) {
throw new Error(`Failed to decode REDIS_TLS_CERT_B64: ${err.message}`);
}
})(),This provides clear error messages during initialization rather than obscure TLS handshake failures later.
🤖 Prompt for AI Agents
In src/configuration/registrations.ts around lines 136 to 143, the code decodes
REDIS_TLS_CERT_B64 without validation; add explicit checks and error handling so
initialization fails with a clear message if the env var is missing, not valid
base64, or the decoded string doesn't look like a PEM certificate. Implement a
small validation flow: read REDIS_TLS_CERT_B64, throw if empty, attempt
Buffer.from(..., 'base64') inside a try/catch and throw a descriptive error on
failure, convert to utf-8 and verify it contains "BEGIN CERTIFICATE" (or other
PEM markers) and throw if not; use the validated decoded PEM as the ca value (or
fall back to REDIS_TLS_CERT when present).
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/configuration/registrations.ts(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.ts: Never use direct string comparisons for ActivityPub IDs; use SHA256 hash-based lookups instead
Use Result helper functions (isError, getError, getValue) and avoid destructuring Result values directly
Awilix uses CLASSIC injection: constructor parameter names must match their registration names
Define routes using decorators rather than direct registration
Prefer immutable entities that emit domain events over mutable entities with dirty flags
Use structured error objects with context in Result types instead of string literals
Prefer class-based handlers with dependency injection over function factories
Files:
src/configuration/registrations.ts
🧠 Learnings (1)
📚 Learning: 2025-09-01T15:36:36.980Z
Learnt from: mike182uk
PR: TryGhost/ActivityPub#1251
File: docs/env-vars.md:67-74
Timestamp: 2025-09-01T15:36:36.980Z
Learning: MQ_PUBSUB_MAX_DELIVERY_ATTEMPTS is a required environment variable for production environments in the TryGhost/ActivityPub project, even though it's hardcoded in dev/pubsub/start.sh for development environments. The retry policy environment variables (MQ_PUBSUB_USE_RETRY_TOPIC, MQ_PUBSUB_RETRY_TOPIC_NAME, MQ_PUBSUB_MAX_DELIVERY_ATTEMPTS) should be documented as they are used in production configuration.
Applied to files:
src/configuration/registrations.ts
🧬 Code graph analysis (1)
src/configuration/registrations.ts (1)
src/pubsub.ts (1)
getFullTopic(18-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build / Build Docker Images (linux/amd64, amd64)
🔇 Additional comments (1)
src/configuration/registrations.ts (1)
140-140: LGTM!The TLS configuration correctly uses
caFromEnvwhen present and leavestlsundefined otherwise. This is the correct approach for conditional TLS.
| const caFromEnv = process.env.REDIS_TLS_CERT_B64 | ||
| ? Buffer.from( | ||
| process.env.REDIS_TLS_CERT_B64, | ||
| 'base64', | ||
| ).toString('utf8') | ||
| : undefined; |
There was a problem hiding this comment.
Regression: Missing REDIS_TLS_CERT fallback and error handling.
The current implementation has two issues:
-
Regression: The previous review suggested considering
REDIS_TLS_CERTas the primary option withREDIS_TLS_CERT_B64as a fallback, butcaFromEnvnow only checksREDIS_TLS_CERT_B64. -
Missing validation: As flagged in a previous review, the base64 decoding lacks error handling. If
REDIS_TLS_CERT_B64contains invalid base64, the connection will fail with cryptic TLS errors at runtime instead of clear initialization errors.
Apply this diff to address both issues:
- const caFromEnv = process.env.REDIS_TLS_CERT_B64
- ? Buffer.from(
- process.env.REDIS_TLS_CERT_B64,
- 'base64',
- ).toString('utf8')
- : undefined;
+ const caFromEnv = (() => {
+ if (process.env.REDIS_TLS_CERT) {
+ return process.env.REDIS_TLS_CERT;
+ }
+ if (process.env.REDIS_TLS_CERT_B64) {
+ try {
+ const decoded = Buffer.from(
+ process.env.REDIS_TLS_CERT_B64,
+ 'base64',
+ ).toString('utf8');
+ if (!decoded.includes('BEGIN CERTIFICATE')) {
+ throw new Error(
+ 'Decoded REDIS_TLS_CERT_B64 does not appear to be valid PEM format',
+ );
+ }
+ return decoded;
+ } catch (err) {
+ throw new Error(
+ `Failed to decode REDIS_TLS_CERT_B64: ${err instanceof Error ? err.message : String(err)}`,
+ );
+ }
+ }
+ return undefined;
+ })();📝 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.
| const caFromEnv = process.env.REDIS_TLS_CERT_B64 | |
| ? Buffer.from( | |
| process.env.REDIS_TLS_CERT_B64, | |
| 'base64', | |
| ).toString('utf8') | |
| : undefined; | |
| // Derive Redis TLS CA from env, preferring raw PEM over base64 | |
| const caFromEnv = (() => { | |
| if (process.env.REDIS_TLS_CERT) { | |
| return process.env.REDIS_TLS_CERT; | |
| } | |
| if (process.env.REDIS_TLS_CERT_B64) { | |
| try { | |
| const decoded = Buffer.from( | |
| process.env.REDIS_TLS_CERT_B64, | |
| 'base64', | |
| ).toString('utf8'); | |
| if (!decoded.includes('BEGIN CERTIFICATE')) { | |
| throw new Error( | |
| 'Decoded REDIS_TLS_CERT_B64 does not appear to be valid PEM format', | |
| ); | |
| } | |
| return decoded; | |
| } catch (err) { | |
| throw new Error( | |
| `Failed to decode REDIS_TLS_CERT_B64: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| } | |
| } | |
| return undefined; | |
| })(); |
| logging.info( | ||
| `Connecting to Redis at ${host}:${port} with TLS: ${caFromEnv}`, | ||
| ); |
There was a problem hiding this comment.
Security: Avoid logging the full certificate.
The log statement prints the entire TLS certificate (caFromEnv), which is sensitive infrastructure data. Logging full certificates can expose sensitive information in log aggregation systems, monitoring tools, or when logs are shared for debugging.
Apply this diff to log only whether TLS is enabled:
logging.info(
- `Connecting to Redis at ${host}:${port} with TLS: ${caFromEnv}`,
+ `Connecting to Redis at ${host}:${port} with TLS: ${caFromEnv ? 'enabled' : 'disabled'}`,
);📝 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.
| logging.info( | |
| `Connecting to Redis at ${host}:${port} with TLS: ${caFromEnv}`, | |
| ); | |
| logging.info( | |
| `Connecting to Redis at ${host}:${port} with TLS: ${caFromEnv ? 'enabled' : 'disabled'}`, | |
| ); |
🤖 Prompt for AI Agents
In src/configuration/registrations.ts around lines 117 to 119, the log prints
the full TLS certificate via caFromEnv which is sensitive; remove the
certificate from logs and instead log only whether TLS is enabled. Replace the
caFromEnv interpolation with a boolean or string flag (e.g., tlsEnabled =
!!caFromEnv) and change the logging call to include host, port and tlsEnabled
(or "TLS enabled"/"TLS disabled"), ensuring the certificate value is never
written to logs.
| Number(process.env.MQ_PUBSUB_MAX_DELIVERY_ATTEMPTS) || | ||
| Number.POSITIVE_INFINITY, | ||
| Number.POSITIVE_INFINITY, |
There was a problem hiding this comment.
Validate required environment variable for production.
Using Number.POSITIVE_INFINITY as a fallback for MQ_PUBSUB_MAX_DELIVERY_ATTEMPTS could lead to infinite retries if the environment variable is not set in production. This is particularly concerning as it would prevent failed messages from ever being moved to a dead-letter topic.
Based on learnings
Consider adding validation that throws an error during initialization if this variable is missing in production:
const maxDeliveryAttempts = (() => {
const value = Number(process.env.MQ_PUBSUB_MAX_DELIVERY_ATTEMPTS);
if (!value && process.env.NODE_ENV === 'production') {
throw new Error(
'MQ_PUBSUB_MAX_DELIVERY_ATTEMPTS is required in production',
);
}
return value || Number.POSITIVE_INFINITY;
})();Then use maxDeliveryAttempts on line 227 instead of the inline expression.
🤖 Prompt for AI Agents
In src/configuration/registrations.ts around lines 226-227, the inline fallback
to Number.POSITIVE_INFINITY for
Number(process.env.MQ_PUBSUB_MAX_DELIVERY_ATTEMPTS) can cause infinite retries
in production; replace the inline expression with a validated const (e.g.,
compute maxDeliveryAttempts by parsing the env var, and if the parsed value is
falsy and NODE_ENV === 'production' throw an Error indicating
MQ_PUBSUB_MAX_DELIVERY_ATTEMPTS is required), then use that maxDeliveryAttempts
variable on line 227 instead of the current inline expression.
1 participant
No description provided.