You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
ScoreCalibration is the only entity in MaveDB whose READ rule is stricter than its parent's: private defaults to True, and publishing a score set does not publish its calibrations. Several read paths served calibration data based solely on the caller's permission to read the owning score set, exposing thresholds, baseline scores, oddsPath ratios and ACMG criteria to anonymous callers.
Separately, the idiom used to narrow those responses — assigning a filtered list back onto an ORM relationship — silently marks the withheld rows as orphans. Every relationship involved is mapped cascade="all, delete-orphan", so the next flush deletes them.
Both problems share a root cause: assert_permission guards the entity a request names, not the entities a response contains. The second set is covered by convention in each handler and must be applied independently each time.
Problem
Leaks
Surface
Exposure
VA-Spec annotation building, recently-published listing, public data export
Anonymous callers received private calibrations' thresholds, baseline scores, oddsPath ratios and ACMG criteria
GET /experiments/{urn}/score-sets
Same, via a route returning the full ScoreSet view model with no calibration filter
The score set owner received community private calibrations owned by others (owners are not entitled to community calibrations, only investigator-provided ones)
Steps to reproduce (representative case):
Create a score set and attach a calibration, leaving private at its default of True.
Publish the score set. The calibration remains private.
As an anonymous caller, request GET /experiments/{experiment_urn}/score-sets.
Expected: the response carries no calibration the caller may not read. Observed: the response carries the full calibration definition, including its functional classifications, ranges, baseline score and ACMG criteria.
slice-assigning experiments → experiment count 2 → 1 on commit, cascading to score sets and variants
setting superseding_score_set = None nulls the other score set's replaces_id, but only because the handler reads the attribute for its permission check first — that read arms the de-association, and without it the assignment is inert
Read paths survived only because two properties happen to hold, neither written down nor enforced: SessionLocal is built autoflush=False, and no read handler commits. The public data export script was genuinely exposed, because it commits under its supported --commit flag, and would have deleted every withheld private calibration plus every published experiment with no CC0 score sets.
Exposure assessment
The dump's universe is published_date IS NOT NULLand CC0-licensed. Two private calibrations on published CC0 score sets reached main.json. One of those is research-use-only and so was excluded from the VA-Spec NDJSON by the annotation eligibility check; the other has score ranges and did reach it. Note that the NDJSON leak is as complete as the JSON one — the calibration-as-document builder embeds URN, title, baseline score, flags and every functional classification, so auditing only main.json understates the exposure.
These calibrations have since been published, so action to remediate the published dumps is not strictly necessary. However, this issue must be closed before another dump is taken and the gap closed before more private calibrations are entered into the database.
Acceptance criteria
An anonymous caller receives no private calibration from: the VA-Spec annotation streams, the recently-published listing, the experiment score-set listing, the public data export, or any score-set mutation endpoint response.
A caller entitled to a private calibration (its creator, a contributor when it is investigator-provided, or an admin) still receives it.
A score set owner does not receive a community private calibration they do not own.
A variant whose only calibration is invisible to the caller degrades to a study result rather than emitting a statement with empty evidence.
Every emitted VA-Spec annotation carries a calibration-scope declaration, including study results that carry no calibration-derived content.
A calibration with an unset private is withheld rather than raising, so it cannot surface as a truncated download mid-stream.
No read path assigns a narrowed list back onto ScoreSet.score_calibrations, ExperimentSet.experiments, or Experiment.score_sets.
Running the public data export with --commit deletes no rows.
Regression tests fail if the default is widened so that an omitted caller means "everything" rather than "the public".
Implementation notes
Fixed across bb814ebd, 08256190, a5e8be80, 349576a9, 6b88a8f8 and 275a3b67 on bugfix/bencap/calibration-permissions.
What was added, deliberately small:
A generic Viewer in the permissions package: may_read, visible, id-keyed memoization, and a fail-closed check for entities that cannot state their own visibility. Concrete viewers live beside their entity's rules and bind that entity's own permission function rather than the central dispatcher, since a viewer only ever asks about one type.
A Principal carrying the caller and caching its viewers, resolved through a FastAPI dependency so request scoping is enforced by the framework rather than by discipline. A Principal must never be used as a default argument value — Python evaluates defaults once at import, so the instance and its caches would be shared across every request. A test enforces this by inspecting every function signature in the package.
A module-private response constructor in the score-sets router that every route returning a ScoreSet view model passes through. It narrows the validated view model, handles both sub-resources, and let all four ORM mutations in that module be deleted. The search routes are a documented exception: they answer with the short view model, which carries neither sub-resource.
Deliberately not done, to avoid scaling a convention that has already failed:
No viewer for any second entity.
No shared response-constructor module. It would only cover routes building the ScoreSet view model, and the same ORM graph is serialized in at least six shapes — full view model, short view model, public dump, CSV, VA-Spec NDJSON, and mapped-variants JSON. The only layer common to all six is the session.
No before_flush read-path guard. One was written and measured, then discarded: it inferred read-only-ness from ambient request state, so with no context it silently no-opped, and a no-op is indistinguishable from an approved check. Its one durable finding is worth keeping — across roughly 4000 tests the entire codebase has exactly one legitimate read-path write, in the authentication path that records login state.
Still latent, accepted rather than fixed: twelve association-filter sites in the collections router, and the gnomAD mapped-variants filter in the score-sets router. All twelve collections sites sit after their handler's commit, so firing one would require someone adding a second commit below the filter. These should be eliminated as a class rather than converted one at a time.
Follow-up: see #808 for a more durable fix which addresses the deliberately deferred bullets from above.
Summary
ScoreCalibrationis the only entity in MaveDB whose READ rule is stricter than its parent's:privatedefaults toTrue, and publishing a score set does not publish its calibrations. Several read paths served calibration data based solely on the caller's permission to read the owning score set, exposing thresholds, baseline scores, oddsPath ratios and ACMG criteria to anonymous callers.Separately, the idiom used to narrow those responses — assigning a filtered list back onto an ORM relationship — silently marks the withheld rows as orphans. Every relationship involved is mapped
cascade="all, delete-orphan", so the next flush deletes them.Both problems share a root cause:
assert_permissionguards the entity a request names, not the entities a response contains. The second set is covered by convention in each handler and must be applied independently each time.Problem
Leaks
GET /experiments/{urn}/score-setsScoreSetview model with no calibration filtercreate_score_set,upload_score_set_variant_data,update_score_set_with_variants,update_score_set,publish_score_setSteps to reproduce (representative case):
privateat its default ofTrue.GET /experiments/{experiment_urn}/score-sets.Expected: the response carries no calibration the caller may not read.
Observed: the response carries the full calibration definition, including its functional classifications, ranges, baseline score and ACMG criteria.
Delete-orphan hazard
The narrowing idiom in use was:
Verified empirically, not inferred from mapper configuration:
score_calibrations→ calibration row count 1 → 0 on commitexperiments→ experiment count 2 → 1 on commit, cascading to score sets and variantssuperseding_score_set = Nonenulls the other score set'sreplaces_id, but only because the handler reads the attribute for its permission check first — that read arms the de-association, and without it the assignment is inertRead paths survived only because two properties happen to hold, neither written down nor enforced:
SessionLocalis builtautoflush=False, and no read handler commits. The public data export script was genuinely exposed, because it commits under its supported--commitflag, and would have deleted every withheld private calibration plus every published experiment with no CC0 score sets.Exposure assessment
The dump's universe is
published_date IS NOT NULLand CC0-licensed. Two private calibrations on published CC0 score sets reachedmain.json. One of those is research-use-only and so was excluded from the VA-Spec NDJSON by the annotation eligibility check; the other has score ranges and did reach it. Note that the NDJSON leak is as complete as the JSON one — the calibration-as-document builder embeds URN, title, baseline score, flags and every functional classification, so auditing onlymain.jsonunderstates the exposure.These calibrations have since been published, so action to remediate the published dumps is not strictly necessary. However, this issue must be closed before another dump is taken and the gap closed before more private calibrations are entered into the database.
Acceptance criteria
privateis withheld rather than raising, so it cannot surface as a truncated download mid-stream.ScoreSet.score_calibrations,ExperimentSet.experiments, orExperiment.score_sets.--commitdeletes no rows.Implementation notes
Fixed across
bb814ebd,08256190,a5e8be80,349576a9,6b88a8f8and275a3b67onbugfix/bencap/calibration-permissions.What was added, deliberately small:
Viewerin the permissions package:may_read,visible, id-keyed memoization, and a fail-closed check for entities that cannot state their own visibility. Concrete viewers live beside their entity's rules and bind that entity's own permission function rather than the central dispatcher, since a viewer only ever asks about one type.Principalcarrying the caller and caching its viewers, resolved through a FastAPI dependency so request scoping is enforced by the framework rather than by discipline. APrincipalmust never be used as a default argument value — Python evaluates defaults once at import, so the instance and its caches would be shared across every request. A test enforces this by inspecting every function signature in the package.ScoreSetview model passes through. It narrows the validated view model, handles both sub-resources, and let all four ORM mutations in that module be deleted. The search routes are a documented exception: they answer with the short view model, which carries neither sub-resource.Deliberately not done, to avoid scaling a convention that has already failed:
ScoreSetview model, and the same ORM graph is serialized in at least six shapes — full view model, short view model, public dump, CSV, VA-Spec NDJSON, and mapped-variants JSON. The only layer common to all six is the session.before_flushread-path guard. One was written and measured, then discarded: it inferred read-only-ness from ambient request state, so with no context it silently no-opped, and a no-op is indistinguishable from an approved check. Its one durable finding is worth keeping — across roughly 4000 tests the entire codebase has exactly one legitimate read-path write, in the authentication path that records login state.Still latent, accepted rather than fixed: twelve association-filter sites in the collections router, and the gnomAD mapped-variants filter in the score-sets router. All twelve collections sites sit after their handler's commit, so firing one would require someone adding a second commit below the filter. These should be eliminated as a class rather than converted one at a time.
Follow-up: see #808 for a more durable fix which addresses the deliberately deferred bullets from above.