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

Open all files with `/files` path except for `.html` and .svg` #2449

Merged
merged 11 commits into from Jun 21, 2017

Conversation

Projects
None yet
3 participants
@gnestor
Contributor

gnestor commented Apr 27, 2017

Closes #2404

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor Apr 27, 2017

Contributor

@takluyver Can you review?

Contributor

gnestor commented Apr 27, 2017

@takluyver Can you review?

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver Apr 28, 2017

Member

.htm should be treated this way as well, it's a common extension for html files. I wonder if we should be doing it on mime type instead of extension - I think anything that our server offers with mimetype text/html can have the same effect, and that can vary by system. In some cases, installed apps can affect what extensions are recognised for a mimetype.

@minrk @rgbkrk do you know of any file types other than html and svg that we should be displaying in the iframe?

Member

takluyver commented Apr 28, 2017

.htm should be treated this way as well, it's a common extension for html files. I wonder if we should be doing it on mime type instead of extension - I think anything that our server offers with mimetype text/html can have the same effect, and that can vary by system. In some cases, installed apps can affect what extensions are recognised for a mimetype.

@minrk @rgbkrk do you know of any file types other than html and svg that we should be displaying in the iframe?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Apr 28, 2017

Member

It's probably best to avoid a 'not_safe' blacklist, because it's easy to miss things (like the many synonyms for html) instead having a 'safe' list for things that can be trusted to be displayed raw. It seems like it makes more sense to specify a list of things that don't work in the iframe (are there examples other than PDF?)

Member

minrk commented Apr 28, 2017

It's probably best to avoid a 'not_safe' blacklist, because it's easy to miss things (like the many synonyms for html) instead having a 'safe' list for things that can be trusted to be displayed raw. It seems like it makes more sense to specify a list of things that don't work in the iframe (are there examples other than PDF?)

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor May 6, 2017

Contributor

I updated so that is_viewable is used in place of is_not_safe (just includes htm and html). Every other file extensions that isn't editable will be opened using the /files path and handled by the browser. I also updated the is_editable list to be more comprehensive (using the list from https://github.com/sindresorhus/text-extensions/blob/master/text-extensions.json).

Contributor

gnestor commented May 6, 2017

I updated so that is_viewable is used in place of is_not_safe (just includes htm and html). Every other file extensions that isn't editable will be opened using the /files path and handled by the browser. I also updated the is_editable list to be more comprehensive (using the list from https://github.com/sindresorhus/text-extensions/blob/master/text-extensions.json).

@takluyver

This comment has been minimized.

Show comment
Hide comment
@takluyver

takluyver May 8, 2017

Member

Isn't that the same blacklisting approach just with the name 'viewable' in place of 'not_safe'?

I see xht, xhtml, mht and mhtml in the editable list - in certain circumstances, any of these could be rendered as HTML in a browser, so viewing them should open them in the iframe. This rather validates Min's point about the problems with a blacklist.

Member

takluyver commented May 8, 2017

Isn't that the same blacklisting approach just with the name 'viewable' in place of 'not_safe'?

I see xht, xhtml, mht and mhtml in the editable list - in certain circumstances, any of these could be rendered as HTML in a browser, so viewing them should open them in the iframe. This rather validates Min's point about the problems with a blacklist.

@gnestor gnestor added this to the 5.1 milestone May 30, 2017

@gnestor

This comment has been minimized.

Show comment
Hide comment
@gnestor

gnestor May 31, 2017

Contributor

I have updated this PR. The way it works now:

  • By default, all files (not directories) are opened using /files prefix and display the "View" button
  • If a file is html (according to the list of html extensions and text/html mime type), it's opened using the /view prefix
  • If a file is editable (according to the list of editable extensions and text/* and application/* mime types), it's opened using the /edit prefix and displays the "Edit" button
  • If a file is a notebook, it's opened using the /notebooks prefix
    • Notebooks can be edited in the text editor using the "Edit" button
Contributor

gnestor commented May 31, 2017

I have updated this PR. The way it works now:

  • By default, all files (not directories) are opened using /files prefix and display the "View" button
  • If a file is html (according to the list of html extensions and text/html mime type), it's opened using the /view prefix
  • If a file is editable (according to the list of editable extensions and text/* and application/* mime types), it's opened using the /edit prefix and displays the "Edit" button
  • If a file is a notebook, it's opened using the /notebooks prefix
    • Notebooks can be edited in the text editor using the "Edit" button

@takluyver takluyver merged commit ce56217 into jupyter:master Jun 21, 2017

4 checks passed

codecov/patch Coverage not affected when comparing 9cd249e...6a1f807
Details
codecov/project 78.13% (-0.33%) compared to 9cd249e
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gnestor gnestor referenced this pull request Aug 3, 2017

Merged

Add 5.1.0 to changelog #2723

@gnestor gnestor deleted the gnestor:issue-2404 branch Oct 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment