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.
feat: fix sharing #169
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
Uh oh!
There was an error while loading. Please reload this page.
feat: fix sharing #169
Changes from all commits
22f82853edd8e4104b1b85322da9ff95472d5bd81066d8ba61d5cbd81f7dbec85d02cc6cd672c8f70bd3125373c7655c58d23ad57d8cedd23161d49File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
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.
🧩 Analysis chain
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 99
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 134
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 1235
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 1523
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 513
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 296
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 497
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 962
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 45
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 12031
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 1485
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 45
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 45
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 1069
🌐 Web query:
Drizzle ORM 0.44.7 transaction support PostgreSQL💡 Result:
Drizzle ORM v0.44.7 already has full PostgreSQL transaction support via
db.transaction(...)(commit/rollback handled automatically). You can also pass PostgreSQL-specific transaction options likeisolationLevel,accessMode, anddeferrable. [1]What changed in 0.44.7? Nothing PostgreSQL-specific—v0.44.7 is a small patch release that fixes a durable-sqlite transaction return value issue, not Postgres transactions. [2]
PostgreSQL transaction usage (v0.44.7):
[1]
Nested transactions:
tx.transaction(...)works on PostgreSQL by using SAVEPOINTs under the hood (so nested blocks can roll back independently). [1]Sources
[1] Drizzle ORM docs – Transactions https://orm.drizzle.team/docs/transactions
[2] Drizzle ORM site – v0.44.7 release note (durable sqlite fix) https://orm.drizzle.team/
🌐 Web query:
Drizzle ORM onConflictDoNothing composite unique key💡 Result:
To use
onConflictDoNothingwith a composite unique key in Drizzle, pass an array of columns as thetarget, and make sure your schema actually creates a UNIQUE constraint (or composite PRIMARY KEY) on the same columns.1) Define a composite UNIQUE constraint in the schema
Drizzle supports composite unique constraints via
unique().on(col1, col2). [1]2) Insert with
onConflictDoNothingtargeting that composite keyonConflictDoNothingsupports an explicittarget, and conflict targets are columns (or an array of columns for composite). [2]SQLite note (common pitfall)
If you get
ON CONFLICT clause does not match any PRIMARY KEY or UNIQUE constraint, it usually means your table has a unique index but not a UNIQUE constraint / PK matching the target (this has come up with SQLite in Drizzle). In that case either:UNIQUE(col1, col2)table constraint (like theunique().on(...)example), or.onConflictDoNothing()withouttargetto ignore conflicts on any unique constraint. [2][3]Sources: Drizzle docs on constraints [1] and insert/onConflictDoNothing [2], plus SQLite-related conflict-target issue discussion [3].
Wrap collaborator insert and invite delete in a transaction to prevent race conditions.
The current code is vulnerable to concurrent requests: two simultaneous claims can both pass the existence check and attempt to insert, causing one to fail on the unique constraint
workspace_collaborators_workspace_user_uniqueon(workspaceId, userId). Usedb.transaction()to make both the insert and delete atomic, and handle conflicts withonConflictDoNothing()targeting the composite key.Proposed transactional fix
📝 Committable suggestion
🤖 Prompt for AI Agents
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.
Don’t expose pending invite emails to viewers.
This endpoint allows “viewer” access, but returning
invitesleaks invitee emails to users who cannot manage access. Consider only returning invites to owners/editors.🔐 Suggested change (gate invite visibility)
Also applies to: 99-99
🤖 Prompt for AI Agents
Uh oh!
There was an error while loading. Please reload this page.
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.
Inconsistent null check for
ws.Line 166 checks
if (ws && ...)before accessingws.userId, but line 186 accessesws.slugwithout a null check. WhileverifyWorkspaceAccessshould ensure the workspace exists, it's safer to be consistent.🛡️ Proposed fix
// Send standard invitation email try { // Use slug if available, otherwise fallback to id - const identifier = ws.slug || workspaceId; + const identifier = ws?.slug || workspaceId;📝 Committable suggestion
🤖 Prompt for AI Agents
Uh oh!
There was an error while loading. Please reload this page.
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.
P1: The fallback to
Math.random()creates predictable tokens, which is a critical security vulnerability for invitation systems.In Next.js 16 (Node.js 18+ or Edge Runtime), the Web Crypto API (
crypto) is globally available. Ifcryptois missing for some reason, the application should fail securely rather than silently degrading to insecure token generation.Remove the fallback and reliance on
Math.random.Prompt for AI agents
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.
🧩 Analysis chain
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 45
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 1325
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 45
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 117
🏁 Script executed:
# Read the beginning of the file to see imports and understand the context head -n 50 src/app/api/workspaces/[id]/collaborators/route.tsRepository: ThinkEx-OSS/thinkex
Length of output: 1628
🏁 Script executed:
Repository: ThinkEx-OSS/thinkex
Length of output: 79
🏁 Script executed:
# Read the full route file to check for runtime configuration cat -n src/app/api/workspaces/[id]/collaborators/route.tsRepository: ThinkEx-OSS/thinkex
Length of output: 11627
🏁 Script executed:
# Check next.config for any edge runtime settings cat next.config.tsRepository: ThinkEx-OSS/thinkex
Length of output: 1208
Remove the insecure Math.random fallback from invite token generation.
The fallback to
Math.random()is cryptographically insecure and should never be used for generating security-sensitive tokens like invitations. Since this is a Node.js API route (no edge runtime is configured), usecrypto.randomBytes()unconditionally instead of the defensive Web Crypto API check with unsafe fallback.🔐 Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents
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.
Insecure invite token fallback
generateToken()falls back toMath.random()whencryptois undefined (lines in the 219-229 range). In any runtime where the global Web Crypto API isn’t present (or is blocked), this produces guessable invite tokens, which would allow unauthorized users to brute-force invite links. Since these tokens gate workspace access, the fallback should be removed and token generation should fail hard (or use Node’scryptomodule) rather than degrading to non-cryptographic randomness.Prompt To Fix With AI
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.
Using
crypto.getRandomValues()in Node.js requires importing fromnode:cryptoor using the globalcryptoobject which may not be available in all environments. Verify this works in your deployment environment.Prompt To Fix With AI
Uh oh!
There was an error while loading. Please reload this page.