Commits
The testOrgs allowlist (['4def6a77d6a','acc701627cb']) hard-coded two
org IDs that were silently suppressed with PERMANENT_ERROR before any
network call. Hard-coded org IDs in source don't belong here; the
NCMEC_ENV-based sandbox/production routing already handles test vs.
prod submissions.
Co-Authored-By: pi
* fix(mrt): guard client date rendering against unparseable createdAt
Unguarded date-fns format/formatDistanceToNow calls threw
RangeError: Invalid time value on an unparseable createdAt, unmounting
the task detail and queue preview into the "Something Went Wrong" error
boundary. Add safeFormat/safeFormatDistanceToNow helpers that fall back
to "Unknown" for invalid dates, and use them on the MRT render path.
Client-side counterpart to the server-side decision-log fix in #913.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AKvvCLkcC7LM1znjzvDjDA
* add guard on more functions to prevent errors on invalid timezone formats.
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Juan Mrad <juansmrad@gmail.com>
getOldestJobCreatedAtForExistingQueue fetched the first waiting/delayed
job with queue.getJobs([state], 0, 0), but BullMQ's getJobs defaults to
descending order, so index 0 is the most recently added job. The MRT
queues dashboard's Oldest Task Age column therefore showed the newest
task's age. Switch to BullMQ's getWaiting/getDelayed getters, which
return jobs oldest-first.
Adds a regression test that enqueues an older job then a newer one and
asserts the older createdAt is returned; it fails against the previous
implementation. The dummy-job payload builder it needs was previously
copied per test file, so it is extracted into a shared
test/fixtureHelpers/makeDummyMrtJobPayload.ts fixture (using
instantiateOpaqueType instead of eslint-disabled type assertions).
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Juan Mrad <juansmrad@gmail.com>
Add paths filter so the GitHub Actions security scan only triggers
when .github/workflows/ files are modified, instead of on every PR.
* Make Scylla-backed features (item investigation, user strikes) optional
* Address review: extract shared flag helper, use @ts-expect-error
Resolve CodeRabbit nitpicks on PR #918:
- Extract the ITEM_INVESTIGATION_AND_STRIKES_ENABLED parsing into a single
exported helper (itemInvestigationAndStrikesEnabled) in noOpScylla.ts; the
iocContainer factory and the unit test now share it instead of keeping two
copies in sync (DRY).
- Keep the helper a pure function of its argument (env is read at the call
site) so the default-enabled test is independent of the ambient environment.
- Replace the 'as unknown as' cast in NoOpScylla.insert() with a scoped
@ts-expect-error and justifying comment, per repo guidelines.
Co-Authored-By: Rovo Dev <rovodev@atlassian.com>
* Fixed formatting
* ci: re-trigger E2E (investigate investigation.spec flake vs regression)
* Gate user strike logic behind ITEM_INVESTIGATION_AND_STRIKES_ENABLED flag
When the flag is false, applyUserStrikeFromPublishedActions early-returns
to avoid running strike threshold checks against always-zero counts from
the NoOpScylla, which would cause escalation actions to fire unexpectedly.
---------
Co-authored-by: Rovo Dev <rovodev@atlassian.com>
Co-authored-by: Juan Mrad <juansmrad@gmail.com>
* fix(mrt): store null for unparseable item createdAt in decision log
Submitting a decision failed with a 500 ("Job submission failed. Please
try again.") for any job whose item carries a truthy but unparseable
createdAt value. `#logDecision` passed `new Date(itemCreatedAtField)`
straight into the `item_created_at` timestamptz column; an unparseable
value yields an Invalid Date, which the pg driver serializes to a NaN
string that Postgres rejects (22007), failing the whole decision insert.
The decision is never recorded and the job is never removed, so the task
is stuck in the queue.
Normal item submissions can't reach this state because the DATETIME field
handler validates dates at intake. A bad value only arrives via a path
that skips that validation (e.g. system-generated reports, or a createdAt
role mapped to a non-DATETIME field).
Normalize at the write boundary: parse the value and store null when it
is not a valid date. The column is already nullable, so the decision
records and the task clears. Adds a unit regression test for the
normalization helper.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEU6YHFiBYfTqjgM5Vyt9n
* docs(changelog): note the unparseable createdAt decision fix
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEU6YHFiBYfTqjgM5Vyt9n
* fix(mrt): preserve epoch 0 in parseItemCreatedAt
Address review: `!value` treated a numeric 0 (a valid 1970-01-01 epoch) as
empty. Guard only null/undefined/empty-string instead, and cover epoch 0 in
the test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEU6YHFiBYfTqjgM5Vyt9n
* fix(mrt): record unparseable item createdAt values
Address review: surface invalid createdAt values instead of silently
nulling them. When a present createdAt can't be parsed, emit a tracer
span (job id, org id, the raw value) so the bad data is diagnosable and
can be backfilled. The decision still saves with a null item_created_at.
new Date() returns an Invalid Date rather than throwing, so this detects
the invalid parse rather than wrapping in try/catch.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEU6YHFiBYfTqjgM5Vyt9n
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* test: add MRT video playback e2e coverage
Co-Authored-By: Pi <pi@users.noreply.github.com>
* test: use Playwright base URL for media fixtures
Co-Authored-By: Pi <pi@users.noreply.github.com>
---------
Co-authored-by: Pi <pi@users.noreply.github.com>
* fix: close express-session store on api shutdown
connect-pg-simple keeps a recurring pruneSessions timer that calls
pool.query on its pool. makeApiServer created the store with the shared
KyselyPgPool but never closed it, so the timer kept running after
shutdown. In tests this leaked a setInterval per makeMockedServer() call
that fired pool.query on the harness's already-closed pinned connection
after each test, logging "Failed to prune sessions: Client was closed
and is not queryable" indefinitely and hanging the test worker — the
loop that forced the manual cancellation of CI run 27951870325.
Hold a reference to the store instance and call its close() in the
shutdown path. ownsPg is false (we pass in our own pool), so close() only
stops the prune timer and won't end the shared pool.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test: migrate DB-backed tests to transactional harness
Move the remaining DB-backed server tests onto the transaction-rollback
harness from #732 so they get per-test isolation with no hand-written
cleanup.
- Convert the makeMockedServer/getBottle-based tests to
makeTransactionalTestWithFixture, dropping manual org/user/queue/action
deletes and KyselyPg.destroy() teardown: userKyselyPersistenceFindByEmailAndOrg,
resolveSamlUser, and the MRT module tests (CommentOperations, JobRouting,
QueueOperations, ReporterInvalidation).
- Rewrite moderationConfigService and manualReviewToolService so every test
is self-contained: each creates its own fresh org (rolled back
automatically) rather than sharing a suite-scoped org. This removes the
cross-test ordering/accumulator dependency in moderationConfigService and
the hardcoded staging-seed-data dependency in manualReviewToolService.
Scylla-touching tests (itemInvestigationService) keep uid-based isolation,
since the Postgres-only harness can't roll those back.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: migrate UserReportSweep and userStrikeService to transactional harness
Two DB-backed test suites were missed by the initial migration commit:
- manualReviewToolService/modules/UserReportSweep.test.ts: a sibling of
the four MRT module tests already migrated (CommentOperations, JobRouting,
QueueOperations, ReporterInvalidation). It used makeTestWithFixture +
getBottle with hand-written cleanup (createOrg/createUser/createMrtQueue
teardown + KyselyPg/KyselyPgReadReplica.destroy()). Converted to
makeTransactionalTestWithFixture; cleanup is now automatic rollback.
- userStrikeService/userStrikeService.test.ts: used beforeAll/afterAll with
a shared getBottle container and uid-based isolation. Converted to
makeTransactionalTestWithFixture for true per-test rollback isolation.
Also fixes a copy-paste describe block name ('Item Investigation Service'
-> 'User Strike Service').
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: resolve typecheck errors in migrated test files
Two files migrated by the prior commit had TypeScript errors that broke
`tsc` (and therefore `check_api_server` and the e2e server start):
- manualReviewToolService.test.ts: the 'records an AUTOMATIC_CLOSE decision
with no human reviewer' test was left as a bare `it(...)` referencing an
out-of-scope `mrtService` and hardcoded staging `orgId`/'queueId'
('e7c89ce7729'/'1') — the very seed-data dependency the migration was
meant to remove. Convert it to `testWithQueue()` like its siblings, using
the fixture's `mrtService`/`org.id`/`queue.id`.
- moderationConfigService.test.ts: the 'should return actions for a rule
scoped to the caller org' snapshot read `it.id` from
`getActionsForRuleId`, which returns `{ action, parameters }[]` —
should be `it.action.id` (as on main). Also add the now-present
`email` field role to the #createUserType inline snapshot.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: create real user in migrated JobRouting fixture
The transactional JobRouting fixture accidentally replaced the pre-migration
createUser() call with a bare uid(). createManualReviewQueue validates queue
users, so setup failed after creating org/item-type rows but before the fixture
returned. Because makeTestWithFixture cannot run cleanup when setup throws,
the outer transaction stayed idle-in-transaction and later tests blocked on the
item_type_versions materialized-view refresh, timing out check_api_server.
Keep the transactional harness, but create a real user and pass user.id to the
queue setup.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: restore QueueOperations org-scoping tests
The rebase conflict resolution for QueueOperations.test.ts accidentally kept
the pre-#872 branch side and dropped the two-org org-scoping regression tests
that now exist on main. Restore testWithTwoOrgs and its cross-org access tests,
using the transactional harness.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: configure decision-reason settings through service API
Replace the decision-reason test helpers' direct writes to
manual_review_tool_settings with a behavior-shaped helper that uses the
ManualReviewToolService update API. The tests still exercise persisted org
settings through submitDecision, but no longer couple setup to table and column
names.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* style: remove api shutdown explanatory comments
Remove the comments added around the express-session store shutdown while keeping the shutdown behavior unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* ci: stop logging every DB query in test runs
Set DATABASE_PRINT_LOGS=false in .env.githubci (used by the docker-compose
`test` service for unit/integration tests) and comment it out in
server/.env.example (copied to server/.env by the e2e workflow) so the
default is off. Kysely still logs query errors regardless, so failures
remain visible; this only suppresses the per-query SQL/params/duration
spam that made CI logs unreadable.
Co-Authored-By: pi <pi@earendil-works>
* ci: drop redundant comment in server/.env.example
Co-Authored-By: pi <pi@earendil-works>
---------
Co-authored-by: pi <pi@earendil-works>
Co-authored-by: Cassidy James <cassidyjames@roost.tools>
Co-authored-by: Pi <pi@earendil.works>
* fix: Support non-standard scylla ports in DB Migrator
* better compat
* whitespace
* test(ncmec): add field coverage and submission tests
Add XML field-coverage regression tests for NCMEC report builders,
including XSD-order locks for the ipCaptureEvent ordering regression
class. Add a submitReport integration test using a stubbed fetchHTTP seam
against real Postgres, plus a fixture helper for NCMEC org settings.
Canonicalise webhook-sourced ipCaptureEvent objects before XML rendering
so webhook key order cannot produce out-of-order CyberTip XML.
Co-Authored-By: pi
* fix(ncmec): correct fileDetails ipCaptureEvent XSD ordering; harden tests
buildFileDetailsObject emits <ipCaptureEvent> in the wrong position relative
to <industryClassification>/<originalFileHash>. PR #869 moved it BEFORE
industryClassification, claiming NCMEC's live validator required that — but
a probe against exttest.cybertip.org proves the opposite:
- Variant A (#869 order, ipCaptureEvent BEFORE industryClassification):
REJECTED — cvc-complex-type.2.4.a: Invalid content was found starting
with element 'industryClassification'. One of '{deviceId, details,
additionalInfo}' is expected.
- Variant B (canonical XSD order, ipCaptureEvent AFTER
industryClassification/originalFileHash): ACCEPTED, responseCode=0.
#869 misread the original #856 error (which fires when an element lands in
the trailing deviceId/details/additionalInfo slot) and moved ipCaptureEvent
backwards to a position NCMEC actually rejects. #869's own PR description
admits the live re-test was never run before merge. This commit restores the
canonical XSD order, which the live validator accepts.
Reorders buildFileDetailsObject and the FileDetails type so ipCaptureEvent
follows industryClassification and originalFileHash, and updates the
ncmecReporting.builders.test.ts 'preserves XSD insertion order (Appendix C)'
assertion to anchor the correct order (the test had been updated in #869 to
anchor the wrong one).
Also includes test-hygiene improvements to the NCMEC #843 test work:
- fieldCoverage.test.ts: strengthen the renderXml assertion from a bare
length>0 check to per-scenario structural XML markers (present/absent),
so the js2xml serialization path is actually exercised. Remove the dead
XSD.fileDetails array (buildSubmitReportObject's <report> envelope never
contains <fileDetails>; its ordering is locked in
ncmecReporting.builders.test.ts).
- ncmec-submission.integ.test.ts: extract a shared RecordedCall type,
replace submitCall! non-null assertions with an early-fail type guard,
and type the stub generic against the real FetchHTTP/CoopRequestQuery
contract (removing the as-never param, inline cast, and final broad
as-unknown-as-FetchHTTP). Requires exporting HandleResponseBody from
networkingService (additive type-only export).
Co-Authored-By: pi
* fix(mrt): org-scope accessible-queue mutations (GHSA-mf74-gf5j-hxr9)
addAccessibleQueuesForUser / removeAccessibleQueuesForUser accepted an
arbitrary queueId / userId with no org-scoping, so an authenticated user
in Org A could grant (or revoke) access to queues and users belonging to
Org B. Thread the caller's orgId through the resolver, service wrapper,
and QueueOperations write layer, and reject (403 AccessibleQueueNotInOrgError)
if any target queue or user does not belong to the caller's org.
Regression tests cover both IDOR dimensions (cross-org queueId and
cross-org userId) for both add and remove, plus a same-org happy path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(mrt): use transactional test harness, trim comments
- Switch the accessible-queue IDOR tests to makeTransactionalTestWithFixture
(per-org rollback, no manual cleanup).
- Drop GHSA references and "what was" narration from code comments; keep
only minimal present-tense notes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(mrt): assert original user keeps access after cross-org revoke attempt
Strengthen the GHSA-mf74 regression test for removeAccessibleQueuesForUser:
after a rejected cross-org revoke, also assert the attacker still sees their
own queue — confirming the rejected call left the original access intact.
Co-Authored-By: pi
* fix(mrt): org-scope create/update queue userIds (GHSA-mf74-gf5j-hxr9)
createManualReviewQueue and updateManualReviewQueue accepted un-scoped
userIds and wrote them straight into users_and_accessible_queues, the
same un-scoped-input shape fixed in add/removeAccessibleQueuesForUser.
create always creates the queue in the caller's org (orgId from the
invoker) but granted access to users in other orgs; update's queue-row
write was org-scoped but the join-table writes ran un-scoped inside the
transaction.
Refactor assertUsersAndQueuesInOrg into composable assertQueuesInOrg +
assertUsersInOrg (add/remove callers unchanged), then guard create with
assertUsersInOrg and update with assertUsersAndQueuesInOrg before any
write, rejecting AccessibleQueueNotInOrgError for cross-org targets.
Defense-in-depth/integrity fix: reads (getReviewableQueuesForUser,
getQueueForOrg) and the dequeue path (checkQueueExists) are already
org-scoped by the invoker, so a cross-org user could not actually see
or act on the queue -- no confidentiality impact. Closes the same
un-scoped-input shape the advisory fixed elsewhere.
Regression tests cover cross-org userId for both create and update,
asserting the mutation rejects and that update leaves the victim user
absent from the queue's viewers.
Co-Authored-By: pi
* test(mrt): use a real org user in JobRouting queue fixtures
The org-scoping guard added to createManualReviewQueue rejects
userIds that are not members of the caller's org. JobRouting.test.ts
invented userId = uid() and never created a matching public.users row,
so every queue fixture in that suite threw AccessibleQueueNotInOrgError.
Create the user via the createUser fixture helper (real org member)
and wire its cleanup, matching QueueOperations.test.ts and
CommentOperations.test.ts.
Co-Authored-By: pi
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: serialize concurrent queries in transactional test harness
The transactional test harness pins a single Postgres connection per test
and rewrites the app's BEGIN/COMMIT/ROLLBACK to savepoints, tracking the
active savepoint with a single shared depth counter. That counter is only
-safe- if the app's transaction-control statements arrive strictly one
after another. The app, however, routinely runs concurrent transactions
(Promise.all of transactionWithRetry, background eventually-consistent
cache refreshers), which interleaves BEGIN/COMMIT across coroutines on
the one pinned connection. A COMMIT then releases the *wrong* savepoint
("savepoint coop_test_sp_N does not exist"), aborting the outer test
transaction and corrupting every later read in the test.
Fix: wrap every query to the pinned connection — transaction control AND
data, from connect().query and pool.query — in a promise-chained queue so
statements execute one at a time in arrival order. This only serializes
concurrent work *within* a single test (each test already has its own
pg.Client); it does not affect cross-test parallelism. openSavepoint /
closeSavepoint / rejectUnsupportedTransactionControl are unchanged.
Resolves the savepoint-corruption failures seen in CI run 27951870325
(wrong item-type version in moderationConfigService.test.ts, the
NoResultError / driver-has-already-been-destroyed cascade in the MRT
suites).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix: serialize whole logical transactions in test harness to prevent savepoint corruption
The transactional test harness serialized queries at the individual SQL
statement level, which fixed wire-protocol interleaving but not savepoint
stack corruption: overlapping Kysely transactions could interleave
BEGIN(A), BEGIN(B), …, COMMIT(A), and A's COMMIT would release B's
savepoint (it read the shared savepointDepth pointing at B's savepoint).
A concurrent rollback could then discard another transaction's committed
work.
Replace per-statement serialization with a transaction-scoped mutex:
BEGIN acquires it (waiting on any in-flight transaction), COMMIT/ROLLBACK
releases it when savepointDepth returns to 0. Overlapping transactions
now run strictly one-after-another, so savepoints stay properly nested.
Handle nested transactions: a BEGIN arriving while the mutex is held is
recognized as nested (concurrent BEGINs are still blocked in the FIFO
queue) and opens a savepoint directly without re-acquiring. COMMIT/ROLLBACK
only releases the mutex at the outermost depth.
Remove the finally block around closeSavepoint so a failed COMMIT/ROLLBACK
leaves the mutex held, allowing Kysely's catch-block ROLLBACK to still see
an in-flight transaction instead of masking the original error.
Add two integration tests:
- concurrent rollback isolation: 5 concurrent transactions, one throws,
only its row is missing
- nested transactions: transactionWithRetry inside transactionWithRetry
does not deadlock
* test: verify FIFO commit order instead of circular id ordering
The 'serializes concurrent commits' test inserted id (the dispatch index)
and asserted select id order by id == [0..15]. Since id was both the value
and the ORDER BY key, the assertion held under any write order — proving
nothing. Add a seq serial column and order by it so the check reflects
actual commit/write order.
* small cleanup
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
NCMEC's validator rejects fileDetails submissions when ipCaptureEvent
appears after industryClassification with cvc-complex-type.2.4.a;
after industryClassification only {deviceId, details, additionalInfo}
are accepted. Move ipCaptureEvent before industryClassification in
both the FileDetails TS type and buildFileDetailsObject's return
literal, and update the XSD-insertion-order test assertion to match.
Also update the originalFileHash docblock that referenced the old
position, and update the CHANGELOG entry for #855 to be honest that
priorCTReports does not populate in production until the follow-up
to the duplicate-submission check lands.
Latent since the /fileinfo submission path was first written;
activated by #641 (IP address field role wiring), which made the
wrong-positioned element actually appear in submissions for any
adopter who configured the IP role on a Content item type.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* CHANGELOG: document unreleased changes since 1.0.1
* CHANGELOG: Add recent NCMEC additions
* CHANGELOG: Add new contributors
* ncmec: auto-populate priorCTReports, originalFileName, fileRelevance
Closes three code-gap rows from the field-coverage audit on #843, all
auto-populated from data coop already has so no adopter configuration
is needed:
- `personOrUserReported.priorCTReports[]`: query
`ncmec_reporting.ncmec_reports` for accepted (`is_test=false`) report
IDs for the same reported user, ordered most-recent-first. Skipped for
exttest submissions since prod and test report IDs don't
cross-reference.
- `fileDetails.originalFileName`: derive from the media URL's pathname
(decoded last segment). Webhook-supplied `additionalInfo.fileName`
wins when present; URL-derived fills the gap for adopters without an
additional-info webhook.
- `fileDetails.fileRelevance`: default to `'Reported'` since
`submitReport`'s current call path only iterates over reviewer-flagged
media. Input slot accepts `'Supplemental Reported'` for future use.
Touch points:
- `NcmecReportingService.getPriorCTReportIds`: new query method.
- `BuildSubmitReportObjectInput.priorCTReports`: new input slot;
`buildSubmitReportObject` renders inside `personOrUserReported` after
`ipCaptureEvent` per XSD insertion order.
- `BuildFileDetailsObjectInput.{originalFileName,fileRelevance}`: new
input slots; `buildFileDetailsObject` renders them in their XSD
positions (3rd and 8th).
- `deriveOriginalFileNameFromUrl`: exported helper; `#upload` uses it
to populate `originalFileName` from `media.url` before calling
`buildFileDetailsObject`.
No migration; no GraphQL/SDL change; no client change. Purely additive
server behavior: existing reports gain three new elements, no existing
elements change.
Tests:
- `deriveOriginalFileNameFromUrl` (new describe block): decoding,
query/fragment stripping, unparseable URLs, malformed percent-encoding
fallback.
- `buildFileDetailsObject` (extended): updated minimum-envelope and
XSD-order expectations for the new elements; new cases for
`fileRelevance` default+override and `originalFileName` emit/omit.
- `buildSubmitReportObject` (new describe block): `priorCTReports`
populates in the right XSD position; omits when empty or unset.
All 69 NCMEC unit tests pass locally.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: log fallback paths in deriveOriginalFileNameFromUrl
Surfaces each of the four fallback cases (unparseable URL, empty
pathname, empty decoded segment, decode failure) via `ncmecDebugLog` so
operators can triage unexpected media URL shapes under `NCMEC_DEBUG=1`.
Addresses review nit on #855.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: extract pure helpers from submitReport for testability
Pulls the inline Report-object construction inside `submitReport`, and
the FileDetails construction inside `#upload`, into module-level exported
helpers. Behavior is unchanged: `submitReport` still does the DB lookups
and HTTP submission; the new `buildSubmitReportObject` and
`buildFileDetailsObject` are pure functions that produce the same objects
that previously sat inline.
Adds 60 unit tests in two new files: `buildSubmitReportObject.test.ts`
(matches the per-function pattern of
`buildSubmitReportParamsFromDecision.test.ts`) and
`ncmecReporting.builders.test.ts` for the smaller helpers. The
field-assembly logic was previously only exercised through integration
tests; the unit tests anchor XSD insertion order (preventing #477-class
regressions without a round-trip to exttest), document the
webhook-vs-field-role precedence, and cover the input-data validations on
`escalateToHighPriority` and `additionalInfo`.
The private `#fileAnnotationArrayToNCMECFileAnnotation` method moved to a
module-level export of the same name (without the `#`) so the new builder
can call it without instantiating `NcmecReportingService`.
No behavior change; no migration; no dependency change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: apply prettier to refactor + builders test
CI's check_formatting flagged ncmecReporting.ts and
ncmecReporting.builders.test.ts after the rebase onto main. Pure
auto-format pass; no logical changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: narrow personOrUserReported instead of non-null assertion in test
Addresses CodeRabbit nit on #853: the test exercises an optional field,
so `toBeDefined()` + nullish-coalescing keeps the contract under test
without `!`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: tighten `email` field role from STRING to EMAIL_ADDRESS scalar
Follow-up to #840 and #841. Now that `@roostorg/coop-types@2.4.0` ships
a dedicated `EMAIL_ADDRESS` scalar, switch the email field role plumbing
to require it instead of any STRING-typed field. Mirrors the two-PR
pattern PR #559 → PR #583 used for `IP_ADDRESS`.
- Bump `@roostorg/coop-types` to ^2.4.0 in server and client
- Add `EMAIL_ADDRESS` to `ScalarType`, `SignalInputType`, `FieldType`
SDL enums; regenerate codegen
- Add `[ScalarTypes.EMAIL_ADDRESS]` handler in `fieldTypeHandlers.ts`
(string coerce + trim; null on empty; format validation deferred per
Caleb's note on #840)
- `FieldRoleToScalarType.email`: STRING → EMAIL_ADDRESS
- Client `schemaFieldRolesFieldTypes.EMAIL`: String → EmailAddress
- Migration: drop the `valid_email_field_field_type` CHECK constraint
(which required STRING) and replace with one requiring EMAIL_ADDRESS
- Exhaustive-switch updates for `ManualReviewJobFieldsComponent` and
`RuleTestModal`
- `EmailAddressArbitrary` for fast-check test fixtures, with culturally
diverse example names on RFC 2606 reserved domains
Migration safety: any existing email_field mapping to a STRING field
will fail the new constraint. #840 merged hours before this; no adopter
should have configured the role yet. Operators between #840 and this
migration who did configure it will need to either change their field
type to EMAIL_ADDRESS or null out the mapping before applying.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* client: add EMAIL_ADDRESS cases to three exhaustive switches
CI on #842 failed `npm run lint` (= `tsc --noEmit`) because widening
`ScalarType` to include `EMAIL_ADDRESS` left three client switches
incomplete. The `JsonValue` errors in `itemTypeCodeSampleUtils.ts`
were downstream of `generateFakeScalarFieldValue` not handling the
new scalar.
- `signal.ts`: comparators for `EmailAddress` mirror `String` / `Url` / `IpAddress`
- `RuleInsightsSamplesTable.tsx`: stringify (mirrors `IpAddress`)
- `itemTypeUtils.ts:generateFakeScalarFieldValue`: returns a fake email
on RFC 2606 `example.com`
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: data-migrate stale email_field mappings before tightening CHECK
Addresses Tao's review on #842: the original "no adopter has had time to
configure the role" assumption was wrong. Deployments between #840 and
this migration may have mapped `email_field` to a STRING field; the new
CHECK constraint would reject those rows and block the migration entirely.
Add a DO-block data migration that runs in the same transaction before
the CHECK swap. For each item_type whose `email_field` doesn't point at
an EMAIL_ADDRESS-typed field in `fields`, NULL out the mapping and emit
a NOTICE naming the row (id, name, org_id) so operators can reconfigure
via the admin UI if needed. The underlying STRING field's data is left
untouched; only the role pointer is cleared.
Header comment updated to reflect the new data-migration step.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* db: forward postgres NOTICE events to console.warn during migrations
Addresses Tao's review on #842: the migration's `RAISE NOTICE` lines
were being swallowed because Sequelize's pg dialect doesn't surface
server-side messages by default. Operators running `npm run db:update`
saw the migration succeed but had no signal about which adopter
configurations had been cleared.
Wire a Sequelize `afterConnect` hook in `pg-base.ts` that attaches a
`notice` listener to every pg client the pool opens, forwarding each
message to `console.warn` prefixed with the severity. Future migrations
that use `RAISE NOTICE` (or `RAISE WARNING`) will also benefit, not just
this one.
Update the migration's header comment to direct operators to scan the
`db:update` output for `[postgres NOTICE]` lines after applying, so the
cleared mappings don't go unnoticed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: wire HMA hashes into outgoing report's fileDetails.originalFileHash (#844)
NCMEC's CyberTipline accepts an `originalFileHash[]` element with a
`hashType` attribute per entry. HMA already computes per-image hashes
at item-submission time (`HMAHashBankService.hashContentFromUrl`,
stored on the image object alongside `url`), but the NCMEC report
builder discarded them.
- Add `OriginalFileHash` type and `originalFileHash?: OriginalFileHash[]`
to `FileDetails` (positioned after `industryClassification`, before
`ipCaptureEvent`, in line with NCMEC XSD ordering for forensic fields).
- Add `hashes?: Record<string, string>` to the internal `Media` type so
the report assembly can pass them through to `#upload`.
- `buildSubmitReportParamsFromDecision` now extracts hashes from the
matching image in the item data (walks scalar, array, and map
containers). New `extractHashesForUrl` helper.
- `#upload` converts the hashes via `toOriginalFileHashes` and includes
the resulting array on `fileDetails` when non-empty.
- New `toOriginalFileHashes` exported helper handles trimming,
empty-value filtering, and the algorithm-name uppercasing for the
`hashType` attribute.
Tests:
- 4 new in `buildSubmitReportParamsFromDecision.test.ts` covering the
scalar IMAGE case, ARRAY-of-IMAGE container traversal, URL-mismatch
omission, and missing-hashes omission.
- 4 new in `ncmecReporting.test.ts` for `toOriginalFileHashes`'s
precedence rules (uppercasing, trimming, empty-value drops,
undefined-on-empty).
This is one of the P1 items from the audit in #843.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* db: also lower client_min_messages so RAISE NOTICE actually surfaces
Initial fix only attached a notice listener, but verified locally that
NOTICE messages from RAISE NOTICE inside PL/pgSQL DO blocks were still
being filtered out. Root cause: Sequelize raises `client_min_messages`
to WARNING by default, which makes postgres filter NOTICE-severity
messages server-side before they reach the client. The listener was
correctly attached but never fired for NOTICEs.
In the same afterConnect hook, also issue `SET client_min_messages =
'notice'` so postgres sends NOTICE messages to the client. Verified
locally: per-row NOTICEs from the email-field-clearing migration now
appear in `db:update` output as expected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: wire webhook fileDetails.hash into outgoing report's originalFileHash (#850)
Closes the follow-up gap noted in PR #844. The additional-info webhook
can return a single `{ hash, hashType }` per media item; #844 wired
HMA-sourced hashes through but webhook-sourced hashes were still
dropped because:
1. `MediaAdditionalInfo` (the internal type that `#upload` consumes)
didn't declare `fileDetails`; the validation-schema-typed
`NcmecAdditionalInfoResponse` has it, but the data was erased at
the boundary between webhook response and internal use.
2. `toOriginalFileHashes` took only the HMA map as input.
Changes:
- Add `fileDetails?: { hash; hashType }` to `MediaAdditionalInfo`.
- Refactor `toOriginalFileHashes` to take both sources via an opts
object (`hmaHashes`, `webhookFileDetails`). Hashes from both are
combined and deduped on (`hashType` uppercase, trimmed hash value)
so a webhook returning the same algorithm as HMA produces one entry
in the outgoing report, not two.
- Update `#upload` call site to pass both sources.
- Remove dead `fileDetails: { ipCaptureEvent: [] }` from the
no-webhook fallback; it never matched any consumed shape and
nothing read it (the `ipCaptureEvent` lives at the top level of
`MediaAdditionalInfo`, not nested).
Tests: 6 new cases covering webhook-only, both sources, exact-match
dedup, same-algorithm-different-hash kept-both, whitespace-only
webhook hash dropped, and case-insensitive dedup on algorithm name.
The toOriginalFileHashes test block is moved to its own file
(`toOriginalFileHashes.test.ts`) to keep `ncmecReporting.test.ts`
under the 500-line max-lines limit.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #662. The full UUID with `whitespace-nowrap` overflowed the ID
column and pushed adjacent columns out of view. Truncate the displayed
value to the first 8 characters with an ellipsis; full UUID stays on
the clipboard via `CopyTextComponent`'s `displayValue` prop.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a dedicated EMAIL_ADDRESS entry to the ScalarTypes enum and maps
it to `string` in ScalarTypeRuntimeTypeMapping. Mirrors the IP_ADDRESS
addition from #559. Lets Coop tag a schema field as an email address so
integrations (NCMEC today; potentially others later) can pull it via
schema-field role metadata instead of accepting any STRING-typed field.
Format validation lives at the field-value level, not in this type.
Required before #840 can switch the `email` field role from STRING to
EMAIL_ADDRESS.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: add `email` schema field role for reported user
Closes #839. Adopters whose NCMEC additional-info webhook isn't
configured (or returns no email) get reports rejected as "incomplete"
because no other path populates the email. This adds an `email` field
role on user item types, parallel to the existing `ipAddress` role,
sourced from a user-schema string field of the adopter's choosing.
The submit codepath now prefers `userAdditionalInfo.email` from the
NCMEC additional-info webhook when present, falling back to the
field-role email otherwise (via new `resolveReportedPersonEmail`
helper).
Migration adds `email_field` to `public.item_types` and its temporal
mirror, with a STRING-type CHECK constraint mirroring the existing
`display_name_field` pattern. The `item_type_versions` materialized
view is dropped and recreated to include the new column, with all four
indexes and the dependent `item_type_latest_versions` view rebuilt.
Admin UI adds `Email` to the role picker for user item types.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: address PR #840 review feedback
- Migration: include `created_at` in the recreated `item_type_versions`
materialized view to match the column declared in
`dbTypes.ts`. The previous `add_ip_address_field_role_to_item_types`
migration omitted it; this PR recovers parity.
- `resolveReportedPersonEmail`: trim the field-role email and treat
whitespace-only as absent, so we don't ship `{ _text: " " }` to
NCMEC (which would fail the same way the original bug did).
- Tests: two new cases on `resolveReportedPersonEmail` cover the
trim and whitespace-only paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: revert created_at addition to MV; fix dbTypes drift instead
The previous commit added `created_at` to the `item_type_versions`
materialized view in response to a CodeRabbit comment about
`dbTypes.ts` declaring the column. CI revealed the column doesn't
exist on `public.item_types_history`, so the UNION ALL SELECT failed:
ERROR: column item_types_history.created_at does not exist
The actual drift was in `dbTypes.ts` claiming a column the MV has
never had. Remove the type declaration to match reality. The MV
itself stays unchanged from the previous (working) `ip_address`
migration's shape.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: update USER item type inline snapshot for email field role
CI surfaced one inline snapshot in moderationConfigService.test.ts that
didn't include the new `email: undefined` key on `schemaFieldRoles`.
Snapshot updated to match the new shape. CONTENT and THREAD snapshots
are unaffected since email is USER-only.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
connect-pg-simple keeps a recurring pruneSessions timer that calls
pool.query on its pool. makeApiServer created the store with the shared
KyselyPgPool but never closed it, so the timer kept running after
shutdown. In tests this leaked a setInterval per makeMockedServer() call
that fired pool.query on the harness's already-closed pinned connection
after each test, logging "Failed to prune sessions: Client was closed
and is not queryable" indefinitely and hanging the test worker — the
loop that forced the manual cancellation of CI run 27951870325.
Hold a reference to the store instance and call its close() in the
shutdown path. ownsPg is false (we pass in our own pool), so close() only
stops the prune timer and won't end the shared pool.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* graphql: add userStrikeCount field to UserItem type and resolver
Exposes the user's cumulative strike score on the UserItem GraphQL
type, calling UserStrikeService.getUserStrikeValue().
* client: show user strike count in Review Console and Investigation
Selects userStrikeCount from the UserItem type in the MRT job fragment,
the Investigation query, and the related user component's query.
Displays it as a "Strikes" field in the primary and related user panels,
mirroring where User Score appeared before it was removed in #726.
Fixes #752
* client: fix ItemTypeFieldFieldData type error in userStrikeCountField
According to Claude, spreading createFieldType widened the `type` property to the full
ScalarType union, which ItemTypeFieldFieldData (a discriminated mapped
type) can't resolve. Inline the object with a literal `type: 'NUMBER'`
instead.
* ManualReviewJobRelatedUserComponent: use @/ import path
Addresses CodeRabbit review feedback on #766.
Two textarea placeholders included the literal backtick-wrapped internal
field name `actorNote`, which is not meaningful to non-developer moderators.
Rewrites both to plain-language descriptions of the field's purpose:
- ItemAction.tsx: "...This note will be recorded in the audit log and
included in any configured webhook payload."
- BulkActioningDashboard.tsx: "Add a short note explaining this decision
— it will be recorded in the audit log and included in any configured
webhook payload."
Fixes #684
Co-authored-by: james <li@jamesdeMacBook-Pro.local>
Co-authored-by: Cassidy James <cassidyjames@roost.tools>
* test(e2e): bootstrap Playwright suite with login flow
Stand up a dedicated /e2e Playwright package (own package.json/lockfile,
matching the per-package repo layout) and the first moderator-critical flow
from #485: login + session.
Also adds a draft CI workflow that runs the suite on PRs and pushes to main,
gated by dorny/paths-filter so it only fires on app-affecting changes, and
caching the Playwright browser binaries. The full-stack bring-up mirrors the
documented local setup and still needs maintainer review + end-to-end CI
validation.
Refs #485
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci(e2e): fix server/client startup hang
Redirect backgrounded server/client output to log files so they don't hold the
step's stdout pipe open, and wait on the server's /ready health endpoint (a
reliable 200) instead of the GraphQL endpoint, which returns 400 on a bare GET.
Dump the logs if readiness times out.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci(e2e): wait on server TCP port, not load-gated /ready
The /ready endpoint returns 500 when CPU usage exceeds 75% (api.ts), which is
always the case while tsc-watch and vite are compiling on a CI runner, so
wait-on never saw a 200. Wait on the server's listening TCP port instead (the
server only logs readiness after all middleware, including GraphQL, is wired).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(e2e): select login email field by input type, not role
getByRole('textbox') matched both the email and password inputs in this Ant
version (strict-mode violation in CI). The email field is the only type=text
input, so select by type to stay unambiguous.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(e2e): move Playwright into server/ and self-seed via DI factories
Relocate the Playwright suite from a standalone top-level package into
server/e2e/ so tests can import the server's DI container and the existing
test/fixtureHelpers factories. Each test now seeds the state it needs
(committed) and tears it down, instead of depending on a pre-seeded org.
- fixtures/coop.ts extends Playwright's test with a worker-scoped `deps`
fixture (getBottle(), dynamically imported so the heavy graph defers to run
time) and a test-scoped `seed` fixture exposing factory wrappers
(seed.orgWithAdmin seeds an org + password-login admin).
- login.spec.ts seeds its own admin and logs in; no env credentials.
- jest testPathIgnorePatterns excludes /e2e/ so its *.spec.ts files never run
under the unit-test runner; eslint test-rules + devDep allowlist extended to
e2e/.
- CI: drop the create-org seed step; @playwright/test is a server devDep so
only the browser binary is installed; cache keyed on server/package-lock.json;
report uploaded from server/e2e/playwright-report.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(e2e): load server runtime from transpiled/ to dodge esbuild type-import bug
Playwright's esbuild loader transpiles each file in isolation and can't elide
type-only imports written with value syntax (e.g. `import { JSON }`), so
importing the server's TS source graph threw "does not provide an export named
'JSON'" at run time. Load the compiled output (transpiled/, emitted by tsc with
those imports correctly elided) instead, via computed specifiers so tsc doesn't
resolve transpiled/ statically; types still come from the .ts source through
`typeof import(<source>)` casts. transpiled/ is present whenever the server runs
(tsc-watch / the Docker build emit it).
Relax `consistent-type-imports` (disallowTypeAnnotations: false) for test/e2e
files so the `typeof import()` casts pass lint.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(e2e): drop per-test cleanup; isolate by tenant and run fully parallel
Each test seeds a unique-id org, so the app's multi-tenancy isolates tests from
one another and no cleanup is needed (the CI database is disposable). This lets
the suite run with `fullyParallel: true` — Playwright has no random-order flag,
and concurrent execution with no fixed order is the idiomatic way to prevent
implicit ordering dependencies. Removes the Seeder's cleanup tracking entirely.
Documents the two disciplines that keep this valid (seed your own data; never
assert on cross-tenant/global state) and a "Scaling to per-worker databases"
note for when the suite grows — that future change is infra-only because the
deps fixture is already worker-scoped and tests already self-seed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* clean up
* ci(e2e): cover root deps in path filter; drop runtime wait-on fetch
Add .nvmrc, root package manifests/lock, and docker-compose.yaml to the
paths-filter so the E2E job runs when files it depends on change.
Replace the dynamic `npx --yes wait-on@8` readiness check (an undeclared
runtime dependency that bypasses the lockfile) with a built-in bash wait
loop polling tcp:8080 and http:3000.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Cassidy James <cassidyjames@roost.tools>
Integration tests (server/test/**/*.integ.test.ts) never ran in CI. The
`test` compose service runs `test:ci`, whose jest config ignores
`*.integ.test.ts`; they only ran via `test:integ`, which nothing in CI
invoked. PRs touching server/ — including Dependabot updates — therefore
merged without integration coverage.
Add a dedicated `check_api_server_integration` job that runs the
integration tests in parallel with the existing unit/lint/build job; both
are gated on server changes and report failures independently.
Also add --runInBand to test:integ: the suites share databases and are
not parallel-safe (report-flow raced the other suites and failed when run
concurrently). This mirrors test:ci, which already runs serially.
Fixes #221
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Moves checklist items from the ethanresnick/pr-checklists action into the PR
template directly, so they work for PRs from forks. The action required
pull-requests:write which GitHub blocks on fork PRs, meaning fork contributors
never saw the checklist.
Closes #671
* [791] Extend parameterized actions to proactive rules and user strikes
* code review fixes
* Move configuredParameters onto Action type, deprecate rule-level actionParameters
- Add `configuredParameters: JSONObject` to ActionBase and all implementing
types so consumers access configured values directly from each action.
- Deprecate the top-level `actionParameters` field on Rule types.
- Update client query to read `configuredParameters` from action objects.
- Remove stale comments from validation tests.
* code review fixes
* ci: make image pulls resilient to Docker Hub flakiness
Intermittent CI failures came from transient Docker Hub registry/auth
timeouts (auth.docker.io / registry-1.docker.io) during image pulls at
`docker compose run` time. These are timeouts, not rate limits, so
authenticating wouldn't help (and can't on fork PRs anyway).
For each compose job in apply_pr_checks.yaml:
- Configure a docker.io pull-through mirror (mirror.gcr.io) via
/etc/docker/daemon.json, bypassing the flaky Docker Hub auth/token
path. No secrets, so it still works on fork PRs.
- Pre-pull/build the images each job needs in a 3-attempt retry loop
before the real steps. Images are then cached locally, so the existing
`docker compose run` steps don't hit the registry -- isolating a flaky
pull to the retried prep step without masking real lint/build/test
failures.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci: extract image-prep into a composite action
The mirror-config + retry-pull logic was duplicated across all three
compose jobs. Extract it into a local composite action
(.github/actions/prepare-docker-images) parameterized by the services to
pull/build, so each job calls it in one step.
Inputs are passed via env rather than interpolated into the run script,
matching the repo's zizmor template-injection guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci: run dockerised checks when CI plumbing changes
The check_api_server / check_generated_graphql / frontend jobs are gated
by paths-filter on server/** and client/**, so a PR that only touches CI
or docker plumbing (this workflow, the compose file, the Dockerfile, the
prepare-docker-images action) skipped all of them -- meaning changes to
the image-pull setup were never actually exercised in CI.
Add those plumbing paths to the server and client filters via a YAML
anchor so the relevant jobs run when they change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore: apply prettier formatting across the codebase
Apply `prettier --write` to all `{ts,tsx,js,jsx,mjs,cjs,json,md,yaml,yml}`
files in a single pass so the codebase matches the formatting enforced by
the pre-commit hook. This is a mechanical, whitespace-only change.
Closes #799 (formatting pass portion).
* ci: enforce prettier formatting in CI
Add a check_formatting job to apply_pr_checks.yaml that runs
`prettier --check` over the same file set the pre-commit hook formats,
via actions/setup-node (no Docker, so docker-compose.yaml is untouched).
Broaden the root `npm run format` script to the same
`{ts,tsx,js,jsx,mjs,cjs,json,md,yaml,yml}` glob so `npm run format`,
lint-staged, and CI all agree on scope.
Document the new job and its local reproduction command in AGENTS.md.
Closes #799 (CI enforcement portion).
* build: add prettier and prettier:fix scripts; use them in CI
Centralize the whole-repo Prettier glob into two root package.json scripts
so CI and local dev share one source of truth:
- `npm run prettier` -> prettier --check (used by the check_formatting CI job)
- `npm run prettier:fix` -> prettier --write
- `npm run format` -> alias of prettier:fix (kept for backwards compat)
CI now runs `npm run prettier` instead of duplicating the glob inline.
The pre-commit hook (lint-staged) intentionally keeps the scoped
`prettier --write` so it only touches staged files; using the whole-repo
glob there would reformat the entire codebase on every commit.
Update AGENTS.md, docs/development/local.md, and copilot-instructions.md
to reference the canonical script names.
* [Fix] Ensure sweep also handles related items
* add tests and fix copilot review
* Add per-queue "clear other reports for a user" action sweep
* code review fixes
* code review fixes
* wording fix
* improve resiliency to help with big batches
* Make clickhouse configs for spillover on queries (#819)
* make clickhouse configs for spillover on queries
* fix bad redirect from strikes
* clickhouse optimizations + strikes fix + nullable fix
* code review fixes
* code review comments
* add tests
* fix
* test: isolate DB-backed server tests via transaction rollback
Introduce a transaction-rollback test harness so DB-backed tests get
automatic per-test isolation with no hand-written cleanup, replacing the
fixture-teardown (and hardcoded-id) patterns that leaked state across runs.
Mechanism (server/test/harness/transactionalPgPool.ts):
- A pg.Pool facade that pins one connection and rewrites the app's
BEGIN/COMMIT/ROLLBACK to SAVEPOINTs, so an outer test transaction rolls
everything back -- including the app's own makeKyselyTransactionWithRetry
transactions. Also exposes pool.query for the express-session store.
Adoption:
- makeMockedServer is now transactional: real Postgres (mocked ClickHouse)
wired through the rolled-back connection, exposing rollback()/shutdown().
getBottleContainerWithIOMocks gains an optional kyselyPool override for it.
- makeTransactionalTestWithFixture drives rollback from the test lifecycle,
so fixtures return only their data -- no cleanup.
- Convert the makeMockedServer-tier tests and detectRulePassRateAnomalies
to the harness, dropping all per-test cleanup.
- Extract getPgConnectionParams so the harness reads the same DATABASE_* env
vars as the app.
Postgres-only: Scylla/ClickHouse/Redis writes are not rolled back; tests
touching those keep uid-based isolation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test: tighten harness query types and guard savepoint underflow
Replace blanket no-explicit-any suppressions on the transactional pool's
query path with proper pg types (string | QueryConfig, unknown[]), and
guard COMMIT/ROLLBACK rewrites against savepoint-depth underflow by
asserting depth > 0 and only decrementing after the SQL succeeds.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: guarantee pinned-connection cleanup with try/finally
Wrap each transactionalPgPool integ test body in try/finally so a
mid-test throw can't leak the pinned Postgres connection; closing it in
finally also aborts any still-open outer transaction.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: reject unsupported transaction-control statements in pg harness
Strip trailing semicolons before matching transaction-control statements
(so `COMMIT;` is rewritten too), and reject transaction-control commands
the savepoint rewrite doesn't handle (END, ABORT, SAVEPOINT, RELEASE,
COMMIT/ROLLBACK PREPARED, PREPARE TRANSACTION) with an explanatory error
instead of silently forwarding them and defeating per-test isolation.
Extract the savepoint open/close logic into helpers to cut duplication,
and add a regression test covering the new rejection behaviour.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Add knip and a `knip` npm script. knip.json sets src/index.ts as the
entry point (the CLI, compiled to build/index.js) and ignores ts-node and
dotenv, which are used via the src/index.ts shebang
(`node --loader ts-node/esm --require dotenv/config`) and so can't be
detected as imports.
Remove dependencies with no references anywhere in src/:
csv-parse, latlon-geohash, lodash, uid, uuid. (uuid only appeared as a
Postgres column type in SQL, never imported as an npm package.)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add knip and a `knip` npm script to find unused files, exports, and
dependencies in the client.
knip.json relies on knip's Vite / Vitest / Storybook / ESLint / Tailwind /
PostCSS plugin auto-detection, ignores the generated GraphQL types
(src/graphql/generated.ts), and ignores @types/google.maps (used as an
ambient global namespace, which knip can't trace via imports).
Remove dependencies with no references anywhere in the source tree:
- framer-motion, jsonpointer, papaparse, react-markdown,
react-router-hash-link, validator
- @eslint/js (not imported by the flat config; bundled by eslint itself)
- orphaned/obsolete types: @types/papaparse, @types/react-router-hash-link,
@types/validator (their runtime packages were removed) and @types/recharts
(recharts ships its own type definitions)
Kept web-vitals — it's loaded via a dynamic import in reportWebVitals.js.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add knip and a `knip` npm script. knip auto-discovers the entry points
(index.ts and cli/cli.ts) from the package's `exports`, so knip.json only
needs the project glob.
No unused dependencies were found, so none are removed. (knip does flag a
dead exported `Bind1` type; left as-is to keep this change scoped to
tooling setup.)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs: fix AGENTS.md first-run setup instructions
Three gaps found when standing up a fresh dev environment:
- Missing .env copy step for /client (only /server and /db were mentioned)
- db:create must precede db:update for Scylla and ClickHouse; skipping it
produces a "keyspace/database does not exist" error at migration time.
Postgres is unaffected (it auto-creates), but the other two are not.
- Wrong access-point ports: client is 3000, server is 8080 (not 3001/3000)
- Fixed --website example to include https:// scheme (required by create-org)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* Add postgres DB create for consistency
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
The clickhouse service had an explicit container_name: clickhouse,
overriding Docker Compose's default project-prefixed naming. Every
other service relies on the default (e.g. coop-queue-delete-postgres-1),
which is unique per worktree directory. With the fixed name, any two
worktrees running simultaneously — or even sequentially without a
docker compose down — collide on the container name.
Removing the override lets Compose derive the name from the project,
consistent with all other services.
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* refactor: migrate CoreSignal to GQLSignal, remove redundant client type casts
* fix: address Copilot and CodeRabbit review comments
* fix: address Copilot review pass
* code review fixes
* fix code review. add missing test
---------
Co-authored-by: Juan Mrad <juansmrad@gmail.com>
* fix: don't hide empty threads
* code review fixes
* code review fixes
---------
Co-authored-by: Juan Mrad <juansmrad@gmail.com>
* Fix email validation in invite user form
The invite form accepted comma-separated email lists and didn't surface
why the submit button was disabled. This fixes three things:
- Thread the `type` prop through CoopInput to the underlying <input>
- Validate email format client-side (rejecting commas, which were the
original bug vector) and mark the field red when invalid
- Show a contextual tooltip on the disabled submit button explaining
what's missing (no email, invalid email, or no role selected)
Fixes #411
* Destructure value for consistency, add aria-invalid for a11y
* [Fix] Potential injection and safety consistency
* fix code review comments
* code review feedback
GitHub is deprecating Node 20 on Actions runners: Node 24 becomes the
default on 2026-06-16 and Node 20 is removed in fall 2026. Every action
pinned in our workflows was still on the Node 20 runtime.
Bump all actions to their latest (Node 24-based) releases via pinact:
- actions/checkout v4.3.1 -> v6.0.3
- dorny/paths-filter v3.0.2 -> v4.0.1
- dorny/test-reporter v1.9.1 -> v3.0.0
- docker/setup-qemu-action v3.6.0 -> v4.1.0
- docker/setup-buildx-action v3.10.0 -> v4.1.0
- docker/login-action v3.4.0 -> v4.2.0
- docker/metadata-action v5.7.0 -> v6.1.0
- docker/build-push-action v6.19.2 -> v7.2.0
These majors only add the Node 24 runtime bump (requiring Actions Runner
>= v2.327.1, satisfied by ubuntu-latest); none of the removed/changed
inputs are used here. zizmor reports no findings.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* [Feature] Add decision reason to recent decision view as well as csv download
* code review fixes
* wrap around policies
When VITE_CONTENT_PROXY_URL is unset or empty, the manual review iframe
now loads the content URL directly instead of falling back to the app's
own origin. Serving content from the dashboard's own origin, combined
with the iframe's `allow-same-origin allow-scripts` sandbox, would let
that content reach out of the sandbox and into the dashboard.
The Translate control is hidden and the proxy postMessage channel is
skipped when no proxy is configured, since those are implemented by the
content proxy.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Point agents at .github/pull_request_template.md when opening a GitHub PR
and ask them to keep PR descriptions succinct.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dorny/paths-filter resolves changed paths via the GitHub API for
pull_request events, but diffs against git history for push events.
Without a checkout the push-to-main run failed with "fatal: not a git
repository". Add an actions/checkout step like the other jobs.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* [Feature] Split require-decision-reason into action vs ignore settings (#757)
Ignoring a job means "no violation / no action", but the single
"Require decision reason" setting forced a written reason on every
decision — including ignores — so the submit button stayed disabled
(#757). Rather than change the existing setting's behaviour (some orgs
rely on requiring a reason for ignores), split it into two independent
org settings:
- Require decision reason (for violating jobs) — the existing setting,
now scoped to non-ignore decisions.
- Require decision reason (when ignoring jobs) — new setting.
A migration renames the existing column to `..._on_action`, adds
`..._on_ignore`, and backfills the ignore flag from the old value so
upgrading preserves current behaviour for every org. Enforcement (both
the client submit gate and the server-side backstop) now picks the flag
based on whether the decision is composed solely of IGNORE; the existing
NCMEC-native bypass is preserved. The decision-reason textarea is now
always shown in the review console.
Also makes db/src/configs/scylla.ts read its env vars lazily so a
Postgres-only local migration apply doesn't require Scylla env to be set
(unrelated dev-ergonomics fix; happy to split out if preferred).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Nest decision-reason settings under a master toggle
Replace the two flat "Require Decision Reason (for violating jobs)" and
"(when ignoring jobs)" switches with a single master "Require decision
reason" toggle that reveals two indented sub-toggles when on:
"When applying an action" and "When ignoring jobs". The master is derived
from the two existing flags (on when either is on); turning it on defaults
to requiring a reason for actions only, and turning it off clears both. No
backend change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* improve copy
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Fix SAML cross-org authentication bypass
The SAML verify callbacks looked up the user by email alone
(kyselyUserFindByEmail), ignoring the org named in the callback path
(/saml/login/:orgId). Because the same email can exist across tenants,
an assertion signed by one org's IdP could resolve a user belonging to
a different org and create a session as that cross-tenant user.
Bind the lookup to the path org via a new org-scoped
kyselyUserFindByEmailAndOrg, used by a shared resolveSamlUser helper for
both the signon and logout verify callbacks. A user from another org is
no longer found, so login fails.
Addresses GHSA-2v93-383c-9fw2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Reject missing/invalid SAML email claim before lookup
resolveSamlUser coerced the email claim with String(profile?.email),
turning a missing claim into the literal "undefined" (and an array claim
into a comma-joined string) and using it as a lookup key. Validate that
the claim is a non-empty string and reject the authentication attempt
before querying otherwise.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Refactor resolveSamlUser to receive KyselyPg directly
Match the codebase DI convention: consumers receive the Bottle-provided
KyselyPg and call the free persistence functions directly (as UserApi,
RoleApi, etc. do), rather than injecting a bespoke findUser closure.
resolveSamlUser now takes db: UsersDb and calls kyselyUserFindByEmailAndOrg
itself; api.ts passes KyselyPg.
Its tests move to the real-DB testWithFixture pattern used for the
persistence layer, exercising the org-scoped lookup against Postgres.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Extract getOrgIdFromPath helper and log SAML login DB failures
- Add a shared getOrgIdFromPath(req) helper and use it in both
getSamlOptions and resolveSamlUser to DRY the orgId path-param check.
- Thread the Tracer into resolveSamlUser and narrow the catch to the DB
lookup so a genuine outage during login is logged via
logActiveSpanFailedIfAny (observable) and surfaced as an internal
error, instead of swallowing the done() dispatch in a broad catch.
- Share one verify callback for the SAML signon and logout slots.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci: typecheck db and migrator in PR CI
db and migrator had no checks in PR CI. Add gated typecheck jobs for
both (setup-node + npm ci + tsc --noEmit), matching the existing
publish-workflow pattern, and extend the paths-filter accordingly.
Add `typecheck` scripts to db and migrator. Fix two pre-existing type
errors in the migrator apply-scripts command: the `name` positional was
never declared, so yargs typed it `unknown`. Declare it as a string
(mirroring the `add` command) and assert non-null where check() already
guarantees presence.
client and server are already linted and typechecked in CI via their
existing lint/build jobs. Formatting in CI is deferred: the repo is not
currently Prettier-clean against its pinned version.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci: also run PR checks on push to main; trim comment
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* [Security] Invalidate sessions on password change (GHSA-g5xq-67g7-36r2)
Resetting a password did not clear the user's PG-stored sessions
(connect-pg-simple, 30-day maxAge), and passport.deserializeUser looks
users up by id only. A phished/attacker session therefore survived a
password reset for up to 30 days, even after the legitimate user took
the expected recovery action.
Add deleteSessionsForUser(), which removes the user's rows from
public.session (matched on passport's `sess -> 'passport' ->> 'user'`),
and call it from both password-change paths:
- resetPasswordForToken: deletes all of the user's sessions.
- UserApi.changePassword: deletes all *other* sessions, preserving the
caller's own (via the request's sessionID) so they aren't logged out
mid-action.
No DB migration; deserializeUser is unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Drop advisory ID from code comments
Keep the explanatory comments; remove the GHSA reference so it isn't
exposed in source while the advisory is unpublished.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Drop file-path reference from sessionPersistence comment
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Make password-change + session invalidation atomic
Wrap the password update and session purge in a single transaction
(via makeKyselyTransactionWithRetry) in both resetPasswordForToken and
UserApi.changePassword. Previously these were separate commits, so a
failure after the password update left the user's sessions alive — the
exact persistence the fix is meant to prevent.
Also narrow the test's eslint-disable for the unused mock constructor
args from a block disable to per-line comments.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The testOrgs allowlist (['4def6a77d6a','acc701627cb']) hard-coded two
org IDs that were silently suppressed with PERMANENT_ERROR before any
network call. Hard-coded org IDs in source don't belong here; the
NCMEC_ENV-based sandbox/production routing already handles test vs.
prod submissions.
Co-Authored-By: pi
* fix(mrt): guard client date rendering against unparseable createdAt
Unguarded date-fns format/formatDistanceToNow calls threw
RangeError: Invalid time value on an unparseable createdAt, unmounting
the task detail and queue preview into the "Something Went Wrong" error
boundary. Add safeFormat/safeFormatDistanceToNow helpers that fall back
to "Unknown" for invalid dates, and use them on the MRT render path.
Client-side counterpart to the server-side decision-log fix in #913.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AKvvCLkcC7LM1znjzvDjDA
* add guard on more functions to prevent errors on invalid timezone formats.
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Juan Mrad <juansmrad@gmail.com>
getOldestJobCreatedAtForExistingQueue fetched the first waiting/delayed
job with queue.getJobs([state], 0, 0), but BullMQ's getJobs defaults to
descending order, so index 0 is the most recently added job. The MRT
queues dashboard's Oldest Task Age column therefore showed the newest
task's age. Switch to BullMQ's getWaiting/getDelayed getters, which
return jobs oldest-first.
Adds a regression test that enqueues an older job then a newer one and
asserts the older createdAt is returned; it fails against the previous
implementation. The dummy-job payload builder it needs was previously
copied per test file, so it is extracted into a shared
test/fixtureHelpers/makeDummyMrtJobPayload.ts fixture (using
instantiateOpaqueType instead of eslint-disabled type assertions).
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Juan Mrad <juansmrad@gmail.com>
* Make Scylla-backed features (item investigation, user strikes) optional
* Address review: extract shared flag helper, use @ts-expect-error
Resolve CodeRabbit nitpicks on PR #918:
- Extract the ITEM_INVESTIGATION_AND_STRIKES_ENABLED parsing into a single
exported helper (itemInvestigationAndStrikesEnabled) in noOpScylla.ts; the
iocContainer factory and the unit test now share it instead of keeping two
copies in sync (DRY).
- Keep the helper a pure function of its argument (env is read at the call
site) so the default-enabled test is independent of the ambient environment.
- Replace the 'as unknown as' cast in NoOpScylla.insert() with a scoped
@ts-expect-error and justifying comment, per repo guidelines.
Co-Authored-By: Rovo Dev <rovodev@atlassian.com>
* Fixed formatting
* ci: re-trigger E2E (investigate investigation.spec flake vs regression)
* Gate user strike logic behind ITEM_INVESTIGATION_AND_STRIKES_ENABLED flag
When the flag is false, applyUserStrikeFromPublishedActions early-returns
to avoid running strike threshold checks against always-zero counts from
the NoOpScylla, which would cause escalation actions to fire unexpectedly.
---------
Co-authored-by: Rovo Dev <rovodev@atlassian.com>
Co-authored-by: Juan Mrad <juansmrad@gmail.com>
* fix(mrt): store null for unparseable item createdAt in decision log
Submitting a decision failed with a 500 ("Job submission failed. Please
try again.") for any job whose item carries a truthy but unparseable
createdAt value. `#logDecision` passed `new Date(itemCreatedAtField)`
straight into the `item_created_at` timestamptz column; an unparseable
value yields an Invalid Date, which the pg driver serializes to a NaN
string that Postgres rejects (22007), failing the whole decision insert.
The decision is never recorded and the job is never removed, so the task
is stuck in the queue.
Normal item submissions can't reach this state because the DATETIME field
handler validates dates at intake. A bad value only arrives via a path
that skips that validation (e.g. system-generated reports, or a createdAt
role mapped to a non-DATETIME field).
Normalize at the write boundary: parse the value and store null when it
is not a valid date. The column is already nullable, so the decision
records and the task clears. Adds a unit regression test for the
normalization helper.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEU6YHFiBYfTqjgM5Vyt9n
* docs(changelog): note the unparseable createdAt decision fix
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEU6YHFiBYfTqjgM5Vyt9n
* fix(mrt): preserve epoch 0 in parseItemCreatedAt
Address review: `!value` treated a numeric 0 (a valid 1970-01-01 epoch) as
empty. Guard only null/undefined/empty-string instead, and cover epoch 0 in
the test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEU6YHFiBYfTqjgM5Vyt9n
* fix(mrt): record unparseable item createdAt values
Address review: surface invalid createdAt values instead of silently
nulling them. When a present createdAt can't be parsed, emit a tracer
span (job id, org id, the raw value) so the bad data is diagnosable and
can be backfilled. The decision still saves with a null item_created_at.
new Date() returns an Invalid Date rather than throwing, so this detects
the invalid parse rather than wrapping in try/catch.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CEU6YHFiBYfTqjgM5Vyt9n
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* fix: close express-session store on api shutdown
connect-pg-simple keeps a recurring pruneSessions timer that calls
pool.query on its pool. makeApiServer created the store with the shared
KyselyPgPool but never closed it, so the timer kept running after
shutdown. In tests this leaked a setInterval per makeMockedServer() call
that fired pool.query on the harness's already-closed pinned connection
after each test, logging "Failed to prune sessions: Client was closed
and is not queryable" indefinitely and hanging the test worker — the
loop that forced the manual cancellation of CI run 27951870325.
Hold a reference to the store instance and call its close() in the
shutdown path. ownsPg is false (we pass in our own pool), so close() only
stops the prune timer and won't end the shared pool.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test: migrate DB-backed tests to transactional harness
Move the remaining DB-backed server tests onto the transaction-rollback
harness from #732 so they get per-test isolation with no hand-written
cleanup.
- Convert the makeMockedServer/getBottle-based tests to
makeTransactionalTestWithFixture, dropping manual org/user/queue/action
deletes and KyselyPg.destroy() teardown: userKyselyPersistenceFindByEmailAndOrg,
resolveSamlUser, and the MRT module tests (CommentOperations, JobRouting,
QueueOperations, ReporterInvalidation).
- Rewrite moderationConfigService and manualReviewToolService so every test
is self-contained: each creates its own fresh org (rolled back
automatically) rather than sharing a suite-scoped org. This removes the
cross-test ordering/accumulator dependency in moderationConfigService and
the hardcoded staging-seed-data dependency in manualReviewToolService.
Scylla-touching tests (itemInvestigationService) keep uid-based isolation,
since the Postgres-only harness can't roll those back.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: migrate UserReportSweep and userStrikeService to transactional harness
Two DB-backed test suites were missed by the initial migration commit:
- manualReviewToolService/modules/UserReportSweep.test.ts: a sibling of
the four MRT module tests already migrated (CommentOperations, JobRouting,
QueueOperations, ReporterInvalidation). It used makeTestWithFixture +
getBottle with hand-written cleanup (createOrg/createUser/createMrtQueue
teardown + KyselyPg/KyselyPgReadReplica.destroy()). Converted to
makeTransactionalTestWithFixture; cleanup is now automatic rollback.
- userStrikeService/userStrikeService.test.ts: used beforeAll/afterAll with
a shared getBottle container and uid-based isolation. Converted to
makeTransactionalTestWithFixture for true per-test rollback isolation.
Also fixes a copy-paste describe block name ('Item Investigation Service'
-> 'User Strike Service').
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: resolve typecheck errors in migrated test files
Two files migrated by the prior commit had TypeScript errors that broke
`tsc` (and therefore `check_api_server` and the e2e server start):
- manualReviewToolService.test.ts: the 'records an AUTOMATIC_CLOSE decision
with no human reviewer' test was left as a bare `it(...)` referencing an
out-of-scope `mrtService` and hardcoded staging `orgId`/'queueId'
('e7c89ce7729'/'1') — the very seed-data dependency the migration was
meant to remove. Convert it to `testWithQueue()` like its siblings, using
the fixture's `mrtService`/`org.id`/`queue.id`.
- moderationConfigService.test.ts: the 'should return actions for a rule
scoped to the caller org' snapshot read `it.id` from
`getActionsForRuleId`, which returns `{ action, parameters }[]` —
should be `it.action.id` (as on main). Also add the now-present
`email` field role to the #createUserType inline snapshot.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: create real user in migrated JobRouting fixture
The transactional JobRouting fixture accidentally replaced the pre-migration
createUser() call with a bare uid(). createManualReviewQueue validates queue
users, so setup failed after creating org/item-type rows but before the fixture
returned. Because makeTestWithFixture cannot run cleanup when setup throws,
the outer transaction stayed idle-in-transaction and later tests blocked on the
item_type_versions materialized-view refresh, timing out check_api_server.
Keep the transactional harness, but create a real user and pass user.id to the
queue setup.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: restore QueueOperations org-scoping tests
The rebase conflict resolution for QueueOperations.test.ts accidentally kept
the pre-#872 branch side and dropped the two-org org-scoping regression tests
that now exist on main. Restore testWithTwoOrgs and its cross-org access tests,
using the transactional harness.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: configure decision-reason settings through service API
Replace the decision-reason test helpers' direct writes to
manual_review_tool_settings with a behavior-shaped helper that uses the
ManualReviewToolService update API. The tests still exercise persisted org
settings through submitDecision, but no longer couple setup to table and column
names.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* style: remove api shutdown explanatory comments
Remove the comments added around the express-session store shutdown while keeping the shutdown behavior unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* ci: stop logging every DB query in test runs
Set DATABASE_PRINT_LOGS=false in .env.githubci (used by the docker-compose
`test` service for unit/integration tests) and comment it out in
server/.env.example (copied to server/.env by the e2e workflow) so the
default is off. Kysely still logs query errors regardless, so failures
remain visible; this only suppresses the per-query SQL/params/duration
spam that made CI logs unreadable.
Co-Authored-By: pi <pi@earendil-works>
* ci: drop redundant comment in server/.env.example
Co-Authored-By: pi <pi@earendil-works>
---------
Co-authored-by: pi <pi@earendil-works>
Co-authored-by: Cassidy James <cassidyjames@roost.tools>
* test(ncmec): add field coverage and submission tests
Add XML field-coverage regression tests for NCMEC report builders,
including XSD-order locks for the ipCaptureEvent ordering regression
class. Add a submitReport integration test using a stubbed fetchHTTP seam
against real Postgres, plus a fixture helper for NCMEC org settings.
Canonicalise webhook-sourced ipCaptureEvent objects before XML rendering
so webhook key order cannot produce out-of-order CyberTip XML.
Co-Authored-By: pi
* fix(ncmec): correct fileDetails ipCaptureEvent XSD ordering; harden tests
buildFileDetailsObject emits <ipCaptureEvent> in the wrong position relative
to <industryClassification>/<originalFileHash>. PR #869 moved it BEFORE
industryClassification, claiming NCMEC's live validator required that — but
a probe against exttest.cybertip.org proves the opposite:
- Variant A (#869 order, ipCaptureEvent BEFORE industryClassification):
REJECTED — cvc-complex-type.2.4.a: Invalid content was found starting
with element 'industryClassification'. One of '{deviceId, details,
additionalInfo}' is expected.
- Variant B (canonical XSD order, ipCaptureEvent AFTER
industryClassification/originalFileHash): ACCEPTED, responseCode=0.
#869 misread the original #856 error (which fires when an element lands in
the trailing deviceId/details/additionalInfo slot) and moved ipCaptureEvent
backwards to a position NCMEC actually rejects. #869's own PR description
admits the live re-test was never run before merge. This commit restores the
canonical XSD order, which the live validator accepts.
Reorders buildFileDetailsObject and the FileDetails type so ipCaptureEvent
follows industryClassification and originalFileHash, and updates the
ncmecReporting.builders.test.ts 'preserves XSD insertion order (Appendix C)'
assertion to anchor the correct order (the test had been updated in #869 to
anchor the wrong one).
Also includes test-hygiene improvements to the NCMEC #843 test work:
- fieldCoverage.test.ts: strengthen the renderXml assertion from a bare
length>0 check to per-scenario structural XML markers (present/absent),
so the js2xml serialization path is actually exercised. Remove the dead
XSD.fileDetails array (buildSubmitReportObject's <report> envelope never
contains <fileDetails>; its ordering is locked in
ncmecReporting.builders.test.ts).
- ncmec-submission.integ.test.ts: extract a shared RecordedCall type,
replace submitCall! non-null assertions with an early-fail type guard,
and type the stub generic against the real FetchHTTP/CoopRequestQuery
contract (removing the as-never param, inline cast, and final broad
as-unknown-as-FetchHTTP). Requires exporting HandleResponseBody from
networkingService (additive type-only export).
Co-Authored-By: pi
* fix(mrt): org-scope accessible-queue mutations (GHSA-mf74-gf5j-hxr9)
addAccessibleQueuesForUser / removeAccessibleQueuesForUser accepted an
arbitrary queueId / userId with no org-scoping, so an authenticated user
in Org A could grant (or revoke) access to queues and users belonging to
Org B. Thread the caller's orgId through the resolver, service wrapper,
and QueueOperations write layer, and reject (403 AccessibleQueueNotInOrgError)
if any target queue or user does not belong to the caller's org.
Regression tests cover both IDOR dimensions (cross-org queueId and
cross-org userId) for both add and remove, plus a same-org happy path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* refactor(mrt): use transactional test harness, trim comments
- Switch the accessible-queue IDOR tests to makeTransactionalTestWithFixture
(per-org rollback, no manual cleanup).
- Drop GHSA references and "what was" narration from code comments; keep
only minimal present-tense notes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(mrt): assert original user keeps access after cross-org revoke attempt
Strengthen the GHSA-mf74 regression test for removeAccessibleQueuesForUser:
after a rejected cross-org revoke, also assert the attacker still sees their
own queue — confirming the rejected call left the original access intact.
Co-Authored-By: pi
* fix(mrt): org-scope create/update queue userIds (GHSA-mf74-gf5j-hxr9)
createManualReviewQueue and updateManualReviewQueue accepted un-scoped
userIds and wrote them straight into users_and_accessible_queues, the
same un-scoped-input shape fixed in add/removeAccessibleQueuesForUser.
create always creates the queue in the caller's org (orgId from the
invoker) but granted access to users in other orgs; update's queue-row
write was org-scoped but the join-table writes ran un-scoped inside the
transaction.
Refactor assertUsersAndQueuesInOrg into composable assertQueuesInOrg +
assertUsersInOrg (add/remove callers unchanged), then guard create with
assertUsersInOrg and update with assertUsersAndQueuesInOrg before any
write, rejecting AccessibleQueueNotInOrgError for cross-org targets.
Defense-in-depth/integrity fix: reads (getReviewableQueuesForUser,
getQueueForOrg) and the dequeue path (checkQueueExists) are already
org-scoped by the invoker, so a cross-org user could not actually see
or act on the queue -- no confidentiality impact. Closes the same
un-scoped-input shape the advisory fixed elsewhere.
Regression tests cover cross-org userId for both create and update,
asserting the mutation rejects and that update leaves the victim user
absent from the queue's viewers.
Co-Authored-By: pi
* test(mrt): use a real org user in JobRouting queue fixtures
The org-scoping guard added to createManualReviewQueue rejects
userIds that are not members of the caller's org. JobRouting.test.ts
invented userId = uid() and never created a matching public.users row,
so every queue fixture in that suite threw AccessibleQueueNotInOrgError.
Create the user via the createUser fixture helper (real org member)
and wire its cleanup, matching QueueOperations.test.ts and
CommentOperations.test.ts.
Co-Authored-By: pi
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* fix: serialize concurrent queries in transactional test harness
The transactional test harness pins a single Postgres connection per test
and rewrites the app's BEGIN/COMMIT/ROLLBACK to savepoints, tracking the
active savepoint with a single shared depth counter. That counter is only
-safe- if the app's transaction-control statements arrive strictly one
after another. The app, however, routinely runs concurrent transactions
(Promise.all of transactionWithRetry, background eventually-consistent
cache refreshers), which interleaves BEGIN/COMMIT across coroutines on
the one pinned connection. A COMMIT then releases the *wrong* savepoint
("savepoint coop_test_sp_N does not exist"), aborting the outer test
transaction and corrupting every later read in the test.
Fix: wrap every query to the pinned connection — transaction control AND
data, from connect().query and pool.query — in a promise-chained queue so
statements execute one at a time in arrival order. This only serializes
concurrent work *within* a single test (each test already has its own
pg.Client); it does not affect cross-test parallelism. openSavepoint /
closeSavepoint / rejectUnsupportedTransactionControl are unchanged.
Resolves the savepoint-corruption failures seen in CI run 27951870325
(wrong item-type version in moderationConfigService.test.ts, the
NoResultError / driver-has-already-been-destroyed cascade in the MRT
suites).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix: serialize whole logical transactions in test harness to prevent savepoint corruption
The transactional test harness serialized queries at the individual SQL
statement level, which fixed wire-protocol interleaving but not savepoint
stack corruption: overlapping Kysely transactions could interleave
BEGIN(A), BEGIN(B), …, COMMIT(A), and A's COMMIT would release B's
savepoint (it read the shared savepointDepth pointing at B's savepoint).
A concurrent rollback could then discard another transaction's committed
work.
Replace per-statement serialization with a transaction-scoped mutex:
BEGIN acquires it (waiting on any in-flight transaction), COMMIT/ROLLBACK
releases it when savepointDepth returns to 0. Overlapping transactions
now run strictly one-after-another, so savepoints stay properly nested.
Handle nested transactions: a BEGIN arriving while the mutex is held is
recognized as nested (concurrent BEGINs are still blocked in the FIFO
queue) and opens a savepoint directly without re-acquiring. COMMIT/ROLLBACK
only releases the mutex at the outermost depth.
Remove the finally block around closeSavepoint so a failed COMMIT/ROLLBACK
leaves the mutex held, allowing Kysely's catch-block ROLLBACK to still see
an in-flight transaction instead of masking the original error.
Add two integration tests:
- concurrent rollback isolation: 5 concurrent transactions, one throws,
only its row is missing
- nested transactions: transactionWithRetry inside transactionWithRetry
does not deadlock
* test: verify FIFO commit order instead of circular id ordering
The 'serializes concurrent commits' test inserted id (the dispatch index)
and asserted select id order by id == [0..15]. Since id was both the value
and the ORDER BY key, the assertion held under any write order — proving
nothing. Add a seq serial column and order by it so the check reflects
actual commit/write order.
* small cleanup
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
NCMEC's validator rejects fileDetails submissions when ipCaptureEvent
appears after industryClassification with cvc-complex-type.2.4.a;
after industryClassification only {deviceId, details, additionalInfo}
are accepted. Move ipCaptureEvent before industryClassification in
both the FileDetails TS type and buildFileDetailsObject's return
literal, and update the XSD-insertion-order test assertion to match.
Also update the originalFileHash docblock that referenced the old
position, and update the CHANGELOG entry for #855 to be honest that
priorCTReports does not populate in production until the follow-up
to the duplicate-submission check lands.
Latent since the /fileinfo submission path was first written;
activated by #641 (IP address field role wiring), which made the
wrong-positioned element actually appear in submissions for any
adopter who configured the IP role on a Content item type.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: auto-populate priorCTReports, originalFileName, fileRelevance
Closes three code-gap rows from the field-coverage audit on #843, all
auto-populated from data coop already has so no adopter configuration
is needed:
- `personOrUserReported.priorCTReports[]`: query
`ncmec_reporting.ncmec_reports` for accepted (`is_test=false`) report
IDs for the same reported user, ordered most-recent-first. Skipped for
exttest submissions since prod and test report IDs don't
cross-reference.
- `fileDetails.originalFileName`: derive from the media URL's pathname
(decoded last segment). Webhook-supplied `additionalInfo.fileName`
wins when present; URL-derived fills the gap for adopters without an
additional-info webhook.
- `fileDetails.fileRelevance`: default to `'Reported'` since
`submitReport`'s current call path only iterates over reviewer-flagged
media. Input slot accepts `'Supplemental Reported'` for future use.
Touch points:
- `NcmecReportingService.getPriorCTReportIds`: new query method.
- `BuildSubmitReportObjectInput.priorCTReports`: new input slot;
`buildSubmitReportObject` renders inside `personOrUserReported` after
`ipCaptureEvent` per XSD insertion order.
- `BuildFileDetailsObjectInput.{originalFileName,fileRelevance}`: new
input slots; `buildFileDetailsObject` renders them in their XSD
positions (3rd and 8th).
- `deriveOriginalFileNameFromUrl`: exported helper; `#upload` uses it
to populate `originalFileName` from `media.url` before calling
`buildFileDetailsObject`.
No migration; no GraphQL/SDL change; no client change. Purely additive
server behavior: existing reports gain three new elements, no existing
elements change.
Tests:
- `deriveOriginalFileNameFromUrl` (new describe block): decoding,
query/fragment stripping, unparseable URLs, malformed percent-encoding
fallback.
- `buildFileDetailsObject` (extended): updated minimum-envelope and
XSD-order expectations for the new elements; new cases for
`fileRelevance` default+override and `originalFileName` emit/omit.
- `buildSubmitReportObject` (new describe block): `priorCTReports`
populates in the right XSD position; omits when empty or unset.
All 69 NCMEC unit tests pass locally.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: log fallback paths in deriveOriginalFileNameFromUrl
Surfaces each of the four fallback cases (unparseable URL, empty
pathname, empty decoded segment, decode failure) via `ncmecDebugLog` so
operators can triage unexpected media URL shapes under `NCMEC_DEBUG=1`.
Addresses review nit on #855.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: extract pure helpers from submitReport for testability
Pulls the inline Report-object construction inside `submitReport`, and
the FileDetails construction inside `#upload`, into module-level exported
helpers. Behavior is unchanged: `submitReport` still does the DB lookups
and HTTP submission; the new `buildSubmitReportObject` and
`buildFileDetailsObject` are pure functions that produce the same objects
that previously sat inline.
Adds 60 unit tests in two new files: `buildSubmitReportObject.test.ts`
(matches the per-function pattern of
`buildSubmitReportParamsFromDecision.test.ts`) and
`ncmecReporting.builders.test.ts` for the smaller helpers. The
field-assembly logic was previously only exercised through integration
tests; the unit tests anchor XSD insertion order (preventing #477-class
regressions without a round-trip to exttest), document the
webhook-vs-field-role precedence, and cover the input-data validations on
`escalateToHighPriority` and `additionalInfo`.
The private `#fileAnnotationArrayToNCMECFileAnnotation` method moved to a
module-level export of the same name (without the `#`) so the new builder
can call it without instantiating `NcmecReportingService`.
No behavior change; no migration; no dependency change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: apply prettier to refactor + builders test
CI's check_formatting flagged ncmecReporting.ts and
ncmecReporting.builders.test.ts after the rebase onto main. Pure
auto-format pass; no logical changes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: narrow personOrUserReported instead of non-null assertion in test
Addresses CodeRabbit nit on #853: the test exercises an optional field,
so `toBeDefined()` + nullish-coalescing keeps the contract under test
without `!`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: tighten `email` field role from STRING to EMAIL_ADDRESS scalar
Follow-up to #840 and #841. Now that `@roostorg/coop-types@2.4.0` ships
a dedicated `EMAIL_ADDRESS` scalar, switch the email field role plumbing
to require it instead of any STRING-typed field. Mirrors the two-PR
pattern PR #559 → PR #583 used for `IP_ADDRESS`.
- Bump `@roostorg/coop-types` to ^2.4.0 in server and client
- Add `EMAIL_ADDRESS` to `ScalarType`, `SignalInputType`, `FieldType`
SDL enums; regenerate codegen
- Add `[ScalarTypes.EMAIL_ADDRESS]` handler in `fieldTypeHandlers.ts`
(string coerce + trim; null on empty; format validation deferred per
Caleb's note on #840)
- `FieldRoleToScalarType.email`: STRING → EMAIL_ADDRESS
- Client `schemaFieldRolesFieldTypes.EMAIL`: String → EmailAddress
- Migration: drop the `valid_email_field_field_type` CHECK constraint
(which required STRING) and replace with one requiring EMAIL_ADDRESS
- Exhaustive-switch updates for `ManualReviewJobFieldsComponent` and
`RuleTestModal`
- `EmailAddressArbitrary` for fast-check test fixtures, with culturally
diverse example names on RFC 2606 reserved domains
Migration safety: any existing email_field mapping to a STRING field
will fail the new constraint. #840 merged hours before this; no adopter
should have configured the role yet. Operators between #840 and this
migration who did configure it will need to either change their field
type to EMAIL_ADDRESS or null out the mapping before applying.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* client: add EMAIL_ADDRESS cases to three exhaustive switches
CI on #842 failed `npm run lint` (= `tsc --noEmit`) because widening
`ScalarType` to include `EMAIL_ADDRESS` left three client switches
incomplete. The `JsonValue` errors in `itemTypeCodeSampleUtils.ts`
were downstream of `generateFakeScalarFieldValue` not handling the
new scalar.
- `signal.ts`: comparators for `EmailAddress` mirror `String` / `Url` / `IpAddress`
- `RuleInsightsSamplesTable.tsx`: stringify (mirrors `IpAddress`)
- `itemTypeUtils.ts:generateFakeScalarFieldValue`: returns a fake email
on RFC 2606 `example.com`
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: data-migrate stale email_field mappings before tightening CHECK
Addresses Tao's review on #842: the original "no adopter has had time to
configure the role" assumption was wrong. Deployments between #840 and
this migration may have mapped `email_field` to a STRING field; the new
CHECK constraint would reject those rows and block the migration entirely.
Add a DO-block data migration that runs in the same transaction before
the CHECK swap. For each item_type whose `email_field` doesn't point at
an EMAIL_ADDRESS-typed field in `fields`, NULL out the mapping and emit
a NOTICE naming the row (id, name, org_id) so operators can reconfigure
via the admin UI if needed. The underlying STRING field's data is left
untouched; only the role pointer is cleared.
Header comment updated to reflect the new data-migration step.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* db: forward postgres NOTICE events to console.warn during migrations
Addresses Tao's review on #842: the migration's `RAISE NOTICE` lines
were being swallowed because Sequelize's pg dialect doesn't surface
server-side messages by default. Operators running `npm run db:update`
saw the migration succeed but had no signal about which adopter
configurations had been cleared.
Wire a Sequelize `afterConnect` hook in `pg-base.ts` that attaches a
`notice` listener to every pg client the pool opens, forwarding each
message to `console.warn` prefixed with the severity. Future migrations
that use `RAISE NOTICE` (or `RAISE WARNING`) will also benefit, not just
this one.
Update the migration's header comment to direct operators to scan the
`db:update` output for `[postgres NOTICE]` lines after applying, so the
cleared mappings don't go unnoticed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: wire HMA hashes into outgoing report's fileDetails.originalFileHash (#844)
NCMEC's CyberTipline accepts an `originalFileHash[]` element with a
`hashType` attribute per entry. HMA already computes per-image hashes
at item-submission time (`HMAHashBankService.hashContentFromUrl`,
stored on the image object alongside `url`), but the NCMEC report
builder discarded them.
- Add `OriginalFileHash` type and `originalFileHash?: OriginalFileHash[]`
to `FileDetails` (positioned after `industryClassification`, before
`ipCaptureEvent`, in line with NCMEC XSD ordering for forensic fields).
- Add `hashes?: Record<string, string>` to the internal `Media` type so
the report assembly can pass them through to `#upload`.
- `buildSubmitReportParamsFromDecision` now extracts hashes from the
matching image in the item data (walks scalar, array, and map
containers). New `extractHashesForUrl` helper.
- `#upload` converts the hashes via `toOriginalFileHashes` and includes
the resulting array on `fileDetails` when non-empty.
- New `toOriginalFileHashes` exported helper handles trimming,
empty-value filtering, and the algorithm-name uppercasing for the
`hashType` attribute.
Tests:
- 4 new in `buildSubmitReportParamsFromDecision.test.ts` covering the
scalar IMAGE case, ARRAY-of-IMAGE container traversal, URL-mismatch
omission, and missing-hashes omission.
- 4 new in `ncmecReporting.test.ts` for `toOriginalFileHashes`'s
precedence rules (uppercasing, trimming, empty-value drops,
undefined-on-empty).
This is one of the P1 items from the audit in #843.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* db: also lower client_min_messages so RAISE NOTICE actually surfaces
Initial fix only attached a notice listener, but verified locally that
NOTICE messages from RAISE NOTICE inside PL/pgSQL DO blocks were still
being filtered out. Root cause: Sequelize raises `client_min_messages`
to WARNING by default, which makes postgres filter NOTICE-severity
messages server-side before they reach the client. The listener was
correctly attached but never fired for NOTICEs.
In the same afterConnect hook, also issue `SET client_min_messages =
'notice'` so postgres sends NOTICE messages to the client. Verified
locally: per-row NOTICEs from the email-field-clearing migration now
appear in `db:update` output as expected.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: wire webhook fileDetails.hash into outgoing report's originalFileHash (#850)
Closes the follow-up gap noted in PR #844. The additional-info webhook
can return a single `{ hash, hashType }` per media item; #844 wired
HMA-sourced hashes through but webhook-sourced hashes were still
dropped because:
1. `MediaAdditionalInfo` (the internal type that `#upload` consumes)
didn't declare `fileDetails`; the validation-schema-typed
`NcmecAdditionalInfoResponse` has it, but the data was erased at
the boundary between webhook response and internal use.
2. `toOriginalFileHashes` took only the HMA map as input.
Changes:
- Add `fileDetails?: { hash; hashType }` to `MediaAdditionalInfo`.
- Refactor `toOriginalFileHashes` to take both sources via an opts
object (`hmaHashes`, `webhookFileDetails`). Hashes from both are
combined and deduped on (`hashType` uppercase, trimmed hash value)
so a webhook returning the same algorithm as HMA produces one entry
in the outgoing report, not two.
- Update `#upload` call site to pass both sources.
- Remove dead `fileDetails: { ipCaptureEvent: [] }` from the
no-webhook fallback; it never matched any consumed shape and
nothing read it (the `ipCaptureEvent` lives at the top level of
`MediaAdditionalInfo`, not nested).
Tests: 6 new cases covering webhook-only, both sources, exact-match
dedup, same-algorithm-different-hash kept-both, whitespace-only
webhook hash dropped, and case-insensitive dedup on algorithm name.
The toOriginalFileHashes test block is moved to its own file
(`toOriginalFileHashes.test.ts`) to keep `ncmecReporting.test.ts`
under the 500-line max-lines limit.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Closes #662. The full UUID with `whitespace-nowrap` overflowed the ID
column and pushed adjacent columns out of view. Truncate the displayed
value to the first 8 characters with an ellipsis; full UUID stays on
the clipboard via `CopyTextComponent`'s `displayValue` prop.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a dedicated EMAIL_ADDRESS entry to the ScalarTypes enum and maps
it to `string` in ScalarTypeRuntimeTypeMapping. Mirrors the IP_ADDRESS
addition from #559. Lets Coop tag a schema field as an email address so
integrations (NCMEC today; potentially others later) can pull it via
schema-field role metadata instead of accepting any STRING-typed field.
Format validation lives at the field-value level, not in this type.
Required before #840 can switch the `email` field role from STRING to
EMAIL_ADDRESS.
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: add `email` schema field role for reported user
Closes #839. Adopters whose NCMEC additional-info webhook isn't
configured (or returns no email) get reports rejected as "incomplete"
because no other path populates the email. This adds an `email` field
role on user item types, parallel to the existing `ipAddress` role,
sourced from a user-schema string field of the adopter's choosing.
The submit codepath now prefers `userAdditionalInfo.email` from the
NCMEC additional-info webhook when present, falling back to the
field-role email otherwise (via new `resolveReportedPersonEmail`
helper).
Migration adds `email_field` to `public.item_types` and its temporal
mirror, with a STRING-type CHECK constraint mirroring the existing
`display_name_field` pattern. The `item_type_versions` materialized
view is dropped and recreated to include the new column, with all four
indexes and the dependent `item_type_latest_versions` view rebuilt.
Admin UI adds `Email` to the role picker for user item types.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: address PR #840 review feedback
- Migration: include `created_at` in the recreated `item_type_versions`
materialized view to match the column declared in
`dbTypes.ts`. The previous `add_ip_address_field_role_to_item_types`
migration omitted it; this PR recovers parity.
- `resolveReportedPersonEmail`: trim the field-role email and treat
whitespace-only as absent, so we don't ship `{ _text: " " }` to
NCMEC (which would fail the same way the original bug did).
- Tests: two new cases on `resolveReportedPersonEmail` cover the
trim and whitespace-only paths.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: revert created_at addition to MV; fix dbTypes drift instead
The previous commit added `created_at` to the `item_type_versions`
materialized view in response to a CodeRabbit comment about
`dbTypes.ts` declaring the column. CI revealed the column doesn't
exist on `public.item_types_history`, so the UNION ALL SELECT failed:
ERROR: column item_types_history.created_at does not exist
The actual drift was in `dbTypes.ts` claiming a column the MV has
never had. Remove the type declaration to match reality. The MV
itself stays unchanged from the previous (working) `ip_address`
migration's shape.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* ncmec: update USER item type inline snapshot for email field role
CI surfaced one inline snapshot in moderationConfigService.test.ts that
didn't include the new `email: undefined` key on `schemaFieldRoles`.
Snapshot updated to match the new shape. CONTENT and THREAD snapshots
are unaffected since email is USER-only.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
connect-pg-simple keeps a recurring pruneSessions timer that calls
pool.query on its pool. makeApiServer created the store with the shared
KyselyPgPool but never closed it, so the timer kept running after
shutdown. In tests this leaked a setInterval per makeMockedServer() call
that fired pool.query on the harness's already-closed pinned connection
after each test, logging "Failed to prune sessions: Client was closed
and is not queryable" indefinitely and hanging the test worker — the
loop that forced the manual cancellation of CI run 27951870325.
Hold a reference to the store instance and call its close() in the
shutdown path. ownsPg is false (we pass in our own pool), so close() only
stops the prune timer and won't end the shared pool.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* graphql: add userStrikeCount field to UserItem type and resolver
Exposes the user's cumulative strike score on the UserItem GraphQL
type, calling UserStrikeService.getUserStrikeValue().
* client: show user strike count in Review Console and Investigation
Selects userStrikeCount from the UserItem type in the MRT job fragment,
the Investigation query, and the related user component's query.
Displays it as a "Strikes" field in the primary and related user panels,
mirroring where User Score appeared before it was removed in #726.
Fixes #752
* client: fix ItemTypeFieldFieldData type error in userStrikeCountField
According to Claude, spreading createFieldType widened the `type` property to the full
ScalarType union, which ItemTypeFieldFieldData (a discriminated mapped
type) can't resolve. Inline the object with a literal `type: 'NUMBER'`
instead.
* ManualReviewJobRelatedUserComponent: use @/ import path
Addresses CodeRabbit review feedback on #766.
Two textarea placeholders included the literal backtick-wrapped internal
field name `actorNote`, which is not meaningful to non-developer moderators.
Rewrites both to plain-language descriptions of the field's purpose:
- ItemAction.tsx: "...This note will be recorded in the audit log and
included in any configured webhook payload."
- BulkActioningDashboard.tsx: "Add a short note explaining this decision
— it will be recorded in the audit log and included in any configured
webhook payload."
Fixes #684
Co-authored-by: james <li@jamesdeMacBook-Pro.local>
Co-authored-by: Cassidy James <cassidyjames@roost.tools>
* test(e2e): bootstrap Playwright suite with login flow
Stand up a dedicated /e2e Playwright package (own package.json/lockfile,
matching the per-package repo layout) and the first moderator-critical flow
from #485: login + session.
Also adds a draft CI workflow that runs the suite on PRs and pushes to main,
gated by dorny/paths-filter so it only fires on app-affecting changes, and
caching the Playwright browser binaries. The full-stack bring-up mirrors the
documented local setup and still needs maintainer review + end-to-end CI
validation.
Refs #485
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci(e2e): fix server/client startup hang
Redirect backgrounded server/client output to log files so they don't hold the
step's stdout pipe open, and wait on the server's /ready health endpoint (a
reliable 200) instead of the GraphQL endpoint, which returns 400 on a bare GET.
Dump the logs if readiness times out.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci(e2e): wait on server TCP port, not load-gated /ready
The /ready endpoint returns 500 when CPU usage exceeds 75% (api.ts), which is
always the case while tsc-watch and vite are compiling on a CI runner, so
wait-on never saw a 200. Wait on the server's listening TCP port instead (the
server only logs readiness after all middleware, including GraphQL, is wired).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(e2e): select login email field by input type, not role
getByRole('textbox') matched both the email and password inputs in this Ant
version (strict-mode violation in CI). The email field is the only type=text
input, so select by type to stay unambiguous.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(e2e): move Playwright into server/ and self-seed via DI factories
Relocate the Playwright suite from a standalone top-level package into
server/e2e/ so tests can import the server's DI container and the existing
test/fixtureHelpers factories. Each test now seeds the state it needs
(committed) and tears it down, instead of depending on a pre-seeded org.
- fixtures/coop.ts extends Playwright's test with a worker-scoped `deps`
fixture (getBottle(), dynamically imported so the heavy graph defers to run
time) and a test-scoped `seed` fixture exposing factory wrappers
(seed.orgWithAdmin seeds an org + password-login admin).
- login.spec.ts seeds its own admin and logs in; no env credentials.
- jest testPathIgnorePatterns excludes /e2e/ so its *.spec.ts files never run
under the unit-test runner; eslint test-rules + devDep allowlist extended to
e2e/.
- CI: drop the create-org seed step; @playwright/test is a server devDep so
only the browser binary is installed; cache keyed on server/package-lock.json;
report uploaded from server/e2e/playwright-report.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(e2e): load server runtime from transpiled/ to dodge esbuild type-import bug
Playwright's esbuild loader transpiles each file in isolation and can't elide
type-only imports written with value syntax (e.g. `import { JSON }`), so
importing the server's TS source graph threw "does not provide an export named
'JSON'" at run time. Load the compiled output (transpiled/, emitted by tsc with
those imports correctly elided) instead, via computed specifiers so tsc doesn't
resolve transpiled/ statically; types still come from the .ts source through
`typeof import(<source>)` casts. transpiled/ is present whenever the server runs
(tsc-watch / the Docker build emit it).
Relax `consistent-type-imports` (disallowTypeAnnotations: false) for test/e2e
files so the `typeof import()` casts pass lint.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test(e2e): drop per-test cleanup; isolate by tenant and run fully parallel
Each test seeds a unique-id org, so the app's multi-tenancy isolates tests from
one another and no cleanup is needed (the CI database is disposable). This lets
the suite run with `fullyParallel: true` — Playwright has no random-order flag,
and concurrent execution with no fixed order is the idiomatic way to prevent
implicit ordering dependencies. Removes the Seeder's cleanup tracking entirely.
Documents the two disciplines that keep this valid (seed your own data; never
assert on cross-tenant/global state) and a "Scaling to per-worker databases"
note for when the suite grows — that future change is infra-only because the
deps fixture is already worker-scoped and tests already self-seed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* clean up
* ci(e2e): cover root deps in path filter; drop runtime wait-on fetch
Add .nvmrc, root package manifests/lock, and docker-compose.yaml to the
paths-filter so the E2E job runs when files it depends on change.
Replace the dynamic `npx --yes wait-on@8` readiness check (an undeclared
runtime dependency that bypasses the lockfile) with a built-in bash wait
loop polling tcp:8080 and http:3000.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Cassidy James <cassidyjames@roost.tools>
Integration tests (server/test/**/*.integ.test.ts) never ran in CI. The
`test` compose service runs `test:ci`, whose jest config ignores
`*.integ.test.ts`; they only ran via `test:integ`, which nothing in CI
invoked. PRs touching server/ — including Dependabot updates — therefore
merged without integration coverage.
Add a dedicated `check_api_server_integration` job that runs the
integration tests in parallel with the existing unit/lint/build job; both
are gated on server changes and report failures independently.
Also add --runInBand to test:integ: the suites share databases and are
not parallel-safe (report-flow raced the other suites and failed when run
concurrently). This mirrors test:ci, which already runs serially.
Fixes #221
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* [791] Extend parameterized actions to proactive rules and user strikes
* code review fixes
* Move configuredParameters onto Action type, deprecate rule-level actionParameters
- Add `configuredParameters: JSONObject` to ActionBase and all implementing
types so consumers access configured values directly from each action.
- Deprecate the top-level `actionParameters` field on Rule types.
- Update client query to read `configuredParameters` from action objects.
- Remove stale comments from validation tests.
* code review fixes
* ci: make image pulls resilient to Docker Hub flakiness
Intermittent CI failures came from transient Docker Hub registry/auth
timeouts (auth.docker.io / registry-1.docker.io) during image pulls at
`docker compose run` time. These are timeouts, not rate limits, so
authenticating wouldn't help (and can't on fork PRs anyway).
For each compose job in apply_pr_checks.yaml:
- Configure a docker.io pull-through mirror (mirror.gcr.io) via
/etc/docker/daemon.json, bypassing the flaky Docker Hub auth/token
path. No secrets, so it still works on fork PRs.
- Pre-pull/build the images each job needs in a 3-attempt retry loop
before the real steps. Images are then cached locally, so the existing
`docker compose run` steps don't hit the registry -- isolating a flaky
pull to the retried prep step without masking real lint/build/test
failures.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci: extract image-prep into a composite action
The mirror-config + retry-pull logic was duplicated across all three
compose jobs. Extract it into a local composite action
(.github/actions/prepare-docker-images) parameterized by the services to
pull/build, so each job calls it in one step.
Inputs are passed via env rather than interpolated into the run script,
matching the repo's zizmor template-injection guard.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci: run dockerised checks when CI plumbing changes
The check_api_server / check_generated_graphql / frontend jobs are gated
by paths-filter on server/** and client/**, so a PR that only touches CI
or docker plumbing (this workflow, the compose file, the Dockerfile, the
prepare-docker-images action) skipped all of them -- meaning changes to
the image-pull setup were never actually exercised in CI.
Add those plumbing paths to the server and client filters via a YAML
anchor so the relevant jobs run when they change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* chore: apply prettier formatting across the codebase
Apply `prettier --write` to all `{ts,tsx,js,jsx,mjs,cjs,json,md,yaml,yml}`
files in a single pass so the codebase matches the formatting enforced by
the pre-commit hook. This is a mechanical, whitespace-only change.
Closes #799 (formatting pass portion).
* ci: enforce prettier formatting in CI
Add a check_formatting job to apply_pr_checks.yaml that runs
`prettier --check` over the same file set the pre-commit hook formats,
via actions/setup-node (no Docker, so docker-compose.yaml is untouched).
Broaden the root `npm run format` script to the same
`{ts,tsx,js,jsx,mjs,cjs,json,md,yaml,yml}` glob so `npm run format`,
lint-staged, and CI all agree on scope.
Document the new job and its local reproduction command in AGENTS.md.
Closes #799 (CI enforcement portion).
* build: add prettier and prettier:fix scripts; use them in CI
Centralize the whole-repo Prettier glob into two root package.json scripts
so CI and local dev share one source of truth:
- `npm run prettier` -> prettier --check (used by the check_formatting CI job)
- `npm run prettier:fix` -> prettier --write
- `npm run format` -> alias of prettier:fix (kept for backwards compat)
CI now runs `npm run prettier` instead of duplicating the glob inline.
The pre-commit hook (lint-staged) intentionally keeps the scoped
`prettier --write` so it only touches staged files; using the whole-repo
glob there would reformat the entire codebase on every commit.
Update AGENTS.md, docs/development/local.md, and copilot-instructions.md
to reference the canonical script names.
* Add per-queue "clear other reports for a user" action sweep
* code review fixes
* code review fixes
* wording fix
* improve resiliency to help with big batches
* Make clickhouse configs for spillover on queries (#819)
* make clickhouse configs for spillover on queries
* fix bad redirect from strikes
* clickhouse optimizations + strikes fix + nullable fix
* code review fixes
* code review comments
* add tests
* fix
* test: isolate DB-backed server tests via transaction rollback
Introduce a transaction-rollback test harness so DB-backed tests get
automatic per-test isolation with no hand-written cleanup, replacing the
fixture-teardown (and hardcoded-id) patterns that leaked state across runs.
Mechanism (server/test/harness/transactionalPgPool.ts):
- A pg.Pool facade that pins one connection and rewrites the app's
BEGIN/COMMIT/ROLLBACK to SAVEPOINTs, so an outer test transaction rolls
everything back -- including the app's own makeKyselyTransactionWithRetry
transactions. Also exposes pool.query for the express-session store.
Adoption:
- makeMockedServer is now transactional: real Postgres (mocked ClickHouse)
wired through the rolled-back connection, exposing rollback()/shutdown().
getBottleContainerWithIOMocks gains an optional kyselyPool override for it.
- makeTransactionalTestWithFixture drives rollback from the test lifecycle,
so fixtures return only their data -- no cleanup.
- Convert the makeMockedServer-tier tests and detectRulePassRateAnomalies
to the harness, dropping all per-test cleanup.
- Extract getPgConnectionParams so the harness reads the same DATABASE_* env
vars as the app.
Postgres-only: Scylla/ClickHouse/Redis writes are not rolled back; tests
touching those keep uid-based isolation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test: tighten harness query types and guard savepoint underflow
Replace blanket no-explicit-any suppressions on the transactional pool's
query path with proper pg types (string | QueryConfig, unknown[]), and
guard COMMIT/ROLLBACK rewrites against savepoint-depth underflow by
asserting depth > 0 and only decrementing after the SQL succeeds.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: guarantee pinned-connection cleanup with try/finally
Wrap each transactionalPgPool integ test body in try/finally so a
mid-test throw can't leak the pinned Postgres connection; closing it in
finally also aborts any still-open outer transaction.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* test: reject unsupported transaction-control statements in pg harness
Strip trailing semicolons before matching transaction-control statements
(so `COMMIT;` is rewritten too), and reject transaction-control commands
the savepoint rewrite doesn't handle (END, ABORT, SAVEPOINT, RELEASE,
COMMIT/ROLLBACK PREPARED, PREPARE TRANSACTION) with an explanatory error
instead of silently forwarding them and defeating per-test isolation.
Extract the savepoint open/close logic into helpers to cut duplication,
and add a regression test covering the new rejection behaviour.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Add knip and a `knip` npm script. knip.json sets src/index.ts as the
entry point (the CLI, compiled to build/index.js) and ignores ts-node and
dotenv, which are used via the src/index.ts shebang
(`node --loader ts-node/esm --require dotenv/config`) and so can't be
detected as imports.
Remove dependencies with no references anywhere in src/:
csv-parse, latlon-geohash, lodash, uid, uuid. (uuid only appeared as a
Postgres column type in SQL, never imported as an npm package.)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add knip and a `knip` npm script to find unused files, exports, and
dependencies in the client.
knip.json relies on knip's Vite / Vitest / Storybook / ESLint / Tailwind /
PostCSS plugin auto-detection, ignores the generated GraphQL types
(src/graphql/generated.ts), and ignores @types/google.maps (used as an
ambient global namespace, which knip can't trace via imports).
Remove dependencies with no references anywhere in the source tree:
- framer-motion, jsonpointer, papaparse, react-markdown,
react-router-hash-link, validator
- @eslint/js (not imported by the flat config; bundled by eslint itself)
- orphaned/obsolete types: @types/papaparse, @types/react-router-hash-link,
@types/validator (their runtime packages were removed) and @types/recharts
(recharts ships its own type definitions)
Kept web-vitals — it's loaded via a dynamic import in reportWebVitals.js.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add knip and a `knip` npm script. knip auto-discovers the entry points
(index.ts and cli/cli.ts) from the package's `exports`, so knip.json only
needs the project glob.
No unused dependencies were found, so none are removed. (knip does flag a
dead exported `Bind1` type; left as-is to keep this change scoped to
tooling setup.)
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* docs: fix AGENTS.md first-run setup instructions
Three gaps found when standing up a fresh dev environment:
- Missing .env copy step for /client (only /server and /db were mentioned)
- db:create must precede db:update for Scylla and ClickHouse; skipping it
produces a "keyspace/database does not exist" error at migration time.
Postgres is unaffected (it auto-creates), but the other two are not.
- Wrong access-point ports: client is 3000, server is 8080 (not 3001/3000)
- Fixed --website example to include https:// scheme (required by create-org)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* Add postgres DB create for consistency
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
The clickhouse service had an explicit container_name: clickhouse,
overriding Docker Compose's default project-prefixed naming. Every
other service relies on the default (e.g. coop-queue-delete-postgres-1),
which is unique per worktree directory. With the fixed name, any two
worktrees running simultaneously — or even sequentially without a
docker compose down — collide on the container name.
Removing the override lets Compose derive the name from the project,
consistent with all other services.
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
* Fix email validation in invite user form
The invite form accepted comma-separated email lists and didn't surface
why the submit button was disabled. This fixes three things:
- Thread the `type` prop through CoopInput to the underlying <input>
- Validate email format client-side (rejecting commas, which were the
original bug vector) and mark the field red when invalid
- Show a contextual tooltip on the disabled submit button explaining
what's missing (no email, invalid email, or no role selected)
Fixes #411
* Destructure value for consistency, add aria-invalid for a11y
GitHub is deprecating Node 20 on Actions runners: Node 24 becomes the
default on 2026-06-16 and Node 20 is removed in fall 2026. Every action
pinned in our workflows was still on the Node 20 runtime.
Bump all actions to their latest (Node 24-based) releases via pinact:
- actions/checkout v4.3.1 -> v6.0.3
- dorny/paths-filter v3.0.2 -> v4.0.1
- dorny/test-reporter v1.9.1 -> v3.0.0
- docker/setup-qemu-action v3.6.0 -> v4.1.0
- docker/setup-buildx-action v3.10.0 -> v4.1.0
- docker/login-action v3.4.0 -> v4.2.0
- docker/metadata-action v5.7.0 -> v6.1.0
- docker/build-push-action v6.19.2 -> v7.2.0
These majors only add the Node 24 runtime bump (requiring Actions Runner
>= v2.327.1, satisfied by ubuntu-latest); none of the removed/changed
inputs are used here. zizmor reports no findings.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
When VITE_CONTENT_PROXY_URL is unset or empty, the manual review iframe
now loads the content URL directly instead of falling back to the app's
own origin. Serving content from the dashboard's own origin, combined
with the iframe's `allow-same-origin allow-scripts` sandbox, would let
that content reach out of the sandbox and into the dashboard.
The Translate control is hidden and the proxy postMessage channel is
skipped when no proxy is configured, since those are implemented by the
content proxy.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dorny/paths-filter resolves changed paths via the GitHub API for
pull_request events, but diffs against git history for push events.
Without a checkout the push-to-main run failed with "fatal: not a git
repository". Add an actions/checkout step like the other jobs.
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* [Feature] Split require-decision-reason into action vs ignore settings (#757)
Ignoring a job means "no violation / no action", but the single
"Require decision reason" setting forced a written reason on every
decision — including ignores — so the submit button stayed disabled
(#757). Rather than change the existing setting's behaviour (some orgs
rely on requiring a reason for ignores), split it into two independent
org settings:
- Require decision reason (for violating jobs) — the existing setting,
now scoped to non-ignore decisions.
- Require decision reason (when ignoring jobs) — new setting.
A migration renames the existing column to `..._on_action`, adds
`..._on_ignore`, and backfills the ignore flag from the old value so
upgrading preserves current behaviour for every org. Enforcement (both
the client submit gate and the server-side backstop) now picks the flag
based on whether the decision is composed solely of IGNORE; the existing
NCMEC-native bypass is preserved. The decision-reason textarea is now
always shown in the review console.
Also makes db/src/configs/scylla.ts read its env vars lazily so a
Postgres-only local migration apply doesn't require Scylla env to be set
(unrelated dev-ergonomics fix; happy to split out if preferred).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Nest decision-reason settings under a master toggle
Replace the two flat "Require Decision Reason (for violating jobs)" and
"(when ignoring jobs)" switches with a single master "Require decision
reason" toggle that reveals two indented sub-toggles when on:
"When applying an action" and "When ignoring jobs". The master is derived
from the two existing flags (on when either is on); turning it on defaults
to requiring a reason for actions only, and turning it off clears both. No
backend change.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* improve copy
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Fix SAML cross-org authentication bypass
The SAML verify callbacks looked up the user by email alone
(kyselyUserFindByEmail), ignoring the org named in the callback path
(/saml/login/:orgId). Because the same email can exist across tenants,
an assertion signed by one org's IdP could resolve a user belonging to
a different org and create a session as that cross-tenant user.
Bind the lookup to the path org via a new org-scoped
kyselyUserFindByEmailAndOrg, used by a shared resolveSamlUser helper for
both the signon and logout verify callbacks. A user from another org is
no longer found, so login fails.
Addresses GHSA-2v93-383c-9fw2.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Reject missing/invalid SAML email claim before lookup
resolveSamlUser coerced the email claim with String(profile?.email),
turning a missing claim into the literal "undefined" (and an array claim
into a comma-joined string) and using it as a lookup key. Validate that
the claim is a non-empty string and reject the authentication attempt
before querying otherwise.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Refactor resolveSamlUser to receive KyselyPg directly
Match the codebase DI convention: consumers receive the Bottle-provided
KyselyPg and call the free persistence functions directly (as UserApi,
RoleApi, etc. do), rather than injecting a bespoke findUser closure.
resolveSamlUser now takes db: UsersDb and calls kyselyUserFindByEmailAndOrg
itself; api.ts passes KyselyPg.
Its tests move to the real-DB testWithFixture pattern used for the
persistence layer, exercising the org-scoped lookup against Postgres.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Extract getOrgIdFromPath helper and log SAML login DB failures
- Add a shared getOrgIdFromPath(req) helper and use it in both
getSamlOptions and resolveSamlUser to DRY the orgId path-param check.
- Thread the Tracer into resolveSamlUser and narrow the catch to the DB
lookup so a genuine outage during login is logged via
logActiveSpanFailedIfAny (observable) and surfaced as an internal
error, instead of swallowing the done() dispatch in a broad catch.
- Share one verify callback for the SAML signon and logout slots.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci: typecheck db and migrator in PR CI
db and migrator had no checks in PR CI. Add gated typecheck jobs for
both (setup-node + npm ci + tsc --noEmit), matching the existing
publish-workflow pattern, and extend the paths-filter accordingly.
Add `typecheck` scripts to db and migrator. Fix two pre-existing type
errors in the migrator apply-scripts command: the `name` positional was
never declared, so yargs typed it `unknown`. Declare it as a string
(mirroring the `add` command) and assert non-null where check() already
guarantees presence.
client and server are already linted and typechecked in CI via their
existing lint/build jobs. Formatting in CI is deferred: the repo is not
currently Prettier-clean against its pinned version.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* ci: also run PR checks on push to main; trim comment
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* [Security] Invalidate sessions on password change (GHSA-g5xq-67g7-36r2)
Resetting a password did not clear the user's PG-stored sessions
(connect-pg-simple, 30-day maxAge), and passport.deserializeUser looks
users up by id only. A phished/attacker session therefore survived a
password reset for up to 30 days, even after the legitimate user took
the expected recovery action.
Add deleteSessionsForUser(), which removes the user's rows from
public.session (matched on passport's `sess -> 'passport' ->> 'user'`),
and call it from both password-change paths:
- resetPasswordForToken: deletes all of the user's sessions.
- UserApi.changePassword: deletes all *other* sessions, preserving the
caller's own (via the request's sessionID) so they aren't logged out
mid-action.
No DB migration; deserializeUser is unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Drop advisory ID from code comments
Keep the explanatory comments; remove the GHSA reference so it isn't
exposed in source while the advisory is unpublished.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Drop file-path reference from sessionPersistence comment
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
* Make password-change + session invalidation atomic
Wrap the password update and session purge in a single transaction
(via makeKyselyTransactionWithRetry) in both resetPasswordForToken and
UserApi.changePassword. Previously these were separate commits, so a
failure after the password update left the user's sessions alive — the
exact persistence the fix is meant to prevent.
Also narrow the test's eslint-disable for the unused mock constructor
args from a block disable to per-line comments.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>