Skip to content

Fix storage and containment validation for registerView#4963

Closed
ayushtkn wants to merge 1 commit into
apache:mainfrom
ayushtkn:viewlocationValidate
Closed

Fix storage and containment validation for registerView#4963
ayushtkn wants to merge 1 commit into
apache:mainfrom
ayushtkn:viewlocationValidate

Conversation

@ayushtkn

@ayushtkn ayushtkn commented Jul 3, 2026

Copy link
Copy Markdown
Member

Currently, LocalIcebergCatalog.registerNewTable properly enforces catalog storage governance by calling validateLocationForTableLike and validateMetadataFileInTableDir on the user-provided metadataFileLocation.

However, registerView omits these checks, directly reading the provided metadata file URI before any authorization or structural containment checks occur. This creates a governance gap where a user could register a view using a metadata file stored outside of the catalog's allowed locations, potentially bypassing storage RBAC policies.

This PR brings registerView into parity with registerTable by adding the missing validation checks

Checklist

  • 🛡️ Don't disclose security issues! (contact security@apache.org)
  • 🔗 Clearly explained why the changes are needed, or linked related issues: Fixes #
  • 🧪 Added/updated tests with good coverage, or manually tested (and explained how)
  • 💡 Added comments for complex logic
  • 🧾 Updated CHANGELOG.md (if needed)
  • 📚 Updated documentation in site/content/in-dev/unreleased (if needed)

Copilot AI review requested due to automatic review settings July 3, 2026 10:30
@github-project-automation github-project-automation Bot moved this to PRs In Progress in Basic Kanban Board Jul 3, 2026

Copilot AI 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.

Pull request overview

This PR closes a storage-governance gap in LocalIcebergCatalog.registerView by adding the same location/containment validation that already exists for table registration, preventing view registration from reading user-supplied metadata from disallowed locations.

Changes:

  • Add validateLocationForTableLike(...) to registerView before loading/reading the metadata file.
  • Add validateMetadataFileInTableDir(...) after parsing view metadata to ensure the metadata file is contained under the view’s declared location.
  • Add targeted tests asserting registerView rejects (1) metadata in disallowed storage locations and (2) metadata files outside the view directory.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/LocalIcebergCatalog.java Adds pre-read location validation and post-parse containment validation to registerView.
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/AbstractLocalIcebergCatalogViewTest.java Adds regression tests for disallowed metadata locations and metadata-file containment enforcement.

Comment on lines 1019 to 1032
@@ -1025,6 +1028,7 @@ public View registerView(TableIdentifier identifier, String metadataFileLocation

InputFile metadataFile = fileIO.newInputFile(metadataFileLocation);
ViewMetadata metadata = ViewMetadataParser.read(metadataFile);
validateMetadataFileInTableDir(identifier, metadata.location(), metadataFileLocation);
ops.commit(null, metadata);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I noticed this while writing the tests—validateMetadataFileInTableDir strictly reads from the realm-level configuration and ignores the catalog-level overrides, meaning a catalog configured with ALLOW_EXTERNAL_TABLE_LOCATION=true will still be blocked by this check.

Since this exact same split-brain configuration bug currently exists for registerTable, I opted to keep the implementation as-is to bring registerView into strict behavioral parity with tables.

I plan to chase that separately if people agree that ain't deliberate rather than expanding the scope here

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.

ALLOW_EXTERNAL_METADATA_FILE_LOCATION is not overridable at catalog level, but ALLOW_EXTERNAL_TABLE_LOCATION is.

This looks like a bug to me. Do you mind opening an issue for this?


InputFile metadataFile = fileIO.newInputFile(metadataFileLocation);
ViewMetadata metadata = ViewMetadataParser.read(metadataFile);
validateMetadataFileInTableDir(identifier, metadata.location(), metadataFileLocation);

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.

I think you also need to call validateLocationForTableLike – the metadata.location() value must be valid wrt to the storage configuration from the table hierarchy, AND wrt to the table directory, if escaping the directory is not allowed:

Suggested change
validateMetadataFileInTableDir(identifier, metadata.location(), metadataFileLocation);
validateLocationForTableLike(identifier, metadata.location(), resolvedParent);
validateMetadataFileInTableDir(identifier, metadata.location(), metadataFileLocation);

@ayushtkn

ayushtkn commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Hey @dimas-b this was a different issue, not sure why this PR got closed. I don't think I have access to reopen either. Should I create another PR?

@dimas-b

dimas-b commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Oops, the closing was not intentional, sorry 😅

@dimas-b dimas-b reopened this Jul 9, 2026
@github-project-automation github-project-automation Bot moved this from Done to PRs In Progress in Basic Kanban Board Jul 9, 2026
@dimas-b

dimas-b commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

@ayushtkn : I guess this got closed automatically by GH because #4966 referred to this PR with a "fixes: " tag in the description 😉

@ayushtkn
ayushtkn force-pushed the viewlocationValidate branch from 58015dc to b029a7b Compare July 9, 2026 17:35
@ayushtkn

ayushtkn commented Jul 9, 2026

Copy link
Copy Markdown
Member Author

Thanx @dimas-b for the help. Must be some bug in Github Auto close, some missing if(issue) then only close check 😉

@ayushtkn
ayushtkn force-pushed the viewlocationValidate branch from b029a7b to db40db6 Compare July 10, 2026 05:20
@ayushtkn
ayushtkn force-pushed the viewlocationValidate branch from db40db6 to 26df577 Compare July 19, 2026 04:50
@ayushtkn
ayushtkn force-pushed the viewlocationValidate branch from 26df577 to 66c2037 Compare July 19, 2026 04:51
@adutra

adutra commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

It seems that this PR is superseded by #5114, FYI.

@ayushtkn ayushtkn closed this Jul 21, 2026
@github-project-automation github-project-automation Bot moved this from PRs In Progress to Done in Basic Kanban Board Jul 21, 2026
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.

4 participants