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.

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

Add workaround to handle URLs of scoped packages with unencoded / #275

Closed
wants to merge 1 commit into from
Closed

Add workaround to handle URLs of scoped packages with unencoded / #275

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions lib/index-api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I can’t disagree, you’re right that proxy shouldn’t decode URL.

app.use(function(req, res, next) {
if (req.url.indexOf('@') != -1) {
// e.g.: /@org/pkg/1.2.3 -> /@org%2Fpkg/1.2.3, /@org%2Fpkg/1.2.3 -> /@org%2Fpkg/1.2.3
req.url = req.url.replace(/^(\/@[^\/%]+)\/(?!$)/, '$1%2F')
}
next()
})

// for "npm whoami"
app.get('/whoami', function(req, res, next) {
if (req.headers.referer === 'whoami') {
Expand Down