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

Being explicit about relative URIs #321

Closed
mnot opened this issue Feb 21, 2020 · 10 comments · Fixed by #367
Closed

Being explicit about relative URIs #321

mnot opened this issue Feb 21, 2020 · 10 comments · Fixed by #367

Comments

@mnot
Copy link
Member

mnot commented Feb 21, 2020

2616's definition of Referer said:

If the field value is a relative URI, it SHOULD be interpreted relative to the Request-URI.

7231 dropped that, thanks to this commit.

We should probably restore it, given it can cause misconceptions like this.

@mnot mnot added the semantics label Feb 21, 2020
@mnot
Copy link
Member Author

mnot commented Feb 21, 2020

@royfielding do you remember why you removed that text?

@royfielding
Copy link
Member

Yes, the requirement moved into the general section on all URI-references. That is not a special case. And, no, its presence or absence would have made no difference to the referenced bug report.

If either one of them had correctly followed the spec, they would have been fine. Instead, npm sent a special value without a special scheme, and Cloudflare marked that for rate limiting based on incorrect assumptions of bad behavior rather than the actual specification. Excuses happen.

@mnot
Copy link
Member Author

mnot commented Mar 20, 2020

@royfielding - I wasn't talking about the actual bug; I was talking about @isaacs -- someone who has implemented a very good HTTP engine -- misinterpreting the spec in that comment because it was reader-unfriendly. That should concern us all, because if he and implementers like him misread the spec so easily, it will be mis-implemented. And, the fault here is definitely not with him; it's with the spec.

It's not reasonable to expect people to hold the entire specification series in their heads as they interpret each phrase and requirement, especially considering that in most renderings it's not cross-referenced (and even where the renderings support it, we don't cross-reference consistently).

I understand that we need to be terse sometimes, and that it's reasonable to have expectations of our readers. But, when we use arcane terms and overload widely-used terms (even if incorrectly) to the point where the only people who understand the spec on a good day are you, Julian and I (and on a bad day, just you), it doesn't do anyone any good.

@mnot mnot changed the title Referer and relative URIs Being explicit about relative URIs Mar 20, 2020
@mnot
Copy link
Member Author

mnot commented Mar 20, 2020

I think there are two ways to resolve this:

  1. Link back to the URI section in each case where it's used, and adjust language to make it clear that there are requirements there
  2. Add language in each case where a URI is a protocol element, and state how it's resolved

I think I prefer (2), because 2.4. Uniform Resource Identifiers is vague about how it specifies this:

Unless otherwise indicated, URI references are parsed relative to the effective request URI (Section 5.5).

Since Section 2 is about HTTP URIs in general -- not just as protocol elements -- it's not clear what the scope of this statement is. It's also not a requirement, which doesn't seem good for interoperability.

@mnot mnot self-assigned this Mar 20, 2020
@mnot mnot added the waiting label Mar 23, 2020
@mnot
Copy link
Member Author

mnot commented Mar 23, 2020

Waiting for #259 to land.

@mnot mnot removed the waiting label May 18, 2020
@mnot
Copy link
Member Author

mnot commented May 18, 2020

Places this will affect:

  • Location
  • Content-Location
  • Referer

mnot added a commit that referenced this issue May 20, 2020
Text taken from existing statement in Location.

Fixes #321.
@royfielding
Copy link
Member

I still don't see any reason for this change. The spec already states how to do relative resolution in general, which it must do because it applies to all header fields (not just the ones we define). Saying it again on every occurrence of a relative or partial URI is just adding length to the spec.

@mnot
Copy link
Member Author

mnot commented May 21, 2020

We already said it for Location, and the amount of added text is minimal.

@mnot
Copy link
Member Author

mnot commented May 21, 2020

... and it's also said explicitly in Link.

It may be obvious to you, but it's pretty clearly not to many implementers.

@royfielding
Copy link
Member

The only confusion has been our change in 7231 of the Location field to allow relative references. I am not aware of anyone ever being confused about the target URI being the base URI for all header fields, but I can live with more text if it says that consistently.

To reiterate, this entire issue is based on a misreading of a completely mistaken assumption about the field content of "Referer: install" being flagged as a potential security issue by Cloudflare because it is unusual, not because Cloudflare somehow might not have been aware that the syntax is allowed by the spec as a relative reference. It's defense by spec-lawyering.

These changes would not have changed Cloudflare's algorithm and would not have convinced npm to choose a more common default. Actually talking to their own CDN before deploying an HTTP change would have helped. Testing would have prevented it entirely.

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

Successfully merging a pull request may close this issue.

2 participants