Skip to content

fix: JSONB array queries fail with "invalid input syntax for type json"#5

Closed
sid-swirl wants to merge 1 commit into
willchen96:mainfrom
sid-swirl:fix/jsonb-shared-with-contains
Closed

fix: JSONB array queries fail with "invalid input syntax for type json"#5
sid-swirl wants to merge 1 commit into
willchen96:mainfrom
sid-swirl:fix/jsonb-shared-with-contains

Conversation

@sid-swirl
Copy link
Copy Markdown

shared_with is a jsonb column, but the Supabase JS .contains() helper serialises JavaScript arrays using Postgres array syntax ({value}) instead of JSON syntax (["value"]), causing every call to GET /projects to return a 500 for any user with a non-empty email address.

Fixed by replacing .contains() with .filter('shared_with', 'cs', JSON.stringify([email])), which produces valid JSON that PostgREST can pass through to the @> operator correctly.

Also adds forcePathStyle: true to the S3 client, which is required when running against a local MinIO instance (or any S3-compatible endpoint that doesn't support virtual-hosted-style bucket URLs).

…le for S3

The Supabase JS .contains() helper serialises JS arrays using Postgres
array syntax ({value}) rather than JSON syntax (["value"]), which causes
a 'invalid input syntax for type json' error on jsonb columns.

Replace .contains('shared_with', [email]) with
  .filter('shared_with', 'cs', JSON.stringify([email]))
in two places:
  - backend/src/routes/projects.ts  (GET /projects)
  - backend/src/lib/access.ts       (listAccessibleProjectIds)

Note: tabular.ts already used JSON.stringify correctly.

Also wrap the GET /projects handler in try/catch so unexpected errors
surface as a JSON response with a message rather than a silent 500.

Add forcePathStyle: true to the S3 client so path-style URLs are used
(`endpoint/bucket/key` instead of `bucket.endpoint/key`). This is
required for local MinIO and also works correctly with Cloudflare R2.
Copy link
Copy Markdown

@osama-ata osama-ata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

endorsing these modifications

@willchen96
Copy link
Copy Markdown
Owner

Thanks for this. I manually ported the two intended fixes:

  • replaced shared_with .contains(...) calls with .filter("shared_with", "cs", JSON.stringify(...)) for JSONB containment
  • added forcePathStyle: true to the S3 client for MinIO/S3-compatible local storage

I did not merge the PR directly because the branch also includes unrelated changes from an older codebase state, including changes to project folder handling and schema/files that would conflict with newer work. Closing this PR after porting the relevant fixes.

@willchen96 willchen96 closed this May 10, 2026
cpatpa pushed a commit to cpatpa/PIP that referenced this pull request May 18, 2026
Discovery (phase-20-findings.md §2 item 8) flagged that
/auth/register has no email-ownership verification: domain
allowlist gates registration but anyone who controls a
domain-allowed inbox can register without proving ownership.
A staffer leaving the firm could be impersonated by anyone who
guessed they hadn't already registered.

Approach mirrors the password-reset flow (same hashed-token
pattern, same email shape, same audit-event style):

  migrations/0038_email_verification.sql (new)
    Adds three columns to public.users:
      email_verified_at      (timestamptz, NULL until confirmed)
      email_verify_token_hash (text)
      email_verify_expires_at (timestamptz)
    Backfills email_verified_at = now() for every existing
    user (they predate the flow). New post-deploy registrations
    start with NULL.
    Partial index on (email_verify_token_hash) WHERE NOT NULL so
    the lookup stays cheap even when most rows have cleared the
    field after verification.

  lib/users.ts
    registerWithPassword: issues a 24h sha256-hashed token at
      registration time, stashes the hash in the new column,
      sends a "Confirm your PIP account" email outside the
      transaction so SMTP hiccups don't roll back the user row.
    completeEmailVerification(rawToken): validates the token
      against the stored hash + expiry, stamps email_verified_at,
      clears the token fields, audits user.email-verify.complete.
    resendVerificationEmail(email): silently no-ops on
      non-existent / disabled / already-verified / Entra-only
      users so an attacker can't enumerate which emails are
      registered-but-unverified.
    verifyCredentials: rejects unverified accounts with the new
      EmailNotVerifiedError (after the bcrypt check, so this
      gate doesn't itself enumerate).
    upsertEntraUser: stamps email_verified_at automatically -
      the OAuth provider verified ownership, no need to re-prove.

  routes/auth.ts
    New routes:
      POST /auth/verify-email      { token } -> { ok, email, onboarded }
      POST /auth/resend-verification { email } -> { ok: true } (always)
    verify-credentials response gains a `code: "email_not_verified"`
    field on the 403 so the frontend can render a "resend
    verification email" affordance instead of generic "invalid
    credentials".

  tests/unit/emailVerification.test.ts pins:
    - Error-class names and messages (frontend depends on these)
    - completeEmailVerification rejects empty/short tokens
      without touching the DB
    - resendVerificationEmail silently no-ops in every
      enumeration-resistance branch

Frontend changes deferred:
  - /verify-email page that POSTs the token and renders the
    outcome
  - "Resend verification email" button on the unverified-login
    response
Both land alongside the 20c correctness pass that's already
queued; the backend half is the security gate so it ships now.

https://claude.ai/code/session_01Cra9usofyvDbTzCnY1dqmf
cpatpa pushed a commit to cpatpa/PIP that referenced this pull request May 18, 2026
Three pieces of frontend wiring that were deferred during 20b
because the backend gates are independently enforced. With this
commit the user-visible flow matches the server-side semantics.

  /verify-email page (Phase 20b willchen96#5)
    New route at frontend/src/app/verify-email/page.tsx. Reads the
    `token` query parameter, POSTs it to /auth/verify-email, and
    renders one of four states: loading / missing / success /
    error. Middleware updated to allowlist the route as public.
    The page mirrors the existing /reset-password style so the
    out-of-app flow feels consistent.

  Sign-out -> /auth/revoke (Phase 20b willchen96#6)
    AuthContext.signOut now POSTs /auth/revoke before clearing
    the NextAuth cookie. Best-effort: a failed revoke (network
    blip, server down) is swallowed so the local sign-out itself
    isn't blocked. The bearer's jti lands on the server-side
    denylist immediately for the happy path.

  Resend-verification affordance (Phase 20b willchen96#5)
    The /auth/verify-credentials response distinguishes "wrong
    password" (401) from "email not verified" (403 with
    code=email_not_verified), but NextAuth's signIn() collapses
    every non-2xx into a generic CredentialsSignin error so we
    can't read the code at the frontend. Pragmatic workaround:
    show a "Resend the verification link" button whenever a
    login fails AND the email field has a value. The endpoint is
    enumeration-resistant (returns 200 regardless of whether the
    email is registered) so exposing it without a precheck is
    safe.

Three new API client helpers in pipApi.ts:
  revokeBackendSessionToken()   POST /auth/revoke
  verifyEmailToken(token)       POST /auth/verify-email
  resendVerificationEmail(email) POST /auth/resend-verification

https://claude.ai/code/session_01Cra9usofyvDbTzCnY1dqmf
cpatpa pushed a commit to cpatpa/PIP that referenced this pull request May 19, 2026
…ontextAssembly

Fifth split step. Moves the pre-LLM context assembly path out of
chatTools.ts into a dedicated module. Five functions in scope:

  resolveDoc / resolveDocLabel  - slug <-> identity translation
  citationReminder              - per-doc citation-shape preamble
                                  (used internally by the runner)
  enrichWithPriorEvents         - append tool-activity summary to
                                  the prior assistant message so
                                  the model doesn't forget what
                                  it just did
  buildMessages                  - assemble the LLM-shape message
                                  array (system prompt + history
                                  + per-doc availability list)
  buildDocContext                - load user-attached document
                                  metadata into doc{Index,Store}
                                  (also pulls in document_ids
                                  from prior assistant events so
                                  generated docs stay accessible
                                  after the turn that created them)
  buildProjectDocContext         - same but project-scoped, with
                                  folder-path lookup
  buildWorkflowStore             - built-in + user-owned + shared
                                  assistant workflows

  lib/chat/contextAssembly.ts (new, 511 lines)
    Verbatim extraction. The only structural change is fixing the
    `./builtinWorkflows` dynamic import to `../builtinWorkflows`
    after the path depth shift.

  lib/chatTools.ts
    Six inline declarations stripped. The five exported functions
    are re-exported from the new module so routes/chat.ts and
    routes/projectChat.ts keep working without import changes.
    `resolveDoc`, `resolveDocLabel`, `citationReminder` are also
    imported back into chatTools.ts for the internal references
    in runToolCalls and the streaming runner.

chatTools.ts line count: 3633 -> 3181 (-452 lines, -12%).
Total since session start: 4305 -> 3181 (-26%).

The 13 chatTools characterisation tests stay green, confirming
resolveDoc / resolveDocLabel / extractAnnotations behave
identically. The DB-heavy functions (enrichWithPriorEvents,
buildDocContext, buildProjectDocContext, buildWorkflowStore)
don't have characterisation tests yet -- those need the
supertest harness scheduled for 20f. The move was checked
against the full test suite + TypeScript build to ensure no
existing call sites broke.

Remaining 20d splits:
  documents/docxGenerate.ts (~500 lines)
  documents/docxEdit.ts (~400 lines)
  tools/*.ts (~1100 lines)
  runner.ts (~600 lines)

https://claude.ai/code/session_01Cra9usofyvDbTzCnY1dqmf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants