Skip to content

Enforce authorization on field-lineage getEndpointSummary endpoint - #16164

Open
adilburaksen wants to merge 1 commit into
cdapio:developfrom
adilburaksen:harden/lineage-endpoint-authz
Open

Enforce authorization on field-lineage getEndpointSummary endpoint#16164
adilburaksen wants to merge 1 commit into
cdapio:developfrom
adilburaksen:harden/lineage-endpoint-authz

Conversation

@adilburaksen

Copy link
Copy Markdown

Summary

LineageHTTPHandler.getEndpointSummary (GET /v3/namespaces/{namespace-id}/apps/{app-name}/{program-type}/{program-name}/runs/{run-id}/endpoints) returns a program run's field-lineage source/sink endpoints without any authorization check, while every other read method in the same handler enforces on the requested resource:

  • datasetLineageaccessEnforcer.enforce(datasetInstance, ..., StandardPermission.GET)
  • datasetFields, datasetFieldLineage, datasetFieldLineageSummary, datasetFieldLineageDetails → enforce on the DatasetId

getEndpointSummary calls fieldLineageAdmin.getEndpoints(...) directly, and FieldLineageAdmin.getEndpoints performs no internal authorization, so the namespace in the {namespace-id} path parameter is never checked against the caller. A user with access to one namespace can read another namespace's program-run lineage endpoints.

Change

Enforce StandardPermission.GET on the requested namespace at the start of getEndpointSummary, consistent with the other read endpoints in this handler.

No behavior change for callers already authorized on the namespace.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request adds an authorization check to the getEndpointSummary endpoint in LineageHTTPHandler by enforcing GET permission on the namespace. The review feedback highlights an existing control flow bug in the same method where a catch block fails to return after sending a bad request response, potentially leading to subsequent exceptions. It is recommended to add a return; statement inside the catch block to prevent this.

Comment on lines +146 to +147
accessEnforcer.enforce(new NamespaceId(namespaceId), authenticationContext.getPrincipal(),
StandardPermission.GET);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

While adding the authorization check here is correct, there is an existing control flow bug in this method. In the catch block (lines 154–156), when an exception is caught, responder.sendJson is called to send a BAD_REQUEST response, but the method does not return.

Consequently, the execution continues to line 157:

EndpointsSummary endpointSummary = fieldLineageAdmin.getEndpoints(namespaceId, programReference, RunIds.fromString(runId));

This will:

  1. Re-evaluate RunIds.fromString(runId), which will throw the same exception again outside the try-catch block, causing a 500 Internal Server Error or propagating an unhandled exception after a response has already been committed.
  2. If ProgramType.valueOfCategoryName failed, programReference will be null, which is then passed to getEndpoints and may cause a NullPointerException.

To fix this, please add a return; statement inside the catch block:

    } catch (Exception e) {
      responder.sendJson(HttpResponseStatus.BAD_REQUEST, e.getMessage());
      return;
    }

@adilburaksen

Copy link
Copy Markdown
Author

@akkaur could you take a look at this one? The field-lineage getEndpointSummary endpoint doesn't currently check namespace authorization, so it leaks lineage across namespaces. The patch just adds the same enforcePrivilege call the neighbouring endpoints use. Happy to adjust the wording of the check if you'd rather.

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.

1 participant