Add workaround to handle URLs of scoped packages with unencoded / #275
Conversation
| @@ -33,6 +33,16 @@ module.exports = function(config, auth, storage) { | |||
| app.use(expressJson5({ strict: false, limit: config.max_body_size || '10mb' })) | |||
| app.use(Middleware.anti_loop(config)) | |||
|
|
|||
| // encode / in a scoped package name to be matched as a single parameter in routes | |||
| // TODO: routes should be modified to properly handle scopes, this is just a workaround | |||
There was a problem hiding this comment.
As far as I understand it, it's a workaround for a nginx bug, and sinopia already handles scopes correctly.
This workaround is still useful to have though.
There was a problem hiding this comment.
It’s not a nginx bug, nginx behaves correctly IMHO. It’s just plain stupid idea to use encoded / in URI when it shouldn’t be represented as a path separator.
There was a problem hiding this comment.
If you have scoped package, it's package descriptor looks like:
{
"name": "@scope/package",
"version": "1.2.3",
}So naturally name of the package is @scope/package and version is 1.2.3 here.
If you properly encode those into URI components (using encodeURIComponent function for example), url would look like this:
> '/' + [ '@scope/package', '1.2.3' ].map(encodeURIComponent).join('/')
'/%40scope%2Fpackage/1.2.3'Note that npm doesn't encode @, for it doesn't cause collisions (and @ looks prettier in logs btw). But encoding of / is actually necessary here.
Nginx decodes it losing information about what was encoded and what wasn't. And it might be useful to prevent path traversal attack like /files/..%2f..%2f..etc%2fpasswd.
But in this case such encoding is intentional. And no matter how stupid this idea might appear, we have standards for God's sake. And RFC 7230 doesn't say "proxy can decode urls all they like".
Now in case of nginx, it is %2f in the input, and it is / in the output. If input is /%2f/, nginx would decode it to /// losing information. It is a bug in nginx, I have no other way of calling it.
There was a problem hiding this comment.
Well, I can’t disagree, you’re right that proxy shouldn’t decode URL.
|
Merged in fde2321 (with added test). Thanks! |
Fixes #104 (comment).