Skip to content

Add recommendations to use https for remote resources #2299

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 31, 2022
Merged

Add recommendations to use https for remote resources #2299

merged 5 commits into from
May 31, 2022

Conversation

@mattgarrish
Copy link
Member

@mattgarrish mattgarrish commented May 19, 2022 •
edited by pr-preview bot
Loading

Adds authoring recommendation to reference resources via https to limit possibility of network attacks and adds reading system recommendation to only load remote resources referenced via https.

This could lead to warnings in existing content, but in this case I think there's a strong security basis for making the change. I don't get the impression that publishers actually use a lot of remote resources, either, but we should discuss this in a WG meeting before merging.

Fixes #2263

💥 Error: 500 Internal Server Error 💥

PR Preview failed to build. (Last tried on May 27, 2022, 1:45 PM 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/iYt5U4?isPreview=true&publishDate=2022-05-27
ReSpec version: 32.1.7
File a bug: https://github.com/w3c/respec/
Error: Error: Evaluation failed: Timeout: document.respec.ready didn't resolve in 27830ms.
    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:244: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.

@mattgarrish
add authoring and reading system recommendations to only reference/lo…
cc856e3 
…ad remote resources when referenced via https
@mattgarrish
Copy link
Member Author

mattgarrish commented May 19, 2022

@mattgarrish
Copy link
Member Author

mattgarrish commented May 19, 2022

Also note that I opted to put the recommendations directly in the relevant resources sections as these are very specific to remote resources.

iherman
iherman approved these changes May 22, 2022
View reviewed changes
@iherman iherman added the Agenda+ Issues that should be discussed during the next working group call. label May 23, 2022
@dlazin dlazin self-requested a review May 27, 2022 00:11
dlazin
dlazin approved these changes May 27, 2022
View reviewed changes
@iherman
Copy link
Member

iherman commented May 27, 2022

The issue was discussed in a meeting on 2022-05-26

List of resolutions:

View the transcript

1.2. External Resources should be loaded securely.

See github issue epub-specs#2263.

See github pull request epub-specs#2299.

Dave Cramer: this issue comes with this PR.
… this is around remote resources, and recommended that they be served over HTTPS.
… avoiding person in the middle and other such network attacks.

Dan Lazin: my only concern is the PR itself is not super clear in its language.
… "RS may not load resources".
… what we mean is there is a chance RS may not load.
… I'll do a pass.

Dave Cramer: this is SHOULD level, so would we test whether a remote resource is served via HTTP, and that would result in a warning from epubcheck?.

Matt Garrish: yes, that is what I would expect.

Dave Cramer: this could affect existing content, but I expect it would be rare.

Matt Garrish: generally the direction of the web is HTTPS too.
… this will cause some warnings, but security outweighs. We should move in the direction of the web.

Dave Cramer: using backwards compatibility as reason not to fix security issue isn't where we want to be.

Proposed resolution: Approve PR 2299, close issue 2265. (Wendy Reid)

Brady Duga: +1.

Wendy Reid: +1.

Shinya Takami (高見真也): +1.

Matthew Chan: +1.

Masakazu Kitahara: +1.

Dan Lazin: +1.

Dave Cramer: +1.

Toshiaki Koike: +1.

Matt Garrish: +1.

Resolution #2: Approve PR 2299, close issue 2265.

@iherman iherman mentioned this pull request May 27, 2022
Closed
mattgarrish and others added 4 commits May 27, 2022 10:29
@mattgarrish @dlazin
Update epub33/core/index.html
307fd15 
Co-authored-by: Dan Lazin <dlazin@users.noreply.github.com>
@mattgarrish @dlazin
Update epub33/core/index.html
a068a01 
Co-authored-by: Dan Lazin <dlazin@users.noreply.github.com>
@mattgarrish
remove "strongly" from before normative recommendations
aa52ad4
@mattgarrish
Merge branch 'main' into fix/issue-2263
f74eb65
@iherman iherman removed the Agenda+ Issues that should be discussed during the next working group call. label May 31, 2022
@iherman
Copy link
Member

iherman commented May 31, 2022

@mattgarrish any reason not to merge this?

@iherman iherman merged commit 87c0c8d into main May 31, 2022
@mattgarrish mattgarrish deleted the fix/issue-2263 branch June 17, 2022 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iherman iherman iherman approved these changes

@shiestyle shiestyle shiestyle approved these changes

@dlazin dlazin dlazin approved these changes

@npdoty npdoty Awaiting requested review from npdoty

@wareid wareid Awaiting requested review from wareid

+1 more reviewer

@dauwhe dauwhe dauwhe approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

External resources should be loaded securely

6 participants

@mattgarrish @iherman @dauwhe @shiestyle @dlazin