Skip to content

Guard material writes in ExpGeneratorHelper with UpdatePermission - #8043

Open
ankurjuneja wants to merge 5 commits into
developfrom
fb_provenanceLineagePermCheck_1452
Open

ankurjuneja wants to merge 5 commits into
developfrom
fb_provenanceLineagePermCheck_1452

Conversation

@ankurjuneja

Copy link
Copy Markdown
Contributor

Rationale

Tightens permission handling in experiment run creation, with added provenance recording test coverage.

Related Pull Requests

Changes

@ankurjuneja
ankurjuneja marked this pull request as ready for review September 15, 2026 00:00

@labkey-jeckels labkey-jeckels left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also see the calls to addProvenance, addProvenanceInputs, and addProvenanceOutputs around line 463. Should they get read checked too?

Comment thread experiment/src/org/labkey/experiment/pipeline/ExpGeneratorHelper.java Outdated

@labkey-jeckels labkey-jeckels left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Parallel finding probably worth addressing at the same time:

Issue: handleMaterial — the globally-resolving lookup that makes this bug possible — is also called from handleRun for output materials, which flow into saveSimpleExperimentRun. That method does outputMaterial.setSourceApplication(protApp2); outputMaterial.setRun(run); Table.update(...) with no permission check and no isOperationPermitted check anywhere in its body.

Why it matters: The experiment-saveBatch / assay-save APIs reach the identical lineage rewrite on an arbitrary cross-container material through a different entry point. Fixing only the ExpGeneratorHelper write site closes the provenance door while leaving the wider one open, so the security goal in the PR title isn't actually met system-wide. (Pre-existing, not introduced here — but it is the same defect class and shares the same root cause.)
Suggestion: Either push the guard down into handleMaterial (which already knows the resolved material and the ViewContext user) so every caller inherits it, or extract the assertCanEditLineage helper somewhere shared and call it from saveSimpleExperimentRun before the Table.update. If that's deliberately out of scope for this PR, file a follow-up issue and say so in the description.

Comment on lines 479 to 480
pvs.addProvenanceInputs(container, stepApp, action.getObjectInputs());
pvs.addProvenanceOutputs(container, stepApp, action.getObjectOutputs());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Similar concerns on these inputs, which can come from ProvenanceController.StopRecordingAction

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.

2 participants