-
Notifications
You must be signed in to change notification settings - Fork 2
Hashing en columnas sensibles #259
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements hashing for sensitive database columns (user tokens and document passwords) to improve security. The main changes include migrating existing plain text tokens and passwords to hashed versions, updating the authentication flow to verify hashed tokens, and fixing transaction handling to support async operations.
Key Changes:
- Implements BLAKE3 hashing with salt for user tokens and document passwords
- Adds database migration to hash existing sensitive data and update the root user ID format
- Updates authentication middleware to support both legacy (unhashed) and new (hashed) tokens during transition period
Reviewed changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/crypto.ts | Adds generateSalt function and updates import path |
| src/utils/user.ts | New file with generateToken function that creates tokens in "id.noise" format |
| src/utils/document.ts | Removes generateToken function and updates root user reference to use dynamic lookup |
| src/utils/validator/user.ts | Adds separate validators for hashed and legacy tokens with updated validation logic |
| src/http/middleware/authorization.ts | Updates authentication to verify both hashed and unhashed tokens with timing attack mitigation |
| src/http/router.ts | Reorganizes imports for consistency |
| src/http/server.ts | Refactors server function to use default parameters and extract handler check logic |
| src/global.ts | Updates token length constant and removes hardcoded root user ULID |
| src/endpoints/user/v1/create.ts | Updates to use dynamic root user lookup and await async user.create |
| src/endpoints/legacy/v2/documents/publish.route.ts | Adds password hashing for published documents |
| src/endpoints/legacy/v2/documents/accessRaw.route.ts | Updates to verify hashed passwords |
| src/endpoints/legacy/v2/documents/access.route.ts | Updates to verify hashed passwords |
| src/endpoints/document/v1/post.ts | Adds password hashing for new documents |
| src/endpoints/document/v1/patch.ts | Adds password hashing when updating documents and removes unused variable reassignment |
| src/endpoints/document/v1/get.ts | Updates to verify hashed passwords |
| src/document/storage.ts | Fixes spelling error in comment and updates import path |
| src/database/query.ts | Updates user.create to be async with token hashing, removes token from UserIndex, adds getRoot method, and reformats SQL queries |
| src/database/migrations/0002.sql | Drops the unique index on user.token column |
| src/database/migrations/0001.sql | New base database schema |
| src/database/migrations.ts | Adds migration infrastructure with pre/post migration hooks and implements migration 0002 to hash existing data |
| src/database/database.ts | Updates migration method to be async, adds root user recovery logic, and prefixes savepoint names |
| src/init.ts | Changes migration call to await and removes root user token initialization |
| src/tasks/sweeper.ts | Updates root user reference to use dynamic lookup |
| .env.example | Replaces JSPB_USER_ROOT_TOKEN with JSPB_USER_ROOT_RECOVERY flag |
Comments suppressed due to low confidence (1)
src/database/database.ts:134
- The transaction method signature does not support async callbacks, but it's being called with an async callback on line 50. The method signature should accept
() => T | Promise<T>and returnT | Promise<T>to properly handle async operations within transactions. Currently, if an async callback is passed, the COMMIT will execute before the async operations complete, potentially leaving the transaction in an inconsistent state.
public transaction<T>(callback: () => T): T {
if (this.database.isTransaction) {
const name = `_${monotonicUlid()}`;
this.exec(`SAVEPOINT ${name};`);
try {
return callback();
} catch (error) {
this.exec(`ROLLBACK TO ${name};`);
throw error;
} finally {
this.exec(`RELEASE ${name};`);
}
}
this.exec("BEGIN IMMEDIATE;");
try {
const result = callback();
this.exec("COMMIT;");
return result;
} catch (error) {
this.exec("ROLLBACK;");
throw error;
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8f509ff to
84a63b8
Compare
También arregla un poco probable bug con las transacciones