fix(core, job): fix class-validator job registry#258
Conversation
Summary of ChangesHello @mizhm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request strengthens the system's security by implementing a worker signature for authentication during the join process. It also improves the reliability of job result data handling through refined transformation logic. Additionally, it includes updates to dependency declarations, which may impact how packages are managed and consumed across the project. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a signature-based authentication mechanism for workers joining the cluster. While this is a valuable security enhancement, the current implementation contains a critical vulnerability that could allow authentication to be bypassed if the signature is not configured. Additionally, the signature comparison method is not secure against timing attacks. I have provided a detailed comment with a suggested code change to address these security issues. The other changes in the pull request are consistent with the new feature and appear correct.
I am having trouble creating individual review comments. Click here to see my feedback.
core-api/src/modules/workers/workers.service.ts (276-281)
The signature validation logic has two critical security vulnerabilities:
- Authentication Bypass: If
WORKER_SIGNATUREis not set in the environment, it defaults to an empty string. A worker can then connect by also providing an empty string, which would bypass the signature check entirely. This should be prevented. - Timing Attack Vulnerability: The
!==operator is not timing-safe for comparing secrets. This could allow an attacker to reconstruct the signature by measuring the time it takes for the comparison to fail.
To address these issues, you should use crypto.timingSafeEqual for the comparison and ensure that an empty signature is not considered valid. Please ensure timingSafeEqual is imported from the crypto module.
Example import:
import { randomUUID, timingSafeEqual } from 'crypto'; const workerSignature = this.configService.get<string>('WORKER_SIGNATURE');
if (!workerSignature || !signature) {
throw new UnauthorizedException(
'Worker signature is not configured or provided.',
);
}
const signatureBuffer = Buffer.from(signature);
const expectedSignatureBuffer = Buffer.from(workerSignature);
if (
signatureBuffer.length !== expectedSignatureBuffer.length ||
!timingSafeEqual(signatureBuffer, expectedSignatureBuffer)
) {
throw new UnauthorizedException('Invalid worker signature');
}There was a problem hiding this comment.
Pull request overview
This PR updates worker registration and job result handling by introducing a worker signature during join, tightening join authorization, and adjusting jobs-registry DTO transforms to better handle incoming result shapes.
Changes:
- Add
signatureto worker join requests across worker/core/proto and generated client types. - Enforce worker
signaturevalidation on the core API join path. - Update jobs-registry DTO transforms for
error,raw, andpayloadextraction/defaulting.
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| worker/tool/tool.ts | Sends signature on join and includes formatting/logging changes. |
| worker/services/core-api/api.ts | Updates generated WorkerJoinDto to include optional signature. |
| worker/bun.lock | Lockfile metadata update (configVersion). |
| package-lock.json | Lockfile regeneration/metadata changes. |
| core-api/src/proto/workers.proto | Adds signature field to gRPC JoinRequest. |
| core-api/src/modules/workers/workers.service.ts | Adds signature verification during worker join. |
| core-api/src/modules/workers/workers.controller.ts | Passes signature through gRPC join to service. |
| core-api/src/modules/workers/dto/workers.dto.ts | Adds signature field to HTTP join DTO. |
| core-api/src/modules/jobs-registry/dto/jobs-registry.dto.ts | Adjusts transforms/defaults for result data fields. |
| console/src/services/apis/gen/queries.ts | Updates generated WorkerJoinDto to include optional signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
l1ttps
left a comment
There was a problem hiding this comment.
This PR looks good! It correctly fixes the class-validator issues in the job registry DTO by adding proper Transform decorators to handle default values and prevent validation errors when fields are undefined or null. The changes make the DTO more robust during deserialization.
No description provided.