Skip to content

Restore requirement that relative URLs resolve to resources #2028

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 8 commits into from
Mar 10, 2022
Merged

Restore requirement that relative URLs resolve to resources #2028

merged 8 commits into from
Mar 10, 2022

Conversation

@mattgarrish
Copy link
Member

@mattgarrish mattgarrish commented Mar 3, 2022 •
edited by pr-preview bot
Loading

I've added a requirement that valid-relative-container-URL-with-fragment strings resolve to resources in the OCF Abstract Container to the end of 6.1.5.

Fixes #2024

Preview | Diff

@mattgarrish
restore requirement that valid-relative-container-URL-with-fragment s…
726c763 
…trings resolve to resources in the OCF Abstract Container
@mattgarrish mattgarrish requested review from iherman and rdeltour March 3, 2022 19:06
@iherman
Copy link
Member

iherman commented Mar 4, 2022

The issue was discussed in a meeting on 2022-03-03

List of resolutions:

View the transcript

3. Clarify that resources must be present for relative paths (issue epub-specs#2024)

See github issue epub-specs#2024.

Dave Cramer: Our other issue is similar. When we did this we lost a requirement that there be a resource at the end of the path.
… originally we had 1. not leak and 2. must exist.

See github pull request epub-specs#2028.

Dave Cramer: We lost part 2. We should put it back. There is already a PR.

Matt Garrish: We just need to put the existence text back.

Dave Cramer: Yes, and it sounds like the PR does just that.
… I am in favor of merging.

Dan Lazin: I don't know if it follows that there must always be a thing that is referenced.
… For instance a book on how to write epub that shows errors (eg this is what it looks like when a resource is missing).
… The cases are rare, but I can imagine some.
… and there are plenty of ways to create bad content.

Wendy Reid: Going with must on this, because nothing stops people from ignoring epubcheck.
… In general epubcheck is heavily relied on by publishers to tell them of problems.
… and this will more often than not help those publishers find problems.

Matt Garrish: I basically agree with wendyreid.
… epubcheck does look for this now.
… There are other things epubcheck will flag.
… This seems like a can of worms to change, and error has worked really well to date.

Dan Lazin: Those are persuasive arguments.
… I thought epubcheck gave warnings on shoulds, isn't that good enough?.

Wendy Reid: Errors scare them more.

Dan Lazin: I am ok with must, but odd that epubcheck is preflight and a conformance checker.

Proposed resolution: Merge PR 2028, close issue 2024. (Wendy Reid)

Dave Cramer: Propose merging 2028.

Dave Cramer: +1.

Matt Garrish: +1.

Dan Lazin: +1.

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

Brady Duga: +1.

Wendy Reid: +1.

Zheng Xu (徐征): +1.

Toshiaki Koike: +1.

Resolution #2: Merge PR 2028, close issue 2024.

Dave Cramer: AOB?.

@iherman iherman mentioned this pull request Mar 4, 2022
Closed
@mattgarrish
Copy link
Member Author

mattgarrish commented Mar 4, 2022

@rdeltour I'm wondering if this wording doesn't really work? The algorithm for determining if you have a valid-relative-container-URL-with-fragment string gives you a true or false value in return, not a URL that you can use to check if a resource exists at the location.

A simple check of the string as relative path could leak back outside the container.

@rdeltour
Copy link
Member

rdeltour commented Mar 4, 2022

@rdeltour I'm wondering if this wording doesn't really work?

Yeah, what "resolve" means for a URL string is not strictly defined.

Alternatively, we could use something along the lines of what I proposed back in #1888 (comment):

All relative-URL-with-fragment strings MUST, after parsing, be equal to the container URL of an existing file in the OCF Abstract Container.

or more precisely:

The result of parsing relative URL string found in the OCF Abstract Container, with the base URL defined by the context where it is found, must be the container URL of an existing file in the OCF Abstract Container.

although it is a bit of a mouthful 🤔

@mattgarrish
Copy link
Member Author

mattgarrish commented Mar 4, 2022

although it is a bit of a mouthful 🤔

Ya, that's a tough read! But what is a container URL? We don't define that in the spec, unless I'm missing something (the link doesn't go anywhere).

@rdeltour
Copy link
Member

rdeltour commented Mar 4, 2022

But what is a container URL? We don't define that in the spec, unless I'm missing something (the link doesn't go anywhere)

Sorry I copied that from the old proposal 🤦‍♂️. We now define this as Content URL.

@mattgarrish
Copy link
Member Author

mattgarrish commented Mar 9, 2022

I've updated the PR with the first proposed text. I'm hoping the reference to parsing is enough, and we don't have to get into the details of resolving base URLs... 😄

@mattgarrish mattgarrish merged commit 553c0c7 into main Mar 10, 2022
@mattgarrish mattgarrish deleted the fix/issue-2024 branch March 10, 2022 11:31
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

@rdeltour rdeltour Awaiting requested review from rdeltour

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Clarify that resources must be present for relative paths

4 participants

@mattgarrish @iherman @rdeltour