Skip to content

Enforce server-side ownership and session authorization (#708) - #711

Merged
rhit-changs merged 5 commits into
mainfrom
fix/708-exp-file-authz
Nov 12, 2026
Merged

rhit-changs merged 5 commits into
mainfrom
fix/708-exp-file-authz

Conversation

@rhit-changs

@rhit-changs rhit-changs commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Several existing Server Actions and experiment/file routes trusted caller-supplied identity or role fields, or lacked ownership checks at the server boundary. This change derives authorization from the existing Web session or authenticated CLI identity, rejects unauthorized operations before database writes or backend dispatch, and preserves legitimate owner and shared-reader workflows.

Refs #708. This covers the agreed application authorization scope; it does not close the broader issue.

Changes

  • Require the current database administrator role for user listing and role changes. Keep experiment cancellation available to the owner and the existing current-admin Stop workflow; sharing does not grant cancellation rights.
  • Use one server-only Web/CLI submission helper with an explicit allowlist of editable settings. Validate the owned executable and derive identity, role, lifecycle fields, counters, and timestamps on the server. Web uploads derive ownership from the session.
  • Apply owner/shared-reader checks to experiment and artifact reads, and owner checks to edits, deletion, archive operations, and file metadata writes. Bind share creation, redemption, unfollow, and file copies to the authenticated user. Copies receive fresh metadata and belong to the recipient; executable reads retain the approved owner/shared-user policy.
  • Move CLI database operations behind private server-only helpers so token-authenticated routes do not depend on Web-session Server Actions. Require an owned executable for CLI submission. Ignore malformed historical sharing values instead of treating non-arrays as access grants.
  • Authenticate experiment starts with either the existing Web session or a CLI token, validate the request, and check ownership before calling Flask. Starting cannot provision a new CLI account. Resolve supported BSON string/ObjectId references without selecting between colliding identities; reject missing or ambiguous mappings without rewriting accounts.
  • Keep file identity consistent: the backend opens the exact requested GridFS ID, and Web/CLI artifact lookups use the authorized experiment's canonical ID. Unrelated and missing experiments produce the same Web log-download 404 response.
  • Use one shared SSE lifecycle with serialized queries and coalesced updates. Recheck identity and admin authorization before reads, before sending snapshots, and on non-overlapping heartbeats. Access-changing updates, replacements, and deletions invalidate captured snapshots. Transient authorization failures suspend delivery until recovery. Confirmed revocation closes the stream.

Collaborator review follow-up

Reviewed all ten comments from the review on df66d89.

Comments Verification and resolution
1, 4 Sustained changes could starve delivery and authorization work could overlap. Queries now serialize and coalesce. Heartbeats skip overlapping work. Transient authorization failures pause delivery and retry. Fresh pre/post-query authorization is retained without a new authorization-cache delay.
2 Confirmed the unhandled file-operation rejection. Copying now preserves experiment settings, clears the executable selection on failure, presents an actionable re-upload/select-file message, and completes the dialog transition. Stale asynchronous results are ignored.
3 Missing/conflicting file ownership metadata can cause denial. Existing upload paths already write ownership metadata, and deployed legacy-data prevalence was not verified. Granting access through an arbitrary historical experiment reference would reintroduce the forged-reference exposure. File provenance checks remain enforced, with the recovery flow above.
5 Confirmed on #711. Cancellation now requires RUNNING, verifies backend acceptance, and writes status/finished atomically with a state condition. The broader #712 cancellation service and collaborator commit 54d2183 are preserved in the dependent branches.
6 Confirmed arbitrary status writes. The action accepts ARCHIVED/UNARCHIVED intents, archives terminal states only, derives the restored status from server data, and conditionally writes against the prior state.
7, 8 Confirmed the denylist/duplication. The shared submission helper allowlists the supported settings and owns the file-ownership check and initial document construction.
9 Confirmed negative hours and duplication. Shared server helpers provide ObjectId validation and filename formatting. Date arithmetic preserves the existing fixed UTC−5 convention and correctly handles day/month/year rollover.
10 Shared SSE lifecycle removes the duplicated control flow. Existing action argument signatures are retained for caller compatibility.

Independent review also identified a replacement-event revocation gap in the listener's watch filter. The filter now includes replacements, and a pipeline-aware regression verifies that revoked shared rows are discarded. Integration review found that archiving could bypass #712's late-runner-write guard; #712 now excludes both CANCELLED and ARCHIVED records, with regressions for all three archived terminal states, and #714 includes that fix. Independent re-review found no remaining actionable issues in these changes.

Validation

Review follow-up committed as d1faa36, against base 9861ac0. The following results separate current automated checks from the earlier browser/CLI validation.

Check Result
Frontend npm test 603 passed, 0 failed, 0 skipped in hosted CI: 52 OAuth/audit/BSON + 320 CLI API + 231 authorization/file/stream/copy/helper tests
npm run type-check Passed
npm run lint 0 errors; 12 existing warnings
Backend file-identity regressions 4 passed in hosted CI; local unittest/pytest cover the same four tests
Commit-range whitespace check Passed
Companion Python CLI suite, separate repository Earlier verification: 35 passed; no changes to the separate CLI repository in this follow-up

The frontend integration suites used a credential-free loopback MongoDB. Targeted regressions reproduced the relevant pre-fix failures. Independent reviews challenged identity resolution, ownership denials, file identity, submission fields, stream ordering, and cleanup races; findings in the reviewed scope were addressed and rechecked.

Hosted results are linked above. Local backend checks used Windows/Python 3.10; local Node 20 type checking and lint passed with 0 errors and 12 existing warnings. All applicable CI checks passed on d1faa36, including frontend/backend container builds. Runner/documentation builds were skipped by path filters.

Local browser verification

Before this review follow-up, two actual signed-in users exercised the local Web app, backend, MongoDB replica set, and Runner. These historical browser checks have not been repeated for d1faa36:

  • Owner upload/download bytes matched, owner submission and execution completed with 4 successes and 0 failures, and CSV values and all ZIP trial outputs matched expectations. Archive/unarchive and ordinary-user administrator access restrictions were checked.
  • The second user's access to the same original log changed from 404 before sharing, to 200 while shared, to 404 after Unfollow. Shared CSV/ZIP downloads matched the owner's bytes.
  • Copying the shared experiment produced a separate file and experiment owned by the second user. The local Runner completed that copy with 4 successes, 0 failures, and exit code 0; the original remained unchanged.
  • Unfollow removed the original from the second user's live list without a refresh, while the second user's owned copy remained available. Independent database reads confirmed an empty original sharing array and preserved ownership, counters, files, and artifacts.

These are Web-to-backend-to-Runner checks, not a live Python CLI end-to-end run. Browser checks did not exercise direct shared-source file-ID downloads, crafted mutation requests, actual session expiry/admin demotion, or SSE query races; those are distinguished from the automated regression coverage. Unfollow tested self-removal from sharing, not owner-forced revocation or share-link invalidation. A shared user's visible Archive button left the original unchanged; no explicit HTTP denial code was captured for that click.

Compatibility and scope

@federerl federerl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review of the ownership/session-authorization changes. Comments 1-4 look like regressions introduced by this PR and are worth resolving before merge; 5-7 are authorization gaps that remain; 8-10 are duplication cleanups. These come from reading the diff, not from running the code.

Comment thread apps/frontend/lib/mongodb_funcs.ts Resolved
Comment thread apps/frontend/lib/mongodb_funcs.ts Resolved
Comment thread apps/frontend/lib/mongodb_funcs.ts Resolved
Comment thread apps/frontend/lib/mongodb_funcs.ts Resolved
Comment thread apps/frontend/lib/mongodb_funcs.ts Resolved
Comment thread apps/frontend/app/api/cli/files.ts Resolved
@rhit-changs
rhit-changs requested a review from federerl November 12, 2026 23:12
@rhit-changs
rhit-changs added this pull request to the merge queue Nov 12, 2026
Merged via the queue into main with commit d1ccafe Nov 12, 2026
8 checks passed
@rhit-changs
rhit-changs deleted the fix/708-exp-file-authz branch November 12, 2026 23:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants