j3k0 / ganomede-avatars

Ganomede user's avatars
0 stars 0 forks source link

Security: Posting avatar exposes internal database URL #15

Closed j3k0 closed 7 years ago

j3k0 commented 7 years ago

Request: POST https://HOST/avatars/v1/auth/AUTH_TOKEN/pictures

Reply (edited):

{"ok":true,"url":"http://INTERNAL_COUCHDB_IP:INTERNAL_COUCHDB_PORT/DATABASE_NAME/DOCUMENT_NAME/original.png"}

I'm pretty sure the url field isn't required in the reply (better not expose internal configuration).

elmigranto commented 7 years ago

Does client use this url, or does it construct one for itself based on userId? In other words, do we need to return https://<production-server>/avatars/v1/elmigranto/original.png here, or just {ok: true} will do (or maybe simple send(200, 'OK') without any JSON?

elmigranto commented 7 years ago

I'll just drop a url for now, let me know if that's not appropriate @j3k0.

j3k0 commented 7 years ago

Response code 200 OK and reply with only {"ok":true}.

elmigranto commented 7 years ago

Alright.

j3k0 commented 7 years ago

While double thinking, I believe this url field was supposed to be the cdn-friendly url (might be on a static domain, see https://github.com/j3k0/ganomede-avatars#configuration → CDN_HOST.

elmigranto commented 7 years ago

Oh, okay, so we change it to <CDN_HOST>/stuff then? And in case that's missing, drop it?

j3k0 commented 7 years ago

Yes, change to something like http://CDN_HOST/avatars/v1/USERNAME/original.png

elmigranto commented 7 years ago

http or https? Or protocol would be include in CDN_HOST?

j3k0 commented 7 years ago

Good question... maybe CDN_HOST should be CDN_URL

j3k0 commented 7 years ago

We're overthinking this: because we don't using it anyway... Let's just drop the CDN thing and url

elmigranto commented 7 years ago

Ha-ha, okay :)

More important problem is underspecified package.json and node 0.10. Nothing works, some modules use const which throws outside strict mode, and if we run in strict (--use_strict), some modules throw due to func definition. And image lib won't install on newer versions, I believe. I will try more stuff tomorrow, but you better put our current node_modules into tarball or something…

elmigranto commented 7 years ago

Also, could you maybe dump npm ls output from production so we can pin those versions in package.json?

j3k0 commented 7 years ago

The production install is a docker image (contains all deps). So all is archived if needed: image is pullable with docker pull ganomede/avatars

elmigranto commented 7 years ago

So all is archived if needed

IDK if that runs npm install as part of pull? Or is registry thingy a result of docker build with all the commands but RUN already executed?

j3k0 commented 7 years ago

The idea of a docker image is to package a full OS (excluding just the kernel) with all required dependencies to run a specific service... This way we guarantee that dev, test and production use the complete same environment... A well designed docker image doesn't require to download any stuff to launch the service, it's like a disk image designed to run a single software.

So, yes npm install is run as part of the image build. The image in the registry is the result of all commands (including RUN), only excluding the ENTRYPOINT and CMD, which are the command to actually launch a service.

You can check this out with docker run --rm -it ganomede/avatars bash. You'll see that node_modules are here. npm ls will dump the versions used in production.

elmigranto commented 7 years ago

Thanks for explanation. Wasn’t sure what is hosted at hub in the result of “build”.

Yeah, meant a command for starting an app, RUN pop up into brain, and if I bothered to think about it more, kinda obvious that’s the wrong thing, welp :)