Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly allow imported file entries pointing to file system #2083

Merged
merged 2 commits into from
Jul 18, 2023

Conversation

jonatanklosko
Copy link
Member

@jonatanklosko jonatanklosko commented Jul 17, 2023

If there are any files that point directly to a file system, we always stamp the notebook. When importing, if the stamp is invalid we put those files in quarantine and require the user to explicitly allow them.

This prevents the scenario where the user imports a valid-looking notebook that does something with a local/s3 file, except the file points somewhere they not expect.

file_entries_import.mov

@github-actions
Copy link

github-actions bot commented Jul 17, 2023

Uffizzi Preview deployment-31106 was deleted.

Copy link
Contributor

@josevalim josevalim left a comment

Choose a reason for hiding this comment

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

LGTM!!! Quick question, when we store the file entry in the stamp, which information do we store? I assume the name, the file path, and the file system? Which parameters of the file system do we store? I assume we only store its hash? We don't store any information about where the bucket actually is, correct?

@jonatanklosko
Copy link
Member Author

@josevalim the file entries information is stored as part of plain notebook metadata (readable at the top), it includes the name, file system id (for S3 we use hash of the bucket url) and the path.

With this PR what we store in the stamp is the names of file entries yet to be allowed. So if you import a file and there are disallowed entries, and you save it with the stamp, we know which entries are not yet allowed. If all files are allowed we store an empty list (because we still need the stamp).

@jonatanklosko jonatanklosko merged commit 5ff5e09 into main Jul 18, 2023
6 checks passed
@jonatanklosko jonatanklosko deleted the jk-files-allow branch July 18, 2023 00:00
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