-
Notifications
You must be signed in to change notification settings - Fork 43
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
Comments
@royfielding do you remember why you removed that text? |
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. |
@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. |
I think there are two ways to resolve this:
I think I prefer (2), because 2.4. Uniform Resource Identifiers is vague about how it specifies this:
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. |
Waiting for #259 to land. |
Places this will affect:
|
Text taken from existing statement in Location. Fixes #321.
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. |
We already said it for Location, and the amount of added text is minimal. |
... and it's also said explicitly in Link. It may be obvious to you, but it's pretty clearly not to many implementers. |
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. |
2616's definition of
Referer
said:7231 dropped that, thanks to this commit.
We should probably restore it, given it can cause misconceptions like this.
The text was updated successfully, but these errors were encountered: