[jfrench9]

Summary

Hardening from an independent platform security review; the full record is in the private vault (archive/security-audit-2026-10-24.md). Public text is deliberately brief and non-actionable.

• Cypher analyzer classifies keywords by token
• Custom-schema identifiers validated at the API boundary and in the runtime; creation DDL restricted to table definitions
• Cross-app SSO handoffs bound to the browser that started them (browser_nonce_hash on exchange, browser_nonce on complete — optional until the apps send them)
• Memory ids validated on every surface before a Graph API path is built
• Burst limiter runs before credential checks on graph routes
• Stripe webhook inert without billing; placeholder secrets refused at boot; the bootstrap no longer seeds them
• Worker re-checks the enqueuing user's graph role at dequeue (AuthorizationRevoked)
• Event-handler approval bound to the reviewed version (expected_updated_at; 409 on mismatch); a rename over MCP counts as a change to what it books, since a template reads handler.name
• Counterparty tax IDs masked at the GraphQL field on the MCP surface
• Unknown fields refused on command bodies whose fields all default
• Lambda image installs with lockfile hashes; CI builds check out the dispatched commit

Client contract

Additive request fields → client minor; regenerated in robosystems-typescript-client #302 and robosystems-python-client #286. extra="forbid" on the listed command bodies is generated-tier.

Companion PRs and deploy order

API (this) → robosystems-core #98 + the two clients publish → robosystems-app RoboFinSystems/robosystems-app#518 → roboledger-app #527 (+ a roboinvestor-app bump for the core nonce). Until roboledger-app ships, approving an AI-suggested posting rule returns 409.

Test plan

• just test — 17,480 passed; the one failure was the approval journey test, updated for the new field and re-run green with its file
• ruff, basedpyright (pre-commit)

🤖 Generated with Claude Code

[github-actions]

Change classification: normal — platform or application code changed.

A normal change carries a documented automated review on this pull request before merge, in place of a second human approver. The change-classification job requests that review and passes once it has been posted. Recorded by the change-classification job.

[jfrench9]

@claude please review this PR

[claude]

Claude finished @jfrench9's task in 46s —— View job

---

Review of #1945: input validation and authorization hardening

I read the non-test source diff (about 40 files) and spot-checked the surrounding code. I did not run the test suite. The PR description reports just test green.

Verdict: The approach is sound, and the changes line up with the descriptions in the PR body. I found no blocking defects. Items 1–3 are worth fixing before merge, and the rest are minor.

Should fix

1. The docstring in update_memory is now dead. In robosystems/operations/memory/service.py (~L93), memory_id = require_memory_id(memory_id) was inserted above the docstring. The string below it is now a bare expression, not the method's __doc__. Move the call below the docstring.

2. Custom schema names can pass API validation but fail at DDL execution. require_identifier accepts any [A-Za-z_]\w{0,127}, which includes node and property names such as update, set, delete, copy, load, import, export, use, insert and remove.

   • The new guard in manager.py (~L864–880) runs _keyword_tokens over the whole DDL string. A property named import or update therefore trips is_bulk_operation, is_admin_operation or is_non_read_call, or ends up misclassified.
   • The user would see a generic runtime ValueError at apply time, not a 422 at the API boundary.
   • The old \b regex had the same blind spot, so this is not a new regression. The new guard does make it visible.
   • Fix: reject these as reserved words in require_identifier, or run the same classifier on each name there. Also add a test with a keyword-named property, e.g. {name: "update", type: "STRING"}.

3. Confirm every worker task's user_id is a real graph member, or the new dequeue check will fail it.

   • reauthorize_task calls GraphUser.get_effective_role(user_id, …) for every task whose required_role is not None. A non-user principal such as "system" gets role is None and is refused with AuthorizationRevoked.
   • dispatch_connection_sync is called with SYSTEM_USER_ID (dagster/jobs/connection_sync.py:252). The comment there says the run is stamped with the connection's own user, so it is probably fine.
   • The QuickBooks auto-map enqueue (adapters/quickbooks/pipeline/load.py:597) uses config.user_id for an operator task, which now requires viewer.
   • Please confirm that neither path can carry "system". Also check that a connection owner who is not a GraphUser on the graph (only an org member) resolves through the implicit org-admin grant. If either case fails, the failure is silent: the task is marked failed with AuthorizationRevoked.

Minor / nits

4. Dead code in the Cypher analyzer. After this change nothing calls write_pattern, bulk_pattern or admin_pattern (cypher_analyzer.py L210–219), and _validate_keyword_context (L670) is only used by its own tests. Remove them, or note that they are kept deliberately.

5. The DDL head check is strict about spacing and case. " ".join(statement.split()[:3]).upper() normalizes whitespace and case, which is good. A statement like CREATE NODE TABLE IF NOT EXISTS … still passes. Because the guard only looks at the first three words, it relies on the identifier rules to keep a second statement out of the same string (e.g. via ;). That seems fine, since the pattern forbids ;. A one-line comment saying so would help.

6. SSO binding is optional on both sides during rollout (this is by design).

   • A session minted with no hash is redeemed only by a client that sends no nonce.
   • A client that sends a nonce against an unbound session is rejected, which is the safe direction.
   • Until the apps send browser_nonce_hash, new handoffs are not actually bound. The PR body says as much. Add a tracking item to make the field required once the apps ship.
   • The nonce comparison uses secrets.compare_digest on hex digests, which is correct.

7. The STRIPE_WEBHOOK_SECRET check is heuristic. "your_" in secret or len < 32 only covers prod and staging. That is fine as defense in depth. Real whsec_… secrets are about 38 characters, so the length floor is safe.

8. The webhook returns 404 when billing is off. This is correct, and the test covers it. Check that Stripe's dashboard does not retry against a disabled endpoint for long. It is only a concern if a billing-off environment still has a registered endpoint.

9. Memory-id validation is consistent. It is applied on REST (path pattern), on MCP, in MemoryService and with quote(..., safe='') in the client. The list route and any other routes taking memory_id were not in the diff. I believe get_memory is the only REST route with that parameter. Please confirm.

Looks good

• Keyword classification by token: 1SET splits into 1 and SET, while x1set stays one identifier. This matches the engine lexer and closes the \b bypass.
• The burst limiter is now a router-level dependency, so it runs before per-endpoint credential checks.
• tax_id is masked per field (strawberry.Private plus a resolver) rather than by response key, so aliases and fragments cannot bypass it.
• The approval-binding flow is solid. It compares at millisecond grain to survive the JavaScript round-trip, and a rename now counts as a substantive change because templates read handler.name. StaleApprovalError maps to 409 as documented.
• Dequeue re-authorization runs on every path (first run, reaper requeue, DLQ retry, resume). It releases the task lock only on the first attempt, and the per-task overrides (required_role = None for graph creation, admin with require_write = False for tier upgrade, and the backup special case in the Dagster monitor) read as deliberate.
• Test coverage is good for each hardening item.

Not checked: CI results, the workflow and Dockerfile changes (.github/workflows/*, Dockerfile.lambda, bin/setup/aws.sh), and the companion client and app PRs.
· branch bugfix/input-validation-hardening

[jfrench9]

Deploy precondition: boot validation in prod and staging now refuses a placeholder or short STRIPE_WEBHOOK_SECRET (the earlier check accepted any whsec_-prefixed value). Confirm both environments' values are the real endpoint secrets before this deploys.

[jfrench9]

Precondition confirmed 2026-10-24: prod and staging both carry real endpoint secrets; nothing to set before deploying.

[jfrench9]

Review addressed in the three commits after 9f088f3c5: the unlocked exchange path now stores the browser binding (covered in test_sso_exchange.py); statement keywords are refused as custom-schema names at the API boundary, in step with the DDL guard; the analyzer's unused regex attributes are gone and the update_memory docstring is back in place. The dequeue re-check concern does not arise: connection syncs are Dagster runs, not worker tasks, and every worker enqueue stamps the acting user.