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

Disallow file URLs #2329

Merged
merged 7 commits into from
Jun 10, 2022
Merged

Disallow file URLs #2329

merged 7 commits into from
Jun 10, 2022

Conversation

iherman
Copy link
Member

@iherman iherman commented Jun 7, 2022

This is to fix #2324

I was wondering, however, whether disallowing File URLs in general is implementable by an RS; it may require the RS to patch the fetch library or something similar if a WebView is used for implementing the system. I would welcome the reaction of @bduga or @danielweck on this. (Note that a similar question may arise for Data URLs.)

See:


💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on Jun 8, 2022, 9:45 AM UTC).

More

PR Preview relies on a number of web services to run. There seems to be an issue with the following one:

🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.

🔗 Related URL


😭  Sorry, there was an error generating the HTML. Please report this issue!
Specification: http://labs.w3.org/spec-generator/uploads/rIFmii?isPreview=true&publishDate=2022-06-08
ReSpec version: 32.1.8
File a bug: https://github.com/w3c/respec/
Error: Error: Evaluation failed: Timeout: document.respec.ready didn't resolve in 27558ms.
    at ExecutionContext._evaluateInternal (/u/spec-generator/node_modules/puppeteer/lib/cjs/puppeteer/common/ExecutionContext.js:221:19)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at async ExecutionContext.evaluate (/u/spec-generator/node_modules/puppeteer/lib/cjs/puppeteer/common/ExecutionContext.js:110:16)
    at async generateHTML (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:221:12)
    at async toHTML (/u/spec-generator/node_modules/respec/tools/respecDocWriter.js:92:18)
    at async Object.generate [as respec] (file:///u/spec-generator/generators/respec.js:15:44)
    at async file:///u/spec-generator/server.js:247:48

If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.

@iherman
Copy link
Member Author

iherman commented Jun 7, 2022

cc @GJFR

@OriIdan
Copy link

OriIdan commented Jun 7, 2022 via email

@iherman
Copy link
Member Author

iherman commented Jun 7, 2022

Yes, it is implementable as the RS decides on each file to load. most RS's do not simply hand files to WebView or other loaders, they first get the file name from the OPF file and then loads it.

And what happens if a runtime script tries to use a file URL?

@OriIdan
Copy link

OriIdan commented Jun 7, 2022 via email

@iherman
Copy link
Member Author

iherman commented Jun 7, 2022

@mattgarrish I have not added the entry to the list of changes yet. I would prefer to do that before merge but after #2326 will have been merged (and maybe also #2297), to avoid nasty merge conflicts...

@mattgarrish
Copy link
Member

I'll merge main into this to get the new change log structure added and then if you add an entry it shouldn't be any worse than integrating any others.

epub33/core/index.html Outdated Show resolved Hide resolved
epub33/rs/index.html Show resolved Hide resolved
Copy link
Collaborator

@bduga bduga left a comment

Choose a reason for hiding this comment

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

I am fine with the restriction, it seems implementable to me.

epub33/core/index.html Outdated Show resolved Hide resolved
epub33/rs/index.html Show resolved Hide resolved
@iherman iherman added the Agenda+ Issues that should be discussed during the next working group call. label Jun 8, 2022
@iherman
Copy link
Member Author

iherman commented Jun 10, 2022

The issue was discussed in a meeting on 2022-06-09

List of resolutions:

View the transcript

1. Disallowing File URLs.

See github issue epub-specs#2324.

See github pull request epub-specs#2329.

Dave Cramer: I can't think of good reason to have these, but a lot of good reasons to prohibit them.

Brady Duga: yes, sounds good. File URLs seem to be interoperable. What file would you load from an epub?.

Ben Schroeter: most common thing i've seen is youtube videos, but those aren't file URLs, right?.

Brady Duga: depends on how the epub is created. Could be a link to youtube, or link to external resource that plays in your epub, but neither of those are file URLs.

Dave Cramer: file URL goes against idea that epub should be self contained, you don't want epub author to look at your files in your local machine.

Ben Schroeter: when Play gets file URL what happens?.

Brady Duga: probably gets stripped on the server, but probably gets intercepted and rejected. We might try to open it in the browser, but then the browser would probably reject it.
… there are also a lot of origin issues with file URLs.

Dave Cramer: I propose we forbid file URLs.

Brady Duga: there's already a PR open for that.

Proposed resolution: Remove file URLs, merge PR 2329 and close issue 2324. (Wendy Reid)

Ben Schroeter: +1.

Dave Cramer: +1.

Brady Duga: +1.

Masakazu Kitahara: +1.

Wendy Reid: +1.

Matthew Chan: +1.

Matt Garrish: +1.

Ben Schroeter: +1.

Toshiaki Koike: +1.

Resolution #1: Remove file URLs, merge PR 2329 and close issue 2324.

@iherman iherman mentioned this pull request Jun 10, 2022
@iherman iherman merged commit 25d0907 into main Jun 10, 2022
@iherman iherman deleted the disallow-file-scheme branch June 10, 2022 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agenda+ Issues that should be discussed during the next working group call.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disallowing file URLs
4 participants